Re: [PATCH] doc: ls-tree paths do not support wildcards

[Jeff King <peff@peff.net>]

On Thu, May 28, 2020 at 09:23:16PM +0000, Steven Willis via GitGitGadget wrote:

> From: Steven Willis <onlynone@gmail.com>
> 
> Signed-off-by: Steven Willis <onlynone@gmail.com>
> ---
>     doc: ls-tree paths do not support wildcards
>     
>     The documentation for ls-tree says that paths can be wildcards, but this
>     appears to be incorrect, only raw paths seem to work.

This part probably should be in the commit message. :)

>  [<path>...]::
> -	When paths are given, show them (note that this isn't really raw
> -	pathnames, but rather a list of patterns to match).  Otherwise
> +	When paths are given, show them (note that this is really raw
> +	pathnames, not a list of patterns to match).  Otherwise
>  	implicitly uses the root level of the tree as the sole path argument.

You're right that we don't match globs, but I don't think it's accurate
to say that these aren't patterns, or that they are raw pathnames. We
_do_ parse them as pathspecs, include magic prefixes. E.g.:

  $ git -C Documentation ls-tree HEAD -- :/Makefile
  100644 blob 90aa329eb7836824a7a45383e4b5b157124d815c	../Makefile

And we complain about pathspec magic that isn't supported in the
matching code:

  $ git ls-tree HEAD -- :^Makefile
  fatal: :^Makefile: pathspec magic not supported by this command: 'exclude' (mnemonic: '!')

But we don't complain about an attempt to use glob characters (which
_could_ really be an attempt to specify a funny-named file, though I
guess you could argue the same for ":" magic).

So I think for now we ought to explain the situation a bit more clearly:
leave this language as-is, but add a new section describing what
patterns we do support.

In the long run it would be nice to actually match regular pathspecs.
That would be a backwards-incompatibility, which I think is why nobody
has pursued it further (and ls-tree is meant to be plumbing that should
stay consistent, so we need to be extra careful). So we'd need a
transition plan. Perhaps:

  1. Deprecate the current behavior in the documentation and release
     notes, encouraging people who want literal matching to use
     --literal-pathspecs or the ":(literal)" magic. AFAICT we've
     supported these since at least 2013 for this command, so it should
     be safe to use unconditionally.

  2. Add a new option, "--use-pathspecs" or similar, that switches the
     matching code to use match_pathspec(). That lets people use the new
     feature immediately if they want to.

  3. When --use-pathspecs is not in use, warn to stderr about any
     wildcard characters in the input. That reinforces the deprecation
     notice in (1) and is likely to get more people's attention.

  4. After several releases, flip the default to --use-pathspecs,
     leaving --no-use-pathspecs as an escape hatch for people who still
     haven't switched their scripts.

  5. After several more releases, eventually remove the old-style
     matching (perhaps leaving --use-pathspecs as a noop).

To be honest, that may be more careful than we absolutely need to be.
We'd only do the wrong thing if you really do have files with glob
metacharacters in their names. And if you're expecting to match names
literally and not using ":(literal)" or similar, your script is
_already_ wrong, since it would be barfing on names starting with a
colon. I have a feeling we made that backwards-incompatible change when
this was converted to pathspecs years ago, and nobody noticed either
way.

-Peff