[PATCH 0/5] recursing submodules with relative pathspec (grep and ls-files)

It was discovered that when using the --recurse-submodules flag with `git grep`
and `git ls-files` and specifying a relative path when not at the root causes
the child processes spawned to error out with an error like:

fatal: ..: '..' is outside repository

While true that ".." is outside the scope of the submodule repository, it
probably doesn't make much sense to the user who gave that pathspec with
respect to the superproject.  Since the child processes that are spawned to
handle the submodules have some context that they are executing underneath a
superproject (via the 'super_prefix'), they should be able to prevent dying
under this circumstance.

This series fixes this bug in both git grep and git ls-files as well as
correctly formatting the output from submodules to handle the relative paths
with "..".

One of the changes made to fix this was to add an additional flag for the
parse_pathspec() function in order to treat all paths provided as being from
the root of the repository.  I hesitantly selected the name 'PATHSPEC_FROMROOT'
but I'm not fond of it since its too similar to the pathspec magic define
'PATHSPEC_FROMTOP'.  So I'm open for naming suggestions.

Brandon Williams (5):
  grep: illustrate bug when recursing with relative pathspec
  pathspec: add PATHSPEC_FROMROOT flag
  grep: fix bug when recuring with relative pathspec
  ls-files: illustrate bug when recursing with relative pathspec
  ls-files: fix bug when recuring with relative pathspec

 builtin/grep.c                         |  8 ++++--
 builtin/ls-files.c                     |  8 ++++--
 pathspec.c                             |  2 +-
 pathspec.h                             |  2 ++
 t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++
 t/t7814-grep-recurse-submodules.sh     | 42 ++++++++++++++++++++++++++++
 6 files changed, 107 insertions(+), 5 deletions(-)

-- 
2.11.0.483.g087da7b7c-goog

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for
a submodule.  When creating the pathspec struct in the child, the ".."
is interpreted to mean "go up a directory" which causes an error stating
that the path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was
indeed still inside the scope of the superproject.  Since the child
process luanched for the submodule has some context that it is operating
underneath a superproject, this error could be avoided.

Signed-off-by: Brandon Williams <redacted>
---
 t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 67247a01d..418ba68fe 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh

@@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '
 	test_cmp expect actual
 '
 
