{"thread":{"id":"54657","subject":"[PATCH v2] help.c: configurable suggestions","startedAt":"2020-11-17T15:26:04Z","lastAt":"2020-11-17T21:26:20Z","messageCount":4,"participants":["Drew DeVault","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"410085","messageId":"20201117152535.9795-1-sir@cmpwn.com","threadId":"54657","inReplyTo":null,"subject":"[PATCH v2] help.c: configurable suggestions","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-11-17T15:25:35Z","receivedAt":"2020-11-17T15:26:04Z","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---\n Documentation/config/help.txt | 14 +++++++-------\n help.c                        | 27 ++++++++++++++++++++++-----\n 2 files changed, 29 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/config/help.txt b/Documentation/config/help.txt\nindex 224bbf5a28..99d1e33750 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 -1, the suggested command is run\n+\timmediately. If -2, suggestions are not shown at all. The default value\n+\tis 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..2fd27cffb0 100644\n--- a/help.c\n+++ b/help.c\n@@ -472,12 +472,18 @@ 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 (!strcasecmp(var, \"help.autocorrect\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tautocorrect = git_config_int(var, value);\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 +531,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@@ -586,7 +597,7 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t     n++)\n \t\t\t; /* still counting */\n \t}\n-\tif (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) {\n+\tif (autocorrect != 0 && n == 1 && SIMILAR_ENOUGH(best_similarity)) {\n \t\tconst char *assumed = main_cmds.names[0]->name;\n \t\tmain_cmds.names[0] = NULL;\n \t\tclean_cmdnames(&main_cmds);\n@@ -594,7 +605,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 +717,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":"410124","messageId":"xmqqd00b698g.fsf@gitster.c.googlers.com","threadId":"54657","inReplyTo":"20201117152535.9795-1-sir@cmpwn.com","subject":"Re: [PATCH v2] help.c: configurable suggestions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-17T20:04:15Z","receivedAt":"2020-11-17T20:04:25Z","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> ---\n\nMissing sign-off.\n\n>  Documentation/config/help.txt | 14 +++++++-------\n>  help.c                        | 27 ++++++++++++++++++++++-----\n>  2 files changed, 29 insertions(+), 12 deletions(-)\n>\n> diff --git a/Documentation/config/help.txt b/Documentation/config/help.txt\n> index 224bbf5a28..99d1e33750 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 -1, the suggested command is run\n> +\timmediately. If -2, suggestions are not shown at all. The default value\n> +\tis zero.\n\nThis changes behaviour for those who have relied on our promise that\nany negative value means immediate execution.  Now -2 means a totally\ndifferent thing.\n\nI thought I already showed you how to make it a \"number or keyword\"\nin my previous review comment to avoid such a needless behaviour\nchange---did our mails cross?\n\nThanks.\n\n>  help.htmlPath::\n>  \tSpecify the path where the HTML documentation resides. File system paths\n> diff --git a/help.c b/help.c\n> index 919cbb9206..2fd27cffb0 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -472,12 +472,18 @@ 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 (!strcasecmp(var, \"help.autocorrect\")) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n> +\t\tautocorrect = git_config_int(var, value);\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 +531,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> @@ -586,7 +597,7 @@ const char *help_unknown_cmd(const char *cmd)\n>  \t\t     n++)\n>  \t\t\t; /* still counting */\n>  \t}\n> -\tif (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) {\n> +\tif (autocorrect != 0 && n == 1 && SIMILAR_ENOUGH(best_similarity)) {\n\nI see there is nothing gained by writing \"!= 0\" explicitly here.\n\n"},{"id":"410127","messageId":"C75T4RXIM970.2CVY5MIDCX25Q@taiga","threadId":"54657","inReplyTo":"xmqqd00b698g.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] help.c: configurable suggestions","fromName":"Drew DeVault","fromEmail":"sir@cmpwn.com","sentAt":"2020-11-17T20:05:41Z","receivedAt":"2020-11-17T20:15:50Z","isPatch":true,"sender":{"key":"sir@cmpwn.com","avatar":"https://avatars.githubusercontent.com/u/1310872?v=4"},"body":"On Tue Nov 17, 2020 at 3:04 PM EST, Junio C Hamano wrote:\n> 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> > ---\n>\n> Missing sign-off.\n\nWhoops!\n\n> This changes behaviour for those who have relied on our promise that\n> any negative value means immediate execution. Now -2 means a totally\n> different thing.\n>\n> I thought I already showed you how to make it a \"number or keyword\"\n> in my previous review comment to avoid such a needless behaviour\n> change---did our mails cross?\n\nI must not have understood the psuedocode you sent. I'll take another\nlook at it. Thanks for the feedback!\n\n> > @@ -586,7 +597,7 @@ const char *help_unknown_cmd(const char *cmd)\n> >  \t\t     n++)\n> >  \t\t\t; /* still counting */\n> >  \t}\n> > -\tif (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) {\n> > +\tif (autocorrect != 0 && n == 1 && SIMILAR_ENOUGH(best_similarity)) {\n>\n> I see there is nothing gained by writing \"!= 0\" explicitly here.\n\n*shrug*\n"},{"id":"410135","messageId":"xmqq4kln65gh.fsf@gitster.c.googlers.com","threadId":"54657","inReplyTo":"20201117152535.9795-1-sir@cmpwn.com","subject":"Re: [PATCH v2] help.c: configurable suggestions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-17T21:25:50Z","receivedAt":"2020-11-17T21:26:20Z","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> -\tif (!strcmp(var, \"help.autocorrect\"))\n> -\t\tautocorrect = git_config_int(var,value);\n> +\tif (!strcasecmp(var, \"help.autocorrect\")) {\n\nThis is an unwarranted change.  The first part and the last part of\na configuration variable name are downcased before the config\ncallback functions are called, so strcmp() is sufficient.\n\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(var);\n\nThis is to protect us against\n\n\t[help]\n\t\tautocorrect\n\n(i.e. \"true\" expressed without \"= true\"), which is not strictly\nneeded when the only thing we do with value is git_config_int()\nbecause it knows how to handle value==NULL case.  But it starts\nto matter when parsing variables with more elaborate shape, like\n\"int or some known token\".\n\nAnd because we want to keep the promise \"if you write negative\nvalue, it means immediate\" we made to our users, we can instead make\nthis variable \"number or 'never'\".  To do so, keep that \"barf if\nNULL\" check above, and then add something like:\n\n\t\tif (!strcmp(value, \"never\"))\n\t\t\tautocorrect = AUTOCORRECT_NEVER;\n\t\telse {\n\t\t\tint v = git_config_int(var, value);\n                        autocorrect = (v < 0) \n\t\t\t\t? AUTOCORRECT_IMMEDIATELY : v;\n\t\t}\n\nThe configuration the end user has may say '-2', but that is\ndifferent from 'never', so instead of leaving a negative value as\nis, like this code below does ...\n\n> +\t\tautocorrect = git_config_int(var, value);\n\n... we normalize any negative value the user gave us to\nAUTOCORRECT_IMMEDIATELY.  That way, we can keep configuration the\nuser has working the same way as before, while allowing a new value\n'never' to have a new meaning.\n\nWe might want to further make the variable \"number, 'never' or\n'immediate'\" for consistency, with an eye on the possibility that we\nmight eventually want to deprecate the \"negative means immediate\".\n\nTo do so, we could do:\n\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                        autocorrect = (v < 0) \n\t\t\t\t? AUTOCORRECT_IMMEDIATELY : v;\n\t\t}\n\ninstead.\n\n"}]}