Re: git diff -B: splitting up complete renames

Jakub Narebski <jnareb@gmail.com> writes:

Correct. The description is based on an earlier round of the code before 366175ef8c3b1e145f4ba846e63a1dea3ec3cacc (Rework -B output). "git show" of that patch has description in its changes to Documentation/diffcore.txt.

The original motivation of -B was not about helping three way renames it allows you when used with -M/-C. It was about showing textual diff more readably for quite dissimilar filepairs.

It doesn't, for two reasons.

 * Look at diffcore_break().  It breaks filepairs with identical
   paths and nothing else.

   The reason for this check is that the caller can call
   diffcore_rename() _before_ diffcore_break() [*1*] and a
   filepair that was detected as renamed pair ought to be
   already found similar enough.  These days, I think you can
   replace the check with p->renamed_pair.

 * Look at diffcore_merge_broken(), which is responsible for
   re-merging a broken pair.  Because we match them by looking
   at their names (iow, we do not use pointers to link halves of
   a broken pair pointing at each other).

   This was correct before "no-index" stuff because "git diff"
   was about comparing pairs of blobs found at corresponding
   paths in two tree-like things (i.e. "a/one" and "b/one"
   corresponds to each other when you do "git diff -- one").
   The modification to introduce no-index forgot to update this
   logic.

If you wanted to fix this, you can change p->broken_pair from a boolean to a pointer that points at the other half of the broken pair (and record that when we break a pair in diffcore_break()), and look at that pointer to decide which one to match up with which other one in diffcore_merge_broken(). Together with a change to look at p->renamed_pair instead of paths I mentioned, I think it would work more like the regular git diff for "no-index" case.

I consider that "no-index" frontend more or less a bolted on half-baked hack that covers only minimally necessary cases to serve as non-git "diff" replacement, without sharing enough with the real "git diff" internals; I would not be surprised at all if there are more corner cases like this that does not work. But I do not think it is fundamentally unfixable. The change to add "no-index" support just needed to be more careful, and it is not too late to make it so.

[Footnote]

*1* diffcore was designed as a generic library to allow experimenting more combinations of transformations than what then-current git used. All of our existing callers ended up calling the transformations in the same order (i.e. the order diffcore_std() calls them), but individual transformations were written not to assume too much about the order they would be called.