[BUG] 'diff A...B' fails with multiple merge bases

[BUG] 'diff A...B' fails with multiple merge bases

[Pickens, James E]

Hi,

The command 'git diff A...B' is supposed to be equivalent to 'git diff $(git
merge-base A B) B'.  But when there are multiple merge bases between A and B,
the former gives no output.  Here's a recipe to reproduce the problem:

    git init
    git commit --allow-empty -m 1
    git checkout -b A
    touch file1
    git add file1
    git commit -m A
    git checkout master
    touch file2
    git add file2
    git commit -m B
    git checkout -b B
    git merge A
    git checkout A
    git merge master
    git diff A...B
    git diff $(git merge-base A B) B

The diff commands at the end will give different results.  It bisects to:

commit b75271d93a9e4be960d53fc4f955802530e0e733
Author: Matt McCutchen <matt@mattmccutchen.net>
Date:   Fri Oct 10 21:56:15 2008 -0400

    "git diff <tree>{3,}": do not reverse order of arguments

    According to the message of commit 0fe7c1de16f71312e6adac4b85bddf0d62a47168,
    "git diff" with three or more trees expects the merged tree first followed by
    the parents, in order.  However, this command reversed the order of its
    arguments, resulting in confusing diffs.  A comment /* Again, the revs are all
    reverse */ suggested there was a reason for this, but I can't figure out the
    reason, so I removed the reversal of the arguments.  Test case included.


I verified that if I revert that commit, 'diff A...B' works as expected, but
test t4013-diff-various.sh fails.  The failing command is 'git diff master
master^ side'.  I don't understand what that command is supposed to do, so I
didn't go any further.

Am I right that this is a bug, and if so can someone help to address it?

James

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Junio C Hamano]

"Pickens, James E" <james.e.pickens@intel.com> writes:

> The command 'git diff A...B' is supposed to be equivalent to 'git diff $(git
> merge-base A B) B'.  But when there are multiple merge bases between A and B,
> the former gives no output...
> ...
> The diff commands at the end will give different results.  It bisects to:
>
> commit b75271d93a9e4be960d53fc4f955802530e0e733
> Author: Matt McCutchen <matt@mattmccutchen.net>
> Date:   Fri Oct 10 21:56:15 2008 -0400

Thanks for a report, and for bisecting.

This unfortunately is somewhat an expected fallout from Matt's patch.

The low-level diff dispatcher in cmd_diff() function, where "ents" are
tree-ish given from the command line (either using 'diff A', 'diff A B',
'diff A..B', 'diff A...B' or 'diff A B C D E ...', syntaxes), says this:

	...
	else if ((ents == 3) && (ent[0].item->flags & UNINTERESTING)) {
		/* diff A...B where there is one sane merge base between
		 * A and B.  We have ent[0] == merge-base, ent[1] == A,
		 * and ent[2] == B.  Show diff between the base and B.
		 */
		ent[1] = ent[2];
		result = builtin_diff_tree(&rev, argc, argv, ent);
	}
	else
		result = builtin_diff_combined(&rev, argc, argv,
					     ent, ents);

I omitted the 1 or 2 trees case where we do naturally diff-index or
diff-tree from the above.

When the user gives more than two trees (e.g. "diff A B C"), we show a
combined diff that explains a merge of B and C that produces A, which was
introduced by 0fe7c1d (built-in diff: assorted updates., 2006-04-29).
Remember that the first tree is the merge result, and the user is asking
us to explain that result relative to its parents.

The special case with three trees, among which the first one being
uninteresting, came much later.  The revision parser parses A...B into a
list of "--not $(merge-bases A B)", A (SYMMETRIC_LEFT), and then B.  As
the purpose of "diff A...B" is to show what you did up to B since you
forked from A, showing the tree diff between the ent[0] (merge base) and
ent[2] (B) is the right thing to do.  But the codepath is of course
prepared to about dealing with a single-base merges, so your criss-cross
merge case does not trigger this special case.

So we fall into the "combined diff" case, which does this:

    git diff $(git merge-base --all A B) A B

As defined by 0fe7c1de, this should output the combined diff to explain as
if one of the merge bases (that happens to be the first one in merge-base
output) were the merge result of all the other merge bases, the refs A and
B you gave from the command line.  Which does not make _any_ sense, as it
is picking one of the criss-cross merge bases at random and forcing the
history to flow backwards.

Before Matt's patch, I think diff_combined() was giving a _slightly_ more
reasonable result because it (incorrectly) reversed the arguments to
explain as if B (typically yours) is the merge across all the merge bases
and the other tip A.  I say that is a "slightly" more reasonable, only
because what is explained is what you are familiar with, i.e. B.  I don't
think the way it explains it as a pseudo merge across all the merge bases
and the other tip makes any sense.

