| Message ID | pull.1869.git.git.1736364707068.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | help: interpret help.autocorrect=1 as "immediate" rather than 0.1s | expand |
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
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 > >
"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.
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/
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;