git pull --verbose with submodules ends in error message

git pull --verbose with submodules ends in error message

[Fink, Mike]

Dear Sir or Madam, 


Bug Description
===============

when doing a git pull on a repository with submodules, the --verbose option causes an error message like: 
usage: git submodule [--quiet] [--cached]
   or: git submodule [--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
... and so on.
Exit code is 1

This happens, when recursing the submodules. Either add to .gitconfig:
[submodule]
	recurse = true
or use --recurse-submodules for the git pull command.

Version 2.37.3-64-bit is OK.
Version 2.38.1-64-bit is shows the error.
Reproducible on Windows and Linux


How to Reproduce
================
# steps to reproduce: 
# run the following commands in a debian:sid container, 
# e.g. "docker container run -it debian:sid".

# Any other environment with git 2.38.1 should be fine, too. 
# Git for windows 2.38.1 also shows the same behaviour.

# install git 2.38.1 (at the time of writing) and clone a public repo with submodules
$ apt update && apt install git
$ git clone --recurse-submodules https://gitlab.com/tortoisegit/tortoisegit.git
$ cd tortoisegit

# this one succeeds
$ git pull --recurse-submodules

# this one fails after fetching the submodules, showing 
# the "git submodule" help text as if we had made a "git submodule" 
# call with insufficient/wrong arguments.
$ git pull --recurse-submodules --verbose


Workaround
==========
1) Do not use --verbose when pulling a repository with submodules.
   Unfortunately this workaround does not apply to our workflow, 
   since we happily use TortoiseGit as our graphical Git client on windows. 
   TortoiseGit automatically uses the option -v (--verbose).
   $ git.exe pull --progress -v --no-rebase "origin"
2) Use Version 2.37.3-64-bit. 


Questions
=========
Any questions regarding this bug description? Happy to help.


Kind regards, Mike.

--

Mike Fink
Softwareentwickler
E44

SAMSON AKTIENGESELLSCHAFT
Weismüllerstraße 3 · 60314 Frankfurt am Main

Telefon: +49 69 4009-1682
E-Mail: Mike.Fink@samsongroup.com · Internet: www.samsongroup.com

Re: git pull --verbose with submodules ends in error message

[Sven Strickroth]

Hi,

the reason for the failure of the submodule command after issuing
"git pull -v --recurse-submodules"
is that the verbosity of the pull command is passed to the submodules. 
Commit a56771a668dd4963675914bc5da0e1e015952dae introduced this regression.

I suppose the intention was to pass the '-q' flag to the submodule 
command, but the issue is that also '-v' is passed which, however, is 
not supported by the submodule command.

So, either don't pass '-v' to the submodule command or add it there...

Best,
  Sven

Am 24.11.2022 um 13:47 schrieb Fink, Mike:
> Dear Sir or Madam,
> 
> 
> Bug Description
> ===============
> 
> when doing a git pull on a repository with submodules, the --verbose option causes an error message like:
> usage: git submodule [--quiet] [--cached]
>     or: git submodule [--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <repository>] [--] <repository> [<path>]
> ... and so on.
> Exit code is 1
> 
> This happens, when recursing the submodules. Either add to .gitconfig:
> [submodule]
> 	recurse = true
> or use --recurse-submodules for the git pull command.
> 
> Version 2.37.3-64-bit is OK.
> Version 2.38.1-64-bit is shows the error.
> Reproducible on Windows and Linux
> 
> 
> How to Reproduce
> ================
> # steps to reproduce:
> # run the following commands in a debian:sid container,
> # e.g. "docker container run -it debian:sid".
> 
> # Any other environment with git 2.38.1 should be fine, too.
> # Git for windows 2.38.1 also shows the same behaviour.
> 
> # install git 2.38.1 (at the time of writing) and clone a public repo with submodules
> $ apt update && apt install git
> $ git clone --recurse-submodules https://gitlab.com/tortoisegit/tortoisegit.git
> $ cd tortoisegit
> 
> # this one succeeds
> $ git pull --recurse-submodules
> 
> # this one fails after fetching the submodules, showing
> # the "git submodule" help text as if we had made a "git submodule"
> # call with insufficient/wrong arguments.
> $ git pull --recurse-submodules --verbose
> 
> 
> Workaround
> ==========
> 1) Do not use --verbose when pulling a repository with submodules.
>     Unfortunately this workaround does not apply to our workflow,
>     since we happily use TortoiseGit as our graphical Git client on windows.
>     TortoiseGit automatically uses the option -v (--verbose).
>     $ git.exe pull --progress -v --no-rebase "origin"
> 2) Use Version 2.37.3-64-bit.
> 
> 
> Questions
> =========
> Any questions regarding this bug description? Happy to help.
> 
> 
> Kind regards, Mike.

