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

Junio C Hamano [off-list ref] writes:

"Pickens, James E" [off-list ref] writes:

quoted

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