threads / patch / 62770

patchhelp: interpret help.autocorrect=1 as "immediate" rather than 0.1s

Subject: [PATCH] help: interpret help.autocorrect=1 as "immediate" rather than 0.1s

## tl;dr

24 messages between Jan 8, 2025 and Feb 4, 2025. Diffs are folded; open one to read it.

replies: 23people: 8as markdown or json

Scott Chacon via GitGitGadget· Jan 8, 2025, 19:31 UTC · lore
From: Scott Chacon <schacon@gmail.com>

Many people confusingly set the "help.autocorrect" setting to 1 believing it to be a boolean that turns on the autocorrect feature rather than an integer value of deciseconds wait time. Since it's impossible for a human being to react this quickly, the help message stating that it's waiting for 0.1s before continuing becomes confusingly comical.

This patch simply interprets a "1" value as the same as the "immedate" autocorrect setting, which makes it skip the 0.1s and simply say that it's running the command, which is almost certainly what everyone setting it to that value is actually trying to do.

Signed-off-by: Scott Chacon <schacon@gmail.com>
---
    help: interpret help.autocorrect=1 as "immediate" rather than 0.1s
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1869%2Fschacon%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1869/schacon/master-v1
Pull-Request: https://github.com/git/git/pull/1869
 help.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to help.c +1 −1
