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

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

[Brandon Williams]

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

[PATCH 1/5] grep: illustrate bug when recursing with relative pathspec

[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 luanched for the submodule has some context that it is operating
underneath a superproject, this error could be avoided.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 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

[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 <bmwill@google.com>
---
 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

[PATCH 3/5] grep: fix bug when recuring with relative pathspec

[Brandon Williams]

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 <bmwill@google.com>
---
 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

[PATCH 4/5] ls-files: illustrate bug when recursing with relative pathspec

[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 luanched
for the submodule has some context that it is operating underneath a
superproject, this error could be avoided.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 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

[PATCH 5/5] ls-files: fix bug when recuring with relative pathspec

[Brandon Williams]

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 <bmwill@google.com>
---
 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

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

[Stefan Beller]

On Fri, Feb 24, 2017 at 3:50 PM, Brandon Williams <bmwill@google.com> 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

Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec

[Duy Nguyen]

On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams <bmwill@google.com> 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.

> underneath a superproject, this error could be avoided.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  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?

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

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

[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

Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec

[Brandon Williams]

On 02/26, Duy Nguyen wrote:
> On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams <bmwill@google.com> 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

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.

> > underneath a superproject, this error could be avoided.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  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.

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

-- 
Brandon Williams

Re: [PATCH 3/5] grep: fix bug when recuring with relative pathspec

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>  	/* Add super prefix */
> +	quote_path_relative(name, opt->prefix, &buf);

Hmph, do you want a quoted version here, not just relative_path()?

Perhaps add a test with an "unusual" byte (e.g. a double-quote) in
the path?

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

Re: [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> 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 <bmwill@google.com>
> ---
>  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/",

Same comment as 3/5.  quote_path is to produce c-quote and is not
even meant for shell script quoting.  run_command() interface would
do its own shell quoting when needed, so  I think you just want the
exact string you want to pass here.

Re: [PATCH 3/5] grep: fix bug when recuring with relative pathspec

[Brandon Williams]

On 02/28, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> >  	/* Add super prefix */
> > +	quote_path_relative(name, opt->prefix, &buf);
> 
> Hmph, do you want a quoted version here, not just relative_path()?
> 
> Perhaps add a test with an "unusual" byte (e.g. a double-quote) in
> the path?

You're absolutely correct.

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

-- 
Brandon Williams

Re: [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec

[Brandon Williams]

On 02/28, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > 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 <bmwill@google.com>
> > ---
> >  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/",
> 
> Same comment as 3/5.  quote_path is to produce c-quote and is not
> even meant for shell script quoting.  run_command() interface would
> do its own shell quoting when needed, so  I think you just want the
> exact string you want to pass here.
> 

Yeah I don't know what I was thinking when using that instead of
'relative_path()'.  Will change for this and 3/5.

-- 
Brandon Williams

[RFC PATCH] grep: fix bug when recursing with relative pathspec

[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 <bmwill@google.com>
---

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

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

[Brandon Williams]

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

[PATCH v2 1/4] grep: fix help text typo

[Brandon Williams]

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

[PATCH v2 2/4] setup: allow for prefix to be passed to git commands

[Brandon Williams]

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

[PATCH v2 3/4] grep: fix bug when recursing with relative pathspec

[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 <bmwill@google.com>
---
 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

[PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec

[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 <bmwill@google.com>
---
 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

Re: [PATCH v2 2/4] setup: allow for prefix to be passed to git commands

[Johannes Schindelin]

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 ;-)

> 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`).

>  	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

Re: [PATCH v2 2/4] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/14, Johannes Schindelin wrote:
> 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 ;-)
> 

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.

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

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

Re: [PATCH v2 1/4] grep: fix help text typo

[Stefan Beller]

On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams <bmwill@google.com> wrote:

Missing SoB here, too.

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

Re: [PATCH v2 3/4] grep: fix bug when recursing with relative pathspec

[Stefan Beller]

On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams <bmwill@google.com> 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) ?

Re: [PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec

[Stefan Beller]

On Tue, Mar 14, 2017 at 3:11 PM, Brandon Williams <bmwill@google.com> 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 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 <bmwill@google.com>
> ---
>  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.

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

Re: [PATCH v2 1/4] grep: fix help text typo

[Brandon Williams]

On 03/14, Stefan Beller wrote:
> On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams <bmwill@google.com> wrote:
> 
> Missing SoB here, too.

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

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

Re: [PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec

[Brandon Williams]

On 03/14, Stefan Beller wrote:
> On Tue, Mar 14, 2017 at 3:11 PM, Brandon Williams <bmwill@google.com> 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 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 <bmwill@google.com>
> > ---
> >  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.

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

-- 
Brandon Williams

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

[Brandon Williams]

Changes in v3:
* Added sign-off to the patches where it was missing.
* slight tweak to the style and commit msgs of a few patches.
* broke up the last patch (for ls-files) into a typo fix followed by fixing the
  bug itself.  This was to make the diff easier to review.

Brandon Williams (5):
  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 typo in variable name
  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                                |  7 +++-
 t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++
 t/t7814-grep-recurse-submodules.sh     | 75 ++++++++++++++++++++++++++++++++++
 7 files changed, 167 insertions(+), 39 deletions(-)

-- 
2.12.0.367.g23dc2f6d3c-goog

[PATCH v3 1/5] grep: fix help text typo

[Brandon Williams]

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 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

[PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

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`
instead of the prefix that was potentally found during the git directory
setup process.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 cache.h | 1 +
 git.c   | 2 --
 setup.c | 7 ++++++-
 3 files changed, 7 insertions(+), 3 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..0d76b9828 100644
--- a/setup.c
+++ b/setup.c
@@ -939,9 +939,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 *prefix, *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

[PATCH v3 3/5] grep: fix bug when recursing with relative pathspec

[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 <bmwill@google.com>
---
 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

[PATCH v3 4/5] ls-files: fix typo in variable name

[Brandon Williams]

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 1c0f057d0..ca5b48db0 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;
@@ -175,17 +175,17 @@ static void show_killed_files(struct dir_struct *dir)
 static void compile_submodule_options(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");
 }
 
 /**
@@ -204,7 +204,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);
+	argv_array_pushv(&cp.args, submodule_options.argv);
 
 	/*
 	 * Pass in the original pathspec args.  The submodule will be
-- 
2.12.0.367.g23dc2f6d3c-goog

[PATCH v3 5/5] ls-files: fix bug when recursing with relative pathspec

[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 <bmwill@google.com>
---
 builtin/ls-files.c                     | 25 +++++++++++-----------
 t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index ca5b48db0..d449e46db 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -172,7 +172,9 @@ 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(&submodule_options, "-z");
@@ -186,6 +188,11 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag
 		argv_array_push(&submodule_options, "--eol");
 	if (debug_mode)
 		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);
@@ -206,15 +216,6 @@ static void show_gitlink(const struct cache_entry *ce)
 	/* add supported options */
 	argv_array_pushv(&cp.args, submodule_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);
-
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -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

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Stefan Beller]

>         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);

so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
such that e.g. aliased commands will know about the superprefix, too.

ok, sounds reasonable to me; though I do not use this feature,
so my judgement is not as good.

Do we need a test for this behavior?

Thanks,
Stefan

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Stefan Beller wrote:
> >         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);
> 
> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> such that e.g. aliased commands will know about the superprefix, too.

I don't follow, this doesn't have anything to do with super-prefix.

> 
> ok, sounds reasonable to me; though I do not use this feature,
> so my judgement is not as good.
> 
> Do we need a test for this behavior?
> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Stefan Beller]

On Fri, Mar 17, 2017 at 12:08 PM, Brandon Williams <bmwill@google.com> wrote:
> On 03/17, Stefan Beller wrote:
>> >         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);
>>
>> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
>> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
>> such that e.g. aliased commands will know about the superprefix, too.
>
> I don't follow, this doesn't have anything to do with super-prefix.
>

s/superprefix/prefix as passed in via GIT_TOPLEVEL_PREFIX_ENVIRONMENT/

sorry for the confusion.

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Stefan Beller wrote:
> On Fri, Mar 17, 2017 at 12:08 PM, Brandon Williams <bmwill@google.com> wrote:
> > On 03/17, Stefan Beller wrote:
> >> >         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);
> >>
> >> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> >> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> >> such that e.g. aliased commands will know about the superprefix, too.
> >
> > I don't follow, this doesn't have anything to do with super-prefix.
> >
> 
> s/superprefix/prefix as passed in via GIT_TOPLEVEL_PREFIX_ENVIRONMENT/
> 
> sorry for the confusion.

Alternatively we could not copy it into GIT_PREFIX_ENVIRONMENT.

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>>         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);
>
> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> such that e.g. aliased commands will know about the superprefix, too.

If the aliased commands or anything else spawned from this process
is happy with GIT_PREFIX set to the outside of the current
repository, doing this setenv() is OK.  If you are in ~/dir1, and
your repository is in ~/repos/repo1, and if you somehow had a way
to run your "git" inside ~/repos/repo1 without doing any chdir(2),
then you are essentially setting ../../dir1/ as GIT_PREFIX for that
"git" invocation (this has nothing to do with submodules).

But if your "git" is fine with GIT_PREFIX pointing outside the root
level of the working tree of the current repository like that, do we
even need a separate toplevel prefix environment, I have to wonder?

That is, if this "if TOPLEVEL_PREFIX environment is there, set it to
local variable prefix and export it as GIT_PREFIX" is expected to
work correctly for anything that would inherit that GIT_PREFIX, then
we should be able to invoke the "git" that got TOPLEVEL_PREFIX
without setting that environment, but instead setting the same value
to GIT_PREFIX and we should get the same behaviour, no?

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
> >>         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);
> >
> > so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> > first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> > such that e.g. aliased commands will know about the superprefix, too.
> 
> If the aliased commands or anything else spawned from this process
> is happy with GIT_PREFIX set to the outside of the current
> repository, doing this setenv() is OK.  If you are in ~/dir1, and
> your repository is in ~/repos/repo1, and if you somehow had a way
> to run your "git" inside ~/repos/repo1 without doing any chdir(2),
> then you are essentially setting ../../dir1/ as GIT_PREFIX for that
> "git" invocation (this has nothing to do with submodules).
> 
> But if your "git" is fine with GIT_PREFIX pointing outside the root
> level of the working tree of the current repository like that, do we
> even need a separate toplevel prefix environment, I have to wonder?
> 
> That is, if this "if TOPLEVEL_PREFIX environment is there, set it to
> local variable prefix and export it as GIT_PREFIX" is expected to
> work correctly for anything that would inherit that GIT_PREFIX, then
> we should be able to invoke the "git" that got TOPLEVEL_PREFIX
> without setting that environment, but instead setting the same value
> to GIT_PREFIX and we should get the same behaviour, no?
> 

Very true, potentially we could just use GIT_PREFIX instead of
introducing a brand new env var (which is essentially just the same
thing).  I was being cautious with this patch since git didn't currently
read GIT_PREFIX.  I was hoping other with more knowledge in this area
would voice their opinions and lead me in the right direction ;)

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> ...  I was being cautious with this patch since git didn't currently
> read GIT_PREFIX.

Ahh, I forgot about that.  Processes we spawn do expect GIT_PREFIX
to tell them where the original $cwd was and they also do expect
that "git" invoked by them would not be affected by GIT_PREFIX
environment variable.  So we cannot change that now.

If you recurse into sub-sub module, it is likely that you would want
to update the TOPLEVEL_PREFIX relative to that sub-sub module you
are descending into.

That probably also means that processes we spawn now need to also
pay attention to TOPLEVEL_PREFIX in addition to GIT_PREFIX, and we
should NOT re-export what we got from TOPLEVEL_PREFIX to GIT_PREFIX.
I.e. if a "git" process started from src/ subdirectory of the
superproject that goes into module/sub1/ submodule, top-level prefix
may export ../src/ to point at the original location, but the
process that is running in the submodule will be running at the root
level of the submodule working tree, so its prefix should be NULL or
"", no?

Adjusting pathspec and other file references on the caller's side,
instead of exporting toplevel-prefix to have them adjusted by the
callee, started to smell more and more like an easier/more correct
approach to me, but perhaps I haven't thought things deeply enough.

I dunno.

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > ...  I was being cautious with this patch since git didn't currently
> > read GIT_PREFIX.
> 
> Ahh, I forgot about that.  Processes we spawn do expect GIT_PREFIX
> to tell them where the original $cwd was and they also do expect
> that "git" invoked by them would not be affected by GIT_PREFIX
> environment variable.  So we cannot change that now.
> 

Yep.  I just ran the test suite locally doing this...it fails on a bunch
of stuff :)

