On Sat, May 27, 2023, at 7:39 PM, Jeff King wrote:
Show 99 quoted lines
> 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).
>
> -PeffConfirmed 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.