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

[Stefan Beller]

On Fri, Feb 24, 2017 at 3:50 PM, Brandon Williams  wrote:
> 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"  -- 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

[PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag

[Brandon Williams]

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.

Signed-off-by: Brandon Williams 
---
 pathspec.c | 2 +-
 pathspec.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/pathspec.c b/pathspec.c
index 7ababb315..ff622ba18 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -353,7 +353,7 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
-   } else if (magic & PATHSPEC_FROMTOP) {
+   } else if ((magic & PATHSPEC_FROMTOP) || (flags & PATHSPEC_FROMROOT)) {
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
diff --git a/pathspec.h b/pathspec.h
index 49fd823dd..cad197436 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -66,6 +66,8 @@ struct pathspec {
  * allowed, then it will automatically set for every pathspec.
  */
 #define PATHSPEC_LITERAL_PATH (1<<8)
+/* For callers that know all paths are relative to the root of the repository 
*/
+#define PATHSPEC_FROMROOT (1<<9)
 
 extern void parse_pathspec(struct pathspec *pathspec,
   unsigned magic_mask,
-- 
2.11.0.483.g087da7b7c-goog