submodule foreach: fix recursion of options

[...]
. Add -- before the command to execute, such that now correctly

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive -- git reset --hard

is called.

Signed-off-by Morian Sonnet <[email protected]>

---

From: Morian Sonnet <[email protected]>

Calling

git submodule foreach --recursive git reset --hard

leads to an error stating that the option --hard is unknown to submodule--helper.

Reasons:

. Above call is internally translated into

    git submodule--helper foreach --recursive -- git reset --hard

. After calling

    git reset --hard

inside the first first level submodule,

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive git reset --hard

is called. Note the missing --.

. Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the
option --hard is not passed to

git reset

anymore, but leads to git submodule--helper complaining about an unknown option.

Fix:

. Add -- before the command to execute, such that now correctly

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive -- git reset --hard

is called.

Signed-off-by Morian Sonnet <[email protected]>

---
 builtin/submodule--helper.c  | 1 +
 t/t7407-submodule-foreach.sh | 7 +++++++
 2 files changed, 8 insertions(+)

builtin/submodule--helper.c +1 -0

index 0bf4aa088e..afaf0819c9 100644
@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 		if (info->quiet)
 			argv_array_push(&cpr.args, "--quiet");
 
+		argv_array_push(&cpr.args, "--");
 		argv_array_pushv(&cpr.args, info->argv);
 
 		if (run_command(&cpr))

t/t7407-submodule-foreach.sh +7 -0

index 706ae762e0..c554589e6f 100755
@@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo
 	test_cmp expected actual
 '
 
+test_expect_success 'option-like arguments passed to foreach recurse correctly' '
+  (
+    cd super &&
+    git submodule foreach --recursive git reset --hard
+  )
+'
+
 test_done

Signature · 1 line

gitgitgadget

Quoted 32 lines · Morian Sonnet via GitGitGadget

Calling

git submodule foreach --recursive git reset --hard

leads to an error stating that the option --hard is unknown to submodule--helper.

Reasons:

. Above call is internally translated into

    git submodule--helper foreach --recursive -- git reset --hard

. After calling

    git reset --hard

inside the first first level submodule,

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive git reset --hard

is called. Note the missing --.

. Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the
option --hard is not passed to

git reset

anymore, but leads to git submodule--helper complaining about an unknown option.

Quoted 8 lines

Fix:

. Add -- before the command to execute, such that now correctly

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive -- git reset --hard

is called.

Quoted 14 lines

Signed-off-by Morian Sonnet <[email protected]>

---
 builtin/submodule--helper.c  | 1 +
 t/t7407-submodule-foreach.sh | 7 +++++++
 2 files changed, 8 insertions(+)

builtin/submodule--helper.c +0 -0

index 0bf4aa088e..afaf0819c9 100644
@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 		if (info->quiet)
 			argv_array_push(&cpr.args, "--quiet");
 

builtin/submodule--helper.c static void runcommand_in_submodule_cb(const struct cache_entry *list_item, · line 543

Quoted 1 line

builtin/submodule--helper.c +1 -0

+		argv_array_push(&cpr.args, "--");

Quoted 11 lines

builtin/submodule--helper.c +0 -0

 		argv_array_pushv(&cpr.args, info->argv);
 
 		if (run_command(&cpr))

t/t7407-submodule-foreach.sh +0 -0

index 706ae762e0..c554589e6f 100755
@@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo
 	test_cmp expected actual
 '
 

t/t7407-submodule-foreach.sh test_expect_success 'option-like arguments passed to foreach commands are not lo · line 428

Quoted 5 lines

t/t7407-submodule-foreach.sh +5 -0

+test_expect_success 'option-like arguments passed to foreach recurse correctly' '
+  (
+    cd super &&
+    git submodule foreach --recursive git reset --hard
+  )

Quoted 5 lines

t/t7407-submodule-foreach.sh +2 -1

+'
+
 test_done
--

gitgitgadget

Hi Morian,

Quoted 41 lines · Morian Sonnet

Quoted 39 lines · Morian Sonnet via GitGitGadget

Calling

git submodule foreach --recursive git reset --hard

leads to an error stating that the option --hard is unknown to submodule--helper.

Reasons:

. Above call is internally translated into

    git submodule--helper foreach --recursive -- git reset --hard

. After calling

    git reset --hard

inside the first first level submodule,

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive git reset --hard

is called. Note the missing --.

. Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the
option --hard is not passed to

git reset

anymore, but leads to git submodule--helper complaining about an unknown option.

Fix:

. Add -- before the command to execute, such that now correctly

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive -- git reset --hard

Quoted 1 line

Quoted 1 line

is called.

This is a good explanation, even if the format is not exactly close to the existing commit messages ;-) If you look e.g. at
https://github.com/git/git/commit/e5a329a279c7, you will see what I mean:
there is much more "prose" (and less bullet points) going on.