+test_expect_failure 'grep using relative path' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	echo "foobar" >parent/file &&
+	git -C parent add file &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file2 &&
+	git -C parent add src/file2 &&
+	git -C parent submodule add ../sub &&
+	git -C parent commit -m "add files and submodule" &&
+
+	# From top works
+	cat >expect <<-\EOF &&
+	file:foobar
+	src/file2:foobar
+	sub/file:foobar
+	EOF
+	git -C parent grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to top errors out
+	cat >expect <<-\EOF &&
+	../file:foobar
+	file2:foobar
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to submodule errors out
+	cat >expect <<-\EOF &&
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
+	test_cmp expect actual
+'
+
 test_incompatible_with_recurse_submodules ()
 {
 	test_expect_success "--recurse-submodules and $1 are incompatible" "

-- 
2.11.0.483.g087da7b7c-goog

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 <redacted>
---
 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

Fix a bug which causes a child process for a submodule to error out when
a relative pathspec with a ".." is provided in the superproject.

While at it, correctly construct the super-prefix to be used in a
submodule when not at the root of the repository.

Signed-off-by: Brandon Williams <redacted>
---
 builtin/grep.c                     | 8 ++++++--
 t/t7814-grep-recurse-submodules.sh | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..65f3413d1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -538,6 +538,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	int status, i;
 	const char *end_of_base;
 	const char *name;
+	struct strbuf buf = STRBUF_INIT;
 	struct work_item *w = opt->output_priv;
 
 	end_of_base = strchr(gs->name, ':');

@@ -550,9 +551,11 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
 
 	/* Add super prefix */
+	quote_path_relative(name, opt->prefix, &buf);
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",
-			 name);
+			 buf.buf);
+	strbuf_release(&buf);
 	argv_array_push(&cp.args, "grep");
 
 	/*

@@ -1199,7 +1202,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
-		       (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+		       (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |
+		       (super_prefix ? PATHSPEC_FROMROOT : 0),
 		       prefix, argv + i);
 	pathspec.max_depth = opt.max_depth;
 	pathspec.recursive = 1;

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 418ba68fe..e0932b2b7 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh

@@ -227,7 +227,7 @@ test_expect_success 'grep history with moved submoules' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'grep using relative path' '
+test_expect_success 'grep using relative path' '
 	test_when_finished "rm -rf parent sub" &&
 	git init sub &&
 	echo "foobar" >sub/file &&

-- 
2.11.0.483.g087da7b7c-goog

Fix a bug which causes a child process for a submodule to error out when a
relative pathspec with a ".." is provided in the superproject.

While at it, correctly construct the super-prefix to be used in a submodule
when not at the root of the repository.

Signed-off-by: Brandon Williams <redacted>
---
 builtin/ls-files.c                     | 8 ++++++--
 t/t3007-ls-files-recurse-submodules.sh | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 159229081..89533ab8e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag
 static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf name = STRBUF_INIT;
 	int status;
 	int i;
 
+	quote_path_relative(ce->name, prefix, &name);
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",
-			 ce->name);
+			 name.buf);
+	strbuf_release(&name);
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 

@@ -615,7 +618,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
-		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+		       (super_prefix ? PATHSPEC_FROMROOT : 0),
 		       prefix, argv);
 
 	/*

diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index d8ab10866..1522ed235 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh

@@ -188,7 +188,7 @@ test_expect_success '--recurse-submodules and pathspecs' '
 	test_cmp expect actual
 '
 
-test_expect_failure '--recurse-submodules and relative paths' '
+test_expect_success '--recurse-submodules and relative paths' '
 	# From top works
 	cat >expect <<-\EOF &&
 	.gitmodules

-- 
2.11.0.483.g087da7b7c-goog

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for a
submodule.  When creating the pathspec struct in the child, the ".." is
interpreted to mean "go up a directory" which causes an error stating that the
path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was indeed
still inside the scope of the superproject.  Since the child process luanched
for the submodule has some context that it is operating underneath a
superproject, this error could be avoided.

Signed-off-by: Brandon Williams <redacted>
---
 t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index a5426171d..d8ab10866 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh

@@ -188,6 +188,56 @@ test_expect_success '--recurse-submodules and pathspecs' '
 	test_cmp expect actual
 '
 
+test_expect_failure '--recurse-submodules and relative paths' '
+	# From top works
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	h.txt
+	sib/file
+	sub/file
+	submodule/.gitmodules
+	submodule/c
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/d
+	submodule/subsub/e.txt
+	EOF
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to top errors out
+	cat >expect <<-\EOF &&
+	../.gitmodules
+	../a
+	b
+	../h.txt
+	../sib/file
+	../sub/file
+	../submodule/.gitmodules
+	../submodule/c
+	../submodule/f.TXT
+	../submodule/g.txt
+	../submodule/subsub/d
+	../submodule/subsub/e.txt
+	EOF
+	git -C b ls-files --recurse-submodules -- .. >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to submodule errors out
+	cat >expect <<-\EOF &&
+	../submodule/.gitmodules
+	../submodule/c
+	../submodule/f.TXT
+	../submodule/g.txt
+	../submodule/subsub/d
+	../submodule/subsub/e.txt
+	EOF
+	git -C b ls-files --recurse-submodules -- ../submodule >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--recurse-submodules does not support --error-unmatch' '
 	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
 	test_i18ngrep "does not support --error-unmatch" actual

-- 
2.11.0.483.g087da7b7c-goog

On Fri, Feb 24, 2017 at 3:50 PM, Brandon Williams [off-list ref] 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" <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

On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams [off-list ref] wrote:

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for
a submodule.  When creating the pathspec struct in the child, the ".."
is interpreted to mean "go up a directory" which causes an error stating
that the path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was
indeed still inside the scope of the superproject.  Since the child
process luanched for the submodule has some context that it is operating

s/luanched/launched/

I would prefer 1/5 t to be merged with 3/5 though. The problem
description is very light there, and the test demonstration in the
diff is simply switching from failure to success, which forces the
reader to come back here. It's easier to find here now, but it'll be a
bit harder when it enters master and we have to read it from git-log,
I think.

I'm still munching through the super-prefix patches. From how you
changed match_pathspec call in 0281e487fd (grep: optionally recurse
into submodules - 2016-12-16), I guess pathspecs should be handled
with super-prefix instead of the submodule's prefix (which is empty
anyway, I guess). The right solution wrt. handling relative paths may
be teach pathspec about super-prefix (and even original super's cwd)
then let it processes path in supermodule's context.

Does it handle relative paths with wildcards correctly btw? Ones that
cross submodules? I have a feeling it doesn't, but I haven't seen how
exactly super-prefix works yet.

There's another problem with passing pathspec from one process to
another. The issue with preserving the prefix, see 233c3e6c59
(parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN -
2013-07-14). :(icase) needs this because given a path
"<prefix>/foobar", only the "foobar" part is considered case
insensitive, the prefix part is always case-sensitive. For example, if
you have 4 paths "abc/def", "abc/DEF", "ABC/def" and "ABC/DEF" and are
standing at "abc", you would want ":(icase)def" to match the first two
only, not all of them.

After 3/5, it's not "errors out" any more, is it?

+       cat >expect <<-\EOF &&
+       ../file:foobar
+       file2:foobar
+       ../sub/file:foobar
+       EOF
+       git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
+       test_cmp expect actual &&
+
+       # Relative path to submodule errors out

ditto

+       cat >expect <<-\EOF &&
+       ../sub/file:foobar
+       EOF
+       git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
+       test_cmp expect actual
+'
+
 test_incompatible_with_recurse_submodules ()
 {
        test_expect_success "--recurse-submodules and $1 are incompatible" "
--
2.11.0.483.g087da7b7c-goog

-- 
Duy

On 02/26, Duy Nguyen wrote:

On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams [off-list ref] wrote:

quoted

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for
a submodule.  When creating the pathspec struct in the child, the ".."
is interpreted to mean "go up a directory" which causes an error stating
that the path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was
indeed still inside the scope of the superproject.  Since the child
process luanched for the submodule has some context that it is operating

s/luanched/launched/

I would prefer 1/5 t to be merged with 3/5 though. The problem

We can definitely merge them and I agree that it may be easier for
looking through the logs if they are merged.

description is very light there, and the test demonstration in the
diff is simply switching from failure to success, which forces the
reader to come back here. It's easier to find here now, but it'll be a
bit harder when it enters master and we have to read it from git-log,
I think.

I'm still munching through the super-prefix patches. From how you
changed match_pathspec call in 0281e487fd (grep: optionally recurse
into submodules - 2016-12-16), I guess pathspecs should be handled
with super-prefix instead of the submodule's prefix (which is empty
anyway, I guess). The right solution wrt. handling relative paths may
be teach pathspec about super-prefix (and even original super's cwd)
then let it processes path in supermodule's context.

Does it handle relative paths with wildcards correctly btw? Ones that
cross submodules? I have a feeling it doesn't, but I haven't seen how
exactly super-prefix works yet.

I'm not 100% sure about the relative paths with wildcards.  I did notice
that this series solves one problem and introduces another (recursing
from a subdirectory doesn't work with this series) so I need to
rethink the solution a little bit.  And I'll take into account wildcards
as well.

There's another problem with passing pathspec from one process to
another. The issue with preserving the prefix, see 233c3e6c59
(parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN -
2013-07-14). :(icase) needs this because given a path
"<prefix>/foobar", only the "foobar" part is considered case
insensitive, the prefix part is always case-sensitive. For example, if
you have 4 paths "abc/def", "abc/DEF", "ABC/def" and "ABC/DEF" and are
standing at "abc", you would want ":(icase)def" to match the first two
only, not all of them.

Hmm...yeah its a really difficult thing to get 100% correct (as I'm now
noticing).  It also makes it a little more challenging because you don't
have the same state in both processes (child and parent).  I'm thinking
I may have to a little bit more work, which is more involved, to
properly handle the pathspecs passed to the children.  As in we may not
be able to pass the raw pathspec used in the parent to the child and
instead may have to do a little pre-processing on it.

quoted

underneath a superproject, this error could be avoided.

Signed-off-by: Brandon Williams <redacted>
---
 t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 67247a01d..418ba68fe 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh

@@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '
        test_cmp expect actual
 '

+test_expect_failure 'grep using relative path' '
+       test_when_finished "rm -rf parent sub" &&
+       git init sub &&
+       echo "foobar" >sub/file &&
+       git -C sub add file &&
+       git -C sub commit -m "add file" &&
+
+       git init parent &&
+       echo "foobar" >parent/file &&
+       git -C parent add file &&
+       mkdir parent/src &&
+       echo "foobar" >parent/src/file2 &&
+       git -C parent add src/file2 &&
+       git -C parent submodule add ../sub &&
+       git -C parent commit -m "add files and submodule" &&
+
+       # From top works
+       cat >expect <<-\EOF &&
+       file:foobar
+       src/file2:foobar
+       sub/file:foobar
+       EOF
+       git -C parent grep --recurse-submodules -e "foobar" >actual &&
+       test_cmp expect actual &&
+
+       # Relative path to top errors out

After 3/5, it's not "errors out" any more, is it?

Correct, will fix the comment.

quoted

+       cat >expect <<-\EOF &&
+       ../file:foobar
+       file2:foobar
+       ../sub/file:foobar
+       EOF
+       git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
+       test_cmp expect actual &&
+
+       # Relative path to submodule errors out

ditto

quoted

+       cat >expect <<-\EOF &&
+       ../sub/file:foobar
+       EOF
+       git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
+       test_cmp expect actual
+'
+
 test_incompatible_with_recurse_submodules ()
 {
        test_expect_success "--recurse-submodules and $1 are incompatible" "
--
2.11.0.483.g087da7b7c-goog

-- 
Duy

-- 
Brandon Williams

On 02/24, Brandon Williams wrote:

It was discovered that when using the --recurse-submodules flag with `git grep`
and `git ls-files` and specifying a relative path when not at the root causes
the child processes spawned to error out with an error like:

fatal: ..: '..' is outside repository

While true that ".." is outside the scope of the submodule repository, it
probably doesn't make much sense to the user who gave that pathspec with
respect to the superproject.  Since the child processes that are spawned to
handle the submodules have some context that they are executing underneath a
superproject (via the 'super_prefix'), they should be able to prevent dying
under this circumstance.

This series fixes this bug in both git grep and git ls-files as well as
correctly formatting the output from submodules to handle the relative paths
with "..".

One of the changes made to fix this was to add an additional flag for the
parse_pathspec() function in order to treat all paths provided as being from
the root of the repository.  I hesitantly selected the name 'PATHSPEC_FROMROOT'
but I'm not fond of it since its too similar to the pathspec magic define
'PATHSPEC_FROMTOP'.  So I'm open for naming suggestions.

Brandon Williams (5):
  grep: illustrate bug when recursing with relative pathspec
  pathspec: add PATHSPEC_FROMROOT flag
  grep: fix bug when recuring with relative pathspec
  ls-files: illustrate bug when recursing with relative pathspec
  ls-files: fix bug when recuring with relative pathspec

 builtin/grep.c                         |  8 ++++--
 builtin/ls-files.c                     |  8 ++++--
 pathspec.c                             |  2 +-
 pathspec.h                             |  2 ++
 t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++
 t/t7814-grep-recurse-submodules.sh     | 42 ++++++++++++++++++++++++++++
 6 files changed, 107 insertions(+), 5 deletions(-)

Turns out that this series doesn't address all of the issues.  This
series also seems to introduce broken behavior when recursing from a
subdirectory.  So I need to think about this problem a little bit more
and reroll.

-- 
Brandon Williams

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for
a submodule.  When creating the pathspec struct in the child, the ".."
is interpreted to mean "go up a directory" which causes an error stating
that the path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was
indeed still inside the scope of the superproject.  Since the child
process launched for the submodule has some context that it is operating
underneath a superproject, this error could be avoided.

This patch fixes the bug by passing the 'prefix' to the child process.
Now each child process that works on a submodule has two points of
reference to the superproject: (1) the 'super_prefix' which is the path
from the root of the superproject down to root of the submodule and (2)
the 'prefix' which is the path from the root of the superproject down to
the directory where the user invoked the git command.

With these two pieces of information a child process can correctly
interpret the pathspecs provided by the user as well as being able to
properly format the its output relative to the directory the user
invoked a git command from.

Signed-off-by: Brandon Williams <redacted>
---

After taking a closer look at this bug I determined that I did a poor job at
thinking of all the corner cases when implementing a recursive grep.  As it
turns out I think we really need to pass on the prefix information to the child
process so that it has enough context to do path matching and to format its
output.  Unfortunately I don't know the best way to pass on this information
without breaking other commands that rely on the GIT_PREFIX being overridden
during discovery.  Maybe we'll need to add in another env var to account for
that?  For the purposes of showing how to fix this bug while still getting the
test suite to pass, I have git respect the GIT_PREFIX env var only if the
super_prefix is set (which it should only be used in submodule operations at
this point in time).  Its a pretty hacky fix, so I'm up for other suggestions
on how to properly pass on this information to the child process.


 builtin/grep.c                     | 39 ++++++++++++--------
 git.c                              |  2 --
 setup.c                            |  5 +++
 t/t7814-grep-recurse-submodules.sh | 73 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 102 insertions(+), 17 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..acecad0e0 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -310,10 +310,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 {
 	struct strbuf pathbuf = STRBUF_INIT;
 
-	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);
-		strbuf_insert(&pathbuf, 0, filename, tree_name_len);
-	} else if (super_prefix) {
+	if (super_prefix) {
 		strbuf_add(&pathbuf, filename, tree_name_len);
 		strbuf_addstr(&pathbuf, super_prefix);
 		strbuf_addstr(&pathbuf, filename + tree_name_len);

@@ -321,6 +318,13 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 		strbuf_addstr(&pathbuf, filename);
 	}
 
+	if (opt->relative && opt->prefix_length) {
+		char *name = strbuf_detach(&pathbuf, NULL);
+		quote_path_relative(name + tree_name_len, opt->prefix, &pathbuf);
+		strbuf_insert(&pathbuf, 0, name, tree_name_len);
+		free(name);
+	}
+
 #ifndef NO_PTHREADS
 	if (num_threads) {
 		add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);

@@ -345,12 +349,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 {
 	struct strbuf buf = STRBUF_INIT;
 
+	if (super_prefix)
+		strbuf_addstr(&buf, super_prefix);
+	strbuf_addstr(&buf, filename);
+
 	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(filename, opt->prefix, &buf);
-	} else {
-		if (super_prefix)
-			strbuf_addstr(&buf, super_prefix);
-		strbuf_addstr(&buf, filename);
+		char *name = strbuf_detach(&buf, NULL);
+		quote_path_relative(name, opt->prefix, &buf);
+		free(name);
 	}
 
 #ifndef NO_PTHREADS

@@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
 }
 
 static void compile_submodule_options(const struct grep_opt *opt,
-				      const struct pathspec *pathspec,
+				      const char **argv,
 				      int cached, int untracked,
 				      int opt_exclude, int use_index,
 				      int pattern_type_arg)
 {
 	struct grep_pat *pattern;
-	int i;
 
 	if (recurse_submodules)
 		argv_array_push(&submodule_options, "--recurse-submodules");

@@ -523,9 +528,8 @@ static void compile_submodule_options(const struct grep_opt *opt,
 
 	/* Add Pathspecs */
 	argv_array_push(&submodule_options, "--");
