Clemens Buchacher <drizzd@xxxxxx> writes:
>> 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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html