diff --git a/help.c b/help.c
index 5483ea8fd29..e6576644b99 100644
--- a/help.c
+++ b/help.c
@@ -568,7 +568,7 @@ static int git_unknown_cmd_config(const char *var, const char *value,
 			return config_error_nonbool(var);
 		if (!strcmp(value, "never")) {
 			cfg->autocorrect = AUTOCORRECT_NEVER;
-		} else if (!strcmp(value, "immediate")) {
+		} else if (!strcmp(value, "immediate") || !strcmp(value, "1")) {
 			cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
 		} else if (!strcmp(value, "prompt")) {
 			cfg->autocorrect = AUTOCORRECT_PROMPT;

base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e
-- 
gitgitgadget
Kristoffer Haugsbakk· Jan 8, 2025, 21:42 UTC · re: Scott Chacon via GitGitGadget · lore

Re: [PATCH] help: interpret help.autocorrect=1 as "immediate" rather than 0.1s

On Wed, Jan 8, 2025, at 20:31, Scott Chacon via GitGitGadget wrote:
Show 10 quoted lines
> From: Scott Chacon <schacon@gmail.com>
>
> Many people confusingly set the "help.autocorrect" setting to 1 believing it
> to be a boolean that turns on the autocorrect feature rather than an integer
> value of deciseconds wait time. Since it's impossible for a human being to
> react this quickly, the help message stating that it's waiting for 0.1s
> before continuing becomes confusingly comical.
>
> This patch simply interprets a "1" value as the same as the "immedate"
> autocorrect setting, which makes it skip the 0.1s and simply say that it's
Maybe: s/This patch simply interprets a/Interpret a "1"/
From “imperative-mood” section in SubmittingPatches.
Or: Interpret "1" as "immediate"
Since the sentence is getting a bit complex with “as the same as the”.
> running the command, which is almost certainly what everyone setting it to
> that value is actually trying to do.
The section in `man git config` should get an update I think.
Show 32 quoted lines
>
> Signed-off-by: Scott Chacon <schacon@gmail.com>
> ---
>     help: interpret help.autocorrect=1 as "immediate" rather than 0.1s
>
> Published-As:
> https://github.com/gitgitgadget/git/releases/tag/pr-git-1869%2Fschacon%2Fmaster-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> pr-git-1869/schacon/master-v1
> Pull-Request: https://github.com/git/git/pull/1869
>
>  help.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/help.c b/help.c
> index 5483ea8fd29..e6576644b99 100644
> --- a/help.c
> +++ b/help.c
> @@ -568,7 +568,7 @@ static int git_unknown_cmd_config(const char *var,
> const char *value,
>  			return config_error_nonbool(var);
>  		if (!strcmp(value, "never")) {
>  			cfg->autocorrect = AUTOCORRECT_NEVER;
> -		} else if (!strcmp(value, "immediate")) {
> +		} else if (!strcmp(value, "immediate") || !strcmp(value, "1")) {
>  			cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
>  		} else if (!strcmp(value, "prompt")) {
>  			cfg->autocorrect = AUTOCORRECT_PROMPT;
>
> base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e
> --
> gitgitgadget
Johannes Schindelin· Jan 9, 2025, 00:18 UTC · re: Scott Chacon via GitGitGadget · lore

Re: [PATCH] help: interpret help.autocorrect=1 as "immediate" rather than 0.1s

Hi Scott,
On Wed, 8 Jan 2025, Scott Chacon via GitGitGadget wrote:
Show 12 quoted lines
> From: Scott Chacon <schacon@gmail.com>
>
> Many people confusingly set the "help.autocorrect" setting to 1 believing it
> to be a boolean that turns on the autocorrect feature rather than an integer
> value of deciseconds wait time. Since it's impossible for a human being to
> react this quickly, the help message stating that it's waiting for 0.1s
> before continuing becomes confusingly comical.
>
> This patch simply interprets a "1" value as the same as the "immedate"
> autocorrect setting, which makes it skip the 0.1s and simply say that it's
> running the command, which is almost certainly what everyone setting it to
> that value is actually trying to do.

Not trying to brag but I had no problems understanding this commit message as-is.

Show 21 quoted lines
> Signed-off-by: Scott Chacon <schacon@gmail.com>
> ---
>     help: interpret help.autocorrect=1 as "immediate" rather than 0.1s
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1869%2Fschacon%2Fmaster-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1869/schacon/master-v1
> Pull-Request: https://github.com/git/git/pull/1869
>
>  help.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/help.c b/help.c
> index 5483ea8fd29..e6576644b99 100644
> --- a/help.c
> +++ b/help.c
> @@ -568,7 +568,7 @@ static int git_unknown_cmd_config(const char *var, const char *value,
>  			return config_error_nonbool(var);
>  		if (!strcmp(value, "never")) {
>  			cfg->autocorrect = AUTOCORRECT_NEVER;
> -		} else if (!strcmp(value, "immediate")) {
> +		} else if (!strcmp(value, "immediate") || !strcmp(value, "1")) {
Makes sense to me!

For the record, I do think it was a mistake to treat number values as "deciseconds" here, it is inconsistent with pretty much any other config setting. But I also don't see any way to remediate this design mistake at this stage.

Thank you for working on this and making the feature at least a little bit more usable.

Ciao, Johannes

Show 9 quoted lines
>  			cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
>  		} else if (!strcmp(value, "prompt")) {
>  			cfg->autocorrect = AUTOCORRECT_PROMPT;
>
> base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e
> --
> gitgitgadget
>
>
Junio C Hamano· Jan 9, 2025, 01:12 UTC · re: Scott Chacon via GitGitGadget · lore

Re: [PATCH] help: interpret help.autocorrect=1 as "immediate" rather than 0.1s

"Scott Chacon via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This patch simply interprets a "1" value as the same as the "immedate"
> autocorrect setting, which makes it skip the 0.1s and simply say that it's
> running the command, which is almost certainly what everyone setting it to
> that value is actually trying to do.

It is a cute hack, but special casing a string that is a single letter "1" in a value that can take a number smells somewhat bad to me X-<. If we were redoing this from the start, we would probably pick a better name for the variable (with "delay" somewhere in the name), but that is water under the bridge.

I however wonder if we should allow people to have their cake and eat it too. It currently says it is *not* a boolean, and manually interpret "never" and other things, ...

		if (!value)
Show 8 quoted lines
>  			return config_error_nonbool(var);
>  		if (!strcmp(value, "never")) {
>  			cfg->autocorrect = AUTOCORRECT_NEVER;
> -		} else if (!strcmp(value, "immediate")) {
> +		} else if (!strcmp(value, "immediate") || !strcmp(value, "1")) {
>  			cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
>  		} else if (!strcmp(value, "prompt")) {
>  			cfg->autocorrect = AUTOCORRECT_PROMPT;
... but would it be simpler if we made it an extended boolean, i.e.
    true, yes, on, 1  -> same as "immediate"
    false, no, off, 0 -> same as "never"
    immediate         -> same as what we currently do
    never             -> same as what we currently do
    prompt            -> same as what we currently do
    number            -> same as what we currently do

It would kill many birds with a stone (e.g., help.autocorrect=no does not work in the current system as anybody would expect, but it would with the "this is an extended boolean" approach).

I dunno.
Thanks.
Yongmin· Jan 9, 2025, 07:05 UTC · re: Scott Chacon via GitGitGadget · lore

Re: [PATCH] help: interpret help.autocorrect=1 as "immediate" rather than 0.1s

On 2025-01-09 (Thu) 04:31:46+09:00, Scott Chacon via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 8 quoted lines
> From: Scott Chacon <schacon@gmail.com>
>
> [snip]
>
> This patch simply interprets a "1" value as the same as the "immedate"
> autocorrect setting, which makes it skip the 0.1s and simply say that it's
> running the command, which is almost certainly what everyone setting it to
> that value is actually trying to do.
I think Kristoffer somewhat mentioned this but…
s/immedate/immediate/
-- 
----
revi | 레비 (IPA: lɛbi)
- 홍용민
- https://revi.xyz
- he/him <https://revi.xyz/pronoun-is/>
- What time is it in my timezone? <https://revi.kr/time>
- OpenPGP <https://revi.xyz/pgp/>
- In this Korean name <https://revi.kr/ng3ul59>, the family name is Hong 
<https://revi.kr/617ZTqb>,
   which makes my name HONG Yongmin.
- I reply when my time permits. Don't feel pressured to reply ASAP;
   take your time and respond at your schedule.
Scott Chacon via GitGitGadget· Jan 9, 2025, 10:49 UTC · re: Scott Chacon via GitGitGadget · lore

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

From: Scott Chacon <schacon@gmail.com>

A help.autocorrect value of 1 is currently interpreted as "wait 1 decisecond", which can be confusing to users who believe they are setting a boolean value to turn the autocorrect feature on.

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.

False boolean string values ("off", "false", "no") are now equivalent to 0, meaning that guessed values are still shown but nothing is executed (as opposed to "never", which does not show the guesses). True boolean string values are interpreted as "immediate".

Signed-off-by: Scott Chacon <schacon@gmail.com>
---
    help: interpret help.autocorrect=1 as "immediate" rather than 0.1s
    
    Took Junio's suggestion to include all boolean values as valid, though
    I'm not interpreting "false" as "never", but instead as 0, as they're
    subtly different. 0 will show the guessed commands and exit, "never"
    will not guess the commands.
    
    Changes since v1:
    
     * Include all boolean values rather than special casing "1"
     * Update the help.txt documentation
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1869%2Fschacon%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1869/schacon/master-v2
Pull-Request: https://github.com/git/git/pull/1869
Range-diff vs v1:
 1:  dbda79cd4fc < -:  ----------- help: interpret help.autocorrect=1 as "immediate" rather than 0.1s
 -:  ----------- > 1:  07b47b70ded help: interpret boolean string values for help.autocorrect
 Documentation/config/help.txt |  5 +++--
 help.c                        | 18 +++++++++++++++---
 2 files changed, 18 insertions(+), 5 deletions(-)
Show changes to 2 files +18 −5

Documentation/config/help.txt, help.c

diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt
index 610701f9a37..6d9c2e06908 100644
--- a/Documentation/config/help.txt
+++ b/Documentation/config/help.txt
@@ -11,8 +11,9 @@ 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, false boolean string: show the suggested command (default).
+	 - 1, true boolean string: 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
diff --git a/help.c b/help.c
index 5483ea8fd29..9e0f66c26dc 100644
--- a/help.c
+++ b/help.c
@@ -573,9 +573,21 @@ static int git_unknown_cmd_config(const char *var, const char *value,
 		} 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 is_bool;
+			int v = git_config_bool_or_int(var, value, ctx->kvi, &is_bool);
+			if (is_bool) {
+				if (v == 0) {
+					cfg->autocorrect = 0;
+				} else {
+					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
+				}
+			} else {
+				if (v < 0 || v == 1) {
+					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
+				} else {
+					cfg->autocorrect = v;
+				}
+			}
 		}
 	}
 	/* Also use aliases for command lookup */

base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e
-- 
gitgitgadget
Junio C Hamano· Jan 9, 2025, 16:32 UTC · re: Scott Chacon via GitGitGadget · lore

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

"Scott Chacon via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 16 quoted lines
> diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt
> index 610701f9a37..6d9c2e06908 100644
> --- a/Documentation/config/help.txt
> +++ b/Documentation/config/help.txt
> @@ -11,8 +11,9 @@ 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, false boolean string: show the suggested command (default).
> +	 - 1, true boolean string: 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

Not a problem this patch introduces, but it looed funny to see the second line abut to the left edge of the page there.

In any case, the updated semantics look quite sensible.
Show 28 quoted lines
> diff --git a/help.c b/help.c
> index 5483ea8fd29..9e0f66c26dc 100644
> --- a/help.c
> +++ b/help.c
> @@ -573,9 +573,21 @@ static int git_unknown_cmd_config(const char *var, const char *value,
>  		} 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 is_bool;
> +			int v = git_config_bool_or_int(var, value, ctx->kvi, &is_bool);
> +			if (is_bool) {
> +				if (v == 0) {
> +					cfg->autocorrect = 0;
> +				} else {
> +					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
> +				}
> +			} else {
> +				if (v < 0 || v == 1) {
> +					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
> +				} else {
> +					cfg->autocorrect = v;
> +				}
> +			}
>  		}
>  	}

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.

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;
	}
perhaps?
Scott Chacon· Jan 10, 2025, 07:43 UTC · re: Junio C Hamano · lore

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

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
Scott Chacon· Jan 10, 2025, 09:30 UTC · re: Scott Chacon · lore

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

On Fri, Jan 10, 2025 at 8:43 AM Scott Chacon <schacon@gmail.com> wrote:
Show 30 quoted lines
>
> Hey,
>
> 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?

Scott
Jeff King· Jan 10, 2025, 12:11 UTC · re: Scott Chacon · lore

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

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
Junio C Hamano· Jan 10, 2025, 15:02 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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.
Exactly.

Thanks for filling the blank in for me while I was away from the keyboard ;-)

Scott Chacon via GitGitGadget· Jan 11, 2025, 11:27 UTC · re: Scott Chacon via GitGitGadget · lore

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

From: Scott Chacon <schacon@gmail.com>

A help.autocorrect value of 1 is currently interpreted as "wait 1 decisecond", which can be confusing to users who believe they are setting a boolean value to turn the autocorrect feature on.

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.

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

Signed-off-by: Scott Chacon <schacon@gmail.com>
---
    help: interpret boolean string values for help.autocorrect
    
    Basically just using Junio's suggested code from the ML, splitting most
    of the logic out into a parse_autocorrect method and then special casing
    a 1 integer as "immediate". I reverted to interpreting false boolean
    values as NEVER rather than 0, which means they no longer show guesses,
    which the last patch did.
    
    Changes since v2:
    
     * split out most logic into parse_autocorrect
     * interpret false boolean values as NEVER rather than 0
     * Update the help.txt documentation
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1869%2Fschacon%2Fmaster-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1869/schacon/master-v3
Pull-Request: https://github.com/git/git/pull/1869
Range-diff vs v2:
 1:  07b47b70ded ! 1:  4ce7652d19e help: interpret boolean string values for help.autocorrect
     @@ Commit message
          but as a true boolean, the equivalent of "immediate". If the value is 2 or
          more, continue treating it as a decisecond wait time.
      
     -    False boolean string values ("off", "false", "no") are now equivalent to 0,
     -    meaning that guessed values are still shown but nothing is executed (as
     -    opposed to "never", which does not show the guesses). True boolean string
     -    values are interpreted as "immediate".
     +    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".
      
          Signed-off-by: Scott Chacon <schacon@gmail.com>
      
     @@ Documentation/config/help.txt: help.autoCorrect::
       	run the suggestion automatically. Possible config values are:
      -	 - 0 (default): show the suggested command.
      -	 - positive number: run the suggested command after specified
     -+	 - 0, false boolean string: show the suggested command (default).
     -+	 - 1, true boolean string: run the suggested command immediately.
     ++	 - 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
      
       ## help.c ##
     +@@ help.c: 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)
      @@ help.c: static int git_unknown_cmd_config(const char *var, const char *value,
     - 		} else if (!strcmp(value, "prompt")) {
     - 			cfg->autocorrect = AUTOCORRECT_PROMPT;
     - 		} else {
     + 	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 is_bool;
     -+			int v = git_config_bool_or_int(var, value, ctx->kvi, &is_bool);
     -+			if (is_bool) {
     -+				if (v == 0) {
     -+					cfg->autocorrect = 0;
     -+				} else {
     -+					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
     -+				}
     -+			} else {
     -+				if (v < 0 || v == 1) {
     -+					cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
     -+				} else {
     -+					cfg->autocorrect = 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));
 Documentation/config/help.txt |  7 +++---
 help.c                        | 42 +++++++++++++++++++++++++----------
 2 files changed, 34 insertions(+), 15 deletions(-)
Show changes to 2 files +34 −15

Documentation/config/help.txt, help.c

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

base-commit: fbe8d3079d4a96aeb4e4529cc93cc0043b759a05
-- 
gitgitgadget
Jeff King· Jan 13, 2025, 05:43 UTC · re: Scott Chacon via GitGitGadget · lore

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

On Sat, Jan 11, 2025 at 11:27:19AM +0000, Scott Chacon via GitGitGadget wrote:
Show 5 quoted lines
> 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".

Show 19 quoted lines
> 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?

Show 18 quoted lines
> 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.

Show 9 quoted lines
> +	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".

Show 27 quoted lines
> @@ -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
Scott Chacon· Jan 13, 2025, 09:31 UTC · re: Jeff King · lore

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

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

Scott Chacon via GitGitGadget· Jan 13, 2025, 09:33 UTC · re: Scott Chacon via GitGitGadget · lore

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

From: Scott Chacon <schacon@gmail.com>

A help.autocorrect value of 1 is currently interpreted as "wait 1 decisecond", which can be confusing to users who believe they are setting a boolean value to turn the autocorrect feature on.

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.

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

Signed-off-by: Scott Chacon <schacon@gmail.com>
---
    help: interpret boolean string values for help.autocorrect
    
    Just updating the docs with Peff's suggestion.
    
    Changes since v3:
    
     * docs update to group "immediate" in with the true bools
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1869%2Fschacon%2Fmaster-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1869/schacon/master-v4
Pull-Request: https://github.com/git/git/pull/1869
Range-diff vs v3:
 1:  4ce7652d19e ! 1:  64482b5249b help: interpret boolean string values for help.autocorrect
     @@ Documentation/config/help.txt: help.autoCorrect::
      -	 - 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.
     ++	 - 1, "true", "on", "yes", "immediate": run the suggested command
     ++immediately.
      +	 - positive number > 1: run the suggested command after specified
       deciseconds (0.1 sec).
     - 	 - "immediate": run the suggested command immediately.
     +-	 - "immediate": run the suggested command immediately.
     ++	 - "false", "off", "no", "never": don't run or show any suggested command.
       	 - "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
 Documentation/config/help.txt |  9 ++++----
 help.c                        | 42 +++++++++++++++++++++++++----------
 2 files changed, 35 insertions(+), 16 deletions(-)
Show changes to 2 files +35 −16

Documentation/config/help.txt, help.c

diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt
index 610701f9a37..a4c6079af81 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", "immediate": run the suggested command
+immediately.
+	 - positive number > 1: run the suggested command after specified
 deciseconds (0.1 sec).
-	 - "immediate": run the suggested command immediately.
+	 - "false", "off", "no", "never": don't run or show any suggested command.
 	 - "prompt": show the suggestion and prompt for confirmation to run
 the command.
-	 - "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));

base-commit: fbe8d3079d4a96aeb4e4529cc93cc0043b759a05
-- 
gitgitgadget
Junio C Hamano· Jan 13, 2025, 16:18 UTC · re: Jeff King · lore

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

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

Taylor Blau· Jan 13, 2025, 23:33 UTC · re: Johannes Schindelin · lore

Re: [PATCH] help: interpret help.autocorrect=1 as "immediate" rather than 0.1s

On Thu, Jan 09, 2025 at 01:18:15AM +0100, Johannes Schindelin wrote:
> For the record, I do think it was a mistake to treat number values as
> "deciseconds" here, it is inconsistent with pretty much any other config
> setting. But I also don't see any way to remediate this design mistake at
> this stage.

I almost made this same mistake when working on pseudo-merge bitmaps, in particular with the non-integral configuration options like:

  - bitampPseudoMerge.<name>.decay
  - bitampPseudoMerge.<name>.sampleRate

If memory serves, I think this mostly had to do with the lack of a double parser in the config system. I ended up adding one in 5831f8ac41 (config: introduce `git_config_double()`, 2024-05-23), and made those configuration options take values like '0.1', etc.

I think it may be worth considering what "starting from scratch" would look like, as Junio suggested above. To be clear, I think that that should happen outside of the current patch and not hold it up, as what Scott is proposing is a strict improvement.

But it may be worth thinking about what a different interface might look like. If we settle on something we like, perhaps we could start nudging users towards it and "deprecate" the existing syntax.

> Thank you for working on this and making the feature at least a little bit
> more usable.
I concur.

Thanks, Taylor

Junio C Hamano· Jan 18, 2025, 01:12 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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).

