From: Jeff King <peff@peff.net>
To: Jim Pryor <dubiousjim@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs
Date: Sat, 27 May 2023 13:39:03 -0400 [thread overview]
Message-ID: <20230527173903.GB1460206@coredump.intra.peff.net> (raw)
In-Reply-To: <c2b73d05-1214-4e30-bab6-14e0b8d69773@app.fastmail.com>
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
next prev parent reply other threads:[~2023-05-27 17:39 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 [this message]
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
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=20230527173903.GB1460206@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=dubiousjim@gmail.com \
--cc=git@vger.kernel.org \
/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).