-	for (i = 0; i < pathspec->nr; i++)
-		argv_array_push(&submodule_options,
-				pathspec->items[i].original);
+	for (; *argv; argv++)
+		argv_array_push(&submodule_options, *argv);
 }
 
 /*

@@ -549,6 +553,11 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	prepare_submodule_repo_env(&cp.env_array);
 	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
 
+	if (opt->relative && opt->prefix_length)
+		argv_array_pushf(&cp.env_array, "%s=%s",
+				 GIT_PREFIX_ENVIRONMENT,
+				 opt->prefix);
+
 	/* Add super prefix */
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",

@@ -1206,7 +1215,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (recurse_submodules) {
 		gitmodules_config();
-		compile_submodule_options(&opt, &pathspec, cached, untracked,
+		compile_submodule_options(&opt, argv + i, cached, untracked,
 					  opt_exclude, use_index,
 					  pattern_type_arg);
 	}

diff --git a/git.c b/git.c
index b367cf668..b05108afd 100644
--- a/git.c
+++ b/git.c

@@ -361,8 +361,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	if (!help && get_super_prefix()) {
 		if (!(p->option & SUPPORT_SUPER_PREFIX))
 			die("%s doesn't support --super-prefix", p->cmd);
-		if (prefix)
-			die("can't use --super-prefix from a subdirectory");
 	}
 
 	if (!help && p->option & NEED_WORK_TREE)

diff --git a/setup.c b/setup.c
index 1b534a750..0de379319 100644
--- a/setup.c
+++ b/setup.c

@@ -936,10 +936,15 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 const char *setup_git_directory_gently(int *nongit_ok)
 {
 	const char *prefix;
+	const char *env_prefix;
 
 	prefix = setup_git_directory_gently_1(nongit_ok);
+	env_prefix = getenv(GIT_PREFIX_ENVIRONMENT);
+
 	if (prefix)
 		setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
+	else if (env_prefix && get_super_prefix())
+		prefix = env_prefix;
 	else
 		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
 

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 67247a01d..d8ed3d64f 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh

@@ -227,6 +227,79 @@ test_expect_success 'grep history with moved submoules' '
 	test_cmp expect actual
 '
 
+test_expect_success 'grep using relative path' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	echo "foobar" >parent/file &&
+	git -C parent add file &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file2 &&
+	git -C parent add src/file2 &&
+	git -C parent submodule add ../sub &&
+	git -C parent commit -m "add files and submodule" &&
+
+	# From top works
+	cat >expect <<-\EOF &&
+	file:foobar
+	src/file2:foobar
+	sub/file:foobar
+	EOF
+	git -C parent grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to top errors out
+	cat >expect <<-\EOF &&
+	../file:foobar
+	file2:foobar
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to submodule errors out
+	cat >expect <<-\EOF &&
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'grep from a subdir' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file &&
+	git -C parent add src/file &&
+	git -C parent submodule add ../sub src/sub &&
+	git -C parent submodule add ../sub sub &&
+	git -C parent commit -m "add files and submodules" &&
+
+	cat >expect <<-\EOF &&
+	src/file:foobar
+	src/sub/file:foobar
+	sub/file:foobar
+	EOF
+	git -C parent grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	file:foobar
+	sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual
+'
+
 test_incompatible_with_recurse_submodules ()
 {
 	test_expect_success "--recurse-submodules and $1 are incompatible" "

-- 
2.12.0.rc1.440.g5b76565f74-goog

v2 of the series tackles the problem slightly differently than v1 did, and in a
less invasive way in my opinion.  v1 also didn't fix everything.

v2 introduces a way to pass a 'prefix' that is respected by a git process.
This allows the git child processes (which operate on submodules) to have the
super_prefix (the path from the root of the superproject to the submodule) and
the prefix (path from the root of the superproject to the directory in which
the original command was launched).  With these two pieces of information the
child process can correctly handle pathspec matching as well as correctly
formatting their output with relative paths.

In order to pass the prefix to a child I made a new env var since the existing
GIT_PREFIX isn't respected.  I'm not sure this is the best method so I'm open
to ideas on the best way to convey this information to a child process.

Brandon Williams (4):
  grep: fix help text typo
  setup: allow for prefix to be passed to git commands
  grep: fix bug when recursing with relative pathspec
  ls-files: fix bug when recursing with relative pathspec

 builtin/grep.c                         | 41 +++++++++++--------
 builtin/ls-files.c                     | 41 ++++++++++---------
 cache.h                                |  1 +
 git.c                                  |  2 -
 setup.c                                |  6 +++
 t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++
 t/t7814-grep-recurse-submodules.sh     | 75 ++++++++++++++++++++++++++++++++++
 7 files changed, 167 insertions(+), 38 deletions(-)

-- 
2.12.0.367.g23dc2f6d3c-goog

---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 9304c33e7..4694e68f3 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -979,7 +979,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "exclude-standard", &opt_exclude,
 			    N_("ignore files specified via '.gitignore'"), 1),
 		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
-			 N_("recursivley search in each submodule")),
+			 N_("recursively search in each submodule")),
 		OPT_STRING(0, "parent-basename", &parent_basename,
 			   N_("basename"),
 			   N_("prepend parent project's basename to output")),

