Jeff King [off-list ref] 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 <redacted>
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...
On Tue, Feb 09, 2016 at 12:37:32PM -0800, Junio C Hamano wrote:
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 "--".
Yes, because ":/" is treated specially in check_filename(), and avoids
kicking in the wildcard behavior. That is certainly preferring revs to
pathspecs, but I think preferring one over the other is preferable to
barfing. If the user wants carefulness, they should use "--"
unconditionally. If they want to DWIM, we should make it as painless as
possible, even if we sometimes guess wrong.
Also:
$ git log "HEAD^{/tighten.*}"
is already dwimmed as a rev.
Is it? That is the exact case I think regressed in v2.5.0, because it
clearly _is_ a rev, didn't previously require a "--", and now does:
$ git.v2.4.8 log --oneline -1 'HEAD^{/tighten.*}'
913c2c7 Merge branch 'jk/sanity' into maint
$ git.v2.5.0 log --oneline -1 'HEAD^{/tighten.*}'
fatal: ambiguous argument 'HEAD^{/tighten.*}': both revision and filename
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
And that's what my patch is trying to do: pull back the over-broad match
in 28fcc0b for cases that are pretty clearly revs.
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 "--".
Yeah, I can buy that line of reasoning.
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.
Hmm. I think at least for the revision-parser it is the other way
around. We actually check first "is it a rev". If it is, and there is no
"--", then we ask "could it also be a filename" and complain if so. If
the revision parser says "no, it cannot be", then we say "well, could it
plausibly be a filename"?
And both of those use the same check_filename() test, but I think they
are asking two different things. The first one probably wants to say "is
it definitely a filename, because if so, that's ambiguous". And it would
be OK to look at a wildcard and say "sure, it _could_ be a path, but
taking it as a rev is reasonable". IOW, to err on the side of "not a
filename", and allow something possibly ambiguous. And then the second
test is the opposite; we know it's not a rev, so if it could plausibly
be a file, then take it as one.
But I have a feeling from what you've written that you do not agree with
the "err and allow something possibly ambiguous" philosophy.
I'll note also two things:
1. I've _just_ looked at revision.c here; there are other callers of
verify_filename and verify_non_filename (and even a bare
check_filename() call!) that may not all have the same needs.
2. The ":/*.c" case is much more complicated. Before we get to
check_filename() at all, we use get_sha1(), which says "yes, this
is a pattern". And then barfs with a fatal error when it sees that
"*.c" is not a valid regex. So no matter what check_filename()
does, we would have to propagate that error from get_sha1() to make
anything interesting work there.
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...
Yes. After writing the above, I was envisioning pushing the "err on this
side" logic into check_filename() with a flag. The main callers are
verify_filename() and verify_non_filename(), and they would use opposite
flags from each other. But pulling that logic out to the caller would
be fine, too.
IOW, something like this implements the "permissive" thing I wrote above
(i.e., be inclusive when seeing if something could plausibly be a
filename, but exclusive when complaining that it _could_ be one):
diff --git a/setup.c b/setup.c
index 2c4b22c..995e924 100644
--- a/setup.c
+++ b/setup.c
@@ -139,9 +139,7 @@ int check_filename(const char *prefix, const char *arg)
if (arg[2] == '\0') /* ":/" is root dir, always exists */
return 1;
name = arg + 2;
- } else if (!no_wildcard(arg))
- return 1;
- else if (prefix)
+ } else if (prefix)
name = prefix_filename(prefix, strlen(prefix), arg);
else
name = arg;
@@ -202,7 +200,7 @@ void verify_filename(const char *prefix,
{
if (*arg == '-')
die("bad flag '%s' used after filename", arg);
- if (check_filename(prefix, arg))
+ if (check_filename(prefix, arg) || !no_wildcard(arg))
return;
die_verify_filename(prefix, arg, diagnose_misspelt_rev);
}