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

From: Jeff King <peff@peff.net>
Date: Jan 10, 2025, 12:11 UTC
Message-ID: <20250110121100.GE1014503@coredump.intra.peff.net>
In-Reply-To: <CAP2yMa+5ca22tNMc4qu=yBVd9t74uNnLFbKE3_=EcA5_goM6zw@mail.gmail.com>

On Fri, Jan 10, 2025 at 10:30:12AM +0100, Scott Chacon wrote:

Show 37 quoted lines

> > On Thu, Jan 9, 2025 at 5:32 PM Junio C Hamano <gitster@pobox.com> 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.
> 
> Ah, I see. You can leave off the `=` and that will trigger this error.
> Though it seems to simultaneously be seen as a configuration error.
> 
>   ❯ ./git test
>   error: missing value for 'help.autocorrect'
>   fatal: bad config line 19 in file .git/config
> 
> But if that's the only way it seems to trigger this code path, to
> essentially have a corrupted config file, does it matter?

It's not corrupted; that syntax is allowed for boolean variables[1]. The "bad config line" is due to the early "return config_error_nonbool(var)" quoted above. It is passing the error back to the general config code, which then just prints the "bad config" line.

I think what Junio is saying is that if we are going to turn this into an option which accepts bool values, it should accept this special syntax, too. And that first "if (!value)" has to either go away (and get replace by a maybe_bool() call, as mentioned earlier) or has to set AUTOCORRECT_IMMEDIATELY itself.

-Peff

[1] There's a similar syntax for the "-c" option, which can make testing
    easier:

      git -c help.autocorrect foo