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.
