recursing submodules with relative pathspec (grep and ls-files)
cover recursing submodules with relative pathspec (grep and ls-files)
It was discovered that when using the --recurse-submodules flag with git grep and git ls-files and specifying a relative path when not at the root causes the child processes spawned to error out with an error like:
fatal: ..: '..' is outside repository
While true that ".." is outside the scope of the submodule repository, it probably doesn't make much sense to the user who gave that pathspec with respect to the superproject. Since the child processes that are spawned to handle the submodules have some context that they are executing underneath a superproject (via the 'super_prefix'), they should be able to prevent dying under this circumstance.
This series fixes this bug in both git grep and git ls-files as well as correctly formatting the output from submodules to handle the relative paths with "..".
One of the changes made to fix this was to add an additional flag for the parse_pathspec() function in order to treat all paths provided as being from the root of the repository. I hesitantly selected the name 'PATHSPEC_FROMROOT' but I'm not fond of it since its too similar to the pathspec magic define 'PATHSPEC_FROMTOP'. So I'm open for naming suggestions.
Brandon Williams (5):
grep: illustrate bug when recursing with relative pathspec
pathspec: add PATHSPEC_FROMROOT flag
grep: fix bug when recuring with relative pathspec
ls-files: illustrate bug when recursing with relative pathspec
ls-files: fix bug when recuring with relative pathspec
builtin/grep.c | 8 ++++-- builtin/ls-files.c | 8 ++++-- pathspec.c | 2 +- pathspec.h | 2 ++ t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++ t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++ 6 files changed, 107 insertions(+), 5 deletions(-)
Signature · 1 line
2.11.0.483.g087da7b7c-goog
PATCH 1/5 grep: illustrate bug when recursing with relative pathspec
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.
--- t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
t/t7814-grep-recurse-submodules.sh +42 -0
@@ -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" "
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec
Quoted 10 lines · 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
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.
Quoted 31 lines
underneath a superproject, this error could be avoided.
Signed-off-by Brandon Williams <[email protected]>--- t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)t/t7814-grep-recurse-submodules.sh +16
-0@@ -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" && +
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep history with moved submoules' ' · line 255
Quoted 10 lines
t/t7814-grep-recurse-submodules.sh +10
-0+ # 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?
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep history with moved submoules' ' · line 264
Quoted 9 lines
t/t7814-grep-recurse-submodules.sh +9
-0+ cat >expect <<-\EOF && + ../file:foobar + file2:foobar + ../sub/file:foobar + EOF + git -C parent/src grep --recurse-submodules -e "foobar" -- .. >actual && + test_cmp expect actual && + + # Relative path to submodule errors out
ditto
Quoted 13 lines
t/t7814-grep-recurse-submodules.sh +7
-1+ 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
Signature · 1 line
Duy
Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec
Quoted 7 lines · Duy Nguyen
Quoted 6 lines · 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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 14 lines
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_pathspeccall in0281e487fd(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.
Quoted 3 lines
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.
Quoted 10 lines
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 viaPATHSPEC_PREFIX_ORIGIN- 2013-07-14). :(icase) needs this because given a path "<prefix>/foobar", only the "foobar" part is considered case insensitive, the prefix part is always case-sensitive. For example, if you have 4 paths "abc/def", "abc/DEF", "ABC/def" and "ABC/DEF" and are standing at "abc", you would want ":(icase)def" to match the first two only, not all of them.
Hmm...yeah its a really difficult thing to get 100% correct (as I'm now noticing). It also makes it a little more challenging because you don't have the same state in both processes (child and parent). I'm thinking I may have to a little bit more work, which is more involved, to properly handle the pathspecs passed to the children. As in we may not be able to pass the raw pathspec used in the parent to the child and instead may have to do a little pre-processing on it.
Quoted 35 lines
Quoted 35 lines
underneath a superproject, this error could be avoided.
Signed-off-by Brandon Williams <[email protected]>--- t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)t/t7814-grep-recurse-submodules.sh +20
-0@@ -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
Quoted 9 lines
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep history with moved submoules' ' · line 255
Quoted 6 lines
t/t7814-grep-recurse-submodules.sh +6
-0+ sub/file:foobar + EOF + git -C parent grep --recurse-submodules -e "foobar" >actual && + test_cmp expect actual && + + # Relative path to top errors outAfter 3/5, it's not "errors out" any more, is it?
Correct, will fix the comment.
Quoted 30 lines
t/t7814-grep-recurse-submodules.sh test_expect_success 'grep history with moved submoules' ' · line 264
Quoted 9 lines
t/t7814-grep-recurse-submodules.sh +9
-0+ 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 outditto
Quoted 13 lines
t/t7814-grep-recurse-submodules.sh +7
-1+ 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
Signature · 1 line
Duy
Signature · 1 line
Brandon Williams
PATCH 2/5 pathspec: add PATHSPEC_FROMROOT flag
In reply to Brandon WilliamsAdd 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.
--- pathspec.c | 2 +- pathspec.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-)
pathspec.c +1 -1
@@ -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 {
pathspec.h +2 -0
@@ -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,
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag
pathspec.h struct pathspec { · line 70
Quoted 10 lines · Brandon Williams
Add the
PATHSPEC_FROMROOTflag 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.pathspec.h +2
-0+/* 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
PATCH 3/5 grep: fix bug when recuring with relative pathspec
In reply to Brandon WilliamsFix 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.
--- builtin/grep.c | 8 ++++++-- t/t7814-grep-recurse-submodules.sh | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-)
builtin/grep.c +6 -2
@@ -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 ;
t/t7814-grep-recurse-submodules.sh +1 -1
@@ -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 &&
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH 3/5] grep: fix bug when recuring with relative pathspec
builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 554
Quoted 2 lines · Brandon Williams
builtin/grep.c +1
-0/* 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?
Quoted 31 lines
builtin/grep.c +4
-2argv_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 ;t/t7814-grep-recurse-submodules.sh +1
-1@@ -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 3/5] grep: fix bug when recuring with relative pathspec
Quoted 9 lines · Junio C Hamano
builtin/grep.c static int grep_submodule_launch(struct grep_opt *opt, · line 554
Quoted 2 lines · Brandon Williams
builtin/grep.c +1
-0/* 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.
Quoted 32 lines
Quoted 32 lines
builtin/grep.c +4
-2argv_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 ;t/t7814-grep-recurse-submodules.sh +1
-1@@ -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 &&
Signature · 1 line
Brandon Williams
PATCH 4/5 ls-files: illustrate bug when recursing with relative pathspec
In reply to Brandon WilliamsWhen 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.
--- t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
t/t3007-ls-files-recurse-submodules.sh +50 -0
@@ -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
Signature · 1 line
2.11.0.483.g087da7b7c-goog
PATCH 5/5 ls-files: fix bug when recuring with relative pathspec
In reply to Brandon WilliamsFix 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.
--- builtin/ls-files.c | 8 ++++++-- t/t3007-ls-files-recurse-submodules.sh | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-)
builtin/ls-files.c +6 -2
@@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tagstatic 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);/*
t/t3007-ls-files-recurse-submodules.sh +1 -1
@@ -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
Signature · 1 line
2.11.0.483.g087da7b7c-goog
Re: [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec
Quoted 17 lines · 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 <[email protected]>--- builtin/ls-files.c | 8 ++++++-- t/t3007-ls-files-recurse-submodules.sh | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-)builtin/ls-files.c +0
-0
builtin/ls-files.c static void compile_submodule_options(const struct dir_struct *dir, int show_tag · line 202
Quoted 9 lines
builtin/ls-files.c +2
-0@@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tagstatic 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 5/5] ls-files: fix bug when recuring with relative pathspec
Quoted 26 lines · Junio C Hamano
Quoted 24 lines · 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 <[email protected]>--- builtin/ls-files.c | 8 ++++++-- t/t3007-ls-files-recurse-submodules.sh | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-)builtin/ls-files.c +1
-0@@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tagstatic void show_gitlink (const struct cache_entry *ce) {struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf name =STRBUF_INIT ;int status;int i;
Quoted 8 lines
builtin/ls-files.c static void compile_submodule_options(const struct dir_struct *dir, int show_tag · line 202
Quoted 2 lines
builtin/ls-files.c +1
-0+quote_path_relative (ce->name, prefix, &name); argv_array_pushf(&cp.args, "--super-prefix=%s%s/",Same comment as 3/5.
quote_pathis 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.
Signature · 1 line
Brandon Williams
Quoted 30 lines · Brandon Williams
It was discovered that when using the --recurse-submodules flag with
git grepandgit ls-filesand 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: addPATHSPEC_FROMROOTflag
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
Quoted 8 lines
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.
Signature · 1 line
Brandon Williams
RFC PATCH 1/1 grep: fix bug when recursing with relative pathspec
In reply to Brandon WilliamsWhen 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.
---
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(-)
builtin/grep.c +24 -15
@@ -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); }
git.c +0 -2
@@ -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 )
setup.c +5 -0
@@ -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 );
t/t7814-grep-recurse-submodules.sh +73 -0
79 lines not on this page · raw message
Signature · 1 line
2.12.0.rc1.440.g5b76565f74-goog
v2 cover recursing submodules with relative pathspec (grep and ls-files)
In reply to Brandon Williamsv2 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(-)
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
PATCH v2 1/4 grep: fix help text typo
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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")),
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH v2 1/4] grep: fix help text typo
On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams <[email protected]> wrote:
Missing SoB here, too.
Quoted 20 lines
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)builtin/grep.c +1
-2@@ -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 1/4] grep: fix help text typo
Quoted 3 lines · Stefan Beller
On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams <[email protected]> wrote:
Missing SoB here, too.
I guess I'm having an off day...Will fix.
Quoted 21 lines
Quoted 21 lines
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)builtin/grep.c +1
-2@@ -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
Signature · 1 line
Brandon Williams
PATCH v2 2/4 setup: allow for prefix to be passed to git commands
In reply to Brandon WilliamsIn 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(-)
cache.h +1 -0
@@ -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"
git.c +0 -2
@@ -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 )
setup.c +6 -0
@@ -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
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH v2 2/4] setup: allow for prefix to be passed to git commands
Hi Brandon,
Quoted 4 lines · 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_PREFIXif another prefix wasn't found during the git directory setup process.
Missing SOB ;-)
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 943
Quoted 9 lines
setup.c +1
-0@@ -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).
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 949
Quoted 5 lines
setup.c +4
-0prefix =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
Quoted 11 lines · Johannes Schindelin
Hi Brandon,
Quoted 4 lines · 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_PREFIXif another prefix wasn't found during the git directory setup process.Missing SOB ;-)
Thanks for that catch. I'm expecting some discussion on this patch in particular (and cc'd you since you've been working on this area of the code) so I'll make sure to add it in the next reroll.
Quoted 4 lines
Quoted 4 lines
setup.c +0
-0
Quoted 8 lines
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 943
Quoted 5 lines
setup.c +1
-0@@ -940,8 +940,14 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)const char *setup_git_directory_gently (int *nongit_ok) {const char *prefix; +const char *env_prefix;I'd just append this to the previous line (
const char *prefix, *env_prefix).
That would be cleaner.
Quoted 11 lines
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 949
Quoted 6 lines
setup.c +4
-0prefix =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_prefixis used if no other prefix was found, but this code ignores any prefix if the environment variable was set.
Quoted 1 line
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.
Quoted 3 lines
Ciao,
Johannes
Signature · 1 line
Brandon Williams
PATCH v2 3/4 grep: fix bug when recursing with relative pathspec
In reply to Brandon WilliamsWhen 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.
--- builtin/grep.c | 39 ++++++++++++-------- t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 15 deletions(-)
builtin/grep.c +24 -15
@@ -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); }
t/t7814-grep-recurse-submodules.sh +75 -0
81 lines not on this page · raw message
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH v2 3/4] grep: fix bug when recursing with relative pathspec
Quoted 6 lines · Brandon Williams
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) ?
PATCH v2 4/4 ls-files: fix bug when recursing with relative pathspec
In reply to Brandon WilliamsWhen 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.
--- builtin/ls-files.c | 41 +++++++++++++++++----------------- t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 20 deletions(-)
builtin/ls-files.c +21 -20
@@ -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 ||
t/t3007-ls-files-recurse-submodules.sh +39 -0
@@ -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
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec
Quoted 59 lines · 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 <[email protected]>--- builtin/ls-files.c | 41 +++++++++++++++++----------------- t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 20 deletions(-)builtin/ls-files.c +6
-5@@ -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" );
builtin/ls-files.c static void show_killed_files(struct dir_struct *dir) · line 190
Quoted 10 lines
builtin/ls-files.c +4
-3+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.
Quoted 6 lines
builtin/ls-files.c +5
-0+ +/* Add Pathspecs */ +argv_array_push (&submodule_options,"--" ); +for (; *argv; argv++) +argv_array_push (&submodule_options, *argv); }
Re: [PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec
Quoted 66 lines · Stefan Beller
Quoted 65 lines · 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 <[email protected]>--- builtin/ls-files.c | 41 +++++++++++++++++----------------- t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 20 deletions(-)builtin/ls-files.c +8
-7@@ -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" );
Quoted 8 lines
builtin/ls-files.c static void show_killed_files(struct dir_struct *dir) · line 190
Quoted 4 lines
builtin/ls-files.c +2
-1+argv_array_push (&submodule_options,"--eol" );if (debug_mode) -argv_array_push (&submodules_options,"--debug" ); +argv_array_push (&submodule_options,"--debug" );Up to here we only rename a variable? If you want to help reviewers, please separate this into two patches. One refactoring, stating it doesn't change behavior; and the other adding the behavioral changes.
I can do that.
Quoted 7 lines
Quoted 7 lines
builtin/ls-files.c +5
-0+ +/* Add Pathspecs */ +argv_array_push (&submodule_options,"--" ); +for (; *argv; argv++) +argv_array_push (&submodule_options, *argv); }
Signature · 1 line
Brandon Williams
v3 cover recursing submodules with relative pathspec (grep and ls-files)
In reply to Brandon WilliamsChanges 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(-)
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
PATCH v3 1/5 grep: fix help text typo
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -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")),
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
PATCH v3 2/5 setup: allow for prefix to be passed to git commands
In reply to Brandon WilliamsIn 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.
--- cache.h | 1 + git.c | 2 -- setup.c | 7 ++++++- 3 files changed, 7 insertions(+), 3 deletions(-)
cache.h +1 -0
@@ -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"
git.c +0 -2
@@ -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 )
setup.c +6 -1
@@ -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
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 951
Quoted 8 lines
prefix = setup_git_directory_gently_1(nongit_ok);setup.c +5
-0+ 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
Quoted 4 lines · Stefan Beller
Quoted 4 lines
prefix = setup_git_directory_gently_1(nongit_ok);setup.c +3
-0+ env_prefix =getenv (GIT_TOPLEVEL_PREFIX_ENVIRONMENT ); + +if (env_prefix)
Quoted 8 lines
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 951
Quoted 4 lines
setup.c +2
-0+ prefix = env_prefix; +if (prefix)setenv (GIT_PREFIX_ENVIRONMENT , prefix,1 );so we load that
GIT_TOPLEVEL_PREFIX_ENVIRONMENTprefix first, such that we essentially copy it intoGIT_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.
Quoted 8 lines
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
Signature · 1 line
Brandon Williams
Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands
Quoted 14 lines · Brandon Williams
Quoted 4 lines · Stefan Beller
Quoted 4 lines
prefix = setup_git_directory_gently_1(nongit_ok);setup.c +3
-0+ env_prefix =getenv (GIT_TOPLEVEL_PREFIX_ENVIRONMENT ); + +if (env_prefix)Quoted 8 lines
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 951
Quoted 4 lines
setup.c +2
-0+ prefix = env_prefix; +if (prefix)setenv (GIT_PREFIX_ENVIRONMENT , prefix,1 );so we load that
GIT_TOPLEVEL_PREFIX_ENVIRONMENTprefix first, such that we essentially copy it intoGIT_PREFIX_ENVIRONMENT, such that e.g. aliased commands will know about the superprefix, too.
Quoted 2 lines
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
Quoted 15 lines · Stefan Beller
Quoted 14 lines · Brandon Williams
Quoted 12 lines · Stefan Beller
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 951
Quoted 8 lines
prefix = setup_git_directory_gently_1(nongit_ok);setup.c +5
-0+ 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_ENVIRONMENTprefix first, such that we essentially copy it intoGIT_PREFIX_ENVIRONMENT, such that e.g. aliased commands will know about the superprefix, too.
Quoted 6 lines
Quoted 2 lines
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.
Signature · 1 line
Brandon Williams
Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands
In reply to Stefan BellerQuoted 4 lines · Stefan Beller
Quoted 4 lines
prefix = setup_git_directory_gently_1(nongit_ok);setup.c +3
-0+ env_prefix =getenv (GIT_TOPLEVEL_PREFIX_ENVIRONMENT ); + +if (env_prefix)
Quoted 8 lines
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 951
Quoted 4 lines
setup.c +2
-0+ prefix = env_prefix; +if (prefix)setenv (GIT_PREFIX_ENVIRONMENT , prefix,1 );so we load that
GIT_TOPLEVEL_PREFIX_ENVIRONMENTprefix first, such that we essentially copy it intoGIT_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
Quoted 27 lines · Junio C Hamano
Quoted 4 lines · Stefan Beller
Quoted 4 lines
prefix = setup_git_directory_gently_1(nongit_ok);setup.c +3
-0+ env_prefix =getenv (GIT_TOPLEVEL_PREFIX_ENVIRONMENT ); + +if (env_prefix)Quoted 8 lines
setup.c static const char *setup_git_directory_gently_1(int *nongit_ok) · line 951
Quoted 4 lines
setup.c +2
-0+ prefix = env_prefix; +if (prefix)setenv (GIT_PREFIX_ENVIRONMENT , prefix,1 );so we load that
GIT_TOPLEVEL_PREFIX_ENVIRONMENTprefix first, such that we essentially copy it intoGIT_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_PREFIXset to the outside of the current repository, doing thissetenv()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/ asGIT_PREFIXfor that "git" invocation (this has nothing to do with submodules).But if your "git" is fine with
GIT_PREFIXpointing 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?
Quoted 7 lines
That is, if this "if
TOPLEVEL_PREFIXenvironment is there, set it to local variable prefix and export it asGIT_PREFIX" is expected to work correctly for anything that would inherit thatGIT_PREFIX, then we should be able to invoke the "git" that gotTOPLEVEL_PREFIXwithout setting that environment, but instead setting the same value toGIT_PREFIXand 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 ;)
Signature · 1 line
Brandon Williams
Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands
Quoted 2 lines · Brandon Williams
... 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
Quoted 10 lines · Junio C Hamano
Quoted 2 lines · Brandon Williams
... 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_PREFIXto tell them where the original $cwd was and they also do expect that "git" invoked by them would not be affected byGIT_PREFIXenvironment variable. So we cannot change that now.
Yep. I just ran the test suite locally doing this...it fails on a bunch of stuff :)
Quoted 3 lines
If you recurse into sub-sub module, it is likely that you would want to update the
TOPLEVEL_PREFIXrelative 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).
Quoted 10 lines
That probably also means that processes we spawn now need to also pay attention to
TOPLEVEL_PREFIXin addition toGIT_PREFIX, and we should NOT re-export what we got fromTOPLEVEL_PREFIXtoGIT_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/".
Quoted 8 lines
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.
Signature · 1 line
Brandon Williams
Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands
Quoted 4 lines · Brandon Williams
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
Quoted 11 lines · Junio C Hamano
Quoted 4 lines · Brandon Williams
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.
Signature · 1 line
Brandon Williams
Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands
Quoted 3 lines · Brandon Williams
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.
Quoted 2 lines
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 2/5] setup: allow for prefix to be passed to git commands
In reply to Brandon WilliamsQuoted 2 lines · Brandon Williams
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)"
Quoted 3 lines
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
setup.c +4 -1
@@ -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
PATCH v3 3/5 grep: fix bug when recursing with relative pathspec
In reply to Brandon WilliamsWhen 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.
--- builtin/grep.c | 39 ++++++++++++-------- t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 15 deletions(-)
builtin/grep.c +24 -15
@@ -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); }
t/t7814-grep-recurse-submodules.sh +75 -0
81 lines not on this page · raw message
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec
Quoted 4 lines · Brandon Williams
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.
Quoted 5 lines
builtin/grep.c +0
-1@@ -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,
builtin/grep.c static void run_pager(struct grep_opt *opt, const char *prefix) · line 416
Quoted 10 lines
builtin/grep.c +1
-1+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.
Signature · 1 line
Duy
Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec
Quoted 9 lines · Duy Nguyen
Quoted 2 lines
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 outargv_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
In reply to Duy NguyenQuoted 8 lines · Duy Nguyen
Quoted 4 lines · Brandon Williams
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.
Quoted 14 lines
Quoted 14 lines
builtin/grep.c +1
-2@@ -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;
Quoted 9 lines
builtin/grep.c static void run_pager(struct grep_opt *opt, const char *prefix) · line 416
Quoted 2 lines
builtin/grep.c +0
-0if (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 outargv_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.
Signature · 1 line
Brandon Williams
PATCH v3 4/5 ls-files: fix typo in variable name
In reply to Brandon Williams--- builtin/ls-files.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
builtin/ls-files.c +8 -8
@@ -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
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
PATCH v3 5/5 ls-files: fix bug when recursing with relative pathspec
In reply to Brandon WilliamsWhen 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.
--- builtin/ls-files.c | 25 +++++++++++----------- t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 12 deletions(-)
builtin/ls-files.c +13 -12
@@ -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_tagargv_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 ||
t/t3007-ls-files-recurse-submodules.sh +39 -0
@@ -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
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog