Skip to content

submodule foreach: fix recursion of options #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

submodule foreach: fix recursion of options #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions builtin/submodule--helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item,
if (info->quiet)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Wed, Jun 12, 2019 at 2:10 PM Morian Sonnet via GitGitGadget
<[email protected]> wrote:
> [...]
> . 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]>
> ---
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> @@ -421,4 +421,9 @@ test_expect_success 'option-like arguments passed to foreach commands are not lo
> +test_expect_success 'option-like arguments passed to foreach recurse correctly' '
> +  cd super &&
> +  git submodule foreach --recursive git reset --hard
> +'

Follow the example of the existing tests in this script and do the
'cd' in a subshell so that it doesn't bleed into tests added later
following this one, thus confusing them.

    (
        cd super &&
        git submodule foreach ...
    )

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Morian Sonnet wrote (reply to this):

"Morian Sonnet via GitGitGadget" <[email protected]> wrote:

I fixed the problem with the test case. Please take another look.

> 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(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 0bf4aa088e..afaf0819c9 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -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))
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 706ae762e0..c554589e6f 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -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
> -- 
> gitgitgadget

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Morian,

On Tue, 18 Jun 2019, Morian Sonnet wrote:

> "Morian Sonnet via GitGitGadget" <[email protected]> wrote:
>
> > 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 th=
e
> >   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.

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.

> > Signed-off-by: Morian Sonnet <[email protected]>
> > ---
> >  builtin/submodule--helper.c  | 1 +
> >  t/t7407-submodule-foreach.sh | 7 +++++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 0bf4aa088e..afaf0819c9 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -540,6 +540,7 @@ static void runcommand_in_submodule_cb(const struc=
t cache_entry *list_item,
> >  		if (info->quiet)
> >  			argv_array_push(&cpr.args, "--quiet");
> >
> > +		argv_array_push(&cpr.args, "--");

This is obviously correct.

> >  		argv_array_pushv(&cpr.args, info->argv);
> >
> >  		if (run_command(&cpr))
> > diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.=
sh
> > index 706ae762e0..c554589e6f 100755
> > --- a/t/t7407-submodule-foreach.sh
> > +++ b/t/t7407-submodule-foreach.sh
> > @@ -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
> > +  )

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

> > +'
> > +
> >  test_done
> > --
> > gitgitgadget
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Morian Sonnet wrote (reply to this):

Johannes Schindelin <[email protected]> wrote:

Hello Johannes,

thank your for the review. Sorry for the spam, I messed up with the
replytoall command.

> Hi Morian,
>
> On Tue, 18 Jun 2019, Morian Sonnet wrote:
>
> > "Morian Sonnet via GitGitGadget" <[email protected]> wrote:
> >
> > > 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.
>
> 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?

>
> > > Signed-off-by: Morian Sonnet <[email protected]>
> > > ---
> > >  builtin/submodule--helper.c  | 1 +
> > >  t/t7407-submodule-foreach.sh | 7 +++++++
> > >  2 files changed, 8 insertions(+)
> > >
> > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > > index 0bf4aa088e..afaf0819c9 100644
> > > --- a/builtin/submodule--helper.c
> > > +++ b/builtin/submodule--helper.c
> > > @@ -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, "--");
>
> This is obviously correct.
>
> > >  		argv_array_pushv(&cpr.args, info->argv);
> > >
> > >  		if (run_command(&cpr))
> > > diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> > > index 706ae762e0..c554589e6f 100755
> > > --- a/t/t7407-submodule-foreach.sh
> > > +++ b/t/t7407-submodule-foreach.sh
> > > @@ -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
> > > +  )
>
> 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.

Alright, I have to admit I wasn't aware of that option.