[PATCH] Don't pass -v to submodule command

[Sven Strickroth]

"git pull -v --recurse-submodules" propagates the "-v" to the submdoule
command which does not support "-v".

Commit a56771a668dd4963675914bc5da0e1e015952dae introduced this
regression.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
  builtin/pull.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 1ab4de0005..b67320fa5f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -256,7 +256,7 @@ static struct option pull_options[] = {
  /**
   * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
   */
-static void argv_push_verbosity(struct strvec *arr)
+static void argv_push_verbosity(struct strvec *arr, int include_v)
  {
  	int verbosity;

@@ -520,7 +520,7 @@ static int run_fetch(const char *repo, const char **refspecs)
  	strvec_pushl(&cmd.args, "fetch", "--update-head-ok", NULL);

  	/* Shared options */
-	argv_push_verbosity(&cmd.args);
+	argv_push_verbosity(&cmd.args, 1);
  	if (opt_progress)
  		strvec_push(&cmd.args, opt_progress);

@@ -629,7 +629,7 @@ static int rebase_submodules(void)
  	cp.no_stdin = 1;
  	strvec_pushl(&cp.args, "submodule", "update",
  		     "--recursive", "--rebase", NULL);
-	argv_push_verbosity(&cp.args);
+	argv_push_verbosity(&cp.args, 0);

  	return run_command(&cp);
  }
@@ -642,7 +642,7 @@ static int update_submodules(void)
  	cp.no_stdin = 1;
  	strvec_pushl(&cp.args, "submodule", "update",
  		     "--recursive", "--checkout", NULL);
-	argv_push_verbosity(&cp.args);
+	argv_push_verbosity(&cp.args, 0);

  	return run_command(&cp);
  }
@@ -657,7 +657,7 @@ static int run_merge(void)
  	strvec_pushl(&cmd.args, "merge", NULL);

  	/* Shared options */
-	argv_push_verbosity(&cmd.args);
+	argv_push_verbosity(&cmd.args, 1);
  	if (opt_progress)
  		strvec_push(&cmd.args, opt_progress);

