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

From: Jeff King <peff@peff.net>
Date: Jun 15, 2023, 07:26 UTC
Message-ID: <20230615072625.GD1460739@coredump.intra.peff.net>
In-Reply-To: <xmqq4jnq9vee.fsf@gitster.g>

On Fri, Jun 02, 2023 at 04:27:05PM +0900, Junio C Hamano wrote:

Show 13 quoted lines

> Jeff King <peff@peff.net> 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?

Sorry, I'm a little behind on list reading; I know this topic has advanced almost to 'master' in the meantime, but I had a few thoughts worth getting out.

Basically, yes, I agree with everything in your email. My series is more or less papering over deficiencies in the --follow code, and that code would be much better off thinking of single literal paths that it is following.

I was trying with my series not to go down the rabbit hole of --follow deficiencies. So in that sense, I think it is still a good idea for the short-term. And though we might perhaps revert some of it if we actually improve how --follow works, I don't think it would be too hard to do so later. But of course if we are going to improve --follow _now_, then it could replace my series.

And what you outlined here:

Show 8 quoted lines

> 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"?

seems mostly sensible to me, except that the notion of "starting point" is ambiguous. If I do:

  git log --follow branch1 branch2 -- foo*

then we have two treeishes. Do we look at one? Both? What happens if the pathspec resolves differently?

Off the top of my head, I'd say that we should look at all starting tips, resolve the pathspec in all of them, and complain if it doesn't resolve to the same single path in each one. Because it otherwise would produce garbled results.

To be fair, this same garbled case already happens when traversing a merge sends us down two paths of history, as a followed rename might be present on one but not the other, and we keep only a single path. But as you know, that is a long-time deficiency of --follow. :) At least detecting it from the starting tips is easy and something the user can deal with by modifying the command invocation.

So I dunno. That doesn't sound _too_ hard to do, though it probably also needs a lot of the same infrastructure I introduced in my series. Namely that log.follow needs to ask "hey, is this pathspec usable for following?" so that it can quietly turn the feature off, rather than triggering an error.

So I'd suggest to let my series continue graduating, and then if anybody wants to work on more sensible pathspec handling, to do so on top.

-Peff