> If you recurse into sub-sub module, it is likely that you would want
> to update the TOPLEVEL_PREFIX relative to that sub-sub module you
> are descending into.

So the idea would be that 'super_prefix' would keep getting updated when
recursing deeper, but the 'prefix' should stay the same (in what I'm
proposing here that is).

> 
> That probably also means that processes we spawn now need to also
> pay attention to TOPLEVEL_PREFIX in addition to GIT_PREFIX, and we
> should NOT re-export what we got from TOPLEVEL_PREFIX to GIT_PREFIX.
> I.e. if a "git" process started from src/ subdirectory of the
> superproject that goes into module/sub1/ submodule, top-level prefix
> may export ../src/ to point at the original location, but the
> process that is running in the submodule will be running at the root
> level of the submodule working tree, so its prefix should be NULL or
> "", no?

I don't think that prefix can ever have ".." in it.  From what I
understand it is always a path from the root of the repository to the
cwd that the git command was invoked by.  So in your example prefix
would be "src/".

> 
> Adjusting pathspec and other file references on the caller's side,
> instead of exporting toplevel-prefix to have them adjusted by the
> callee, started to smell more and more like an easier/more correct
> approach to me, but perhaps I haven't thought things deeply enough.
> 
> I dunno.
> 

Yeah, doing all of the adjusting on the caller's side is quite
difficult.  Getting the pathspecs and file references right seems to be
overly complicated if done by the caller.  That why I opted to punt to
the callee; given enough information (e.g. prefix and super_prefix) the
callee should be able to handle both the pathspecs and file references
properly.

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> I don't think that prefix can ever have ".." in it.  From what I
> understand it is always a path from the root of the repository to the
> cwd that the git command was invoked by.  So in your example prefix
> would be "src/".