>
> 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`.

This is not entirely correct, I think. In the case where we found the
error, no nested submodules were present. The buildkite agent however,
due to the mere presence of submodules, still calls git submodule with
the recursive flag.
Inside submodule-helper the next level of the submodule-helper is called
again, without checking for the presence of nested submodules.
Hence, one level of submodules should be enough for now.

However, in sight of making the test more robust, one should actually
use at least a two level deep git for checking, so that in future no
potential false positives are created. That is, if the submodule-helper
suddenly starts checking for the presence of nested submodules, before
calling its next iteration.

>
> 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!

The reasons for these points are explained by the text above, as the
submodule-helper does not check for the presence of nested submodules
before calling its next iteration.

>
> 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?

My test so far was designed to not actually check whether git reset
--hard has shown any effect. I think other tests in the test suite exist
already, testing for the correct execution of the command in each
submodule. See for example "test messages from "foreach --recursive" from
subdirectory'" and following.

This is why I think the second test of yours would be totally sufficient
to test the introduced changes. That is of course only in combination
with the already existing tests, whose testing functionality will
remain. (I think that is a safe assumption.)

However, I think that your test might be a little bit too much,
comparing it to the other tests of the suite.

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

By using clone2 we can be sure that we have a fully initalized git setup
with at least two levels of submodules, as it is also used by several
previous tests. By grepping the expected we can also be sure that the
echo command was correctly executed as reference. Finally, comparing the
actual output with the expected output then shows that the option is
also correctly passed without using the "".

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.

Please tell me what you think about the proposed test.

Best regards,
    Morian

>
> Ciao,
> Johannes
>
> > > +'
> > > +
> > >  test_done
> > > --
> > > gitgitgadget
> >

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Morian,

On Sat, 22 Jun 2019, Morian Sonnet wrote:

> Johannes Schindelin <[email protected]> wrote:
>
> > On Tue, 18 Jun 2019, Morian Sonnet wrote:
> >
> > > "Morian Sonnet via GitGitGadget" <[email protected]> wrote:
> > >
> > > > 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 a282f5a90=
6 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.
> >
> > 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 me=
an:
> > 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 ;-)

> 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...

> 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`.

> 	test_cmp expected actual

Sounds good to me, especially with this suggestion:

> 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`?

Ciao,
Johannes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Morian Sonnet wrote (reply to this):

here comes the next version.

> Johannes Schindelin <[email protected]> wrote:
> 
> Hi Morian,
> 
> On Sat, 22 Jun 2019, Morian Sonnet wrote:
> 
> > Johannes Schindelin <[email protected]> wrote:
> >
> > > On Tue, 18 Jun 2019, Morian Sonnet wrote:
> > >
> > > > "Morian Sonnet via GitGitGadget" <[email protected]> wrote:
> > > >
> > > > > 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.
> > >
> > > 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 ;-)

Ok, done.

> 
> > 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...

The commit message is very specific regarding git reset --hard, right.
Changed that.

> 
> > 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`.

Oh, ok, i took it from another test in that suite.

> 
> >   test_cmp expected actual
> 
> Sounds good to me, especially with this suggestion:
> 
> > 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`?

I think this is a misunderstanding. I did not mean that the subcommand
is executed in `nested2` (which it actually is not) but rather that
`git submodule--helper foreach ...`
is again executed in `nested1` and this even if `nested1` does not
include any submodules.

Best regards,
  Morian

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Morian Sonnet <[email protected]> writes:

> 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

first first???

> 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(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 0bf4aa088e..afaf0819c9 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -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))
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 706ae762e0..43da184d40 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Morian Sonnet wrote (reply to this):

Hi Junio,

thanks for your comments.

I fixed the type in the commit message and adapted my test case to the
required shell script style.

Best regards,
   Morian


Junio C Hamano <[email protected]> writes:

> > 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
> 
> first first???
> 
> > 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(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 0bf4aa088e..afaf0819c9 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -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))
> > diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> > index 706ae762e0..43da184d40 100755
> > --- a/t/t7407-submodule-foreach.sh
> > +++ b/t/t7407-submodule-foreach.sh
> > @@ -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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Morian Sonnet wrote (reply to this):

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.

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(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0bf4aa088e..afaf0819c9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -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))
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 706ae762e0..6b2aa917e1 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -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" >expect &&
+	git -C clone2 submodule foreach --recursive echo be --an-option >actual &&
+	grep -e "--an-option" expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.20.1

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Duy Nguyen wrote (reply to this):

On Tue, Jun 25, 2019 at 5:02 AM Morian Sonnet <[email protected]> wrote:
>
> 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.

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 "--".
-- 
Duy

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Duy Nguyen <[email protected]> writes:

> On Tue, Jun 25, 2019 at 5:02 AM Morian Sonnet <[email protected]> wrote:
>>
>> 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.
>
> 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 "--".

Thanks, both.  Will queue.

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

argv_array_push(&cpr.args, "--");
argv_array_pushv(&cpr.args, info->argv);

if (run_command(&cpr))
Expand Down
7 changes: 7 additions & 0 deletions t/t7407-submodule-foreach.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" >expect &&
git -C clone2 submodule foreach --recursive echo be --an-option >actual &&
grep -e "--an-option" expect &&
test_cmp expect actual
'

test_done