Re: [GSoC PATCH v2] submodule: warn on valueless active config
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 14, 2026, 22:04 UTC
- Message-ID
- <xmqqqzk0l5oz.fsf@gitster.g>
- In-Reply-To
- <20260814212431.43626-1-raaztilak07@gmail.com>
tilak-raaz <raaztilak07@gmail.com> writes:
> The config parser previously threw a hard error if 'submodule.active' > was provided without a value, causing commands to abort.
An exerpt from Documentation/SubmittingPatches:
[[present-tense]]
The problem statement that describes the status quo is written in the
present tense. Write "The code does X when it is given input Y",
instead of "The code used to do Y when given input X". You do not
have to say "Currently"---the status quo in the problem statement is
about the code _without_ your change, by project convention.> Swap repo_config_get_string_multi() to repo_config_get_value_multi() > to parse valueless keys safely. Use the standard config_error_nonbool()
"valueless true", I think.
> helper to emit a warning to the user rather than crashing.
Good.
> This resolves a NEEDSWORK comment in submodule.c.
Good. Resolving an existing NEEDSWORK is a two step process, (1) to determine if it still does make sense to do what it suggests to do, and then (2) do it. The early part of the proposed log message solves a half of step (1), in a sense that crashing is bad. The other half is what we should do instead of crashing.
Show 26 quoted lines
> -/*
> - * NEEDSWORK: Emit a warning if submodule.active exists, but is valueless,
> - * ie, the config looks like: "[submodule] active\n".
> - * Since that is an invalid pathspec, we should inform the user.
> - */
> +
> int is_tree_submodule_active(struct repository *repo,
> const struct object_id *treeish_name,
> const char *path)
> @@ -261,12 +257,16 @@ int is_tree_submodule_active(struct repository *repo,
> free(key);
>
> /* submodule.active is set */
> - if (!repo_config_get_string_multi(repo, "submodule.active", &sl)) {
> + if (!repo_config_get_value_multi(repo, "submodule.active", &sl)) {
> struct pathspec ps;
> struct strvec args = STRVEC_INIT;
> const struct string_list_item *item;
>
> for_each_string_list_item(item, sl) {
> + if (!item->string) {
> + config_error_nonbool("submodule.active");
> + continue;
> + }
> strvec_push(&args, item->string);
> }And we do warn, but I am not sure if "continue" is sensible, though.
Since we know that the configuration is broken, we should cause the command to fail (i.e., exit with a non-zero status), shouldn't we?
Show 18 quoted lines
> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index eefdecb0bd..74c26f6630 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -1549,4 +1549,15 @@ test_expect_success 'submodule add fails when name is reused' ' > ) > ' > > + > +test_expect_success 'warn on valueless submodule.active' ' > +test_when_finished "rm -rf empty-active" && > +git init empty-active && > +test_commit -C empty-active initial && > +git -c protocol.file.allow=always -C empty-active submodule add ../empty-active sub && > +git -C empty-active config --unset submodule.sub.active && > +printf "[submodule]\n\tactive\n" >>empty-active/.git/config && > +git -C empty-active submodule status 2>err &&
In other words, shouldn't this say
test_must_fail git submodule status &&
> +grep "missing value for .submodule.active." err > +'
Curiously, the test part of your patch is severely whitespace-damaged, even though the C part looked OK. This is quite puzzling.