The prefix would be NULL or "", as you will be at the root-level of
the working tree when you are running _IN_ the submodule (by
recursing into it).  Not src/, nor anything with ../ in it, I would
think.

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > I don't think that prefix can ever have ".." in it.  From what I
> > understand it is always a path from the root of the repository to the
> > cwd that the git command was invoked by.  So in your example prefix
> > would be "src/".
> 
> The prefix would be NULL or "", as you will be at the root-level of
> the working tree when you are running _IN_ the submodule (by
> recursing into it).  Not src/, nor anything with ../ in it, I would
> think.

Yes, the prefix that is found during setup of a submodule process would
be NULL or "" as the command would be invoked from the root of that
repository.  This series would sort of change that though.

If a command was invoked from 'src/' with a pathspec of '../dir/' and
there is a submodule at 'dir/sub', the process working on the submodule
will have the following:

super_prefix = 'dir/sub/'
prefix = 'src/' (Passed from the parent process via the
                 GIT_INTERNAL_TOPLEVEL_PREFIX env var)
pathspec = '../dir/'

With that information the child process will be able to properly resolve
the pathspec to be 'dir/' (using the prefix) and will be able to match
against it by pre-pending the super_prefix (e.g. dir/sub/some/file, where
some/file is a file in the submodule).  It will also be able to generate
correct output relative to the directory the command was originally
invoked from by first pre-pending the super_prefix so we have
'dir/sub/some/file' and then calling relative_path() with the prefix
that was passed in such that the output for this file looks like
'../dir/sub/some/file'

