Re: [PATCH] fix segfault with git log -c --follow

Clemens Buchacher [off-list ref] writes:

quoted

I wonder, just like we force recursive and disable external on the
copy before we use it to call diff_tree_sha1(), if we should disable
follow-renames on it.  "--follow" is an option that is given to the
history traversal part and it should not play any role in getting the
pairwise diff with all parents diff_tree_combined() does.

Can't parse that last sentence.

In any case, I don't think disabling diff_tree_sha1 is a solution. The
bug is in diff_tree_sha1 and its subfunctions, because they manipulate a
data structures such that it becomes corrupt. And they do so in an
obfuscated and clearly unintentional manner. So we should not blame the
user for calling diff_tree_sha1 in such a way that it causes corruption.

quoted

Besides,

 - "--follow" hack lets us keep track of only one path; and

Ok. Good to know it is considered a hack. The code is quite strange
indeed.

The problem with --follow is that it only tracks one path globally.
In a history like this, suppose that a path X long time ago was
renamed to Y at commit B:

    ---o---A---B---C---o HEAD

and you start digging with "log --follow -c HEAD -- Y".  When
looking at C, because it and its parent B both have path Y, the
try-to-follow hack does not kick in, and when trying to show C, we
will show the change in Y (because that is the pathspec).

Then we look at B.  Because B has path we are following, i.e. Y, and
its parent A does not, try-to-follow hack kicks in, and it mangles
the pathspec that is used globally for history traversal to X while
showing the difference between A's X and B's Y.  Then we dig further
to find A; at this point the global pathspec is swapped and now it
is X.

That makes --follow a working hack for a simplest single strand of
pearls.  But if you have a mergy history, e.g.

    ---o---A---------------B---C---o HEAD
            \                 /
             D---E---F---G---H

it can break in interesting ways.  We are likely to have looked at H
before looking at B and used pathspec Y while inspecting H, but
after looking at B, the global pathspec is swapped to X, and then we
try to look at G, F, E and D, none of which may have renamed the
original X, so you would likely miss the change to the path Y you
wanted to follow.

To fix this, we would need to keep "what path are we following" not
in the global revs->pathspec, but per the traversal paths that are
currently active (e.g. when we look at C and H, it is Y, when we
look at B, it is X, when we look at G, that is inherited from H and
still Y, not affected by the rename at B.  And then when we look at
A (we need topo-order traversal to do this), it needs to notice that
one child (i.e. B) has been following X while the other (i.e. D) Y,
and merge the "I've been following this path" information in a
sensible way (e.g. look at its own tree and see what is available,
in this case X).