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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2016, 19:20 UTC
Message-ID: <xmqqbmze1kzt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1474500486-101640-1-git-send-email-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 12 quoted lines

>  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."

;-)