From: "Jim Pryor" <dubiousjim@gmail.com>
To: "Jeff King" <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs
Date: Sat, 27 May 2023 21:00:27 +0200 [thread overview]
Message-ID: <51012e79-c6ce-41dd-9755-960e065214ef@app.fastmail.com> (raw)
In-Reply-To: <20230527173903.GB1460206@coredump.intra.peff.net>
On Sat, May 27, 2023, at 7:39 PM, Jeff King wrote:
> On Sat, May 27, 2023 at 03:53:31PM +0200, Jim Pryor wrote:
>
>> $ git --no-pager log --oneline -- ':(glob)**/test2'
>>
>> What did you expect to happen? (Expected behavior)
>>
>> 13a1b11 (HEAD -> master) third commit
>> 2bacc0c second commit
>> c6eb1c1 first commit
>>
>> What happened instead? (Actual behavior)
>>
>> 13a1b11 (HEAD -> master) third commit
>> BUG: tree-diff.c:596: unsupported magic 8
>> error: git died of signal 6
>
> Thanks for your report. I had trouble reproducing at first, but there is
> one extra twist. By default, the command you gave produces:
>
> $ git --no-pager log --oneline -- ':(glob)**/test2'
> 48eab09 (HEAD -> main) third commit
> 89a47e5 second commit
>
> which makes sense; we did not ask about "test", which is the path the
> original commit touched. So I guessed you might have log.follow set, and
> indeed:
>
> $ git --no-pager log --oneline --follow -- ':(glob)**/test2'
> 48eab09 (HEAD -> main) third commit
> BUG: tree-diff.c:596: unsupported magic 8
> Aborted
>
> I _think_ in this case that ":(glob)" is not actually doing much. It is
> already the default that we will glob pathspecs; it is only needed to
> override "git --literal" (which changes the default to not glob).
>
> But this also seems to be wading into a spot that is not actually
> supported by --follow. The code near where the BUG() is triggered looks
> like this:
>
> static void try_to_follow_renames([...])
> {
> [...]
> /*
> * follow-rename code is very specific, we need exactly one
> * path. Magic that matches more than one path is not
> * supported.
> */
> GUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);
> #if 0
> /*
> * We should reject wildcards as well. Unfortunately we
> * haven't got a reliable way to detect that 'foo\*bar' in
> * fact has no wildcards. nowildcard_len is merely a hint for
> * optimization. Let it slip for now until wildmatch is taught
> * about dry-run mode and returns wildcard info.
> */
> if (opt->pathspec.has_wildcard)
> BUG("wildcards are not supported");
> #endif
>
> So follow-mode does not actually work with wildcards, but we err on the
> side of not rejecting them outright. In that sense, the simplest "fix"
> here would be to allow :(glob) to match the '#if 0' section, like:
>
> diff --git a/tree-diff.c b/tree-diff.c
> index 69031d7cba..d287079253 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -593,7 +593,8 @@ static void try_to_follow_renames(const struct
> object_id *old_oid,
> * path. Magic that matches more than one path is not
> * supported.
> */
> - GUARD_PATHSPEC(&opt->pathspec, PATHSPEC_FROMTOP | PATHSPEC_LITERAL);
> + GUARD_PATHSPEC(&opt->pathspec,
> + PATHSPEC_FROMTOP | PATHSPEC_LITERAL | PATHSPEC_GLOB);
> #if 0
> /*
> * We should reject wildcards as well. Unfortunately we
>
> That would avoid the BUG() and make things work consistently. But it
> still would not produce the output you expect, because --follow simply
> does not work with wildcard pathspecs. And fixing that would presumably
> be a much bigger change (I didn't dig into it).
>
> Complicating this slightly is the fact that you didn't explicitly ask
> for --follow, but just had log.follow set. I think we automatically
> disable log.follow when there is more than one pathspec, and in theory
> we ought to do the same for actual wildcards. But maybe it would be
> enough for follow-mode to kick in, but to end up mostly ignored (which
> is what the patch above would do).
>
> (I say "mostly" because I suspect you could construct a history where
> the glob matched a literal filename, too, and the follow code gets
> confused. But outside of intentionally-constructed pathological cases,
> that seems unlikely).
>
> -Peff
Confirmed I have log.follow = true, and also GIT_NOGLOB_PATHSPECS set, thus the explicit `(glob)` magic.
I'd be happy with --follow not working with the wildcard pathspecs, but with the crash avoided.
--
dubiousjim@gmail.com
next prev parent reply other threads:[~2023-05-27 19:01 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 [this message]
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
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=51012e79-c6ce-41dd-9755-960e065214ef@app.fastmail.com \
--to=dubiousjim@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/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).