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

From: Scott Chacon <schacon@gmail.com>
Date: Jan 13, 2025, 09:31 UTC
Message-ID: <CAP2yMa+Upc0i688H5+no3sYbz5X-Ma=KBuaVALpPBdNDHuG8QA@mail.gmail.com>
In-Reply-To: <20250113054355.GB767856@coredump.intra.peff.net>

On Mon, Jan 13, 2025 at 6:43 AM Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

>
> On Sat, Jan 11, 2025 at 11:27:19AM +0000, Scott Chacon via GitGitGadget wrote:
>
> > Interpret the value of help.autocorrect as either one of the accepted list
> > of special values ("never", "immediate", ...), a boolean or an integer. If
> > the value is 1, it is no longer interpreted as a decisecond value of 0.1s
> > but as a true boolean, the equivalent of "immediate". If the value is 2 or
> > more, continue treating it as a decisecond wait time.
>
> This mostly looks good to me, though this part gave me a little pause:
>
> > False boolean string values ("off", "false", "no") are now equivalent to
> > "never", meaning that guessed values are still shown but nothing is
> > executed. True boolean string values are interpreted as "immediate".
>
> I think false boolean values end up as "never", which shows _nothing_.
> As opposed to "0", which continues to be "show but do not execute" (and
> which we can't change if we want to retain historical compatibility).
>
> That's probably OK, though it is a little unlike other bools in that "0"
> is usually a strict synonym for "false". So we could go the other way,
> with "0, false, off, no" meaning "show but don't run" and leaving
> "never" by itself to mean "do nothing".

Yeah, the first patch I sent that interpreted booleans actually did do 0 rather than "never", but Junio continued to suggest that this returns "never" so I did it this way in this latest patch.

I looked for a hot second into how to do this, but the problem is that `parse_autocorrect` can't return 0 because that's the failure mode, so then we would need another constant or some other way to return something that means "don't run, but also show the matches", which right now is only this special value 0.

Honestly, I think "never" is a fine option if someone is actually explicitly setting this to a false string, even if it's _slightly_ inconsistent with the 0 value.

Show 6 quoted lines

> OK, so parse_autocorrect() handles all of the non-numeric values. And
> then we fall back on the integer values. Makes sense.
>
> So assuming we are OK with the "0" vs "false" split, the whole patch
> looks good to me, modulo the nit about folding the "immediate" line in
> the documentation.

OK, sending v4 now with just the docs change.

Thanks, Scott