From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 10 Mar 2026 14:06:17 GMT
Subject: Re: [PATCH v3 3/8] help: move tty check for autocorrection to autocorrect.c
Message-ID: <CAOLa=ZS=2msGg5d-6-LgVD6XSduYze46iWcT2X_dJDd=6AOOkA@mail.gmail.com>
In-Reply-To: <SY0P300MB0801E98F538F5320E7B663F0CE46A@SY0P300MB0801.AUSP300.PROD.OUTLOOK.COM>

Jiamu Sun <39@barroit.sh> writes:

> TTY checking is the autocorrect config parser's responsibility. It must
> ensure the parsed value is correct and reliable. Thus, move the check to
> autocorr_resolve_config().
>
> Signed-off-by: Jiamu Sun <39@barroit.sh>
> ---
> In parse_autocorrect() I kept the if/else if cascade. In my opinion,
> this is not a style change. It's a control-flow clarification, because
> those returns describe different states of the mode. Chaining them
> together is better than leaving them discrete.
>
> Also, I dropped the float cast in the delay calculation, which is
> redundant.
>
>  autocorrect.c | 50 +++++++++++++++++++++++++++++---------------------
>  autocorrect.h | 20 ++++++++++++++------
>  help.c        | 17 ++++++-----------
>  3 files changed, 49 insertions(+), 38 deletions(-)
>
> diff --git a/autocorrect.c b/autocorrect.c
> index 1037f032019b..50d7f116d85d 100644
> --- a/autocorrect.c
> +++ b/autocorrect.c
> @@ -6,7 +6,7 @@
>  #include "prompt.h"
>  #include "gettext.h"
>
> -static int parse_autocorrect(const char *value)
> +static enum autocorr_mode parse_autocorrect(const char *value)
>  {
>  	switch (git_parse_maybe_bool_text(value)) {
>  		case 1:
> @@ -19,41 +19,49 @@ static int parse_autocorrect(const char *value)
>
>  	if (!strcmp(value, "prompt"))
>  		return AUTOCORRECT_PROMPT;
> -	if (!strcmp(value, "never"))
> +	else if (!strcmp(value, "never"))
>  		return AUTOCORRECT_NEVER;
> -	if (!strcmp(value, "immediate"))
> +	else if (!strcmp(value, "immediate"))
>  		return AUTOCORRECT_IMMEDIATELY;
> -	if (!strcmp(value, "show"))
> +	else if (!strcmp(value, "show"))
>  		return AUTOCORRECT_SHOW;
> -
> -	return 0;
> +	else
> +		return AUTOCORRECT_DELAY;
>  }
>

Okay so since we introduce an enum we use that here.

>  void autocorr_resolve_config(const char *var, const char *value,
>  			     const struct config_context *ctx, void *data)
>  {
> -	int *out = data;
> +	struct autocorr *conf = data;
> +
> +	if (strcmp(var, "help.autocorrect"))
> +		return;
> +
> +	conf->mode = parse_autocorrect(value);
>
> -	if (!strcmp(var, "help.autocorrect")) {
> -		int v = parse_autocorrect(value);
> +	/*
> +	 * Disable autocorrection prompt in a non-interactive session.
> +	 */
> +	if (conf->mode == AUTOCORRECT_PROMPT && (!isatty(0) || !isatty(2)))
> +		conf->mode = AUTOCORRECT_NEVER;
>
> -		if (!v) {
> -			v = git_config_int(var, value, ctx->kvi);
> -			if (v < 0 || v == 1)
> -				v = AUTOCORRECT_IMMEDIATELY;
> -		}
> +	if (conf->mode == AUTOCORRECT_DELAY) {
> +		conf->delay = git_config_int(var, value, ctx->kvi);
>
> -		*out = v;
> +		if (!conf->delay)
> +			conf->mode = AUTOCORRECT_SHOW;
> +		else if (conf->delay <= 1)
> +			conf->mode = AUTOCORRECT_IMMEDIATELY;
>  	}
>  }
>
> -void autocorr_confirm(int autocorrect, const char *assumed)
> +void autocorr_confirm(struct autocorr *conf, const char *assumed)
>  {
> -	if (autocorrect == AUTOCORRECT_IMMEDIATELY) {
> +	if (conf->mode == AUTOCORRECT_IMMEDIATELY) {
>  		fprintf_ln(stderr,
>  			   _("Continuing under the assumption that you meant '%s'."),
>  			   assumed);
> -	} else if (autocorrect == AUTOCORRECT_PROMPT) {
> +	} else if (conf->mode == AUTOCORRECT_PROMPT) {
>  		char *answer;
>  		struct strbuf msg = STRBUF_INIT;
>
> @@ -63,10 +71,10 @@ void autocorr_confirm(int autocorrect, const char *assumed)
>
>  		if (!(starts_with(answer, "y") || starts_with(answer, "Y")))
>  			exit(1);
> -	} else {
> +	} else if (conf->mode == AUTOCORRECT_DELAY) {
>  		fprintf_ln(stderr,
>  			   _("Continuing in %0.1f seconds, assuming that you meant '%s'."),
> -			   (float)autocorrect / 10.0, assumed);
> -		sleep_millisec(autocorrect * 100);
> +			   conf->delay / 10.0, assumed);
> +		sleep_millisec(conf->delay * 100);
>  	}
>  }
> diff --git a/autocorrect.h b/autocorrect.h
> index 45609990c77e..ce4a68379f2f 100644
> --- a/autocorrect.h
> +++ b/autocorrect.h
> @@ -1,16 +1,24 @@
>  #ifndef AUTOCORRECT_H
>  #define AUTOCORRECT_H
>
> -#define AUTOCORRECT_SHOW (-4)
> -#define AUTOCORRECT_PROMPT (-3)
> -#define AUTOCORRECT_NEVER (-2)
> -#define AUTOCORRECT_IMMEDIATELY (-1)
> -
>  struct config_context;
>
> +enum autocorr_mode {
> +	AUTOCORRECT_SHOW,
> +	AUTOCORRECT_NEVER,
> +	AUTOCORRECT_PROMPT,
> +	AUTOCORRECT_IMMEDIATELY,
> +	AUTOCORRECT_DELAY,
> +};
> +
> +struct autocorr {
> +	enum autocorr_mode mode;
> +	int delay;
> +};
> +

I would say the naming doesn't indicate what it is used for. How about
'autocorrect_config'?

[snip]