Quoted 15 lines

Quoted 15 lines

Signed-off-by Morian Sonnet <[email protected]>

---
 builtin/submodule--helper.c  | 1 +
 t/t7407-submodule-foreach.sh | 7 +++++++
 2 files changed, 8 insertions(+)

builtin/submodule--helper.c +0 -0

index 0bf4aa088e..afaf0819c9 100644
@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 		if (info->quiet)
 			argv_array_push(&cpr.args, "--quiet");
 

Quoted 1 line

builtin/submodule--helper.c static void runcommand_in_submodule_cb(const struct cache_entry *list_item, · line 543

Quoted 1 line

builtin/submodule--helper.c +1 -0

+		argv_array_push(&cpr.args, "--");

This is obviously correct.

Quoted 11 lines

Quoted 11 lines

builtin/submodule--helper.c +0 -0

 		argv_array_pushv(&cpr.args, info->argv);
 
 		if (run_command(&cpr))

t/t7407-submodule-foreach.sh +0 -0

index 706ae762e0..c554589e6f 100755
@@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo
 	test_cmp expected actual
 '
 

Quoted 5 lines

t/t7407-submodule-foreach.sh test_expect_success 'option-like arguments passed to foreach commands are not lo · line 428

Quoted 5 lines

t/t7407-submodule-foreach.sh +5 -0

+test_expect_success 'option-like arguments passed to foreach recurse correctly' '
+  (
+    cd super &&
+    git submodule foreach --recursive git reset --hard
+  )

I am terribly sorry that I did not catch this in the first round. I would find it even easier to read if it used the -C option, like so:

git -C super submodule foreach --recursive git reset --hard

Then you do not need the subshell, and neither the cd. It would become a one-liner.

However, what is less obvious to me is that this would catch the regression, as I do not see from the context whether the current submodule structure is deep enough to trigger the reported problem. If I understand the commit message correctly, super would have to contain a submodule that itself contains a submodule.

If there was only one level of submodules (and from the context of this diff, it is not clear whether that is the case), the test case would pass even without the code change to submodule.c.

Of course, I can always dig deeper and find out myself (and of course I did exactly that). But in my mind, that points to something we can improve.

What I found is that the test case indeed fails without the fixed builtin/submodule.c, but that the nested submodule does not even need to be checked out, contrary to what I expected after reading the commit message.

And in fact, the nested submodule is not even checked out in the test script!

To make this test case more obvious, and at the same time to test a little more thoroughly, maybe it would make sense to initialize that "inner" submodule (in this test script, super/nested1/nested2), then make it dirty by changing a file before the reset --hard, and afterwards verify that the file in question was successfully reset.

That way, the test case would start to fail if anybody changed the script in a way where the submodule nesting was all of a sudden no longer deep enough to verify what the test case wants to verify.

Otherwise we would risk that this test case would start passing for the wrong reasons at some stage.

What I have in mind would look somewhat like this:

	: make sure that nested2 is initialized &&
	git -C super/nested1 submodule update --init nested2 &&

	: make sure that nested2 is dirty &&
	echo dirty >super/nested1/nested2/file &&
	test_must_fail git -C super/nested1/nested2 update-index --refresh &&

	git -C super submodule foreach --recursive git reset --hard &&
	: now nested2 is clean &&
	git -C super/nested1/nested2 update-index --refresh

I might be overthinking this, though. Maybe it would be enough to make sure that nested1/nested2 is a nested submodule, e.g. by something like this:

	is_submodule () {
		case "$(git -C "${1%/*}" ls-files --stage "${1##*/}")" in
		160000*) return 0;;
		*) return 1;;
		esac
	} &&
	is_submodule super/nested1 &&
	is_submodule super/nested1/nested2 &&
	git -C super submodule foreach --recursive git reset --hard

