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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 30, 2022, 19:17 UTC
Message-ID: <221130.868rjsi6bn.gmgdl@evledraar.gmail.com>
In-Reply-To: <cad05012-7bf9-5975-3add-253b11c7bcc8@cs-ware.de>

On Wed, Nov 30 2022, Sven Strickroth wrote:

Show 5 quoted lines

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

Show 18 quoted lines

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

Show 45 quoted lines

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