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

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);
 }