{"thread":{"id":"30438","subject":"[PATCH 1/4] help.c::uniq: plug a leak","startedAt":"2012-05-06T06:55:26Z","lastAt":"2012-08-06T20:01:38Z","messageCount":37,"participants":["Tay Ray Chuan","Jeff King","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"190878","messageId":"1336287330-7215-1-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":null,"subject":"[PATCH 0/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T06:55:26Z","receivedAt":"2012-05-06T06:55:26Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Patch 4 has the meat of this series.\n\nWhile running valgrind to check that I didn't leak any memory, a couple\nof leaks were spotted. Patches 1-3 address them.\n\nTay Ray Chuan (4):\n help.c::uniq: plug a leak\n help.c::exclude_cmds: plug a leak\n help.c: plug a leak when help.autocorrect is set\n allow recovery from command name typos\n\n help.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 62 insertions(+), 12 deletions(-)\n\n-- \n1.7.10.1.611.g8a79d96\n"},{"id":"190877","messageId":"1336287330-7215-2-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1336287330-7215-1-git-send-email-rctay89@gmail.com","subject":"[PATCH 1/4] help.c::uniq: plug a leak","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T06:55:27Z","receivedAt":"2012-05-06T06:55:27Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Signed-off-by: Tay Ray Chuan \n---\n help.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 69d483d..b64056d 100644\n--- a/help.c\n+++ b/help.c\n@@ -38,14 +38,24 @@ static int cmdname_compare(const void *a_, const void *b_)\n \n static void uniq(struct cmdnames *cmds)\n {\n-\tint i, j;\n+\tint i, j, c = 0;\n \n \tif (!cmds->cnt)\n \t\treturn;\n \n \tfor (i = j = 1; i < cmds->cnt; i++)\n-\t\tif (strcmp(cmds->names[i]->name, cmds->names[i-1]->name))\n+\t\tif (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) {\n+\n+\t\t\t/* The i-1 entry was the cth duplicate\n+\t\t\t * Guarantees c=0\n+\t\t\t */\n+\t\t\tfor (; c >= 1; c--)\n+\t\t\t\tfree(cmds->names[i - c]);\n+\n \t\t\tcmds->names[j++] = cmds->names[i];\n+\t\t} else {\n+\t\t\tc++;\n+\t\t}\n \n \tcmds->cnt = j;\n }\n-- \n1.7.10.1.611.g8a79d96\n"},{"id":"190880","messageId":"1336287330-7215-3-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1336287330-7215-2-git-send-email-rctay89@gmail.com","subject":"[PATCH 2/4] help.c::exclude_cmds: plug a leak","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T06:55:28Z","receivedAt":"2012-05-06T06:55:28Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Create a fresh cmdnames to hold the entries we want to keep, such that\nwe free the excluded entries in cmds only.\n\nSigned-off-by: Tay Ray Chuan \n\n---\n\nA solution that does not require a fresh cmdnames escapes me.\n---\n help.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex b64056d..705f152 100644\n--- a/help.c\n+++ b/help.c\n@@ -65,21 +65,29 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)\n \tint ci, cj, ei;\n \tint cmp;\n \n+\tstruct cmdnames excluded;\n+\tmemset(&excluded, 0, sizeof(excluded));\n+\tALLOC_GROW(excluded.names, cmds->cnt, excluded.alloc);\n+\n \tci = cj = ei = 0;\n \twhile (ci < cmds->cnt && ei < excludes->cnt) {\n \t\tcmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name);\n-\t\tif (cmp < 0)\n-\t\t\tcmds->names[cj++] = cmds->names[ci++];\n-\t\telse if (cmp == 0)\n+\t\tif (cmp < 0) {\n+\t\t\texcluded.names[cj] = cmds->names[ci];\n+\t\t\tcmds->names[ci] = NULL;\n+\t\t\tci++, cj++;\n+\t\t} else if (cmp == 0)\n \t\t\tci++, ei++;\n \t\telse if (cmp > 0)\n \t\t\tei++;\n \t}\n \n-\twhile (ci < cmds->cnt)\n-\t\tcmds->names[cj++] = cmds->names[ci++];\n-\n+\tclean_cmdnames(cmds);\n+\tcmds->alloc = excluded.alloc;\n \tcmds->cnt = cj;\n+\tcmds->names = excluded.names;\n+\twhile (cj--)\n+\t\tcmds->names[cj] = excluded.names[cj];\n }\n \n static void pretty_print_string_list(struct cmdnames *cmds,\n-- \n1.7.10.1.611.g8a79d96\n"},{"id":"190879","messageId":"1336287330-7215-4-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1336287330-7215-3-git-send-email-rctay89@gmail.com","subject":"[PATCH 3/4] help.c: plug a leak when help.autocorrect is set","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T06:55:29Z","receivedAt":"2012-05-06T06:55:29Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"In an attempt to retain the memory to the string name in main_cmds, we\nunfortunately leaked the struct cmdname that held it. Fix this by\ncreating a copy of the name.\n\nSigned-off-by: Tay Ray Chuan \n---\n help.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 705f152..f296d95 100644\n--- a/help.c\n+++ b/help.c\n@@ -360,8 +360,7 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t\t; /* still counting */\n \t}\n \tif (autocorrect && 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\tconst char *assumed = xstrdup(main_cmds.names[0]->name);\n \t\tclean_cmdnames(&main_cmds);\n \t\tfprintf_ln(stderr,\n \t\t\t _(\"WARNING: You called a Git command named '%s', \"\n-- \n1.7.10.1.611.g8a79d96\n"},{"id":"190881","messageId":"1336287330-7215-5-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1336287330-7215-4-git-send-email-rctay89@gmail.com","subject":"[PATCH 4/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T06:55:30Z","receivedAt":"2012-05-06T06:55:30Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"If suggestions are available (based on Levenshtein distance) and if the\nterminal isatty(), present a prompt to the user to select one of the\ncomputed suggestions.\n\nIn the case where there is a single suggestion, present the prompt\n\"[Y/n]\", such that \"\", \"y\" and \"Y\" as input leads git to proceed\nexecuting the suggestion, while everything else (possibly \"n\") leads git\nto terminate.\n\nIn the case where there are multiple suggestions, number the suggestions\n1 to n, and accept as input one of the numbers, while everything else\n(possibly \"n\") leads git to terminate. In this case there is no default;\nthat is, an empty input leads git to terminate. A sample run:\n\n $ git sh --pretty=oneline\n git: 'sh' is not a git command. See 'git --help'.\n\n Did you mean one of these?\n 1:\tshow\n 2:\tpush\n [1/2/.../n] 1\n\nSigned-off-by: Tay Ray Chuan \n---\n help.c | 37 +++++++++++++++++++++++++++++++++++--\n 1 file changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex f296d95..4aa1d88 100644\n--- a/help.c\n+++ b/help.c\n@@ -6,6 +6,7 @@\n #include \"common-cmds.h\"\n #include \"string-list.h\"\n #include \"column.h\"\n+#include \"compat/terminal.h\"\n \n void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n {\n@@ -383,8 +384,40 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t\t \"\\nDid you mean one of these?\",\n \t\t\t n));\n \n-\t\tfor (i = 0; i < n; i++)\n-\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n+\t\tif (!isatty(2))\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n+\t\telse if (n == 1) {\n+\t\t\tchar *in;\n+\t\t\tconst char *ret;\n+\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[0]->name);\n+\t\t\tin = git_terminal_prompt(\"[Y/n] \", 1);\n+\t\t\tswitch (in[0]) {\n+\t\t\tcase 'y': case 'Y': case 0:\n+\t\t\t\tret = xstrdup(main_cmds.names[0]->name);\n+\t\t\t\tclean_cmdnames(&main_cmds);\n+\t\t\t\treturn ret;\n+\t\t\t/* otherwise, don't do anything */\n+\t\t\t}\n+\t\t} else {\n+\t\t\tchar *in;\n+\t\t\tconst char *ret;\n+\t\t\tint opt;\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tfprintf(stderr, \"%d:\\t%s\\n\", i + 1, main_cmds.names[i]->name);\n+\t\t\tin = git_terminal_prompt(\"[1/2/.../n] \", 1);\n+\t\t\tswitch (in[0]) {\n+\t\t\t\tcase 'n': case 'N': case 0:\n+\t\t\t\t\t;\n+\t\t\t\tdefault:\n+\t\t\t\t\topt = atoi(in);\n+\t\t\t\t\tif (0 < opt && opt <= n) {\n+\t\t\t\t\t\tret = xstrdup(main_cmds.names[opt - 1]->name);\n+\t\t\t\t\t\tclean_cmdnames(&main_cmds);\n+\t\t\t\t\t\treturn ret;\n+\t\t\t\t\t}\n+\t\t\t}\n+\t\t}\n \t}\n \n \texit(1);\n-- \n1.7.10.1.611.g8a79d96\n"},{"id":"190882","messageId":"20120506081213.GA27878@sigill.intra.peff.net","threadId":"30438","inReplyTo":"1336287330-7215-2-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH 1/4] help.c::uniq: plug a leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-06T08:12:14Z","receivedAt":"2012-05-06T08:12:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 06, 2012 at 02:55:27PM +0800, Tay Ray Chuan wrote:\n\n> static void uniq(struct cmdnames *cmds)\n> {\n> -\tint i, j;\n> +\tint i, j, c = 0;\n> \n> \tif (!cmds->cnt)\n> \t\treturn;\n> \n> \tfor (i = j = 1; i < cmds->cnt; i++)\n> -\t\tif (strcmp(cmds->names[i]->name, cmds->names[i-1]->name))\n> +\t\tif (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) {\n> +\n> +\t\t\t/* The i-1 entry was the cth duplicate\n> +\t\t\t * Guarantees c=0\n> +\t\t\t */\n> +\t\t\tfor (; c >= 1; c--)\n> +\t\t\t\tfree(cmds->names[i - c]);\n> +\n> \t\t\tcmds->names[j++] = cmds->names[i];\n> +\t\t} else {\n> +\t\t\tc++;\n> +\t\t}\n> \n> \tcmds->cnt = j;\n> }\n\nFreeing the strings at the end of each run of duplicates is confusing to\nread. And your implementation is buggy: if there are duplicates at the\nvery end of the list, you would never free them (you would need to check\n'c' again at the end of the loop).\n\nI think you avoided freeing as you go because that invalidates the i-1\nelement that we use in the comparison. However, we can observe that the\nj-1 element can serve the same purpose, as it is either:\n\n 1. Exactly i-1, when the loop begins (and until we see a duplicate).\n\n 2. The same pointer that was stored at i-1 (if it was not a duplicate,\n and we just copied it into place).\n\n 3. A pointer to an equivalent string (i.e., we rejected i-1 _because_\n it was identical to j-1).\n\nSo this shorter patch should be sufficient (though I didn't actually\ntest it):\n\ndiff --git a/help.c b/help.c\nindex 69d483d..d3868b3 100644\n--- a/help.c\n+++ b/help.c\n@@ -43,9 +43,12 @@ static void uniq(struct cmdnames *cmds)\n \tif (!cmds->cnt)\n \t\treturn;\n \n-\tfor (i = j = 1; i < cmds->cnt; i++)\n-\t\tif (strcmp(cmds->names[i]->name, cmds->names[i-1]->name))\n+\tfor (i = j = 1; i < cmds->cnt; i++) {\n+\t\tif (!strcmp(cmds->names[i]->name, cmds->names[j-1]->name))\n+\t\t\tfree(cmds->names[i]);\n+\t\telse\n \t\t\tcmds->names[j++] = cmds->names[i];\n+\t}\n \n \tcmds->cnt = j;\n }\n"},{"id":"190883","messageId":"20120506082130.GB27878@sigill.intra.peff.net","threadId":"30438","inReplyTo":"1336287330-7215-5-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-06T08:21:30Z","receivedAt":"2012-05-06T08:21:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote:\n\n> If suggestions are available (based on Levenshtein distance) and if the\n> terminal isatty(), present a prompt to the user to select one of the\n> computed suggestions.\n> \n> In the case where there is a single suggestion, present the prompt\n> \"[Y/n]\", such that \"\", \"y\" and \"Y\" as input leads git to proceed\n> executing the suggestion, while everything else (possibly \"n\") leads git\n> to terminate.\n> \n> In the case where there are multiple suggestions, number the suggestions\n> 1 to n, and accept as input one of the numbers, while everything else\n> (possibly \"n\") leads git to terminate. In this case there is no default;\n> that is, an empty input leads git to terminate. A sample run:\n> \n> $ git sh --pretty=oneline\n> git: 'sh' is not a git command. See 'git --help'.\n> \n> Did you mean one of these?\n> 1:\tshow\n> 2:\tpush\n> [1/2/.../n] 1\n\nUgh. Please protect this with a config variable that defaults to\n\"off\". It is very un-Unix to prompt unexpectedly, and I suspect a lot\nof people would be annoyed by this behavior changing by default (I know\nI would be).\n\n-Peff\n"},{"id":"190924","messageId":"CALUzUxqtKGd9REqwyZLVnr4zcd20GmSREeNL7tDpA8kYaTtWBg@mail.gmail.com","threadId":"30438","inReplyTo":"20120506081213.GA27878@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] help.c::uniq: plug a leak","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T15:54:20Z","receivedAt":"2012-05-06T15:54:20Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sun, May 6, 2012 at 4:12 PM, Jeff King wrote:\n> So this shorter patch should be sufficient (though I didn't actually\n> test it):\n\nTested and works fine.\n\n> diff --git a/help.c b/help.c\n> index 69d483d..d3868b3 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -43,9 +43,12 @@ static void uniq(struct cmdnames *cmds)\n>        if (!cmds->cnt)\n>                return;\n>\n> -       for (i = j = 1; i < cmds->cnt; i++)\n> -               if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name))\n> +       for (i = j = 1; i < cmds->cnt; i++) {\n> +               if (!strcmp(cmds->names[i]->name, cmds->names[j-1]->name))\n> +                       free(cmds->names[i]);\n> +               else\n>                        cmds->names[j++] = cmds->names[i];\n> +       }\n>\n>        cmds->cnt = j;\n>  }\n\nNot only is this better than mine in terms of readability, it is\nbetter than the original code.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"190927","messageId":"CALUzUxqzi7aJ30q16+dwSnu_ULoC2zM-EDp1+BHTu2cPU9ihnQ@mail.gmail.com","threadId":"30438","inReplyTo":"CAOBOgRaDEgAqXWmdC6hrudkL5OwzeMffbj2RtKMxf2TsYWzotA@mail.gmail.com","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T16:04:54Z","receivedAt":"2012-05-06T16:04:54Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sun, May 6, 2012 at 4:40 PM, Angus Hammond wrote:\n> On 6 May 2012 07:55, Tay Ray Chuan wrote:\n>>\n>> In the case where there is a single suggestion, present the prompt\n>> \"[Y/n]\", such that \"\", \"y\" and \"Y\" as input leads git to proceed\n>> executing the suggestion, while everything else (possibly \"n\") leads git\n>> to terminate.\n>>\n>\n> Minor point, as well as ensuring this is configurable behavior, if\n> terminating is the default, then the prompt should be \"[y/N]\", so that the\n> default action is clearly marked. Not capitalising at all would be\n> reasonable, but making the 'Y' uppercase is actively confusing.\n\nI believe you were referring to the 1/2/.../n case, since for the y/n\ncase, terminating is not the default.\n\nIf so, then yes, the \"n\" there should be in caps, since terminating is\nthe default; my bad.\n\n> Secondly, if we're at a tty, I suspect this behavior would be totally\n> unnecessary. Making close suggestions rather than just a complete list is\n> neat, but if people want to use one of them all they have to do is copy\n> paste the old command down and modify it, which I suspect would be much\n> faster than actually considering and responding to a prompt.\n\nI believe that there would be very few options to choose from - in\nfact, I was trying very hard to get the number of suggestions to equal\nor exceed 5. I guess one would have better luck than I if they studied\nthe Levenshtein distance algorithm, which I didn't.\n\nIn other words - the time to consider is small.\n\nIn fact I was hoping this would be faster than copy-paste - typing the\noption (1 key) and enter (1 key) makes a total of 2 keys only.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"190928","messageId":"CALUzUxqXrsB8XfQL6vOiQo1pLHNRjxRUxJLRiK_mcSU8fvTSCg@mail.gmail.com","threadId":"30438","inReplyTo":"20120506082130.GB27878@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-06T16:07:08Z","receivedAt":"2012-05-06T16:07:08Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sun, May 6, 2012 at 4:21 PM, Jeff King wrote:\n> On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote:\n>\n>> If suggestions are available (based on Levenshtein distance) and if the\n>> terminal isatty(), present a prompt to the user to select one of the\n>> computed suggestions.\n>>\n>> In the case where there is a single suggestion, present the prompt\n>> \"[Y/n]\", such that \"\", \"y\" and \"Y\" as input leads git to proceed\n>> executing the suggestion, while everything else (possibly \"n\") leads git\n>> to terminate.\n>>\n>> In the case where there are multiple suggestions, number the suggestions\n>> 1 to n, and accept as input one of the numbers, while everything else\n>> (possibly \"n\") leads git to terminate. In this case there is no default;\n>> that is, an empty input leads git to terminate. A sample run:\n>>\n>>   $ git sh --pretty=oneline\n>>   git: 'sh' is not a git command. See 'git --help'.\n>>\n>>   Did you mean one of these?\n>>   1:  show\n>>   2:  push\n>>   [1/2/.../n] 1\n>\n> Ugh. Please protect this with a config variable that defaults to\n> \"off\".  It is very un-Unix to prompt unexpectedly, and I suspect a lot\n> of people would be annoyed by this behavior changing by default (I know\n> I would be).\n>\n> -Peff\n\nWhile I agree there should be a config to protect this, I was hoping\nthis would be useful to users on the terminal who make the occasional\nslip-up, without having to do any prior configuration.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"190966","messageId":"20120507073059.GD19874@sigill.intra.peff.net","threadId":"30438","inReplyTo":"CALUzUxqtKGd9REqwyZLVnr4zcd20GmSREeNL7tDpA8kYaTtWBg@mail.gmail.com","subject":"Re: [PATCH 1/4] help.c::uniq: plug a leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-05-07T07:30:59Z","receivedAt":"2012-05-07T07:30:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 06, 2012 at 11:54:20PM +0800, Tay Ray Chuan wrote:\n\n> On Sun, May 6, 2012 at 4:12 PM, Jeff King wrote:\n> > So this shorter patch should be sufficient (though I didn't actually\n> > test it):\n> \n> Tested and works fine.\n\nThanks. Do you want to just include it in your re-roll, or should I pick\nit up and re-post it with a commit message?\n\n-Peff\n"},{"id":"190976","messageId":"878vh4con4.fsf@thomas.inf.ethz.ch","threadId":"30438","inReplyTo":"CALUzUxqXrsB8XfQL6vOiQo1pLHNRjxRUxJLRiK_mcSU8fvTSCg@mail.gmail.com","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-05-07T09:43:27Z","receivedAt":"2012-05-07T09:43:27Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Tay Ray Chuan writes:\n\n> On Sun, May 6, 2012 at 4:21 PM, Jeff King wrote:\n>> On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote:\n>>\n>>>   $ git sh --pretty=oneline\n>>>   git: 'sh' is not a git command. See 'git --help'.\n>>>\n>>>   Did you mean one of these?\n>>>   1:  show\n>>>   2:  push\n>>>   [1/2/.../n] 1\n>>\n>> Ugh. Please protect this with a config variable that defaults to\n>> \"off\".  It is very un-Unix to prompt unexpectedly, and I suspect a lot\n>> of people would be annoyed by this behavior changing by default (I know\n>> I would be).\n>>\n>> -Peff\n>\n> While I agree there should be a config to protect this, I was hoping\n> this would be useful to users on the terminal who make the occasional\n> slip-up, without having to do any prior configuration.\n\nWe already have help.autocorrect. It defaults to 0, which results in\n\n $ g rebest\n git: 'rebest' is not a git command. See 'git --help'.\n \n Did you mean one of these?\n rebase\n reset\n revert\n\nBut it can also be a timeout in deciseconds, after which the match is\nautomatically executed (if there is only one). You could hijack it by\n\n* making 'ask' mean your new feature\n\n* making 'off' etc. be the same as 0 for sanity\n\n* making the default value be like 0, but with an extra message such as\n\n Use 'git config --global help.autocorrect ask' to let me prompt for\n the correct command.\n\n though I'm sure you can improve on the wording.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"191007","messageId":"CALUzUxren063JA8NfDNWKXxf4=4jRDftpTHB76-1fFD-900XAw@mail.gmail.com","threadId":"30438","inReplyTo":"878vh4con4.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-07T15:49:25Z","receivedAt":"2012-05-07T15:49:25Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Mon, May 7, 2012 at 5:43 PM, Thomas Rast wrote:\n> Tay Ray Chuan writes:\n>\n>> On Sun, May 6, 2012 at 4:21 PM, Jeff King wrote:\n>>> On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote:\n>>>\n>>>>   $ git sh --pretty=oneline\n>>>>   git: 'sh' is not a git command. See 'git --help'.\n>>>>\n>>>>   Did you mean one of these?\n>>>>   1:  show\n>>>>   2:  push\n>>>>   [1/2/.../n] 1\n>>>\n>>> Ugh. Please protect this with a config variable that defaults to\n>>> \"off\".  It is very un-Unix to prompt unexpectedly, and I suspect a lot\n>>> of people would be annoyed by this behavior changing by default (I know\n>>> I would be).\n>>>\n>>> -Peff\n>>\n>> While I agree there should be a config to protect this, I was hoping\n>> this would be useful to users on the terminal who make the occasional\n>> slip-up, without having to do any prior configuration.\n>\n> We already have help.autocorrect.  It defaults to 0, which results in\n>\n>  $ g rebest\n>  git: 'rebest' is not a git command. See 'git --help'.\n>\n>  Did you mean one of these?\n>          rebase\n>          reset\n>          revert\n>\n> But it can also be a timeout in deciseconds, after which the match is\n> automatically executed (if there is only one).  You could hijack it by\n>\n> * making 'ask' mean your new feature\n>\n> * making 'off' etc. be the same as 0 for sanity\n>\n> * making the default value be like 0, but with an extra message such as\n>\n>    Use 'git config --global help.autocorrect ask' to let me prompt for\n>    the correct command.\n>\n>  though I'm sure you can improve on the wording.\n\nThomas, that's a brilliant idea.\n\nRe-roll coming up.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"191013","messageId":"7v62c77uss.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"878vh4con4.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-07T17:41:39Z","receivedAt":"2012-05-07T17:41:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> We already have help.autocorrect. It defaults to 0, which results in\n>\n> $ g rebest\n> git: 'rebest' is not a git command. See 'git --help'.\n> \n> Did you mean one of these?\n> rebase\n> reset\n> revert\n>\n> But it can also be a timeout in deciseconds, after which the match is\n> automatically executed (if there is only one). You could hijack it by\n>\n> * making 'ask' mean your new feature\n>\n> * making 'off' etc. be the same as 0 for sanity\n>\n> * making the default value be like 0, but with an extra message such as\n>\n> Use 'git config --global help.autocorrect ask' to let me prompt for\n> the correct command.\n>\n> though I'm sure you can improve on the wording.\n\nSounds good.\n\nBy the way, does anybody actually use the deciseconds grace period to ^C\nthe process? I know I was the guilty party for suggesting it, but it\nstrikes me that it is rather a dangerous option. When checking out\nanother branch with great difference with \"git chekcout foo\", you would be\nasked \"did you mean checkout?\", and if you hit ^C a bit too late, you may\nnot kill autocorrect but end up killing a lengthy \"checkout\" in the\nmiddle, messing up the working tree with a mixture of files in old and new\nbranches, needing a \"reset --hard\" to recover. We might want to update\nthe documentation to warn about this, even though I personally do not\nthink it is worth removing the support (and going through the trouble of\nhaving to deal with \"why did you remove the useful feature\" complaints).\n"},{"id":"191198","messageId":"CALUzUxpF0zn0V89BcayavbVs6muuXPv4+eYWgCWJn90hj6s6hQ@mail.gmail.com","threadId":"30438","inReplyTo":"7v62c77uss.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-05-09T15:06:59Z","receivedAt":"2012-05-09T15:06:59Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Tue, May 8, 2012 at 1:41 AM, Junio C Hamano wrote:\n> By the way, does anybody actually use the deciseconds grace period to ^C\n> the process?  I know I was the guilty party for suggesting it, but it\n> strikes me that it is rather a dangerous option.  When checking out\n> another branch with great difference with \"git chekcout foo\", you would be\n> asked \"did you mean checkout?\", and if you hit ^C a bit too late, you may\n> not kill autocorrect but end up killing a lengthy \"checkout\" in the\n> middle, messing up the working tree with a mixture of files in old and new\n> branches, needing a \"reset --hard\" to recover.  We might want to update\n> the documentation to warn about this, even though I personally do not\n> think it is worth removing the support (and going through the trouble of\n> having to deal with \"why did you remove the useful feature\" complaints).\n>\n\nActually, I've never heard of that feature, until I was reading help.c.\n\nHowever, it's listed on Progit [1], so I'd imagine there'd be *some*\nusers in the wild.\n\n[1] http://git-scm.com/book/ch7-1.html\n\nPersonally, I think it's a little dangerous - imagine your script has\na typo'd command that just runs anyway if help.autocorrect without any\nchance for user intervention. Perhaps there should be a isatty(2)\ncheck to guard it, like the prompting patch does.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"191200","messageId":"7vwr4lthfo.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"CALUzUxpF0zn0V89BcayavbVs6muuXPv4+eYWgCWJn90hj6s6hQ@mail.gmail.com","subject":"Re: [PATCH 4/4] allow recovery from command name typos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-09T17:03:55Z","receivedAt":"2012-05-09T17:03:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> Actually, I've never heard of that feature, until I was reading help.c.\n>\n> However, it's listed on Progit [1], so I'd imagine there'd be *some*\n> users in the wild.\n>\n> [1] http://git-scm.com/book/ch7-1.html\n>\n> Personally, I think it's a little dangerous - imagine your script has\n> a typo'd command that just runs anyway if help.autocorrect without any\n> chance for user intervention. Perhaps there should be a isatty(2)\n> check to guard it, like the prompting patch does.\n\nThe whole \"did you mean one of these\" autocorrection should trigger only\nin interactive to begin with, I would have thought. Are you saying that\nwe don't have isatty(3) check in the early in the codepath already?\n\nIn any case, we drifted into a tangent without seeing the patch series to\ncompletion. Are you rerolling with Peff's fixups, Peff hinted he is\nwilling to do a re-post, and are you counting on it, or should I just pick\nup the pieces?\n\nThanks.\n"},{"id":"195758","messageId":"1343232982-10540-1-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1336287330-7215-1-git-send-email-rctay89@gmail.com","subject":"[PATCH v2 0/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-07-25T16:16:18Z","receivedAt":"2012-07-25T16:16:18Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Patch 4 has the meat of this series.\n\nWhile running valgrind to check that I didn't leak any memory, a couple\nof leaks were spotted. Patches 1-3 address them.\n\nMajor change in v2: implement Thomas' idea [1] about using\nhelp.autocorrect to configure this behaviour.\n\n[1] <878vh4con4.fsf@thomas.inf.ethz.ch>\n\nJeff King (1):\n help.c::uniq: plug a leak\n\nTay Ray Chuan (3):\n help.c::exclude_cmds: realloc() before copy, plug a leak\n help.c: plug leaks with(out) help.autocorrect\n allow recovery from command name typos\n\n Documentation/config.txt | 30 ++++++++++++----\n advice.c | 2 ++\n advice.h | 1 +\n help.c | 94 ++++++++++++++++++++++++++++++++++++++++++------\n 4 files changed, 110 insertions(+), 17 deletions(-)\n\n-- \n1.7.11.1.116.g8228a23\n"},{"id":"195759","messageId":"1343232982-10540-2-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1343232982-10540-1-git-send-email-rctay89@gmail.com","subject":"[PATCH v2 1/4] help.c::uniq: plug a leak","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-07-25T16:16:19Z","receivedAt":"2012-07-25T16:16:19Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"From: Jeff King \n\nWe observe that the j-1 element can serve the same purpose as the i-1\nelement that we use in the strcmp(); it is either:\n\n 1. Exactly i-1, when the loop begins (and until we see a duplicate).\n\n 2. The same pointer that was stored at i-1 (if it was not a duplicate,\n and we just copied it into place).\n\n 3. A pointer to an equivalent string (i.e., we rejected i-1 _because_\n it was identical to j-1).\n\nSigned-off-by: Jeff King \nSigned-off-by: Tay Ray Chuan \n\n---\n\nChanged in v2: used Jeff's code from [1]. Patch text was also based on\nit.\n\n[1] <20120506081213.GA27878@sigill.intra.peff.net>\n---\n help.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 662349d..6991492 100644\n--- a/help.c\n+++ b/help.c\n@@ -44,9 +44,12 @@ static void uniq(struct cmdnames *cmds)\n \tif (!cmds->cnt)\n \t\treturn;\n \n-\tfor (i = j = 1; i < cmds->cnt; i++)\n-\t\tif (strcmp(cmds->names[i]->name, cmds->names[i-1]->name))\n+\tfor (i = j = 1; i < cmds->cnt; i++) {\n+\t\tif (!strcmp(cmds->names[i]->name, cmds->names[j-1]->name))\n+\t\t\tfree(cmds->names[i]);\n+\t\telse\n \t\t\tcmds->names[j++] = cmds->names[i];\n+\t}\n \n \tcmds->cnt = j;\n }\n-- \n1.7.11.1.116.g8228a23\n"},{"id":"195760","messageId":"1343232982-10540-3-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1343232982-10540-2-git-send-email-rctay89@gmail.com","subject":"[PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-07-25T16:16:20Z","receivedAt":"2012-07-25T16:16:20Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Copying with structural assignment may not take into account that the\nLHS struct has sufficient memory, especially since the cmdname->name\nmember is nonfixed in size. Be unambiguous about it by realloc()'ing it\nto be of sufficient size.\n\nAdditionally, free the unused cmdnames, which are no longer accessible\nanyway.\n\nSigned-off-by: Tay Ray Chuan \n---\n help.c | 20 ++++++++++++++++++--\n 1 file changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 6991492..dfb2e9d 100644\n--- a/help.c\n+++ b/help.c\n@@ -20,6 +20,17 @@ void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n \tcmds->names[cmds->cnt++] = ent;\n }\n \n+static void copy_cmdname(struct cmdname **dest, struct cmdname *src)\n+{\n+\tstruct cmdname *ent = xrealloc(*dest, sizeof(*ent) + src->len + 1);\n+\n+\tent->len = src->len;\n+\tmemcpy(ent->name, src->name, src->len);\n+\tent->name[src->len] = 0;\n+\n+\t*dest = ent;\n+}\n+\n static void clean_cmdnames(struct cmdnames *cmds)\n {\n \tint i;\n@@ -58,20 +69,25 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)\n {\n \tint ci, cj, ei;\n \tint cmp;\n+\tint last_cj;\n \n \tci = cj = ei = 0;\n \twhile (ci < cmds->cnt && ei < excludes->cnt) {\n \t\tcmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name);\n \t\tif (cmp < 0)\n-\t\t\tcmds->names[cj++] = cmds->names[ci++];\n+\t\t\tcopy_cmdname(&cmds->names[cj++], cmds->names[ci++]);\n \t\telse if (cmp == 0)\n \t\t\tci++, ei++;\n \t\telse if (cmp > 0)\n \t\t\tei++;\n \t}\n+\tlast_cj = cj;\n \n \twhile (ci < cmds->cnt)\n-\t\tcmds->names[cj++] = cmds->names[ci++];\n+\t\tcopy_cmdname(&cmds->names[cj++], cmds->names[ci++]);\n+\n+\twhile (last_cj < cmds->cnt)\n+\t\tfree(cmds->names[last_cj++]);\n \n \tcmds->cnt = cj;\n }\n-- \n1.7.11.1.116.g8228a23\n"},{"id":"195761","messageId":"1343232982-10540-4-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1343232982-10540-3-git-send-email-rctay89@gmail.com","subject":"[PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-07-25T16:16:21Z","receivedAt":"2012-07-25T16:16:21Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"When help.autocorrect is set, in an attempt to retain the memory to the\nstring name in main_cmds, we unfortunately leaked the struct cmdname\nthat held it. Fix this by creating a copy of the string name.\n\nWhen help.autocorrect is not set, we exit()'d without free'ing it like\nwe do when the config is set; fix this.\n\nSigned-off-by: Tay Ray Chuan \n---\n\nChanged in v2: plug leak when help.autocorrect is not set.\n\n---\n help.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex dfb2e9d..ee261f4 100644\n--- a/help.c\n+++ b/help.c\n@@ -362,8 +362,7 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t\t; /* still counting */\n \t}\n \tif (autocorrect && 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\tconst char *assumed = xstrdup(main_cmds.names[0]->name);\n \t\tclean_cmdnames(&main_cmds);\n \t\tfprintf_ln(stderr,\n \t\t\t _(\"WARNING: You called a Git command named '%s', \"\n@@ -390,6 +389,7 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n \t}\n \n+\tclean_cmdnames(&main_cmds);\n \texit(1);\n }\n \n-- \n1.7.11.1.116.g8228a23\n"},{"id":"195762","messageId":"1343232982-10540-5-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1343232982-10540-4-git-send-email-rctay89@gmail.com","subject":"[PATCH v2 4/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-07-25T16:16:22Z","receivedAt":"2012-07-25T16:16:22Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"If suggestions are available (based on Levenshtein distance) and if the\nterminal isatty(), present a prompt to the user to select one of the\ncomputed suggestions.\n\nIn the case where there is a single suggestion, present the prompt\n\"[Y/n]\", such that \"\" (ie. the default), \"y\" and \"Y\" as input leads git\nto proceed executing the suggestion, while everything else (possibly\n\"n\") leads git to terminate.\n\nIn the case where there are multiple suggestions, number the suggestions\n1 to (the number of suggestions), and accept an integer as input,\nwhile everything else (possibly \"n\") leads git to terminate. In this\ncase there is no default; an empty input leads git to terminate. A\nsample run:\n\n $ git sh --pretty=oneline\n git: 'sh' is not a git command. See 'git --help'.\n\n Did you mean one of these?\n 1: show\n 2: push\n [N/1/2/...]\n\nThis prompt is enabled only if help.autocorrect is set to ask; if unset,\nadvise the user about this ability.\n\nSigned-off-by: Tay Ray Chuan \n---\n\nChanged in v2: implement Thomas' idea [1] to hijack help.autocorrect to\nconfigure this behaviour.\n\n[1] <878vh4con4.fsf@thomas.inf.ethz.ch>\n\n---\n Documentation/config.txt | 30 +++++++++++++++++------\n advice.c | 2 ++\n advice.h | 1 +\n help.c | 63 +++++++++++++++++++++++++++++++++++++++++++++---\n 4 files changed, 85 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcea8a..0bb175a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -177,6 +177,10 @@ advice.*::\n \t\tAdvice shown when you used linkgit:git-checkout[1] to\n \t\tmove to the detach HEAD state, to instruct how to create\n \t\ta local branch after the fact.\n+\ttypoPrompt::\n+\t\tUpon a mistyped command, if 'help.autocorrect' is unset\n+\t\tadvise that an interactive prompt can be displayed to\n+\t\trecover from the typo.\n --\n \n core.fileMode::\n@@ -1323,13 +1327,25 @@ 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+\tSpecifies behaviour to recover from mistyped commands.\n++\n+When set to `ask`, an interactive prompt is displayed, allowing the user\n+to select a suggested command for execution.\n++\n+When set to `off`, no attempt to recover is made.\n++\n+If a number is given, it will be interpreted as the deciseconds (0.1\n+sec) to wait before automatically correcting and executing the mistyped\n+command, with the following behaviour:\n++\n+* If more than one command can be deduced from the entered text, nothing\n+ will be executed.\n+* If the value of this option is negative, the corrected command will be\n+ executed immediately.\n+* If the value is 0 - the command will be just shown but not executed.\n++\n+The default is to display a message suggesting that this option be set\n+to `ask`, without attempting to recover (see `advice.typoPrompt`).\n \n http.proxy::\n \tOverride the HTTP proxy, normally configured using the 'http_proxy',\ndiff --git a/advice.c b/advice.c\nindex a492eea..d070a05 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -9,6 +9,7 @@ int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n int advice_implicit_identity = 1;\n int advice_detached_head = 1;\n+int advice_typo_prompt = 1;\n \n static struct {\n \tconst char *name;\n@@ -23,6 +24,7 @@ static struct {\n \t{ \"resolveconflict\", &advice_resolve_conflict },\n \t{ \"implicitidentity\", &advice_implicit_identity },\n \t{ \"detachedhead\", &advice_detached_head },\n+\t{ \"typoprompt\", &advice_typo_prompt },\n };\n \n void advise(const char *advice, ...)\ndiff --git a/advice.h b/advice.h\nindex f3cdbbf..050068d 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -12,6 +12,7 @@ extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\n extern int advice_implicit_identity;\n extern int advice_detached_head;\n+extern int advice_typo_prompt;\n \n int git_default_advice_config(const char *var, const char *value);\n void advise(const char *advice, ...);\ndiff --git a/help.c b/help.c\nindex ee261f4..4b45e43 100644\n--- a/help.c\n+++ b/help.c\n@@ -7,6 +7,7 @@\n #include \"string-list.h\"\n #include \"column.h\"\n #include \"version.h\"\n+#include \"compat/terminal.h\"\n \n void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n {\n@@ -248,12 +249,30 @@ int is_in_cmdlist(struct cmdnames *c, const char *s)\n }\n \n static int autocorrect;\n+static int shall_advise = 1;\n+static int shall_prompt;\n+static const char message_advice_prompt_ability[] =\n+\tN_(\"I can display an interactive prompt to proceed with one of the above\\n\"\n+\t \"suggestions; if you wish me to do so, use\\n\"\n+\t \"\\n\"\n+\t \" git config --global help.autocorrect ask\\n\"\n+\t \"\\n\"\n+\t \"See 'git help config' and search for 'help.autocorrect' for further\\n\"\n+\t \"information.\\n\");\n static struct cmdnames aliases;\n \n static int git_unknown_cmd_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"help.autocorrect\"))\n-\t\tautocorrect = git_config_int(var,value);\n+\tif (!strcmp(var, \"help.autocorrect\") && value) {\n+\t\tshall_advise = 0;\n+\t\tif (!strcasecmp(value, \"off\"))\n+\t\t\t;\n+\t\telse if (!strcasecmp(value, \"ask\"))\n+\t\t\tshall_prompt = 1;\n+\t\telse\n+\t\t\tautocorrect = git_config_int(var, value);\n+\t}\n+\n \t/* Also use aliases for command lookup */\n \tif (!prefixcmp(var, \"alias.\"))\n \t\tadd_cmdname(&aliases, var + 6, strlen(var + 6));\n@@ -385,8 +404,44 @@ const char *help_unknown_cmd(const char *cmd)\n \t\t\t \"\\nDid you mean one of these?\",\n \t\t\t n));\n \n-\t\tfor (i = 0; i < n; i++)\n-\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n+\t\tif (!isatty(2) || !shall_prompt) {\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n+\t\t\tif (isatty(2) && shall_advise && advice_typo_prompt) {\n+\t\t\t\tfprintf(stderr, \"\\n\");\n+\t\t\t\tadvise(_(message_advice_prompt_ability));\n+\t\t\t}\n+\t\t} else if (n == 1) {\n+\t\t\tchar *in;\n+\t\t\tconst char *ret;\n+\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[0]->name);\n+\t\t\tin = git_terminal_prompt(\"[Y/n] \", 1);\n+\t\t\tswitch (in[0]) {\n+\t\t\tcase 'y': case 'Y': case 0:\n+\t\t\t\tret = xstrdup(main_cmds.names[0]->name);\n+\t\t\t\tclean_cmdnames(&main_cmds);\n+\t\t\t\treturn ret;\n+\t\t\t/* otherwise, don't do anything */\n+\t\t\t}\n+\t\t} else {\n+\t\t\tchar *in;\n+\t\t\tconst char *ret;\n+\t\t\tint opt;\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tfprintf(stderr, \"%d:\\t%s\\n\", i + 1, main_cmds.names[i]->name);\n+\t\t\tin = git_terminal_prompt(\"[N/1/2/...] \", 1);\n+\t\t\tswitch (in[0]) {\n+\t\t\tcase 'n': case 'N': case 0:\n+\t\t\t\t;\n+\t\t\tdefault:\n+\t\t\t\topt = atoi(in);\n+\t\t\t\tif (0 < opt && opt <= n) {\n+\t\t\t\t\tret = xstrdup(main_cmds.names[opt - 1]->name);\n+\t\t\t\t\tclean_cmdnames(&main_cmds);\n+\t\t\t\t\treturn ret;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n \t}\n \n \tclean_cmdnames(&main_cmds);\n-- \n1.7.11.1.116.g8228a23\n"},{"id":"195769","messageId":"7v394fd9k6.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"1343232982-10540-3-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T17:39:37Z","receivedAt":"2012-07-25T17:39:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> Copying with structural assignment may not take into account that the\n> LHS struct has sufficient memory, especially since the cmdname->name\n> member is nonfixed in size. Be unambiguous about it by realloc()'ing it\n> to be of sufficient size.\n\nIf the original code were\n\n\t*(cmd->names[cj++]) = *(cmd->names[ci++]);\n\nthere may be a structural assignment involved, but\n\n\tcmds->names[dst] = cmd->names[src]\n\njust copies the pointer that points at a struct cmdname that records\nthe src command name to another slot of cmds->names[] array, whose\nelements are pointers, no? What's there to realloc?\n\n> @@ -58,20 +69,25 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)\n> {\n> \tint ci, cj, ei;\n> \tint cmp;\n> +\tint last_cj;\n> \n> \tci = cj = ei = 0;\n> \twhile (ci < cmds->cnt && ei < excludes->cnt) {\n> \t\tcmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name);\n> \t\tif (cmp < 0)\n> -\t\t\tcmds->names[cj++] = cmds->names[ci++];\n> +\t\t\tcopy_cmdname(&cmds->names[cj++], cmds->names[ci++]);\n> \t\telse if (cmp == 0)\n> \t\t\tci++, ei++;\n> \t\telse if (cmp > 0)\n> \t\t\tei++;\n> \t}\n> +\tlast_cj = cj;\n> \n> \twhile (ci < cmds->cnt)\n> -\t\tcmds->names[cj++] = cmds->names[ci++];\n> +\t\tcopy_cmdname(&cmds->names[cj++], cmds->names[ci++]);\n> +\n> +\twhile (last_cj < cmds->cnt)\n> +\t\tfree(cmds->names[last_cj++]);\n> \n> \tcmds->cnt = cj;\n> }\n\nWe shifted cmds->names[] array to skip entries that appear in\nexcludes. If original cmds->names[] had \"0\", \"1\", \"2\", \"3\", ...\nand excludes had \"0\" and \"1\", cmds->names[] would contain \"2\", \"3\",\n\"2\", \"3\"; the first two are copied over \"0\" and \"1\" that are\nexcluded, and the latter two are leftover beyond last_cj. The\ncorresponding names share the same structure (cmds->names[] is an\narray of pointers). Doesn't freeing cmds->names[2] free the\nstructure that is used by both cmds->names[0] and cmds->names[2]?\n\nConfused.\n\nThe function drops cmds->names[ci] when it appears in excludes, so\nyou may want to free it when it happens, though.\n\n help.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex 6991492..cae389b 100644\n--- a/help.c\n+++ b/help.c\n@@ -64,9 +64,10 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes)\n \t\tcmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name);\n \t\tif (cmp < 0)\n \t\t\tcmds->names[cj++] = cmds->names[ci++];\n-\t\telse if (cmp == 0)\n-\t\t\tci++, ei++;\n-\t\telse if (cmp > 0)\n+\t\telse if (cmp == 0) {\n+\t\t\tei++;\n+\t\t\tfree(cmd->names[ci++]);\n+\t\t} else if (cmp > 0)\n \t\t\tei++;\n \t}\n \n"},{"id":"195770","messageId":"7vy5m7bulu.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"1343232982-10540-4-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T17:47:57Z","receivedAt":"2012-07-25T17:47:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> When help.autocorrect is set, in an attempt to retain the memory to the\n> string name in main_cmds, we unfortunately leaked the struct cmdname\n> that held it. Fix this by creating a copy of the string name.\n\nIf you are updating help_unknown_cmd() so that the caller can free\n(and is responsible for freeing if it wants to avoid leaks) the\npiece of memory it returns, that change to \"assumed\" makes sense.\nWhat we were returning were not freeable.\n\nButthe caller does not free it; what you did is merely to shift the\nleakage from here to its caller.\n\nIs it worth the churn and one extra allocation to still leak\nslightly (namely, by sizeof(size_t)) smaller chunk of memory than\nwhat the code currently does? I doubt it.\n\n> When help.autocorrect is not set, we exit()'d without free'ing it like\n> we do when the config is set; fix this.\n\nI don't see any point in doing this, immediately in the same\nfunction on the previous line of exit(1).\n\n> Signed-off-by: Tay Ray Chuan \n> ---\n>\n> Changed in v2: plug leak when help.autocorrect is not set.\n>\n> ---\n> help.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/help.c b/help.c\n> index dfb2e9d..ee261f4 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -362,8 +362,7 @@ const char *help_unknown_cmd(const char *cmd)\n> \t\t\t; /* still counting */\n> \t}\n> \tif (autocorrect && 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\tconst char *assumed = xstrdup(main_cmds.names[0]->name);\n> \t\tclean_cmdnames(&main_cmds);\n> \t\tfprintf_ln(stderr,\n> \t\t\t _(\"WARNING: You called a Git command named '%s', \"\n> @@ -390,6 +389,7 @@ const char *help_unknown_cmd(const char *cmd)\n> \t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n> \t}\n> \n> +\tclean_cmdnames(&main_cmds);\n> \texit(1);\n> }\n"},{"id":"195771","messageId":"7vtxwvbu5s.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"1343232982-10540-5-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH v2 4/4] allow recovery from command name typos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T17:57:35Z","receivedAt":"2012-07-25T17:57:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> If suggestions are available (based on Levenshtein distance) and if the\n> terminal isatty(), present a prompt to the user to select one of the\n> computed suggestions.\n\nThe way to determine \"If the terminal is a tty\" used in this patch\nlooks overly dangerous, given that we do not know what kind of \"git\"\ncommand we may be invoking at this point.\n\nPerhaps we should audit \"isatty()\" calls and replace them with a\nhelper function that does this kind of thing consistently in a more\nrobust way (my recent favorite is Linus's somewhat anal logic used\nin builtin/merge.c::default_edit_option()).\n\n> +static int shall_advise = 1;\n> +static int shall_prompt;\n\nNaming \"shall_foo\" is a first here. It is not wrong per-se, but I\nthink we tend to call these \"do we use/perform/etc X\" do_X in our\ncodebase (see builtin/{config.c,fetch-pack.c,notes.c} for examples).\n"},{"id":"195868","messageId":"CALUzUxp91zubHEkWMC1z2xp7kJCRYrtznQS_=pVSZoNkZMihig@mail.gmail.com","threadId":"30438","inReplyTo":"7vtxwvbu5s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/4] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-07-26T17:08:34Z","receivedAt":"2012-07-26T17:08:34Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Thu, Jul 26, 2012 at 1:57 AM, Junio C Hamano wrote:\n>\n> Tay Ray Chuan writes:\n>\n> > If suggestions are available (based on Levenshtein distance) and if the\n> > terminal isatty(), present a prompt to the user to select one of the\n> > computed suggestions.\n>\n> The way to determine \"If the terminal is a tty\" used in this patch\n> looks overly dangerous, given that we do not know what kind of \"git\"\n> command we may be invoking at this point.\n\nIndeed, it should also have considered stdin's tty-ness.\n\n> Perhaps we should audit \"isatty()\" calls and replace them with a\n> helper function that does this kind of thing consistently in a more\n> robust way (my recent favorite is Linus's somewhat anal logic used\n> in builtin/merge.c::default_edit_option()).\n\nAny specific callers to isatty() you have in mind? A quick grep shows\nthat a significant portion of the \"offenders\" are isatty(2) calls to\ndetermine whether to display progress, I think those are ok.\n\nThe credential helper has some prompting functionality that is close\nto what I intend to do here, but I think it can make some assumptions\nabout stdin/stdout that we can't, as you have pointed out. So that\nleaves merge-edit and this patch as the beneficiaries of a\nbuiltin/merge.c::default_edit_option() refactor. That's just off the\ntop of my head.\n\nPerhaps the helper function could be named \"git_can_prompt()\" and\nplaced in prompt.c?\n\n--\nCheers,\nRay Chuan\n"},{"id":"195871","messageId":"20120726172630.GD13942@sigill.intra.peff.net","threadId":"30438","inReplyTo":"CALUzUxp91zubHEkWMC1z2xp7kJCRYrtznQS_=pVSZoNkZMihig@mail.gmail.com","subject":"Re: [PATCH v2 4/4] allow recovery from command name typos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T17:26:30Z","receivedAt":"2012-07-26T17:26:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 27, 2012 at 01:08:34AM +0800, Tay Ray Chuan wrote:\n\n> > Perhaps we should audit \"isatty()\" calls and replace them with a\n> > helper function that does this kind of thing consistently in a more\n> > robust way (my recent favorite is Linus's somewhat anal logic used\n> > in builtin/merge.c::default_edit_option()).\n> \n> Any specific callers to isatty() you have in mind? A quick grep shows\n> that a significant portion of the \"offenders\" are isatty(2) calls to\n> determine whether to display progress, I think those are ok.\n\nYeah, those are probably fine. Grep reveals that besides isatty(2) and\nthe merge default_edit_option check, we have:\n\n - isatty(1) for checking auto-output munging, including auto-colors,\n auto-columns, and the pager. These are all fine, as they are not\n about interactivity, but specifically about whether stdout is a tty.\n\n - isatty(0) in commit.c to print a message when reading \"-F -\" from\n stdin. OK.\n\n - isatty(0) in pack-redundant to avoid reading stdin when it is a\n terminal (a questionable choice, perhaps, but not really something\n that would want a full interactivity check).\n\n - isatty(0) check in cmd_revert to set opts.edit automatically. This\n one should match merge's behavior.\n\n - isatty(0) in shortlog; this is a compatibility hack as shortlog\n traditionally accepted log output on stdin, but can now be used\n stand-alone. OK.\n\nSo I think the only one that could be improved is the one in cmd_revert.\n\n> The credential helper has some prompting functionality that is close\n> to what I intend to do here, but I think it can make some assumptions\n> about stdin/stdout that we can't, as you have pointed out. So that\n> leaves merge-edit and this patch as the beneficiaries of a\n> builtin/merge.c::default_edit_option() refactor. That's just off the\n> top of my head.\n\nThe credential code uses git_terminal_prompt, which actually opens\n/dev/tty directly. So it is probably sane to use for your new prompt,\nbut it does not (and should not) rely on isatty.\n\n> Perhaps the helper function could be named \"git_can_prompt()\" and\n> placed in prompt.c?\n\nPlease don't. The isatty() checks have nothing to do with whether\ngit_prompt can run. The only thing such a git_can_prompt function should\ndo is see if we can open /dev/tty.\n\nThe isatty check in merge.c is more about \"are we interactive, so that\nit is sane to run $EDITOR\".\n\n-Peff\n"},{"id":"195877","messageId":"7v394e8l4b.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"CALUzUxp91zubHEkWMC1z2xp7kJCRYrtznQS_=pVSZoNkZMihig@mail.gmail.com","subject":"Re: [PATCH v2 4/4] allow recovery from command name typos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T17:53:24Z","receivedAt":"2012-07-26T17:53:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> On Thu, Jul 26, 2012 at 1:57 AM, Junio C Hamano wrote:\n>>\n>> Tay Ray Chuan writes:\n>>\n>> > If suggestions are available (based on Levenshtein distance) and if the\n>> > terminal isatty(), present a prompt to the user to select one of the\n>> > computed suggestions.\n>>\n>> The way to determine \"If the terminal is a tty\" used in this patch\n>> looks overly dangerous, given that we do not know what kind of \"git\"\n>> command we may be invoking at this point.\n>\n> Indeed, it should also have considered stdin's tty-ness.\n\nNot necessarily. As long as you call git_prompt(), which opens\n/dev/tty on its own and does not break even if its standard input is\ncoming from elsewhere, you should be OK.\n"},{"id":"195878","messageId":"7vy5m67694.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"20120726172630.GD13942@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] allow recovery from command name typos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T17:59:51Z","receivedAt":"2012-07-26T17:59:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - isatty(0) check in cmd_revert to set opts.edit automatically. This\n> one should match merge's behavior.\n> ...\n> So I think the only one that could be improved is the one in cmd_revert.\n\nYeah, that matches the result of my grep.\n\nThanks for sanity checking.\n\n> The credential code uses git_terminal_prompt, which actually opens\n> /dev/tty directly. So it is probably sane to use for your new prompt,\n> but it does not (and should not) rely on isatty.\n\nI think using git_terminal_prompt() after doing a looser \"does the\nuser sit at a terminal and is capable of answering interactive\nprompt\" check with isatty(2) is OK, as long as we know that all\nimplementations of git_terminal_prompt() never read from whatever\nhappens to be connected to the standard input.\n\nThe function falls back to getpass() on platforms without DEV_TTY,\nand if getpass() on some platforms reads from the standard input,\nthat would be a disaster. I wasn't sure about that part.\n"},{"id":"195880","messageId":"20120726183734.GA16037@sigill.intra.peff.net","threadId":"30438","inReplyTo":"7vy5m67694.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/4] allow recovery from command name typos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-26T18:37:35Z","receivedAt":"2012-07-26T18:37:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 26, 2012 at 10:59:51AM -0700, Junio C Hamano wrote:\n\n> > The credential code uses git_terminal_prompt, which actually opens\n> > /dev/tty directly. So it is probably sane to use for your new prompt,\n> > but it does not (and should not) rely on isatty.\n> \n> I think using git_terminal_prompt() after doing a looser \"does the\n> user sit at a terminal and is capable of answering interactive\n> prompt\" check with isatty(2) is OK, as long as we know that all\n> implementations of git_terminal_prompt() never read from whatever\n> happens to be connected to the standard input.\n\nI don't think isatty(2) is correct, though. It would yield false\nnegatives when the user has redirected stderr but /dev/tty is still\navailable. I don't know if it possible for getpass to fallback to stdin\nwhen stderr is a tty (it would mean that opening /dev/tty failed, which\nwould mean that we have no controlling terminal _but_ our stderr is\nstill connected to some terminal. That might be bizarre enough not to\ncare about).\n\nI think the right answer would be a real is_prompt_available() that\nchecked /dev/tty when HAVE_DEV_TTY was set, and otherwise checked\nisatty(2) (or whatever was appropriate for the platform).\n\n> The function falls back to getpass() on platforms without DEV_TTY,\n> and if getpass() on some platforms reads from the standard input,\n> that would be a disaster. I wasn't sure about that part.\n\nYeah, although it is already a disaster in those cases, as the main\ncaller of git_terminal_prompt is remote-curl, whose stdin is connected\nto git via the remote-helper protocol. Which isn't to say this wouldn't\nmake things worse. It would, but the real solution is to implement a\nsane git_terminal_prompt for those platforms. Erik had a patch for\nWindows to use their magical CONIN$, but I think it is temporarily\nstalled. I don't know if there are any other platforms that do not have\n/dev/tty (I know we do not set HAVE_DEV_TTY by default, but that is only\nbecause I was being conservative and waiting for people on particular\nplatforms to confirm that it works before tweaking our Makefile\ndefaults).\n\n-Peff\n"},{"id":"196491","messageId":"1344192340-19415-1-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1336287330-7215-1-git-send-email-rctay89@gmail.com","subject":"[PATCH v3 0/2] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-08-05T18:45:38Z","receivedAt":"2012-08-05T18:45:38Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"As discussed in the previous iteration, testing for prompt-availabilty\nhas been reworked (patch #2).\n\nThis is done with the aid of patch #1, which extracts the opening of\n/dev/tty from git_terminal_prompt() into a terminal_open(). Its return\nvalue indicates if a terminal is available for prompting. On systems\nwith HAVE_DEV_TTY unset, terminal_prompt() falls back to checking the\ntty-ness of stdin and stderr (as getpass() uses both).\n\nContents:\n[PATCH v3 1/2] add interface for /dev/tty interaction\n[PATCH v3 2/2] allow recovery from command name typos\n\n-- \n1.7.12.rc1.187.g6dd9156\n"},{"id":"196492","messageId":"1344192340-19415-2-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1344192340-19415-1-git-send-email-rctay89@gmail.com","subject":"[PATCH v3 1/2] add interface for /dev/tty interaction","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-08-05T18:45:39Z","receivedAt":"2012-08-05T18:45:39Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Factor out the opening and closing of /dev/tty from\ngit_terminal_prompt(), so that callers may first test if a controlling\nterminal is available before proceeding with prompting proper.\n\nWhen HAVE_DEV_TTY is not defined, terminal_open() falls back to checking\ntty-ness of stdin and stderr, as getpass() uses them both.\n\nSigned-off-by: Tay Ray Chuan \n---\n compat/terminal.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------\n compat/terminal.h | 10 ++++++++++\n 2 files changed, 54 insertions(+), 8 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 6d16c8f..c85d5c7 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -24,15 +24,21 @@ static void restore_term_on_signal(int sig)\n \traise(sig);\n }\n \n-char *git_terminal_prompt(const char *prompt, int echo)\n+term_t terminal_open(void)\n+{\n+\treturn fopen(\"/dev/tty\", \"w+\");\n+}\n+\n+int terminal_close(term_t term)\n+{\n+\treturn fclose(term);\n+}\n+\n+char *terminal_prompt(term_t term, const char *prompt, int echo)\n {\n \tstatic struct strbuf buf = STRBUF_INIT;\n \tint r;\n-\tFILE *fh;\n-\n-\tfh = fopen(\"/dev/tty\", \"w+\");\n-\tif (!fh)\n-\t\treturn NULL;\n+\tFILE *fh = term;\n \n \tif (!echo) {\n \t\tstruct termios t;\n@@ -64,18 +70,48 @@ char *git_terminal_prompt(const char *prompt, int echo)\n \t}\n \n \trestore_term();\n-\tfclose(fh);\n \n \tif (r == EOF)\n \t\treturn NULL;\n \treturn buf.buf;\n }\n \n+char *git_terminal_prompt(const char *prompt, int echo)\n+{\n+\tchar *ret;\n+\tterm_t term;\n+\n+\tterm = terminal_open();\n+\tif (!term)\n+\t\treturn NULL;\n+\n+\tret = terminal_prompt(term, prompt, echo);\n+\n+\tterminal_close(term);\n+\n+\treturn ret;\n+}\n+\n #else\n \n-char *git_terminal_prompt(const char *prompt, int echo)\n+term_t terminal_open()\n+{\n+\treturn isatty(0) && isatty(2);\n+}\n+\n+int terminal_close(term_t term)\n+{\n+\treturn 0;\n+}\n+\n+char *terminal_prompt(term_t term, const char *prompt, int echo)\n {\n \treturn getpass(prompt);\n }\n \n+char *git_terminal_prompt(const char *prompt, int echo)\n+{\n+\treturn terminal_prompt(prompt, echo);\n+}\n+\n #endif\ndiff --git a/compat/terminal.h b/compat/terminal.h\nindex 97db7cd..cf2aa10 100644\n--- a/compat/terminal.h\n+++ b/compat/terminal.h\n@@ -1,6 +1,16 @@\n #ifndef COMPAT_TERMINAL_H\n #define COMPAT_TERMINAL_H\n \n+#ifdef HAVE_DEV_TTY\n+typedef FILE *term_t;\n+#else\n+typedef int term_t;\n+#endif\n+\n+term_t terminal_open();\n+int terminal_close(term_t term);\n+char *terminal_prompt(term_t term, const char *prompt, int echo);\n+\n char *git_terminal_prompt(const char *prompt, int echo);\n \n #endif /* COMPAT_TERMINAL_H */\n-- \n1.7.12.rc1.187.g6dd9156\n"},{"id":"196493","messageId":"1344192340-19415-3-git-send-email-rctay89@gmail.com","threadId":"30438","inReplyTo":"1344192340-19415-2-git-send-email-rctay89@gmail.com","subject":"[PATCH v3 2/2] allow recovery from command name typos","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2012-08-05T18:45:40Z","receivedAt":"2012-08-05T18:45:40Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"If suggestions are available (based on Levenshtein distance) and if the\nterminal isatty(), present a prompt to the user to select one of the\ncomputed suggestions.\n\nIn the case where there is a single suggestion, present the prompt\n\"[Y/n]\", such that \"\" (ie. the default), \"y\" and \"Y\" as input leads git\nto proceed executing the suggestion, while everything else (possibly\n\"n\") leads git to terminate.\n\nIn the case where there are multiple suggestions, number the suggestions\n1 to (the number of suggestions), and accept an integer as input,\nwhile everything else (possibly \"n\") leads git to terminate. In this\ncase there is no default; an empty input leads git to terminate. A\nsample run:\n\n $ git sh --pretty=oneline\n git: 'sh' is not a git command. See 'git --help'.\n\n Did you mean one of these?\n 1: show\n 2: push\n [N/1/2/...]\n\nThis prompt is enabled only if help.autocorrect is set to ask; if unset,\nadvise the user about this ability.\n\nHelped-by: Thomas Rast \nSigned-off-by: Tay Ray Chuan \n---\n\nChanged in v3:\n - say do_* instead of shall_*\n - use new terminal interface\n\n Documentation/config.txt | 30 ++++++++++++++++-----\n advice.c | 2 ++\n advice.h | 1 +\n help.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---\n 4 files changed, 90 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcea8a..0bb175a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -177,6 +177,10 @@ advice.*::\n \t\tAdvice shown when you used linkgit:git-checkout[1] to\n \t\tmove to the detach HEAD state, to instruct how to create\n \t\ta local branch after the fact.\n+\ttypoPrompt::\n+\t\tUpon a mistyped command, if 'help.autocorrect' is unset\n+\t\tadvise that an interactive prompt can be displayed to\n+\t\trecover from the typo.\n --\n \n core.fileMode::\n@@ -1323,13 +1327,25 @@ 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+\tSpecifies behaviour to recover from mistyped commands.\n++\n+When set to `ask`, an interactive prompt is displayed, allowing the user\n+to select a suggested command for execution.\n++\n+When set to `off`, no attempt to recover is made.\n++\n+If a number is given, it will be interpreted as the deciseconds (0.1\n+sec) to wait before automatically correcting and executing the mistyped\n+command, with the following behaviour:\n++\n+* If more than one command can be deduced from the entered text, nothing\n+ will be executed.\n+* If the value of this option is negative, the corrected command will be\n+ executed immediately.\n+* If the value is 0 - the command will be just shown but not executed.\n++\n+The default is to display a message suggesting that this option be set\n+to `ask`, without attempting to recover (see `advice.typoPrompt`).\n \n http.proxy::\n \tOverride the HTTP proxy, normally configured using the 'http_proxy',\ndiff --git a/advice.c b/advice.c\nindex a492eea..d070a05 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -9,6 +9,7 @@ int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n int advice_implicit_identity = 1;\n int advice_detached_head = 1;\n+int advice_typo_prompt = 1;\n \n static struct {\n \tconst char *name;\n@@ -23,6 +24,7 @@ static struct {\n \t{ \"resolveconflict\", &advice_resolve_conflict },\n \t{ \"implicitidentity\", &advice_implicit_identity },\n \t{ \"detachedhead\", &advice_detached_head },\n+\t{ \"typoprompt\", &advice_typo_prompt },\n };\n \n void advise(const char *advice, ...)\ndiff --git a/advice.h b/advice.h\nindex f3cdbbf..050068d 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -12,6 +12,7 @@ extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\n extern int advice_implicit_identity;\n extern int advice_detached_head;\n+extern int advice_typo_prompt;\n \n int git_default_advice_config(const char *var, const char *value);\n void advise(const char *advice, ...);\ndiff --git a/help.c b/help.c\nindex c4285a5..cc13b92 100644\n--- a/help.c\n+++ b/help.c\n@@ -7,6 +7,7 @@\n #include \"string-list.h\"\n #include \"column.h\"\n #include \"version.h\"\n+#include \"compat/terminal.h\"\n \n void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n {\n@@ -233,12 +234,30 @@ int is_in_cmdlist(struct cmdnames *c, const char *s)\n }\n \n static int autocorrect;\n+static int do_advise = 1;\n+static int do_prompt;\n+static const char message_advice_prompt_ability[] =\n+\tN_(\"I can display an interactive prompt to proceed with one of the above\\n\"\n+\t \"suggestions; if you wish me to do so, use\\n\"\n+\t \"\\n\"\n+\t \" git config --global help.autocorrect ask\\n\"\n+\t \"\\n\"\n+\t \"See 'git help config' and search for 'help.autocorrect' for further\\n\"\n+\t \"information.\\n\");\n static struct cmdnames aliases;\n \n static int git_unknown_cmd_config(const char *var, const char *value, void *cb)\n {\n-\tif (!strcmp(var, \"help.autocorrect\"))\n-\t\tautocorrect = git_config_int(var,value);\n+\tif (!strcmp(var, \"help.autocorrect\") && value) {\n+\t\tdo_advise = 0;\n+\t\tif (!strcasecmp(value, \"off\"))\n+\t\t\t;\n+\t\telse if (!strcasecmp(value, \"ask\"))\n+\t\t\tdo_prompt = 1;\n+\t\telse\n+\t\t\tautocorrect = git_config_int(var, value);\n+\t}\n+\n \t/* Also use aliases for command lookup */\n \tif (!prefixcmp(var, \"alias.\"))\n \t\tadd_cmdname(&aliases, var + 6, strlen(var + 6));\n@@ -366,13 +385,54 @@ const char *help_unknown_cmd(const char *cmd)\n \tfprintf_ln(stderr, _(\"git: '%s' is not a git command. See 'git --help'.\"), cmd);\n \n \tif (SIMILAR_ENOUGH(best_similarity)) {\n+\t\tterm_t term;\n+\n \t\tfprintf_ln(stderr,\n \t\t\t Q_(\"\\nDid you mean this?\",\n \t\t\t \"\\nDid you mean one of these?\",\n \t\t\t n));\n \n-\t\tfor (i = 0; i < n; i++)\n-\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n+\t\tterm = terminal_open();\n+\t\tif (!term || !do_prompt) {\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n+\t\t\tif (isatty(2) && do_advise && advice_typo_prompt) {\n+\t\t\t\tfprintf(stderr, \"\\n\");\n+\t\t\t\tadvise(_(message_advice_prompt_ability));\n+\t\t\t}\n+\t\t} else if (n == 1) {\n+\t\t\tchar *in;\n+\t\t\tconst char *ret;\n+\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[0]->name);\n+\t\t\tin = terminal_prompt(term, \"[Y/n] \", 1);\n+\t\t\tterminal_close(term);\n+\t\t\tswitch (in[0]) {\n+\t\t\tcase 'y': case 'Y': case 0:\n+\t\t\t\tret = xstrdup(main_cmds.names[0]->name);\n+\t\t\t\tclean_cmdnames(&main_cmds);\n+\t\t\t\treturn ret;\n+\t\t\t/* otherwise, don't do anything */\n+\t\t\t}\n+\t\t} else {\n+\t\t\tchar *in;\n+\t\t\tconst char *ret;\n+\t\t\tint opt;\n+\t\t\tfor (i = 0; i < n; i++)\n+\t\t\t\tfprintf(stderr, \"%d:\\t%s\\n\", i + 1, main_cmds.names[i]->name);\n+\t\t\tin = terminal_prompt(term, \"[N/1/2/...] \", 1);\n+\t\t\tterminal_close(term);\n+\t\t\tswitch (in[0]) {\n+\t\t\tcase 'n': case 'N': case 0:\n+\t\t\t\t;\n+\t\t\tdefault:\n+\t\t\t\topt = atoi(in);\n+\t\t\t\tif (0 < opt && opt <= n) {\n+\t\t\t\t\tret = xstrdup(main_cmds.names[opt - 1]->name);\n+\t\t\t\t\tclean_cmdnames(&main_cmds);\n+\t\t\t\t\treturn ret;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n \t}\n \n \texit(1);\n-- \n1.7.12.rc1.187.g6dd9156\n"},{"id":"196499","messageId":"7vsjc12j5o.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"1344192340-19415-2-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH v3 1/2] add interface for /dev/tty interaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-05T20:11:47Z","receivedAt":"2012-08-05T20:11:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> Factor out the opening and closing of /dev/tty from\n> git_terminal_prompt(), so that callers may first test if a controlling\n> terminal is available before proceeding with prompting proper.\n>\n> When HAVE_DEV_TTY is not defined, terminal_open() falls back to checking\n> tty-ness of stdin and stderr, as getpass() uses them both.\n>\n> Signed-off-by: Tay Ray Chuan \n> ---\n\nThis is not your fault but seeing term_t made me go \"eek, yuck\".\n\nAs far as I can see, use of \"FILE *\" in existing compat/terminal.c\nis not buying us anything useful. The stdio calls made on FILE *fh\nare only fopen(), fputs(), fflush() and fclose(), and everything\nelse goes through fileno(fh).\n\nSo perhaps it is a saner approach to fix that function first before\nthis patch so that it works on file descriptors.\n\n> compat/terminal.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------\n> compat/terminal.h | 10 ++++++++++\n> 2 files changed, 54 insertions(+), 8 deletions(-)\n>\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index 6d16c8f..c85d5c7 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -24,15 +24,21 @@ static void restore_term_on_signal(int sig)\n> \traise(sig);\n> }\n> \n> -char *git_terminal_prompt(const char *prompt, int echo)\n> +term_t terminal_open(void)\n> +{\n> +\treturn fopen(\"/dev/tty\", \"w+\");\n> +}\n> +\n> +int terminal_close(term_t term)\n> +{\n> +\treturn fclose(term);\n> +}\n> +\n> +char *terminal_prompt(term_t term, const char *prompt, int echo)\n> {\n> \tstatic struct strbuf buf = STRBUF_INIT;\n> \tint r;\n> -\tFILE *fh;\n> -\n> -\tfh = fopen(\"/dev/tty\", \"w+\");\n> -\tif (!fh)\n> -\t\treturn NULL;\n> +\tFILE *fh = term;\n> \n> \tif (!echo) {\n> \t\tstruct termios t;\n> @@ -64,18 +70,48 @@ char *git_terminal_prompt(const char *prompt, int echo)\n> \t}\n> \n> \trestore_term();\n> -\tfclose(fh);\n> \n> \tif (r == EOF)\n> \t\treturn NULL;\n> \treturn buf.buf;\n> }\n> \n> +char *git_terminal_prompt(const char *prompt, int echo)\n> +{\n> +\tchar *ret;\n> +\tterm_t term;\n> +\n> +\tterm = terminal_open();\n> +\tif (!term)\n> +\t\treturn NULL;\n> +\n> +\tret = terminal_prompt(term, prompt, echo);\n> +\n> +\tterminal_close(term);\n> +\n> +\treturn ret;\n> +}\n> +\n> #else\n> \n> -char *git_terminal_prompt(const char *prompt, int echo)\n> +term_t terminal_open()\n> +{\n> +\treturn isatty(0) && isatty(2);\n> +}\n> +\n> +int terminal_close(term_t term)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +char *terminal_prompt(term_t term, const char *prompt, int echo)\n> {\n> \treturn getpass(prompt);\n> }\n> \n> +char *git_terminal_prompt(const char *prompt, int echo)\n> +{\n> +\treturn terminal_prompt(prompt, echo);\n> +}\n> +\n> #endif\n> diff --git a/compat/terminal.h b/compat/terminal.h\n> index 97db7cd..cf2aa10 100644\n> --- a/compat/terminal.h\n> +++ b/compat/terminal.h\n> @@ -1,6 +1,16 @@\n> #ifndef COMPAT_TERMINAL_H\n> #define COMPAT_TERMINAL_H\n> \n> +#ifdef HAVE_DEV_TTY\n> +typedef FILE *term_t;\n> +#else\n> +typedef int term_t;\n> +#endif\n> +\n> +term_t terminal_open();\n> +int terminal_close(term_t term);\n> +char *terminal_prompt(term_t term, const char *prompt, int echo);\n> +\n> char *git_terminal_prompt(const char *prompt, int echo);\n> \n> #endif /* COMPAT_TERMINAL_H */\n"},{"id":"196519","messageId":"7vehnk3kti.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"1344192340-19415-3-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH v3 2/2] allow recovery from command name typos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-06T00:50:33Z","receivedAt":"2012-08-06T00:50:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan writes:\n\n> If suggestions are available (based on Levenshtein distance) and if the\n> terminal isatty(), present a prompt to the user to select one of the\n> computed suggestions.\n>\n> In the case where there is a single suggestion, present the prompt\n> \"[Y/n]\", such that \"\" (ie. the default), \"y\" and \"Y\" as input leads git\n> to proceed executing the suggestion, while everything else (possibly\n> \"n\") leads git to terminate.\n>\n> In the case where there are multiple suggestions, number the suggestions\n> 1 to (the number of suggestions), and accept an integer as input,\n> while everything else (possibly \"n\") leads git to terminate. In this\n> case there is no default; an empty input leads git to terminate. A\n> sample run:\n>\n> $ git sh --pretty=oneline\n> git: 'sh' is not a git command. See 'git --help'.\n>\n> Did you mean one of these?\n> 1: show\n> 2: push\n> [N/1/2/...]\n>\n> This prompt is enabled only if help.autocorrect is set to ask; if unset,\n> advise the user about this ability.\n>\n> Helped-by: Thomas Rast \n> Signed-off-by: Tay Ray Chuan \n> ---\n>\n> Changed in v3:\n> - say do_* instead of shall_*\n> - use new terminal interface\n>\n> Documentation/config.txt | 30 ++++++++++++++++-----\n> advice.c | 2 ++\n> advice.h | 1 +\n> help.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---\n> 4 files changed, 90 insertions(+), 11 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 0bcea8a..0bb175a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -177,6 +177,10 @@ advice.*::\n> \t\tAdvice shown when you used linkgit:git-checkout[1] to\n> \t\tmove to the detach HEAD state, to instruct how to create\n> \t\ta local branch after the fact.\n> +\ttypoPrompt::\n> +\t\tUpon a mistyped command, if 'help.autocorrect' is unset\n> +\t\tadvise that an interactive prompt can be displayed to\n> +\t\trecover from the typo.\n> --\n\nI have a moderately strong reaction against this; \"advice\" is for\nhelping users out of common pitfalls, and we generally do not use\nthe \"advise\" mechanism to advertise random shiny features.\n\n> @@ -1323,13 +1327,25 @@ help.format::\n> ...\n> help.autocorrect::\n> +\tSpecifies behaviour to recover from mistyped commands.\n> ++\n> +When set to `ask`, an interactive prompt is displayed, allowing the user\n> +to select a suggested command for execution.\n> ++\n> +When set to `off`, no attempt to recover is made.\n\nI notice that with the current code, even if help.autocorrect is set\nto 0 to decline the guessing, we still get \"did you mean one of\nthese\" as long as the typo is similar enough to existing command.\n\nI am guessing that this new value `off` is a way to remedy the\nsituation so that users can choose to decline any guessing, and just\nget \"no such subcommand\". If that is the case, I think it is a vast\nimprovement.\n\n> +If a number is given, it will be interpreted as the deciseconds (0.1\n> +sec) to wait before automatically correcting and executing the mistyped\n> +command, with the following behaviour:\n> ++\n> +* If more than one command can be deduced from the entered text, nothing\n> + will be executed.\n\nThe above is from the original text, but I've always found the \"can\nbe deduced\" part hard to understand. It is a quite roundabout way\nto say we cannot guess with confidence what the user meant and avoid\ncommitting to a wrong guess. We may want to think a better way to\nphrase the whole thing. Perhaps something along this line:\n\n\thelp.autocorrect::\n\t\tWhen you mistype the name of a subcommand during an\n\t\tinteractive session, Git can try to guess which one\n\t\tof available subcommands you meant (Git does not\n\t\twaste cycles in a non-interactive session). This\n\t\tconfiguration variable specifies what happens when\n\t\tthere are one or more subcommands that you are\n\t\tlikely to have meant.\n\n \t- when set to 'ask', the choices are presented and\n you can pick one to execute. If the command is\n used non-interactively,\n\n\t\t- when set to `off`, ...\n\nThis can be done after this patch series settles, of course.\n\n> +* If the value of this option is negative, the corrected command will be\n> + executed immediately.\n> +* If the value is 0 - the command will be just shown but not executed.\n> ++\n> +The default is to display a message suggesting that this option be set\n> +to `ask`, without attempting to recover (see `advice.typoPrompt`).\n\nMy comment to 'advice.typoPrompt' leads me to suggest not to change\nthe default to `ask`, but leave it to 0, and remove the change to\nthe following two files.\n\n> diff --git a/advice.c b/advice.c\n> diff --git a/advice.h b/advice.h\n\n> diff --git a/help.c b/help.c\n> index c4285a5..cc13b92 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -7,6 +7,7 @@\n> #include \"string-list.h\"\n> #include \"column.h\"\n> #include \"version.h\"\n> +#include \"compat/terminal.h\"\n> \n> void add_cmdname(struct cmdnames *cmds, const char *name, int len)\n> {\n> @@ -233,12 +234,30 @@ int is_in_cmdlist(struct cmdnames *c, const char *s)\n> }\n> \n> static int autocorrect;\n> +static int do_advise = 1;\n> +static int do_prompt;\n> +static const char message_advice_prompt_ability[] =\n> +\tN_(\"I can display an interactive prompt to proceed with one of the above\\n\"\n> +\t \"suggestions; if you wish me to do so, use\\n\"\n> +\t \"\\n\"\n> +\t \" git config --global help.autocorrect ask\\n\"\n> +\t \"\\n\"\n> +\t \"See 'git help config' and search for 'help.autocorrect' for further\\n\"\n> +\t \"information.\\n\");\n> static struct cmdnames aliases;\n\nNah. No unsolicited advertisement, please.\n\n> static int git_unknown_cmd_config(const char *var, const char *value, void *cb)\n> {\n> -\tif (!strcmp(var, \"help.autocorrect\"))\n> -\t\tautocorrect = git_config_int(var,value);\n> +\tif (!strcmp(var, \"help.autocorrect\") && value) {\n> +\t\tdo_advise = 0;\n> +\t\tif (!strcasecmp(value, \"off\"))\n> +\t\t\t;\n> +\t\telse if (!strcasecmp(value, \"ask\"))\n> +\t\t\tdo_prompt = 1;\n> +\t\telse\n> +\t\t\tautocorrect = git_config_int(var, value);\n> +\t}\n\nI think the current code diagnoses\n\n\t[help]\n \tautocorrect\n\nthat tries to say \"true\" as a syntax error. The above simply\nignores such an entry, no?\n\nI was hoping \"off\" would be usable to bypass the whole levenstein\nthing, but the above code does not suggest that the remainder of\nthis patch would be doing that X-<.\n\n> @@ -366,13 +385,54 @@ const char *help_unknown_cmd(const char *cmd)\n> \tfprintf_ln(stderr, _(\"git: '%s' is not a git command. See 'git --help'.\"), cmd);\n> \n> \tif (SIMILAR_ENOUGH(best_similarity)) {\n> +\t\tterm_t term;\n> +\n> \t\tfprintf_ln(stderr,\n> \t\t\t Q_(\"\\nDid you mean this?\",\n> \t\t\t \"\\nDid you mean one of these?\",\n> \t\t\t n));\n> \n> -\t\tfor (i = 0; i < n; i++)\n> -\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n> +\t\tterm = terminal_open();\n> +\t\tif (!term || !do_prompt) {\n> +\t\t\tfor (i = 0; i < n; i++)\n> +\t\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[i]->name);\n\nIt is the same as what is done with the current code, but if there\nis no terminal available, do we even want to give this list?\n\n> +\t\t\tif (isatty(2) && do_advise && advice_typo_prompt) {\n> +\t\t\t\tfprintf(stderr, \"\\n\");\n> +\t\t\t\tadvise(_(message_advice_prompt_ability));\n> +\t\t\t}\n\nNah. No unsolicited advertisement, please.\n\n> +\t\t} else if (n == 1) {\n> +\t\t\tchar *in;\n> +\t\t\tconst char *ret;\n> +\t\t\tfprintf(stderr, \"\\t%s\\n\", main_cmds.names[0]->name);\n> +\t\t\tin = terminal_prompt(term, \"[Y/n] \", 1);\n> +\t\t\tterminal_close(term);\n> +\t\t\tswitch (in[0]) {\n> +\t\t\tcase 'y': case 'Y': case 0:\n> +\t\t\t\tret = xstrdup(main_cmds.names[0]->name);\n> +\t\t\t\tclean_cmdnames(&main_cmds);\n> +\t\t\t\treturn ret;\n\nOK.\n\n> +\t\t\t/* otherwise, don't do anything */\n> +\t\t\t}\n\nIndent the comment one level deeper?\n\n> +\t\t} else {\n> +\t\t\tchar *in;\n> +\t\t\tconst char *ret;\n> +\t\t\tint opt;\n> +\t\t\tfor (i = 0; i < n; i++)\n\nCan we have too many choices for this \"prompt\" codepath to be\npractical?\n\n> +\t\t\t\tfprintf(stderr, \"%d:\\t%s\\n\", i + 1, main_cmds.names[i]->name);\n> +\t\t\tin = terminal_prompt(term, \"[N/1/2/...] \", 1);\n\nWould it be too much trouble to spell the actual choices out here,\ninstead of the ugly \"/...\"?\n\n> +\t\t\tterminal_close(term);\n> +\t\t\tswitch (in[0]) {\n> +\t\t\tcase 'n': case 'N': case 0:\n> +\t\t\t\t;\n> +\t\t\tdefault:\n> +\t\t\t\topt = atoi(in);\n> +\t\t\t\tif (0 < opt && opt <= n) {\n> +\t\t\t\t\tret = xstrdup(main_cmds.names[opt - 1]->name);\n> +\t\t\t\t\tclean_cmdnames(&main_cmds);\n> +\t\t\t\t\treturn ret;\n> +\t\t\t\t}\n\nWhen the user mistypes the choice (perhaps say '8' when there are\nonly 7 choices available), it might be more helpful to loop here to\ngive him another chance. Would such an enhancement be worth it?\n"},{"id":"196556","messageId":"20120806194511.GB10039@sigill.intra.peff.net","threadId":"30438","inReplyTo":"7vsjc12j5o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/2] add interface for /dev/tty interaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-06T19:45:11Z","receivedAt":"2012-08-06T19:45:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 05, 2012 at 01:11:47PM -0700, Junio C Hamano wrote:\n\n> Tay Ray Chuan writes:\n> \n> > Factor out the opening and closing of /dev/tty from\n> > git_terminal_prompt(), so that callers may first test if a controlling\n> > terminal is available before proceeding with prompting proper.\n> >\n> > When HAVE_DEV_TTY is not defined, terminal_open() falls back to checking\n> > tty-ness of stdin and stderr, as getpass() uses them both.\n> >\n> > Signed-off-by: Tay Ray Chuan \n> > ---\n> \n> This is not your fault but seeing term_t made me go \"eek, yuck\".\n\nAgreed.\n\n> As far as I can see, use of \"FILE *\" in existing compat/terminal.c\n> is not buying us anything useful. The stdio calls made on FILE *fh\n> are only fopen(), fputs(), fflush() and fclose(), and everything\n> else goes through fileno(fh).\n> \n> So perhaps it is a saner approach to fix that function first before\n> this patch so that it works on file descriptors.\n\nYeah, I think that is a good path. I think my original use of stdio\nwas mostly because I started by paring down glibc's implementation of\ngetpass. Since we have niceties like write_in_full, I don't think\nthere's any reason not to just skip stdio.\n\n-Peff\n"},{"id":"196557","messageId":"20120806195616.GC10039@sigill.intra.peff.net","threadId":"30438","inReplyTo":"20120806194511.GB10039@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/2] add interface for /dev/tty interaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-06T19:56:16Z","receivedAt":"2012-08-06T19:56:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2012 at 03:45:11PM -0400, Jeff King wrote:\n\n> > This is not your fault but seeing term_t made me go \"eek, yuck\".\n> \n> Agreed.\n> \n> > As far as I can see, use of \"FILE *\" in existing compat/terminal.c\n> > is not buying us anything useful. The stdio calls made on FILE *fh\n> > are only fopen(), fputs(), fflush() and fclose(), and everything\n> > else goes through fileno(fh).\n> > \n> > So perhaps it is a saner approach to fix that function first before\n> > this patch so that it works on file descriptors.\n> \n> Yeah, I think that is a good path. I think my original use of stdio\n> was mostly because I started by paring down glibc's implementation of\n> getpass. Since we have niceties like write_in_full, I don't think\n> there's any reason not to just skip stdio.\n\nI forgot to mention: even if we changed the HAVE_DEV_TTY code path to\nuse an integer descriptor (which I think is a sane thing to do\nregardless), that may not be sufficient to solve this problem.\n\nErik has looked into doing a Windows alternative in compat/terminal.c,\nand I think an integer would be insufficient there. In particular, I\nthink Windows needs two descriptors to accomplish the same thing (one\nfor CONIN$ and one for CONOUT$). So you'd need to turn term_t into a\nstruct (and you'd probably not want to return it by value then).\n\nMaybe it would be better to keep the abstraction as non-leaky as\npossible, and just provide \"terminal_can_prompt()\" or similar?\n\nReturning the open descriptor (as Tay's patch does) avoids a race\ncondition where /dev/tty can be opened when terminal_can_prompt runs,\nbut not when we try to actually read from it. But we can either:\n\n 1. Not care. Even if the tty is opened, if a user has closed the\n terminal we are going to get a read error anyway. So you can never\n avoid that race condition in some form.\n\n 2. Open the terminal descriptor when either function is called, and\n never close it. I don't think there is any reason we can't just\n leak the descriptor.\n\n-Peff\n"},{"id":"196559","messageId":"7v1ujjzt5p.fsf@alter.siamese.dyndns.org","threadId":"30438","inReplyTo":"20120806195616.GC10039@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/2] add interface for /dev/tty interaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-06T20:01:38Z","receivedAt":"2012-08-06T20:01:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Erik has looked into doing a Windows alternative in compat/terminal.c,\n> and I think an integer would be insufficient there. In particular, I\n> think Windows needs two descriptors to accomplish the same thing (one\n> for CONIN$ and one for CONOUT$). So you'd need to turn term_t into a\n> struct (and you'd probably not want to return it by value then).\n>\n> Maybe it would be better to keep the abstraction as non-leaky as\n> possible, and just provide \"terminal_can_prompt()\" or similar?\n\nOK. As we won't be giving separate instances of terminals to\ndifferent callers anyway, compat/terminal.c can keep a static\nvariable of whatever type that is necessary for the implementation\naround. That sounds like a reasonable way to go.\n\n> Returning the open descriptor (as Tay's patch does) avoids a race\n> condition where /dev/tty can be opened when terminal_can_prompt runs,\n> but not when we try to actually read from it. But we can either:\n>\n> 1. Not care. Even if the tty is opened, if a user has closed the\n> terminal we are going to get a read error anyway. So you can never\n> avoid that race condition in some form.\n>\n> 2. Open the terminal descriptor when either function is called, and\n> never close it. I don't think there is any reason we can't just\n> leak the descriptor.\n>\n> -Peff\n"}]}