Hey,
On Thu, Jan 9, 2025 at 5:32 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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.
> 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
