{"thread":{"id":"45672","subject":"[PATCH] ls-files: properly prepare submodule environment","startedAt":"2017-04-12T00:39:27Z","lastAt":"2017-08-01T18:42:12Z","messageCount":33,"participants":["Jacob Keller","Brandon Williams","Stefan Beller","Junio C Hamano","Paul Jolly","paul@myitcv.io"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316661","messageId":"20170412003911.1142-1-jacob.e.keller@intel.com","threadId":"45672","inReplyTo":null,"subject":"[PATCH] ls-files: properly prepare submodule environment","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-04-12T00:39:11Z","receivedAt":"2017-04-12T00:39:27Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nSince commit e77aa336f116 (\"ls-files: optionally recurse into\nsubmodules\", 2016-10-07) ls-files has known how to recurse into\nsubmodules when displaying files.\n\nUnfortunately this code does not work as expected. Initially, it\nproduced results indicating that a --super-prefix can't be used from\na subdirectory:\n\n fatal: can't use --super-prefix from a subdirectory\n\nAfter commit b58a68c1c187 (\"setup: allow for prefix to be passed to git\ncommands\", 2017-03-17) this behavior changed and resulted in repeated\ncalls of ls-files with an expanding super-prefix.\n\nThis recursive expanding super-prefix appears to be the result of\nls-files acting on the super-project instead of on the submodule files.\n\nWe can fix this by properly preparing the submodule environment when\nsetting up the submodule process. This ensures that the command we\nexecute properly reads the directory and ensures that we correctly\noperate in the submodule instead of repeating oureslves on the\nsuper-project contents forever.\n\nWhile we're here lets also add some tests to ensure that ls-files works\nwith recurse-submodules to catch these issues in the future.\n\nSigned-off-by: Jacob Keller \n---\nI found this fix on accident after looking at git-grep code and\ncomparing how ls-files instantiated the submodule. Can someone who knows\nmore about submodules explain the reasoning better for this fix?\nEssentially we \"recurse\" into the submodule folder, but still operate as\nif we're at the root of the project but with a new prefix. So then we\nkeep recursing into the submodule forever.\n\nI also added some tests here, and I *definitely* think this should be a\nmaintenance backport into any place which supports ls-files\n--recurse-submodules, since as far as I can tell it is otherwise\nuseless.\n\nThere were no tests for it, so I added some based on git-grep tests. I\ndid not try them against the broken setups, because of the endless\nrecursion.\n\n builtin/ls-files.c | 4 +\n t/t3080-ls-files-recurse-submodules.sh | 162 +++++++++++++++++++++++++++++++++\n 2 files changed, 166 insertions(+)\n create mode 100755 t/t3080-ls-files-recurse-submodules.sh\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db551..e9b3546ca053 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -15,6 +15,7 @@\n #include \"string-list.h\"\n #include \"pathspec.h\"\n #include \"run-command.h\"\n+#include \"submodule.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n \n+\tprepare_submodule_repo_env(&cp.env_array);\n+\targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n+\n \tif (prefix_len)\n \t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n \t\t\t\t GIT_TOPLEVEL_PREFIX_ENVIRONMENT,\ndiff --git a/t/t3080-ls-files-recurse-submodules.sh b/t/t3080-ls-files-recurse-submodules.sh\nnew file mode 100755\nindex 000000000000..6788a8f09635\n--- /dev/null\n+++ b/t/t3080-ls-files-recurse-submodules.sh\n@@ -0,0 +1,162 @@\n+#!/bin/sh\n+\n+test_description='Test ls-files recurse-submodules feature\n+\n+This test verifies the recurse-submodules feature correctly lists files across\n+submodules.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup directory structure and submodule' '\n+\techo \"foobar\" >a &&\n+\tmkdir b &&\n+\techo \"bar\" >b/b &&\n+\tgit add a b &&\n+\tgit commit -m \"add a and b\" &&\n+\tgit init submodule &&\n+\techo \"foobar\" >submodule/a &&\n+\tgit -C submodule add a &&\n+\tgit -C submodule commit -m \"add a\" &&\n+\tgit submodule add ./submodule &&\n+\tgit commit -m \"added submodule\"\n+'\n+\n+test_expect_success 'ls-files correctly lists files in a submodule' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/a\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files and basic pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules -- submodule >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files and nested submodules' '\n+\tgit init submodule/sub &&\n+\techo \"foobar\" >submodule/sub/a &&\n+\tgit -C submodule/sub add a &&\n+\tgit -C submodule/sub commit -m \"add a\" &&\n+\tgit -C submodule submodule add ./sub &&\n+\tgit -C submodule add sub &&\n+\tgit -C submodule commit -m \"added sub\" &&\n+\tgit add submodule &&\n+\tgit commit -m \"updated submodule\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/.gitmodules\n+\tsubmodule/a\n+\tsubmodule/sub/a\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files 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+\t.gitmodules\n+\tfile\n+\tsrc/file2\n+\tsub/file\n+\tEOF\n+\tgit -C parent 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../file\n+\tfile2\n+\t../sub/file\n+\tEOF\n+\tgit -C parent/src ls-files --recurse-submodules .. >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Relative path to submodule\n+\tcat >expect <<-\\EOF &&\n+\t../sub/file\n+\tEOF\n+\tgit -C parent/src ls-files --recurse-submodules ../sub >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files 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+\t.gitmodules\n+\tsrc/file\n+\tsrc/sub/file\n+\tsub/file\n+\tEOF\n+\tgit -C parent ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Verify grep from a subdir works\n+\tcat >expect <<-\\EOF &&\n+\tfile\n+\tsub/file\n+\tEOF\n+\tgit -C parent/src ls-files --recurse-submodules >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+\t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n+\t\ttest_i18ngrep -- '--recurse-submodules unsupported mode' actual\n+\t\"\n+}\n+\n+test_incompatible_with_recurse_submodules --deleted\n+test_incompatible_with_recurse_submodules --others\n+test_incompatible_with_recurse_submodules --unmerged\n+test_incompatible_with_recurse_submodules --killed\n+test_incompatible_with_recurse_submodules --modified\n+\n+test_done\n-- \n2.12.2.776.gded3dc243c29.dirty\n\n"},{"id":"316692","messageId":"20170412165851.GA79499@google.com","threadId":"45672","inReplyTo":"20170412003911.1142-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] ls-files: properly prepare submodule environment","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-12T16:58:51Z","receivedAt":"2017-04-12T16:59:11Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/11, Jacob Keller wrote:\n> From: Jacob Keller \n> \n> Since commit e77aa336f116 (\"ls-files: optionally recurse into\n> submodules\", 2016-10-07) ls-files has known how to recurse into\n> submodules when displaying files.\n> \n> Unfortunately this code does not work as expected. Initially, it\n> produced results indicating that a --super-prefix can't be used from\n> a subdirectory:\n> \n> fatal: can't use --super-prefix from a subdirectory\n> \n> After commit b58a68c1c187 (\"setup: allow for prefix to be passed to git\n> commands\", 2017-03-17) this behavior changed and resulted in repeated\n> calls of ls-files with an expanding super-prefix.\n> \n> This recursive expanding super-prefix appears to be the result of\n> ls-files acting on the super-project instead of on the submodule files.\n> \n> We can fix this by properly preparing the submodule environment when\n> setting up the submodule process. This ensures that the command we\n> execute properly reads the directory and ensures that we correctly\n> operate in the submodule instead of repeating oureslves on the\n> super-project contents forever.\n> \n> While we're here lets also add some tests to ensure that ls-files works\n> with recurse-submodules to catch these issues in the future.\n> \n> Signed-off-by: Jacob Keller \n> ---\n> I found this fix on accident after looking at git-grep code and\n> comparing how ls-files instantiated the submodule. Can someone who knows\n> more about submodules explain the reasoning better for this fix?\n> Essentially we \"recurse\" into the submodule folder, but still operate as\n> if we're at the root of the project but with a new prefix. So then we\n> keep recursing into the submodule forever.\n\nThe reason why this fix is required is that the env var GIT_DIR is set\nto be the parents gitdir. When recursing the childprocess just uses the\nparents gitdir instead of its own causing it to recurse into itself\nagain and again. The child process's environment needs to have the\nGIT_DIR var cleared so that the child will do discovery and actually\nfind its own gitdir.\n\n> \n> I also added some tests here, and I *definitely* think this should be a\n> maintenance backport into any place which supports ls-files\n> --recurse-submodules, since as far as I can tell it is otherwise\n> useless.\n> \n> There were no tests for it, so I added some based on git-grep tests. I\n> did not try them against the broken setups, because of the endless\n> recursion.\n\nThere are tests for ls-files --recurse-submodules in\nt3007-ls-files-recurse-submodules.sh, though it looks like this\nparticular case (which triggers this bug) maybe didn't have any tests.\nI'm actually unsure of why the existing tests didn't catch this (I'm\nprobably just bad at writing tests).\n\n\n> \n> builtin/ls-files.c | 4 +\n> t/t3080-ls-files-recurse-submodules.sh | 162 +++++++++++++++++++++++++++++++++\n> 2 files changed, 166 insertions(+)\n> create mode 100755 t/t3080-ls-files-recurse-submodules.sh\n> \n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index d449e46db551..e9b3546ca053 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -15,6 +15,7 @@\n> #include \"string-list.h\"\n> #include \"pathspec.h\"\n> #include \"run-command.h\"\n> +#include \"submodule.h\"\n> \n> static int abbrev;\n> static int show_deleted;\n> @@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tint status;\n> \n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n\nYes these lines need to be included in order to prepare the environment.\nThanks for catching that.\n\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> diff --git a/t/t3080-ls-files-recurse-submodules.sh b/t/t3080-ls-files-recurse-submodules.sh\n> new file mode 100755\n> index 000000000000..6788a8f09635\n> --- /dev/null\n> +++ b/t/t3080-ls-files-recurse-submodules.sh\n> @@ -0,0 +1,162 @@\n> +#!/bin/sh\n> +\n> +test_description='Test ls-files recurse-submodules feature\n> +\n> +This test verifies the recurse-submodules feature correctly lists files across\n> +submodules.\n> +'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup directory structure and submodule' '\n> +\techo \"foobar\" >a &&\n> +\tmkdir b &&\n> +\techo \"bar\" >b/b &&\n> +\tgit add a b &&\n> +\tgit commit -m \"add a and b\" &&\n> +\tgit init submodule &&\n> +\techo \"foobar\" >submodule/a &&\n> +\tgit -C submodule add a &&\n> +\tgit -C submodule commit -m \"add a\" &&\n> +\tgit submodule add ./submodule &&\n> +\tgit commit -m \"added submodule\"\n> +'\n> +\n> +test_expect_success 'ls-files correctly lists files in a submodule' '\n> +\tcat >expect <<-\\EOF &&\n> +\t.gitmodules\n> +\ta\n> +\tb/b\n> +\tsubmodule/a\n> +\tEOF\n> +\n> +\tgit ls-files --recurse-submodules >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'ls-files and basic pathspecs' '\n> +\tcat >expect <<-\\EOF &&\n> +\tsubmodule/a\n> +\tEOF\n> +\n> +\tgit ls-files --recurse-submodules -- submodule >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'ls-files and nested submodules' '\n> +\tgit init submodule/sub &&\n> +\techo \"foobar\" >submodule/sub/a &&\n> +\tgit -C submodule/sub add a &&\n> +\tgit -C submodule/sub commit -m \"add a\" &&\n> +\tgit -C submodule submodule add ./sub &&\n> +\tgit -C submodule add sub &&\n> +\tgit -C submodule commit -m \"added sub\" &&\n> +\tgit add submodule &&\n> +\tgit commit -m \"updated submodule\" &&\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t.gitmodules\n> +\ta\n> +\tb/b\n> +\tsubmodule/.gitmodules\n> +\tsubmodule/a\n> +\tsubmodule/sub/a\n> +\tEOF\n> +\n> +\tgit ls-files --recurse-submodules >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'ls-files 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> +\t.gitmodules\n> +\tfile\n> +\tsrc/file2\n> +\tsub/file\n> +\tEOF\n> +\tgit -C parent 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../file\n> +\tfile2\n> +\t../sub/file\n> +\tEOF\n> +\tgit -C parent/src ls-files --recurse-submodules .. >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# Relative path to submodule\n> +\tcat >expect <<-\\EOF &&\n> +\t../sub/file\n> +\tEOF\n> +\tgit -C parent/src ls-files --recurse-submodules ../sub >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'ls-files 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> +\t.gitmodules\n> +\tsrc/file\n> +\tsrc/sub/file\n> +\tsub/file\n> +\tEOF\n> +\tgit -C parent ls-files --recurse-submodules >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# Verify grep from a subdir works\n> +\tcat >expect <<-\\EOF &&\n> +\tfile\n> +\tsub/file\n> +\tEOF\n> +\tgit -C parent/src ls-files --recurse-submodules >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> +\t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n> +\t\ttest_i18ngrep -- '--recurse-submodules unsupported mode' actual\n> +\t\"\n> +}\n> +\n> +test_incompatible_with_recurse_submodules --deleted\n> +test_incompatible_with_recurse_submodules --others\n> +test_incompatible_with_recurse_submodules --unmerged\n> +test_incompatible_with_recurse_submodules --killed\n> +test_incompatible_with_recurse_submodules --modified\n> +\n> +test_done\n> -- \n> 2.12.2.776.gded3dc243c29.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"316707","messageId":"CA+P7+xqE-SZ5D6pk6-Dx+-VzHijmgmrUR2F=-370Rh8oKr10Nw@mail.gmail.com","threadId":"45672","inReplyTo":"20170412165851.GA79499@google.com","subject":"Re: [PATCH] ls-files: properly prepare submodule environment","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-12T22:45:25Z","receivedAt":"2017-04-12T22:45:53Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Apr 12, 2017 at 9:58 AM, Brandon Williams wrote:\n> On 04/11, Jacob Keller wrote:\n>> From: Jacob Keller \n>>\n>> Since commit e77aa336f116 (\"ls-files: optionally recurse into\n>> submodules\", 2016-10-07) ls-files has known how to recurse into\n>> submodules when displaying files.\n>>\n>> Unfortunately this code does not work as expected. Initially, it\n>> produced results indicating that a --super-prefix can't be used from\n>> a subdirectory:\n>>\n>> fatal: can't use --super-prefix from a subdirectory\n>>\n>> After commit b58a68c1c187 (\"setup: allow for prefix to be passed to git\n>> commands\", 2017-03-17) this behavior changed and resulted in repeated\n>> calls of ls-files with an expanding super-prefix.\n>>\n>> This recursive expanding super-prefix appears to be the result of\n>> ls-files acting on the super-project instead of on the submodule files.\n>>\n>> We can fix this by properly preparing the submodule environment when\n>> setting up the submodule process. This ensures that the command we\n>> execute properly reads the directory and ensures that we correctly\n>> operate in the submodule instead of repeating oureslves on the\n>> super-project contents forever.\n>>\n>> While we're here lets also add some tests to ensure that ls-files works\n>> with recurse-submodules to catch these issues in the future.\n>>\n>> Signed-off-by: Jacob Keller \n>> ---\n>> I found this fix on accident after looking at git-grep code and\n>> comparing how ls-files instantiated the submodule. Can someone who knows\n>> more about submodules explain the reasoning better for this fix?\n>> Essentially we \"recurse\" into the submodule folder, but still operate as\n>> if we're at the root of the project but with a new prefix. So then we\n>> keep recursing into the submodule forever.\n>\n> The reason why this fix is required is that the env var GIT_DIR is set\n> to be the parents gitdir. When recursing the childprocess just uses the\n> parents gitdir instead of its own causing it to recurse into itself\n> again and again. The child process's environment needs to have the\n> GIT_DIR var cleared so that the child will do discovery and actually\n> find its own gitdir.\n\nRight. That makes sense, but that raises the question of how or where\nthis worked in the first place?\n\n>\n>>\n>> I also added some tests here, and I *definitely* think this should be a\n>> maintenance backport into any place which supports ls-files\n>> --recurse-submodules, since as far as I can tell it is otherwise\n>> useless.\n>>\n>> There were no tests for it, so I added some based on git-grep tests. I\n>> did not try them against the broken setups, because of the endless\n>> recursion.\n>\n> There are tests for ls-files --recurse-submodules in\n> t3007-ls-files-recurse-submodules.sh, though it looks like this\n> particular case (which triggers this bug) maybe didn't have any tests.\n> I'm actually unsure of why the existing tests didn't catch this (I'm\n> probably just bad at writing tests).\n\nIt seems to me like this would be a problem for any setup with\nsubmodules, no? Or is it specific case for me? I have a submodule\nwithin a submodule and I'm not setting GIT_DIR manually myself. I want\nto isolate exactly what scenario fails here so that the commit\ndescription can be accurate and we know for sure the test cases cover\nit.\n\nThanks,\nJake\n\n>\n>\n>>\n>> builtin/ls-files.c | 4 +\n>> t/t3080-ls-files-recurse-submodules.sh | 162 +++++++++++++++++++++++++++++++++\n>> 2 files changed, 166 insertions(+)\n>> create mode 100755 t/t3080-ls-files-recurse-submodules.sh\n>>\n>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n>> index d449e46db551..e9b3546ca053 100644\n>> --- a/builtin/ls-files.c\n>> +++ b/builtin/ls-files.c\n>> @@ -15,6 +15,7 @@\n>> #include \"string-list.h\"\n>> #include \"pathspec.h\"\n>> #include \"run-command.h\"\n>> +#include \"submodule.h\"\n>>\n>> static int abbrev;\n>> static int show_deleted;\n>> @@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)\n>> struct child_process cp = CHILD_PROCESS_INIT;\n>> int status;\n>>\n>> + prepare_submodule_repo_env(&cp.env_array);\n>> + argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n>\n> Yes these lines need to be included in order to prepare the environment.\n> Thanks for catching that.\n>\n>> +\n>> if (prefix_len)\n>> argv_array_pushf(&cp.env_array, \"%s=%s\",\n>> GIT_TOPLEVEL_PREFIX_ENVIRONMENT,\n>> diff --git a/t/t3080-ls-files-recurse-submodules.sh b/t/t3080-ls-files-recurse-submodules.sh\n>> new file mode 100755\n>> index 000000000000..6788a8f09635\n>> --- /dev/null\n>> +++ b/t/t3080-ls-files-recurse-submodules.sh\n>> @@ -0,0 +1,162 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='Test ls-files recurse-submodules feature\n>> +\n>> +This test verifies the recurse-submodules feature correctly lists files across\n>> +submodules.\n>> +'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'setup directory structure and submodule' '\n>> + echo \"foobar\" >a &&\n>> + mkdir b &&\n>> + echo \"bar\" >b/b &&\n>> + git add a b &&\n>> + git commit -m \"add a and b\" &&\n>> + git init submodule &&\n>> + echo \"foobar\" >submodule/a &&\n>> + git -C submodule add a &&\n>> + git -C submodule commit -m \"add a\" &&\n>> + git submodule add ./submodule &&\n>> + git commit -m \"added submodule\"\n>> +'\n>> +\n>> +test_expect_success 'ls-files correctly lists files in a submodule' '\n>> + cat >expect <<-\\EOF &&\n>> + .gitmodules\n>> + a\n>> + b/b\n>> + submodule/a\n>> + EOF\n>> +\n>> + git ls-files --recurse-submodules >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'ls-files and basic pathspecs' '\n>> + cat >expect <<-\\EOF &&\n>> + submodule/a\n>> + EOF\n>> +\n>> + git ls-files --recurse-submodules -- submodule >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'ls-files and nested submodules' '\n>> + git init submodule/sub &&\n>> + echo \"foobar\" >submodule/sub/a &&\n>> + git -C submodule/sub add a &&\n>> + git -C submodule/sub commit -m \"add a\" &&\n>> + git -C submodule submodule add ./sub &&\n>> + git -C submodule add sub &&\n>> + git -C submodule commit -m \"added sub\" &&\n>> + git add submodule &&\n>> + git commit -m \"updated submodule\" &&\n>> +\n>> + cat >expect <<-\\EOF &&\n>> + .gitmodules\n>> + a\n>> + b/b\n>> + submodule/.gitmodules\n>> + submodule/a\n>> + submodule/sub/a\n>> + EOF\n>> +\n>> + git ls-files --recurse-submodules >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'ls-files 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>> + .gitmodules\n>> + file\n>> + src/file2\n>> + sub/file\n>> + EOF\n>> + git -C parent ls-files --recurse-submodules >actual &&\n>> + test_cmp expect actual &&\n>> +\n>> + # Relative path to top\n>> + cat >expect <<-\\EOF &&\n>> + ../.gitmodules\n>> + ../file\n>> + file2\n>> + ../sub/file\n>> + EOF\n>> + git -C parent/src ls-files --recurse-submodules .. >actual &&\n>> + test_cmp expect actual &&\n>> +\n>> + # Relative path to submodule\n>> + cat >expect <<-\\EOF &&\n>> + ../sub/file\n>> + EOF\n>> + git -C parent/src ls-files --recurse-submodules ../sub >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'ls-files from a subdir' '\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>> + mkdir parent/src &&\n>> + echo \"foobar\" >parent/src/file &&\n>> + git -C parent add src/file &&\n>> + git -C parent submodule add ../sub src/sub &&\n>> + git -C parent submodule add ../sub sub &&\n>> + git -C parent commit -m \"add files and submodules\" &&\n>> +\n>> + # Verify grep from root works\n>> + cat >expect <<-\\EOF &&\n>> + .gitmodules\n>> + src/file\n>> + src/sub/file\n>> + sub/file\n>> + EOF\n>> + git -C parent ls-files --recurse-submodules >actual &&\n>> + test_cmp expect actual &&\n>> +\n>> + # Verify grep from a subdir works\n>> + cat >expect <<-\\EOF &&\n>> + file\n>> + sub/file\n>> + EOF\n>> + git -C parent/src ls-files --recurse-submodules >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>> + test_must_fail git ls-files --recurse-submodules $1 2>actual &&\n>> + test_i18ngrep -- '--recurse-submodules unsupported mode' actual\n>> + \"\n>> +}\n>> +\n>> +test_incompatible_with_recurse_submodules --deleted\n>> +test_incompatible_with_recurse_submodules --others\n>> +test_incompatible_with_recurse_submodules --unmerged\n>> +test_incompatible_with_recurse_submodules --killed\n>> +test_incompatible_with_recurse_submodules --modified\n>> +\n>> +test_done\n>> --\n>> 2.12.2.776.gded3dc243c29.dirty\n>>\n>\n> --\n> Brandon Williams\n"},{"id":"316733","messageId":"20170413171224.3537-1-jacob.e.keller@intel.com","threadId":"45672","inReplyTo":"CA+P7+xqE-SZ5D6pk6-Dx+-VzHijmgmrUR2F=-370Rh8oKr10Nw@mail.gmail.com","subject":"[PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-04-13T17:12:23Z","receivedAt":"2017-04-13T17:12:35Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nSince commit e77aa336f116 (\"ls-files: optionally recurse into\nsubmodules\", 2016-10-07) ls-files has known how to recurse into\nsubmodules when displaying files.\n\nUnfortunately this fails for certain cases, including when nesting more\nthan one submodule, called from within a submodule that itself has\nsubmodules, or when the GIT_DIR environemnt variable is set.\n\nPrior to commit b58a68c1c187 (\"setup: allow for prefix to be passed to\ngit commands\", 2017-03-17) this resulted in an error indicating that\n--prefix and --super-prefix were incompatible.\n\nAfter this commit, instead, the process loops forever with a GIT_DIR set\nto the parent and continuously reads the parent submodule files and\nrecursing forever.\n\nFix this by preparing the environment properly for submodules when\nsetting up the child process. This is similar to how other commands such\nas grep behave.\n\nThis was not caught by the original tests because the scenario is\navoided if the submodules are created separately and not stored as the\nstandard method of putting the submodule git directory under\n.git/modules/. We can update the test to show the failure by the\naddition of \"git submodule absorbgitdirs\" to the test case. However,\nnote that this new test would run forever without the necessary fix in\nthis patch.\n\nSigned-off-by: Jacob Keller \n---\nI updated and reworded the description so that the problem would be more\nobvious. It doesn't occur always, but only when run nested with properly\nabsorbed gitdirs for the submodules. This explains the reason why the\ntest case had not caught the issue before.\n\n builtin/ls-files.c | 4 ++++\n t/t3007-ls-files-recurse-submodules.sh | 1 +\n 2 files changed, 5 insertions(+)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db551..e9b3546ca053 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -15,6 +15,7 @@\n #include \"string-list.h\"\n #include \"pathspec.h\"\n #include \"run-command.h\"\n+#include \"submodule.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n \n+\tprepare_submodule_repo_env(&cp.env_array);\n+\targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n+\n \tif (prefix_len)\n \t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n \t\t\t\t GIT_TOPLEVEL_PREFIX_ENVIRONMENT,\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex 4cf6ccf5a8ea..c8030dd3299a 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -77,6 +77,7 @@ test_expect_success 'ls-files recurses more than 1 level' '\n \tgit -C submodule/subsub commit -m \"add d\" &&\n \tgit -C submodule submodule add ./subsub &&\n \tgit -C submodule commit -m \"added subsub\" &&\n+\tgit submodule absorbgitdirs &&\n \tgit ls-files --recurse-submodules >actual &&\n \ttest_cmp expect actual\n '\n-- \n2.12.2.776.gded3dc243c29.dirty\n\n"},{"id":"316734","messageId":"20170413171224.3537-2-jacob.e.keller@intel.com","threadId":"45672","inReplyTo":"20170413171224.3537-1-jacob.e.keller@intel.com","subject":"[PATCH v2 2/2] ls-files: fix path used when recursing into submodules","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-04-13T17:12:24Z","receivedAt":"2017-04-13T17:13:33Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nDon't assume that the current working directory is the root of the\nrepository. Correctly generate the path for the recursing child\nprocesses by building it from the work_tree() root instead. Otherwise if\nwe run ls-files using --git-dir or --work-tree it will not work\ncorrectly as it attempts to change directory into a potentially invalid\nlocation. Best case, it doesn't exist and we produce an error. Worst\ncase we cd into the wrong location and unknown behavior occurs.\n\nAdd a new test which highlights this possibility.\n\nSigned-off-by: Jacob Keller \n---\nI'm not sure that I'm convinced by this method of solving the problem as\nI suspect it has some corner cases (what about when run inside a\nsubdirectory? It seems to work for me but I'm not sure...) Additionally,\nit felt weird that there's no helper function for creating a toplevel\nrelative path.\n\n builtin/ls-files.c | 5 ++++-\n t/t3007-ls-files-recurse-submodules.sh | 11 +++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex e9b3546ca053..a6c70dbe9ec8 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -203,6 +203,7 @@ static void show_gitlink(const struct cache_entry *ce)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n+\tchar *dir;\n \n \tprepare_submodule_repo_env(&cp.env_array);\n \targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n@@ -221,8 +222,10 @@ static void show_gitlink(const struct cache_entry *ce)\n \targv_array_pushv(&cp.args, submodule_options.argv);\n \n \tcp.git_cmd = 1;\n-\tcp.dir = ce->name;\n+\tdir = mkpathdup(\"%s/%s\", get_git_work_tree(), ce->name);\n+\tcp.dir = dir;\n \tstatus = run_command(&cp);\n+\tfree(dir);\n \tif (status)\n \t\texit(status);\n }\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex c8030dd3299a..ebb956fd16cc 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -82,6 +82,17 @@ test_expect_success 'ls-files recurses more than 1 level' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ls-files works with GIT_DIR' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\tc\n+\tsubsub/d\n+\tEOF\n+\n+\tgit --git-dir=submodule/.git ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--recurse-submodules and pathspecs setup' '\n \techo e >submodule/subsub/e.txt &&\n \tgit -C submodule/subsub add e.txt &&\n-- \n2.12.2.776.gded3dc243c29.dirty\n\n"},{"id":"316740","messageId":"20170413180357.GA96917@google.com","threadId":"45672","inReplyTo":"20170413171224.3537-1-jacob.e.keller@intel.com","subject":"Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-13T18:03:57Z","receivedAt":"2017-04-13T18:04:23Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/13, Jacob Keller wrote:\n> From: Jacob Keller \n> \n> Since commit e77aa336f116 (\"ls-files: optionally recurse into\n> submodules\", 2016-10-07) ls-files has known how to recurse into\n> submodules when displaying files.\n> \n> Unfortunately this fails for certain cases, including when nesting more\n> than one submodule, called from within a submodule that itself has\n> submodules, or when the GIT_DIR environemnt variable is set.\n> \n> Prior to commit b58a68c1c187 (\"setup: allow for prefix to be passed to\n> git commands\", 2017-03-17) this resulted in an error indicating that\n> --prefix and --super-prefix were incompatible.\n> \n> After this commit, instead, the process loops forever with a GIT_DIR set\n> to the parent and continuously reads the parent submodule files and\n> recursing forever.\n> \n> Fix this by preparing the environment properly for submodules when\n> setting up the child process. This is similar to how other commands such\n> as grep behave.\n> \n> This was not caught by the original tests because the scenario is\n> avoided if the submodules are created separately and not stored as the\n> standard method of putting the submodule git directory under\n> .git/modules/. We can update the test to show the failure by the\n> addition of \"git submodule absorbgitdirs\" to the test case. However,\n> note that this new test would run forever without the necessary fix in\n> this patch.\n> \n> Signed-off-by: Jacob Keller \n\nThis looks good to me. Thanks again for catching this. Dealing with\nsubmodules definitely isn't easy (I seem to have made a lot of mistakes\nthat have been cropping up recently)...it would be easier if we didn't\nhave to spin out a process for each submodule but that's not the world\nwe live in today :)\n\n> ---\n> I updated and reworded the description so that the problem would be more\n> obvious. It doesn't occur always, but only when run nested with properly\n> absorbed gitdirs for the submodules. This explains the reason why the\n> test case had not caught the issue before.\n> \n> builtin/ls-files.c | 4 ++++\n> t/t3007-ls-files-recurse-submodules.sh | 1 +\n> 2 files changed, 5 insertions(+)\n> \n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index d449e46db551..e9b3546ca053 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -15,6 +15,7 @@\n> #include \"string-list.h\"\n> #include \"pathspec.h\"\n> #include \"run-command.h\"\n> +#include \"submodule.h\"\n> \n> static int abbrev;\n> static int show_deleted;\n> @@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tint status;\n> \n> +\tprepare_submodule_repo_env(&cp.env_array);\n> +\targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\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> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\n> index 4cf6ccf5a8ea..c8030dd3299a 100755\n> --- a/t/t3007-ls-files-recurse-submodules.sh\n> +++ b/t/t3007-ls-files-recurse-submodules.sh\n> @@ -77,6 +77,7 @@ test_expect_success 'ls-files recurses more than 1 level' '\n> \tgit -C submodule/subsub commit -m \"add d\" &&\n> \tgit -C submodule submodule add ./subsub &&\n> \tgit -C submodule commit -m \"added subsub\" &&\n> +\tgit submodule absorbgitdirs &&\n> \tgit ls-files --recurse-submodules >actual &&\n> \ttest_cmp expect actual\n> '\n> -- \n> 2.12.2.776.gded3dc243c29.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"316743","messageId":"20170413181031.GB96917@google.com","threadId":"45672","inReplyTo":"20170413171224.3537-2-jacob.e.keller@intel.com","subject":"Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-13T18:10:31Z","receivedAt":"2017-04-13T18:10:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/13, Jacob Keller wrote:\n> From: Jacob Keller \n> \n> Don't assume that the current working directory is the root of the\n> repository. Correctly generate the path for the recursing child\n> processes by building it from the work_tree() root instead. Otherwise if\n> we run ls-files using --git-dir or --work-tree it will not work\n> correctly as it attempts to change directory into a potentially invalid\n> location. Best case, it doesn't exist and we produce an error. Worst\n> case we cd into the wrong location and unknown behavior occurs.\n> \n> Add a new test which highlights this possibility.\n> \n> Signed-off-by: Jacob Keller \n> ---\n> I'm not sure that I'm convinced by this method of solving the problem as\n> I suspect it has some corner cases (what about when run inside a\n> subdirectory? It seems to work for me but I'm not sure...) Additionally,\n> it felt weird that there's no helper function for creating a toplevel\n> relative path.\n\nI never considered the case where you use --git-dir or --work-tree,\ndefinitely an oversight on my part. This change seems reasonable to me.\n\n-- \nBrandon Williams\n"},{"id":"316745","messageId":"CAGZ79kYi4X2cE8uF3cEwh5YndoSL6S2GUK-KYSYibqr+O2qG6g@mail.gmail.com","threadId":"45672","inReplyTo":"20170413171224.3537-2-jacob.e.keller@intel.com","subject":"Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-13T18:15:46Z","receivedAt":"2017-04-13T18:15:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 13, 2017 at 10:12 AM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> Don't assume that the current working directory is the root of the\n> repository.\n\n1) Oh! This bug might be hidden in other commands, too.\n($ git grep cp.dir -- submodule.c)\n\n2) But why?\nIsn't that what most of setup.c is all about ? (discovery of the root of the\nrepository, staying there, and invoking the correct subcommand with a prefix)\n\n> Correctly generate the path for the recursing child\n> processes by building it from the work_tree() root instead. Otherwise if\n> we run ls-files using --git-dir or --work-tree it will not work\n> correctly as it attempts to change directory into a potentially invalid\n> location.\n\nOh, I see. In that case the setup doesn't cd into the worktree.\n\n> Best case, it doesn't exist and we produce an error. Worst\n> case we cd into the wrong location and unknown behavior occurs.\n>\n> Add a new test which highlights this possibility.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> I'm not sure that I'm convinced by this method of solving the problem as\n> I suspect it has some corner cases (what about when run inside a\n> subdirectory? It seems to work for me but I'm not sure...) Additionally,\n> it felt weird that there's no helper function for creating a toplevel\n> relative path.\n\nDo we want to run ls-files from the working tree or from the git dir?\nFor the git dir there would be git_pathdup_submodule.\n\nWe could introduce\n const char *get_submodule_work_tree(const char *submodule_path);\nas a wrapper around\n mkpathdup(\"%s/%s\", get_git_work_tree(), ce->name);\n\nCode and test look fine in this patch,\n\nThanks,\nStefan\n"},{"id":"316747","messageId":"CA+P7+xr6zkb4kLTM-+0f5OTtt583WR_0j=NEebOdYuO3Z6h1+w@mail.gmail.com","threadId":"45672","inReplyTo":"20170413180357.GA96917@google.com","subject":"Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-13T18:31:03Z","receivedAt":"2017-04-13T18:31:36Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 13, 2017 at 11:03 AM, Brandon Williams wrote:\n> On 04/13, Jacob Keller wrote:\n>> From: Jacob Keller \n>>\n>> Since commit e77aa336f116 (\"ls-files: optionally recurse into\n>> submodules\", 2016-10-07) ls-files has known how to recurse into\n>> submodules when displaying files.\n>>\n>> Unfortunately this fails for certain cases, including when nesting more\n>> than one submodule, called from within a submodule that itself has\n>> submodules, or when the GIT_DIR environemnt variable is set.\n>>\n>> Prior to commit b58a68c1c187 (\"setup: allow for prefix to be passed to\n>> git commands\", 2017-03-17) this resulted in an error indicating that\n>> --prefix and --super-prefix were incompatible.\n>>\n>> After this commit, instead, the process loops forever with a GIT_DIR set\n>> to the parent and continuously reads the parent submodule files and\n>> recursing forever.\n>>\n>> Fix this by preparing the environment properly for submodules when\n>> setting up the child process. This is similar to how other commands such\n>> as grep behave.\n>>\n>> This was not caught by the original tests because the scenario is\n>> avoided if the submodules are created separately and not stored as the\n>> standard method of putting the submodule git directory under\n>> .git/modules/. We can update the test to show the failure by the\n>> addition of \"git submodule absorbgitdirs\" to the test case. However,\n>> note that this new test would run forever without the necessary fix in\n>> this patch.\n>>\n>> Signed-off-by: Jacob Keller \n>\n> This looks good to me. Thanks again for catching this. Dealing with\n> submodules definitely isn't easy (I seem to have made a lot of mistakes\n> that have been cropping up recently)...it would be easier if we didn't\n> have to spin out a process for each submodule but that's not the world\n> we live in today :)\n>\n\nSpinning out a process is one of the big downsides of working with\nsubmodules in our code. Unfortunately, spinning out a process is also\none of the biggest ways we isolate submodules, and if we wanted to do\nthis \"in-process\" we would need to add an abstraction layer that lets\nus handle submodules in-process in some clean way.\n"},{"id":"316752","messageId":"CA+P7+xpFVcj=Ssu6mjPs8km-GEsyhzbeVdb0ENH4bdzdqXOTsA@mail.gmail.com","threadId":"45672","inReplyTo":"CAGZ79kYi4X2cE8uF3cEwh5YndoSL6S2GUK-KYSYibqr+O2qG6g@mail.gmail.com","subject":"Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-13T18:34:17Z","receivedAt":"2017-04-13T18:34:49Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 13, 2017 at 11:15 AM, Stefan Beller wrote:\n> On Thu, Apr 13, 2017 at 10:12 AM, Jacob Keller wrote:\n>> From: Jacob Keller \n>>\n>> Don't assume that the current working directory is the root of the\n>> repository.\n>\n> 1) Oh! This bug might be hidden in other commands, too.\n> ($ git grep cp.dir -- submodule.c)\n>\n\nAlmost certainly. I'm not sure how best to audit this.\n\n> 2) But why?\n> Isn't that what most of setup.c is all about ? (discovery of the root of the\n> repository, staying there, and invoking the correct subcommand with a prefix)\n>\n>> Correctly generate the path for the recursing child\n>> processes by building it from the work_tree() root instead. Otherwise if\n>> we run ls-files using --git-dir or --work-tree it will not work\n>> correctly as it attempts to change directory into a potentially invalid\n>> location.\n>\n> Oh, I see. In that case the setup doesn't cd into the worktree.\n>\n\nYea we aren't in the worktree when we thought we were.\n\n>> Best case, it doesn't exist and we produce an error. Worst\n>> case we cd into the wrong location and unknown behavior occurs.\n>>\n>> Add a new test which highlights this possibility.\n>>\n>> Signed-off-by: Jacob Keller \n>> ---\n>> I'm not sure that I'm convinced by this method of solving the problem as\n>> I suspect it has some corner cases (what about when run inside a\n>> subdirectory? It seems to work for me but I'm not sure...) Additionally,\n>> it felt weird that there's no helper function for creating a toplevel\n>> relative path.\n>\n> Do we want to run ls-files from the working tree or from the git dir?\n> For the git dir there would be git_pathdup_submodule.\n>\n\nWell prior to this code we're assuming we are in the worktree. I\nwasn't actually certain if we should run from within the gitdir or the\nworktree. Probably we actually want to be in the gitdir so that we can\nwork even if we're not checked out. Additionally, we probably need to\nprotect this check with a \"is_submodule_initialized\" unless ls-files\nsomehow ignores the submodule already in this case.. it didn't look\nlike it at a glance.\n\n> We could introduce\n> const char *get_submodule_work_tree(const char *submodule_path);\n> as a wrapper around\n> mkpathdup(\"%s/%s\", get_git_work_tree(), ce->name);\n>\n> Code and test look fine in this patch,\n>\n\nYea adding a helper function seems like a good thing.\n\nThanks,\nJake\n\n> Thanks,\n> Stefan\n"},{"id":"316754","messageId":"CAGZ79kaRpAPFd8+rQUxKTmDfrQWdPGEEWGSVzstwUbj-7hRAKQ@mail.gmail.com","threadId":"45672","inReplyTo":"CA+P7+xr6zkb4kLTM-+0f5OTtt583WR_0j=NEebOdYuO3Z6h1+w@mail.gmail.com","subject":"Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-13T18:35:09Z","receivedAt":"2017-04-13T18:35:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 13, 2017 at 11:31 AM, Jacob Keller wrote:\n> Spinning out a process is one of the big downsides of working with\n> submodules in our code. Unfortunately, spinning out a process is also\n> one of the biggest ways we isolate submodules, and if we wanted to do\n> this \"in-process\" we would need to add an abstraction layer that lets\n> us handle submodules in-process in some clean way.\n\nYeah if we had less globals and a repo struct (class) which we could\noperate on, that would be bring in the abstractions that we'd need.\n\nStefan\n"},{"id":"316759","messageId":"20170413183615.GC96917@google.com","threadId":"45672","inReplyTo":"CAGZ79kaRpAPFd8+rQUxKTmDfrQWdPGEEWGSVzstwUbj-7hRAKQ@mail.gmail.com","subject":"Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-13T18:36:15Z","receivedAt":"2017-04-13T18:36:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/13, Stefan Beller wrote:\n> On Thu, Apr 13, 2017 at 11:31 AM, Jacob Keller wrote:\n> > Spinning out a process is one of the big downsides of working with\n> > submodules in our code. Unfortunately, spinning out a process is also\n> > one of the biggest ways we isolate submodules, and if we wanted to do\n> > this \"in-process\" we would need to add an abstraction layer that lets\n> > us handle submodules in-process in some clean way.\n> \n> Yeah if we had less globals and a repo struct (class) which we could\n> operate on, that would be bring in the abstractions that we'd need.\n\nAgreed, though we're probably pretty far from that becoming a reality.\nOne day though!\n\n-- \nBrandon Williams\n"},{"id":"316760","messageId":"20170413185707.97746-1-bmwill@google.com","threadId":"45672","inReplyTo":"20170413171224.3537-1-jacob.e.keller@intel.com","subject":"[PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-13T18:57:07Z","receivedAt":"2017-04-13T18:57:21Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Add in a check to see if a submodule is active before attempting to\nrecurse. This prevents 'ls-files' from trying to operate on a submodule\nwhich may not exist in the working directory.\n\nSigned-off-by: Brandon Williams \n---\n\nAfter you mentioned possibly needing to check if a submodule is initialized, I\nwent back and noticed that there was indeed no check for it...So this patch\nadds in the necessary check to see if a submodule is active or not before\nattempting to recurse.\n\n builtin/ls-files.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex d449e46db..10ddc0306 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -15,6 +15,7 @@\n #include \"string-list.h\"\n #include \"pathspec.h\"\n #include \"run-command.h\"\n+#include \"submodule.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -235,7 +236,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n \t\tdie(\"git ls-files: internal error - cache entry not superset of prefix\");\n \n \tif (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n-\t submodule_path_match(&pathspec, name.buf, ps_matched)) {\n+\t submodule_path_match(&pathspec, name.buf, ps_matched) &&\n+\t is_submodule_initialized(ce->name)) {\n \t\tshow_gitlink(ce);\n \t} else if (match_pathspec(&pathspec, name.buf, name.len,\n \t\t\t\t len, ps_matched,\n@@ -604,8 +606,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n-\tif (recurse_submodules)\n+\tif (recurse_submodules) {\n+\t\tgitmodules_config();\n \t\tcompile_submodule_options(argv, &dir, show_tag);\n+\t}\n \n \tif (recurse_submodules &&\n \t (show_stage || show_deleted || show_others || show_unmerged ||\n-- \n2.12.2.762.g0e3151a226-goog\n\n"},{"id":"316763","messageId":"CAGZ79kZ8KV+c2StKR8tp=s_E1+uEaSezgsmUfyyO9HUrmzdT+g@mail.gmail.com","threadId":"45672","inReplyTo":"20170413185707.97746-1-bmwill@google.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-13T19:05:32Z","receivedAt":"2017-04-13T19:05:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams wrote:\n> Add in a check to see if a submodule is active before attempting to\n> recurse. This prevents 'ls-files' from trying to operate on a submodule\n> which may not exist in the working directory.\n\nWhat would currently happen on recursing into non-active submodules?\nCan we have a test for this?\n\nThanks,\nStefan\n"},{"id":"316767","messageId":"CA+P7+xp=1PUsq1_or=J8ED+-1NMaF=BckC9bK4jqWbA+RFeszw@mail.gmail.com","threadId":"45672","inReplyTo":"CAGZ79kZ8KV+c2StKR8tp=s_E1+uEaSezgsmUfyyO9HUrmzdT+g@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-13T19:12:01Z","receivedAt":"2017-04-13T19:12:27Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller wrote:\n> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams wrote:\n>> Add in a check to see if a submodule is active before attempting to\n>> recurse. This prevents 'ls-files' from trying to operate on a submodule\n>> which may not exist in the working directory.\n>\n> What would currently happen on recursing into non-active submodules?\n> Can we have a test for this?\n>\n> Thanks,\n> Stefan\n\nWe should be able to test for this. Is it possible that we can recurse\ninto a submodule as long as we have the clone in .git/modules/\neven if we don't have it checked out currently?\n\nThanks,\nJake\n"},{"id":"316774","messageId":"CAGZ79kaRTrgHMnP6mA3V2rDHzf8iOidqLTpp2aXmi6x+6YeZxw@mail.gmail.com","threadId":"45672","inReplyTo":"CA+P7+xp=1PUsq1_or=J8ED+-1NMaF=BckC9bK4jqWbA+RFeszw@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-13T19:25:28Z","receivedAt":"2017-04-13T19:25:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 13, 2017 at 12:12 PM, Jacob Keller wrote:\n> On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller wrote:\n>> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams wrote:\n>>> Add in a check to see if a submodule is active before attempting to\n>>> recurse. This prevents 'ls-files' from trying to operate on a submodule\n>>> which may not exist in the working directory.\n>>\n>> What would currently happen on recursing into non-active submodules?\n>> Can we have a test for this?\n>>\n>> Thanks,\n>> Stefan\n>\n> We should be able to test for this. Is it possible that we can recurse\n> into a submodule as long as we have the clone in .git/modules/\n> even if we don't have it checked out currently?\n\nConceptually that should be possible, e.g.\n\n git ls-files --recurse-submodules \n\nwhere the ancient ref contained submodules that are not present any more.\nIn that case we would need to do\n\n struct strbuf sb = STRBUF_INIT;\n struct child_process = CHILD_PROCESS_INIT;\n struct submodule *sub = submodule_from_path( \\\n , )\n strbuf_git_path(&sb, \"modules/%s\", sub->name);\n\n argv_array_pushl(&cp.args, \"git\", \"ls-files\", \"--recurse\", ...);\n cp.dir = sb.buf;\n run_command(&cp);\n\nStefan\n"},{"id":"316776","messageId":"CA+P7+xq7VUAT3pLahj72YUnF3zDm2u-ZBFu6TxWRYcJCc0c=Ew@mail.gmail.com","threadId":"45672","inReplyTo":"CAGZ79kaRTrgHMnP6mA3V2rDHzf8iOidqLTpp2aXmi6x+6YeZxw@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-13T19:30:19Z","receivedAt":"2017-04-13T19:30:47Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 13, 2017 at 12:25 PM, Stefan Beller wrote:\n> On Thu, Apr 13, 2017 at 12:12 PM, Jacob Keller wrote:\n>> On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller wrote:\n>>> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams wrote:\n>>>> Add in a check to see if a submodule is active before attempting to\n>>>> recurse. This prevents 'ls-files' from trying to operate on a submodule\n>>>> which may not exist in the working directory.\n>>>\n>>> What would currently happen on recursing into non-active submodules?\n>>> Can we have a test for this?\n>>>\n>>> Thanks,\n>>> Stefan\n>>\n>> We should be able to test for this. Is it possible that we can recurse\n>> into a submodule as long as we have the clone in .git/modules/\n>> even if we don't have it checked out currently?\n>\n> Conceptually that should be possible, e.g.\n>\n> git ls-files --recurse-submodules \n>\n> where the ancient ref contained submodules that are not present any more.\n> In that case we would need to do\n>\n> struct strbuf sb = STRBUF_INIT;\n> struct child_process = CHILD_PROCESS_INIT;\n> struct submodule *sub = submodule_from_path( \\\n> , )\n> strbuf_git_path(&sb, \"modules/%s\", sub->name);\n>\n> argv_array_pushl(&cp.args, \"git\", \"ls-files\", \"--recurse\", ...);\n> cp.dir = sb.buf;\n> run_command(&cp);\n>\n> Stefan\n\nThis is probably the right flow to use then.\n\nThanks,\nJake\n"},{"id":"316818","messageId":"CA+P7+xoY-FFXEkPgQBA4=Twq+G5YZrHkBqiO+sKHcGJe7xt7vw@mail.gmail.com","threadId":"45672","inReplyTo":"CAGZ79kaRTrgHMnP6mA3V2rDHzf8iOidqLTpp2aXmi6x+6YeZxw@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-14T16:33:01Z","receivedAt":"2017-04-14T16:33:27Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 13, 2017 at 12:25 PM, Stefan Beller wrote:\n> On Thu, Apr 13, 2017 at 12:12 PM, Jacob Keller wrote:\n>> On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller wrote:\n>>> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams wrote:\n>>>> Add in a check to see if a submodule is active before attempting to\n>>>> recurse. This prevents 'ls-files' from trying to operate on a submodule\n>>>> which may not exist in the working directory.\n>>>\n>>> What would currently happen on recursing into non-active submodules?\n>>> Can we have a test for this?\n>>>\n>>> Thanks,\n>>> Stefan\n>>\n>> We should be able to test for this. Is it possible that we can recurse\n>> into a submodule as long as we have the clone in .git/modules/\n>> even if we don't have it checked out currently?\n>\n> Conceptually that should be possible, e.g.\n>\n> git ls-files --recurse-submodules \n>\n> where the ancient ref contained submodules that are not present any more.\n> In that case we would need to do\n>\n> struct strbuf sb = STRBUF_INIT;\n> struct child_process = CHILD_PROCESS_INIT;\n> struct submodule *sub = submodule_from_path( \\\n> , )\n> strbuf_git_path(&sb, \"modules/%s\", sub->name);\n>\n> argv_array_pushl(&cp.args, \"git\", \"ls-files\", \"--recurse\", ...);\n> cp.dir = sb.buf;\n> run_command(&cp);\n>\n> Stefan\n\nNever mind. git ls-files doesn't support showing files for a specific\nancient history. (I guess you'd use ls-tree for that?). I'm guessing\nwe want to run in the actual work-tree for ls-files here.\n\nDoes \"is_submodule_initialized()\" going to ensure that we only operate\non a submodule that's currently checked out?\n\nThanks,\nJake\n"},{"id":"316831","messageId":"CAGZ79kY_Faantt9s3aBrWk6F9vDaKidjYpWt0be00w1vXu9PWg@mail.gmail.com","threadId":"45672","inReplyTo":"CA+P7+xoY-FFXEkPgQBA4=Twq+G5YZrHkBqiO+sKHcGJe7xt7vw@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-14T17:02:21Z","receivedAt":"2017-04-14T17:02:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 14, 2017 at 9:33 AM, Jacob Keller wrote:\n>\n> Never mind. git ls-files doesn't support showing files for a specific\n> ancient history. (I guess you'd use ls-tree for that?). I'm guessing\n> we want to run in the actual work-tree for ls-files here.\n>\n> Does \"is_submodule_initialized()\" going to ensure that we only operate\n> on a submodule that's currently checked out?\n\nI think for that we rather want to use is_submodule_populated.\nAnd I think it would make sense as well to check for that instead\nof the initialized state.\n\nThanks,\nStefan\n"},{"id":"316868","messageId":"CA+P7+xoFNve37_QOwQoicSm0B1ko76ntHrSMHZnCi5=4Bn2twg@mail.gmail.com","threadId":"45672","inReplyTo":"CAGZ79kY_Faantt9s3aBrWk6F9vDaKidjYpWt0be00w1vXu9PWg@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-14T23:49:44Z","receivedAt":"2017-04-14T23:50:11Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Apr 14, 2017 at 10:02 AM, Stefan Beller wrote:\n> On Fri, Apr 14, 2017 at 9:33 AM, Jacob Keller wrote:\n>>\n>> Never mind. git ls-files doesn't support showing files for a specific\n>> ancient history. (I guess you'd use ls-tree for that?). I'm guessing\n>> we want to run in the actual work-tree for ls-files here.\n>>\n>> Does \"is_submodule_initialized()\" going to ensure that we only operate\n>> on a submodule that's currently checked out?\n>\n> I think for that we rather want to use is_submodule_populated.\n> And I think it would make sense as well to check for that instead\n> of the initialized state.\n>\n> Thanks,\n> Stefan\n\nRight ok.\n\nThanks,\nJake\n"},{"id":"317062","messageId":"xmqqk26ih3qf.fsf@gitster.mtv.corp.google.com","threadId":"45672","inReplyTo":"20170413171224.3537-2-jacob.e.keller@intel.com","subject":"Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T02:03:04Z","receivedAt":"2017-04-18T02:03:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \n>\n> Don't assume that the current working directory is the root of the\n> repository. Correctly generate the path for the recursing child\n> processes by building it from the work_tree() root instead. Otherwise if\n> we run ls-files using --git-dir or --work-tree it will not work\n> correctly as it attempts to change directory into a potentially invalid\n> location. Best case, it doesn't exist and we produce an error. Worst\n> case we cd into the wrong location and unknown behavior occurs.\n>\n> Add a new test which highlights this possibility.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> I'm not sure that I'm convinced by this method of solving the problem as\n> I suspect it has some corner cases (what about when run inside a\n> subdirectory? It seems to work for me but I'm not sure...) Additionally,\n> it felt weird that there's no helper function for creating a toplevel\n> relative path.\n\nIs this a similar issue as discussed in a nearby thread e.g.\n\n \n\nI do think it is a bug that sometimes we do not go to the root of\nthe working tree when we know the repository is not a bare one.\n"},{"id":"317084","messageId":"CA+P7+xoSszSrHCijG4gDE6r_Eo-AeAk2zsecwpAW_KEdBiLWuA@mail.gmail.com","threadId":"45672","inReplyTo":"xmqqk26ih3qf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-04-18T07:42:00Z","receivedAt":"2017-04-18T07:45:40Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Apr 17, 2017 at 7:03 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> From: Jacob Keller \n>>\n>> Don't assume that the current working directory is the root of the\n>> repository. Correctly generate the path for the recursing child\n>> processes by building it from the work_tree() root instead. Otherwise if\n>> we run ls-files using --git-dir or --work-tree it will not work\n>> correctly as it attempts to change directory into a potentially invalid\n>> location. Best case, it doesn't exist and we produce an error. Worst\n>> case we cd into the wrong location and unknown behavior occurs.\n>>\n>> Add a new test which highlights this possibility.\n>>\n>> Signed-off-by: Jacob Keller \n>> ---\n>> I'm not sure that I'm convinced by this method of solving the problem as\n>> I suspect it has some corner cases (what about when run inside a\n>> subdirectory? It seems to work for me but I'm not sure...) Additionally,\n>> it felt weird that there's no helper function for creating a toplevel\n>> relative path.\n>\n> Is this a similar issue as discussed in a nearby thread e.g.\n>\n> \n>\n> I do think it is a bug that sometimes we do not go to the root of\n> the working tree when we know the repository is not a bare one.\n\nYes and no. I think that this would be a problem in both a bare and\nnon-bare repo ? I think the correct thing to do here is really to do\nwhat we proposed, and properly lookup the full file name.\n\nI do think it makes the most sense conceptually to always cd into the\ntop level directory of a non-bare repo though.\n\nThanks,\nJake\n"},{"id":"317127","messageId":"CAGZ79kY9aEPp6BgWxp85ssNhdr=0AjMAh5tHJkMo6c+JhLjg-Q@mail.gmail.com","threadId":"45672","inReplyTo":"xmqqk26ih3qf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-18T18:41:01Z","receivedAt":"2017-04-18T18:41:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 17, 2017 at 7:03 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> From: Jacob Keller \n>>\n>> Don't assume that the current working directory is the root of the\n>> repository. Correctly generate the path for the recursing child\n>> processes by building it from the work_tree() root instead. Otherwise if\n>> we run ls-files using --git-dir or --work-tree it will not work\n>> correctly as it attempts to change directory into a potentially invalid\n>> location. Best case, it doesn't exist and we produce an error. Worst\n>> case we cd into the wrong location and unknown behavior occurs.\n>>\n>> Add a new test which highlights this possibility.\n>>\n>> Signed-off-by: Jacob Keller \n>> ---\n>> I'm not sure that I'm convinced by this method of solving the problem as\n>> I suspect it has some corner cases (what about when run inside a\n>> subdirectory? It seems to work for me but I'm not sure...) Additionally,\n>> it felt weird that there's no helper function for creating a toplevel\n>> relative path.\n>\n> Is this a similar issue as discussed in a nearby thread e.g.\n>\n> \n\nyes, it's the same broader issue.\n\n>\n> I do think it is a bug that sometimes we do not go to the root of\n> the working tree when we know the repository is not a bare one.\n\nok, that would have been my understanding as well.\n\nThat nearby thread however highlights how it may be a user breaking if\nwe just fix the paths. The patch here doesn't cd to the root of the repo,\nbut constructs the correct path, which is correct. It may be considered\nclumsy if we decide that cd'ing to the root of the repo is the correct bug fix\nfor the underlying issue.\n\nThanks,\nStefan\n"},{"id":"317163","messageId":"xmqqefwpdx9x.fsf@gitster.mtv.corp.google.com","threadId":"45672","inReplyTo":"20170413185707.97746-1-bmwill@google.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-19T01:03:06Z","receivedAt":"2017-04-19T01:03:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Add in a check to see if a submodule is active before attempting to\n> recurse. This prevents 'ls-files' from trying to operate on a submodule\n> which may not exist in the working directory.\n>\n> Signed-off-by: Brandon Williams \n> ---\n\nSounds like this is something we can demonstrate its effect with a\nsimple test in t/ directory?\n"},{"id":"319498","messageId":"CACoUkn5fyk054BJ5e+C8KynYMJg=EkMLOTiZya8KTNZh+=u5dQ@mail.gmail.com","threadId":"45672","inReplyTo":"20170512172657.GA98586@google.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Paul Jolly","fromEmail":"paul@myitcv.io","sentAt":"2017-05-12T18:12:35Z","receivedAt":"2017-05-12T18:44:25Z","isPatch":true,"sender":{"key":"paul@myitcv.io","avatar":"https://gravatar.com/avatar/6a83179b2ef0386af693cc03a9c50ea897f5246c5dcc0fa5d126171ba17c3a63?d=mp&s=160"},"body":">> How can I help diagnose what's going on here?\n\n\n\n> Welp that's a pretty terrible bug which stems from\n> missing a check to see if a submodule is initialized, and not explicitly\n> setting GIT_DIR=.git (theres cases where we don't want this but turns\n> out we probably should do that here). Let me know if this fixes the\n> problem:\n\nThat's certainly fixed the fork explosion. Indeed seems to now work as\nexpected from my perspective...\n\nAnd also answers the question that I was going to ask, namely whether\ngit ls-files --recurse-submodules should work on repos that do not contain\nany submodules... to which I'd hoped the answer was \"yes\" (because as I\nsaid it was a fatal error in v2.11.x, despite there being some output). The\nbehaviour I'm now seeing appears to affirm that, assuming it's as expected\nfrom your perspective!\n\nThanks for the quick response.\n"},{"id":"319526","messageId":"20170512172657.GA98586@google.com","threadId":"45672","inReplyTo":"20170512162109.49752-1-paul@myitcv.io","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-12T17:26:57Z","receivedAt":"2017-05-12T18:45:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/12, paul@myitcv.io wrote:\n> > ...\n> \n> I'm nowhere near up to speed with the entire history of this chain so\n> please excuse me if I've missed some detail somewhere.\n> \n> As of b06d3643105c8758ed019125a4399cb7efdcce2c, the following command\n> \"hangs\" at near 100% CPU in a repo of mine:\n> \n> git ls-files --recurse-submodules\n> \n> The \"hang\" appears to be an explosion of subprocesses (snipped the\n> COMMAND detail from ps):\n> \n> git ls-files --recurse-submodules\n> git --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --\n> git --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --\n> ....\n\nYeah the way the recursion works as of 2.13 is that a new process is\nspawned for each submodule. This really is kind of clunky and I'd like\nto move to an implementation which doesn't require spawning a ton of\nprocesses, so that's a goal of mine I'm trying to work towards. This is\nstill a relatively new feature so I expect there's still a lot of bugs to\niron out.\n\n> \n> In v2.11.0 I simply get a fatal error (this repo doesn't actually\n> contain any submodules):\n> \n> fatal: can't use --super-prefix from a subdirectory\n> \n> I haven't yet been able to work out what's special about the\n> subdirectory g/_vendor/src/github.com/ramya-rao-a/go-outline/\n> \n> Is this expected? (I'm guessing not)\n> \n> How can I help diagnose what's going on here?\n\nLooking at this it appears to me that you have a submodule loop, either\ndirectly or indirectly. That is if you have a repo 'a' which contains a\nsubmodule 'b' and repo 'b' happens to contain 'a' as a submodule you'd\nfind yourself with an infinite loop. But that's what I'm guessing\nbased on that output you've shown me.\n\nBut you're saying that this repo doesn't actually contain submodules?\nHmm then I'm at a loss...unless!\n\nThe directory 'go-outline' is a place holder for a submodule so ls-files\nspawns a process in that directory. But since its not populated, during\nthe gitdir discovery it falls back to the superproject's gitdir,\nrestarting the whole process and creating an infinite loop...\n\nWelp that's a pretty terrible bug which stems from\nmissing a check to see if a submodule is initialized, and not explicitly\nsetting GIT_DIR=.git (theres cases where we don't want this but turns\nout we probably should do that here). Let me know if this fixes the\nproblem:\n\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex a6c70dbe9..b0796fc10 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -206,7 +206,6 @@ static void show_gitlink(const struct cache_entry *ce)\n \tchar *dir;\n \n \tprepare_submodule_repo_env(&cp.env_array);\n-\targv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);\n \n \tif (prefix_len)\n \t\targv_array_pushf(&cp.env_array, \"%s=%s\",\n@@ -242,6 +241,7 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n \t\tdie(\"git ls-files: internal error - cache entry not superset of prefix\");\n \n \tif (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n+\t is_submodule_initialized(ce->name) &&\n \t submodule_path_match(&pathspec, name.buf, ps_matched)) {\n \t\tshow_gitlink(ce);\n \t} else if (match_pathspec(&pathspec, name.buf, name.len,\n@@ -611,8 +611,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n-\tif (recurse_submodules)\n+\tif (recurse_submodules) {\n+\t\tgitmodules_config();\n \t\tcompile_submodule_options(argv, &dir, show_tag);\n+\t}\n \n \tif (recurse_submodules &&\n \t (show_stage || show_deleted || show_others || show_unmerged ||\n\n-- \nBrandon Williams\n"},{"id":"319529","messageId":"20170512162109.49752-1-paul@myitcv.io","threadId":"45672","inReplyTo":"xmqqefwpdx9x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"","fromEmail":"paul@myitcv.io","sentAt":"2017-05-12T16:21:09Z","receivedAt":"2017-05-12T18:45:21Z","isPatch":true,"sender":{"key":"paul@myitcv.io","avatar":"https://gravatar.com/avatar/6a83179b2ef0386af693cc03a9c50ea897f5246c5dcc0fa5d126171ba17c3a63?d=mp&s=160"},"body":"> ...\n\nI'm nowhere near up to speed with the entire history of this chain so\nplease excuse me if I've missed some detail somewhere.\n\nAs of b06d3643105c8758ed019125a4399cb7efdcce2c, the following command\n\"hangs\" at near 100% CPU in a repo of mine:\n\ngit ls-files --recurse-submodules\n\nThe \"hang\" appears to be an explosion of subprocesses (snipped the\nCOMMAND detail from ps):\n\ngit ls-files --recurse-submodules\ngit --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --\ngit --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --\n....\n\nIn v2.11.0 I simply get a fatal error (this repo doesn't actually\ncontain any submodules):\n\nfatal: can't use --super-prefix from a subdirectory\n\nI haven't yet been able to work out what's special about the\nsubdirectory g/_vendor/src/github.com/ramya-rao-a/go-outline/\n\nIs this expected? (I'm guessing not)\n\nHow can I help diagnose what's going on here?\n\nThanks\n\n"},{"id":"319538","messageId":"20170512181950.GE98586@google.com","threadId":"45672","inReplyTo":"CACoUkn5fyk054BJ5e+C8KynYMJg=EkMLOTiZya8KTNZh+=u5dQ@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-12T18:19:50Z","receivedAt":"2017-05-12T18:45:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/12, Paul Jolly wrote:\n> >> How can I help diagnose what's going on here?\n> \n> \n> \n> > Welp that's a pretty terrible bug which stems from\n> > missing a check to see if a submodule is initialized, and not explicitly\n> > setting GIT_DIR=.git (theres cases where we don't want this but turns\n> > out we probably should do that here). Let me know if this fixes the\n> > problem:\n> \n> That's certainly fixed the fork explosion. Indeed seems to now work as\n> expected from my perspective...\n> \n> And also answers the question that I was going to ask, namely whether\n> git ls-files --recurse-submodules should work on repos that do not contain\n> any submodules... to which I'd hoped the answer was \"yes\" (because as I\n> said it was a fatal error in v2.11.x, despite there being some output). The\n> behaviour I'm now seeing appears to affirm that, assuming it's as expected\n> from your perspective!\n\nYep it should definitely work on repos with no submodules. I'll put\nthis on my list of things to do so that I can come up with a more robust\nsolution to the problem (with tests).\n\n> \n> Thanks for the quick response.\n\nCourse, let me know if you find anything else! :D\n\n-- \nBrandon Williams\n"},{"id":"325386","messageId":"CACoUkn7wv2QUjf__frfCd7mczBj-j3YsPmaiaHssL0LG0f+fkg@mail.gmail.com","threadId":"45672","inReplyTo":"20170512181950.GE98586@google.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Paul Jolly","fromEmail":"paul@myitcv.io","sentAt":"2017-08-01T18:16:01Z","receivedAt":"2017-08-01T18:16:11Z","isPatch":true,"sender":{"key":"paul@myitcv.io","avatar":"https://gravatar.com/avatar/6a83179b2ef0386af693cc03a9c50ea897f5246c5dcc0fa5d126171ba17c3a63?d=mp&s=160"},"body":">> Thanks for the quick response.\n>\n> Course, let me know if you find anything else! :D\n\nBrandon - doesn't look like this change has made it's way into master:\n\nhttps://github.com/git/git/blob/master/builtin/ls-files.c\n\nIs there a plan to merge it?\n\nThanks\n"},{"id":"325387","messageId":"20170801181820.GA110646@google.com","threadId":"45672","inReplyTo":"CACoUkn7i76dEsQa3eoN+7WR8QmsD1pWsRQ0dvhkxzFN0sxTmRQ@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-01T18:18:20Z","receivedAt":"2017-08-01T18:18:27Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/01, Paul Jolly wrote:\n> >\n> > > Thanks for the quick response.\n> >\n> > Course, let me know if you find anything else! :D\n> >\n> \n> Brandon - doesn't look like this change has made it's way into master:\n> \n> https://github.com/git/git/blob/master/builtin/ls-files.c\n> \n> Is there a plan to merge it?\n> \n> Thanks\n\nIt looks like it was merged to master. This should be the relevant\ncommit: 188dce131 (ls-files: use repository object, 2017-06-22).\n\n-- \nBrandon Williams\n"},{"id":"325388","messageId":"CACoUkn46an6rStWhhJfgP-vbRU_+898S-TgDcefN3zL-jyDFJQ@mail.gmail.com","threadId":"45672","inReplyTo":"20170801181820.GA110646@google.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Paul Jolly","fromEmail":"paul@myitcv.io","sentAt":"2017-08-01T18:19:29Z","receivedAt":"2017-08-01T18:19:36Z","isPatch":true,"sender":{"key":"paul@myitcv.io","avatar":"https://gravatar.com/avatar/6a83179b2ef0386af693cc03a9c50ea897f5246c5dcc0fa5d126171ba17c3a63?d=mp&s=160"},"body":"> It looks like it was merged to master. This should be the relevant\n> commit: 188dce131 (ls-files: use repository object, 2017-06-22).\n\nI was just typing a response to my response. Apologies, I was testing\nlocally with the wrong compiled version of git.\n\nConfirmed fixed for me in e2d9c4613 at least.\n\nThanks again\n"},{"id":"325389","messageId":"20170801182033.GB110646@google.com","threadId":"45672","inReplyTo":"CACoUkn46an6rStWhhJfgP-vbRU_+898S-TgDcefN3zL-jyDFJQ@mail.gmail.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-01T18:20:33Z","receivedAt":"2017-08-01T18:20:40Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/01, Paul Jolly wrote:\n> > It looks like it was merged to master. This should be the relevant\n> > commit: 188dce131 (ls-files: use repository object, 2017-06-22).\n> \n> I was just typing a response to my response. Apologies, I was testing\n> locally with the wrong compiled version of git.\n> \n> Confirmed fixed for me in e2d9c4613 at least.\n\nPerfect! Glad to hear that it fixed the problem.\n\n-- \nBrandon Williams\n"},{"id":"325395","messageId":"xmqqshhbf7uy.fsf@gitster.mtv.corp.google.com","threadId":"45672","inReplyTo":"20170801182033.GB110646@google.com","subject":"Re: [PATCH 3/2] ls-files: only recurse on active submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-01T18:41:57Z","receivedAt":"2017-08-01T18:42:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> On 08/01, Paul Jolly wrote:\n>> > It looks like it was merged to master. This should be the relevant\n>> > commit: 188dce131 (ls-files: use repository object, 2017-06-22).\n>> \n>> I was just typing a response to my response. Apologies, I was testing\n>> locally with the wrong compiled version of git.\n>> \n>> Confirmed fixed for me in e2d9c4613 at least.\n>\n> Perfect! Glad to hear that it fixed the problem.\n\nThanks, both, for tying potentially loose ends.\n"}]}