Re: git show doesn't work on file names with square brackets

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2016, 20:37 UTC
Message-ID: <xmqqziv939ir.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160208195230.GA30693@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 43 quoted lines

> Subject: [PATCH] check_filename: tighten requirements for dwim-wildcards
>
> Commit 28fcc0b (pathspec: avoid the need of "--" when
> wildcard is used, 2015-05-02) introduced a convenience to
> our dwim-parsing: when "--" is not present, we guess that
> items with wildcard characters are probably pathspecs.
>
> This makes a lot of cases simpler (e.g., "git log '*.c'"),
> but makes others harder. While revision expressions do not
> typically have wildcard characters in them (because they are
> not valid in refnames), there are a few constructs where we
> take more arbitrary strings, such as:
>
>   - pathnames in tree:path syntax (or :0:path) for the
>     index)
>
>   - :/foo and ^{/foo} for searching commit messages;
>     likewise "^{}" is extensible and may learn new formats
>     in the future
>
>   - @{foo}, which can take arbitrary approxidate text (which
>     is not itself that likely to have wildcards, but @{} is
>     also a potential generic extension mechanism).
>
> When we see these constructs, they are almost certainly an
> attempt at a revision, and not a pathspec; we should not
> give them the magic "wildcard characters mean a pathspec"
> treatment.
>
> We can afford to be fairly slack in our parsing here. We are
> not making a real decision on "this is or is not definitely
> a revision" here, but rather just deciding whether or not
> the extra "wildcards mean pathspecs" magic kicks in.
>
> Note that we drop the tests in t2019 in favor of a more
> complete set in t6133. t2019 was not the right place for
> them (it's about refname ambiguity, not dwim parsing
> ambiguity), and the second test explicitly checked for the
> opposite result of the case we are fixing here (which didn't
> really make any sense; as show by the test_must_fail in the
> test, it would only serve to annoy people).
>
> Signed-off-by: Jeff King <peff@peff.net>

I was leaning towards merging this version, but I became unsure while writing an entry for "What's cooking" (which will be used as a merge summary message and then will appear in the Release Notes).

We would surely want

    $ git log ':/tighten.*'

to find this commit, not take it as a pathspec.  But running

    $ git log ':/*.c'

in a subdirectory to find commits that touched any .c file, taking it as a pathspec, would equally be a sensible thing to want.

I would feel that we should require "--" for both cases; with or without this patch, we already treat these as revs without "--", making the latter fail without "--". Also:

    $ git log "HEAD^{/tighten.*}"

is already dwimmed as a rev.

And a path with glob(3) metacharacters is an insane thing, be it inside a treeish or in the working tree, and I think it is OK to require users to explicitly say what they mean with "--".

And the patch does not leave much if we ignore that ":" bit. With the patch, "HEAD@{now [or thereabouts]}" will be taken as a rev without "--", which is an improvement, but to me that seems to be the only improvement this change brings us.

And I do not think we want either glob(3) or fill_directory() to slow things down, as this is merely a heuristic.

We may want to rethink the interface into check_filename(). The callers of this function that try to help users who did not use "--" want the function to say "It is likely that this was meant as a pathname" and when this function says "No, the user did not mean it as a filename." they will in turn ask the revision parser "Is this a rev?". At that point, if it is not a revision, these callers can say "Not a file, not a rev" and die.

In order to allow "':/tighten.*' is a rev, ':/*.c' is a pathspec, they are equally likely and you must disambiguate", the current interface is inadequate.

 * check_filename() cannot say "No, it is not a filename"--a later
   call to get_sha1() will barf on ":/*.c" saying that it is not a
   rev, but the fault is in Git that initially guessed it would be a
   rev when the user meant it as a pathspec.

 * The function cannot say "Yes, it is a filename"--then get_sha1()
   will not be called for ":/tighten.*" and we would silently use it
   as pathspec, possibly producing an empty result.

There needs to be a way for it to say "I refuse to disambiguate".

I actually think that no_wildcard() check added in check_filename() was the original mistake. If we revert the check_filename() to a simple "Is this a filename?" and move the "does this thing have a wildcard" aka "can this be a pathspec even when check_filename() says there is no file with that exact name?" to the code that tries to allow users omit "--", i.e. the caller of check_filename(), would that make the code structure and the semantics much cleaner, I wonder...