From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jim Pryor <dubiousjim@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 3/3] diff: detect pathspec magic not supported by --follow
Date: Thu, 15 Jun 2023 03:26:25 -0400 [thread overview]
Message-ID: <20230615072625.GD1460739@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq4jnq9vee.fsf@gitster.g>
On Fri, Jun 02, 2023 at 04:27:05PM +0900, Junio C Hamano wrote:
> 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:
> 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
next prev parent reply other threads:[~2023-06-15 7:26 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-27 13:53 git 2.40.1 tree-diff crashes with (glob) magic on pathspecs Jim Pryor
2023-05-27 17:39 ` Jeff King
2023-05-27 19:00 ` Jim Pryor
2023-06-01 17:37 ` [PATCH 0/3] handling pathspec magic with --follow Jeff King
2023-06-01 17:38 ` [PATCH 1/3] pathspec: factor out magic-to-name function Jeff King
2023-06-01 17:41 ` [PATCH 2/3] diff: factor out --follow pathspec check Jeff King
2023-06-01 17:43 ` [PATCH 3/3] diff: detect pathspec magic not supported by --follow Jeff King
2023-06-02 7:27 ` Junio C Hamano
2023-06-15 7:26 ` Jeff King [this message]
2023-06-15 19:00 ` Junio C Hamano
2023-06-01 5:38 ` git 2.40.1 tree-diff crashes with (glob) magic on pathspecs Junio C Hamano
2023-06-01 13:30 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230615072625.GD1460739@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=dubiousjim@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).