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

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2013, 23:24 UTC
Message-ID: <7vr4gqwuuw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130528225453.GA9820@ecki>

Clemens Buchacher <drizzd@aon.at> writes:

Show 20 quoted lines

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