That the gist of how I'm hoping to solve the problem.  Hopefully that was
clear enough to get some feedback on.

-- 
Brandon Williams

Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec

[Duy Nguyen]

On Sat, Mar 18, 2017 at 12:22 AM, Brandon Williams <bmwill@google.com> wrote:
> 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.

This part can stand alone as a separate patch right? It would help
focus on the pathspec thingy first.

> @@ -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");

Side note. It would be awesome if you could make parse_options() (or a
new function) do the reverse process: given a 'struct option' with
valid data, spit out argv_array. Less worrying about git-grep having
new option but not passed to subgrep by accident. You can have a new
flag to tell it to ignore certain options if you don't want to pass
all.
-- 
Duy

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Yes, the prefix that is found during setup of a submodule process would
> be NULL or "" as the command would be invoked from the root of that
> repository.  This series would sort of change that though.

OK, because you (eh, rather your caller) do not adjust pathnames and
pathspecs when calling into a submodule, to the process setting at
the top-level of the submodule, the paths and pathnames given by the
end user cannot be taken as-is, but needs to be adjusted inside the
context they were given, i.e. taking account the relative location
between where the user started and where the submodule is bound in
the superproject's tree.  I forgot about that and "must be NULL at
the top, no?" was a nonsense.

> That the gist of how I'm hoping to solve the problem.  Hopefully that was
> clear enough to get some feedback on.