It should not be too hard to add logic to reverse the list of revisions as
another special case in the above else-if chain to support the old output
you saw before Matt's fix if such an output were useful.  You would detect
if the list begins with a run of UNINTERESTING ones followed by two
interesting ones (because that is how A...B parser gives its output to
us), and in that case feed diff_combined with a reversed list.

But I do not see how such an pseudo-merge output is useful, so please
enlighten me with an illustration.  Your "earlier it showed something, now
it doesn't show anything" is not good enough here, as I am doubting that
something we used to show in a criss-cross merge case was a useful output.

Thanks.

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Pickens, James E" <james.e.pickens@intel.com> writes:
>
>> The command 'git diff A...B' is supposed to be equivalent to 'git diff $(git
>> merge-base A B) B'.  But when there are multiple merge bases between A and B,
>> the former gives no output...
>> ...
> It should not be too hard to add logic to reverse the list of revisions as
> another special case in the above else-if chain to support the old output
> you saw before Matt's fix if such an output were useful.  You would detect
> if the list begins with a run of UNINTERESTING ones followed by two
> interesting ones (because that is how A...B parser gives its output to
> us), and in that case feed diff_combined with a reversed list.
>
> But I do not see how such an pseudo-merge output is useful, so please
> enlighten me with an illustration.  Your "earlier it showed something, now
> it doesn't show anything" is not good enough here, as I am doubting that
> something we used to show in a criss-cross merge case was a useful output.

I think I half-misread what you wanted to do.  "git diff A...B" is not
equivalent to "git diff $(git merge-base A B) B" but is supposed to be
equivalent to "git diff $(git merge-base --all A B) B".

I prepared a patch to reject such a request when there are more than one
merge base (see below---it is against 1.6.4 maintenance track).  While I
think giving _one_ possible explanation of what you did since you forked
would be better than rejecting, which I'll try in a separate message, but
at the same time it may be misleading to give such an output without
telling the user that we chose one merge base at random to diff against
it.

 builtin-diff.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 2e51f40..f65bc99 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -405,10 +405,28 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		result = builtin_diff_index(&rev, argc, argv);
 	else if (ents == 2)
 		result = builtin_diff_tree(&rev, argc, argv, ent);
-	else if ((ents == 3) && (ent[0].item->flags & UNINTERESTING)) {
-		/* diff A...B where there is one sane merge base between
-		 * A and B.  We have ent[0] == merge-base, ent[1] == A,
-		 * and ent[2] == B.  Show diff between the base and B.
+	else if (ent[0].item->flags & UNINTERESTING) {
+		/*
+		 * Perhaps the user gave us A...B, which expands
+		 * to a list of negative merge bases followed by
+		 * A (symmetric-left) and B?  Let's make sure...
+		 */
+		for (i = 1; i < ents; i++)
+			if (!(ent[i].item->flags & UNINTERESTING))
+				break;
+		if (ents != i + 2 ||
+		    (ent[i+1].item->flags & UNINTERESTING) ||
+		    (!ent[i].item->flags & SYMMETRIC_LEFT) ||
+		    (ent[i+1].item->flags & SYMMETRIC_LEFT))
+			die("what do you mean by that?");
+		if (ents != 3)
+			die("There are more than one merge bases in %s",
+			    ent[ents-2].name);
+		/*
+		 * diff A...B where there is one sane merge base
+		 * between A and B.  We have ent[0] == merge-base,
+		 * ent[1] == A, and ent[2] == B.  Show diff between
+		 * the base and B.
 		 */
 		ent[1] = ent[2];
 		result = builtin_diff_tree(&rev, argc, argv, ent);

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I prepared a patch to reject such a request when there are more than one
> merge base (see below---it is against 1.6.4 maintenance track).  While I
> think giving _one_ possible explanation of what you did since you forked
> would be better than rejecting, which I'll try in a separate message, but
> at the same time it may be misleading to give such an output without
> telling the user that we chose one merge base at random to diff against
> it.

And this is the other one (not relative to the previous patch) that shows
diff since one randomly chosen merge base.

 builtin-diff.c |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 2e51f40..1f44f5b 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -405,12 +405,28 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		result = builtin_diff_index(&rev, argc, argv);
 	else if (ents == 2)
 		result = builtin_diff_tree(&rev, argc, argv, ent);
-	else if ((ents == 3) && (ent[0].item->flags & UNINTERESTING)) {
-		/* diff A...B where there is one sane merge base between
-		 * A and B.  We have ent[0] == merge-base, ent[1] == A,
-		 * and ent[2] == B.  Show diff between the base and B.
+	else if (ent[0].item->flags & UNINTERESTING) {
+		/*
+		 * Perhaps the user gave us A...B, which expands
+		 * to a list of negative merge bases followed by
+		 * A (symmetric-left) and B?  Let's make sure...
 		 */
-		ent[1] = ent[2];
+		for (i = 1; i < ents; i++)
+			if (!(ent[i].item->flags & UNINTERESTING))
+				break;
+		if (ents != i + 2 ||
+		    (ent[i+1].item->flags & UNINTERESTING) ||
+		    (!ent[i].item->flags & SYMMETRIC_LEFT) ||
+		    (ent[i+1].item->flags & SYMMETRIC_LEFT))
+			die("what do you mean by that?");
+		/*
+		 * diff A...B where there is at least one merge base
+		 * between A and B.  We have ent[0] == merge-base,
+		 * ent[ents-2] == A, and ent[ents-1] == B.  Show diff
+		 * between the base and B.  Note that we pick one
+		 * merge base at random if there are more than one.
+		 */
+		ent[1] = ent[ents-1];
 		result = builtin_diff_tree(&rev, argc, argv, ent);
 	}
 	else

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Sverre Rabbelier]