which has the advantage of looking shorter, but it does not really verify that git reset --hard *has* been working correctly in nested2: it did not, as that nested submodule was not even initialized at that point.

So I dunno... What do you think? How can we make this test case both more understandable and more robust against future edits at the same time?

Ciao,
Johannes

Quoted 6 lines

Quoted 5 lines

t/t7407-submodule-foreach.sh +2 -1

+'
+
 test_done
--

gitgitgadget

Quoted 43 lines · Johannes Schindelin

Quoted 41 lines · Morian Sonnet

Quoted 39 lines · Morian Sonnet via GitGitGadget

Calling

git submodule foreach --recursive git reset --hard

leads to an error stating that the option --hard is unknown to submodule--helper.

Reasons:

. Above call is internally translated into

    git submodule--helper foreach --recursive -- git reset --hard

. After calling

    git reset --hard

inside the first first level submodule,

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive git reset --hard

is called. Note the missing --.

. Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the
option --hard is not passed to

git reset

anymore, but leads to git submodule--helper complaining about an unknown option.

Fix:

. Add -- before the command to execute, such that now correctly

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive -- git reset --hard

Quoted 6 lines

Quoted 1 line

Quoted 1 line

is called.

This is a good explanation, even if the format is not exactly close to the existing commit messages ;-) If you look e.g. at
https://github.com/git/git/commit/e5a329a279c7, you will see what I mean:
there is much more "prose" (and less bullet points) going on.

Ok, I see the differences. Shall I adapt the commit description?

My test so far was designed to not actually check whether git reset --hard has shown any effect.

Taking the test "test "foreach --quiet --recursive"" and the test "option-like arguments passed to foreach commands are not lost" for example, I suggest the following:

	git -C clone2 submodule foreach --recursive "echo be --hard" > expected &&
	git -C clone2 submodule foreach --recursive echo be --hard > actual &&
	grep -sq -e "--hard" expected &&

test_cmp expected actual

As final comment: One could change the flag --hard to something which will definetely never be an option flag for both, echo and git submodule foreach, e.g. --this-will-never-be-an-option.

Johannes Schindelin <[email protected]> wrote:

Hi Morian,

Quoted 47 lines · Morian Sonnet

Quoted 45 lines · Johannes Schindelin

Quoted 42 lines · Morian Sonnet

Quoted 40 lines · Morian Sonnet via GitGitGadget

Calling

git submodule foreach --recursive git reset --hard

leads to an error stating that the option --hard is unknown to submodule--helper.

Reasons:

. Above call is internally translated into

    git submodule--helper foreach --recursive -- git reset --hard

. After calling

    git reset --hard

inside the first first level submodule,

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive git reset --hard

is called. Note the missing --.

. Due to the removal of PARSE_OPT_KEEP_UNKNOWN in commit a282f5a906 the
option --hard is not passed to

git reset

anymore, but leads to git submodule--helper complaining about an unknown option.

Fix:

. Add -- before the command to execute, such that now correctly

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive -- git reset --hard

is called.

Quoted 6 lines

Quoted 4 lines

This is a good explanation, even if the format is not exactly close to the existing commit messages ;-) If you look e.g. at
https://github.com/git/git/commit/e5a329a279c7, you will see what I mean:
there is much more "prose" (and less bullet points) going on.

Ok, I see the differences. Shall I adapt the commit description?

Well, I would if I were you ;-)

Quoted 3 lines

My test so far was designed to not actually check whether git reset --hard has shown any effect.

Right, but the test case's title suggested that...

Quoted 8 lines

Taking the test "test "foreach --quiet --recursive"" and the test "option-like arguments passed to foreach commands are not lost" for example, I suggest the following:

  git -C clone2 submodule foreach --recursive "echo be --hard" > expected &&
  git -C clone2 submodule foreach --recursive echo be --hard > actual &&
  grep -sq -e "--hard" expected &&

Please without the -sq.

Quoted 2 lines

test_cmp expected actual

Sounds good to me, especially with this suggestion:

Quoted 3 lines

As final comment: One could change the flag --hard to something which will definetely never be an option flag for both, echo and git submodule foreach, e.g. --this-will-never-be-an-option.

