Re: [PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag

From: Stefan Beller <sbeller@google.com>
Date: Feb 25, 2017, 00:31 UTC
Message-ID: <CAGZ79kYRrAmJ+=563PBkdiBwaUzqqQOMVANT_fTvURFv+=Yiog@mail.gmail.com>
In-Reply-To: <20170224235100.52627-3-bmwill@google.com>

On Fri, Feb 24, 2017 at 3:50 PM, Brandon Williams <bmwill@google.com> wrote:

Show 6 quoted lines

> Add the `PATHSPEC_FROMROOT` flag to allow callers to instruct
> 'parse_pathspec()' that all provided pathspecs are relative to the root
> of the repository.  This allows a caller to prevent a path that may be
> outside of the repository from erroring out during the pathspec struct
> construction.
>

> +/* For callers that know all paths are relative to the root of the repository */
> +#define PATHSPEC_FROMROOT (1<<9)

What is the calling convention for these submodule pathspecs? IIRC, we'd pass on the super-prefix and the literal pathspec, e.g. when there is a submodule "sub" inside the superproject, The invocation on the submodule would be

    git FOO --super-prefix="sub" <arguments> -- sub/pathspec/inside/...

and then the submodule process would need to "subtract" the superprefix from the pathspec argument to see its actual pathspec, e.g. in gerrit:

$ GIT_TRACE=1 git grep --recurse-submodules -i test -- \
    plugins/cookbook-plugin/
...

trace: run_command: '--super-prefix=plugins/cookbook-plugin/' 'grep' \
  '--recurse-submodules' '-i' '-etest' '--threads=4' '--'
'plugins/cookbook-plugin/'
..
but also:
...
 trace: run_command: '--super-prefix=plugins/download-commands/' 'grep' \
  '--recurse-submodules' '-i' '-etest' '--threads=4' '--'
'plugins/cookbook-plugin/'
...

So if I change into a directory:

$ cd plugins plugins$ git grep --recurse-submodules -i test -- cookbook-plugin/ plugins$ #empty? plugins$ git grep --recurse-submodules -i test -- plugins/cookbook-plugin/ ... Usual output, so the pathspecs are absolute path to the superprojects root? Let's try relative path: plugins$ git grep --recurse-submodules -i test -- ../plugins/cookbook-plugin/ fatal: ../plugins/cookbook-plugin/: '../plugins/cookbook-plugin/' is outside repository ... Running with GIT_TRACE=1:

trace: run_command: '--super-prefix=plugins/cookbook-plugin/' 'grep' \
    '--recurse-submodules' '-i' '-etest' '--threads=4' '--'
'../plugins/cookbook-plugin/'

that seems like a mismatch of pathspec and superproject prefix, the prefix ought to be different then? Maybe also including ../ because that is the relative path from cwd to the superporojects root and that is where we anchor all paths?

Easy to test that out:

plugins$ GIT_TRACE=1 git --super-prefix=../ grep --recurse-submodules \
    -i test -- ../plugins/cookbook-plugin/
fatal: can't use --super-prefix from a subdirectory

ok, not as easy. :/

So another test with relative path: (in git.git)

cd t/diff-lib t/diff-lib$ git grep freedom COPYING:freedom to share and change it. By contrast, the GNU General Public ...

So the path displayed is relative to the cwd (and the search results as well) In the submodule case we would expect to have the super prefix to be computed to be relative to the cwd?

Checking the tests, this is handled correctly with this patch series. :)

But nevertheless, I think I know why I dislike this approach now:
The super prefix is handled "too dumb" IMHO, see the case
    plugins$ git grep test  -- cookbook-plugin/
above, that doesn't correctly figure out the correct output.
Although this might be a separate bug, but it sounds like it
is the same underlying issue.

-- for the naming: How about PATHSPEC_FROMOUTSIDE when going with the series as is here? (the superprefix is not resolved, so the pathspecs given are literally pathspecs that are outside this repo and we can ignore them?

Thanks, Stefan