Now this left the patch in a stuck state, with the latest round v4 still having the "0 and false mean different things" caused by mistake. We should do the "0 and usual 'false' all show but don't run, say 'never' if you want it to be absolute no-op", which is more natural.

If I find time, I can update further based on v4, but no promises.
Thanks.
David Aguilar· Feb 1, 2025, 21:33 UTC · re: Scott Chacon via GitGitGadget · lore

[PATCH 1/2] help: show the suggested command when help.autocorrect is false

Make the handling of false boolean values for help.autocorrect consistent with the handling of value 0 by showing the suggested commands but not running them.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is based on the sc/help-autocorrect-one patches from this thread
and is in response to the open question from "What's coooking in git.git":
> On Fri, 31 Jan 2025 18:51:33 -0800 Junio C Hamano <gitster@pobox.com> wrote:
> Looking good except for "should 0 and false be 'tell it without doing it'?".
source: <xmqq5xlu4bt6.fsf@gitster.g>
This is what it would look like if the answer were to be, "yes".
 Documentation/config/help.txt |  4 ++--
 help.c                        |  6 ++++--
 t/t9003-help-autocorrect.sh   | 17 ++++++++++-------
 3 files changed, 16 insertions(+), 11 deletions(-)
Show changes to 3 files +16 −11

