Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs

From: Jeff King <peff@peff.net>
Date: May 27, 2023, 17:39 UTC
Message-ID: <20230527173903.GB1460206@coredump.intra.peff.net>
In-Reply-To: <c2b73d05-1214-4e30-bab6-14e0b8d69773@app.fastmail.com>

On Sat, May 27, 2023 at 03:53:31PM +0200, Jim Pryor wrote:

Show 13 quoted lines

> $ 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