| Message ID | cad05012-7bf9-5975-3add-253b11c7bcc8@cs-ware.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Don't pass -v to submodule command | expand |
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.
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.
Æ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.
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)
"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(-)