-- 
2.12.0.367.g23dc2f6d3c-goog

In a future patch child processes which act on submodules need a little
more context about the original command that was invoked.  This patch
teaches git to use the prefix stored in `GIT_INTERNAL_TOPLEVEL_PREFIX`
if another prefix wasn't found during the git directory setup process.

---
 cache.h | 1 +
 git.c   | 2 --
 setup.c | 6 ++++++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 8c0e64420..7d253a078 100644
--- a/cache.h
+++ b/cache.h

@@ -410,6 +410,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
 #define GIT_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX"
+#define GIT_TOPLEVEL_PREFIX_ENVIRONMENT "GIT_INTERNAL_TOPLEVEL_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"

diff --git a/git.c b/git.c
index 33f52acbc..8ff44f081 100644
--- a/git.c
+++ b/git.c

@@ -361,8 +361,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	if (!help && get_super_prefix()) {
 		if (!(p->option & SUPPORT_SUPER_PREFIX))
 			die("%s doesn't support --super-prefix", p->cmd);
-		if (prefix)
-			die("can't use --super-prefix from a subdirectory");
 	}
 
 	if (!help && p->option & NEED_WORK_TREE)

diff --git a/setup.c b/setup.c
index 8f64fbdfb..c8492ea8a 100644
--- a/setup.c
+++ b/setup.c

