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

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

"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.