Yup, understood (I think).  Thanks.

Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>>         if (recurse_submodules)
>>                 argv_array_push(&submodule_options, "--recurse-submodules");
>
> Side note. It would be awesome if you could make parse_options() (or a
> new function) do the reverse process: given a 'struct option' with
> valid data, spit out argv_array. Less worrying about git-grep having
> new option but not passed to subgrep by accident. You can have a new
> flag to tell it to ignore certain options if you don't want to pass
> all.

It indeed would be awesome.  I do not offhand know if it would be
feasible or the result is easy to use without mistakes, though.

Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec

[Brandon Williams]

On 03/21, Duy Nguyen wrote:
> On Sat, Mar 18, 2017 at 12:22 AM, Brandon Williams <bmwill@google.com> wrote:
> > 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.
> 
> This part can stand alone as a separate patch right? It would help
> focus on the pathspec thingy first.

I guess it could probably be factored out, though it is necessary for it
to work.  The issue I was running into was that when no pathspecs were
given it would create a 'prefix' pathspec.  So if we were in directory
'dir/' the pathspec that would get created would be:

ps.match = "dir/"
ps.original = "dir/"

Since it also set the original field it messed up when the child tried
to interpret the pathspecs again.  I solved this by just passing the raw
pathspec through to the child.

> 
> > @@ -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");
> 
> Side note. It would be awesome if you could make parse_options() (or a
> new function) do the reverse process: given a 'struct option' with
> valid data, spit out argv_array. Less worrying about git-grep having
> new option but not passed to subgrep by accident. You can have a new
> flag to tell it to ignore certain options if you don't want to pass
> all.

I thought about this for a second but didn't pursue it very far.  Mostly
because of how you would handle options with callback routines.  Maybe
if you want the option to be reversible you need to have an additional
callback routine to do the conversion?  I agree that having this sort of
functionality would be nice as it does save you from forgetting about
passing on options to a child process.

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Stefan Beller]

On Mon, Mar 20, 2017 at 3:34 PM, Brandon Williams <bmwill@google.com> wrote:
> That the gist of how I'm hoping to solve the problem.  Hopefully that was
> clear enough to get some feedback on.

Junio wrote in  "What's cooking in git.git (Mar 2017, #10; Fri, 24)"
> I saw no particular issues myself.  Do others find this series good
> (not just "meh--it is harmless" but I want to hear a positive "yes
> these are good changes")?

So I reviewed them again, and I think they are good to go.
As a followup we may want to consider this

diff --git a/setup.c b/setup.c
index 56cd68ba93..bdc294091a 100644
--- a/setup.c
+++ b/setup.c
@@ -944,6 +944,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
         prefix = setup_git_directory_gently_1(nongit_ok);
         env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);

+        if (prefix && env_prefix)
+                die("BUG: can't invoke command in sub directory with %s set",
+                    GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
+
         if (env_prefix)
                 prefix = env_prefix;
--

Thanks,
Stefan