Personally, I'd go for echo --an-option, and yes, you are absolutely right, the intention of your bug fix is not so much to fix the recursive git reset --hard as it is to fix _any_ recursive command with command-line options.

To be honest, I had missed that git submodule foreach --recursive executes commands even in uninitialized submodules... I could even see how some users might consider that behavior a bug... are you sure this is the case? Is this echo really executed even in nested2?

Calling

    git submodule foreach --recursive <subcommand> --<option>

leads to an error stating that the option --<option> is unknown to submodule--helper. That is of course only, when <option> is not a valid option for git submodule foreach.

The reason for this is, that above call is internally translated into a call to submodule--helper:

    git submodule--helper foreach --recursive \
        -- <subcommand> --<option>

This call starts by executing the subcommand with its option inside the first first level submodule and continues by calling the next iteration of the submodule foreach call

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive <subcommand> --<option>

inside the first level submodule. Note that the double dash in front of the subcommand is missing.

This problem starts to arise only recently, as the PARSE_OPT_KEEP_UNKNOWN flag for the argument parsing of git submodule foreach was removed in commit a282f5a906. Hence, the unknown option is complained about now, as the argument parsing is not properly ended by the double dash.

This commit fixes the problem by adding the double dash in front of the subcommand during the recursion.

Signed-off-by Morian Sonnet <[email protected]>

---
 builtin/submodule--helper.c  | 1 +
 t/t7407-submodule-foreach.sh | 7 +++++++
 2 files changed, 8 insertions(+)

builtin/submodule--helper.c +1 -0

index 0bf4aa088e..afaf0819c9 100644
@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 		if (info->quiet)
 			argv_array_push(&cpr.args, "--quiet");
 
+		argv_array_push(&cpr.args, "--");
 		argv_array_pushv(&cpr.args, info->argv);
 
 		if (run_command(&cpr))

t/t7407-submodule-foreach.sh +7 -1

index 706ae762e0..43da184d40 100755
@@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo
 	test_cmp expected actual
 '
 
+test_expect_success 'option-like arguments passed to foreach recurse correctly' '
+  git -C clone2 submodule foreach --recursive "echo be --an-option" > expected &&
+  git -C clone2 submodule foreach --recursive echo be --an-option > actual &&
+  grep -e "--an-option" expected &&
+  test_cmp expected actual
+'
+
 test_done
--

2.20.1

Calling

    git submodule foreach --recursive <subcommand> --<option>

leads to an error stating that the option --<option> is unknown to submodule--helper. That is of course only, when <option> is not a valid option for git submodule foreach.

The reason for this is, that above call is internally translated into a call to submodule--helper:

    git submodule--helper foreach --recursive \
        -- <subcommand> --<option>

This call starts by executing the subcommand with its option inside the first first level submodule and continues by calling the next iteration

of the submodule foreach call

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive <subcommand> --<option>

inside the first level submodule. Note that the double dash in front of the subcommand is missing.

This problem starts to arise only recently, as the PARSE_OPT_KEEP_UNKNOWN flag for the argument parsing of git submodule foreach was removed in commit a282f5a906. Hence, the unknown option is complained about now, as the argument parsing is not properly ended by the double dash.

This commit fixes the problem by adding the double dash in front of the subcommand during the recursion.

Signed-off-by Morian Sonnet <[email protected]>

---
 builtin/submodule--helper.c  | 1 +
 t/t7407-submodule-foreach.sh | 7 +++++++
 2 files changed, 8 insertions(+)

builtin/submodule--helper.c +1 -0

index 0bf4aa088e..afaf0819c9 100644
@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 		if (info->quiet)
 			argv_array_push(&cpr.args, "--quiet");
 
+		argv_array_push(&cpr.args, "--");
 		argv_array_pushv(&cpr.args, info->argv);
 
 		if (run_command(&cpr))

t/t7407-submodule-foreach.sh +0 -0

index 706ae762e0..43da184d40 100755

Quoted 8 lines

Calling

    git submodule foreach --recursive <subcommand> --<option>

leads to an error stating that the option --<option> is unknown to submodule--helper. That is of course only, when <option> is not a valid option for git submodule foreach.

Quoted 8 lines

The reason for this is, that above call is internally translated into a call to submodule--helper:

    git submodule--helper foreach --recursive \
        -- <subcommand> --<option>

