{"thread":{"id":"51298","subject":"[PATCH 0/1] submodule foreach: fix recursion of options","startedAt":"2019-06-12T18:10:12Z","lastAt":"2019-06-25T18:18:36Z","messageCount":19,"participants":["Morian Sonnet via GitGitGadget","Eric Sunshine","Morian Sonnet","Johannes Schindelin","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"377096","messageId":"pull.263.git.gitgitgadget@gmail.com","threadId":"51298","inReplyTo":null,"subject":"[PATCH 0/1] submodule foreach: fix recursion of options","fromName":"Morian Sonnet via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-12T18:10:08Z","receivedAt":"2019-06-12T18:10:12Z","isPatch":true,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"During the usage of git in Buildkite we noted that git fails upon calling \ngit submodule foreach --recursive git reset --hardafter updating git version\nto 2.22.0.\n\nThis is due to a problem with the recursive calling of git submodule--helper \nitself, which is fixed in the patch below.\n\nMorian Sonnet (1):\n submodule foreach: fix recursion of options\n\n builtin/submodule--helper.c | 1 +\n t/t7407-submodule-foreach.sh | 5 +++++\n 2 files changed, 6 insertions(+)\n\n\nbase-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-263%2Fmomoson%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-263/momoson/master-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/263\n-- \ngitgitgadget\n"},{"id":"377097","messageId":"e766e5177722dc20837adbd902d596186b1e4997.1560363008.git.gitgitgadget@gmail.com","threadId":"51298","inReplyTo":"pull.263.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] submodule foreach: fix recursion of options","fromName":"Morian Sonnet via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-12T18:10:09Z","receivedAt":"2019-06-12T18:10:14Z","isPatch":true,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"From: Morian Sonnet \n\nCalling\n\n git submodule foreach --recursive git reset --hard\n\nleads to an error stating that the option --hard is unknown to\nsubmodule--helper.\n\nReasons:\n\n. Above call is internally translated into\n\n git submodule--helper foreach --recursive -- git reset --hard\n\n. After calling\n\n git reset --hard\n\n inside the first first level submodule,\n\n git --super-prefix submodule--helper \\\n foreach --recursive git reset --hard\n\n is called. Note the missing --.\n\n. Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the\n option --hard is not passed to\n\n git reset\n\n anymore, but leads to git submodule--helper complaining about an\n unknown option.\n\nFix:\n\n. Add -- before the command to execute, such that now correctly\n\n git --super-prefix submodule--helper \\\n foreach --recursive -- git reset --hard\n\n is called.\n\nSigned-off-by: Morian Sonnet \n---\n builtin/submodule--helper.c | 1 +\n t/t7407-submodule-foreach.sh | 5 +++++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 0bf4aa088e..afaf0819c9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n \t\tif (info->quiet)\n \t\t\targv_array_push(&cpr.args, \"--quiet\");\n \n+\t\targv_array_push(&cpr.args, \"--\");\n \t\targv_array_pushv(&cpr.args, info->argv);\n \n \t\tif (run_command(&cpr))\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 706ae762e0..6110742a7c 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -421,4 +421,9 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'option-like arguments passed to foreach recurse correctly' '\n+ cd super &&\n+ git submodule foreach --recursive git reset --hard\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"377107","messageId":"CAPig+cR5hV3+8RjB=8Vz2cF6Ycnh_kj+LwraZAehAqYxrEt0xg@mail.gmail.com","threadId":"51298","inReplyTo":"e766e5177722dc20837adbd902d596186b1e4997.1560363008.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] submodule foreach: fix recursion of options","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-06-12T19:35:50Z","receivedAt":"2019-06-12T19:36:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 12, 2019 at 2:10 PM Morian Sonnet via GitGitGadget\n wrote:\n> [...]\n> . Add -- before the command to execute, such that now correctly\n>\n> git --super-prefix submodule--helper \\\n> foreach --recursive -- git reset --hard\n>\n> is called.\n>\n> Signed-off-by: Morian Sonnet \n> ---\n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> @@ -421,4 +421,9 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo\n> +test_expect_success 'option-like arguments passed to foreach recurse correctly' '\n> + cd super &&\n> + git submodule foreach --recursive git reset --hard\n> +'\n\nFollow the example of the existing tests in this script and do the\n'cd' in a subshell so that it doesn't bleed into tests added later\nfollowing this one, thus confusing them.\n\n (\n cd super &&\n git submodule foreach ...\n )\n"},{"id":"377113","messageId":"pull.263.v2.git.gitgitgadget@gmail.com","threadId":"51298","inReplyTo":"pull.263.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] submodule foreach: fix recursion of options","fromName":"Morian Sonnet via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-12T20:28:13Z","receivedAt":"2019-06-12T20:28:17Z","isPatch":true,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"During the usage of git in Buildkite we noted that git fails upon calling \ngit submodule foreach --recursive git reset --hardafter updating git version\nto 2.22.0.\n\nThis is due to a problem with the recursive calling of git submodule--helper \nitself, which is fixed in the patch below.\n\nMorian Sonnet (1):\n submodule foreach: fix recursion of options\n\n builtin/submodule--helper.c | 1 +\n t/t7407-submodule-foreach.sh | 7 +++++++\n 2 files changed, 8 insertions(+)\n\n\nbase-commit: b697d92f56511e804b8ba20ccbe7bdc85dc66810\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-263%2Fmomoson%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-263/momoson/master-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/263\n\nRange-diff vs v1:\n\n 1: e766e51777 ! 1: c46e5bd140 submodule foreach: fix recursion of options\n @@ -65,8 +65,10 @@\n '\n \n +test_expect_success 'option-like arguments passed to foreach recurse correctly' '\n -+ cd super &&\n -+ git submodule foreach --recursive git reset --hard\n ++ (\n ++ cd super &&\n ++ git submodule foreach --recursive git reset --hard\n ++ )\n +'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"377114","messageId":"c46e5bd1403b45d25192220ce689fcd55d897981.1560371293.git.gitgitgadget@gmail.com","threadId":"51298","inReplyTo":"pull.263.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] submodule foreach: fix recursion of options","fromName":"Morian Sonnet via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-12T20:28:14Z","receivedAt":"2019-06-12T20:28:18Z","isPatch":true,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"From: Morian Sonnet \n\nCalling\n\n git submodule foreach --recursive git reset --hard\n\nleads to an error stating that the option --hard is unknown to\nsubmodule--helper.\n\nReasons:\n\n. Above call is internally translated into\n\n git submodule--helper foreach --recursive -- git reset --hard\n\n. After calling\n\n git reset --hard\n\n inside the first first level submodule,\n\n git --super-prefix submodule--helper \\\n foreach --recursive git reset --hard\n\n is called. Note the missing --.\n\n. Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the\n option --hard is not passed to\n\n git reset\n\n anymore, but leads to git submodule--helper complaining about an\n unknown option.\n\nFix:\n\n. Add -- before the command to execute, such that now correctly\n\n git --super-prefix submodule--helper \\\n foreach --recursive -- git reset --hard\n\n is called.\n\nSigned-off-by: Morian Sonnet \n---\n builtin/submodule--helper.c | 1 +\n t/t7407-submodule-foreach.sh | 7 +++++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 0bf4aa088e..afaf0819c9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n \t\tif (info->quiet)\n \t\t\targv_array_push(&cpr.args, \"--quiet\");\n \n+\t\targv_array_push(&cpr.args, \"--\");\n \t\targv_array_pushv(&cpr.args, info->argv);\n \n \t\tif (run_command(&cpr))\ndiff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\nindex 706ae762e0..c554589e6f 100755\n--- a/t/t7407-submodule-foreach.sh\n+++ b/t/t7407-submodule-foreach.sh\n@@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'option-like arguments passed to foreach recurse correctly' '\n+ (\n+ cd super &&\n+ git submodule foreach --recursive git reset --hard\n+ )\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"377411","messageId":"201906181515.x5IFF8bg005587@pool-147-74-zam859.wlan.kfa-juelich.de","threadId":"51298","inReplyTo":"c46e5bd1403b45d25192220ce689fcd55d897981.1560371293.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] submodule foreach: fix recursion of options","fromName":"Morian Sonnet","fromEmail":"moriansonnet@gmail.com","sentAt":"2019-06-18T15:15:08Z","receivedAt":"2019-06-18T15:15:14Z","isPatch":true,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"\"Morian Sonnet via GitGitGadget\" wrote:\n\nI fixed the problem with the test case. Please take another look.\n\n> From: Morian Sonnet \n>\n> Calling\n>\n> git submodule foreach --recursive git reset --hard\n>\n> leads to an error stating that the option --hard is unknown to\n> submodule--helper.\n>\n> Reasons:\n>\n> . Above call is internally translated into\n>\n> git submodule--helper foreach --recursive -- git reset --hard\n>\n> . After calling\n>\n> git reset --hard\n>\n> inside the first first level submodule,\n>\n> git --super-prefix submodule--helper \\\n> foreach --recursive git reset --hard\n>\n> is called. Note the missing --.\n>\n> . Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the\n> option --hard is not passed to\n>\n> git reset\n>\n> anymore, but leads to git submodule--helper complaining about an\n> unknown option.\n>\n> Fix:\n>\n> . Add -- before the command to execute, such that now correctly\n>\n> git --super-prefix submodule--helper \\\n> foreach --recursive -- git reset --hard\n>\n> is called.\n>\n> Signed-off-by: Morian Sonnet \n> ---\n> builtin/submodule--helper.c | 1 +\n> t/t7407-submodule-foreach.sh | 7 +++++++\n> 2 files changed, 8 insertions(+)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 0bf4aa088e..afaf0819c9 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n> \t\tif (info->quiet)\n> \t\t\targv_array_push(&cpr.args, \"--quiet\");\n> \n> +\t\targv_array_push(&cpr.args, \"--\");\n> \t\targv_array_pushv(&cpr.args, info->argv);\n> \n> \t\tif (run_command(&cpr))\n> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> index 706ae762e0..c554589e6f 100755\n> --- a/t/t7407-submodule-foreach.sh\n> +++ b/t/t7407-submodule-foreach.sh\n> @@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'option-like arguments passed to foreach recurse correctly' '\n> + (\n> + cd super &&\n> + git submodule foreach --recursive git reset --hard\n> + )\n> +'\n> +\n> test_done\n> -- \n> gitgitgadget\n"},{"id":"377546","messageId":"nycvar.QRO.7.76.6.1906191928420.44@tvgsbejvaqbjf.bet","threadId":"51298","inReplyTo":"201906181515.x5IFF8bg005587@pool-147-74-zam859.wlan.kfa-juelich.de","subject":"Re: [PATCH v2 1/1] submodule foreach: fix recursion of options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-19T18:19:34Z","receivedAt":"2019-06-19T18:19:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Morian,\n\nOn Tue, 18 Jun 2019, Morian Sonnet wrote:\n\n> \"Morian Sonnet via GitGitGadget\" wrote:\n>\n> > Calling\n> >\n> > git submodule foreach --recursive git reset --hard\n> >\n> > leads to an error stating that the option --hard is unknown to\n> > submodule--helper.\n> >\n> > Reasons:\n> >\n> > . Above call is internally translated into\n> >\n> > git submodule--helper foreach --recursive -- git reset --hard\n> >\n> > . After calling\n> >\n> > git reset --hard\n> >\n> > inside the first first level submodule,\n> >\n> > git --super-prefix submodule--helper \\\n> > foreach --recursive git reset --hard\n> >\n> > is called. Note the missing --.\n> >\n> > . Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the\n> > option --hard is not passed to\n> >\n> > git reset\n> >\n> > anymore, but leads to git submodule--helper complaining about an\n> > unknown option.\n> >\n> > Fix:\n> >\n> > . Add -- before the command to execute, such that now correctly\n> >\n> > git --super-prefix submodule--helper \\\n> > foreach --recursive -- git reset --hard\n> >\n> > is called.\n\nThis is a good explanation, even if the format is not exactly close to the\nexisting commit messages ;-) If you look e.g. at\nhttps://github.com/git/git/commit/e5a329a279c7, you will see what I mean:\nthere is much more \"prose\" (and less bullet points) going on.\n\n> > Signed-off-by: Morian Sonnet \n> > ---\n> > builtin/submodule--helper.c | 1 +\n> > t/t7407-submodule-foreach.sh | 7 +++++++\n> > 2 files changed, 8 insertions(+)\n> >\n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index 0bf4aa088e..afaf0819c9 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n> > \t\tif (info->quiet)\n> > \t\t\targv_array_push(&cpr.args, \"--quiet\");\n> >\n> > +\t\targv_array_push(&cpr.args, \"--\");\n\nThis is obviously correct.\n\n> > \t\targv_array_pushv(&cpr.args, info->argv);\n> >\n> > \t\tif (run_command(&cpr))\n> > diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> > index 706ae762e0..c554589e6f 100755\n> > --- a/t/t7407-submodule-foreach.sh\n> > +++ b/t/t7407-submodule-foreach.sh\n> > @@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo\n> > \ttest_cmp expected actual\n> > '\n> >\n> > +test_expect_success 'option-like arguments passed to foreach recurse correctly' '\n> > + (\n> > + cd super &&\n> > + git submodule foreach --recursive git reset --hard\n> > + )\n\nI am terribly sorry that I did not catch this in the first round. I would\nfind it even easier to read if it used the `-C` option, like so:\n\n\tgit -C super submodule foreach --recursive git reset --hard\n\nThen you do not need the subshell, and neither the `cd`. It would become a\none-liner.\n\nHowever, what is less obvious to me is that this would catch the\nregression, as I do not see from the context whether the current submodule\nstructure is deep enough to trigger the reported problem. If I understand\nthe commit message correctly, `super` would have to contain a submodule\nthat itself contains a submodule.\n\nIf there was only one level of submodules (and from the context of this\ndiff, it is not clear whether that is the case), the test case would pass\neven without the code change to `submodule.c`.\n\nOf course, I can always dig deeper and find out myself (and of course I\ndid exactly that). But in my mind, that points to something we can\nimprove.\n\nWhat I found is that the test case indeed fails without the fixed\n`builtin/submodule.c`, but that the nested submodule does not even need to\nbe checked out, contrary to what I expected after reading the commit\nmessage.\n\nAnd in fact, the nested submodule is not even checked out in the test\nscript!\n\nTo make this test case more obvious, and at the same time to test a little\nmore thoroughly, maybe it would make sense to initialize that \"inner\"\nsubmodule (in this test script, `super/nested1/nested2`), then make it\ndirty by changing a file before the `reset --hard`, and afterwards verify\nthat the file in question was successfully reset.\n\nThat way, the test case would start to fail if anybody changed the script\nin a way where the submodule nesting was all of a sudden no longer deep\nenough to verify what the test case wants to verify.\n\nOtherwise we would risk that this test case would start passing for the\nwrong reasons at some stage.\n\nWhat I have in mind would look somewhat like this:\n\n\t: make sure that nested2 is initialized &&\n\tgit -C super/nested1 submodule update --init nested2 &&\n\n\t: make sure that nested2 is dirty &&\n\techo dirty >super/nested1/nested2/file &&\n\ttest_must_fail git -C super/nested1/nested2 update-index --refresh &&\n\n\tgit -C super submodule foreach --recursive git reset --hard &&\n\t: now nested2 is clean &&\n\tgit -C super/nested1/nested2 update-index --refresh\n\n\nI might be overthinking this, though. Maybe it would be enough to make\nsure that nested1/nested2 is a nested submodule, e.g. by something like\nthis:\n\n\tis_submodule () {\n\t\tcase \"$(git -C \"${1%/*}\" ls-files --stage \"${1##*/}\")\" in\n\t\t160000*) return 0;;\n\t\t*) return 1;;\n\t\tesac\n\t} &&\n\tis_submodule super/nested1 &&\n\tis_submodule super/nested1/nested2 &&\n\tgit -C super submodule foreach --recursive git reset --hard\n\nwhich has the advantage of looking shorter, but it does not really verify\nthat `git reset --hard` *has* been working correctly in nested2: it did\nnot, as that nested submodule was not even initialized at that point.\n\nSo I dunno... What do you think? How can we make this test case both more\nunderstandable and more robust against future edits at the same time?\n\nCiao,\nJohannes\n\n> > +'\n> > +\n> > test_done\n> > --\n> > gitgitgadget\n>\n"},{"id":"377811","messageId":"201906221054.x5MAspEQ003711@linux-5.fritz.box","threadId":"51298","inReplyTo":"nycvar.QRO.7.76.6.1906191928420.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] submodule foreach: fix recursion of options","fromName":"Morian Sonnet","fromEmail":"moriansonnet@gmail.com","sentAt":"2019-06-22T10:54:51Z","receivedAt":"2019-06-22T10:54:59Z","isPatch":true,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"Johannes Schindelin wrote:\n\nHello Johannes,\n\nthank your for the review. Sorry for the spam, I messed up with the\nreplytoall command.\n\n> Hi Morian,\n>\n> On Tue, 18 Jun 2019, Morian Sonnet wrote:\n>\n> > \"Morian Sonnet via GitGitGadget\" wrote:\n> >\n> > > Calling\n> > >\n> > > git submodule foreach --recursive git reset --hard\n> > >\n> > > leads to an error stating that the option --hard is unknown to\n> > > submodule--helper.\n> > >\n> > > Reasons:\n> > >\n> > > . Above call is internally translated into\n> > >\n> > > git submodule--helper foreach --recursive -- git reset --hard\n> > >\n> > > . After calling\n> > >\n> > > git reset --hard\n> > >\n> > > inside the first first level submodule,\n> > >\n> > > git --super-prefix submodule--helper \\\n> > > foreach --recursive git reset --hard\n> > >\n> > > is called. Note the missing --.\n> > >\n> > > . Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the\n> > > option --hard is not passed to\n> > >\n> > > git reset\n> > >\n> > > anymore, but leads to git submodule--helper complaining about an\n> > > unknown option.\n> > >\n> > > Fix:\n> > >\n> > > . Add -- before the command to execute, such that now correctly\n> > >\n> > > git --super-prefix submodule--helper \\\n> > > foreach --recursive -- git reset --hard\n> > >\n> > > is called.\n>\n> This is a good explanation, even if the format is not exactly close to the\n> existing commit messages ;-) If you look e.g. at\n> https://github.com/git/git/commit/e5a329a279c7, you will see what I mean:\n> there is much more \"prose\" (and less bullet points) going on.\n\nOk, I see the differences. Shall I adapt the commit description?\n\n>\n> > > Signed-off-by: Morian Sonnet \n> > > ---\n> > > builtin/submodule--helper.c | 1 +\n> > > t/t7407-submodule-foreach.sh | 7 +++++++\n> > > 2 files changed, 8 insertions(+)\n> > >\n> > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > > index 0bf4aa088e..afaf0819c9 100644\n> > > --- a/builtin/submodule--helper.c\n> > > +++ b/builtin/submodule--helper.c\n> > > @@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,\n> > > \t\tif (info->quiet)\n> > > \t\t\targv_array_push(&cpr.args, \"--quiet\");\n> > >\n> > > +\t\targv_array_push(&cpr.args, \"--\");\n>\n> This is obviously correct.\n>\n> > > \t\targv_array_pushv(&cpr.args, info->argv);\n> > >\n> > > \t\tif (run_command(&cpr))\n> > > diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh\n> > > index 706ae762e0..c554589e6f 100755\n> > > --- a/t/t7407-submodule-foreach.sh\n> > > +++ b/t/t7407-submodule-foreach.sh\n> > > @@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo\n> > > \ttest_cmp expected actual\n> > > '\n> > >\n> > > +test_expect_success 'option-like arguments passed to foreach recurse correctly' '\n> > > + (\n> > > + cd super &&\n> > > + git submodule foreach --recursive git reset --hard\n> > > + )\n>\n> I am terribly sorry that I did not catch this in the first round. I would\n> find it even easier to read if it used the `-C` option, like so:\n>\n> \tgit -C super submodule foreach --recursive git reset --hard\n>\n> Then you do not need the subshell, and neither the `cd`. It would become a\n> one-liner.\n\nAlright, I have to admit I wasn't aware of that option.\n\n>\n> However, what is less obvious to me is that this would catch the\n> regression, as I do not see from the context whether the current submodule\n> structure is deep enough to trigger the reported problem. If I understand\n> the commit message correctly, `super` would have to contain a submodule\n> that itself contains a submodule.\n>\n> If there was only one level of submodules (and from the context of this\n> diff, it is not clear whether that is the case), the test case would pass\n> even without the code change to `submodule.c`.\n\nThis is not entirely correct, I think. In the case where we found the\nerror, no nested submodules were present. The buildkite agent however,\ndue to the mere presence of submodules, still calls git submodule with\nthe recursive flag.\nInside submodule-helper the next level of the submodule-helper is called\nagain, without checking for the presence of nested submodules.\nHence, one level of submodules should be enough for now.\n\nHowever, in sight of making the test more robust, one should actually\nuse at least a two level deep git for checking, so that in future no\npotential false positives are created. That is, if the submodule-helper\nsuddenly starts checking for the presence of nested submodules, before\ncalling its next iteration.\n\n>\n> Of course, I can always dig deeper and find out myself (and of course I\n> did exactly that). But in my mind, that points to something we can\n> improve.\n>\n> What I found is that the test case indeed fails without the fixed\n> `builtin/submodule.c`, but that the nested submodule does not even need to\n> be checked out, contrary to what I expected after reading the commit\n> message.\n>\n> And in fact, the nested submodule is not even checked out in the test\n> script!\n\nThe reasons for these points are explained by the text above, as the\nsubmodule-helper does not check for the presence of nested submodules\nbefore calling its next iteration.\n\n>\n> To make this test case more obvious, and at the same time to test a little\n> more thoroughly, maybe it would make sense to initialize that \"inner\"\n> submodule (in this test script, `super/nested1/nested2`), then make it\n> dirty by changing a file before the `reset --hard`, and afterwards verify\n> that the file in question was successfully reset.\n>\n> That way, the test case would start to fail if anybody changed the script\n> in a way where the submodule nesting was all of a sudden no longer deep\n> enough to verify what the test case wants to verify.\n>\n> Otherwise we would risk that this test case would start passing for the\n> wrong reasons at some stage.\n>\n> What I have in mind would look somewhat like this:\n>\n> \t: make sure that nested2 is initialized &&\n> \tgit -C super/nested1 submodule update --init nested2 &&\n>\n> \t: make sure that nested2 is dirty &&\n> \techo dirty >super/nested1/nested2/file &&\n> \ttest_must_fail git -C super/nested1/nested2 update-index --refresh &&\n>\n> \tgit -C super submodule foreach --recursive git reset --hard &&\n> \t: now nested2 is clean &&\n> \tgit -C super/nested1/nested2 update-index --refresh\n>\n>\n> I might be overthinking this, though. Maybe it would be enough to make\n> sure that nested1/nested2 is a nested submodule, e.g. by something like\n> this:\n>\n> \tis_submodule () {\n> \t\tcase \"$(git -C \"${1%/*}\" ls-files --stage \"${1##*/}\")\" in\n> \t\t160000*) return 0;;\n> \t\t*) return 1;;\n> \t\tesac\n> \t} &&\n> \tis_submodule super/nested1 &&\n> \tis_submodule super/nested1/nested2 &&\n> \tgit -C super submodule foreach --recursive git reset --hard\n>\n> which has the advantage of looking shorter, but it does not really verify\n> that `git reset --hard` *has* been working correctly in nested2: it did\n> not, as that nested submodule was not even initialized at that point.\n>\n> So I dunno... What do you think? How can we make this test case both more\n> understandable and more robust against future edits at the same time?\n\nMy test so far was designed to not actually check whether git reset\n--hard has shown any effect. I think other tests in the test suite exist\nalready, testing for the correct execution of the command in each\nsubmodule. See for example \"test messages from \"foreach --recursive\" from\nsubdirectory'\" and following.\n\nThis is why I think the second test of yours would be totally sufficient\nto test the introduced changes. That is of course only in combination\nwith the already existing tests, whose testing functionality will\nremain. (I think that is a safe assumption.)\n\nHowever, I think that your test might be a little bit too much,\ncomparing it to the other tests of the suite.\n\nTaking the test \"test \"foreach --quiet --recursive\"\" and the test\n\"option-like arguments passed to foreach commands are not lost\" for\nexample, I suggest the following:\n\n\tgit -C clone2 submodule foreach --recursive \"echo be --hard\" > expected &&\n\tgit -C clone2 submodule foreach --recursive echo be --hard > actual &&\n\tgrep -sq -e \"--hard\" expected &&\n\ttest_cmp expected actual\n\nBy using clone2 we can be sure that we have a fully initalized git setup\nwith at least two levels of submodules, as it is also used by several\nprevious tests. By grepping the expected we can also be sure that the\necho command was correctly executed as reference. Finally, comparing the\nactual output with the expected output then shows that the option is\nalso correctly passed without using the \"\".\n\nAs final comment: One could change the flag --hard to something which\nwill definetely never be an option flag for both, echo and git submodule\nforeach, e.g. --this-will-never-be-an-option.\n\nPlease tell me what you think about the proposed test.\n\nBest regards,\n Morian\n\n>\n> Ciao,\n> Johannes\n>\n> > > +'\n> > > +\n> > > test_done\n> > > --\n> > > gitgitgadget\n> >\n"},{"id":"377859","messageId":"nycvar.QRO.7.76.6.1906241241080.44@tvgsbejvaqbjf.bet","threadId":"51298","inReplyTo":"201906221054.x5MAspEQ003711@linux-5.fritz.box","subject":"Re: [PATCH v2 1/1] submodule foreach: fix recursion of options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-24T10:49:14Z","receivedAt":"2019-06-24T10:49:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Morian,\n\nOn Sat, 22 Jun 2019, Morian Sonnet wrote:\n\n> Johannes Schindelin wrote:\n>\n> > On Tue, 18 Jun 2019, Morian Sonnet wrote:\n> >\n> > > \"Morian Sonnet via GitGitGadget\" wrote:\n> > >\n> > > > Calling\n> > > >\n> > > > git submodule foreach --recursive git reset --hard\n> > > >\n> > > > leads to an error stating that the option --hard is unknown to\n> > > > submodule--helper.\n> > > >\n> > > > Reasons:\n> > > >\n> > > > . Above call is internally translated into\n> > > >\n> > > > git submodule--helper foreach --recursive -- git reset --hard\n> > > >\n> > > > . After calling\n> > > >\n> > > > git reset --hard\n> > > >\n> > > > inside the first first level submodule,\n> > > >\n> > > > git --super-prefix submodule--helper \\\n> > > > foreach --recursive git reset --hard\n> > > >\n> > > > is called. Note the missing --.\n> > > >\n> > > > . Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the\n> > > > option --hard is not passed to\n> > > >\n> > > > git reset\n> > > >\n> > > > anymore, but leads to git submodule--helper complaining about an\n> > > > unknown option.\n> > > >\n> > > > Fix:\n> > > >\n> > > > . Add -- before the command to execute, such that now correctly\n> > > >\n> > > > git --super-prefix submodule--helper \\\n> > > > foreach --recursive -- git reset --hard\n> > > >\n> > > > is called.\n> >\n> > This is a good explanation, even if the format is not exactly close to the\n> > existing commit messages ;-) If you look e.g. at\n> > https://github.com/git/git/commit/e5a329a279c7, you will see what I mean:\n> > there is much more \"prose\" (and less bullet points) going on.\n>\n> Ok, I see the differences. Shall I adapt the commit description?\n\nWell, I would if I were you ;-)\n\n> My test so far was designed to not actually check whether git reset\n> --hard has shown any effect.\n\nRight, but the test case's title suggested that...\n\n> Taking the test \"test \"foreach --quiet --recursive\"\" and the test\n> \"option-like arguments passed to foreach commands are not lost\" for\n> example, I suggest the following:\n>\n> \tgit -C clone2 submodule foreach --recursive \"echo be --hard\" > expected &&\n> \tgit -C clone2 submodule foreach --recursive echo be --hard > actual &&\n> \tgrep -sq -e \"--hard\" expected &&\n\nPlease without the `-sq`.\n\n> \ttest_cmp expected actual\n\nSounds good to me, especially with this suggestion:\n\n> As final comment: One could change the flag --hard to something which\n> will definetely never be an option flag for both, echo and git submodule\n> foreach, e.g. --this-will-never-be-an-option.\n\nPersonally, I'd go for `echo --an-option`, and yes, you are absolutely\nright, the intention of your bug fix is not so much to fix the recursive\n`git reset --hard` as it is to fix _any_ recursive command with\ncommand-line options.\n\nTo be honest, I had missed that `git submodule foreach --recursive`\nexecutes commands even in uninitialized submodules... I could even see how\nsome users might consider that behavior a bug... are you sure this is the\ncase? Is this `echo` really executed even in `nested2`?\n\nCiao,\nJohannes\n"},{"id":"377893","messageId":"20190624154050.9943-1-MorianSonnet@googlemail.com","threadId":"51298","inReplyTo":"nycvar.QRO.7.76.6.1906241241080.44@tvgsbejvaqbjf.bet","subject":"submodule foreach: fix recursion of options","fromName":"Morian Sonnet","fromEmail":"moriansonnet@gmail.com","sentAt":"2019-06-24T15:40:50Z","receivedAt":"2019-06-24T15:41:28Z","isPatch":false,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"here comes the next version.\n\n> Johannes Schindelin wrote:\n> \n> Hi Morian,\n> \n> On Sat, 22 Jun 2019, Morian Sonnet wrote:\n> \n> > Johannes Schindelin wrote:\n> >\n> > > On Tue, 18 Jun 2019, Morian Sonnet wrote:\n> > >\n> > > > \"Morian Sonnet via GitGitGadget\" wrote:\n> > > >\n> > > > > Calling\n> > > > >\n> > > > > git submodule foreach --recursive git reset --hard\n> > > > >\n> > > > > leads to an error stating that the option --hard is unknown to\n> > > > > submodule--helper.\n> > > > >\n> > > > > Reasons:\n> > > > >\n> > > > > . Above call is internally translated into\n> > > > >\n> > > > > git submodule--helper foreach --recursive -- git reset --hard\n> > > > >\n> > > > > . After calling\n> > > > >\n> > > > > git reset --hard\n> > > > >\n> > > > > inside the first first level submodule,\n> > > > >\n> > > > > git --super-prefix submodule--helper \\\n> > > > > foreach --recursive git reset --hard\n> > > > >\n> > > > > is called. Note the missing --.\n> > > > >\n> > > > > . Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the\n> > > > > option --hard is not passed to\n> > > > >\n> > > > > git reset\n> > > > >\n> > > > > anymore, but leads to git submodule--helper complaining about an\n> > > > > unknown option.\n> > > > >\n> > > > > Fix:\n> > > > >\n> > > > > . Add -- before the command to execute, such that now correctly\n> > > > >\n> > > > > git --super-prefix submodule--helper \\\n> > > > > foreach --recursive -- git reset --hard\n> > > > >\n> > > > > is called.\n> > >\n> > > This is a good explanation, even if the format is not exactly close to the\n> > > existing commit messages ;-) If you look e.g. at\n> > > https://github.com/git/git/commit/e5a329a279c7, you will see what I mean:\n> > > there is much more \"prose\" (and less bullet points) going on.\n> >\n> > Ok, I see the differences. Shall I adapt the commit description?\n> \n> Well, I would if I were you ;-)\n\nOk, done.\n\n> \n> > My test so far was designed to not actually check whether git reset\n> > --hard has shown any effect.\n> \n> Right, but the test case's title suggested that...\n\nThe commit message is very specific regarding git reset --hard, right.\nChanged that.\n\n> \n> > Taking the test \"test \"foreach --quiet --recursive\"\" and the test\n> > \"option-like arguments passed to foreach commands are not lost\" for\n> > example, I suggest the following:\n> >\n> > git -C clone2 submodule foreach --recursive \"echo be --hard\" > expected &&\n> > git -C clone2 submodule foreach --recursive echo be --hard > actual &&\n> > grep -sq -e \"--hard\" expected &&\n> \n> Please without the `-sq`.\n\nOh, ok, i took it from another test in that suite.\n\n> \n> > test_cmp expected actual\n> \n> Sounds good to me, especially with this suggestion:\n> \n> > As final comment: One could change the flag --hard to something which\n> > will definetely never be an option flag for both, echo and git submodule\n> > foreach, e.g. --this-will-never-be-an-option.\n> \n> Personally, I'd go for `echo --an-option`, and yes, you are absolutely\n> right, the intention of your bug fix is not so much to fix the recursive\n> `git reset --hard` as it is to fix _any_ recursive command with\n> command-line options.\n> \n> To be honest, I had missed that `git submodule foreach --recursive`\n> executes commands even in uninitialized submodules... I could even see how\n> some users might consider that behavior a bug... are you sure this is the\n> case? Is this `echo` really executed even in `nested2`?\n\nI think this is a misunderstanding. I did not mean that the subcommand\nis executed in `nested2` (which it actually is not) but rather that\n`git submodule--helper foreach ...`\nis again executed in `nested1` and this even if `nested1` does not\ninclude any submodules.\n\nBest regards,\n Morian\n\n\n"},{"id":"377894","messageId":"20190624154050.9943-2-MorianSonnet@googlemail.com","threadId":"51298","inReplyTo":"nycvar.QRO.7.76.6.1906241241080.44@tvgsbejvaqbjf.bet","subject":"[PATCH] submodule foreach: fix recursion of options","fromName":"Morian Sonnet","fromEmail":"moriansonnet@gmail.com","sentAt":"2019-06-24T15:40:51Z","receivedAt":"2019-06-24T15:41:38Z","isPatch":true,"sender":{"key":"moriansonnet@gmail.com","avatar":"https://gravatar.com/avatar/2ee7902b6c223a0393846779d779f08839eb2247ebad980b6165b46a1cec3b29?d=mp&s=160"},"body":"Calling\n\n git submodule foreach --recursive --