Subject: Re: [PATCH 3/3] diff: detect pathspec magic not supported by --follow

From: Junio C Hamano <[email-masked]>
Date: Fri, 02 Jun 2023 16:27:05 +0900

Jeff King <[email-masked]> writes:

> The --follow code doesn't handle most forms of pathspec magic. We check
> that no unexpected ones have made it to try_to_follow_renames() with a
> runtime GUARD_PATHSPEC() check, which gives behavior like this:
>
>   $ git log --follow ':(icase)makefile' >/dev/null
>   BUG: tree-diff.c:596: unsupported magic 10
>   Aborted

Stepping back a bit, isn't the real reason why follow mode is
incompatible with the pathspec magic because it does not want to
deal with anything but a single literal pathname?

In other words, in "git log --follow ':(icase)makefile'", if the
given pathspec matches a single path in the starting commit, we
should be able to start from the commit inspecting that uniquely
found path, compare the commit with its parent(s) at the path, and
when the parent does not have the path, detect rename and continue
following that the name before the rename as the single path
starting from that parent.  Perhaps the commit we start at, HEAD,
has "Makefile" and not "makefile" (if we have two, that is an
error), so we start traversal, following the pathname "Makefile".
At some point, we may discover that the parent did not have
"Makefile", but has "GNUMakefile" with a similar contents, at which
point we swap the path we follow from "Makefile" to "GNUMakefile".

At each step, tree-diff does not need to and does not want to do
anything fancy with pathspec---the operation is always "this path in
the parent, does it exist in the parent? If not, did it come from a
different path?", and use of "literal pathspec" mode makes sense
there.

So why does try_to_follow_renames() even *need* to say anything
other than "We are in follow mode; what we have is a single path;
use it as a literal pathspec that matches the path literally without
any funky magic"?  GUARD_PATHSPEC() is "please inspect the pathspec
and see what kind of special pattern matching it requests---we
cannot allow it to ask for certain things but do allow it to ask for
other things", that sounds terribly misguided here.  The string is
literal, so we do not even want to look at it to see what it
asks---it is taken as nothing but a literal string and we do not let
it ask for anything special.

For example, if we started at HEAD to follow Makefile and in an
ancestor commit we find that the Makefile came from a funny path
":(icase)foo", instead of allowing the pathspec match code to
interprete it as a magic pathspec that matches any case permutations
of 'f/o/o', we want to force the pathspec match code to match
nothing but the funny path "colon parens icase close parens foo" by
using literal pathspec magic.

So perhaps having GUARD_PATHSPEC() there is the mistake, no?

For the above idea to work, I think we need to resolve the pathspec
early; "log --follow" should find its starting point, apply the
given pathspec to its treeish to grab the literal pathname, and use
that single literal pathname as the literal pathspec and continue,
which I do not think it does right now.  But with it done upfront,
the pathspec limiting done during the history traversal, including
try_to_follow_renames() bit, should be able to limit itself to "is
the path literally the string given as the pathspec"?

I dunno.