Heya,

On Mon, Jul 12, 2010 at 19:25, Junio C Hamano <gitster@pobox.com> wrote:
> +               if (ents != i + 2 ||
> +                   (ent[i+1].item->flags & UNINTERESTING) ||
> +                   (!ent[i].item->flags & SYMMETRIC_LEFT) ||
> +                   (ent[i+1].item->flags & SYMMETRIC_LEFT))
> +                       die("what do you mean by that?");

That's about as helpful an error message as "Your parents must hate
you" or whatever it is. Can we rephrase it to tell the user what they
did wrong, and how to fix it?

-- 
Cheers,

Sverre Rabbelier

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Mon, Jul 12, 2010 at 19:25, Junio C Hamano <gitster@pobox.com> wrote:
>> +               if (ents != i + 2 ||
>> +                   (ent[i+1].item->flags & UNINTERESTING) ||
>> +                   (!ent[i].item->flags & SYMMETRIC_LEFT) ||
>> +                   (ent[i+1].item->flags & SYMMETRIC_LEFT))
>> +                       die("what do you mean by that?");
>
> That's about as helpful an error message as "Your parents must hate
> you" or whatever it is. Can we rephrase it to tell the user what they
> did wrong, and how to fix it?

This will trigger only when you did something random like this.

    $ git diff ^maint master ^next ^pu sr/frotz..

What else would you say against such an input?

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Sverre Rabbelier]

Heya,

On Mon, Jul 12, 2010 at 19:45, Junio C Hamano <gitster@pobox.com> wrote:
> This will trigger only when you did something random like this.
>
>    $ git diff ^maint master ^next ^pu sr/frotz..
>
> What else would you say against such an input?

Something like "too many arguments, please specify at most two commits
to diff against"?

-- 
Cheers,

Sverre Rabbelier

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Mon, Jul 12, 2010 at 19:45, Junio C Hamano <gitster@pobox.com> wrote:
>> This will trigger only when you did something random like this.
>>
>>    $ git diff ^maint master ^next ^pu sr/frotz..
>>
>> What else would you say against such an input?
>
> Something like "too many arguments, please specify at most two commits
> to diff against"?

The first part is correct, but the advice is not quite, as you are
forbidding the combined diff to emulate "git show --cc pu" to view a
merge at the tip of "pu" with

    git diff --cc pu pu~1 pu~2

which obviously has to allow three commits.

Re: [BUG] 'diff A...B' fails with multiple merge bases

[Sverre Rabbelier]

Heya,

On Mon, Jul 12, 2010 at 19:55, Junio C Hamano <gitster@pobox.com> wrote:
> which obviously has to allow three commits.

Well, then say that. "too many arguments for a regular diff, please
specify at most two commits to diff against". Anyone trying to use
diff to show a merge diff likely knows to look for the --cc switch;
either way, it'd be better than "what do you mean?".

-- 
Cheers,

Sverre Rabbelier

RE: [BUG] 'diff A...B' fails with multiple merge bases

[Pickens, James E]

Junio C Hamano <gitster@pobox.com> writes:
>> I prepared a patch to reject such a request when there are more than one
>> merge base (see below---it is against 1.6.4 maintenance track).  While I
>> think giving _one_ possible explanation of what you did since you forked
>> would be better than rejecting, which I'll try in a separate message, but
>> at the same time it may be misleading to give such an output without
>> telling the user that we chose one merge base at random to diff against
>> it.
>
>And this is the other one (not relative to the previous patch) that shows
>diff since one randomly chosen merge base.

Thanks for the detailed explanation and patches!

Personally I like this behavior better than erroring out when there are multiple
merge bases, even though the result is unpredictable.  We were using 'diff
A...B' in a script that users run to submit their changes to a continuous
integration server, to check whether they had any actual changes to submit.  In
that context, we don't care whether the output makes sense, we only care whether
there is any output.

The script has been changed to avoid using the A...B syntax now, so it's not a
big deal if you prefer to just error out in this situation.  But FWIW, this
patch also has the advantage that it makes the code match the existing
documentation... assuming that the randomly chosen merge base is the same one
that 'git merge-base' would print.

James