Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules

[Junio C Hamano <gitster@pobox.com>]

Brandon Williams <bmwill@google.com> writes:

>  static void show_ce_entry(const char *tag, const struct cache_entry *ce)
>  {
> +	struct strbuf name = STRBUF_INIT;
>  	int len = max_prefix_len;
> +	if (submodule_prefix)
> +		strbuf_addstr(&name, submodule_prefix);
> +	strbuf_addstr(&name, ce->name);
> ...  
> +	} else if (match_pathspec(&pathspec, name.buf, name.len,
> +				  len, ps_matched,
> +				  S_ISDIR(ce->ce_mode) ||
> +				  S_ISGITLINK(ce->ce_mode))) {

There is an interesting observation around this code.  Note that it
is just something to keep in mind, even though I think we are in no
position to solve this within the scope of this series, or in fact I
am not sure if there is anything to "fix".

The expectation here is that the leading part of pathspec elements
contain path components above and outside the current working tree,
e.g. in a superproject with a submodule at "sub/", the end-user may
have said from the top of the superproject

    git ls-files --recurse-submodules -- sub/file

and the recursing "ls-files" is spawned as

    git -C sub ls-files -- sub/file

relaying the pathspec literally.

This does not correctly work if the path to the submodule has
wildcard in it.  Imagine that the submodule were at "s*b/".  The
recursing invocation would look like:

    git -C "s*b" ls-files -- "s*b/file"

Further imagine that the index in the submodule at "s*b" has two
paths in it, i.e.

	file
        oob/file

The prefix is prepended to them, to turn them into

	s*b/file
        s*b/oob/file

and I suspect that the pathspec element "s*b/file" would match both
of them.

The pathspec machinery has a provision to prevent a similar gotcha
happening for the "prefix" we internally use.  In a sample
repository created like so:

    $ git init
    $ mkdir -p 's*b/oob' sib
    $ >sib/file
    $ cd 's*b'
    $ >file
    $ >oob/file
    $ git add .
    $ git ls-files -- file

the "ls-files" in the last step gets 's*b/' as the "prefix", and the
pathspec is formed by concatenating "file" to it, but in a special
way.  The part that come from the "prefix" is marked not to honor
any wildcard in it, so 's*b/' even though it has an asterisk, it is
forced to match literally, giving only 's*b/file'.

A saving grace is that "s*b/file" in this case is what the end-user
is giving us, not something we internally generated.  So we can
simply blame the end user, saying "what --recurse-submodules does is
to (conceptually) flatten the indices of submodules into the index
of the superproject and show the entries that match your pathspec.
Because you gave us 's*b/file', which does match 's*b/oob/file',
that is what you get."

;-)