| Message ID | pull.1869.v2.git.git.1736419777235.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] help: interpret boolean string values for help.autocorrect | expand |
"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?
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
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
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
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 ;-)
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 */