{"thread":{"id":"66179","subject":"[PATCH v3] submodule: warn on valueless active config","startedAt":"2026-08-15T07:01:33Z","lastAt":"2026-08-17T17:43:37Z","messageCount":3,"participants":["tilak-raaz","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"550658","messageId":"20260815070004.17538-1-raaztilak07@gmail.com","threadId":"66179","inReplyTo":"MESSAGE_ID_FROM_JUNIOS_LAST_EMAIL","subject":"[PATCH v3] submodule: warn on valueless active config","fromName":"tilak-raaz","fromEmail":"raaztilak07@gmail.com","sentAt":"2026-08-15T07:00:04Z","receivedAt":"2026-08-15T07:01:33Z","isPatch":true,"body":"The config parser previously threw a hard error if 'submodule.active'\nwas provided without a value, causing commands to abort.\n\nSwap repo_config_get_string_multi() to repo_config_get_value_multi()\nto parse valueless keys safely. Use the standard config_error_nonbool()\nhelper to emit a warning to the user rather than crashing.\n\nThis resolves a NEEDSWORK comment in submodule.c.\n\nSigned-off-by: tilak-raaz <raaztilak07@gmail.com>\n---\nJunio, thank you for the review and guidance on the terminology and tense.\nRegarding causing the command to fail on a malformed config: I investigated returning an error code here, but is_tree_submodule_active() is evaluated as a boolean predicate by its callers (for example, if (!is_tree_submodule_active(...))). Since -1 is truthy in C, returning -1 would cause callers to treat the broken submodule as active.\nTo avoid changing the existing caller semantics or introducing process termination from this helper, I kept the continue behavior so the malformed entry is skipped after being reported with config_error_nonbool(), while valid entries continue to be processed.\nPlease let me know if you would prefer a different error-propagation approach.\nChanges in v3:\n-Updated the commit message to use present tense.\n-Updated the commit message to use \"valueless true\".\n-Fixed the whitespace and indentation in the t7400 test script.\n\n submodule.c                | 12 ++++++------\n t/t7400-submodule-basic.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5c92575888..07d1fc63e9 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -231,11 +231,7 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n /*\n  * Determine if a submodule has been initialized at a given 'path'\n  */\n-/*\n- * NEEDSWORK: Emit a warning if submodule.active exists, but is valueless,\n- * ie, the config looks like: \"[submodule] active\\n\".\n- * Since that is an invalid pathspec, we should inform the user.\n- */\n+\n int is_tree_submodule_active(struct repository *repo,\n \t\t\t     const struct object_id *treeish_name,\n \t\t\t     const char *path)\n@@ -261,12 +257,16 @@ int is_tree_submodule_active(struct repository *repo,\n \tfree(key);\n \n \t/* submodule.active is set */\n-\tif (!repo_config_get_string_multi(repo, \"submodule.active\", &sl)) {\n+\tif (!repo_config_get_value_multi(repo, \"submodule.active\", &sl)) {\n \t\tstruct pathspec ps;\n \t\tstruct strvec args = STRVEC_INIT;\n \t\tconst struct string_list_item *item;\n \n \t\tfor_each_string_list_item(item, sl) {\n+\t\t\t if (!item->string) {\n+\t\t\t\tconfig_error_nonbool(\"submodule.active\");\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tstrvec_push(&args, item->string);\n \t\t}\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex eefdecb0bd..62b2537174 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1549,4 +1549,15 @@ test_expect_success 'submodule add fails when name is reused' '\n \t)\n '\n \n+\n+test_expect_success 'warn on valueless submodule.active' '\n+\ttest_when_finished \"rm -rf empty-active\" &&\n+\tgit init empty-active &&\n+\ttest_commit -C empty-active initial &&\n+\tgit -c protocol.file.allow=always -C empty-active submodule add ../empty-active sub &&\n+\tgit -C empty-active config --unset submodule.sub.active &&\n+\tprintf \"[submodule]\\n\\tactive\\n\" >>empty-active/.git/config &&\n+\tgit -C empty-active submodule status 2>err &&\n+\tgrep \"missing value for .submodule.active.\" err\n+'\n test_done\n-- \n2.50.1 (Apple Git-155)\n\n"},{"id":"550659","messageId":"20260815071829.22190-1-raaztilak07@gmail.com","threadId":"66179","inReplyTo":"MESSAGE_ID_FROM_JUNIOS_LAST_EMAIL","subject":"[PATCH v4] submodule: warn on valueless active config","fromName":"tilak-raaz","fromEmail":"raaztilak07@gmail.com","sentAt":"2026-08-15T07:18:29Z","receivedAt":"2026-08-15T07:18:39Z","isPatch":true,"body":"The config parser throws a hard error if 'submodule.active'\nis provided without a value, causing commands to abort.\n\nSwap repo_config_get_string_multi() to repo_config_get_value_multi()\nto parse valueless true safely. Use the standard config_error_nonbool()\nhelper to emit a warning to the user rather than crashing.\n\nThis resolves a NEEDSWORK comment in submodule.c.\n\nSigned-off-by: tilak-raaz <raaztilak07@gmail.com>\n---\nJunio, thank you for the review and guidance on the terminology and tense.\n\n(Apologies for the noisy v3; I botched my --amend and accidentally left the commit message in the past tense. This v4 corrects the commit message.)\n\nRegarding causing the command to fail on a malformed config: I investigated returning an error code here, but is_tree_submodule_active() is evaluated as a boolean predicate by its callers (for example, if (!is_tree_submodule_active(...))). Since -1 is truthy in C, returning -1 would cause callers to treat the broken submodule as active.\n\nTo avoid changing the existing caller semantics or introducing process termination from this helper, I kept the continue behavior so the malformed entry is skipped after being reported with config_error_nonbool(), while valid entries continue to be processed.\n\nPlease let me know if you would prefer a different error-propagation approach.\n\nChanges in v4:\n- Updated the commit message to use present tense (fixing the omission in v3).\n- Updated the commit message to use \"valueless true\".\n- Fixed the whitespace and indentation in the t7400 test script.\n\n submodule.c                | 12 ++++++------\n t/t7400-submodule-basic.sh | 11 +++++++++++\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 5c92575888..07d1fc63e9 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -231,11 +231,7 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n /*\n  * Determine if a submodule has been initialized at a given 'path'\n  */\n-/*\n- * NEEDSWORK: Emit a warning if submodule.active exists, but is valueless,\n- * ie, the config looks like: \"[submodule] active\\n\".\n- * Since that is an invalid pathspec, we should inform the user.\n- */\n+\n int is_tree_submodule_active(struct repository *repo,\n \t\t\t     const struct object_id *treeish_name,\n \t\t\t     const char *path)\n@@ -261,12 +257,16 @@ int is_tree_submodule_active(struct repository *repo,\n \tfree(key);\n \n \t/* submodule.active is set */\n-\tif (!repo_config_get_string_multi(repo, \"submodule.active\", &sl)) {\n+\tif (!repo_config_get_value_multi(repo, \"submodule.active\", &sl)) {\n \t\tstruct pathspec ps;\n \t\tstruct strvec args = STRVEC_INIT;\n \t\tconst struct string_list_item *item;\n \n \t\tfor_each_string_list_item(item, sl) {\n+\t\t\t if (!item->string) {\n+\t\t\t\tconfig_error_nonbool(\"submodule.active\");\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tstrvec_push(&args, item->string);\n \t\t}\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex eefdecb0bd..62b2537174 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1549,4 +1549,15 @@ test_expect_success 'submodule add fails when name is reused' '\n \t)\n '\n \n+\n+test_expect_success 'warn on valueless submodule.active' '\n+\ttest_when_finished \"rm -rf empty-active\" &&\n+\tgit init empty-active &&\n+\ttest_commit -C empty-active initial &&\n+\tgit -c protocol.file.allow=always -C empty-active submodule add ../empty-active sub &&\n+\tgit -C empty-active config --unset submodule.sub.active &&\n+\tprintf \"[submodule]\\n\\tactive\\n\" >>empty-active/.git/config &&\n+\tgit -C empty-active submodule status 2>err &&\n+\tgrep \"missing value for .submodule.active.\" err\n+'\n test_done\n-- \n2.50.1 (Apple Git-155)\n\n"},{"id":"550714","messageId":"xmqqjypo4p89.fsf@gitster.g","threadId":"66179","inReplyTo":"20260815071829.22190-1-raaztilak07@gmail.com","subject":"Re: [PATCH v4] submodule: warn on valueless active config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-17T17:43:34Z","receivedAt":"2026-08-17T17:43:37Z","isPatch":true,"body":"tilak-raaz <raaztilak07@gmail.com> writes:\n\n> The config parser throws a hard error if 'submodule.active'\n> is provided without a value, causing commands to abort.\n\nWhy is it a bad thing in the first place?\n\n    $ echo \"[submodule] config\" >>.git/config\n    $ git submodule\n    error: missing value for 'submodule.active'\n\nIs this error message not sufficient for users to go on finding\nwhere their configuration file is broken and fixing it?\n\n>  \t/* submodule.active is set */\n> -\tif (!repo_config_get_string_multi(repo, \"submodule.active\", &sl)) {\n> +\tif (!repo_config_get_value_multi(repo, \"submodule.active\", &sl)) {\n>  \t\tstruct pathspec ps;\n>  \t\tstruct strvec args = STRVEC_INIT;\n>  \t\tconst struct string_list_item *item;\n>  \n>  \t\tfor_each_string_list_item(item, sl) {\n> +\t\t\t if (!item->string) {\n> +\t\t\t\tconfig_error_nonbool(\"submodule.active\");\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n\nWarning and continuing as if no misconfigured variable existed?  I\ndo not think it is an improvement.  Without stopping the process,\nthe early error messages will just scroll away without giving the\nchance for the user to notice.\n\ntilak-raaz <raaztilak07@gmail.com> writes:\n\n> The config parser throws a hard error if 'submodule.active'\n> is provided without a value, causing commands to abort.\n>\n> Swap repo_config_get_string_multi() to repo_config_get_value_multi()\n> to parse valueless true safely. Use the standard config_error_nonbool()\n> helper to emit a warning to the user rather than crashing.\n>\n> This resolves a NEEDSWORK comment in submodule.c.\n\nNEEDSWORK is different from TODO in that whoever addresses it must\nthink if what the comment suggests to do is sensible in the first\nplace.  I do not think it is in this case.  IOW, unlike TODO, there\nare two valid ways to resolve NEEDSWORK, (1) analyze the issue and\nvalidate that the suggested change is sensible, and then adjust the\ncode to match, or (2) analyze the issue and determine that the\nsuggested change is not a good idea, and then remove (or update) the\ncomment.\n\n> Signed-off-by: tilak-raaz <raaztilak07@gmail.com>\n\nDocumentation/SubmittingPatches::[real-name]???\n\n\n> (Apologies for the noisy v3; I botched my --amend and accidentally left the commit message in the past tense. This v4 corrects the commit message.)\n>\n> Regarding causing the command to fail on a malformed config: I investigated returning an error code here, but is_tree_submodule_active() is evaluated as a boolean predicate by its callers (for example, if (!is_tree_submodule_active(...))). Since -1 is truthy in C, returning -1 would cause callers to treat the broken submodule as active.\n>\n> To avoid changing the existing caller semantics or introducing process termination from this helper, I kept the continue behavior so the malformed entry is skipped after being reported with config_error_nonbool(), while valid entries continue to be processed.\n\nAll overly long lines.  Wrap them ~70 columns.\n\nThanks.\n"}]}