Re: [PATCH v4] submodule: warn on valueless active config

tilak-raaz <[email protected]> writes:

> The config parser throws a hard error if 'submodule.active'
> is provided without a value, causing commands to abort.

Why is it a bad thing in the first place?

    $ echo "[submodule] config" >>.git/config
    $ git submodule
    error: missing value for 'submodule.active'

Is this error message not sufficient for users to go on finding
where their configuration file is broken and fixing it?

>  	/* 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;
> +			}

Warning and continuing as if no misconfigured variable existed?  I
do not think it is an improvement.  Without stopping the process,
the early error messages will just scroll away without giving the
chance for the user to notice.

tilak-raaz <[email protected]> writes:

> The config parser throws a hard error if 'submodule.active'
> is provided without a value, causing commands to abort.
>
> Swap repo_config_get_string_multi() to repo_config_get_value_multi()
> to parse valueless true safely. Use the standard config_error_nonbool()
> helper to emit a warning to the user rather than crashing.
>
> This resolves a NEEDSWORK comment in submodule.c.

NEEDSWORK is different from TODO in that whoever addresses it must
think if what the comment suggests to do is sensible in the first
place.  I do not think it is in this case.  IOW, unlike TODO, there
are two valid ways to resolve NEEDSWORK, (1) analyze the issue and
validate that the suggested change is sensible, and then adjust the
code to match, or (2) analyze the issue and determine that the
suggested change is not a good idea, and then remove (or update) the
comment.

> Signed-off-by: tilak-raaz <[email protected]>

Documentation/SubmittingPatches::[real-name]???


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

All overly long lines.  Wrap them ~70 columns.

Thanks.