This call starts by executing the subcommand with its option inside the first first level submodule and continues by calling the next iteration

first first???

Quoted 40 lines

of the submodule foreach call

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive <subcommand> --<option>

inside the first level submodule. Note that the double dash in front of the subcommand is missing.

This problem starts to arise only recently, as the PARSE_OPT_KEEP_UNKNOWN flag for the argument parsing of git submodule foreach was removed in commit a282f5a906. Hence, the unknown option is complained about now, as the argument parsing is not properly ended by the double dash.

This commit fixes the problem by adding the double dash in front of the subcommand during the recursion.

Signed-off-by Morian Sonnet <[email protected]>

---
 builtin/submodule--helper.c  | 1 +
 t/t7407-submodule-foreach.sh | 7 +++++++
 2 files changed, 8 insertions(+)

builtin/submodule--helper.c +1 -0

index 0bf4aa088e..afaf0819c9 100644
@@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
 		if (info->quiet)
 			argv_array_push(&cpr.args, "--quiet");
 
+		argv_array_push(&cpr.args, "--");
 		argv_array_pushv(&cpr.args, info->argv);
 
 		if (run_command(&cpr))

t/t7407-submodule-foreach.sh +0 -0

index 706ae762e0..43da184d40 100755

t/t7407-submodule-foreach.sh test_expect_success 'option-like arguments passed to foreach commands are not lo · line 429

Quoted 9 lines

t/t7407-submodule-foreach.sh +6 -0

@@ -421,4 +421,11 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo
 	test_cmp expected actual
 '
 
+test_expect_success 'option-like arguments passed to foreach recurse correctly' '
+  git -C clone2 submodule foreach --recursive "echo be --an-option" > expected &&
+  git -C clone2 submodule foreach --recursive echo be --an-option > actual &&
+  grep -e "--an-option" expected &&
+  test_cmp expected actual
+'

Some shell style nits.

- our shell scripts use HT for indentation, not two SPs.

- there is one SP before a redirection operator (you did it correctly), and no SP after a redirection operator before the target filename.

- mostly we prepare the right answer in 'expect' and take the output from tested command in 'actual' to compare. There are a few 'expected' in this test file already, so you are not introducing a new inconsistency, but you are making things worse. Don't.

Other than that, your fix looks quite nicely described and executed.

Thanks.

Calling

    git submodule foreach --recursive <subcommand> --<option>

leads to an error stating that the option --<option> is unknown to submodule--helper. That is of course only, when <option> is not a valid option for git submodule foreach.

The reason for this is, that above call is internally translated into a call to submodule--helper:

    git submodule--helper foreach --recursive \
        -- <subcommand> --<option>

This call starts by executing the subcommand with its option inside the first level submodule and continues by calling the next iteration of the submodule foreach call

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive <subcommand> --<option>

inside the first level submodule. Note that the double dash in front of the subcommand is missing.

This problem starts to arise only recently, as the PARSE_OPT_KEEP_UNKNOWN flag for the argument parsing of git submodule foreach was removed in commit a282f5a906. Hence, the unknown option is complained about now, as the argument parsing is not properly ended by the double dash.

Quoted 22 lines · Morian Sonnet

Calling

    git submodule foreach --recursive <subcommand> --<option>

leads to an error stating that the option --<option> is unknown to submodule--helper. That is of course only, when <option> is not a valid option for git submodule foreach.

The reason for this is, that above call is internally translated into a call to submodule--helper:

    git submodule--helper foreach --recursive \
        -- <subcommand> --<option>

This call starts by executing the subcommand with its option inside the first level submodule and continues by calling the next iteration of the submodule foreach call

    git --super-prefix <submodulepath> submodule--helper \
      foreach --recursive <subcommand> --<option>

Quoted 8 lines

inside the first level submodule. Note that the double dash in front of the subcommand is missing.

This problem starts to arise only recently, as the PARSE_OPT_KEEP_UNKNOWN flag for the argument parsing of git submodule foreach was removed in commit a282f5a906. Hence, the unknown option is complained about now, as the argument parsing is not properly ended by the double dash.

My bad. Last time I checked *.sh but forgot about *.c. I looked around this time in *.c. This should be the only submodule--helper invocation that needs "--".