@@ -881,7 +881,7 @@ static int run_rebase(const struct object_id *newbase,
  	strvec_push(&cmd.args, "rebase");

  	/* Shared options */
-	argv_push_verbosity(&cmd.args);
+	argv_push_verbosity(&cmd.args, 1);

  	/* Options passed to git-rebase */
  	if (opt_rebase == REBASE_MERGES)
-- 
2.38.1.windows.1

Re: [PATCH] Don't pass -v to submodule command

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 30 2022, Sven Strickroth wrote:

> "git pull -v --recurse-submodules" propagates the "-v" to the submdoule
> command which does not support "-v".
>
> Commit a56771a668dd4963675914bc5da0e1e015952dae introduced this
> regression.

We refer to commits in commit messages like this: a56771a668d
(builtin/pull: respect verbosity settings in submodules, 2018-01-25);

Which also shows that this regression is quite old.

> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---
>  builtin/pull.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1ab4de0005..b67320fa5f 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -256,7 +256,7 @@ static struct option pull_options[] = {
>  /**
>   * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
>   */
> -static void argv_push_verbosity(struct strvec *arr)
> +static void argv_push_verbosity(struct strvec *arr, int include_v)
>  {
>  	int verbosity;
>

It looks like you're getting somewhere with this, but you never use this
"include_v", so the bug is still there. We just have the scaffolding
now.

Did you forget to add that part to this commit?

In any case, that serves as a comment on the other thing this patch
really needs: tests, please add some.

I can reproduce this locally by just running the command you noted in a
repo with submodules, so presumably we can use some of the existing
submodule tests, which have already set up such a repo.

> @@ -520,7 +520,7 @@ static int run_fetch(const char *repo, const char **refspecs)
>  	strvec_pushl(&cmd.args, "fetch", "--update-head-ok", NULL);
>
>  	/* Shared options */
> -	argv_push_verbosity(&cmd.args);
> +	argv_push_verbosity(&cmd.args, 1);
>  	if (opt_progress)
>  		strvec_push(&cmd.args, opt_progress);
>
> @@ -629,7 +629,7 @@ static int rebase_submodules(void)
>  	cp.no_stdin = 1;
>  	strvec_pushl(&cp.args, "submodule", "update",
>  		     "--recursive", "--rebase", NULL);
> -	argv_push_verbosity(&cp.args);
> +	argv_push_verbosity(&cp.args, 0);
>
>  	return run_command(&cp);
>  }
> @@ -642,7 +642,7 @@ static int update_submodules(void)
>  	cp.no_stdin = 1;
>  	strvec_pushl(&cp.args, "submodule", "update",
>  		     "--recursive", "--checkout", NULL);
> -	argv_push_verbosity(&cp.args);
> +	argv_push_verbosity(&cp.args, 0);
>
>  	return run_command(&cp);
>  }
> @@ -657,7 +657,7 @@ static int run_merge(void)
>  	strvec_pushl(&cmd.args, "merge", NULL);
>
>  	/* Shared options */
> -	argv_push_verbosity(&cmd.args);
> +	argv_push_verbosity(&cmd.args, 1);
>  	if (opt_progress)
>  		strvec_push(&cmd.args, opt_progress);
>
> @@ -881,7 +881,7 @@ static int run_rebase(const struct object_id *newbase,
>  	strvec_push(&cmd.args, "rebase");
>
>  	/* Shared options */
> -	argv_push_verbosity(&cmd.args);
> +	argv_push_verbosity(&cmd.args, 1);
>
>  	/* Options passed to git-rebase */
>  	if (opt_rebase == REBASE_MERGES)

I think the right longer term fix here is to simply make "git submodule"
support "-v" and "--verbose".

Which, as a funny implementation detail we'd support if we called "git
submodule--helper update", as its OPT__QUIET() adds both variants, but
the git-submodule.sh doesn't support it.

OTOH we've never supported it in "git submodule", so maybe we should
just make the C version stricter, I dunno...

In any case, this is a good fix for now, let's just stop passing the
unsupported flag.

Re: [PATCH] Don't pass -v to submodule command

[Sven Strickroth]

Am 30.11.2022 um 20:17 schrieb Ævar Arnfjörð Bjarmason:
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -256,7 +256,7 @@ static struct option pull_options[] = {
>>   /**
>>    * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
>>    */
>> -static void argv_push_verbosity(struct strvec *arr)
>> +static void argv_push_verbosity(struct strvec *arr, int include_v)
>>   {
>>   	int verbosity;
>>
> 
> It looks like you're getting somewhere with this, but you never use this
> "include_v", so the bug is still there. We just have the scaffolding
> now.
> 
> Did you forget to add that part to this commit?

Opps, seems so.

> In any case, that serves as a comment on the other thing this patch
> really needs: tests, please add some.

I don't know how to add tests and don't have a fully fledged build 
environment for git here.

-- 
Best regards,
   Sven Strickroth
   PGP key id F5A9D4C4 @ any key-server

[PATCH v2] Don't pass -v to submodule command

[Sven Strickroth]

"git pull -v --recurse-submodules" propagates the "-v" to the submdoule
command which does not support "-v" yet.

Commit a56771a668d introduced this regression.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
  builtin/pull.c | 14 +++++++-------
  1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 1ab4de0005..c7f65b39ec 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -256,11 +256,11 @@ static struct option pull_options[] = {
  /**
   * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
   */
-static void argv_push_verbosity(struct strvec *arr)
+static void argv_push_verbosity(struct strvec *arr, int include_v)
  {
  	int verbosity;

-	for (verbosity = opt_verbosity; verbosity > 0; verbosity--)
+	for (verbosity = opt_verbosity; include_v && verbosity > 0; verbosity--)
  		strvec_push(arr, "-v");

  	for (verbosity = opt_verbosity; verbosity < 0; verbosity++)
@@ -520,7 +520,7 @@ static int run_fetch(const char *repo, const char **refspecs)
  	strvec_pushl(&cmd.args, "fetch", "--update-head-ok", NULL);

  	/* Shared options */
-	argv_push_verbosity(&cmd.args);
+	argv_push_verbosity(&cmd.args, 1);
  	if (opt_progress)
  		strvec_push(&cmd.args, opt_progress);

@@ -629,7 +629,7 @@ static int rebase_submodules(void)
  	cp.no_stdin = 1;
  	strvec_pushl(&cp.args, "submodule", "update",
  		     "--recursive", "--rebase", NULL);
-	argv_push_verbosity(&cp.args);
+	argv_push_verbosity(&cp.args, 0);

  	return run_command(&cp);
  }
@@ -642,7 +642,7 @@ static int update_submodules(void)
  	cp.no_stdin = 1;
  	strvec_pushl(&cp.args, "submodule", "update",
  		     "--recursive", "--checkout", NULL);
-	argv_push_verbosity(&cp.args);
+	argv_push_verbosity(&cp.args, 0);

  	return run_command(&cp);
  }
@@ -657,7 +657,7 @@ static int run_merge(void)
  	strvec_pushl(&cmd.args, "merge", NULL);

  	/* Shared options */
-	argv_push_verbosity(&cmd.args);
+	argv_push_verbosity(&cmd.args, 1);
  	if (opt_progress)
  		strvec_push(&cmd.args, opt_progress);

@@ -881,7 +881,7 @@ static int run_rebase(const struct object_id *newbase,
  	strvec_push(&cmd.args, "rebase");

  	/* Shared options */
-	argv_push_verbosity(&cmd.args);
+	argv_push_verbosity(&cmd.args, 1);

  	/* Options passed to git-rebase */
  	if (opt_rebase == REBASE_MERGES)
-- 
2.38.1.windows.1

Re: [PATCH] Don't pass -v to submodule command

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Nov 30 2022, Sven Strickroth wrote:
>
>> "git pull -v --recurse-submodules" propagates the "-v" to the submdoule
>> command which does not support "-v".
>>
>> Commit a56771a668dd4963675914bc5da0e1e015952dae introduced this
>> regression.
>
> We refer to commits in commit messages like this: a56771a668d
> (builtin/pull: respect verbosity settings in submodules, 2018-01-25);
>
> Which also shows that this regression is quite old.

Good point.

While we are commenting on the proposed log message, this subject

> Subject: [PATCH] Don't pass -v to submodule command

is not sufficient to identify the change and remind readers what it
is about when it is shown among "git shortlog --no-merges".  We give
"<area>:" prefix before the title to help that, e.g.

    Subject: [PATCH] pull: don't pass -v to "git submodule update"

or something like that.

>> Signed-off-by: Sven Strickroth <email@cs-ware.de>
>> ---
>>  builtin/pull.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 1ab4de0005..b67320fa5f 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -256,7 +256,7 @@ static struct option pull_options[] = {
>>  /**
>>   * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
>>   */
>> -static void argv_push_verbosity(struct strvec *arr)
>> +static void argv_push_verbosity(struct strvec *arr, int include_v)
>>  {
>>  	int verbosity;
>>
>
> It looks like you're getting somewhere with this, but you never use this
> "include_v", so the bug is still there. We just have the scaffolding
> now.

What is the plan to cope with the evolution of "git submodule
update" command, though?  Will "-v" forever be the single option we
may get at "git pull" level that will never be supported by "git
submodule update"?  I am guessing that the reason we want to call
this flag "include_v" is because it is the author's intention that
"git submodule update" will not change in this regard, and am
wondering if that is a healthy assumption.

> Did you forget to add that part to this commit?
>
> In any case, that serves as a comment on the other thing this patch
> really needs: tests, please add some.

Good advice.

> I think the right longer term fix here is to simply make "git submodule"
> support "-v" and "--verbose".

Yup.

[PATCH] submodule: Accept -v for update command

[Sven Strickroth]

"git pull -v --recurse-submodules" propagates the "-v" to the submodule 
command which did not support "-v" yet.

Commit a56771a668d introduced this regression.

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
  git-submodule.sh | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9a50f2e912..7f9582d923 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -244,6 +244,9 @@ cmd_update()
  		-q|--quiet)
  			quiet=1
  			;;
+		-v|--verbose)
+			quiet=0
+			;;
  		--progress)
  			progress=1
  			;;
-- 
2.38.1.windows.1

Re: [PATCH] submodule: Accept -v for update command

[Junio C Hamano]

Sven Strickroth <email@cs-ware.de> writes:

> Subject: Re: [PATCH] submodule: Accept -v for update command
>
> "git pull -v --recurse-submodules" propagates the "-v" to the
> submodule command which did not support "-v" yet.
>
> Commit a56771a668d introduced this regression.
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

This unfortunately fell in the cracks.  Thanks for a few people who
reported the issue this patch tried to fix recently (it is curous
why this regression that is almost 5 years old suddenly started
biting people).

Applying the improvement suggestions given in the review messages to
the other patch to deal with this regression from the "pull" side,
let's explain the commit this way:

    Subject: [PATCH] submodule: accept -v for the update command

    Since a56771a6 (builtin/pull: respect verbosity settings in
    submodules, 2018-01-25), "git pull -v --recurse-submodules"
    propagates the "-v" to the submodule command, but because the
    latter command does not understand the option, it barfs.

    Teach "git submodule update" to accept the option to fix it.

    Signed-off-by: Sven Strickroth <email@cs-ware.de>
    
Thanks.

>  git-submodule.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9a50f2e912..7f9582d923 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -244,6 +244,9 @@ cmd_update()
>  		-q|--quiet)
>  			quiet=1
>  			;;
> +		-v|--verbose)
> +			quiet=0
> +			;;
>  		--progress)
>  			progress=1
>  			;;