Re: [PATCH v2] help: interpret boolean string values for help.autocorrect

[Junio C Hamano <gitster@pobox.com>]

"Scott Chacon via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt
> index 610701f9a37..6d9c2e06908 100644
> --- a/Documentation/config/help.txt
> +++ b/Documentation/config/help.txt
> @@ -11,8 +11,9 @@ help.autoCorrect::
>  	If git detects typos and can identify exactly one valid command similar
>  	to the error, git will try to suggest the correct command or even
>  	run the suggestion automatically. Possible config values are:
> -	 - 0 (default): show the suggested command.
> -	 - positive number: run the suggested command after specified
> +	 - 0, false boolean string: show the suggested command (default).
> +	 - 1, true boolean string: run the suggested command immediately.
> +	 - positive number > 1: run the suggested command after specified
>  deciseconds (0.1 sec).
>  	 - "immediate": run the suggested command immediately.
>  	 - "prompt": show the suggestion and prompt for confirmation to run

Not a problem this patch introduces, but it looed funny to see the
second line abut to the left edge of the page there.

In any case, the updated semantics look quite sensible.

> diff --git a/help.c b/help.c
> index 5483ea8fd29..9e0f66c26dc 100644
> --- a/help.c
> +++ b/help.c
> @@ -573,9 +573,21 @@ static int git_unknown_cmd_config(const char *var, const char *value,
>  		} else if (!strcmp(value, "prompt")) {
>  			cfg->autocorrect = AUTOCORRECT_PROMPT;
>  		} else {
> -			int v = git_config_int(var, value, ctx->kvi);
> -			cfg->autocorrect = (v < 0)
> -				? AUTOCORRECT_IMMEDIATELY : v;
> +			int is_bool;
> +			int v = git_config_bool_or_int(var, value, ctx->kvi, &is_bool);
> +			if (is_bool) {
> +				if (v == 0) {
> +					cfg->autocorrect = 0;
> +				} else {
> +					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
> +				}
> +			} else {
> +				if (v < 0 || v == 1) {
> +					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
> +				} else {
> +					cfg->autocorrect = v;
> +				}
> +			}
>  		}
>  	}

The flow looks nice, but the pre-context of this hunk starts like
this:

		if (!value)
			return config_error_nonbool(var);
		if (!strcmp(value, "never")) {
			cfg->autocorrect = AUTOCORRECT_NEVER;
		} else if (!strcmp(value, "immediate")) {
			cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
		} else if (!strcmp(value, "prompt")) {

IOW, the new code added at the end of the if/else if/ cascade is way
too late.  

	"[help] autocorrect"

that specifies "true" has already been rejected as an error, with a
now-stale error message saying that the variable is not a Boolean.

We may probably want to use git_parse_maybe_bool_text() upfront,
like

	static int parse_autocorrect(const char *value)
	{
		switch (git_parse_maybe_bool_text(value)) {
	        case 1:
			return AUTOCORRECT_IMMEDIATELY;
		case 0:
			return AUTOCORRECT_NEVER;
		default: /* other random text */
			break;
		}
                if (!strcmp(value, "prompt"))
			return AUTOCORRECT_PROMPT;
		...
		if (!strcmp(value, "prompt"))
			return AUTOCORRECT_PROMPT;

                return 0;
	}

and then in git_unknown_cmd_config(), do something like

	if (!strcmp(var, "help.autocorrect")) {
		int v = parse_autocorrect(value);

                if (!v) {
                	v = git_config_int(var, value, ctx->kvi);
                        if (v < 0)
				v = AUTOCORRECT_IMMEDIATELY;
                }
                cfg->autocorrect = v;
	}

perhaps?