{"thread":{"id":"45217","subject":"[PATCH 1/5] grep: illustrate bug when recursing with relative pathspec","startedAt":"2017-02-24T23:51:15Z","lastAt":"2017-03-28T23:58:35Z","messageCount":48,"participants":["Brandon Williams","Stefan Beller","Duy Nguyen","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"312584","messageId":"20170224235100.52627-2-bmwill@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"[PATCH 1/5] grep: illustrate bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-24T23:50:56Z","receivedAt":"2017-02-24T23:51:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When using the --recurse-submodules flag with a relative pathspec which\nincludes \"..\", an error is produced inside the child process spawned for\na submodule. When creating the pathspec struct in the child, the \"..\"\nis interpreted to mean \"go up a directory\" which causes an error stating\nthat the path \"..\" is outside of the repository.\n\nWhile it is true that \"..\" is outside the scope of the submodule, it is\nconfusing to a user who originally invoked the command where \"..\" was\nindeed still inside the scope of the superproject. Since the child\nprocess luanched for the submodule has some context that it is operating\nunderneath a superproject, this error could be avoided.\n\nSigned-off-by: Brandon Williams \n---\n t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 67247a01d..418ba68fe 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'grep using relative path' '\n+\ttest_when_finished \"rm -rf parent sub\" &&\n+\tgit init sub &&\n+\techo \"foobar\" >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\n+\tgit init parent &&\n+\techo \"foobar\" >parent/file &&\n+\tgit -C parent add file &&\n+\tmkdir parent/src &&\n+\techo \"foobar\" >parent/src/file2 &&\n+\tgit -C parent add src/file2 &&\n+\tgit -C parent submodule add ../sub &&\n+\tgit -C parent commit -m \"add files and submodule\" &&\n+\n+\t# From top works\n+\tcat >expect <<-\\EOF &&\n+\tfile:foobar\n+\tsrc/file2:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to top errors out\n+\tcat >expect <<-\\EOF &&\n+\t../file:foobar\n+\tfile2:foobar\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule errors out\n+\tcat >expect <<-\\EOF &&\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- ../sub >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_incompatible_with_recurse_submodules ()\n {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312585","messageId":"20170224235100.52627-3-bmwill@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"[PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-24T23:50:57Z","receivedAt":"2017-02-24T23:51:17Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Add the `PATHSPEC_FROMROOT` flag to allow callers to instruct\n'parse_pathspec()' that all provided pathspecs are relative to the root\nof the repository. This allows a caller to prevent a path that may be\noutside of the repository from erroring out during the pathspec struct\nconstruction.\n\nSigned-off-by: Brandon Williams \n---\n pathspec.c | 2 +-\n pathspec.h | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 7ababb315..ff622ba18 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -353,7 +353,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \tif (pathspec_prefix >= 0) {\n \t\tmatch = xstrdup(copyfrom);\n \t\tprefixlen = pathspec_prefix;\n-\t} else if (magic & PATHSPEC_FROMTOP) {\n+\t} else if ((magic & PATHSPEC_FROMTOP) || (flags & PATHSPEC_FROMROOT)) {\n \t\tmatch = xstrdup(copyfrom);\n \t\tprefixlen = 0;\n \t} else {\ndiff --git a/pathspec.h b/pathspec.h\nindex 49fd823dd..cad197436 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -66,6 +66,8 @@ struct pathspec {\n * allowed, then it will automatically set for every pathspec.\n */\n #define PATHSPEC_LITERAL_PATH (1<<8)\n+/* For callers that know all paths are relative to the root of the repository */\n+#define PATHSPEC_FROMROOT (1<<9)\n \n extern void parse_pathspec(struct pathspec *pathspec,\n \t\t\t unsigned magic_mask,\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312586","messageId":"20170224235100.52627-1-bmwill@google.com","threadId":"45217","inReplyTo":null,"subject":"[PATCH 0/5] recursing submodules with relative pathspec (grep and ls-files)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-24T23:50:55Z","receivedAt":"2017-02-24T23:51:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"It was discovered that when using the --recurse-submodules flag with `git grep`\nand `git ls-files` and specifying a relative path when not at the root causes\nthe child processes spawned to error out with an error like:\n\nfatal: ..: '..' is outside repository\n\nWhile true that \"..\" is outside the scope of the submodule repository, it\nprobably doesn't make much sense to the user who gave that pathspec with\nrespect to the superproject. Since the child processes that are spawned to\nhandle the submodules have some context that they are executing underneath a\nsuperproject (via the 'super_prefix'), they should be able to prevent dying\nunder this circumstance.\n\nThis series fixes this bug in both git grep and git ls-files as well as\ncorrectly formatting the output from submodules to handle the relative paths\nwith \"..\".\n\nOne of the changes made to fix this was to add an additional flag for the\nparse_pathspec() function in order to treat all paths provided as being from\nthe root of the repository. I hesitantly selected the name 'PATHSPEC_FROMROOT'\nbut I'm not fond of it since its too similar to the pathspec magic define\n'PATHSPEC_FROMTOP'. So I'm open for naming suggestions.\n\nBrandon Williams (5):\n grep: illustrate bug when recursing with relative pathspec\n pathspec: add PATHSPEC_FROMROOT flag\n grep: fix bug when recuring with relative pathspec\n ls-files: illustrate bug when recursing with relative pathspec\n ls-files: fix bug when recuring with relative pathspec\n\n builtin/grep.c | 8 ++++--\n builtin/ls-files.c | 8 ++++--\n pathspec.c | 2 +-\n pathspec.h | 2 ++\n t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++\n t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++\n 6 files changed, 107 insertions(+), 5 deletions(-)\n\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312587","messageId":"20170224235100.52627-4-bmwill@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"[PATCH 3/5] grep: fix bug when recuring with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-24T23:50:58Z","receivedAt":"2017-02-24T23:51:21Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Fix a bug which causes a child process for a submodule to error out when\na relative pathspec with a \"..\" is provided in the superproject.\n\nWhile at it, correctly construct the super-prefix to be used in a\nsubmodule when not at the root of the repository.\n\nSigned-off-by: Brandon Williams \n---\n builtin/grep.c | 8 ++++++--\n t/t7814-grep-recurse-submodules.sh | 2 +-\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2c727ef49..65f3413d1 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -538,6 +538,7 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \tint status, i;\n \tconst char *end_of_base;\n \tconst char *name;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct work_item *w = opt->output_priv;\n \n \tend_of_base = strchr(gs->name, ':');\n@@ -550,9 +551,11 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n \n \t/* Add super prefix */\n+\tquote_path_relative(name, opt->prefix, &buf);\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n-\t\t\t name);\n+\t\t\t buf.buf);\n+\tstrbuf_release(&buf);\n \targv_array_push(&cp.args, \"grep\");\n \n \t/*\n@@ -1199,7 +1202,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_CWD |\n-\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n+\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |\n+\t\t (super_prefix ? PATHSPEC_FROMROOT : 0),\n \t\t prefix, argv + i);\n \tpathspec.max_depth = opt.max_depth;\n \tpathspec.recursive = 1;\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 418ba68fe..e0932b2b7 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -227,7 +227,7 @@ test_expect_success 'grep history with moved submoules' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'grep using relative path' '\n+test_expect_success 'grep using relative path' '\n \ttest_when_finished \"rm -rf parent sub\" &&\n \tgit init sub &&\n \techo \"foobar\" >sub/file &&\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312588","messageId":"20170224235100.52627-6-bmwill@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"[PATCH 5/5] ls-files: fix bug when recuring with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-24T23:51:00Z","receivedAt":"2017-02-24T23:51:23Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Fix a bug which causes a child process for a submodule to error out when a\nrelative pathspec with a \"..\" is provided in the superproject.\n\nWhile at it, correctly construct the super-prefix to be used in a submodule\nwhen not at the root of the repository.\n\nSigned-off-by: Brandon Williams \n---\n builtin/ls-files.c | 8 ++++++--\n t/t3007-ls-files-recurse-submodules.sh | 2 +-\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 159229081..89533ab8e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag\n static void show_gitlink(const struct cache_entry *ce)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf name = STRBUF_INIT;\n \tint status;\n \tint i;\n \n+\tquote_path_relative(ce->name, prefix, &name);\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n-\t\t\t ce->name);\n+\t\t\t name.buf);\n+\tstrbuf_release(&name);\n \targv_array_push(&cp.args, \"ls-files\");\n \targv_array_push(&cp.args, \"--recurse-submodules\");\n \n@@ -615,7 +618,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \n \tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_CWD |\n-\t\t PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n+\t\t PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n+\t\t (super_prefix ? PATHSPEC_FROMROOT : 0),\n \t\t prefix, argv);\n \n \t/*\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex d8ab10866..1522ed235 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -188,7 +188,7 @@ test_expect_success '--recurse-submodules and pathspecs' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure '--recurse-submodules and relative paths' '\n+test_expect_success '--recurse-submodules and relative paths' '\n \t# From top works\n \tcat >expect <<-\\EOF &&\n \t.gitmodules\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312589","messageId":"20170224235100.52627-5-bmwill@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"[PATCH 4/5] ls-files: illustrate bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-24T23:50:59Z","receivedAt":"2017-02-24T23:51:39Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When using the --recurse-submodules flag with a relative pathspec which\nincludes \"..\", an error is produced inside the child process spawned for a\nsubmodule. When creating the pathspec struct in the child, the \"..\" is\ninterpreted to mean \"go up a directory\" which causes an error stating that the\npath \"..\" is outside of the repository.\n\nWhile it is true that \"..\" is outside the scope of the submodule, it is\nconfusing to a user who originally invoked the command where \"..\" was indeed\nstill inside the scope of the superproject. Since the child process luanched\nfor the submodule has some context that it is operating underneath a\nsuperproject, this error could be avoided.\n\nSigned-off-by: Brandon Williams \n---\n t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++\n 1 file changed, 50 insertions(+)\n\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex a5426171d..d8ab10866 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -188,6 +188,56 @@ test_expect_success '--recurse-submodules and pathspecs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure '--recurse-submodules and relative paths' '\n+\t# From top works\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\th.txt\n+\tsib/file\n+\tsub/file\n+\tsubmodule/.gitmodules\n+\tsubmodule/c\n+\tsubmodule/f.TXT\n+\tsubmodule/g.txt\n+\tsubmodule/subsub/d\n+\tsubmodule/subsub/e.txt\n+\tEOF\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to top errors out\n+\tcat >expect <<-\\EOF &&\n+\t../.gitmodules\n+\t../a\n+\tb\n+\t../h.txt\n+\t../sib/file\n+\t../sub/file\n+\t../submodule/.gitmodules\n+\t../submodule/c\n+\t../submodule/f.TXT\n+\t../submodule/g.txt\n+\t../submodule/subsub/d\n+\t../submodule/subsub/e.txt\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules -- .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule errors out\n+\tcat >expect <<-\\EOF &&\n+\t../submodule/.gitmodules\n+\t../submodule/c\n+\t../submodule/f.TXT\n+\t../submodule/g.txt\n+\t../submodule/subsub/d\n+\t../submodule/subsub/e.txt\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules -- ../submodule >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--recurse-submodules does not support --error-unmatch' '\n \ttest_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&\n \ttest_i18ngrep \"does not support --error-unmatch\" actual\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312592","messageId":"CAGZ79kYRrAmJ+=563PBkdiBwaUzqqQOMVANT_fTvURFv+=Yiog@mail.gmail.com","threadId":"45217","inReplyTo":"20170224235100.52627-3-bmwill@google.com","subject":"Re: [PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-25T00:31:17Z","receivedAt":"2017-02-25T00:38:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 24, 2017 at 3:50 PM, Brandon Williams wrote:\n> Add the `PATHSPEC_FROMROOT` flag to allow callers to instruct\n> 'parse_pathspec()' that all provided pathspecs are relative to the root\n> of the repository. This allows a caller to prevent a path that may be\n> outside of the repository from erroring out during the pathspec struct\n> construction.\n>\n\n\n> +/* For callers that know all paths are relative to the root of the repository */\n> +#define PATHSPEC_FROMROOT (1<<9)\n\nWhat is the calling convention for these submodule pathspecs?\nIIRC, we'd pass on the super-prefix and the literal pathspec,\ne.g. when there is a submodule \"sub\" inside the superproject,\nThe invocation on the submodule would be\n\n git FOO --super-prefix=\"sub\" -- sub/pathspec/inside/...\n\nand then the submodule process would need to \"subtract\" the superprefix\nfrom the pathspec argument to see its actual pathspec, e.g. in gerrit:\n\n$ GIT_TRACE=1 git grep --recurse-submodules -i test -- \\\n plugins/cookbook-plugin/\n...\n\ntrace: run_command: '--super-prefix=plugins/cookbook-plugin/' 'grep' \\\n '--recurse-submodules' '-i' '-etest' '--threads=4' '--'\n'plugins/cookbook-plugin/'\n..\nbut also:\n...\n trace: run_command: '--super-prefix=plugins/download-commands/' 'grep' \\\n '--recurse-submodules' '-i' '-etest' '--threads=4' '--'\n'plugins/cookbook-plugin/'\n...\n\nSo if I change into a directory:\n\n$ cd plugins\nplugins$ git grep --recurse-submodules -i test -- cookbook-plugin/\nplugins$ #empty?\nplugins$ git grep --recurse-submodules -i test -- plugins/cookbook-plugin/\n...\nUsual output, so the pathspecs are absolute path to the superprojects\nroot? Let's try relative path:\nplugins$ git grep --recurse-submodules -i test -- ../plugins/cookbook-plugin/\nfatal: ../plugins/cookbook-plugin/: '../plugins/cookbook-plugin/' is\noutside repository\n...\nRunning with GIT_TRACE=1:\n\ntrace: run_command: '--super-prefix=plugins/cookbook-plugin/' 'grep' \\\n '--recurse-submodules' '-i' '-etest' '--threads=4' '--'\n'../plugins/cookbook-plugin/'\n\nthat seems like a mismatch of pathspec and superproject prefix, the prefix ought\nto be different then? Maybe also including ../ because that is the\nrelative path from\ncwd to the superporojects root and that is where we anchor all paths?\n\nEasy to test that out:\n\nplugins$ GIT_TRACE=1 git --super-prefix=../ grep --recurse-submodules \\\n -i test -- ../plugins/cookbook-plugin/\nfatal: can't use --super-prefix from a subdirectory\n\nok, not as easy. :/\n\nSo another test with relative path:\n(in git.git)\n\ncd t/diff-lib\nt/diff-lib$ git grep freedom\nCOPYING:freedom to share and change it. By contrast, the GNU General Public\n...\n\nSo the path displayed is relative to the cwd (and the search results as well)\nIn the submodule case we would expect to have the super prefix\nto be computed to be relative to the cwd?\n\nChecking the tests, this is handled correctly with this patch series. :)\n\nBut nevertheless, I think I know why I dislike this approach now:\nThe super prefix is handled \"too dumb\" IMHO, see the case\n plugins$ git grep test -- cookbook-plugin/\nabove, that doesn't correctly figure out the correct output.\nAlthough this might be a separate bug, but it sounds like it\nis the same underlying issue.\n\n--\nfor the naming: How about PATHSPEC_FROMOUTSIDE\nwhen going with the series as is here?\n(the superprefix is not resolved, so the pathspecs given are\nliterally pathspecs that are outside this repo and we can ignore\nthem?\n\nThanks,\nStefan\n"},{"id":"312682","messageId":"CACsJy8Df6hhTzx5BNx385T4cuCg5w3nvOioXB=q+NCoW9kA6_w@mail.gmail.com","threadId":"45217","inReplyTo":"20170224235100.52627-2-bmwill@google.com","subject":"Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-26T09:53:55Z","receivedAt":"2017-02-26T10:03:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams wrote:\n> When using the --recurse-submodules flag with a relative pathspec which\n> includes \"..\", an error is produced inside the child process spawned for\n> a submodule. When creating the pathspec struct in the child, the \"..\"\n> is interpreted to mean \"go up a directory\" which causes an error stating\n> that the path \"..\" is outside of the repository.\n>\n> While it is true that \"..\" is outside the scope of the submodule, it is\n> confusing to a user who originally invoked the command where \"..\" was\n> indeed still inside the scope of the superproject. Since the child\n> process luanched for the submodule has some context that it is operating\n\ns/luanched/launched/\n\nI would prefer 1/5 t to be merged with 3/5 though. The problem\ndescription is very light there, and the test demonstration in the\ndiff is simply switching from failure to success, which forces the\nreader to come back here. It's easier to find here now, but it'll be a\nbit harder when it enters master and we have to read it from git-log,\nI think.\n\nI'm still munching through the super-prefix patches. From how you\nchanged match_pathspec call in 0281e487fd (grep: optionally recurse\ninto submodules - 2016-12-16), I guess pathspecs should be handled\nwith super-prefix instead of the submodule's prefix (which is empty\nanyway, I guess). The right solution wrt. handling relative paths may\nbe teach pathspec about super-prefix (and even original super's cwd)\nthen let it processes path in supermodule's context.\n\nDoes it handle relative paths with wildcards correctly btw? Ones that\ncross submodules? I have a feeling it doesn't, but I haven't seen how\nexactly super-prefix works yet.\n\nThere's another problem with passing pathspec from one process to\nanother. The issue with preserving the prefix, see 233c3e6c59\n(parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN -\n2013-07-14). :(icase) needs this because given a path\n\"/foobar\", only the \"foobar\" part is considered case\ninsensitive, the prefix part is always case-sensitive. For example, if\nyou have 4 paths \"abc/def\", \"abc/DEF\", \"ABC/def\" and \"ABC/DEF\" and are\nstanding at \"abc\", you would want \":(icase)def\" to match the first two\nonly, not all of them.\n\n> underneath a superproject, this error could be avoided.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 42 insertions(+)\n>\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index 67247a01d..418ba68fe 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_failure 'grep using relative path' '\n> + test_when_finished \"rm -rf parent sub\" &&\n> + git init sub &&\n> + echo \"foobar\" >sub/file &&\n> + git -C sub add file &&\n> + git -C sub commit -m \"add file\" &&\n> +\n> + git init parent &&\n> + echo \"foobar\" >parent/file &&\n> + git -C parent add file &&\n> + mkdir parent/src &&\n> + echo \"foobar\" >parent/src/file2 &&\n> + git -C parent add src/file2 &&\n> + git -C parent submodule add ../sub &&\n> + git -C parent commit -m \"add files and submodule\" &&\n> +\n> + # From top works\n> + cat >expect <<-\\EOF &&\n> + file:foobar\n> + src/file2:foobar\n> + sub/file:foobar\n> + EOF\n> + git -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n> + test_cmp expect actual &&\n> +\n> + # Relative path to top errors out\n\nAfter 3/5, it's not \"errors out\" any more, is it?\n\n> + cat >expect <<-\\EOF &&\n> + ../file:foobar\n> + file2:foobar\n> + ../sub/file:foobar\n> + EOF\n> + git -C parent/src grep --recurse-submodules -e \"foobar\" -- .. >actual &&\n> + test_cmp expect actual &&\n> +\n> + # Relative path to submodule errors out\n\nditto\n\n> + cat >expect <<-\\EOF &&\n> + ../sub/file:foobar\n> + EOF\n> + git -C parent/src grep --recurse-submodules -e \"foobar\" -- ../sub >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_incompatible_with_recurse_submodules ()\n> {\n> test_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n> --\n> 2.11.0.483.g087da7b7c-goog\n>\n\n\n\n-- \nDuy\n"},{"id":"312748","messageId":"20170227181437.GB153455@google.com","threadId":"45217","inReplyTo":"CACsJy8Df6hhTzx5BNx385T4cuCg5w3nvOioXB=q+NCoW9kA6_w@mail.gmail.com","subject":"Re: [PATCH 1/5] grep: illustrate bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-27T18:14:37Z","receivedAt":"2017-02-27T18:16:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/26, Duy Nguyen wrote:\n> On Sat, Feb 25, 2017 at 6:50 AM, Brandon Williams wrote:\n> > When using the --recurse-submodules flag with a relative pathspec which\n> > includes \"..\", an error is produced inside the child process spawned for\n> > a submodule. When creating the pathspec struct in the child, the \"..\"\n> > is interpreted to mean \"go up a directory\" which causes an error stating\n> > that the path \"..\" is outside of the repository.\n> >\n> > While it is true that \"..\" is outside the scope of the submodule, it is\n> > confusing to a user who originally invoked the command where \"..\" was\n> > indeed still inside the scope of the superproject. Since the child\n> > process luanched for the submodule has some context that it is operating\n> \n> s/luanched/launched/\n> \n> I would prefer 1/5 t to be merged with 3/5 though. The problem\n\nWe can definitely merge them and I agree that it may be easier for\nlooking through the logs if they are merged.\n\n> description is very light there, and the test demonstration in the\n> diff is simply switching from failure to success, which forces the\n> reader to come back here. It's easier to find here now, but it'll be a\n> bit harder when it enters master and we have to read it from git-log,\n> I think.\n> \n> I'm still munching through the super-prefix patches. From how you\n> changed match_pathspec call in 0281e487fd (grep: optionally recurse\n> into submodules - 2016-12-16), I guess pathspecs should be handled\n> with super-prefix instead of the submodule's prefix (which is empty\n> anyway, I guess). The right solution wrt. handling relative paths may\n> be teach pathspec about super-prefix (and even original super's cwd)\n> then let it processes path in supermodule's context.\n> \n> Does it handle relative paths with wildcards correctly btw? Ones that\n> cross submodules? I have a feeling it doesn't, but I haven't seen how\n> exactly super-prefix works yet.\n\nI'm not 100% sure about the relative paths with wildcards. I did notice\nthat this series solves one problem and introduces another (recursing\nfrom a subdirectory doesn't work with this series) so I need to\nrethink the solution a little bit. And I'll take into account wildcards\nas well.\n\n> \n> There's another problem with passing pathspec from one process to\n> another. The issue with preserving the prefix, see 233c3e6c59\n> (parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN -\n> 2013-07-14). :(icase) needs this because given a path\n> \"/foobar\", only the \"foobar\" part is considered case\n> insensitive, the prefix part is always case-sensitive. For example, if\n> you have 4 paths \"abc/def\", \"abc/DEF\", \"ABC/def\" and \"ABC/DEF\" and are\n> standing at \"abc\", you would want \":(icase)def\" to match the first two\n> only, not all of them.\n\nHmm...yeah its a really difficult thing to get 100% correct (as I'm now\nnoticing). It also makes it a little more challenging because you don't\nhave the same state in both processes (child and parent). I'm thinking\nI may have to a little bit more work, which is more involved, to\nproperly handle the pathspecs passed to the children. As in we may not\nbe able to pass the raw pathspec used in the parent to the child and\ninstead may have to do a little pre-processing on it.\n\n> > underneath a superproject, this error could be avoided.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++++++++++++\n> > 1 file changed, 42 insertions(+)\n> >\n> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> > index 67247a01d..418ba68fe 100755\n> > --- a/t/t7814-grep-recurse-submodules.sh\n> > +++ b/t/t7814-grep-recurse-submodules.sh\n> > @@ -227,6 +227,48 @@ test_expect_success 'grep history with moved submoules' '\n> > test_cmp expect actual\n> > '\n> >\n> > +test_expect_failure 'grep using relative path' '\n> > + test_when_finished \"rm -rf parent sub\" &&\n> > + git init sub &&\n> > + echo \"foobar\" >sub/file &&\n> > + git -C sub add file &&\n> > + git -C sub commit -m \"add file\" &&\n> > +\n> > + git init parent &&\n> > + echo \"foobar\" >parent/file &&\n> > + git -C parent add file &&\n> > + mkdir parent/src &&\n> > + echo \"foobar\" >parent/src/file2 &&\n> > + git -C parent add src/file2 &&\n> > + git -C parent submodule add ../sub &&\n> > + git -C parent commit -m \"add files and submodule\" &&\n> > +\n> > + # From top works\n> > + cat >expect <<-\\EOF &&\n> > + file:foobar\n> > + src/file2:foobar\n> > + sub/file:foobar\n> > + EOF\n> > + git -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n> > + test_cmp expect actual &&\n> > +\n> > + # Relative path to top errors out\n> \n> After 3/5, it's not \"errors out\" any more, is it?\n> \n\nCorrect, will fix the comment.\n\n> > + cat >expect <<-\\EOF &&\n> > + ../file:foobar\n> > + file2:foobar\n> > + ../sub/file:foobar\n> > + EOF\n> > + git -C parent/src grep --recurse-submodules -e \"foobar\" -- .. >actual &&\n> > + test_cmp expect actual &&\n> > +\n> > + # Relative path to submodule errors out\n> \n> ditto\n> \n> > + cat >expect <<-\\EOF &&\n> > + ../sub/file:foobar\n> > + EOF\n> > + git -C parent/src grep --recurse-submodules -e \"foobar\" -- ../sub >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n> > test_incompatible_with_recurse_submodules ()\n> > {\n> > test_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n> > --\n> > 2.11.0.483.g087da7b7c-goog\n> >\n> \n> \n> \n> -- \n> Duy\n\n-- \nBrandon Williams\n"},{"id":"312755","messageId":"20170227175216.GA153455@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"Re: [PATCH 0/5] recursing submodules with relative pathspec (grep and ls-files)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-27T17:52:16Z","receivedAt":"2017-02-27T18:56:38Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/24, Brandon Williams wrote:\n> It was discovered that when using the --recurse-submodules flag with `git grep`\n> and `git ls-files` and specifying a relative path when not at the root causes\n> the child processes spawned to error out with an error like:\n> \n> fatal: ..: '..' is outside repository\n> \n> While true that \"..\" is outside the scope of the submodule repository, it\n> probably doesn't make much sense to the user who gave that pathspec with\n> respect to the superproject. Since the child processes that are spawned to\n> handle the submodules have some context that they are executing underneath a\n> superproject (via the 'super_prefix'), they should be able to prevent dying\n> under this circumstance.\n> \n> This series fixes this bug in both git grep and git ls-files as well as\n> correctly formatting the output from submodules to handle the relative paths\n> with \"..\".\n> \n> One of the changes made to fix this was to add an additional flag for the\n> parse_pathspec() function in order to treat all paths provided as being from\n> the root of the repository. I hesitantly selected the name 'PATHSPEC_FROMROOT'\n> but I'm not fond of it since its too similar to the pathspec magic define\n> 'PATHSPEC_FROMTOP'. So I'm open for naming suggestions.\n> \n> Brandon Williams (5):\n> grep: illustrate bug when recursing with relative pathspec\n> pathspec: add PATHSPEC_FROMROOT flag\n> grep: fix bug when recuring with relative pathspec\n> ls-files: illustrate bug when recursing with relative pathspec\n> ls-files: fix bug when recuring with relative pathspec\n> \n> builtin/grep.c | 8 ++++--\n> builtin/ls-files.c | 8 ++++--\n> pathspec.c | 2 +-\n> pathspec.h | 2 ++\n> t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++\n> t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++\n> 6 files changed, 107 insertions(+), 5 deletions(-)\n> \n\nTurns out that this series doesn't address all of the issues. This\nseries also seems to introduce broken behavior when recursing from a\nsubdirectory. So I need to think about this problem a little bit more\nand reroll.\n\n-- \nBrandon Williams\n"},{"id":"312897","messageId":"xmqqa896ja7z.fsf@gitster.mtv.corp.google.com","threadId":"45217","inReplyTo":"20170224235100.52627-6-bmwill@google.com","subject":"Re: [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T21:07:12Z","receivedAt":"2017-02-28T21:08:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Fix a bug which causes a child process for a submodule to error out when a\n> relative pathspec with a \"..\" is provided in the superproject.\n>\n> While at it, correctly construct the super-prefix to be used in a submodule\n> when not at the root of the repository.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> builtin/ls-files.c | 8 ++++++--\n> t/t3007-ls-files-recurse-submodules.sh | 2 +-\n> 2 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 159229081..89533ab8e 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag\n> static void show_gitlink(const struct cache_entry *ce)\n> {\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tstruct strbuf name = STRBUF_INIT;\n> \tint status;\n> \tint i;\n> \n> +\tquote_path_relative(ce->name, prefix, &name);\n> \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n\nSame comment as 3/5. quote_path is to produce c-quote and is not\neven meant for shell script quoting. run_command() interface would\ndo its own shell quoting when needed, so I think you just want the\nexact string you want to pass here.\n\n"},{"id":"312904","messageId":"xmqqefyijacj.fsf@gitster.mtv.corp.google.com","threadId":"45217","inReplyTo":"20170224235100.52627-4-bmwill@google.com","subject":"Re: [PATCH 3/5] grep: fix bug when recuring with relative pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T21:04:28Z","receivedAt":"2017-02-28T21:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> \t/* Add super prefix */\n> +\tquote_path_relative(name, opt->prefix, &buf);\n\nHmph, do you want a quoted version here, not just relative_path()?\n\nPerhaps add a test with an \"unusual\" byte (e.g. a double-quote) in\nthe path?\n\n> \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n> \t\t\t super_prefix ? super_prefix : \"\",\n> -\t\t\t name);\n> +\t\t\t buf.buf);\n> +\tstrbuf_release(&buf);\n> \targv_array_push(&cp.args, \"grep\");\n> \n> \t/*\n> @@ -1199,7 +1202,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \n> \tparse_pathspec(&pathspec, 0,\n> \t\t PATHSPEC_PREFER_CWD |\n> -\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n> +\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |\n> +\t\t (super_prefix ? PATHSPEC_FROMROOT : 0),\n> \t\t prefix, argv + i);\n> \tpathspec.max_depth = opt.max_depth;\n> \tpathspec.recursive = 1;\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> index 418ba68fe..e0932b2b7 100755\n> --- a/t/t7814-grep-recurse-submodules.sh\n> +++ b/t/t7814-grep-recurse-submodules.sh\n> @@ -227,7 +227,7 @@ test_expect_success 'grep history with moved submoules' '\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_failure 'grep using relative path' '\n> +test_expect_success 'grep using relative path' '\n> \ttest_when_finished \"rm -rf parent sub\" &&\n> \tgit init sub &&\n> \techo \"foobar\" >sub/file &&\n"},{"id":"313081","messageId":"20170302180038.GB30622@google.com","threadId":"45217","inReplyTo":"xmqqefyijacj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/5] grep: fix bug when recuring with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-02T18:00:38Z","receivedAt":"2017-03-02T18:01:56Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/28, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > \t/* Add super prefix */\n> > +\tquote_path_relative(name, opt->prefix, &buf);\n> \n> Hmph, do you want a quoted version here, not just relative_path()?\n> \n> Perhaps add a test with an \"unusual\" byte (e.g. a double-quote) in\n> the path?\n\nYou're absolutely correct.\n\n> \n> > \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n> > \t\t\t super_prefix ? super_prefix : \"\",\n> > -\t\t\t name);\n> > +\t\t\t buf.buf);\n> > +\tstrbuf_release(&buf);\n> > \targv_array_push(&cp.args, \"grep\");\n> > \n> > \t/*\n> > @@ -1199,7 +1202,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> > \n> > \tparse_pathspec(&pathspec, 0,\n> > \t\t PATHSPEC_PREFER_CWD |\n> > -\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),\n> > +\t\t (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |\n> > +\t\t (super_prefix ? PATHSPEC_FROMROOT : 0),\n> > \t\t prefix, argv + i);\n> > \tpathspec.max_depth = opt.max_depth;\n> > \tpathspec.recursive = 1;\n> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> > index 418ba68fe..e0932b2b7 100755\n> > --- a/t/t7814-grep-recurse-submodules.sh\n> > +++ b/t/t7814-grep-recurse-submodules.sh\n> > @@ -227,7 +227,7 @@ test_expect_success 'grep history with moved submoules' '\n> > \ttest_cmp expect actual\n> > '\n> > \n> > -test_expect_failure 'grep using relative path' '\n> > +test_expect_success 'grep using relative path' '\n> > \ttest_when_finished \"rm -rf parent sub\" &&\n> > \tgit init sub &&\n> > \techo \"foobar\" >sub/file &&\n\n-- \nBrandon Williams\n"},{"id":"313094","messageId":"20170302180143.GC30622@google.com","threadId":"45217","inReplyTo":"xmqqa896ja7z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-02T18:01:43Z","receivedAt":"2017-03-02T19:54:11Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/28, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > Fix a bug which causes a child process for a submodule to error out when a\n> > relative pathspec with a \"..\" is provided in the superproject.\n> >\n> > While at it, correctly construct the super-prefix to be used in a submodule\n> > when not at the root of the repository.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > builtin/ls-files.c | 8 ++++++--\n> > t/t3007-ls-files-recurse-submodules.sh | 2 +-\n> > 2 files changed, 7 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> > index 159229081..89533ab8e 100644\n> > --- a/builtin/ls-files.c\n> > +++ b/builtin/ls-files.c\n> > @@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag\n> > static void show_gitlink(const struct cache_entry *ce)\n> > {\n> > \tstruct child_process cp = CHILD_PROCESS_INIT;\n> > +\tstruct strbuf name = STRBUF_INIT;\n> > \tint status;\n> > \tint i;\n> > \n> > +\tquote_path_relative(ce->name, prefix, &name);\n> > \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n> \n> Same comment as 3/5. quote_path is to produce c-quote and is not\n> even meant for shell script quoting. run_command() interface would\n> do its own shell quoting when needed, so I think you just want the\n> exact string you want to pass here.\n> \n\nYeah I don't know what I was thinking when using that instead of\n'relative_path()'. Will change for this and 3/5.\n\n-- \nBrandon Williams\n"},{"id":"313383","messageId":"20170306230717.29095-1-bmwill@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"[RFC PATCH] grep: fix bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-06T23:07:17Z","receivedAt":"2017-03-06T23:16:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When using the --recurse-submodules flag with a relative pathspec which\nincludes \"..\", an error is produced inside the child process spawned for\na submodule. When creating the pathspec struct in the child, the \"..\"\nis interpreted to mean \"go up a directory\" which causes an error stating\nthat the path \"..\" is outside of the repository.\n\nWhile it is true that \"..\" is outside the scope of the submodule, it is\nconfusing to a user who originally invoked the command where \"..\" was\nindeed still inside the scope of the superproject. Since the child\nprocess launched for the submodule has some context that it is operating\nunderneath a superproject, this error could be avoided.\n\nThis patch fixes the bug by passing the 'prefix' to the child process.\nNow each child process that works on a submodule has two points of\nreference to the superproject: (1) the 'super_prefix' which is the path\nfrom the root of the superproject down to root of the submodule and (2)\nthe 'prefix' which is the path from the root of the superproject down to\nthe directory where the user invoked the git command.\n\nWith these two pieces of information a child process can correctly\ninterpret the pathspecs provided by the user as well as being able to\nproperly format the its output relative to the directory the user\ninvoked a git command from.\n\nSigned-off-by: Brandon Williams \n---\n\nAfter taking a closer look at this bug I determined that I did a poor job at\nthinking of all the corner cases when implementing a recursive grep. As it\nturns out I think we really need to pass on the prefix information to the child\nprocess so that it has enough context to do path matching and to format its\noutput. Unfortunately I don't know the best way to pass on this information\nwithout breaking other commands that rely on the GIT_PREFIX being overridden\nduring discovery. Maybe we'll need to add in another env var to account for\nthat? For the purposes of showing how to fix this bug while still getting the\ntest suite to pass, I have git respect the GIT_PREFIX env var only if the\nsuper_prefix is set (which it should only be used in submodule operations at\nthis point in time). Its a pretty hacky fix, so I'm up for other suggestions\non how to properly pass on this information to the child process.\n\n\n builtin/grep.c | 39 ++++++++++++--------\n git.c | 2 --\n setup.c | 5 +++\n t/t7814-grep-recurse-submodules.sh | 73 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 102 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2c727ef49..acecad0e0 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -310,10 +310,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n-\tif (opt->relative && opt->prefix_length) {\n-\t\tquote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);\n-\t\tstrbuf_insert(&pathbuf, 0, filename, tree_name_len);\n-\t} else if (super_prefix) {\n+\tif (super_prefix) {\n \t\tstrbuf_add(&pathbuf, filename, tree_name_len);\n \t\tstrbuf_addstr(&pathbuf, super_prefix);\n \t\tstrbuf_addstr(&pathbuf, filename + tree_name_len);\n@@ -321,6 +318,13 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n+\tif (opt->relative && opt->prefix_length) {\n+\t\tchar *name = strbuf_detach(&pathbuf, NULL);\n+\t\tquote_path_relative(name + tree_name_len, opt->prefix, &pathbuf);\n+\t\tstrbuf_insert(&pathbuf, 0, name, tree_name_len);\n+\t\tfree(name);\n+\t}\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n \t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n@@ -345,12 +349,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (super_prefix)\n+\t\tstrbuf_addstr(&buf, super_prefix);\n+\tstrbuf_addstr(&buf, filename);\n+\n \tif (opt->relative && opt->prefix_length) {\n-\t\tquote_path_relative(filename, opt->prefix, &buf);\n-\t} else {\n-\t\tif (super_prefix)\n-\t\t\tstrbuf_addstr(&buf, super_prefix);\n-\t\tstrbuf_addstr(&buf, filename);\n+\t\tchar *name = strbuf_detach(&buf, NULL);\n+\t\tquote_path_relative(name, opt->prefix, &buf);\n+\t\tfree(name);\n \t}\n \n #ifndef NO_PTHREADS\n@@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n }\n \n static void compile_submodule_options(const struct grep_opt *opt,\n-\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t const char **argv,\n \t\t\t\t int cached, int untracked,\n \t\t\t\t int opt_exclude, int use_index,\n \t\t\t\t int pattern_type_arg)\n {\n \tstruct grep_pat *pattern;\n-\tint i;\n \n \tif (recurse_submodules)\n \t\targv_array_push(&submodule_options, \"--recurse-submodules\");\n@@ -523,9 +528,8 @@ static void compile_submodule_options(const struct grep_opt *opt,\n \n \t/* Add Pathspecs */\n \targv_array_push(&submodule_options, \"--\");\n-\tfor (i = 0; i < pathspec->nr; i++)\n-\t\targv_array_push(&submodule_options,\n-\t\t\t\tpathspec->items[i].original);\n+\tfor (; *argv; argv++)\n+\t\targv_array_push(&submodule_options, *argv);\n }\n \n /*\n@@ -549,6 +553,11 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \tprepare_submodule_repo_env(&cp.env_array);\n \targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n \n+\tif (opt->relative && opt->prefix_length)\n+\t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n+\t\t\t\t GIT_PREFIX_ENVIRONMENT,\n+\t\t\t\t opt->prefix);\n+\n \t/* Add super prefix */\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n@@ -1206,7 +1215,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (recurse_submodules) {\n \t\tgitmodules_config();\n-\t\tcompile_submodule_options(&opt, &pathspec, cached, untracked,\n+\t\tcompile_submodule_options(&opt, argv + i, cached, untracked,\n \t\t\t\t\t opt_exclude, use_index,\n \t\t\t\t\t pattern_type_arg);\n \t}\ndiff --git a/git.c b/git.c\nindex b367cf668..b05108afd 100644\n--- a/git.c\n+++ b/git.c\n@@ -361,8 +361,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \tif (!help && get_super_prefix()) {\n \t\tif (!(p->option & SUPPORT_SUPER_PREFIX))\n \t\t\tdie(\"%s doesn't support --super-prefix\", p->cmd);\n-\t\tif (prefix)\n-\t\t\tdie(\"can't use --super-prefix from a subdirectory\");\n \t}\n \n \tif (!help && p->option & NEED_WORK_TREE)\ndiff --git a/setup.c b/setup.c\nindex 1b534a750..0de379319 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -936,10 +936,15 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n const char *setup_git_directory_gently(int *nongit_ok)\n {\n \tconst char *prefix;\n+\tconst char *env_prefix;\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n+\tenv_prefix = getenv(GIT_PREFIX_ENVIRONMENT);\n+\n \tif (prefix)\n \t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n+\telse if (env_prefix && get_super_prefix())\n+\t\tprefix = env_prefix;\n \telse\n \t\tsetenv(GIT_PREFIX_ENVIRONMENT, \"\", 1);\n \ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 67247a01d..d8ed3d64f 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -227,6 +227,79 @@ test_expect_success 'grep history with moved submoules' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep using relative path' '\n+\ttest_when_finished \"rm -rf parent sub\" &&\n+\tgit init sub &&\n+\techo \"foobar\" >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\n+\tgit init parent &&\n+\techo \"foobar\" >parent/file &&\n+\tgit -C parent add file &&\n+\tmkdir parent/src &&\n+\techo \"foobar\" >parent/src/file2 &&\n+\tgit -C parent add src/file2 &&\n+\tgit -C parent submodule add ../sub &&\n+\tgit -C parent commit -m \"add files and submodule\" &&\n+\n+\t# From top works\n+\tcat >expect <<-\\EOF &&\n+\tfile:foobar\n+\tsrc/file2:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to top errors out\n+\tcat >expect <<-\\EOF &&\n+\t../file:foobar\n+\tfile2:foobar\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule errors out\n+\tcat >expect <<-\\EOF &&\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- ../sub >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep from a subdir' '\n+\ttest_when_finished \"rm -rf parent sub\" &&\n+\tgit init sub &&\n+\techo \"foobar\" >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\n+\tgit init parent &&\n+\tmkdir parent/src &&\n+\techo \"foobar\" >parent/src/file &&\n+\tgit -C parent add src/file &&\n+\tgit -C parent submodule add ../sub src/sub &&\n+\tgit -C parent submodule add ../sub sub &&\n+\tgit -C parent commit -m \"add files and submodules\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsrc/file:foobar\n+\tsrc/sub/file:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tfile:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_incompatible_with_recurse_submodules ()\n {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n-- \n2.12.0.rc1.440.g5b76565f74-goog\n\n"},{"id":"314090","messageId":"20170314221100.24856-1-bmwill@google.com","threadId":"45217","inReplyTo":"20170224235100.52627-1-bmwill@google.com","subject":"[PATCH v2 0/4] recursing submodules with relative pathspec (grep and ls-files)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-14T22:10:56Z","receivedAt":"2017-03-14T22:11:18Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"v2 of the series tackles the problem slightly differently than v1 did, and in a\nless invasive way in my opinion. v1 also didn't fix everything.\n\nv2 introduces a way to pass a 'prefix' that is respected by a git process.\nThis allows the git child processes (which operate on submodules) to have the\nsuper_prefix (the path from the root of the superproject to the submodule) and\nthe prefix (path from the root of the superproject to the directory in which\nthe original command was launched). With these two pieces of information the\nchild process can correctly handle pathspec matching as well as correctly\nformatting their output with relative paths.\n\nIn order to pass the prefix to a child I made a new env var since the existing\nGIT_PREFIX isn't respected. I'm not sure this is the best method so I'm open\nto ideas on the best way to convey this information to a child process.\n\nBrandon Williams (4):\n grep: fix help text typo\n setup: allow for prefix to be passed to git commands\n grep: fix bug when recursing with relative pathspec\n ls-files: fix bug when recursing with relative pathspec\n\n builtin/grep.c | 41 +++++++++++--------\n builtin/ls-files.c | 41 ++++++++++---------\n cache.h | 1 +\n git.c | 2 -\n setup.c | 6 +++\n t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++\n t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++\n 7 files changed, 167 insertions(+), 38 deletions(-)\n\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314091","messageId":"20170314221100.24856-2-bmwill@google.com","threadId":"45217","inReplyTo":"20170314221100.24856-1-bmwill@google.com","subject":"[PATCH v2 1/4] grep: fix help text typo","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-14T22:10:57Z","receivedAt":"2017-03-14T22:11:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"---\n builtin/grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9304c33e7..4694e68f3 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -979,7 +979,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT(0, \"exclude-standard\", &opt_exclude,\n \t\t\t N_(\"ignore files specified via '.gitignore'\"), 1),\n \t\tOPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n-\t\t\t N_(\"recursivley search in each submodule\")),\n+\t\t\t N_(\"recursively search in each submodule\")),\n \t\tOPT_STRING(0, \"parent-basename\", &parent_basename,\n \t\t\t N_(\"basename\"),\n \t\t\t N_(\"prepend parent project's basename to output\")),\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314092","messageId":"20170314221100.24856-3-bmwill@google.com","threadId":"45217","inReplyTo":"20170314221100.24856-1-bmwill@google.com","subject":"[PATCH v2 2/4] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-14T22:10:58Z","receivedAt":"2017-03-14T22:11:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In a future patch child processes which act on submodules need a little\nmore context about the original command that was invoked. This patch\nteaches git to use the prefix stored in `GIT_INTERNAL_TOPLEVEL_PREFIX`\nif another prefix wasn't found during the git directory setup process.\n\n---\n cache.h | 1 +\n git.c | 2 --\n setup.c | 6 ++++++\n 3 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 8c0e64420..7d253a078 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -410,6 +410,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n #define GIT_PREFIX_ENVIRONMENT \"GIT_PREFIX\"\n #define GIT_SUPER_PREFIX_ENVIRONMENT \"GIT_INTERNAL_SUPER_PREFIX\"\n+#define GIT_TOPLEVEL_PREFIX_ENVIRONMENT \"GIT_INTERNAL_TOPLEVEL_PREFIX\"\n #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\ndiff --git a/git.c b/git.c\nindex 33f52acbc..8ff44f081 100644\n--- a/git.c\n+++ b/git.c\n@@ -361,8 +361,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \tif (!help && get_super_prefix()) {\n \t\tif (!(p->option & SUPPORT_SUPER_PREFIX))\n \t\t\tdie(\"%s doesn't support --super-prefix\", p->cmd);\n-\t\tif (prefix)\n-\t\t\tdie(\"can't use --super-prefix from a subdirectory\");\n \t}\n \n \tif (!help && p->option & NEED_WORK_TREE)\ndiff --git a/setup.c b/setup.c\nindex 8f64fbdfb..c8492ea8a 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -940,8 +940,14 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n const char *setup_git_directory_gently(int *nongit_ok)\n {\n \tconst char *prefix;\n+\tconst char *env_prefix;\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n+\tenv_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n+\n+\tif (env_prefix)\n+\t\tprefix = env_prefix;\n+\n \tif (prefix)\n \t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n \telse\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314093","messageId":"20170314221100.24856-4-bmwill@google.com","threadId":"45217","inReplyTo":"20170314221100.24856-1-bmwill@google.com","subject":"[PATCH v2 3/4] grep: fix bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-14T22:10:59Z","receivedAt":"2017-03-14T22:11:32Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When using the --recurse-submodules flag with a relative pathspec which\nincludes \"..\", an error is produced inside the child process spawned for\na submodule. When creating the pathspec struct in the child, the \"..\"\nis interpreted to mean \"go up a directory\" which causes an error stating\nthat the path \"..\" is outside of the repository.\n\nWhile it is true that \"..\" is outside the scope of the submodule, it is\nconfusing to a user who originally invoked the command where \"..\" was\nindeed still inside the scope of the superproject. Since the child\nprocess launched for the submodule has some context that it is operating\nunderneath a superproject, this error could be avoided.\n\nThis patch fixes the bug by passing the 'prefix' to the child process.\nNow each child process that works on a submodule has two points of\nreference to the superproject: (1) the 'super_prefix' which is the path\nfrom the root of the superproject down to root of the submodule and (2)\nthe 'prefix' which is the path from the root of the superproject down to\nthe directory where the user invoked the git command.\n\nWith these two pieces of information a child process can correctly\ninterpret the pathspecs provided by the user as well as being able to\nproperly format its output relative to the directory the user invoked\nthe original command from.\n\nSigned-off-by: Brandon Williams \n---\n builtin/grep.c | 39 ++++++++++++--------\n t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 99 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 4694e68f3..b5d846393 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -310,10 +310,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n-\tif (opt->relative && opt->prefix_length) {\n-\t\tquote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);\n-\t\tstrbuf_insert(&pathbuf, 0, filename, tree_name_len);\n-\t} else if (super_prefix) {\n+\tif (super_prefix) {\n \t\tstrbuf_add(&pathbuf, filename, tree_name_len);\n \t\tstrbuf_addstr(&pathbuf, super_prefix);\n \t\tstrbuf_addstr(&pathbuf, filename + tree_name_len);\n@@ -321,6 +318,13 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n+\tif (opt->relative && opt->prefix_length) {\n+\t\tchar *name = strbuf_detach(&pathbuf, NULL);\n+\t\tquote_path_relative(name + tree_name_len, opt->prefix, &pathbuf);\n+\t\tstrbuf_insert(&pathbuf, 0, name, tree_name_len);\n+\t\tfree(name);\n+\t}\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n \t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n@@ -345,12 +349,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (super_prefix)\n+\t\tstrbuf_addstr(&buf, super_prefix);\n+\tstrbuf_addstr(&buf, filename);\n+\n \tif (opt->relative && opt->prefix_length) {\n-\t\tquote_path_relative(filename, opt->prefix, &buf);\n-\t} else {\n-\t\tif (super_prefix)\n-\t\t\tstrbuf_addstr(&buf, super_prefix);\n-\t\tstrbuf_addstr(&buf, filename);\n+\t\tchar *name = strbuf_detach(&buf, NULL);\n+\t\tquote_path_relative(name, opt->prefix, &buf);\n+\t\tfree(name);\n \t}\n \n #ifndef NO_PTHREADS\n@@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n }\n \n static void compile_submodule_options(const struct grep_opt *opt,\n-\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t const char **argv,\n \t\t\t\t int cached, int untracked,\n \t\t\t\t int opt_exclude, int use_index,\n \t\t\t\t int pattern_type_arg)\n {\n \tstruct grep_pat *pattern;\n-\tint i;\n \n \tif (recurse_submodules)\n \t\targv_array_push(&submodule_options, \"--recurse-submodules\");\n@@ -523,9 +528,8 @@ static void compile_submodule_options(const struct grep_opt *opt,\n \n \t/* Add Pathspecs */\n \targv_array_push(&submodule_options, \"--\");\n-\tfor (i = 0; i < pathspec->nr; i++)\n-\t\targv_array_push(&submodule_options,\n-\t\t\t\tpathspec->items[i].original);\n+\tfor (; *argv; argv++)\n+\t\targv_array_push(&submodule_options, *argv);\n }\n \n /*\n@@ -549,6 +553,11 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \tprepare_submodule_repo_env(&cp.env_array);\n \targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n \n+\tif (opt->relative && opt->prefix_length)\n+\t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n+\t\t\t\t GIT_TOPLEVEL_PREFIX_ENVIRONMENT,\n+\t\t\t\t opt->prefix);\n+\n \t/* Add super prefix */\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n@@ -1236,7 +1245,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (recurse_submodules) {\n \t\tgitmodules_config();\n-\t\tcompile_submodule_options(&opt, &pathspec, cached, untracked,\n+\t\tcompile_submodule_options(&opt, argv + i, cached, untracked,\n \t\t\t\t\t opt_exclude, use_index,\n \t\t\t\t\t pattern_type_arg);\n \t}\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 67247a01d..5b6eb3a65 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -227,6 +227,81 @@ test_expect_success 'grep history with moved submoules' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep using relative path' '\n+\ttest_when_finished \"rm -rf parent sub\" &&\n+\tgit init sub &&\n+\techo \"foobar\" >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\n+\tgit init parent &&\n+\techo \"foobar\" >parent/file &&\n+\tgit -C parent add file &&\n+\tmkdir parent/src &&\n+\techo \"foobar\" >parent/src/file2 &&\n+\tgit -C parent add src/file2 &&\n+\tgit -C parent submodule add ../sub &&\n+\tgit -C parent commit -m \"add files and submodule\" &&\n+\n+\t# From top works\n+\tcat >expect <<-\\EOF &&\n+\tfile:foobar\n+\tsrc/file2:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to top\n+\tcat >expect <<-\\EOF &&\n+\t../file:foobar\n+\tfile2:foobar\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule\n+\tcat >expect <<-\\EOF &&\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- ../sub >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep from a subdir' '\n+\ttest_when_finished \"rm -rf parent sub\" &&\n+\tgit init sub &&\n+\techo \"foobar\" >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\n+\tgit init parent &&\n+\tmkdir parent/src &&\n+\techo \"foobar\" >parent/src/file &&\n+\tgit -C parent add src/file &&\n+\tgit -C parent submodule add ../sub src/sub &&\n+\tgit -C parent submodule add ../sub sub &&\n+\tgit -C parent commit -m \"add files and submodules\" &&\n+\n+\t# Verify grep from root works\n+\tcat >expect <<-\\EOF &&\n+\tsrc/file:foobar\n+\tsrc/sub/file:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Verify grep from a subdir works\n+\tcat >expect <<-\\EOF &&\n+\tfile:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_incompatible_with_recurse_submodules ()\n {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314094","messageId":"20170314221100.24856-5-bmwill@google.com","threadId":"45217","inReplyTo":"20170314221100.24856-1-bmwill@google.com","subject":"[PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-14T22:11:00Z","receivedAt":"2017-03-14T22:11:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When using the --recurse-submodules flag with a relative pathspec which\nincludes \"..\", an error is produced inside the child process spawned for a\nsubmodule. When creating the pathspec struct in the child, the \"..\" is\ninterpreted to mean \"go up a directory\" which causes an error stating that the\npath \"..\" is outside of the repository.\n\nWhile it is true that \"..\" is outside the scope of the submodule, it is\nconfusing to a user who originally invoked the command where \"..\" was indeed\nstill inside the scope of the superproject. Since the child process launched\nfor the submodule has some context that it is operating underneath a\nsuperproject, this error could be avoided.\n\nThis patch fixes the bug by passing the 'prefix' to the child process. Now\neach child process that works on a submodule has two points of reference to the\nsuperproject: (1) the 'super_prefix' which is the path from the root of the\nsuperproject down to root of the submodule and (2) the 'prefix' which is the\npath from the root of the superproject down to the directory where the user\ninvoked the git command.\n\nWith these two pieces of information a child process can correctly interpret\nthe pathspecs provided by the user as well as being able to properly format its\noutput relative to the directory the user invoked the original command from.\n\nSigned-off-by: Brandon Williams \n---\n builtin/ls-files.c | 41 +++++++++++++++++-----------------\n t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 1c0f057d0..d449e46db 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -30,7 +30,7 @@ static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n static int recurse_submodules;\n-static struct argv_array submodules_options = ARGV_ARRAY_INIT;\n+static struct argv_array submodule_options = ARGV_ARRAY_INIT;\n \n static const char *prefix;\n static const char *super_prefix;\n@@ -172,20 +172,27 @@ static void show_killed_files(struct dir_struct *dir)\n /*\n * Compile an argv_array with all of the options supported by --recurse_submodules\n */\n-static void compile_submodule_options(const struct dir_struct *dir, int show_tag)\n+static void compile_submodule_options(const char **argv,\n+\t\t\t\t const struct dir_struct *dir,\n+\t\t\t\t int show_tag)\n {\n \tif (line_terminator == '\\0')\n-\t\targv_array_push(&submodules_options, \"-z\");\n+\t\targv_array_push(&submodule_options, \"-z\");\n \tif (show_tag)\n-\t\targv_array_push(&submodules_options, \"-t\");\n+\t\targv_array_push(&submodule_options, \"-t\");\n \tif (show_valid_bit)\n-\t\targv_array_push(&submodules_options, \"-v\");\n+\t\targv_array_push(&submodule_options, \"-v\");\n \tif (show_cached)\n-\t\targv_array_push(&submodules_options, \"--cached\");\n+\t\targv_array_push(&submodule_options, \"--cached\");\n \tif (show_eol)\n-\t\targv_array_push(&submodules_options, \"--eol\");\n+\t\targv_array_push(&submodule_options, \"--eol\");\n \tif (debug_mode)\n-\t\targv_array_push(&submodules_options, \"--debug\");\n+\t\targv_array_push(&submodule_options, \"--debug\");\n+\n+\t/* Add Pathspecs */\n+\targv_array_push(&submodule_options, \"--\");\n+\tfor (; *argv; argv++)\n+\t\targv_array_push(&submodule_options, *argv);\n }\n \n /**\n@@ -195,8 +202,11 @@ static void show_gitlink(const struct cache_entry *ce)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n-\tint i;\n \n+\tif (prefix_len)\n+\t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n+\t\t\t\t GIT_TOPLEVEL_PREFIX_ENVIRONMENT,\n+\t\t\t\t prefix);\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n \t\t\t ce->name);\n@@ -204,16 +214,7 @@ static void show_gitlink(const struct cache_entry *ce)\n \targv_array_push(&cp.args, \"--recurse-submodules\");\n \n \t/* add supported options */\n-\targv_array_pushv(&cp.args, submodules_options.argv);\n-\n-\t/*\n-\t * Pass in the original pathspec args. The submodule will be\n-\t * responsible for prepending the 'submodule_prefix' prior to comparing\n-\t * against the pathspec for matches.\n-\t */\n-\targv_array_push(&cp.args, \"--\");\n-\tfor (i = 0; i < pathspec.nr; i++)\n-\t\targv_array_push(&cp.args, pathspec.items[i].original);\n+\targv_array_pushv(&cp.args, submodule_options.argv);\n \n \tcp.git_cmd = 1;\n \tcp.dir = ce->name;\n@@ -604,7 +605,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tsetup_work_tree();\n \n \tif (recurse_submodules)\n-\t\tcompile_submodule_options(&dir, show_tag);\n+\t\tcompile_submodule_options(argv, &dir, show_tag);\n \n \tif (recurse_submodules &&\n \t (show_stage || show_deleted || show_others || show_unmerged ||\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex a5426171d..4cf6ccf5a 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -188,6 +188,45 @@ test_expect_success '--recurse-submodules and pathspecs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--recurse-submodules and relative paths' '\n+\t# From subdir\n+\tcat >expect <<-\\EOF &&\n+\tb\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to top\n+\tcat >expect <<-\\EOF &&\n+\t../.gitmodules\n+\t../a\n+\tb\n+\t../h.txt\n+\t../sib/file\n+\t../sub/file\n+\t../submodule/.gitmodules\n+\t../submodule/c\n+\t../submodule/f.TXT\n+\t../submodule/g.txt\n+\t../submodule/subsub/d\n+\t../submodule/subsub/e.txt\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules -- .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule\n+\tcat >expect <<-\\EOF &&\n+\t../submodule/.gitmodules\n+\t../submodule/c\n+\t../submodule/f.TXT\n+\t../submodule/g.txt\n+\t../submodule/subsub/d\n+\t../submodule/subsub/e.txt\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules -- ../submodule >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--recurse-submodules does not support --error-unmatch' '\n \ttest_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&\n \ttest_i18ngrep \"does not support --error-unmatch\" actual\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314097","messageId":"alpine.DEB.2.20.1703142326460.3767@virtualbox","threadId":"45217","inReplyTo":"20170314221100.24856-3-bmwill@google.com","subject":"Re: [PATCH v2 2/4] setup: allow for prefix to be passed to git commands","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-14T22:28:24Z","receivedAt":"2017-03-14T22:28:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Tue, 14 Mar 2017, Brandon Williams wrote:\n\n> In a future patch child processes which act on submodules need a little\n> more context about the original command that was invoked. This patch\n> teaches git to use the prefix stored in `GIT_INTERNAL_TOPLEVEL_PREFIX`\n> if another prefix wasn't found during the git directory setup process.\n\nMissing SOB ;-)\n\n> diff --git a/setup.c b/setup.c\n> index 8f64fbdfb..c8492ea8a 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -940,8 +940,14 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n> const char *setup_git_directory_gently(int *nongit_ok)\n> {\n> \tconst char *prefix;\n> +\tconst char *env_prefix;\n\nI'd just append this to the previous line (`const char *prefix,\n*env_prefix`).\n\n> \tprefix = setup_git_directory_gently_1(nongit_ok);\n> +\tenv_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n> +\n> +\tif (env_prefix)\n> +\t\tprefix = env_prefix;\n\nThe commit message claims that env_prefix is used if no other prefix was\nfound, but this code ignores any prefix if the environment variable was\nset.\n\nWhich version is correct?\n\nCiao,\nJohannes\n"},{"id":"314098","messageId":"20170314223531.GB168037@google.com","threadId":"45217","inReplyTo":"alpine.DEB.2.20.1703142326460.3767@virtualbox","subject":"Re: [PATCH v2 2/4] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-14T22:35:31Z","receivedAt":"2017-03-14T22:35:40Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/14, Johannes Schindelin wrote:\n> Hi Brandon,\n> \n> On Tue, 14 Mar 2017, Brandon Williams wrote:\n> \n> > In a future patch child processes which act on submodules need a little\n> > more context about the original command that was invoked. This patch\n> > teaches git to use the prefix stored in `GIT_INTERNAL_TOPLEVEL_PREFIX`\n> > if another prefix wasn't found during the git directory setup process.\n> \n> Missing SOB ;-)\n> \n\nThanks for that catch. I'm expecting some discussion on this patch in\nparticular (and cc'd you since you've been working on this area of the\ncode) so I'll make sure to add it in the next reroll.\n\n> > diff --git a/setup.c b/setup.c\n> > index 8f64fbdfb..c8492ea8a 100644\n> > --- a/setup.c\n> > +++ b/setup.c\n> > @@ -940,8 +940,14 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n> > const char *setup_git_directory_gently(int *nongit_ok)\n> > {\n> > \tconst char *prefix;\n> > +\tconst char *env_prefix;\n> \n> I'd just append this to the previous line (`const char *prefix,\n> *env_prefix`).\n\nThat would be cleaner.\n\n> \n> > \tprefix = setup_git_directory_gently_1(nongit_ok);\n> > +\tenv_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n> > +\n> > +\tif (env_prefix)\n> > +\t\tprefix = env_prefix;\n> \n> The commit message claims that env_prefix is used if no other prefix was\n> found, but this code ignores any prefix if the environment variable was\n> set.\n> \n> Which version is correct?\n\nWell, as you can tell I flip-flopped on what I thought the best course\nof action would be. For my intentions (submodule-centric) I don't\nbelieve they would ever both have a value so it doesn't matter to me\nwhich it is. Though future users may want a particular order of\nprecedence.\n\n> \n> Ciao,\n> Johannes\n\n-- \nBrandon Williams\n"},{"id":"314100","messageId":"CAGZ79kZwUcNB=ZKAb38CBMX1Y-EoP_CbXOzEod6wAaKnATghrg@mail.gmail.com","threadId":"45217","inReplyTo":"20170314221100.24856-2-bmwill@google.com","subject":"Re: [PATCH v2 1/4] grep: fix help text typo","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-14T22:49:45Z","receivedAt":"2017-03-14T22:49:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams wrote:\n\nMissing SoB here, too.\n\n> ---\n> builtin/grep.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 9304c33e7..4694e68f3 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -979,7 +979,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> OPT_SET_INT(0, \"exclude-standard\", &opt_exclude,\n> N_(\"ignore files specified via '.gitignore'\"), 1),\n> OPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n> - N_(\"recursivley search in each submodule\")),\n> + N_(\"recursively search in each submodule\")),\n> OPT_STRING(0, \"parent-basename\", &parent_basename,\n> N_(\"basename\"),\n> N_(\"prepend parent project's basename to output\")),\n> --\n> 2.12.0.367.g23dc2f6d3c-goog\n>\n"},{"id":"314103","messageId":"CAGZ79kY0d45SB8JHWO6jo+WTQQLsZ2jMSVEeeKPjWG7FkJTryA@mail.gmail.com","threadId":"45217","inReplyTo":"20170314221100.24856-4-bmwill@google.com","subject":"Re: [PATCH v2 3/4] grep: fix bug when recursing with relative pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-14T23:03:53Z","receivedAt":"2017-03-14T23:03:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams wrote:\n> This patch fixes the bug by passing the 'prefix' to the child process.\n> Now each child process that works on a submodule has two points of\n> reference to the superproject: (1) the 'super_prefix' which is the path\n> from the root of the superproject down to root of the submodule and (2)\n> the 'prefix' which is the path from the root of the superproject down to\n> the directory where the user invoked the git command.\n\nWould the information of this paragraph also be well suited in the\ndocumentation?\n\n(1) clarifies what the super prefix is.\n(2) maybe the docs or this commit message should talk about\n how the prefix is relayed, i.e. it doesn't set 'prefix' in the child,\n but it reads GIT_TOPLEVEL_PREFIX_ENVIRONMENT and\n acts on that as (2) ?\n"},{"id":"314104","messageId":"CAGZ79kaRP0Gbw+zwot_vYo7E=7GMwU7chJsJ7TL8g4TJwivC5Q@mail.gmail.com","threadId":"45217","inReplyTo":"20170314221100.24856-5-bmwill@google.com","subject":"Re: [PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-14T23:06:45Z","receivedAt":"2017-03-14T23:06:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 14, 2017 at 3:11 PM, Brandon Williams wrote:\n> When using the --recurse-submodules flag with a relative pathspec which\n> includes \"..\", an error is produced inside the child process spawned for a\n> submodule. When creating the pathspec struct in the child, the \"..\" is\n> interpreted to mean \"go up a directory\" which causes an error stating that the\n> path \"..\" is outside of the repository.\n>\n> While it is true that \"..\" is outside the scope of the submodule, it is\n> confusing to a user who originally invoked the command where \"..\" was indeed\n> still inside the scope of the superproject. Since the child process launched\n> for the submodule has some context that it is operating underneath a\n> superproject, this error could be avoided.\n>\n> This patch fixes the bug by passing the 'prefix' to the child process. Now\n> each child process that works on a submodule has two points of reference to the\n> superproject: (1) the 'super_prefix' which is the path from the root of the\n> superproject down to root of the submodule and (2) the 'prefix' which is the\n> path from the root of the superproject down to the directory where the user\n> invoked the git command.\n>\n> With these two pieces of information a child process can correctly interpret\n> the pathspecs provided by the user as well as being able to properly format its\n> output relative to the directory the user invoked the original command from.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> builtin/ls-files.c | 41 +++++++++++++++++-----------------\n> t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++\n> 2 files changed, 60 insertions(+), 20 deletions(-)\n>\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index 1c0f057d0..d449e46db 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -30,7 +30,7 @@ static int line_terminator = '\\n';\n> static int debug_mode;\n> static int show_eol;\n> static int recurse_submodules;\n> -static struct argv_array submodules_options = ARGV_ARRAY_INIT;\n> +static struct argv_array submodule_options = ARGV_ARRAY_INIT;\n>\n> static const char *prefix;\n> static const char *super_prefix;\n> @@ -172,20 +172,27 @@ static void show_killed_files(struct dir_struct *dir)\n> /*\n> * Compile an argv_array with all of the options supported by --recurse_submodules\n> */\n> -static void compile_submodule_options(const struct dir_struct *dir, int show_tag)\n> +static void compile_submodule_options(const char **argv,\n> + const struct dir_struct *dir,\n> + int show_tag)\n> {\n> if (line_terminator == '\\0')\n> - argv_array_push(&submodules_options, \"-z\");\n> + argv_array_push(&submodule_options, \"-z\");\n> if (show_tag)\n> - argv_array_push(&submodules_options, \"-t\");\n> + argv_array_push(&submodule_options, \"-t\");\n> if (show_valid_bit)\n> - argv_array_push(&submodules_options, \"-v\");\n> + argv_array_push(&submodule_options, \"-v\");\n> if (show_cached)\n> - argv_array_push(&submodules_options, \"--cached\");\n> + argv_array_push(&submodule_options, \"--cached\");\n> if (show_eol)\n> - argv_array_push(&submodules_options, \"--eol\");\n> + argv_array_push(&submodule_options, \"--eol\");\n> if (debug_mode)\n> - argv_array_push(&submodules_options, \"--debug\");\n> + argv_array_push(&submodule_options, \"--debug\");\n\nUp to here we only rename a variable? If you want to help reviewers,\nplease separate this into two patches. One refactoring, stating it doesn't\nchange behavior; and the other adding the behavioral changes.\n\n> +\n> + /* Add Pathspecs */\n> + argv_array_push(&submodule_options, \"--\");\n> + for (; *argv; argv++)\n> + argv_array_push(&submodule_options, *argv);\n> }\n"},{"id":"314114","messageId":"20170315002007.GH168037@google.com","threadId":"45217","inReplyTo":"CAGZ79kZwUcNB=ZKAb38CBMX1Y-EoP_CbXOzEod6wAaKnATghrg@mail.gmail.com","subject":"Re: [PATCH v2 1/4] grep: fix help text typo","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-15T00:20:07Z","receivedAt":"2017-03-15T00:20:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/14, Stefan Beller wrote:\n> On Tue, Mar 14, 2017 at 3:10 PM, Brandon Williams wrote:\n> \n> Missing SoB here, too.\n\nI guess I'm having an off day...Will fix.\n\n> \n> > ---\n> > builtin/grep.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index 9304c33e7..4694e68f3 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -979,7 +979,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> > OPT_SET_INT(0, \"exclude-standard\", &opt_exclude,\n> > N_(\"ignore files specified via '.gitignore'\"), 1),\n> > OPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n> > - N_(\"recursivley search in each submodule\")),\n> > + N_(\"recursively search in each submodule\")),\n> > OPT_STRING(0, \"parent-basename\", &parent_basename,\n> > N_(\"basename\"),\n> > N_(\"prepend parent project's basename to output\")),\n> > --\n> > 2.12.0.367.g23dc2f6d3c-goog\n> >\n\n-- \nBrandon Williams\n"},{"id":"314146","messageId":"20170315170232.GA159137@google.com","threadId":"45217","inReplyTo":"CAGZ79kaRP0Gbw+zwot_vYo7E=7GMwU7chJsJ7TL8g4TJwivC5Q@mail.gmail.com","subject":"Re: [PATCH v2 4/4] ls-files: fix bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-15T17:02:32Z","receivedAt":"2017-03-15T17:03:58Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/14, Stefan Beller wrote:\n> On Tue, Mar 14, 2017 at 3:11 PM, Brandon Williams wrote:\n> > When using the --recurse-submodules flag with a relative pathspec which\n> > includes \"..\", an error is produced inside the child process spawned for a\n> > submodule. When creating the pathspec struct in the child, the \"..\" is\n> > interpreted to mean \"go up a directory\" which causes an error stating that the\n> > path \"..\" is outside of the repository.\n> >\n> > While it is true that \"..\" is outside the scope of the submodule, it is\n> > confusing to a user who originally invoked the command where \"..\" was indeed\n> > still inside the scope of the superproject. Since the child process launched\n> > for the submodule has some context that it is operating underneath a\n> > superproject, this error could be avoided.\n> >\n> > This patch fixes the bug by passing the 'prefix' to the child process. Now\n> > each child process that works on a submodule has two points of reference to the\n> > superproject: (1) the 'super_prefix' which is the path from the root of the\n> > superproject down to root of the submodule and (2) the 'prefix' which is the\n> > path from the root of the superproject down to the directory where the user\n> > invoked the git command.\n> >\n> > With these two pieces of information a child process can correctly interpret\n> > the pathspecs provided by the user as well as being able to properly format its\n> > output relative to the directory the user invoked the original command from.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > builtin/ls-files.c | 41 +++++++++++++++++-----------------\n> > t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++\n> > 2 files changed, 60 insertions(+), 20 deletions(-)\n> >\n> > diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> > index 1c0f057d0..d449e46db 100644\n> > --- a/builtin/ls-files.c\n> > +++ b/builtin/ls-files.c\n> > @@ -30,7 +30,7 @@ static int line_terminator = '\\n';\n> > static int debug_mode;\n> > static int show_eol;\n> > static int recurse_submodules;\n> > -static struct argv_array submodules_options = ARGV_ARRAY_INIT;\n> > +static struct argv_array submodule_options = ARGV_ARRAY_INIT;\n> >\n> > static const char *prefix;\n> > static const char *super_prefix;\n> > @@ -172,20 +172,27 @@ static void show_killed_files(struct dir_struct *dir)\n> > /*\n> > * Compile an argv_array with all of the options supported by --recurse_submodules\n> > */\n> > -static void compile_submodule_options(const struct dir_struct *dir, int show_tag)\n> > +static void compile_submodule_options(const char **argv,\n> > + const struct dir_struct *dir,\n> > + int show_tag)\n> > {\n> > if (line_terminator == '\\0')\n> > - argv_array_push(&submodules_options, \"-z\");\n> > + argv_array_push(&submodule_options, \"-z\");\n> > if (show_tag)\n> > - argv_array_push(&submodules_options, \"-t\");\n> > + argv_array_push(&submodule_options, \"-t\");\n> > if (show_valid_bit)\n> > - argv_array_push(&submodules_options, \"-v\");\n> > + argv_array_push(&submodule_options, \"-v\");\n> > if (show_cached)\n> > - argv_array_push(&submodules_options, \"--cached\");\n> > + argv_array_push(&submodule_options, \"--cached\");\n> > if (show_eol)\n> > - argv_array_push(&submodules_options, \"--eol\");\n> > + argv_array_push(&submodule_options, \"--eol\");\n> > if (debug_mode)\n> > - argv_array_push(&submodules_options, \"--debug\");\n> > + argv_array_push(&submodule_options, \"--debug\");\n> \n> Up to here we only rename a variable? If you want to help reviewers,\n> please separate this into two patches. One refactoring, stating it doesn't\n> change behavior; and the other adding the behavioral changes.\n\nI can do that.\n\n> \n> > +\n> > + /* Add Pathspecs */\n> > + argv_array_push(&submodule_options, \"--\");\n> > + for (; *argv; argv++)\n> > + argv_array_push(&submodule_options, *argv);\n> > }\n\n-- \nBrandon Williams\n"},{"id":"314354","messageId":"20170317172257.4690-4-bmwill@google.com","threadId":"45217","inReplyTo":"20170317172257.4690-1-bmwill@google.com","subject":"[PATCH v3 3/5] grep: fix bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T17:22:55Z","receivedAt":"2017-03-17T17:23:23Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When using the --recurse-submodules flag with a relative pathspec which\nincludes \"..\", an error is produced inside the child process spawned for\na submodule. When creating the pathspec struct in the child, the \"..\"\nis interpreted to mean \"go up a directory\" which causes an error stating\nthat the path \"..\" is outside of the repository.\n\nWhile it is true that \"..\" is outside the scope of the submodule, it is\nconfusing to a user who originally invoked the command where \"..\" was\nindeed still inside the scope of the superproject. Since the child\nprocess launched for the submodule has some context that it is operating\nunderneath a superproject, this error could be avoided.\n\nThis patch fixes the bug by passing the 'prefix' to the child process.\nNow each child process that works on a submodule has two points of\nreference to the superproject: (1) the 'super_prefix' which is the path\nfrom the root of the superproject down to root of the submodule and (2)\nthe 'prefix' which is the path from the root of the superproject down to\nthe directory where the user invoked the git command.\n\nWith these two pieces of information a child process can correctly\ninterpret the pathspecs provided by the user as well as being able to\nproperly format its output relative to the directory the user invoked\nthe original command from.\n\nSigned-off-by: Brandon Williams \n---\n builtin/grep.c | 39 ++++++++++++--------\n t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 99 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 4694e68f3..b5d846393 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -310,10 +310,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n \n-\tif (opt->relative && opt->prefix_length) {\n-\t\tquote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);\n-\t\tstrbuf_insert(&pathbuf, 0, filename, tree_name_len);\n-\t} else if (super_prefix) {\n+\tif (super_prefix) {\n \t\tstrbuf_add(&pathbuf, filename, tree_name_len);\n \t\tstrbuf_addstr(&pathbuf, super_prefix);\n \t\tstrbuf_addstr(&pathbuf, filename + tree_name_len);\n@@ -321,6 +318,13 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n+\tif (opt->relative && opt->prefix_length) {\n+\t\tchar *name = strbuf_detach(&pathbuf, NULL);\n+\t\tquote_path_relative(name + tree_name_len, opt->prefix, &pathbuf);\n+\t\tstrbuf_insert(&pathbuf, 0, name, tree_name_len);\n+\t\tfree(name);\n+\t}\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n \t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n@@ -345,12 +349,14 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (super_prefix)\n+\t\tstrbuf_addstr(&buf, super_prefix);\n+\tstrbuf_addstr(&buf, filename);\n+\n \tif (opt->relative && opt->prefix_length) {\n-\t\tquote_path_relative(filename, opt->prefix, &buf);\n-\t} else {\n-\t\tif (super_prefix)\n-\t\t\tstrbuf_addstr(&buf, super_prefix);\n-\t\tstrbuf_addstr(&buf, filename);\n+\t\tchar *name = strbuf_detach(&buf, NULL);\n+\t\tquote_path_relative(name, opt->prefix, &buf);\n+\t\tfree(name);\n \t}\n \n #ifndef NO_PTHREADS\n@@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n }\n \n static void compile_submodule_options(const struct grep_opt *opt,\n-\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t const char **argv,\n \t\t\t\t int cached, int untracked,\n \t\t\t\t int opt_exclude, int use_index,\n \t\t\t\t int pattern_type_arg)\n {\n \tstruct grep_pat *pattern;\n-\tint i;\n \n \tif (recurse_submodules)\n \t\targv_array_push(&submodule_options, \"--recurse-submodules\");\n@@ -523,9 +528,8 @@ static void compile_submodule_options(const struct grep_opt *opt,\n \n \t/* Add Pathspecs */\n \targv_array_push(&submodule_options, \"--\");\n-\tfor (i = 0; i < pathspec->nr; i++)\n-\t\targv_array_push(&submodule_options,\n-\t\t\t\tpathspec->items[i].original);\n+\tfor (; *argv; argv++)\n+\t\targv_array_push(&submodule_options, *argv);\n }\n \n /*\n@@ -549,6 +553,11 @@ static int grep_submodule_launch(struct grep_opt *opt,\n \tprepare_submodule_repo_env(&cp.env_array);\n \targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n \n+\tif (opt->relative && opt->prefix_length)\n+\t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n+\t\t\t\t GIT_TOPLEVEL_PREFIX_ENVIRONMENT,\n+\t\t\t\t opt->prefix);\n+\n \t/* Add super prefix */\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n@@ -1236,7 +1245,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \n \tif (recurse_submodules) {\n \t\tgitmodules_config();\n-\t\tcompile_submodule_options(&opt, &pathspec, cached, untracked,\n+\t\tcompile_submodule_options(&opt, argv + i, cached, untracked,\n \t\t\t\t\t opt_exclude, use_index,\n \t\t\t\t\t pattern_type_arg);\n \t}\ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 67247a01d..5b6eb3a65 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -227,6 +227,81 @@ test_expect_success 'grep history with moved submoules' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'grep using relative path' '\n+\ttest_when_finished \"rm -rf parent sub\" &&\n+\tgit init sub &&\n+\techo \"foobar\" >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\n+\tgit init parent &&\n+\techo \"foobar\" >parent/file &&\n+\tgit -C parent add file &&\n+\tmkdir parent/src &&\n+\techo \"foobar\" >parent/src/file2 &&\n+\tgit -C parent add src/file2 &&\n+\tgit -C parent submodule add ../sub &&\n+\tgit -C parent commit -m \"add files and submodule\" &&\n+\n+\t# From top works\n+\tcat >expect <<-\\EOF &&\n+\tfile:foobar\n+\tsrc/file2:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to top\n+\tcat >expect <<-\\EOF &&\n+\t../file:foobar\n+\tfile2:foobar\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule\n+\tcat >expect <<-\\EOF &&\n+\t../sub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" -- ../sub >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep from a subdir' '\n+\ttest_when_finished \"rm -rf parent sub\" &&\n+\tgit init sub &&\n+\techo \"foobar\" >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\n+\tgit init parent &&\n+\tmkdir parent/src &&\n+\techo \"foobar\" >parent/src/file &&\n+\tgit -C parent add src/file &&\n+\tgit -C parent submodule add ../sub src/sub &&\n+\tgit -C parent submodule add ../sub sub &&\n+\tgit -C parent commit -m \"add files and submodules\" &&\n+\n+\t# Verify grep from root works\n+\tcat >expect <<-\\EOF &&\n+\tsrc/file:foobar\n+\tsrc/sub/file:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Verify grep from a subdir works\n+\tcat >expect <<-\\EOF &&\n+\tfile:foobar\n+\tsub/file:foobar\n+\tEOF\n+\tgit -C parent/src grep --recurse-submodules -e \"foobar\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_incompatible_with_recurse_submodules ()\n {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314355","messageId":"20170317172257.4690-6-bmwill@google.com","threadId":"45217","inReplyTo":"20170317172257.4690-1-bmwill@google.com","subject":"[PATCH v3 5/5] ls-files: fix bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T17:22:57Z","receivedAt":"2017-03-17T17:23:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"When using the --recurse-submodules flag with a relative pathspec which\nincludes \"..\", an error is produced inside the child process spawned for a\nsubmodule. When creating the pathspec struct in the child, the \"..\" is\ninterpreted to mean \"go up a directory\" which causes an error stating that the\npath \"..\" is outside of the repository.\n\nWhile it is true that \"..\" is outside the scope of the submodule, it is\nconfusing to a user who originally invoked the command where \"..\" was indeed\nstill inside the scope of the superproject. Since the child process launched\nfor the submodule has some context that it is operating underneath a\nsuperproject, this error could be avoided.\n\nThis patch fixes the bug by passing the 'prefix' to the child process. Now\neach child process that works on a submodule has two points of reference to the\nsuperproject: (1) the 'super_prefix' which is the path from the root of the\nsuperproject down to root of the submodule and (2) the 'prefix' which is the\npath from the root of the superproject down to the directory where the user\ninvoked the git command.\n\nWith these two pieces of information a child process can correctly interpret\nthe pathspecs provided by the user as well as being able to properly format its\noutput relative to the directory the user invoked the original command from.\n\nSigned-off-by: Brandon Williams \n---\n builtin/ls-files.c | 25 +++++++++++-----------\n t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex ca5b48db0..d449e46db 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -172,7 +172,9 @@ static void show_killed_files(struct dir_struct *dir)\n /*\n * Compile an argv_array with all of the options supported by --recurse_submodules\n */\n-static void compile_submodule_options(const struct dir_struct *dir, int show_tag)\n+static void compile_submodule_options(const char **argv,\n+\t\t\t\t const struct dir_struct *dir,\n+\t\t\t\t int show_tag)\n {\n \tif (line_terminator == '\\0')\n \t\targv_array_push(&submodule_options, \"-z\");\n@@ -186,6 +188,11 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag\n \t\targv_array_push(&submodule_options, \"--eol\");\n \tif (debug_mode)\n \t\targv_array_push(&submodule_options, \"--debug\");\n+\n+\t/* Add Pathspecs */\n+\targv_array_push(&submodule_options, \"--\");\n+\tfor (; *argv; argv++)\n+\t\targv_array_push(&submodule_options, *argv);\n }\n \n /**\n@@ -195,8 +202,11 @@ static void show_gitlink(const struct cache_entry *ce)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n-\tint i;\n \n+\tif (prefix_len)\n+\t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n+\t\t\t\t GIT_TOPLEVEL_PREFIX_ENVIRONMENT,\n+\t\t\t\t prefix);\n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\t super_prefix ? super_prefix : \"\",\n \t\t\t ce->name);\n@@ -206,15 +216,6 @@ static void show_gitlink(const struct cache_entry *ce)\n \t/* add supported options */\n \targv_array_pushv(&cp.args, submodule_options.argv);\n \n-\t/*\n-\t * Pass in the original pathspec args. The submodule will be\n-\t * responsible for prepending the 'submodule_prefix' prior to comparing\n-\t * against the pathspec for matches.\n-\t */\n-\targv_array_push(&cp.args, \"--\");\n-\tfor (i = 0; i < pathspec.nr; i++)\n-\t\targv_array_push(&cp.args, pathspec.items[i].original);\n-\n \tcp.git_cmd = 1;\n \tcp.dir = ce->name;\n \tstatus = run_command(&cp);\n@@ -604,7 +605,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tsetup_work_tree();\n \n \tif (recurse_submodules)\n-\t\tcompile_submodule_options(&dir, show_tag);\n+\t\tcompile_submodule_options(argv, &dir, show_tag);\n \n \tif (recurse_submodules &&\n \t (show_stage || show_deleted || show_others || show_unmerged ||\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex a5426171d..4cf6ccf5a 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -188,6 +188,45 @@ test_expect_success '--recurse-submodules and pathspecs' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--recurse-submodules and relative paths' '\n+\t# From subdir\n+\tcat >expect <<-\\EOF &&\n+\tb\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to top\n+\tcat >expect <<-\\EOF &&\n+\t../.gitmodules\n+\t../a\n+\tb\n+\t../h.txt\n+\t../sib/file\n+\t../sub/file\n+\t../submodule/.gitmodules\n+\t../submodule/c\n+\t../submodule/f.TXT\n+\t../submodule/g.txt\n+\t../submodule/subsub/d\n+\t../submodule/subsub/e.txt\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules -- .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule\n+\tcat >expect <<-\\EOF &&\n+\t../submodule/.gitmodules\n+\t../submodule/c\n+\t../submodule/f.TXT\n+\t../submodule/g.txt\n+\t../submodule/subsub/d\n+\t../submodule/subsub/e.txt\n+\tEOF\n+\tgit -C b ls-files --recurse-submodules -- ../submodule >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--recurse-submodules does not support --error-unmatch' '\n \ttest_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&\n \ttest_i18ngrep \"does not support --error-unmatch\" actual\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314356","messageId":"20170317172257.4690-5-bmwill@google.com","threadId":"45217","inReplyTo":"20170317172257.4690-1-bmwill@google.com","subject":"[PATCH v3 4/5] ls-files: fix typo in variable name","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T17:22:56Z","receivedAt":"2017-03-17T17:23:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Signed-off-by: Brandon Williams \n---\n builtin/ls-files.c | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 1c0f057d0..ca5b48db0 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -30,7 +30,7 @@ static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n static int recurse_submodules;\n-static struct argv_array submodules_options = ARGV_ARRAY_INIT;\n+static struct argv_array submodule_options = ARGV_ARRAY_INIT;\n \n static const char *prefix;\n static const char *super_prefix;\n@@ -175,17 +175,17 @@ static void show_killed_files(struct dir_struct *dir)\n static void compile_submodule_options(const struct dir_struct *dir, int show_tag)\n {\n \tif (line_terminator == '\\0')\n-\t\targv_array_push(&submodules_options, \"-z\");\n+\t\targv_array_push(&submodule_options, \"-z\");\n \tif (show_tag)\n-\t\targv_array_push(&submodules_options, \"-t\");\n+\t\targv_array_push(&submodule_options, \"-t\");\n \tif (show_valid_bit)\n-\t\targv_array_push(&submodules_options, \"-v\");\n+\t\targv_array_push(&submodule_options, \"-v\");\n \tif (show_cached)\n-\t\targv_array_push(&submodules_options, \"--cached\");\n+\t\targv_array_push(&submodule_options, \"--cached\");\n \tif (show_eol)\n-\t\targv_array_push(&submodules_options, \"--eol\");\n+\t\targv_array_push(&submodule_options, \"--eol\");\n \tif (debug_mode)\n-\t\targv_array_push(&submodules_options, \"--debug\");\n+\t\targv_array_push(&submodule_options, \"--debug\");\n }\n \n /**\n@@ -204,7 +204,7 @@ static void show_gitlink(const struct cache_entry *ce)\n \targv_array_push(&cp.args, \"--recurse-submodules\");\n \n \t/* add supported options */\n-\targv_array_pushv(&cp.args, submodules_options.argv);\n+\targv_array_pushv(&cp.args, submodule_options.argv);\n \n \t/*\n \t * Pass in the original pathspec args. The submodule will be\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314357","messageId":"20170317172257.4690-3-bmwill@google.com","threadId":"45217","inReplyTo":"20170317172257.4690-1-bmwill@google.com","subject":"[PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T17:22:54Z","receivedAt":"2017-03-17T17:23:27Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In a future patch child processes which act on submodules need a little\nmore context about the original command that was invoked. This patch\nteaches git to use the prefix stored in `GIT_INTERNAL_TOPLEVEL_PREFIX`\ninstead of the prefix that was potentally found during the git directory\nsetup process.\n\nSigned-off-by: Brandon Williams \n---\n cache.h | 1 +\n git.c | 2 --\n setup.c | 7 ++++++-\n 3 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 8c0e64420..7d253a078 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -410,6 +410,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_WORK_TREE_ENVIRONMENT \"GIT_WORK_TREE\"\n #define GIT_PREFIX_ENVIRONMENT \"GIT_PREFIX\"\n #define GIT_SUPER_PREFIX_ENVIRONMENT \"GIT_INTERNAL_SUPER_PREFIX\"\n+#define GIT_TOPLEVEL_PREFIX_ENVIRONMENT \"GIT_INTERNAL_TOPLEVEL_PREFIX\"\n #define DEFAULT_GIT_DIR_ENVIRONMENT \".git\"\n #define DB_ENVIRONMENT \"GIT_OBJECT_DIRECTORY\"\n #define INDEX_ENVIRONMENT \"GIT_INDEX_FILE\"\ndiff --git a/git.c b/git.c\nindex 33f52acbc..8ff44f081 100644\n--- a/git.c\n+++ b/git.c\n@@ -361,8 +361,6 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \tif (!help && get_super_prefix()) {\n \t\tif (!(p->option & SUPPORT_SUPER_PREFIX))\n \t\t\tdie(\"%s doesn't support --super-prefix\", p->cmd);\n-\t\tif (prefix)\n-\t\t\tdie(\"can't use --super-prefix from a subdirectory\");\n \t}\n \n \tif (!help && p->option & NEED_WORK_TREE)\ndiff --git a/setup.c b/setup.c\nindex 8f64fbdfb..0d76b9828 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -939,9 +939,14 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n \n const char *setup_git_directory_gently(int *nongit_ok)\n {\n-\tconst char *prefix;\n+\tconst char *prefix, *env_prefix;\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n+\tenv_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n+\n+\tif (env_prefix)\n+\t\tprefix = env_prefix;\n+\n \tif (prefix)\n \t\tsetenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n \telse\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314358","messageId":"20170317172257.4690-1-bmwill@google.com","threadId":"45217","inReplyTo":"20170314221100.24856-1-bmwill@google.com","subject":"[PATCH v3 0/5] recursing submodules with relative pathspec (grep and ls-files)","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T17:22:52Z","receivedAt":"2017-03-17T17:23:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Changes in v3:\n* Added sign-off to the patches where it was missing.\n* slight tweak to the style and commit msgs of a few patches.\n* broke up the last patch (for ls-files) into a typo fix followed by fixing the\n bug itself. This was to make the diff easier to review.\n\nBrandon Williams (5):\n grep: fix help text typo\n setup: allow for prefix to be passed to git commands\n grep: fix bug when recursing with relative pathspec\n ls-files: fix typo in variable name\n ls-files: fix bug when recursing with relative pathspec\n\n builtin/grep.c | 41 +++++++++++--------\n builtin/ls-files.c | 41 ++++++++++---------\n cache.h | 1 +\n git.c | 2 -\n setup.c | 7 +++-\n t/t3007-ls-files-recurse-submodules.sh | 39 ++++++++++++++++++\n t/t7814-grep-recurse-submodules.sh | 75 ++++++++++++++++++++++++++++++++++\n 7 files changed, 167 insertions(+), 39 deletions(-)\n\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314359","messageId":"20170317172257.4690-2-bmwill@google.com","threadId":"45217","inReplyTo":"20170317172257.4690-1-bmwill@google.com","subject":"[PATCH v3 1/5] grep: fix help text typo","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T17:22:53Z","receivedAt":"2017-03-17T17:23:42Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Signed-off-by: Brandon Williams \n---\n builtin/grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 9304c33e7..4694e68f3 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -979,7 +979,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT(0, \"exclude-standard\", &opt_exclude,\n \t\t\t N_(\"ignore files specified via '.gitignore'\"), 1),\n \t\tOPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n-\t\t\t N_(\"recursivley search in each submodule\")),\n+\t\t\t N_(\"recursively search in each submodule\")),\n \t\tOPT_STRING(0, \"parent-basename\", &parent_basename,\n \t\t\t N_(\"basename\"),\n \t\t\t N_(\"prepend parent project's basename to output\")),\n-- \n2.12.0.367.g23dc2f6d3c-goog\n\n"},{"id":"314377","messageId":"CAGZ79kZAZeb5rsL80dty_tRM5SDCAXVq_yATpDQREq_vV4Yj+Q@mail.gmail.com","threadId":"45217","inReplyTo":"20170317172257.4690-3-bmwill@google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-17T19:07:46Z","receivedAt":"2017-03-17T19:08:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> prefix = setup_git_directory_gently_1(nongit_ok);\n> + env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n> +\n> + if (env_prefix)\n> + prefix = env_prefix;\n> +\n> if (prefix)\n> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n\nso we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix\nfirst, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,\nsuch that e.g. aliased commands will know about the superprefix, too.\n\nok, sounds reasonable to me; though I do not use this feature,\nso my judgement is not as good.\n\nDo we need a test for this behavior?\n\nThanks,\nStefan\n"},{"id":"314378","messageId":"CAGZ79kaTNVZBZg5jiVzAKdu96pENcRJmvZ1WR37TP2MXLR-nUg@mail.gmail.com","threadId":"45217","inReplyTo":"20170317190856.GC110341@google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-17T19:10:39Z","receivedAt":"2017-03-17T19:18:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 17, 2017 at 12:08 PM, Brandon Williams wrote:\n> On 03/17, Stefan Beller wrote:\n>> > prefix = setup_git_directory_gently_1(nongit_ok);\n>> > + env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n>> > +\n>> > + if (env_prefix)\n>> > + prefix = env_prefix;\n>> > +\n>> > if (prefix)\n>> > setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n>>\n>> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix\n>> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,\n>> such that e.g. aliased commands will know about the superprefix, too.\n>\n> I don't follow, this doesn't have anything to do with super-prefix.\n>\n\ns/superprefix/prefix as passed in via GIT_TOPLEVEL_PREFIX_ENVIRONMENT/\n\nsorry for the confusion.\n"},{"id":"314379","messageId":"xmqq60j77lx3.fsf@gitster.mtv.corp.google.com","threadId":"45217","inReplyTo":"CAGZ79kZAZeb5rsL80dty_tRM5SDCAXVq_yATpDQREq_vV4Yj+Q@mail.gmail.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-17T19:17:28Z","receivedAt":"2017-03-17T19:18:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> prefix = setup_git_directory_gently_1(nongit_ok);\n>> + env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n>> +\n>> + if (env_prefix)\n>> + prefix = env_prefix;\n>> +\n>> if (prefix)\n>> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n>\n> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix\n> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,\n> such that e.g. aliased commands will know about the superprefix, too.\n\nIf the aliased commands or anything else spawned from this process\nis happy with GIT_PREFIX set to the outside of the current\nrepository, doing this setenv() is OK. If you are in ~/dir1, and\nyour repository is in ~/repos/repo1, and if you somehow had a way\nto run your \"git\" inside ~/repos/repo1 without doing any chdir(2),\nthen you are essentially setting ../../dir1/ as GIT_PREFIX for that\n\"git\" invocation (this has nothing to do with submodules).\n\nBut if your \"git\" is fine with GIT_PREFIX pointing outside the root\nlevel of the working tree of the current repository like that, do we\neven need a separate toplevel prefix environment, I have to wonder?\n\nThat is, if this \"if TOPLEVEL_PREFIX environment is there, set it to\nlocal variable prefix and export it as GIT_PREFIX\" is expected to\nwork correctly for anything that would inherit that GIT_PREFIX, then\nwe should be able to invoke the \"git\" that got TOPLEVEL_PREFIX\nwithout setting that environment, but instead setting the same value\nto GIT_PREFIX and we should get the same behaviour, no?\n\n"},{"id":"314380","messageId":"20170317190856.GC110341@google.com","threadId":"45217","inReplyTo":"CAGZ79kZAZeb5rsL80dty_tRM5SDCAXVq_yATpDQREq_vV4Yj+Q@mail.gmail.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T19:08:56Z","receivedAt":"2017-03-17T19:18:26Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Stefan Beller wrote:\n> > prefix = setup_git_directory_gently_1(nongit_ok);\n> > + env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n> > +\n> > + if (env_prefix)\n> > + prefix = env_prefix;\n> > +\n> > if (prefix)\n> > setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n> \n> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix\n> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,\n> such that e.g. aliased commands will know about the superprefix, too.\n\nI don't follow, this doesn't have anything to do with super-prefix.\n\n> \n> ok, sounds reasonable to me; though I do not use this feature,\n> so my judgement is not as good.\n> \n> Do we need a test for this behavior?\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"314381","messageId":"20170317191719.GD110341@google.com","threadId":"45217","inReplyTo":"CAGZ79kaTNVZBZg5jiVzAKdu96pENcRJmvZ1WR37TP2MXLR-nUg@mail.gmail.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T19:17:19Z","receivedAt":"2017-03-17T19:18:28Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Stefan Beller wrote:\n> On Fri, Mar 17, 2017 at 12:08 PM, Brandon Williams wrote:\n> > On 03/17, Stefan Beller wrote:\n> >> > prefix = setup_git_directory_gently_1(nongit_ok);\n> >> > + env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n> >> > +\n> >> > + if (env_prefix)\n> >> > + prefix = env_prefix;\n> >> > +\n> >> > if (prefix)\n> >> > setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n> >>\n> >> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix\n> >> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,\n> >> such that e.g. aliased commands will know about the superprefix, too.\n> >\n> > I don't follow, this doesn't have anything to do with super-prefix.\n> >\n> \n> s/superprefix/prefix as passed in via GIT_TOPLEVEL_PREFIX_ENVIRONMENT/\n> \n> sorry for the confusion.\n\nAlternatively we could not copy it into GIT_PREFIX_ENVIRONMENT.\n\n-- \nBrandon Williams\n"},{"id":"314382","messageId":"20170317192103.GE110341@google.com","threadId":"45217","inReplyTo":"xmqq60j77lx3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T19:21:03Z","receivedAt":"2017-03-17T19:21:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Junio C Hamano wrote:\n> Stefan Beller writes:\n> \n> >> prefix = setup_git_directory_gently_1(nongit_ok);\n> >> + env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n> >> +\n> >> + if (env_prefix)\n> >> + prefix = env_prefix;\n> >> +\n> >> if (prefix)\n> >> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);\n> >\n> > so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix\n> > first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,\n> > such that e.g. aliased commands will know about the superprefix, too.\n> \n> If the aliased commands or anything else spawned from this process\n> is happy with GIT_PREFIX set to the outside of the current\n> repository, doing this setenv() is OK. If you are in ~/dir1, and\n> your repository is in ~/repos/repo1, and if you somehow had a way\n> to run your \"git\" inside ~/repos/repo1 without doing any chdir(2),\n> then you are essentially setting ../../dir1/ as GIT_PREFIX for that\n> \"git\" invocation (this has nothing to do with submodules).\n> \n> But if your \"git\" is fine with GIT_PREFIX pointing outside the root\n> level of the working tree of the current repository like that, do we\n> even need a separate toplevel prefix environment, I have to wonder?\n> \n> That is, if this \"if TOPLEVEL_PREFIX environment is there, set it to\n> local variable prefix and export it as GIT_PREFIX\" is expected to\n> work correctly for anything that would inherit that GIT_PREFIX, then\n> we should be able to invoke the \"git\" that got TOPLEVEL_PREFIX\n> without setting that environment, but instead setting the same value\n> to GIT_PREFIX and we should get the same behaviour, no?\n> \n\nVery true, potentially we could just use GIT_PREFIX instead of\nintroducing a brand new env var (which is essentially just the same\nthing). I was being cautious with this patch since git didn't currently\nread GIT_PREFIX. I was hoping other with more knowledge in this area\nwould voice their opinions and lead me in the right direction ;)\n\n-- \nBrandon Williams\n"},{"id":"314386","messageId":"xmqqy3w363yx.fsf@gitster.mtv.corp.google.com","threadId":"45217","inReplyTo":"20170317192103.GE110341@google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-17T20:30:30Z","receivedAt":"2017-03-17T20:30:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> ... I was being cautious with this patch since git didn't currently\n> read GIT_PREFIX.\n\nAhh, I forgot about that. Processes we spawn do expect GIT_PREFIX\nto tell them where the original $cwd was and they also do expect\nthat \"git\" invoked by them would not be affected by GIT_PREFIX\nenvironment variable. So we cannot change that now.\n\nIf you recurse into sub-sub module, it is likely that you would want\nto update the TOPLEVEL_PREFIX relative to that sub-sub module you\nare descending into.\n\nThat probably also means that processes we spawn now need to also\npay attention to TOPLEVEL_PREFIX in addition to GIT_PREFIX, and we\nshould NOT re-export what we got from TOPLEVEL_PREFIX to GIT_PREFIX.\nI.e. if a \"git\" process started from src/ subdirectory of the\nsuperproject that goes into module/sub1/ submodule, top-level prefix\nmay export ../src/ to point at the original location, but the\nprocess that is running in the submodule will be running at the root\nlevel of the submodule working tree, so its prefix should be NULL or\n\"\", no?\n\nAdjusting pathspec and other file references on the caller's side,\ninstead of exporting toplevel-prefix to have them adjusted by the\ncallee, started to smell more and more like an easier/more correct\napproach to me, but perhaps I haven't thought things deeply enough.\n\nI dunno.\n\n"},{"id":"314388","messageId":"20170317210031.GA63813@google.com","threadId":"45217","inReplyTo":"xmqqy3w363yx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-17T21:00:31Z","receivedAt":"2017-03-17T21:09:51Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > ... I was being cautious with this patch since git didn't currently\n> > read GIT_PREFIX.\n> \n> Ahh, I forgot about that. Processes we spawn do expect GIT_PREFIX\n> to tell them where the original $cwd was and they also do expect\n> that \"git\" invoked by them would not be affected by GIT_PREFIX\n> environment variable. So we cannot change that now.\n> \n\nYep. I just ran the test suite locally doing this...it fails on a bunch\nof stuff :)\n\n> If you recurse into sub-sub module, it is likely that you would want\n> to update the TOPLEVEL_PREFIX relative to that sub-sub module you\n> are descending into.\n\nSo the idea would be that 'super_prefix' would keep getting updated when\nrecursing deeper, but the 'prefix' should stay the same (in what I'm\nproposing here that is).\n\n> \n> That probably also means that processes we spawn now need to also\n> pay attention to TOPLEVEL_PREFIX in addition to GIT_PREFIX, and we\n> should NOT re-export what we got from TOPLEVEL_PREFIX to GIT_PREFIX.\n> I.e. if a \"git\" process started from src/ subdirectory of the\n> superproject that goes into module/sub1/ submodule, top-level prefix\n> may export ../src/ to point at the original location, but the\n> process that is running in the submodule will be running at the root\n> level of the submodule working tree, so its prefix should be NULL or\n> \"\", no?\n\nI don't think that prefix can ever have \"..\" in it. From what I\nunderstand it is always a path from the root of the repository to the\ncwd that the git command was invoked by. So in your example prefix\nwould be \"src/\".\n\n> \n> Adjusting pathspec and other file references on the caller's side,\n> instead of exporting toplevel-prefix to have them adjusted by the\n> callee, started to smell more and more like an easier/more correct\n> approach to me, but perhaps I haven't thought things deeply enough.\n> \n> I dunno.\n> \n\nYeah, doing all of the adjusting on the caller's side is quite\ndifficult. Getting the pathspecs and file references right seems to be\noverly complicated if done by the caller. That why I opted to punt to\nthe callee; given enough information (e.g. prefix and super_prefix) the\ncallee should be able to handle both the pathspecs and file references\nproperly.\n\n-- \nBrandon Williams\n"},{"id":"314389","messageId":"xmqqinn761f7.fsf@gitster.mtv.corp.google.com","threadId":"45217","inReplyTo":"20170317210031.GA63813@google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-17T21:25:32Z","receivedAt":"2017-03-17T21:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> I don't think that prefix can ever have \"..\" in it. From what I\n> understand it is always a path from the root of the repository to the\n> cwd that the git command was invoked by. So in your example prefix\n> would be \"src/\".\n\nThe prefix would be NULL or \"\", as you will be at the root-level of\nthe working tree when you are running _IN_ the submodule (by\nrecursing into it). Not src/, nor anything with ../ in it, I would\nthink.\n"},{"id":"314740","messageId":"20170320223418.GB188475@google.com","threadId":"45217","inReplyTo":"xmqqinn761f7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-20T22:34:18Z","receivedAt":"2017-03-20T22:34:33Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/17, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > I don't think that prefix can ever have \"..\" in it. From what I\n> > understand it is always a path from the root of the repository to the\n> > cwd that the git command was invoked by. So in your example prefix\n> > would be \"src/\".\n> \n> The prefix would be NULL or \"\", as you will be at the root-level of\n> the working tree when you are running _IN_ the submodule (by\n> recursing into it). Not src/, nor anything with ../ in it, I would\n> think.\n\nYes, the prefix that is found during setup of a submodule process would\nbe NULL or \"\" as the command would be invoked from the root of that\nrepository. This series would sort of change that though.\n\nIf a command was invoked from 'src/' with a pathspec of '../dir/' and\nthere is a submodule at 'dir/sub', the process working on the submodule\nwill have the following:\n\nsuper_prefix = 'dir/sub/'\nprefix = 'src/' (Passed from the parent process via the\n GIT_INTERNAL_TOPLEVEL_PREFIX env var)\npathspec = '../dir/'\n\nWith that information the child process will be able to properly resolve\nthe pathspec to be 'dir/' (using the prefix) and will be able to match\nagainst it by pre-pending the super_prefix (e.g. dir/sub/some/file, where\nsome/file is a file in the submodule). It will also be able to generate\ncorrect output relative to the directory the command was originally\ninvoked from by first pre-pending the super_prefix so we have\n'dir/sub/some/file' and then calling relative_path() with the prefix\nthat was passed in such that the output for this file looks like\n'../dir/sub/some/file'\n\nThat the gist of how I'm hoping to solve the problem. Hopefully that was\nclear enough to get some feedback on.\n\n-- \nBrandon Williams\n"},{"id":"314790","messageId":"CACsJy8Cu8cgtJzDDM09GoJr5Ny+G+nP17GjZjXng6ZPQv9eXXg@mail.gmail.com","threadId":"45217","inReplyTo":"20170317172257.4690-4-bmwill@google.com","subject":"Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-21T11:47:10Z","receivedAt":"2017-03-21T11:47:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 18, 2017 at 12:22 AM, Brandon Williams wrote:\n> With these two pieces of information a child process can correctly\n> interpret the pathspecs provided by the user as well as being able to\n> properly format its output relative to the directory the user invoked\n> the original command from.\n\nThis part can stand alone as a separate patch right? It would help\nfocus on the pathspec thingy first.\n\n> @@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n> }\n>\n> static void compile_submodule_options(const struct grep_opt *opt,\n> - const struct pathspec *pathspec,\n> + const char **argv,\n> int cached, int untracked,\n> int opt_exclude, int use_index,\n> int pattern_type_arg)\n> {\n> struct grep_pat *pattern;\n> - int i;\n>\n> if (recurse_submodules)\n> argv_array_push(&submodule_options, \"--recurse-submodules\");\n\nSide note. It would be awesome if you could make parse_options() (or a\nnew function) do the reverse process: given a 'struct option' with\nvalid data, spit out argv_array. Less worrying about git-grep having\nnew option but not passed to subgrep by accident. You can have a new\nflag to tell it to ignore certain options if you don't want to pass\nall.\n-- \nDuy\n"},{"id":"314819","messageId":"xmqqmvcesh5q.fsf@gitster.mtv.corp.google.com","threadId":"45217","inReplyTo":"20170320223418.GB188475@google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T16:56:01Z","receivedAt":"2017-03-21T16:56:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Yes, the prefix that is found during setup of a submodule process would\n> be NULL or \"\" as the command would be invoked from the root of that\n> repository. This series would sort of change that though.\n\nOK, because you (eh, rather your caller) do not adjust pathnames and\npathspecs when calling into a submodule, to the process setting at\nthe top-level of the submodule, the paths and pathnames given by the\nend user cannot be taken as-is, but needs to be adjusted inside the\ncontext they were given, i.e. taking account the relative location\nbetween where the user started and where the submodule is bound in\nthe superproject's tree. I forgot about that and \"must be NULL at\nthe top, no?\" was a nonsense.\n\n> That the gist of how I'm hoping to solve the problem. Hopefully that was\n> clear enough to get some feedback on.\n\nYup, understood (I think). Thanks.\n"},{"id":"314827","messageId":"xmqqzigeqzsz.fsf@gitster.mtv.corp.google.com","threadId":"45217","inReplyTo":"CACsJy8Cu8cgtJzDDM09GoJr5Ny+G+nP17GjZjXng6ZPQv9eXXg@mail.gmail.com","subject":"Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T17:56:12Z","receivedAt":"2017-03-21T17:59:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> if (recurse_submodules)\n>> argv_array_push(&submodule_options, \"--recurse-submodules\");\n>\n> Side note. It would be awesome if you could make parse_options() (or a\n> new function) do the reverse process: given a 'struct option' with\n> valid data, spit out argv_array. Less worrying about git-grep having\n> new option but not passed to subgrep by accident. You can have a new\n> flag to tell it to ignore certain options if you don't want to pass\n> all.\n\nIt indeed would be awesome. I do not offhand know if it would be\nfeasible or the result is easy to use without mistakes, though.\n"},{"id":"314984","messageId":"20170322214654.GC11254@google.com","threadId":"45217","inReplyTo":"CACsJy8Cu8cgtJzDDM09GoJr5Ny+G+nP17GjZjXng6ZPQv9eXXg@mail.gmail.com","subject":"Re: [PATCH v3 3/5] grep: fix bug when recursing with relative pathspec","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-03-22T21:46:54Z","receivedAt":"2017-03-22T21:47:02Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 03/21, Duy Nguyen wrote:\n> On Sat, Mar 18, 2017 at 12:22 AM, Brandon Williams wrote:\n> > With these two pieces of information a child process can correctly\n> > interpret the pathspecs provided by the user as well as being able to\n> > properly format its output relative to the directory the user invoked\n> > the original command from.\n> \n> This part can stand alone as a separate patch right? It would help\n> focus on the pathspec thingy first.\n\nI guess it could probably be factored out, though it is necessary for it\nto work. The issue I was running into was that when no pathspecs were\ngiven it would create a 'prefix' pathspec. So if we were in directory\n'dir/' the pathspec that would get created would be:\n\nps.match = \"dir/\"\nps.original = \"dir/\"\n\nSince it also set the original field it messed up when the child tried\nto interpret the pathspecs again. I solved this by just passing the raw\npathspec through to the child.\n\n> \n> > @@ -399,13 +405,12 @@ static void run_pager(struct grep_opt *opt, const char *prefix)\n> > }\n> >\n> > static void compile_submodule_options(const struct grep_opt *opt,\n> > - const struct pathspec *pathspec,\n> > + const char **argv,\n> > int cached, int untracked,\n> > int opt_exclude, int use_index,\n> > int pattern_type_arg)\n> > {\n> > struct grep_pat *pattern;\n> > - int i;\n> >\n> > if (recurse_submodules)\n> > argv_array_push(&submodule_options, \"--recurse-submodules\");\n> \n> Side note. It would be awesome if you could make parse_options() (or a\n> new function) do the reverse process: given a 'struct option' with\n> valid data, spit out argv_array. Less worrying about git-grep having\n> new option but not passed to subgrep by accident. You can have a new\n> flag to tell it to ignore certain options if you don't want to pass\n> all.\n\nI thought about this for a second but didn't pursue it very far. Mostly\nbecause of how you would handle options with callback routines. Maybe\nif you want the option to be reversible you need to have an additional\ncallback routine to do the conversion? I agree that having this sort of\nfunctionality would be nice as it does save you from forgetting about\npassing on options to a child process.\n\n-- \nBrandon Williams\n"},{"id":"315635","messageId":"CAGZ79kbCxhs82PD7wiB6UXh2=5k_EfnFYTZYqUs-FJU2WuxDxg@mail.gmail.com","threadId":"45217","inReplyTo":"20170320223418.GB188475@google.com","subject":"Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-28T23:58:29Z","receivedAt":"2017-03-28T23:58:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Mar 20, 2017 at 3:34 PM, Brandon Williams wrote:\n> That the gist of how I'm hoping to solve the problem. Hopefully that was\n> clear enough to get some feedback on.\n\nJunio wrote in \"What's cooking in git.git (Mar 2017, #10; Fri, 24)\"\n> I saw no particular issues myself. Do others find this series good\n> (not just \"meh--it is harmless\" but I want to hear a positive \"yes\n> these are good changes\")?\n\nSo I reviewed them again, and I think they are good to go.\nAs a followup we may want to consider this\n\ndiff --git a/setup.c b/setup.c\nindex 56cd68ba93..bdc294091a 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -944,6 +944,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n prefix = setup_git_directory_gently_1(nongit_ok);\n env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n\n+ if (prefix && env_prefix)\n+ die(\"BUG: can't invoke command in sub directory with %s set\",\n+ GIT_TOPLEVEL_PREFIX_ENVIRONMENT);\n+\n if (env_prefix)\n prefix = env_prefix;\n--\n\nThanks,\nStefan\n"}]}