Re: [PATCH 2/5] help: refactor command autocorrection handling

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

Jiamu Sun <39@barroit.sh> writes:

> Refactor autocorrect config into struct autocorr, with an explicit mode
> enum and a separate delay field. Move config parsing, TTY validation,
> and prompt handling into autocorrect.c.
>
> This simplifies help.c and makes state handling for autocorrect explicit
> and easier to maintain.
>
> Signed-off-by: Jiamu Sun <39@barroit.sh>
> ---
>  Makefile      |   1 +
>  autocorrect.c |  92 +++++++++++++++++++++++++++++++++++++++++++
>  autocorrect.h |  23 +++++++++++
>  help.c        | 106 ++++++++++----------------------------------------
>  4 files changed, 136 insertions(+), 86 deletions(-)
>  create mode 100644 autocorrect.c
>  create mode 100644 autocorrect.h
>
> diff --git a/Makefile b/Makefile
> index f3264d0a37cc..6111631c2caa 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1098,6 +1098,7 @@ LIB_OBJS += archive-tar.o
>  LIB_OBJS += archive-zip.o
>  LIB_OBJS += archive.o
>  LIB_OBJS += attr.o
> +LIB_OBJS += autocorrect.o
>  LIB_OBJS += base85.o
>  LIB_OBJS += bisect.o
>  LIB_OBJS += blame.o
> diff --git a/autocorrect.c b/autocorrect.c
> new file mode 100644
> index 000000000000..eaae01645910
> --- /dev/null
> +++ b/autocorrect.c
> @@ -0,0 +1,92 @@
> +#define USE_THE_REPOSITORY_VARIABLE
> +
> +#include "git-compat-util.h"
> +#include "autocorrect.h"
> +#include "config.h"
> +#include "parse.h"
> +#include "strbuf.h"
> +#include "prompt.h"
> +#include "gettext.h"
> +
> +static enum autocorr_mode parse_autocorr(const char *value)
> +{
> +	switch (git_parse_maybe_bool_text(value)) {
> +		case 1:
> +			return AUTOCORR_IMMEDIATELY;
> +		case 0:
> +			return AUTOCORR_HINTONLY;
> +		default: /* other random text */
> +			break;
> +	}
> +
> +	if (!strcmp(value, "prompt"))
> +		return AUTOCORR_PROMPT;
> +	else if (!strcmp(value, "never"))
> +		return AUTOCORR_NEVER;
> +	else if (!strcmp(value, "immediate"))
> +		return AUTOCORR_IMMEDIATELY;
> +	else if (!strcmp(value, "show"))
> +		return AUTOCORR_HINTONLY;
> +	else
> +		return AUTOCORR_DELAY;
> +}

You snuck in unnecessary "style fixes" to make bunch of "if()return"
into if/else if cascade.  Also what was AUTOCORRECT_SHOW is now
returned as AUTOCORR_HINTONLY.  There is no explanation on the
reason behind these changes in the proposed log message, and hiding
these small changes in a code movement patch makes reviewing the
series harder than necessary.

The patch is doing too many things (namely, (1) code movement that
will make it later reusable as a side effect but has no semantic
changes in the current code, plus (2) change in style (like the one
we see here), semantics (possibly the difference in SHOW and
HINTONLY we see here) and features, possibly including the renaming
of AUTOCORRECT_* into AUTOCORR_*.)  Let's have "restructure with
code movement and nothing else", followed by "other changes

I'll stop here, and expect this step to be split into at least two
patches to make it more readable before we can review it again.

Thanks.