Documentation/config/help.txt, help.c, t/t9003-help-autocorrect.sh

diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt
index a4c6079af8..676ba3a55f 100644
--- a/Documentation/config/help.txt
+++ b/Documentation/config/help.txt
@@ -11,12 +11,12 @@ 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: show the suggested command (default).
+	 - 0, "false", "off", "no": show the suggested command (default).
 	 - 1, "true", "on", "yes", "immediate": run the suggested command
 immediately.
 	 - positive number > 1: run the suggested command after specified
 deciseconds (0.1 sec).
-	 - "false", "off", "no", "never": don't run or show any suggested command.
+	 - "never": don't run or show any suggested command.
 	 - "prompt": show the suggestion and prompt for confirmation to run
 the command.
 
diff --git a/help.c b/help.c
index 7148963e46..55425c0d97 100644
--- a/help.c
+++ b/help.c
@@ -552,6 +552,7 @@ struct help_unknown_cmd_config {
 	struct cmdnames aliases;
 };
 
+#define AUTOCORRECT_SHOW (-4)
 #define AUTOCORRECT_PROMPT (-3)
 #define AUTOCORRECT_NEVER (-2)
 #define AUTOCORRECT_IMMEDIATELY (-1)
@@ -562,7 +563,7 @@ static int parse_autocorrect(const char *value)
 		case 1:
 			return AUTOCORRECT_IMMEDIATELY;
 		case 0:
