{"thread":{"id":"54663","subject":"[PATCH v3] help.c: configurable suggestions","startedAt":"2020-11-18T14:42:27Z","lastAt":"2020-11-18T18:27:55Z","messageCount":5,"participants":["Drew DeVault","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"410229","messageId":"20201118144218.23233-1-sir@cmpwn.com","threadId":"54663","inReplyTo":null,"subject":"[PATCH v3] help.c: configurable suggestions","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-11-18T14:42:18Z","receivedAt":"2020-11-18T14:42:27Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"This allows users to disable guessing the commands or options that they\nmeant to use.\n\nSigned-off-by: Drew DeVault <sir@cmpwn.com>\n---\nThanks for being patient with me, Junio - I didn't entirely grok what\nyou were getting at in your initial review.\n\n Documentation/config/help.txt | 14 +++++++-------\n help.c                        | 33 +++++++++++++++++++++++++++++----\n 2 files changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config/help.txt b/Documentation/config/help.txt\nindex 224bbf5a28..dea236268a 100644\n--- a/Documentation/config/help.txt\n+++ b/Documentation/config/help.txt\n@@ -8,13 +8,13 @@ help.format::\n \tthe default. 'web' and 'html' are the same.\n \n help.autoCorrect::\n-\tAutomatically correct and execute mistyped commands after\n-\twaiting for the given number of deciseconds (0.1 sec). If more\n-\tthan one command can be deduced from the entered text, nothing\n-\twill be executed.  If the value of this option is negative,\n-\tthe corrected command will be executed immediately. If the\n-\tvalue is 0 - the command will be just shown but not executed.\n-\tThis is the default.\n+\tIf git detects typos and can identify exactly one valid command similar\n+\tto the error, git will automatically run the intended command after\n+\twaiting a duration of time defined by this configuration value in\n+\tdeciseconds (0.1 sec).  If this value is 0, the suggested corrections\n+\twill be shown, but not executed. If \"immediate\", the suggested command\n+\tis run immediately. If \"never\", suggestions are not shown at all. The\n+\tdefault value is zero.\n \n help.htmlPath::\n \tSpecify the path where the HTML documentation resides. File system paths\ndiff --git a/help.c b/help.c\nindex 919cbb9206..61a7c1ea17 100644\n--- a/help.c\n+++ b/help.c\n@@ -472,12 +472,26 @@ int is_in_cmdlist(struct cmdnames *c, const char *s)\n static int autocorrect;\n static struct cmdnames aliases;\n \n+#define AUTOCORRECT_NEVER (-2)\n+#define AUTOCORRECT_IMMEDIATELY (-1)\n+\n static int git_unknown_cmd_config(const char *var, const char *value, void *cb)\n {\n \tconst char *p;\n \n-\tif (!strcmp(var, \"help.autocorrect\"))\n-\t\tautocorrect = git_config_int(var,value);\n+\tif (!strcmp(var, \"help.autocorrect\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tif (!strcmp(value, \"never\")) {\n+\t\t\tautocorrect = AUTOCORRECT_NEVER;\n+\t\t} else if (!strcmp(value, \"immediate\")) {\n+\t\t\tautocorrect = AUTOCORRECT_IMMEDIATELY;\n+\t\t} else {\n+\t\t\tint v = git_config_int(var, value);\n+\t\t\tautocorrect = (v < 0)\n+\t\t\t\t? AUTOCORRECT_IMMEDIATELY : v;\n+\t\t}\n+\t}\n \t/* Also use aliases for command lookup */\n \tif (skip_prefix(var, \"alias.\", &p))\n \t\tadd_cmdname(&aliases, p, strlen(p));\n@@ -525,6 +539,11 @@ const char *help_unknown_cmd(const char *cmd)\n \n \tread_early_config(git_unknown_cmd_config, NULL);\n \n+\tif (autocorrect == AUTOCORRECT_NEVER) {\n+\t\tfprintf_ln(stderr, _(\"git: '%s' is not a git command. See 'git --help'.\"), cmd);\n+\t\texit(1);\n+\t}\n+\n \tload_command_list(\"git-\", &main_cmds, &other_cmds);\n \n \tadd_cmd_list(&main_cmds, &aliases);\n@@ -594,7 +613,7 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t\t   _(\"WARNING: You called a Git command named '%s', \"\n \t\t\t     \"which does not exist.\"),\n \t\t\t   cmd);\n-\t\tif (autocorrect < 0)\n+\t\tif (autocorrect == AUTOCORRECT_IMMEDIATELY)\n \t\t\tfprintf_ln(stderr,\n \t\t\t\t   _(\"Continuing under the assumption that \"\n \t\t\t\t     \"you meant '%s'.\"),\n@@ -706,10 +725,16 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n \t\t\t       const char *error)\n {\n \tint i;\n-\tstruct string_list suggested_refs = guess_refs(ref);\n+\tstruct string_list suggested_refs;\n \n \tfprintf_ln(stderr, _(\"%s: %s - %s\"), cmd, ref, error);\n \n+\tif (autocorrect == AUTOCORRECT_NEVER) {\n+\t\texit(1);\n+\t}\n+\n+\tsuggested_refs = guess_refs(ref);\n+\n \tif (suggested_refs.nr > 0) {\n \t\tfprintf_ln(stderr,\n \t\t\t   Q_(\"\\nDid you mean this?\",\n-- \n2.29.2\n\n"},{"id":"410241","messageId":"xmqqima237qt.fsf@gitster.c.googlers.com","threadId":"54663","inReplyTo":"20201118144218.23233-1-sir@cmpwn.com","subject":"Re: [PATCH v3] help.c: configurable suggestions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-18T17:16:58Z","receivedAt":"2020-11-18T17:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Drew DeVault <sir@cmpwn.com> writes:\n\n> This allows users to disable guessing the commands or options that they\n> meant to use.\n\nIt is unclear from this description alone why this is needed.  The\nseller of this change needs to emphasize how this is better than\nsetting the variable to \"0\" (do not autocorrect).  My guess is that\nsome users do not even need the suggestion of correct spelling when\nthey made a typo?\n\n>  help.autoCorrect::\n> -\tAutomatically correct and execute mistyped commands after\n> -\twaiting for the given number of deciseconds (0.1 sec). If more\n> -\tthan one command can be deduced from the entered text, nothing\n> -\twill be executed.  If the value of this option is negative,\n> -\tthe corrected command will be executed immediately. If the\n> -\tvalue is 0 - the command will be just shown but not executed.\n> -\tThis is the default.\n> +\tIf git detects typos and can identify exactly one valid command similar\n> +\tto the error, git will automatically run the intended command after\n> +\twaiting a duration of time defined by this configuration value in\n> +\tdeciseconds (0.1 sec).  If this value is 0, the suggested corrections\n> +\twill be shown, but not executed. If \"immediate\", the suggested command\n> +\tis run immediately. If \"never\", suggestions are not shown at all. The\n> +\tdefault value is zero.\n\nImagine existing users who set the variable to -1 long time ago\nwonders what it is doing in their ~/.gitconfig file.  This paragraph\nstill needs to describe how a negative value is treated.\n\n> diff --git a/help.c b/help.c\n> index 919cbb9206..61a7c1ea17 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -472,12 +472,26 @@ int is_in_cmdlist(struct cmdnames *c, const char *s)\n>  static int autocorrect;\n>  static struct cmdnames aliases;\n>  \n> +#define AUTOCORRECT_NEVER (-2)\n> +#define AUTOCORRECT_IMMEDIATELY (-1)\n> +\n>  static int git_unknown_cmd_config(const char *var, const char *value, void *cb)\n>  {\n>  \tconst char *p;\n>  \n> -\tif (!strcmp(var, \"help.autocorrect\"))\n> -\t\tautocorrect = git_config_int(var,value);\n> +\tif (!strcmp(var, \"help.autocorrect\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n> +\t\tif (!strcmp(value, \"never\")) {\n> +\t\t\tautocorrect = AUTOCORRECT_NEVER;\n> +\t\t} else if (!strcmp(value, \"immediate\")) {\n> +\t\t\tautocorrect = AUTOCORRECT_IMMEDIATELY;\n> +\t\t} else {\n> +\t\t\tint v = git_config_int(var, value);\n> +\t\t\tautocorrect = (v < 0)\n> +\t\t\t\t? AUTOCORRECT_IMMEDIATELY : v;\n> +\t\t}\n> +\t}\n>  \t/* Also use aliases for command lookup */\n>  \tif (skip_prefix(var, \"alias.\", &p))\n>  \t\tadd_cmdname(&aliases, p, strlen(p));\n> @@ -525,6 +539,11 @@ const char *help_unknown_cmd(const char *cmd)\n>  \n>  \tread_early_config(git_unknown_cmd_config, NULL);\n>  \n> +\tif (autocorrect == AUTOCORRECT_NEVER) {\n> +\t\tfprintf_ln(stderr, _(\"git: '%s' is not a git command. See 'git --help'.\"), cmd);\n> +\t\texit(1);\n\nOK, so when we encounter an unknown word that follows \"git\" on the\ncomand line, we immediately exit without doing the guesswork.  This\nis needed because we are skipping not just the guessing and\nsuggesting, but showing this error message by exiting early here.\n\nMakes sense.\n\n> +\t}\n> +\n>  \tload_command_list(\"git-\", &main_cmds, &other_cmds);\n>  \n>  \tadd_cmd_list(&main_cmds, &aliases);\n> @@ -594,7 +613,7 @@ const char *help_unknown_cmd(const char *cmd)\n>  \t\t\t   _(\"WARNING: You called a Git command named '%s', \"\n>  \t\t\t     \"which does not exist.\"),\n>  \t\t\t   cmd);\n> -\t\tif (autocorrect < 0)\n> +\t\tif (autocorrect == AUTOCORRECT_IMMEDIATELY)\n>  \t\t\tfprintf_ln(stderr,\n>  \t\t\t\t   _(\"Continuing under the assumption that \"\n>  \t\t\t\t     \"you meant '%s'.\"),\n> @@ -706,10 +725,16 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n>  \t\t\t       const char *error)\n>  {\n>  \tint i;\n> -\tstruct string_list suggested_refs = guess_refs(ref);\n> +\tstruct string_list suggested_refs;\n>  \n>  \tfprintf_ln(stderr, _(\"%s: %s - %s\"), cmd, ref, error);\n>  \n> +\tif (autocorrect == AUTOCORRECT_NEVER) {\n> +\t\texit(1);\n> +\t}\n\nI am not sure how the change in this hunk is justifiable.\n\nThe auto correction is about guessing mistyped commands and\noptionally going forward to execute it.  Some users (like me) who\nset it to 0 currently may want to set it to \"never\", but I'd imagine\nthat not all of these users would want to lose the local branch vs\nremote-tracking branch correction (certainly not me).  It might be\nconvenient for some folks if both of these can be squelched with a\nsingle knob, but it must be possible to enable them independently.\n\nThis is a tangent, but it seems to me that help_unknown_ref() is way\nunder-used and lacks usefulness too much---probably these contribute\nto each other.\n\nIt only gets used by \"git merge $name\" and only when the missing\n$name branch exists as remote-tracking branches of some remotes,\ni.e.  \"you said $name, which does not exist, but you could have\ntyped $remote/$name to refer to the remote-tracking branch\").\n\nI would imagine that it would help users who type \"git checkout\nmext\" to suggest \"you may have typoed 'next'\", for example, and \"git\ncheckout -b my-topic next\" to offer \"you said 'next' that does not\nexist; did you mean 'origin/next'?\"  It might be a good exercise to\nfirst make inventory of other possible places that would benefit,\nwhich may suggest what other sources guess_refs() should grab its\ncandidates from (#leftoverbits).\n\n> +\n> +\tsuggested_refs = guess_refs(ref);\n> +\n>  \tif (suggested_refs.nr > 0) {\n>  \t\tfprintf_ln(stderr,\n>  \t\t\t   Q_(\"\\nDid you mean this?\",\n"},{"id":"410242","messageId":"C76K6Q09ATLR.1OI5KW390LKFX@taiga","threadId":"54663","inReplyTo":"xmqqima237qt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] help.c: configurable suggestions","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-11-18T17:17:43Z","receivedAt":"2020-11-18T17:21:43Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Wed Nov 18, 2020 at 12:16 PM EST, Junio C Hamano wrote:\n> It is unclear from this description alone why this is needed. The\n> seller of this change needs to emphasize how this is better than\n> setting the variable to \"0\" (do not autocorrect). My guess is that\n> some users do not even need the suggestion of correct spelling when\n> they made a typo?\n\nThis is one reason. Another is that spell checking can be\ncomputationally expensive, and take an annoyingly long time on low-end\ndevices.\n\n> > @@ -706,10 +725,16 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n> >  \t\t\t       const char *error)\n> >  {\n> >  \tint i;\n> > -\tstruct string_list suggested_refs = guess_refs(ref);\n> > +\tstruct string_list suggested_refs;\n> >  \n> >  \tfprintf_ln(stderr, _(\"%s: %s - %s\"), cmd, ref, error);\n> >  \n> > +\tif (autocorrect == AUTOCORRECT_NEVER) {\n> > +\t\texit(1);\n> > +\t}\n>\n> I am not sure how the change in this hunk is justifiable.\n\nI think this was caught up in the move from a new config option to\nchanging the meaning of help.autocorrect.\n\n> This is a tangent, but it seems to me that help_unknown_ref() is way\n> under-used and lacks usefulness too much---probably these contribute\n> to each other.\n\nGiven that this is under-utilized anyway, and no longer applicable to\nthe essential purpose of the patch, I'm just going to pull this change\nout of the patch. If this function starts to enjoy broader usage within\ngit, we could revisit a config lever later on.\n\nThanks for the feedback, sending v4 shortly.\n"},{"id":"410246","messageId":"xmqq8say36e5.fsf@gitster.c.googlers.com","threadId":"54663","inReplyTo":"C76K6Q09ATLR.1OI5KW390LKFX@taiga","subject":"Re: [PATCH v3] help.c: configurable suggestions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-18T17:46:10Z","receivedAt":"2020-11-18T17:47:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Drew DeVault\" <sir@cmpwn.com> writes:\n\n> On Wed Nov 18, 2020 at 12:16 PM EST, Junio C Hamano wrote:\n>> It is unclear from this description alone why this is needed. The\n>> seller of this change needs to emphasize how this is better than\n>> setting the variable to \"0\" (do not autocorrect). My guess is that\n>> some users do not even need the suggestion of correct spelling when\n>> they made a typo?\n>\n> This is one reason. Another is that spell checking can be\n> computationally expensive, and take an annoyingly long time on low-end\n> devices.\n\nBoth are good reasons to describe in the log message, so that the\nnext reader of \"git log\" won't have to ask \"why?\"\n\n\n"},{"id":"410254","messageId":"877dqir04a.fsf@evledraar.gmail.com","threadId":"54663","inReplyTo":"C76K6Q09ATLR.1OI5KW390LKFX@taiga","subject":"Re: [PATCH v3] help.c: configurable suggestions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-18T18:27:49Z","receivedAt":"2020-11-18T18:27:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 18 2020, Drew DeVault wrote:\n\n> On Wed Nov 18, 2020 at 12:16 PM EST, Junio C Hamano wrote:\n>> It is unclear from this description alone why this is needed. The\n>> seller of this change needs to emphasize how this is better than\n>> setting the variable to \"0\" (do not autocorrect). My guess is that\n>> some users do not even need the suggestion of correct spelling when\n>> they made a typo?\n>\n> This is one reason. Another is that spell checking can be\n> computationally expensive, and take an annoyingly long time on low-end\n> devices.\n\nAs an aside it would be interesting if you're in a position to test it\nwhether it's the suggestion code itself that's so expensive, or e.g. the\nstat()-ing on \"git xyz\" looking for \"git-xyz\" in $PATH. I if it's\nFS-limited that there's easy optimization opportunities there.\n"}]}