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

[Junio C Hamano <gitster@pobox.com>]

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);