-			return AUTOCORRECT_NEVER;
+			return AUTOCORRECT_SHOW;
 		default: /* other random text */
 			break;
 	}
@@ -713,7 +714,8 @@ char *help_unknown_cmd(const char *cmd)
 		     n++)
 			; /* still counting */
 	}
-	if (cfg.autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) {
+	if (cfg.autocorrect && cfg.autocorrect != AUTOCORRECT_SHOW && n == 1 &&
+	    SIMILAR_ENOUGH(best_similarity)) {
 		char *assumed = xstrdup(main_cmds.names[0]->name);
 
 		fprintf_ln(stderr,
diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
index 85a5074b5e..5ebfc00f52 100755
--- a/t/t9003-help-autocorrect.sh
+++ b/t/t9003-help-autocorrect.sh
@@ -28,15 +28,18 @@ test_expect_success 'setup' '
 	test_cmp expect actual
 '
 
-test_expect_success 'autocorrect showing candidates' '
-	git config help.autocorrect 0 &&
+for show in false no off 0
+do
+	test_expect_success 'autocorrect showing candidates' '
+		git config help.autocorrect $show &&
 
-	test_must_fail git lfg 2>actual &&
-	grep "^	lgf" actual &&
+		test_must_fail git lfg 2>actual &&
+		grep "^	lgf" actual &&
 
-	test_must_fail git distimdist 2>actual &&
-	grep "^	distimdistim" actual
-'
+		test_must_fail git distimdist 2>actual &&
+		grep "^	distimdistim" actual
+	'
+done
 
 for immediate in -1 immediate
 do
-- 
2.48.0.rc2.34.gefa3f50b25
David Aguilar· Feb 1, 2025, 21:33 UTC · re: David Aguilar · lore

[PATCH 2/2] help: add "show" as a valid configuration value

Add a literal value for showing the suggested autocorrection for consistency with the rest of the help.autocorrect options.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is just for consistency with the other config values and
can be dropped if it's not useful.
 Documentation/config/help.txt | 2 +-
 help.c                        | 2 ++
 t/t9003-help-autocorrect.sh   | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)
Show changes to 3 files +4 −2

Documentation/config/help.txt, help.c, t/t9003-help-autocorrect.sh

diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt
index 676ba3a55f..b369589cec 100644
--- a/Documentation/config/help.txt
+++ b/Documentation/config/help.txt
@@ -11,7 +11,7 @@ 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, "false", "off", "no": show the suggested command (default).
+	 - 0, "false", "off", "no", "show": show the suggested command (default).
 	 - 1, "true", "on", "yes", "immediate": run the suggested command
 immediately.
 	 - positive number > 1: run the suggested command after specified
diff --git a/help.c b/help.c
index 55425c0d97..8d91afe851 100644
--- a/help.c
+++ b/help.c
@@ -574,6 +574,8 @@ static int parse_autocorrect(const char *value)
 		return AUTOCORRECT_NEVER;
 	if (!strcmp(value, "immediate"))
 		return AUTOCORRECT_IMMEDIATELY;
+	if (!strcmp(value, "show"))
+		return AUTOCORRECT_SHOW;
 
 	return 0;
 }
diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
index 5ebfc00f52..8da318d2b5 100755
--- a/t/t9003-help-autocorrect.sh
+++ b/t/t9003-help-autocorrect.sh
@@ -28,7 +28,7 @@ test_expect_success 'setup' '
 	test_cmp expect actual
 '
 
-for show in false no off 0
+for show in false no off 0 show
 do
 	test_expect_success 'autocorrect showing candidates' '
 		git config help.autocorrect $show &&
-- 
2.48.0.rc2.34.gefa3f50b25
Junio C Hamano· Feb 3, 2025, 22:53 UTC · re: David Aguilar · lore

Re: [PATCH 1/2] help: show the suggested command when help.autocorrect is false

David Aguilar <davvid@gmail.com> writes:
Show 16 quoted lines
> Make the handling of false boolean values for help.autocorrect
> consistent with the handling of value 0 by showing the suggested
> commands but not running them.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This is based on the sc/help-autocorrect-one patches from this thread
> and is in response to the open question from "What's coooking in git.git":
>
>> On Fri, 31 Jan 2025 18:51:33 -0800 Junio C Hamano <gitster@pobox.com> wrote:
>> Looking good except for "should 0 and false be 'tell it without doing it'?".
>
> source: <xmqq5xlu4bt6.fsf@gitster.g>
>
> This is what it would look like if the answer were to be, "yes".

I obviously like the updated semantics myself. Thanks for updating it.

Let's see what others think.
Thanks.
Junio C Hamano· Feb 3, 2025, 22:53 UTC · re: David Aguilar · lore

Re: [PATCH 2/2] help: add "show" as a valid configuration value

David Aguilar <davvid@gmail.com> writes:
Show 23 quoted lines
> Add a literal value for showing the suggested autocorrection
> for consistency with the rest of the help.autocorrect options.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This is just for consistency with the other config values and
> can be dropped if it's not useful.
>
>  Documentation/config/help.txt | 2 +-
>  help.c                        | 2 ++
>  t/t9003-help-autocorrect.sh   | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt
> index 676ba3a55f..b369589cec 100644
> --- a/Documentation/config/help.txt
> +++ b/Documentation/config/help.txt
> @@ -11,7 +11,7 @@ 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, "false", "off", "no": show the suggested command (default).
> +	 - 0, "false", "off", "no", "show": show the suggested command (default).
Makes sense.
Show 29 quoted lines
>  	 - 1, "true", "on", "yes", "immediate": run the suggested command
>  immediately.
>  	 - positive number > 1: run the suggested command after specified
> diff --git a/help.c b/help.c
> index 55425c0d97..8d91afe851 100644
> --- a/help.c
> +++ b/help.c
> @@ -574,6 +574,8 @@ static int parse_autocorrect(const char *value)
>  		return AUTOCORRECT_NEVER;
>  	if (!strcmp(value, "immediate"))
>  		return AUTOCORRECT_IMMEDIATELY;
> +	if (!strcmp(value, "show"))
> +		return AUTOCORRECT_SHOW;
>  
>  	return 0;
>  }
> diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
> index 5ebfc00f52..8da318d2b5 100755
> --- a/t/t9003-help-autocorrect.sh
> +++ b/t/t9003-help-autocorrect.sh
> @@ -28,7 +28,7 @@ test_expect_success 'setup' '
>  	test_cmp expect actual
>  '
>  
> -for show in false no off 0
> +for show in false no off 0 show
>  do
>  	test_expect_success 'autocorrect showing candidates' '
>  		git config help.autocorrect $show &&
Jeff King· Feb 4, 2025, 03:05 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] help: show the suggested command when help.autocorrect is false

