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

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

[Scott Chacon via GitGitGadget]

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

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

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

[Kristoffer Haugsbakk]

On Wed, Jan 8, 2025, at 20:31, Scott Chacon via GitGitGadget wrote:
> 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.

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

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

[Johannes Schindelin]

Hi Scott,

On Wed, 8 Jan 2025, Scott Chacon via GitGitGadget wrote:

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

> 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

>  			cfg->autocorrect = AUTOCORRECT_IMMEDIATELY;
>  		} else if (!strcmp(value, "prompt")) {
>  			cfg->autocorrect = AUTOCORRECT_PROMPT;
>
> base-commit: 14650065b76b28d3cfa9453356ac5669b19e706e
> --
> gitgitgadget
>
>

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

[Junio C Hamano]

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

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

[Yongmin]

[-- Attachment #1.1.1: Type: text/plain, Size: 974 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3983 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Scott Chacon via GitGitGadget]

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

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

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

[Junio C Hamano]

"Scott Chacon via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

> 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?

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

[Scott Chacon]

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.

> 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

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

[Scott Chacon]

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

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

[Jeff King]

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Scott Chacon via GitGitGadget]

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

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

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

[Jeff King]

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

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

[Scott Chacon]

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

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

[Scott Chacon via GitGitGadget]

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

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

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

[Junio C Hamano]

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

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

[Taylor Blau]

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[David Aguilar]

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

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

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

[David Aguilar]

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

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

[Junio C Hamano]

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.

Thanks.

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

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


>  	 - 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 &&

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

[Jeff King]

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

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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