Jeff King <peff@xxxxxxxx> writes:
> 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@xxxxxxxx>
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...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html