On Mon, Feb 03, 2025 at 02:53:00PM -0800, Junio C Hamano wrote:
Show 23 quoted lines
> David Aguilar <davvid@gmail.com> writes:
> 
> > Make the handling of false boolean values for help.autocorrect
> > consistent with the handling of value 0 by showing the suggested
> > commands but not running them.
> >
> > Suggested-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> > This is based on the sc/help-autocorrect-one patches from this thread
> > and is in response to the open question from "What's coooking in git.git":
> >
> >> On Fri, 31 Jan 2025 18:51:33 -0800 Junio C Hamano <gitster@pobox.com> wrote:
> >> Looking good except for "should 0 and false be 'tell it without doing it'?".
> >
> > source: <xmqq5xlu4bt6.fsf@gitster.g>
> >
> > This is what it would look like if the answer were to be, "yes".
> 
> I obviously like the updated semantics myself.
> Thanks for updating it.
> 
> Let's see what others think.
I like it (including the new "show" which is even more descriptive).
-Peff
Junio C Hamano· Feb 4, 2025, 13:38 UTC · re: Jeff King · lore

Re: [PATCH 1/2] help: show the suggested command when help.autocorrect is false

Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> On Mon, Feb 03, 2025 at 02:53:00PM -0800, Junio C Hamano wrote:
>
>> David Aguilar <davvid@gmail.com> writes:
>> 
>> > Make the handling of false boolean values for help.autocorrect
>> > consistent with the handling of value 0 by showing the suggested
>> > commands but not running them.
>> >
>> > Suggested-by: Junio C Hamano <gitster@pobox.com>
>> > Signed-off-by: David Aguilar <davvid@gmail.com>
>> > ---
>> > This is based on the sc/help-autocorrect-one patches from this thread
>> > and is in response to the open question from "What's coooking in git.git":
>> >
>> >> On Fri, 31 Jan 2025 18:51:33 -0800 Junio C Hamano <gitster@pobox.com> wrote:
>> >> Looking good except for "should 0 and false be 'tell it without doing it'?".
>> >
>> > source: <xmqq5xlu4bt6.fsf@gitster.g>
>> >
>> > This is what it would look like if the answer were to be, "yes".
>> 
>> I obviously like the updated semantics myself.
>> Thanks for updating it.
>> 
>> Let's see what others think.
>
> I like it (including the new "show" which is even more descriptive).
Thanks, really appreciate it.

← back to recent threads