@@ -940,8 +940,14 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 const char *setup_git_directory_gently(int *nongit_ok)
 {
 	const char *prefix;
+	const char *env_prefix;
 
 	prefix = setup_git_directory_gently_1(nongit_ok);
+	env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
+
+	if (env_prefix)
+		prefix = env_prefix;
+
 	if (prefix)
 		setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
 	else

-- 
2.12.0.367.g23dc2f6d3c-goog

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for
a submodule.  When creating the pathspec struct in the child, the ".."
is interpreted to mean "go up a directory" which causes an error stating
that the path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was
indeed still inside the scope of the superproject.  Since the child
process launched for the submodule has some context that it is operating
underneath a superproject, this error could be avoided.

This patch fixes the bug by passing the 'prefix' to the child process.
Now each child process that works on a submodule has two points of
reference to the superproject: (1) the 'super_prefix' which is the path
from the root of the superproject down to root of the submodule and (2)
the 'prefix' which is the path from the root of the superproject down to
the directory where the user invoked the git command.

With these two pieces of information a child process can correctly
interpret the pathspecs provided by the user as well as being able to
properly format its output relative to the directory the user invoked
the original command from.

Signed-off-by: Brandon Williams <redacted>
---
 builtin/grep.c                     | 39 ++++++++++++--------
 t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 15 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 4694e68f3..b5d846393 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -310,10 +310,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 {
 	struct strbuf pathbuf = STRBUF_INIT;
 
-	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);
-		strbuf_insert(&pathbuf, 0, filename, tree_name_len);
-	} else if (super_prefix) {
+	if (super_prefix) {
 		strbuf_add(&pathbuf, filename, tree_name_len);
 		strbuf_addstr(&pathbuf, super_prefix);
 		strbuf_addstr(&pathbuf, filename + tree_name_len);

@@ -321,6 +318,13 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 		strbuf_addstr(&pathbuf, filename);
 	}
 
+	if (opt->relative && opt->prefix_length) {
+		char *name = strbuf_detach(&pathbuf, NULL);
+		quote_path_relative(name + tree_name_len, opt->prefix, &pathbuf);
+		strbuf_insert(&pathbuf, 0, name, tree_name_len);
+		free(name);
+	}
+
 #ifndef NO_PTHREADS
 	if (num_threads) {
 		add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);

@@ -345,12 +349,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 {
 	struct strbuf buf = STRBUF_INIT;
 
+	if (super_prefix)
+		strbuf_addstr(&buf, super_prefix);
+	strbuf_addstr(&buf, filename);
+
 	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(filename, opt->prefix, &buf);
-	} else {
-		if (super_prefix)
-			strbuf_addstr(&buf, super_prefix);
-		strbuf_addstr(&buf, filename);
+		char *name = strbuf_detach(&buf, NULL);
+		quote_path_relative(name, opt->prefix, &buf);
+		free(name);
 	}
 
 #ifndef NO_PTHREADS

@@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
 }
 
 static void compile_submodule_options(const struct grep_opt *opt,
-				      const struct pathspec *pathspec,
+				      const char **argv,
 				      int cached, int untracked,
 				      int opt_exclude, int use_index,
 				      int pattern_type_arg)
 {
 	struct grep_pat *pattern;
-	int i;
 
 	if (recurse_submodules)
 		argv_array_push(&submodule_options, "--recurse-submodules");

@@ -523,9 +528,8 @@ static void compile_submodule_options(const struct grep_opt *opt,
 
 	/* Add Pathspecs */
 	argv_array_push(&submodule_options, "--");
-	for (i = 0; i < pathspec->nr; i++)
-		argv_array_push(&submodule_options,
-				pathspec->items[i].original);
+	for (; *argv; argv++)
+		argv_array_push(&submodule_options, *argv);
 }
 
 /*

@@ -549,6 +553,11 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	prepare_submodule_repo_env(&cp.env_array);
 	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
 
+	if (opt->relative && opt->prefix_length)
+		argv_array_pushf(&cp.env_array, "%s=%s",
+				 GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
+				 opt->prefix);
+
 	/* Add super prefix */
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",

@@ -1236,7 +1245,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (recurse_submodules) {
 		gitmodules_config();
-		compile_submodule_options(&opt, &pathspec, cached, untracked,
+		compile_submodule_options(&opt, argv + i, cached, untracked,
 					  opt_exclude, use_index,
 					  pattern_type_arg);
 	}

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 67247a01d..5b6eb3a65 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh

@@ -227,6 +227,81 @@ test_expect_success 'grep history with moved submoules' '
 	test_cmp expect actual
 '
 
+test_expect_success 'grep using relative path' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	echo "foobar" >parent/file &&
+	git -C parent add file &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file2 &&
+	git -C parent add src/file2 &&
+	git -C parent submodule add ../sub &&
+	git -C parent commit -m "add files and submodule" &&
+
+	# From top works
+	cat >expect <<-\EOF &&
+	file:foobar
+	src/file2:foobar
+	sub/file:foobar
+	EOF
+	git -C parent grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to top
+	cat >expect <<-\EOF &&
+	../file:foobar
+	file2:foobar
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to submodule
+	cat >expect <<-\EOF &&
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'grep from a subdir' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file &&
+	git -C parent add src/file &&
+	git -C parent submodule add ../sub src/sub &&
+	git -C parent submodule add ../sub sub &&
+	git -C parent commit -m "add files and submodules" &&
+
+	# Verify grep from root works
+	cat >expect <<-\EOF &&
+	src/file:foobar
+	src/sub/file:foobar
+	sub/file:foobar
+	EOF
+	git -C parent grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual &&
+
+	# Verify grep from a subdir works
+	cat >expect <<-\EOF &&
+	file:foobar
+	sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual
+'
+
 test_incompatible_with_recurse_submodules ()
 {
 	test_expect_success "--recurse-submodules and $1 are incompatible" "

-- 
2.12.0.367.g23dc2f6d3c-goog

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for a
submodule.  When creating the pathspec struct in the child, the ".." is
interpreted to mean "go up a directory" which causes an error stating that the
path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was indeed
still inside the scope of the superproject.  Since the child process launched
for the submodule has some context that it is operating underneath a
superproject, this error could be avoided.

This patch fixes the bug by passing the 'prefix' to the child process.  Now
each child process that works on a submodule has two points of reference to the
superproject: (1) the 'super_prefix' which is the path from the root of the
superproject down to root of the submodule and (2) the 'prefix' which is the
path from the root of the superproject down to the directory where the user
invoked the git command.

With these two pieces of information a child process can correctly interpret
the pathspecs provided by the user as well as being able to properly format its
output relative to the directory the user invoked the original command from.

Signed-off-by: Brandon Williams <redacted>
---
 builtin/ls-files.c                     | 41 +++++++++++++++++-----------------
 t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1c0f057d0..d449e46db 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -30,7 +30,7 @@ static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
-static struct argv_array submodules_options = ARGV_ARRAY_INIT;
+static struct argv_array submodule_options = ARGV_ARRAY_INIT;
 
 static const char *prefix;
 static const char *super_prefix;

@@ -172,20 +172,27 @@ static void show_killed_files(struct dir_struct *dir)
 /*
  * Compile an argv_array with all of the options supported by --recurse_submodules
  */
-static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+static void compile_submodule_options(const char **argv,
+				      const struct dir_struct *dir,
+				      int show_tag)
 {
 	if (line_terminator == '\0')
-		argv_array_push(&submodules_options, "-z");
+		argv_array_push(&submodule_options, "-z");
 	if (show_tag)
-		argv_array_push(&submodules_options, "-t");
+		argv_array_push(&submodule_options, "-t");
 	if (show_valid_bit)
-		argv_array_push(&submodules_options, "-v");
+		argv_array_push(&submodule_options, "-v");
 	if (show_cached)
-		argv_array_push(&submodules_options, "--cached");
+		argv_array_push(&submodule_options, "--cached");
 	if (show_eol)
-		argv_array_push(&submodules_options, "--eol");
+		argv_array_push(&submodule_options, "--eol");
 	if (debug_mode)
-		argv_array_push(&submodules_options, "--debug");
+		argv_array_push(&submodule_options, "--debug");
+
+	/* Add Pathspecs */
+	argv_array_push(&submodule_options, "--");
+	for (; *argv; argv++)
+		argv_array_push(&submodule_options, *argv);
 }
 
 /**

@@ -195,8 +202,11 @@ static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int status;
-	int i;
 
+	if (prefix_len)
+		argv_array_pushf(&cp.env_array, "%s=%s",
+				 GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
+				 prefix);
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",
 			 ce->name);

@@ -204,16 +214,7 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "--recurse-submodules");
 
 	/* add supported options */
-	argv_array_pushv(&cp.args, submodules_options.argv);
-
-	/*
-	 * Pass in the original pathspec args.  The submodule will be
-	 * responsible for prepending the 'submodule_prefix' prior to comparing
-	 * against the pathspec for matches.
-	 */
-	argv_array_push(&cp.args, "--");
-	for (i = 0; i < pathspec.nr; i++)
-		argv_array_push(&cp.args, pathspec.items[i].original);
+	argv_array_pushv(&cp.args, submodule_options.argv);
 
 	cp.git_cmd = 1;
 	cp.dir = ce->name;

@@ -604,7 +605,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		setup_work_tree();
 
 	if (recurse_submodules)
-		compile_submodule_options(&dir, show_tag);
+		compile_submodule_options(argv, &dir, show_tag);
 
 	if (recurse_submodules &&
 	    (show_stage || show_deleted || show_others || show_unmerged ||

diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index a5426171d..4cf6ccf5a 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh

@@ -188,6 +188,45 @@ test_expect_success '--recurse-submodules and pathspecs' '
 	test_cmp expect actual
 '
 
+test_expect_success '--recurse-submodules and relative paths' '
+	# From subdir
+	cat >expect <<-\EOF &&
+	b
+	EOF
+	git -C b ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to top
+	cat >expect <<-\EOF &&
+	../.gitmodules
+	../a
+	b
+	../h.txt
+	../sib/file
+	../sub/file
+	../submodule/.gitmodules
+	../submodule/c
+	../submodule/f.TXT
+	../submodule/g.txt
+	../submodule/subsub/d
+	../submodule/subsub/e.txt
+	EOF
+	git -C b ls-files --recurse-submodules -- .. >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to submodule
+	cat >expect <<-\EOF &&
+	../submodule/.gitmodules
+	../submodule/c
+	../submodule/f.TXT
+	../submodule/g.txt
+	../submodule/subsub/d
+	../submodule/subsub/e.txt
+	EOF
+	git -C b ls-files --recurse-submodules -- ../submodule >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--recurse-submodules does not support --error-unmatch' '
 	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
 	test_i18ngrep "does not support --error-unmatch" actual

-- 
2.12.0.367.g23dc2f6d3c-goog

Hi Brandon,

On Tue, 14 Mar 2017, Brandon Williams wrote:

In a future patch child processes which act on submodules need a little
more context about the original command that was invoked.  This patch
teaches git to use the prefix stored in `GIT_INTERNAL_TOPLEVEL_PREFIX`
if another prefix wasn't found during the git directory setup process.

Missing SOB ;-)

I'd just append this to the previous line (`const char *prefix,
*env_prefix`).

 	prefix = setup_git_directory_gently_1(nongit_ok);
+	env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
+
+	if (env_prefix)
+		prefix = env_prefix;

The commit message claims that env_prefix is used if no other prefix was
found, but this code ignores any prefix if the environment variable was
set.

Which version is correct?

Ciao,
Johannes

On 03/14, Johannes Schindelin wrote:

Hi Brandon,

On Tue, 14 Mar 2017, Brandon Williams wrote:

quoted

In a future patch child processes which act on submodules need a little
more context about the original command that was invoked.  This patch
teaches git to use the prefix stored in `GIT_INTERNAL_TOPLEVEL_PREFIX`
if another prefix wasn't found during the git directory setup process.

Missing SOB ;-)

Thanks for that catch.  I'm expecting some discussion on this patch in
particular (and cc'd you since you've been working on this area of the
code) so I'll make sure to add it in the next reroll.

quoted

diff --git a/setup.c b/setup.c
index 8f64fbdfb..c8492ea8a 100644
--- a/setup.c
+++ b/setup.c

@@ -940,8 +940,14 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 const char *setup_git_directory_gently(int *nongit_ok)
 {
 	const char *prefix;
+	const char *env_prefix;

I'd just append this to the previous line (`const char *prefix,
*env_prefix`).

That would be cleaner.

quoted

 	prefix = setup_git_directory_gently_1(nongit_ok);
+	env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
+
+	if (env_prefix)
+		prefix = env_prefix;

The commit message claims that env_prefix is used if no other prefix was
found, but this code ignores any prefix if the environment variable was
set.

Which version is correct?

Well, as you can tell I flip-flopped on what I thought the best course
of action would be.  For my intentions (submodule-centric) I don't
believe they would ever both have a value so it doesn't matter to me
which it is.  Though future users may want a particular order of
precedence.

Ciao,
Johannes

-- 
Brandon Williams

On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams [off-list ref] wrote:

Missing SoB here, too.

On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams [off-list ref] wrote:

This patch fixes the bug by passing the 'prefix' to the child process.
Now each child process that works on a submodule has two points of
reference to the superproject: (1) the 'super_prefix' which is the path
from the root of the superproject down to root of the submodule and (2)
the 'prefix' which is the path from the root of the superproject down to
the directory where the user invoked the git command.

Would the information of this paragraph also be well suited in the
documentation?

(1) clarifies what the super prefix is.
(2) maybe the docs or this commit message should talk about
  how the prefix is relayed, i.e. it doesn't set 'prefix' in the child,
  but it reads GIT_TOPLEVEL_PREFIX_ENVIRONMENT and
  acts on that as (2) ?

On Tue, Mar 14, 2017 at 3:11 PM, Brandon Williams [off-list ref] wrote:

Up to here we only rename a variable? If you want to help reviewers,
please separate this into two patches. One refactoring, stating it doesn't
change behavior; and the other adding the behavioral changes.

+
+       /* Add Pathspecs */
+       argv_array_push(&submodule_options, "--");
+       for (; *argv; argv++)
+               argv_array_push(&submodule_options, *argv);
 }

On 03/14, Stefan Beller wrote:

On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams [off-list ref] wrote:

Missing SoB here, too.

I guess I'm having an off day...Will fix.

quoted

---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 9304c33e7..4694e68f3 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -979,7 +979,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                OPT_SET_INT(0, "exclude-standard", &opt_exclude,
                            N_("ignore files specified via '.gitignore'"), 1),
                OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
-                        N_("recursivley search in each submodule")),
+                        N_("recursively search in each submodule")),
                OPT_STRING(0, "parent-basename", &parent_basename,
                           N_("basename"),
                           N_("prepend parent project's basename to output")),
--

2.12.0.367.g23dc2f6d3c-goog

-- 
Brandon Williams

On 03/14, Stefan Beller wrote:

On Tue, Mar 14, 2017 at 3:11 PM, Brandon Williams [off-list ref] wrote:

quoted

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for a
submodule.  When creating the pathspec struct in the child, the ".." is
interpreted to mean "go up a directory" which causes an error stating that the
path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was indeed
still inside the scope of the superproject.  Since the child process launched
for the submodule has some context that it is operating underneath a
superproject, this error could be avoided.

This patch fixes the bug by passing the 'prefix' to the child process.  Now
each child process that works on a submodule has two points of reference to the
superproject: (1) the 'super_prefix' which is the path from the root of the
superproject down to root of the submodule and (2) the 'prefix' which is the
path from the root of the superproject down to the directory where the user
invoked the git command.

With these two pieces of information a child process can correctly interpret
the pathspecs provided by the user as well as being able to properly format its
output relative to the directory the user invoked the original command from.

Signed-off-by: Brandon Williams <redacted>
---
 builtin/ls-files.c                     | 41 +++++++++++++++++-----------------
 t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 20 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1c0f057d0..d449e46db 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c

@@ -30,7 +30,7 @@ static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
-static struct argv_array submodules_options = ARGV_ARRAY_INIT;
+static struct argv_array submodule_options = ARGV_ARRAY_INIT;

 static const char *prefix;
 static const char *super_prefix;

@@ -172,20 +172,27 @@ static void show_killed_files(struct dir_struct *dir)
 /*
  * Compile an argv_array with all of the options supported by --recurse_submodules
  */
-static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+static void compile_submodule_options(const char **argv,
+                                     const struct dir_struct *dir,
+                                     int show_tag)
 {
        if (line_terminator == '\0')
-               argv_array_push(&submodules_options, "-z");
+               argv_array_push(&submodule_options, "-z");
        if (show_tag)
-               argv_array_push(&submodules_options, "-t");
+               argv_array_push(&submodule_options, "-t");
        if (show_valid_bit)
-               argv_array_push(&submodules_options, "-v");
+               argv_array_push(&submodule_options, "-v");
        if (show_cached)
-               argv_array_push(&submodules_options, "--cached");
+               argv_array_push(&submodule_options, "--cached");
        if (show_eol)
-               argv_array_push(&submodules_options, "--eol");
+               argv_array_push(&submodule_options, "--eol");
        if (debug_mode)
-               argv_array_push(&submodules_options, "--debug");
+               argv_array_push(&submodule_options, "--debug");

Up to here we only rename a variable? If you want to help reviewers,
please separate this into two patches. One refactoring, stating it doesn't
change behavior; and the other adding the behavioral changes.

I can do that.

quoted

+
+       /* Add Pathspecs */
+       argv_array_push(&submodule_options, "--");
+       for (; *argv; argv++)
+               argv_array_push(&submodule_options, *argv);
 }

-- 
Brandon Williams

When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for
a submodule.  When creating the pathspec struct in the child, the ".."
is interpreted to mean "go up a directory" which causes an error stating
that the path ".." is outside of the repository.

While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was
indeed still inside the scope of the superproject.  Since the child
process launched for the submodule has some context that it is operating
underneath a superproject, this error could be avoided.

This patch fixes the bug by passing the 'prefix' to the child process.
Now each child process that works on a submodule has two points of
reference to the superproject: (1) the 'super_prefix' which is the path
from the root of the superproject down to root of the submodule and (2)
the 'prefix' which is the path from the root of the superproject down to
the directory where the user invoked the git command.

With these two pieces of information a child process can correctly
interpret the pathspecs provided by the user as well as being able to
properly format its output relative to the directory the user invoked
the original command from.

Signed-off-by: Brandon Williams <redacted>
---
 builtin/grep.c                     | 39 ++++++++++++--------
 t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 15 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 4694e68f3..b5d846393 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -310,10 +310,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 {
 	struct strbuf pathbuf = STRBUF_INIT;
 
-	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);
-		strbuf_insert(&pathbuf, 0, filename, tree_name_len);
-	} else if (super_prefix) {
+	if (super_prefix) {
 		strbuf_add(&pathbuf, filename, tree_name_len);
 		strbuf_addstr(&pathbuf, super_prefix);
 		strbuf_addstr(&pathbuf, filename + tree_name_len);

@@ -321,6 +318,13 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 		strbuf_addstr(&pathbuf, filename);
 	}
 
+	if (opt->relative && opt->prefix_length) {
+		char *name = strbuf_detach(&pathbuf, NULL);
+		quote_path_relative(name + tree_name_len, opt->prefix, &pathbuf);
+		strbuf_insert(&pathbuf, 0, name, tree_name_len);
+		free(name);
+	}
+
 #ifndef NO_PTHREADS
 	if (num_threads) {
 		add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);

@@ -345,12 +349,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 {
 	struct strbuf buf = STRBUF_INIT;
 
+	if (super_prefix)
+		strbuf_addstr(&buf, super_prefix);
+	strbuf_addstr(&buf, filename);
+
 	if (opt->relative && opt->prefix_length) {
-		quote_path_relative(filename, opt->prefix, &buf);
-	} else {
-		if (super_prefix)
-			strbuf_addstr(&buf, super_prefix);
-		strbuf_addstr(&buf, filename);
+		char *name = strbuf_detach(&buf, NULL);
+		quote_path_relative(name, opt->prefix, &buf);
+		free(name);
 	}
 
 #ifndef NO_PTHREADS

@@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)
 }
 
 static void compile_submodule_options(const struct grep_opt *opt,
-				      const struct pathspec *pathspec,
+				      const char **argv,
 				      int cached, int untracked,
 				      int opt_exclude, int use_index,
 				      int pattern_type_arg)
 {
 	struct grep_pat *pattern;
-	int i;
 
 	if (recurse_submodules)
 		argv_array_push(&submodule_options, "--recurse-submodules");

@@ -523,9 +528,8 @@ static void compile_submodule_options(const struct grep_opt *opt,
 
 	/* Add Pathspecs */
 	argv_array_push(&submodule_options, "--");
-	for (i = 0; i < pathspec->nr; i++)
-		argv_array_push(&submodule_options,
-				pathspec->items[i].original);
+	for (; *argv; argv++)
+		argv_array_push(&submodule_options, *argv);
 }
 
 /*

@@ -549,6 +553,11 @@ static int grep_submodule_launch(struct grep_opt *opt,
 	prepare_submodule_repo_env(&cp.env_array);
 	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
 
+	if (opt->relative && opt->prefix_length)
+		argv_array_pushf(&cp.env_array, "%s=%s",
+				 GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
+				 opt->prefix);
+
 	/* Add super prefix */
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",

@@ -1236,7 +1245,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 
 	if (recurse_submodules) {
 		gitmodules_config();
-		compile_submodule_options(&opt, &pathspec, cached, untracked,
+		compile_submodule_options(&opt, argv + i, cached, untracked,
 					  opt_exclude, use_index,
 					  pattern_type_arg);
 	}

diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 67247a01d..5b6eb3a65 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh

@@ -227,6 +227,81 @@ test_expect_success 'grep history with moved submoules' '
 	test_cmp expect actual
 '
 
+test_expect_success 'grep using relative path' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	echo "foobar" >parent/file &&
+	git -C parent add file &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file2 &&
+	git -C parent add src/file2 &&
+	git -C parent submodule add ../sub &&
+	git -C parent commit -m "add files and submodule" &&
+
+	# From top works
+	cat >expect <<-\EOF &&
+	file:foobar
+	src/file2:foobar
+	sub/file:foobar
+	EOF
+	git -C parent grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to top
+	cat >expect <<-\EOF &&
+	../file:foobar
+	file2:foobar
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to submodule
+	cat >expect <<-\EOF &&
+	../sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" -- ../sub >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'grep from a subdir' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file &&
+	git -C parent add src/file &&
+	git -C parent submodule add ../sub src/sub &&
+	git -C parent submodule add ../sub sub &&
+	git -C parent commit -m "add files and submodules" &&
+
+	# Verify grep from root works
+	cat >expect <<-\EOF &&
+	src/file:foobar
+	src/sub/file:foobar
+	sub/file:foobar
+	EOF
+	git -C parent grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual &&
+
+	# Verify grep from a subdir works
+	cat >expect <<-\EOF &&
+	file:foobar
+	sub/file:foobar
+	EOF
+	git -C parent/src grep --recurse-submodules -e "foobar" >actual &&
+	test_cmp expect actual
+'
+
 test_incompatible_with_recurse_submodules ()
 {
 	test_expect_success "--recurse-submodules and $1 are incompatible" "