| Message ID | pull.1869.v3.git.git.1736594839527.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] help: interpret boolean string values for help.autocorrect | expand |
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". > diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt > index 610701f9a37..16b124b1c17 100644 > --- a/Documentation/config/help.txt > +++ b/Documentation/config/help.txt > @@ -11,13 +11,14 @@ help.autoCorrect:: > If git detects typos and can identify exactly one valid command similar > to the error, git will try to suggest the correct command or even > run the suggestion automatically. Possible config values are: > - - 0 (default): show the suggested command. > - - positive number: run the suggested command after specified > + - 0: show the suggested command (default). > + - 1, "true", "on", "yes": run the suggested command immediately. > + - positive number > 1: run the suggested command after specified > deciseconds (0.1 sec). > - "immediate": run the suggested command immediately. > - "prompt": show the suggestion and prompt for confirmation to run > the command. > - - "never": don't run or show any suggested command. > + - "false", "off", "no", "never": don't run or show any suggested command. "never" gets folded into the list of other false booleans. But "immediate" still gets its own bullet point. Should it be folded into the "true" line? > diff --git a/help.c b/help.c > index 5483ea8fd29..7148963e468 100644 > --- a/help.c > +++ b/help.c > @@ -556,6 +556,27 @@ struct help_unknown_cmd_config { > #define AUTOCORRECT_NEVER (-2) > #define AUTOCORRECT_IMMEDIATELY (-1) > > +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; > + } One of the reasons I looked so closely at the "0" behavior above is that I thought the maybe_bool() parser might eat your "0" before you get a chance to act on it. But because you use maybe_bool_text(), it doesn't do any integer interpretation at all. Good. > + if (!strcmp(value, "prompt")) > + return AUTOCORRECT_PROMPT; > + if (!strcmp(value, "never")) > + return AUTOCORRECT_NEVER; > + if (!strcmp(value, "immediate")) > + return AUTOCORRECT_IMMEDIATELY; > + > + return 0; > +} And these all make sense. I wondered if we might ever mistake this 0 return for AUTOCORRECT_*, but they are all defined with non-zero values (which makes sense, since we store them in the same variable that might hold a "0" or positive value). And in fact it would make my bool alternative suggestion above trickier, since this function could return a true "0" to mean "show but don't run". > @@ -564,20 +585,17 @@ static int git_unknown_cmd_config(const char *var, const char *value, > const char *p; > > if (!strcmp(var, "help.autocorrect")) { > - 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")) { > - cfg->autocorrect = AUTOCORRECT_PROMPT; > - } else { > - int v = git_config_int(var, value, ctx->kvi); > - cfg->autocorrect = (v < 0) > - ? AUTOCORRECT_IMMEDIATELY : v; > + int v = parse_autocorrect(value); > + > + if (!v) { > + v = git_config_int(var, value, ctx->kvi); > + if (v < 0 || v == 1) > + v = AUTOCORRECT_IMMEDIATELY; > } > + > + cfg->autocorrect = v; > } > + 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. -Peff
On Mon, Jan 13, 2025 at 6:43 AM Jeff King <peff@peff.net> wrote: > > 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. > 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
Jeff King <peff@peff.net> writes: > 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". That's my fault. Your version makes perfect sense. Thanks for being extra careful (well, more careful than myself, that is).
diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt index 610701f9a37..16b124b1c17 100644 --- a/Documentation/config/help.txt +++ b/Documentation/config/help.txt @@ -11,13 +11,14 @@ help.autoCorrect:: If git detects typos and can identify exactly one valid command similar to the error, git will try to suggest the correct command or even run the suggestion automatically. Possible config values are: - - 0 (default): show the suggested command. - - positive number: run the suggested command after specified + - 0: show the suggested command (default). + - 1, "true", "on", "yes": run the suggested command immediately. + - positive number > 1: run the suggested command after specified deciseconds (0.1 sec). - "immediate": run the suggested command immediately. - "prompt": show the suggestion and prompt for confirmation to run the command. - - "never": don't run or show any suggested command. + - "false", "off", "no", "never": don't run or show any suggested command. help.htmlPath:: Specify the path where the HTML documentation resides. File system paths diff --git a/help.c b/help.c index 5483ea8fd29..7148963e468 100644 --- a/help.c +++ b/help.c @@ -556,6 +556,27 @@ struct help_unknown_cmd_config { #define AUTOCORRECT_NEVER (-2) #define AUTOCORRECT_IMMEDIATELY (-1) +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, "never")) + return AUTOCORRECT_NEVER; + if (!strcmp(value, "immediate")) + return AUTOCORRECT_IMMEDIATELY; + + return 0; +} + static int git_unknown_cmd_config(const char *var, const char *value, const struct config_context *ctx, void *cb) @@ -564,20 +585,17 @@ static int git_unknown_cmd_config(const char *var, const char *value, const char *p; if (!strcmp(var, "help.autocorrect")) { - 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")) { - cfg->autocorrect = AUTOCORRECT_PROMPT; - } else { - int v = git_config_int(var, value, ctx->kvi); - cfg->autocorrect = (v < 0) - ? AUTOCORRECT_IMMEDIATELY : v; + int v = parse_autocorrect(value); + + if (!v) { + v = git_config_int(var, value, ctx->kvi); + if (v < 0 || v == 1) + v = AUTOCORRECT_IMMEDIATELY; } + + cfg->autocorrect = v; } + /* Also use aliases for command lookup */ if (skip_prefix(var, "alias.", &p)) add_cmdname(&cfg->aliases, p, strlen(p));