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

From: Scott Chacon <schacon@gmail.com>
Date: Jan 10, 2025, 07:43 UTC
Message-ID: <CAP2yMaJzHM+AyDwjfVNJrDeJt0iip5h_ZxgNJL3V-gVSOjoBWQ@mail.gmail.com>
In-Reply-To: <xmqq5xmoj6cz.fsf@gitster.g>

Hey,

On Thu, Jan 9, 2025 at 5:32 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

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

I'm not super familiar with this codebase, honestly, but ifaict this
is not what this does. That top block makes sure that value isn't
null, which I can't figure out how it would ever be - I've tried a
bunch of different config values, but I'm not sure it's possible to do
- and if so it just prints "missing value for help.autocorrect" (the
nonbool part of that function is something of a misnomer, it appears).
But again, I can't see how those two lines aren't essentially a no-op.

Show 34 quoted lines

> 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;
>         }

I _can_ do this, but it seems somewhat more complicated and I believe it would have the same end result, no?

Also, in thinking about this a bit more, while I updated the patch with the suggestion to make it accept all boolean text values rather than the "1" hack, it should be kept in mind that if someone does do this, that config setting will be backwards incompatible with previous Git versions in a way that will have a fatal error if it encounters a string boolean value when a command is mistyped. Maybe that's not super horrible, but I'm honestly not sure that accepting more boolean string values is helpful - it's been 17 years of this feature and I doubt that many people have tried to set it to 'on' or probably would in the future.

Anyhow, I'm happy to redo this patch in the manner suggested, but personally I think the first simple DWIM hack is a realistically better solution.

Scott