* [PATCH 0/4] allow recovery from command name typos @ 2012-05-06 6:55 Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 1/4] help.c::uniq: plug a leak Tay Ray Chuan ` (2 more replies) 0 siblings, 3 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 6:55 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano Patch 4 has the meat of this series. While running valgrind to check that I didn't leak any memory, a couple of leaks were spotted. Patches 1-3 address them. Tay Ray Chuan (4): help.c::uniq: plug a leak help.c::exclude_cmds: plug a leak help.c: plug a leak when help.autocorrect is set allow recovery from command name typos help.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 62 insertions(+), 12 deletions(-) -- 1.7.10.1.611.g8a79d96 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 1/4] help.c::uniq: plug a leak 2012-05-06 6:55 [PATCH 0/4] allow recovery from command name typos Tay Ray Chuan @ 2012-05-06 6:55 ` Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 2/4] help.c::exclude_cmds: " Tay Ray Chuan 2012-05-06 8:12 ` [PATCH 1/4] help.c::uniq: plug a leak Jeff King 2012-07-25 16:16 ` [PATCH v2 0/4] allow recovery from command name typos Tay Ray Chuan 2012-08-05 18:45 ` [PATCH v3 0/2] allow recovery from command name typos Tay Ray Chuan 2 siblings, 2 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 6:55 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- help.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index 69d483d..b64056d 100644 --- a/help.c +++ b/help.c @@ -38,14 +38,24 @@ static int cmdname_compare(const void *a_, const void *b_) static void uniq(struct cmdnames *cmds) { - int i, j; + int i, j, c = 0; if (!cmds->cnt) return; for (i = j = 1; i < cmds->cnt; i++) - if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) + if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) { + + /* The i-1 entry was the cth duplicate + * Guarantees c=0 + */ + for (; c >= 1; c--) + free(cmds->names[i - c]); + cmds->names[j++] = cmds->names[i]; + } else { + c++; + } cmds->cnt = j; } -- 1.7.10.1.611.g8a79d96 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 2/4] help.c::exclude_cmds: plug a leak 2012-05-06 6:55 ` [PATCH 1/4] help.c::uniq: plug a leak Tay Ray Chuan @ 2012-05-06 6:55 ` Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 3/4] help.c: plug a leak when help.autocorrect is set Tay Ray Chuan 2012-05-06 8:12 ` [PATCH 1/4] help.c::uniq: plug a leak Jeff King 1 sibling, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 6:55 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano Create a fresh cmdnames to hold the entries we want to keep, such that we free the excluded entries in cmds only. Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- A solution that does not require a fresh cmdnames escapes me. --- help.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/help.c b/help.c index b64056d..705f152 100644 --- a/help.c +++ b/help.c @@ -65,21 +65,29 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) int ci, cj, ei; int cmp; + struct cmdnames excluded; + memset(&excluded, 0, sizeof(excluded)); + ALLOC_GROW(excluded.names, cmds->cnt, excluded.alloc); + ci = cj = ei = 0; while (ci < cmds->cnt && ei < excludes->cnt) { cmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name); - if (cmp < 0) - cmds->names[cj++] = cmds->names[ci++]; - else if (cmp == 0) + if (cmp < 0) { + excluded.names[cj] = cmds->names[ci]; + cmds->names[ci] = NULL; + ci++, cj++; + } else if (cmp == 0) ci++, ei++; else if (cmp > 0) ei++; } - while (ci < cmds->cnt) - cmds->names[cj++] = cmds->names[ci++]; - + clean_cmdnames(cmds); + cmds->alloc = excluded.alloc; cmds->cnt = cj; + cmds->names = excluded.names; + while (cj--) + cmds->names[cj] = excluded.names[cj]; } static void pretty_print_string_list(struct cmdnames *cmds, -- 1.7.10.1.611.g8a79d96 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 3/4] help.c: plug a leak when help.autocorrect is set 2012-05-06 6:55 ` [PATCH 2/4] help.c::exclude_cmds: " Tay Ray Chuan @ 2012-05-06 6:55 ` Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 4/4] allow recovery from command name typos Tay Ray Chuan 0 siblings, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 6:55 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano In an attempt to retain the memory to the string name in main_cmds, we unfortunately leaked the struct cmdname that held it. Fix this by creating a copy of the name. Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- help.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/help.c b/help.c index 705f152..f296d95 100644 --- a/help.c +++ b/help.c @@ -360,8 +360,7 @@ const char *help_unknown_cmd(const char *cmd) ; /* still counting */ } if (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) { - const char *assumed = main_cmds.names[0]->name; - main_cmds.names[0] = NULL; + const char *assumed = xstrdup(main_cmds.names[0]->name); clean_cmdnames(&main_cmds); fprintf_ln(stderr, _("WARNING: You called a Git command named '%s', " -- 1.7.10.1.611.g8a79d96 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 4/4] allow recovery from command name typos 2012-05-06 6:55 ` [PATCH 3/4] help.c: plug a leak when help.autocorrect is set Tay Ray Chuan @ 2012-05-06 6:55 ` Tay Ray Chuan 2012-05-06 8:21 ` Jeff King [not found] ` <CAOBOgRaDEgAqXWmdC6hrudkL5OwzeMffbj2RtKMxf2TsYWzotA@mail.gmail.com> 0 siblings, 2 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 6:55 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano If suggestions are available (based on Levenshtein distance) and if the terminal isatty(), present a prompt to the user to select one of the computed suggestions. In the case where there is a single suggestion, present the prompt "[Y/n]", such that "", "y" and "Y" as input leads git to proceed executing the suggestion, while everything else (possibly "n") leads git to terminate. In the case where there are multiple suggestions, number the suggestions 1 to n, and accept as input one of the numbers, while everything else (possibly "n") leads git to terminate. In this case there is no default; that is, an empty input leads git to terminate. A sample run: $ git sh --pretty=oneline git: 'sh' is not a git command. See 'git --help'. Did you mean one of these? 1: show 2: push [1/2/.../n] 1 Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- help.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index f296d95..4aa1d88 100644 --- a/help.c +++ b/help.c @@ -6,6 +6,7 @@ #include "common-cmds.h" #include "string-list.h" #include "column.h" +#include "compat/terminal.h" void add_cmdname(struct cmdnames *cmds, const char *name, int len) { @@ -383,8 +384,40 @@ const char *help_unknown_cmd(const char *cmd) "\nDid you mean one of these?", n)); - for (i = 0; i < n; i++) - fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); + if (!isatty(2)) + for (i = 0; i < n; i++) + fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); + else if (n == 1) { + char *in; + const char *ret; + fprintf(stderr, "\t%s\n", main_cmds.names[0]->name); + in = git_terminal_prompt("[Y/n] ", 1); + switch (in[0]) { + case 'y': case 'Y': case 0: + ret = xstrdup(main_cmds.names[0]->name); + clean_cmdnames(&main_cmds); + return ret; + /* otherwise, don't do anything */ + } + } else { + char *in; + const char *ret; + int opt; + for (i = 0; i < n; i++) + fprintf(stderr, "%d:\t%s\n", i + 1, main_cmds.names[i]->name); + in = git_terminal_prompt("[1/2/.../n] ", 1); + switch (in[0]) { + case 'n': case 'N': case 0: + ; + default: + opt = atoi(in); + if (0 < opt && opt <= n) { + ret = xstrdup(main_cmds.names[opt - 1]->name); + clean_cmdnames(&main_cmds); + return ret; + } + } + } } exit(1); -- 1.7.10.1.611.g8a79d96 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] allow recovery from command name typos 2012-05-06 6:55 ` [PATCH 4/4] allow recovery from command name typos Tay Ray Chuan @ 2012-05-06 8:21 ` Jeff King 2012-05-06 16:07 ` Tay Ray Chuan [not found] ` <CAOBOgRaDEgAqXWmdC6hrudkL5OwzeMffbj2RtKMxf2TsYWzotA@mail.gmail.com> 1 sibling, 1 reply; 37+ messages in thread From: Jeff King @ 2012-05-06 8:21 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Junio C Hamano On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote: > If suggestions are available (based on Levenshtein distance) and if the > terminal isatty(), present a prompt to the user to select one of the > computed suggestions. > > In the case where there is a single suggestion, present the prompt > "[Y/n]", such that "", "y" and "Y" as input leads git to proceed > executing the suggestion, while everything else (possibly "n") leads git > to terminate. > > In the case where there are multiple suggestions, number the suggestions > 1 to n, and accept as input one of the numbers, while everything else > (possibly "n") leads git to terminate. In this case there is no default; > that is, an empty input leads git to terminate. A sample run: > > $ git sh --pretty=oneline > git: 'sh' is not a git command. See 'git --help'. > > Did you mean one of these? > 1: show > 2: push > [1/2/.../n] 1 Ugh. Please protect this with a config variable that defaults to "off". It is very un-Unix to prompt unexpectedly, and I suspect a lot of people would be annoyed by this behavior changing by default (I know I would be). -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] allow recovery from command name typos 2012-05-06 8:21 ` Jeff King @ 2012-05-06 16:07 ` Tay Ray Chuan 2012-05-07 9:43 ` Thomas Rast 0 siblings, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 16:07 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano On Sun, May 6, 2012 at 4:21 PM, Jeff King <peff@peff.net> wrote: > On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote: > >> If suggestions are available (based on Levenshtein distance) and if the >> terminal isatty(), present a prompt to the user to select one of the >> computed suggestions. >> >> In the case where there is a single suggestion, present the prompt >> "[Y/n]", such that "", "y" and "Y" as input leads git to proceed >> executing the suggestion, while everything else (possibly "n") leads git >> to terminate. >> >> In the case where there are multiple suggestions, number the suggestions >> 1 to n, and accept as input one of the numbers, while everything else >> (possibly "n") leads git to terminate. In this case there is no default; >> that is, an empty input leads git to terminate. A sample run: >> >> $ git sh --pretty=oneline >> git: 'sh' is not a git command. See 'git --help'. >> >> Did you mean one of these? >> 1: show >> 2: push >> [1/2/.../n] 1 > > Ugh. Please protect this with a config variable that defaults to > "off". It is very un-Unix to prompt unexpectedly, and I suspect a lot > of people would be annoyed by this behavior changing by default (I know > I would be). > > -Peff While I agree there should be a config to protect this, I was hoping this would be useful to users on the terminal who make the occasional slip-up, without having to do any prior configuration. -- Cheers, Ray Chuan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] allow recovery from command name typos 2012-05-06 16:07 ` Tay Ray Chuan @ 2012-05-07 9:43 ` Thomas Rast 2012-05-07 15:49 ` Tay Ray Chuan 2012-05-07 17:41 ` Junio C Hamano 0 siblings, 2 replies; 37+ messages in thread From: Thomas Rast @ 2012-05-07 9:43 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Jeff King, Git Mailing List, Junio C Hamano Tay Ray Chuan <rctay89@gmail.com> writes: > On Sun, May 6, 2012 at 4:21 PM, Jeff King <peff@peff.net> wrote: >> On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote: >> >>> $ git sh --pretty=oneline >>> git: 'sh' is not a git command. See 'git --help'. >>> >>> Did you mean one of these? >>> 1: show >>> 2: push >>> [1/2/.../n] 1 >> >> Ugh. Please protect this with a config variable that defaults to >> "off". It is very un-Unix to prompt unexpectedly, and I suspect a lot >> of people would be annoyed by this behavior changing by default (I know >> I would be). >> >> -Peff > > While I agree there should be a config to protect this, I was hoping > this would be useful to users on the terminal who make the occasional > slip-up, without having to do any prior configuration. We already have help.autocorrect. It defaults to 0, which results in $ g rebest git: 'rebest' is not a git command. See 'git --help'. Did you mean one of these? rebase reset revert But it can also be a timeout in deciseconds, after which the match is automatically executed (if there is only one). You could hijack it by * making 'ask' mean your new feature * making 'off' etc. be the same as 0 for sanity * making the default value be like 0, but with an extra message such as Use 'git config --global help.autocorrect ask' to let me prompt for the correct command. though I'm sure you can improve on the wording. -- Thomas Rast trast@{inf,student}.ethz.ch ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] allow recovery from command name typos 2012-05-07 9:43 ` Thomas Rast @ 2012-05-07 15:49 ` Tay Ray Chuan 2012-05-07 17:41 ` Junio C Hamano 1 sibling, 0 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-07 15:49 UTC (permalink / raw) To: Thomas Rast; +Cc: Jeff King, Git Mailing List, Junio C Hamano On Mon, May 7, 2012 at 5:43 PM, Thomas Rast <trast@student.ethz.ch> wrote: > Tay Ray Chuan <rctay89@gmail.com> writes: > >> On Sun, May 6, 2012 at 4:21 PM, Jeff King <peff@peff.net> wrote: >>> On Sun, May 06, 2012 at 02:55:30PM +0800, Tay Ray Chuan wrote: >>> >>>> $ git sh --pretty=oneline >>>> git: 'sh' is not a git command. See 'git --help'. >>>> >>>> Did you mean one of these? >>>> 1: show >>>> 2: push >>>> [1/2/.../n] 1 >>> >>> Ugh. Please protect this with a config variable that defaults to >>> "off". It is very un-Unix to prompt unexpectedly, and I suspect a lot >>> of people would be annoyed by this behavior changing by default (I know >>> I would be). >>> >>> -Peff >> >> While I agree there should be a config to protect this, I was hoping >> this would be useful to users on the terminal who make the occasional >> slip-up, without having to do any prior configuration. > > We already have help.autocorrect. It defaults to 0, which results in > > $ g rebest > git: 'rebest' is not a git command. See 'git --help'. > > Did you mean one of these? > rebase > reset > revert > > But it can also be a timeout in deciseconds, after which the match is > automatically executed (if there is only one). You could hijack it by > > * making 'ask' mean your new feature > > * making 'off' etc. be the same as 0 for sanity > > * making the default value be like 0, but with an extra message such as > > Use 'git config --global help.autocorrect ask' to let me prompt for > the correct command. > > though I'm sure you can improve on the wording. Thomas, that's a brilliant idea. Re-roll coming up. -- Cheers, Ray Chuan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] allow recovery from command name typos 2012-05-07 9:43 ` Thomas Rast 2012-05-07 15:49 ` Tay Ray Chuan @ 2012-05-07 17:41 ` Junio C Hamano 2012-05-09 15:06 ` Tay Ray Chuan 1 sibling, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2012-05-07 17:41 UTC (permalink / raw) To: Thomas Rast; +Cc: Tay Ray Chuan, Jeff King, Git Mailing List Thomas Rast <trast@student.ethz.ch> writes: > We already have help.autocorrect. It defaults to 0, which results in > > $ g rebest > git: 'rebest' is not a git command. See 'git --help'. > > Did you mean one of these? > rebase > reset > revert > > But it can also be a timeout in deciseconds, after which the match is > automatically executed (if there is only one). You could hijack it by > > * making 'ask' mean your new feature > > * making 'off' etc. be the same as 0 for sanity > > * making the default value be like 0, but with an extra message such as > > Use 'git config --global help.autocorrect ask' to let me prompt for > the correct command. > > though I'm sure you can improve on the wording. Sounds good. By the way, does anybody actually use the deciseconds grace period to ^C the process? I know I was the guilty party for suggesting it, but it strikes me that it is rather a dangerous option. When checking out another branch with great difference with "git chekcout foo", you would be asked "did you mean checkout?", and if you hit ^C a bit too late, you may not kill autocorrect but end up killing a lengthy "checkout" in the middle, messing up the working tree with a mixture of files in old and new branches, needing a "reset --hard" to recover. We might want to update the documentation to warn about this, even though I personally do not think it is worth removing the support (and going through the trouble of having to deal with "why did you remove the useful feature" complaints). ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] allow recovery from command name typos 2012-05-07 17:41 ` Junio C Hamano @ 2012-05-09 15:06 ` Tay Ray Chuan 2012-05-09 17:03 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-09 15:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Thomas Rast, Jeff King, Git Mailing List On Tue, May 8, 2012 at 1:41 AM, Junio C Hamano <gitster@pobox.com> wrote: > By the way, does anybody actually use the deciseconds grace period to ^C > the process? I know I was the guilty party for suggesting it, but it > strikes me that it is rather a dangerous option. When checking out > another branch with great difference with "git chekcout foo", you would be > asked "did you mean checkout?", and if you hit ^C a bit too late, you may > not kill autocorrect but end up killing a lengthy "checkout" in the > middle, messing up the working tree with a mixture of files in old and new > branches, needing a "reset --hard" to recover. We might want to update > the documentation to warn about this, even though I personally do not > think it is worth removing the support (and going through the trouble of > having to deal with "why did you remove the useful feature" complaints). > Actually, I've never heard of that feature, until I was reading help.c. However, it's listed on Progit [1], so I'd imagine there'd be *some* users in the wild. [1] http://git-scm.com/book/ch7-1.html Personally, I think it's a little dangerous - imagine your script has a typo'd command that just runs anyway if help.autocorrect without any chance for user intervention. Perhaps there should be a isatty(2) check to guard it, like the prompting patch does. -- Cheers, Ray Chuan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] allow recovery from command name typos 2012-05-09 15:06 ` Tay Ray Chuan @ 2012-05-09 17:03 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-05-09 17:03 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Thomas Rast, Jeff King, Git Mailing List Tay Ray Chuan <rctay89@gmail.com> writes: > Actually, I've never heard of that feature, until I was reading help.c. > > However, it's listed on Progit [1], so I'd imagine there'd be *some* > users in the wild. > > [1] http://git-scm.com/book/ch7-1.html > > Personally, I think it's a little dangerous - imagine your script has > a typo'd command that just runs anyway if help.autocorrect without any > chance for user intervention. Perhaps there should be a isatty(2) > check to guard it, like the prompting patch does. The whole "did you mean one of these" autocorrection should trigger only in interactive to begin with, I would have thought. Are you saying that we don't have isatty(3) check in the early in the codepath already? In any case, we drifted into a tangent without seeing the patch series to completion. Are you rerolling with Peff's fixups, Peff hinted he is willing to do a re-post, and are you counting on it, or should I just pick up the pieces? Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
[parent not found: <CAOBOgRaDEgAqXWmdC6hrudkL5OwzeMffbj2RtKMxf2TsYWzotA@mail.gmail.com>]
* Re: [PATCH 4/4] allow recovery from command name typos [not found] ` <CAOBOgRaDEgAqXWmdC6hrudkL5OwzeMffbj2RtKMxf2TsYWzotA@mail.gmail.com> @ 2012-05-06 16:04 ` Tay Ray Chuan 0 siblings, 0 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 16:04 UTC (permalink / raw) To: Angus Hammond; +Cc: Jeff King, Git Mailing List, Junio C Hamano On Sun, May 6, 2012 at 4:40 PM, Angus Hammond <angusgh@gmail.com> wrote: > On 6 May 2012 07:55, Tay Ray Chuan <rctay89@gmail.com> wrote: >> >> In the case where there is a single suggestion, present the prompt >> "[Y/n]", such that "", "y" and "Y" as input leads git to proceed >> executing the suggestion, while everything else (possibly "n") leads git >> to terminate. >> > > Minor point, as well as ensuring this is configurable behavior, if > terminating is the default, then the prompt should be "[y/N]", so that the > default action is clearly marked. Not capitalising at all would be > reasonable, but making the 'Y' uppercase is actively confusing. I believe you were referring to the 1/2/.../n case, since for the y/n case, terminating is not the default. If so, then yes, the "n" there should be in caps, since terminating is the default; my bad. > Secondly, if we're at a tty, I suspect this behavior would be totally > unnecessary. Making close suggestions rather than just a complete list is > neat, but if people want to use one of them all they have to do is copy > paste the old command down and modify it, which I suspect would be much > faster than actually considering and responding to a prompt. I believe that there would be very few options to choose from - in fact, I was trying very hard to get the number of suggestions to equal or exceed 5. I guess one would have better luck than I if they studied the Levenshtein distance algorithm, which I didn't. In other words - the time to consider is small. In fact I was hoping this would be faster than copy-paste - typing the option (1 key) and enter (1 key) makes a total of 2 keys only. -- Cheers, Ray Chuan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/4] help.c::uniq: plug a leak 2012-05-06 6:55 ` [PATCH 1/4] help.c::uniq: plug a leak Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 2/4] help.c::exclude_cmds: " Tay Ray Chuan @ 2012-05-06 8:12 ` Jeff King 2012-05-06 15:54 ` Tay Ray Chuan 1 sibling, 1 reply; 37+ messages in thread From: Jeff King @ 2012-05-06 8:12 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Junio C Hamano On Sun, May 06, 2012 at 02:55:27PM +0800, Tay Ray Chuan wrote: > static void uniq(struct cmdnames *cmds) > { > - int i, j; > + int i, j, c = 0; > > if (!cmds->cnt) > return; > > for (i = j = 1; i < cmds->cnt; i++) > - if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) > + if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) { > + > + /* The i-1 entry was the cth duplicate > + * Guarantees c=0 > + */ > + for (; c >= 1; c--) > + free(cmds->names[i - c]); > + > cmds->names[j++] = cmds->names[i]; > + } else { > + c++; > + } > > cmds->cnt = j; > } Freeing the strings at the end of each run of duplicates is confusing to read. And your implementation is buggy: if there are duplicates at the very end of the list, you would never free them (you would need to check 'c' again at the end of the loop). I think you avoided freeing as you go because that invalidates the i-1 element that we use in the comparison. However, we can observe that the j-1 element can serve the same purpose, as it is either: 1. Exactly i-1, when the loop begins (and until we see a duplicate). 2. The same pointer that was stored at i-1 (if it was not a duplicate, and we just copied it into place). 3. A pointer to an equivalent string (i.e., we rejected i-1 _because_ it was identical to j-1). So this shorter patch should be sufficient (though I didn't actually test it): diff --git a/help.c b/help.c index 69d483d..d3868b3 100644 --- a/help.c +++ b/help.c @@ -43,9 +43,12 @@ static void uniq(struct cmdnames *cmds) if (!cmds->cnt) return; - for (i = j = 1; i < cmds->cnt; i++) - if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) + for (i = j = 1; i < cmds->cnt; i++) { + if (!strcmp(cmds->names[i]->name, cmds->names[j-1]->name)) + free(cmds->names[i]); + else cmds->names[j++] = cmds->names[i]; + } cmds->cnt = j; } ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 1/4] help.c::uniq: plug a leak 2012-05-06 8:12 ` [PATCH 1/4] help.c::uniq: plug a leak Jeff King @ 2012-05-06 15:54 ` Tay Ray Chuan 2012-05-07 7:30 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-05-06 15:54 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano On Sun, May 6, 2012 at 4:12 PM, Jeff King <peff@peff.net> wrote: > So this shorter patch should be sufficient (though I didn't actually > test it): Tested and works fine. > diff --git a/help.c b/help.c > index 69d483d..d3868b3 100644 > --- a/help.c > +++ b/help.c > @@ -43,9 +43,12 @@ static void uniq(struct cmdnames *cmds) > if (!cmds->cnt) > return; > > - for (i = j = 1; i < cmds->cnt; i++) > - if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) > + for (i = j = 1; i < cmds->cnt; i++) { > + if (!strcmp(cmds->names[i]->name, cmds->names[j-1]->name)) > + free(cmds->names[i]); > + else > cmds->names[j++] = cmds->names[i]; > + } > > cmds->cnt = j; > } Not only is this better than mine in terms of readability, it is better than the original code. -- Cheers, Ray Chuan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 1/4] help.c::uniq: plug a leak 2012-05-06 15:54 ` Tay Ray Chuan @ 2012-05-07 7:30 ` Jeff King 0 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2012-05-07 7:30 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Junio C Hamano On Sun, May 06, 2012 at 11:54:20PM +0800, Tay Ray Chuan wrote: > On Sun, May 6, 2012 at 4:12 PM, Jeff King <peff@peff.net> wrote: > > So this shorter patch should be sufficient (though I didn't actually > > test it): > > Tested and works fine. Thanks. Do you want to just include it in your re-roll, or should I pick it up and re-post it with a commit message? -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 0/4] allow recovery from command name typos 2012-05-06 6:55 [PATCH 0/4] allow recovery from command name typos Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 1/4] help.c::uniq: plug a leak Tay Ray Chuan @ 2012-07-25 16:16 ` Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 1/4] help.c::uniq: plug a leak Tay Ray Chuan 2012-08-05 18:45 ` [PATCH v3 0/2] allow recovery from command name typos Tay Ray Chuan 2 siblings, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-07-25 16:16 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King, Thomas Rast Patch 4 has the meat of this series. While running valgrind to check that I didn't leak any memory, a couple of leaks were spotted. Patches 1-3 address them. Major change in v2: implement Thomas' idea [1] about using help.autocorrect to configure this behaviour. [1] <878vh4con4.fsf@thomas.inf.ethz.ch> Jeff King (1): help.c::uniq: plug a leak Tay Ray Chuan (3): help.c::exclude_cmds: realloc() before copy, plug a leak help.c: plug leaks with(out) help.autocorrect allow recovery from command name typos Documentation/config.txt | 30 ++++++++++++---- advice.c | 2 ++ advice.h | 1 + help.c | 94 ++++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 110 insertions(+), 17 deletions(-) -- 1.7.11.1.116.g8228a23 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 1/4] help.c::uniq: plug a leak 2012-07-25 16:16 ` [PATCH v2 0/4] allow recovery from command name typos Tay Ray Chuan @ 2012-07-25 16:16 ` Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, " Tay Ray Chuan 0 siblings, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-07-25 16:16 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King, Thomas Rast From: Jeff King <peff@peff.net> We observe that the j-1 element can serve the same purpose as the i-1 element that we use in the strcmp(); it is either: 1. Exactly i-1, when the loop begins (and until we see a duplicate). 2. The same pointer that was stored at i-1 (if it was not a duplicate, and we just copied it into place). 3. A pointer to an equivalent string (i.e., we rejected i-1 _because_ it was identical to j-1). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- Changed in v2: used Jeff's code from [1]. Patch text was also based on it. [1] <20120506081213.GA27878@sigill.intra.peff.net> --- help.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index 662349d..6991492 100644 --- a/help.c +++ b/help.c @@ -44,9 +44,12 @@ static void uniq(struct cmdnames *cmds) if (!cmds->cnt) return; - for (i = j = 1; i < cmds->cnt; i++) - if (strcmp(cmds->names[i]->name, cmds->names[i-1]->name)) + for (i = j = 1; i < cmds->cnt; i++) { + if (!strcmp(cmds->names[i]->name, cmds->names[j-1]->name)) + free(cmds->names[i]); + else cmds->names[j++] = cmds->names[i]; + } cmds->cnt = j; } -- 1.7.11.1.116.g8228a23 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak 2012-07-25 16:16 ` [PATCH v2 1/4] help.c::uniq: plug a leak Tay Ray Chuan @ 2012-07-25 16:16 ` Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Tay Ray Chuan 2012-07-25 17:39 ` [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak Junio C Hamano 0 siblings, 2 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-07-25 16:16 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King, Thomas Rast Copying with structural assignment may not take into account that the LHS struct has sufficient memory, especially since the cmdname->name member is nonfixed in size. Be unambiguous about it by realloc()'ing it to be of sufficient size. Additionally, free the unused cmdnames, which are no longer accessible anyway. Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- help.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index 6991492..dfb2e9d 100644 --- a/help.c +++ b/help.c @@ -20,6 +20,17 @@ void add_cmdname(struct cmdnames *cmds, const char *name, int len) cmds->names[cmds->cnt++] = ent; } +static void copy_cmdname(struct cmdname **dest, struct cmdname *src) +{ + struct cmdname *ent = xrealloc(*dest, sizeof(*ent) + src->len + 1); + + ent->len = src->len; + memcpy(ent->name, src->name, src->len); + ent->name[src->len] = 0; + + *dest = ent; +} + static void clean_cmdnames(struct cmdnames *cmds) { int i; @@ -58,20 +69,25 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) { int ci, cj, ei; int cmp; + int last_cj; ci = cj = ei = 0; while (ci < cmds->cnt && ei < excludes->cnt) { cmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name); if (cmp < 0) - cmds->names[cj++] = cmds->names[ci++]; + copy_cmdname(&cmds->names[cj++], cmds->names[ci++]); else if (cmp == 0) ci++, ei++; else if (cmp > 0) ei++; } + last_cj = cj; while (ci < cmds->cnt) - cmds->names[cj++] = cmds->names[ci++]; + copy_cmdname(&cmds->names[cj++], cmds->names[ci++]); + + while (last_cj < cmds->cnt) + free(cmds->names[last_cj++]); cmds->cnt = cj; } -- 1.7.11.1.116.g8228a23 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect 2012-07-25 16:16 ` [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, " Tay Ray Chuan @ 2012-07-25 16:16 ` Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 4/4] allow recovery from command name typos Tay Ray Chuan 2012-07-25 17:47 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Junio C Hamano 2012-07-25 17:39 ` [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak Junio C Hamano 1 sibling, 2 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-07-25 16:16 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King, Thomas Rast When help.autocorrect is set, in an attempt to retain the memory to the string name in main_cmds, we unfortunately leaked the struct cmdname that held it. Fix this by creating a copy of the string name. When help.autocorrect is not set, we exit()'d without free'ing it like we do when the config is set; fix this. Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- Changed in v2: plug leak when help.autocorrect is not set. --- help.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/help.c b/help.c index dfb2e9d..ee261f4 100644 --- a/help.c +++ b/help.c @@ -362,8 +362,7 @@ const char *help_unknown_cmd(const char *cmd) ; /* still counting */ } if (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) { - const char *assumed = main_cmds.names[0]->name; - main_cmds.names[0] = NULL; + const char *assumed = xstrdup(main_cmds.names[0]->name); clean_cmdnames(&main_cmds); fprintf_ln(stderr, _("WARNING: You called a Git command named '%s', " @@ -390,6 +389,7 @@ const char *help_unknown_cmd(const char *cmd) fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); } + clean_cmdnames(&main_cmds); exit(1); } -- 1.7.11.1.116.g8228a23 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v2 4/4] allow recovery from command name typos 2012-07-25 16:16 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Tay Ray Chuan @ 2012-07-25 16:16 ` Tay Ray Chuan 2012-07-25 17:57 ` Junio C Hamano 2012-07-25 17:47 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-07-25 16:16 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King, Thomas Rast If suggestions are available (based on Levenshtein distance) and if the terminal isatty(), present a prompt to the user to select one of the computed suggestions. In the case where there is a single suggestion, present the prompt "[Y/n]", such that "" (ie. the default), "y" and "Y" as input leads git to proceed executing the suggestion, while everything else (possibly "n") leads git to terminate. In the case where there are multiple suggestions, number the suggestions 1 to <n> (the number of suggestions), and accept an integer as input, while everything else (possibly "n") leads git to terminate. In this case there is no default; an empty input leads git to terminate. A sample run: $ git sh --pretty=oneline git: 'sh' is not a git command. See 'git --help'. Did you mean one of these? 1: show 2: push [N/1/2/...] This prompt is enabled only if help.autocorrect is set to ask; if unset, advise the user about this ability. Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- Changed in v2: implement Thomas' idea [1] to hijack help.autocorrect to configure this behaviour. [1] <878vh4con4.fsf@thomas.inf.ethz.ch> --- Documentation/config.txt | 30 +++++++++++++++++------ advice.c | 2 ++ advice.h | 1 + help.c | 63 +++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 85 insertions(+), 11 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 0bcea8a..0bb175a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -177,6 +177,10 @@ advice.*:: Advice shown when you used linkgit:git-checkout[1] to move to the detach HEAD state, to instruct how to create a local branch after the fact. + typoPrompt:: + Upon a mistyped command, if 'help.autocorrect' is unset + advise that an interactive prompt can be displayed to + recover from the typo. -- core.fileMode:: @@ -1323,13 +1327,25 @@ help.format:: the default. 'web' and 'html' are the same. help.autocorrect:: - Automatically correct and execute mistyped commands after - waiting for the given number of deciseconds (0.1 sec). If more - than one command can be deduced from the entered text, nothing - will be executed. If the value of this option is negative, - the corrected command will be executed immediately. If the - value is 0 - the command will be just shown but not executed. - This is the default. + Specifies behaviour to recover from mistyped commands. ++ +When set to `ask`, an interactive prompt is displayed, allowing the user +to select a suggested command for execution. ++ +When set to `off`, no attempt to recover is made. ++ +If a number is given, it will be interpreted as the deciseconds (0.1 +sec) to wait before automatically correcting and executing the mistyped +command, with the following behaviour: ++ +* If more than one command can be deduced from the entered text, nothing + will be executed. +* If the value of this option is negative, the corrected command will be + executed immediately. +* If the value is 0 - the command will be just shown but not executed. ++ +The default is to display a message suggesting that this option be set +to `ask`, without attempting to recover (see `advice.typoPrompt`). http.proxy:: Override the HTTP proxy, normally configured using the 'http_proxy', diff --git a/advice.c b/advice.c index a492eea..d070a05 100644 --- a/advice.c +++ b/advice.c @@ -9,6 +9,7 @@ int advice_commit_before_merge = 1; int advice_resolve_conflict = 1; int advice_implicit_identity = 1; int advice_detached_head = 1; +int advice_typo_prompt = 1; static struct { const char *name; @@ -23,6 +24,7 @@ static struct { { "resolveconflict", &advice_resolve_conflict }, { "implicitidentity", &advice_implicit_identity }, { "detachedhead", &advice_detached_head }, + { "typoprompt", &advice_typo_prompt }, }; void advise(const char *advice, ...) diff --git a/advice.h b/advice.h index f3cdbbf..050068d 100644 --- a/advice.h +++ b/advice.h @@ -12,6 +12,7 @@ extern int advice_commit_before_merge; extern int advice_resolve_conflict; extern int advice_implicit_identity; extern int advice_detached_head; +extern int advice_typo_prompt; int git_default_advice_config(const char *var, const char *value); void advise(const char *advice, ...); diff --git a/help.c b/help.c index ee261f4..4b45e43 100644 --- a/help.c +++ b/help.c @@ -7,6 +7,7 @@ #include "string-list.h" #include "column.h" #include "version.h" +#include "compat/terminal.h" void add_cmdname(struct cmdnames *cmds, const char *name, int len) { @@ -248,12 +249,30 @@ int is_in_cmdlist(struct cmdnames *c, const char *s) } static int autocorrect; +static int shall_advise = 1; +static int shall_prompt; +static const char message_advice_prompt_ability[] = + N_("I can display an interactive prompt to proceed with one of the above\n" + "suggestions; if you wish me to do so, use\n" + "\n" + " git config --global help.autocorrect ask\n" + "\n" + "See 'git help config' and search for 'help.autocorrect' for further\n" + "information.\n"); static struct cmdnames aliases; static int git_unknown_cmd_config(const char *var, const char *value, void *cb) { - if (!strcmp(var, "help.autocorrect")) - autocorrect = git_config_int(var,value); + if (!strcmp(var, "help.autocorrect") && value) { + shall_advise = 0; + if (!strcasecmp(value, "off")) + ; + else if (!strcasecmp(value, "ask")) + shall_prompt = 1; + else + autocorrect = git_config_int(var, value); + } + /* Also use aliases for command lookup */ if (!prefixcmp(var, "alias.")) add_cmdname(&aliases, var + 6, strlen(var + 6)); @@ -385,8 +404,44 @@ const char *help_unknown_cmd(const char *cmd) "\nDid you mean one of these?", n)); - for (i = 0; i < n; i++) - fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); + if (!isatty(2) || !shall_prompt) { + for (i = 0; i < n; i++) + fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); + if (isatty(2) && shall_advise && advice_typo_prompt) { + fprintf(stderr, "\n"); + advise(_(message_advice_prompt_ability)); + } + } else if (n == 1) { + char *in; + const char *ret; + fprintf(stderr, "\t%s\n", main_cmds.names[0]->name); + in = git_terminal_prompt("[Y/n] ", 1); + switch (in[0]) { + case 'y': case 'Y': case 0: + ret = xstrdup(main_cmds.names[0]->name); + clean_cmdnames(&main_cmds); + return ret; + /* otherwise, don't do anything */ + } + } else { + char *in; + const char *ret; + int opt; + for (i = 0; i < n; i++) + fprintf(stderr, "%d:\t%s\n", i + 1, main_cmds.names[i]->name); + in = git_terminal_prompt("[N/1/2/...] ", 1); + switch (in[0]) { + case 'n': case 'N': case 0: + ; + default: + opt = atoi(in); + if (0 < opt && opt <= n) { + ret = xstrdup(main_cmds.names[opt - 1]->name); + clean_cmdnames(&main_cmds); + return ret; + } + } + } } clean_cmdnames(&main_cmds); -- 1.7.11.1.116.g8228a23 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2 4/4] allow recovery from command name typos 2012-07-25 16:16 ` [PATCH v2 4/4] allow recovery from command name typos Tay Ray Chuan @ 2012-07-25 17:57 ` Junio C Hamano 2012-07-26 17:08 ` Tay Ray Chuan 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2012-07-25 17:57 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Jeff King, Thomas Rast Tay Ray Chuan <rctay89@gmail.com> writes: > If suggestions are available (based on Levenshtein distance) and if the > terminal isatty(), present a prompt to the user to select one of the > computed suggestions. The way to determine "If the terminal is a tty" used in this patch looks overly dangerous, given that we do not know what kind of "git" command we may be invoking at this point. Perhaps we should audit "isatty()" calls and replace them with a helper function that does this kind of thing consistently in a more robust way (my recent favorite is Linus's somewhat anal logic used in builtin/merge.c::default_edit_option()). > +static int shall_advise = 1; > +static int shall_prompt; Naming "shall_foo" is a first here. It is not wrong per-se, but I think we tend to call these "do we use/perform/etc X" do_X in our codebase (see builtin/{config.c,fetch-pack.c,notes.c} for examples). ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 4/4] allow recovery from command name typos 2012-07-25 17:57 ` Junio C Hamano @ 2012-07-26 17:08 ` Tay Ray Chuan 2012-07-26 17:26 ` Jeff King 2012-07-26 17:53 ` Junio C Hamano 0 siblings, 2 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-07-26 17:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Jeff King, Thomas Rast On Thu, Jul 26, 2012 at 1:57 AM, Junio C Hamano <gitster@pobox.com> wrote: > > Tay Ray Chuan <rctay89@gmail.com> writes: > > > If suggestions are available (based on Levenshtein distance) and if the > > terminal isatty(), present a prompt to the user to select one of the > > computed suggestions. > > The way to determine "If the terminal is a tty" used in this patch > looks overly dangerous, given that we do not know what kind of "git" > command we may be invoking at this point. Indeed, it should also have considered stdin's tty-ness. > Perhaps we should audit "isatty()" calls and replace them with a > helper function that does this kind of thing consistently in a more > robust way (my recent favorite is Linus's somewhat anal logic used > in builtin/merge.c::default_edit_option()). Any specific callers to isatty() you have in mind? A quick grep shows that a significant portion of the "offenders" are isatty(2) calls to determine whether to display progress, I think those are ok. The credential helper has some prompting functionality that is close to what I intend to do here, but I think it can make some assumptions about stdin/stdout that we can't, as you have pointed out. So that leaves merge-edit and this patch as the beneficiaries of a builtin/merge.c::default_edit_option() refactor. That's just off the top of my head. Perhaps the helper function could be named "git_can_prompt()" and placed in prompt.c? -- Cheers, Ray Chuan ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 4/4] allow recovery from command name typos 2012-07-26 17:08 ` Tay Ray Chuan @ 2012-07-26 17:26 ` Jeff King 2012-07-26 17:59 ` Junio C Hamano 2012-07-26 17:53 ` Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: Jeff King @ 2012-07-26 17:26 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Junio C Hamano, Git Mailing List, Thomas Rast On Fri, Jul 27, 2012 at 01:08:34AM +0800, Tay Ray Chuan wrote: > > Perhaps we should audit "isatty()" calls and replace them with a > > helper function that does this kind of thing consistently in a more > > robust way (my recent favorite is Linus's somewhat anal logic used > > in builtin/merge.c::default_edit_option()). > > Any specific callers to isatty() you have in mind? A quick grep shows > that a significant portion of the "offenders" are isatty(2) calls to > determine whether to display progress, I think those are ok. Yeah, those are probably fine. Grep reveals that besides isatty(2) and the merge default_edit_option check, we have: - isatty(1) for checking auto-output munging, including auto-colors, auto-columns, and the pager. These are all fine, as they are not about interactivity, but specifically about whether stdout is a tty. - isatty(0) in commit.c to print a message when reading "-F -" from stdin. OK. - isatty(0) in pack-redundant to avoid reading stdin when it is a terminal (a questionable choice, perhaps, but not really something that would want a full interactivity check). - isatty(0) check in cmd_revert to set opts.edit automatically. This one should match merge's behavior. - isatty(0) in shortlog; this is a compatibility hack as shortlog traditionally accepted log output on stdin, but can now be used stand-alone. OK. So I think the only one that could be improved is the one in cmd_revert. > The credential helper has some prompting functionality that is close > to what I intend to do here, but I think it can make some assumptions > about stdin/stdout that we can't, as you have pointed out. So that > leaves merge-edit and this patch as the beneficiaries of a > builtin/merge.c::default_edit_option() refactor. That's just off the > top of my head. The credential code uses git_terminal_prompt, which actually opens /dev/tty directly. So it is probably sane to use for your new prompt, but it does not (and should not) rely on isatty. > Perhaps the helper function could be named "git_can_prompt()" and > placed in prompt.c? Please don't. The isatty() checks have nothing to do with whether git_prompt can run. The only thing such a git_can_prompt function should do is see if we can open /dev/tty. The isatty check in merge.c is more about "are we interactive, so that it is sane to run $EDITOR". -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 4/4] allow recovery from command name typos 2012-07-26 17:26 ` Jeff King @ 2012-07-26 17:59 ` Junio C Hamano 2012-07-26 18:37 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2012-07-26 17:59 UTC (permalink / raw) To: Jeff King; +Cc: Tay Ray Chuan, Git Mailing List, Thomas Rast Jeff King <peff@peff.net> writes: > - isatty(0) check in cmd_revert to set opts.edit automatically. This > one should match merge's behavior. > ... > So I think the only one that could be improved is the one in cmd_revert. Yeah, that matches the result of my grep. Thanks for sanity checking. > The credential code uses git_terminal_prompt, which actually opens > /dev/tty directly. So it is probably sane to use for your new prompt, > but it does not (and should not) rely on isatty. I think using git_terminal_prompt() after doing a looser "does the user sit at a terminal and is capable of answering interactive prompt" check with isatty(2) is OK, as long as we know that all implementations of git_terminal_prompt() never read from whatever happens to be connected to the standard input. The function falls back to getpass() on platforms without DEV_TTY, and if getpass() on some platforms reads from the standard input, that would be a disaster. I wasn't sure about that part. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 4/4] allow recovery from command name typos 2012-07-26 17:59 ` Junio C Hamano @ 2012-07-26 18:37 ` Jeff King 0 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2012-07-26 18:37 UTC (permalink / raw) To: Junio C Hamano Cc: Tay Ray Chuan, Git Mailing List, Thomas Rast, Erik Faye-Lund On Thu, Jul 26, 2012 at 10:59:51AM -0700, Junio C Hamano wrote: > > The credential code uses git_terminal_prompt, which actually opens > > /dev/tty directly. So it is probably sane to use for your new prompt, > > but it does not (and should not) rely on isatty. > > I think using git_terminal_prompt() after doing a looser "does the > user sit at a terminal and is capable of answering interactive > prompt" check with isatty(2) is OK, as long as we know that all > implementations of git_terminal_prompt() never read from whatever > happens to be connected to the standard input. I don't think isatty(2) is correct, though. It would yield false negatives when the user has redirected stderr but /dev/tty is still available. I don't know if it possible for getpass to fallback to stdin when stderr is a tty (it would mean that opening /dev/tty failed, which would mean that we have no controlling terminal _but_ our stderr is still connected to some terminal. That might be bizarre enough not to care about). I think the right answer would be a real is_prompt_available() that checked /dev/tty when HAVE_DEV_TTY was set, and otherwise checked isatty(2) (or whatever was appropriate for the platform). > The function falls back to getpass() on platforms without DEV_TTY, > and if getpass() on some platforms reads from the standard input, > that would be a disaster. I wasn't sure about that part. Yeah, although it is already a disaster in those cases, as the main caller of git_terminal_prompt is remote-curl, whose stdin is connected to git via the remote-helper protocol. Which isn't to say this wouldn't make things worse. It would, but the real solution is to implement a sane git_terminal_prompt for those platforms. Erik had a patch for Windows to use their magical CONIN$, but I think it is temporarily stalled. I don't know if there are any other platforms that do not have /dev/tty (I know we do not set HAVE_DEV_TTY by default, but that is only because I was being conservative and waiting for people on particular platforms to confirm that it works before tweaking our Makefile defaults). -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 4/4] allow recovery from command name typos 2012-07-26 17:08 ` Tay Ray Chuan 2012-07-26 17:26 ` Jeff King @ 2012-07-26 17:53 ` Junio C Hamano 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-07-26 17:53 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Jeff King, Thomas Rast Tay Ray Chuan <rctay89@gmail.com> writes: > On Thu, Jul 26, 2012 at 1:57 AM, Junio C Hamano <gitster@pobox.com> wrote: >> >> Tay Ray Chuan <rctay89@gmail.com> writes: >> >> > If suggestions are available (based on Levenshtein distance) and if the >> > terminal isatty(), present a prompt to the user to select one of the >> > computed suggestions. >> >> The way to determine "If the terminal is a tty" used in this patch >> looks overly dangerous, given that we do not know what kind of "git" >> command we may be invoking at this point. > > Indeed, it should also have considered stdin's tty-ness. Not necessarily. As long as you call git_prompt(), which opens /dev/tty on its own and does not break even if its standard input is coming from elsewhere, you should be OK. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect 2012-07-25 16:16 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 4/4] allow recovery from command name typos Tay Ray Chuan @ 2012-07-25 17:47 ` Junio C Hamano 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-07-25 17:47 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Jeff King, Thomas Rast Tay Ray Chuan <rctay89@gmail.com> writes: > When help.autocorrect is set, in an attempt to retain the memory to the > string name in main_cmds, we unfortunately leaked the struct cmdname > that held it. Fix this by creating a copy of the string name. If you are updating help_unknown_cmd() so that the caller can free (and is responsible for freeing if it wants to avoid leaks) the piece of memory it returns, that change to "assumed" makes sense. What we were returning were not freeable. Butthe caller does not free it; what you did is merely to shift the leakage from here to its caller. Is it worth the churn and one extra allocation to still leak slightly (namely, by sizeof(size_t)) smaller chunk of memory than what the code currently does? I doubt it. > When help.autocorrect is not set, we exit()'d without free'ing it like > we do when the config is set; fix this. I don't see any point in doing this, immediately in the same function on the previous line of exit(1). > Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> > --- > > Changed in v2: plug leak when help.autocorrect is not set. > > --- > help.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/help.c b/help.c > index dfb2e9d..ee261f4 100644 > --- a/help.c > +++ b/help.c > @@ -362,8 +362,7 @@ const char *help_unknown_cmd(const char *cmd) > ; /* still counting */ > } > if (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) { > - const char *assumed = main_cmds.names[0]->name; > - main_cmds.names[0] = NULL; > + const char *assumed = xstrdup(main_cmds.names[0]->name); > clean_cmdnames(&main_cmds); > fprintf_ln(stderr, > _("WARNING: You called a Git command named '%s', " > @@ -390,6 +389,7 @@ const char *help_unknown_cmd(const char *cmd) > fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); > } > > + clean_cmdnames(&main_cmds); > exit(1); > } ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak 2012-07-25 16:16 ` [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, " Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Tay Ray Chuan @ 2012-07-25 17:39 ` Junio C Hamano 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-07-25 17:39 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Jeff King, Thomas Rast Tay Ray Chuan <rctay89@gmail.com> writes: > Copying with structural assignment may not take into account that the > LHS struct has sufficient memory, especially since the cmdname->name > member is nonfixed in size. Be unambiguous about it by realloc()'ing it > to be of sufficient size. If the original code were *(cmd->names[cj++]) = *(cmd->names[ci++]); there may be a structural assignment involved, but cmds->names[dst] = cmd->names[src] just copies the pointer that points at a struct cmdname that records the src command name to another slot of cmds->names[] array, whose elements are pointers, no? What's there to realloc? > @@ -58,20 +69,25 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) > { > int ci, cj, ei; > int cmp; > + int last_cj; > > ci = cj = ei = 0; > while (ci < cmds->cnt && ei < excludes->cnt) { > cmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name); > if (cmp < 0) > - cmds->names[cj++] = cmds->names[ci++]; > + copy_cmdname(&cmds->names[cj++], cmds->names[ci++]); > else if (cmp == 0) > ci++, ei++; > else if (cmp > 0) > ei++; > } > + last_cj = cj; > > while (ci < cmds->cnt) > - cmds->names[cj++] = cmds->names[ci++]; > + copy_cmdname(&cmds->names[cj++], cmds->names[ci++]); > + > + while (last_cj < cmds->cnt) > + free(cmds->names[last_cj++]); > > cmds->cnt = cj; > } We shifted cmds->names[] array to skip entries that appear in excludes. If original cmds->names[] had "0", "1", "2", "3", ... and excludes had "0" and "1", cmds->names[] would contain "2", "3", "2", "3"; the first two are copied over "0" and "1" that are excluded, and the latter two are leftover beyond last_cj. The corresponding names share the same structure (cmds->names[] is an array of pointers). Doesn't freeing cmds->names[2] free the structure that is used by both cmds->names[0] and cmds->names[2]? Confused. The function drops cmds->names[ci] when it appears in excludes, so you may want to free it when it happens, though. help.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/help.c b/help.c index 6991492..cae389b 100644 --- a/help.c +++ b/help.c @@ -64,9 +64,10 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) cmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name); if (cmp < 0) cmds->names[cj++] = cmds->names[ci++]; - else if (cmp == 0) - ci++, ei++; - else if (cmp > 0) + else if (cmp == 0) { + ei++; + free(cmd->names[ci++]); + } else if (cmp > 0) ei++; } ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v3 0/2] allow recovery from command name typos 2012-05-06 6:55 [PATCH 0/4] allow recovery from command name typos Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 1/4] help.c::uniq: plug a leak Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 0/4] allow recovery from command name typos Tay Ray Chuan @ 2012-08-05 18:45 ` Tay Ray Chuan 2012-08-05 18:45 ` [PATCH v3 1/2] add interface for /dev/tty interaction Tay Ray Chuan 2 siblings, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-08-05 18:45 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King As discussed in the previous iteration, testing for prompt-availabilty has been reworked (patch #2). This is done with the aid of patch #1, which extracts the opening of /dev/tty from git_terminal_prompt() into a terminal_open(). Its return value indicates if a terminal is available for prompting. On systems with HAVE_DEV_TTY unset, terminal_prompt() falls back to checking the tty-ness of stdin and stderr (as getpass() uses both). Contents: [PATCH v3 1/2] add interface for /dev/tty interaction [PATCH v3 2/2] allow recovery from command name typos -- 1.7.12.rc1.187.g6dd9156 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v3 1/2] add interface for /dev/tty interaction 2012-08-05 18:45 ` [PATCH v3 0/2] allow recovery from command name typos Tay Ray Chuan @ 2012-08-05 18:45 ` Tay Ray Chuan 2012-08-05 18:45 ` [PATCH v3 2/2] allow recovery from command name typos Tay Ray Chuan 2012-08-05 20:11 ` [PATCH v3 1/2] add interface for /dev/tty interaction Junio C Hamano 0 siblings, 2 replies; 37+ messages in thread From: Tay Ray Chuan @ 2012-08-05 18:45 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King Factor out the opening and closing of /dev/tty from git_terminal_prompt(), so that callers may first test if a controlling terminal is available before proceeding with prompting proper. When HAVE_DEV_TTY is not defined, terminal_open() falls back to checking tty-ness of stdin and stderr, as getpass() uses them both. Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- compat/terminal.c | 52 ++++++++++++++++++++++++++++++++++++++++++++-------- compat/terminal.h | 10 ++++++++++ 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/compat/terminal.c b/compat/terminal.c index 6d16c8f..c85d5c7 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -24,15 +24,21 @@ static void restore_term_on_signal(int sig) raise(sig); } -char *git_terminal_prompt(const char *prompt, int echo) +term_t terminal_open(void) +{ + return fopen("/dev/tty", "w+"); +} + +int terminal_close(term_t term) +{ + return fclose(term); +} + +char *terminal_prompt(term_t term, const char *prompt, int echo) { static struct strbuf buf = STRBUF_INIT; int r; - FILE *fh; - - fh = fopen("/dev/tty", "w+"); - if (!fh) - return NULL; + FILE *fh = term; if (!echo) { struct termios t; @@ -64,18 +70,48 @@ char *git_terminal_prompt(const char *prompt, int echo) } restore_term(); - fclose(fh); if (r == EOF) return NULL; return buf.buf; } +char *git_terminal_prompt(const char *prompt, int echo) +{ + char *ret; + term_t term; + + term = terminal_open(); + if (!term) + return NULL; + + ret = terminal_prompt(term, prompt, echo); + + terminal_close(term); + + return ret; +} + #else -char *git_terminal_prompt(const char *prompt, int echo) +term_t terminal_open() +{ + return isatty(0) && isatty(2); +} + +int terminal_close(term_t term) +{ + return 0; +} + +char *terminal_prompt(term_t term, const char *prompt, int echo) { return getpass(prompt); } +char *git_terminal_prompt(const char *prompt, int echo) +{ + return terminal_prompt(prompt, echo); +} + #endif diff --git a/compat/terminal.h b/compat/terminal.h index 97db7cd..cf2aa10 100644 --- a/compat/terminal.h +++ b/compat/terminal.h @@ -1,6 +1,16 @@ #ifndef COMPAT_TERMINAL_H #define COMPAT_TERMINAL_H +#ifdef HAVE_DEV_TTY +typedef FILE *term_t; +#else +typedef int term_t; +#endif + +term_t terminal_open(); +int terminal_close(term_t term); +char *terminal_prompt(term_t term, const char *prompt, int echo); + char *git_terminal_prompt(const char *prompt, int echo); #endif /* COMPAT_TERMINAL_H */ -- 1.7.12.rc1.187.g6dd9156 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH v3 2/2] allow recovery from command name typos 2012-08-05 18:45 ` [PATCH v3 1/2] add interface for /dev/tty interaction Tay Ray Chuan @ 2012-08-05 18:45 ` Tay Ray Chuan 2012-08-06 0:50 ` Junio C Hamano 2012-08-05 20:11 ` [PATCH v3 1/2] add interface for /dev/tty interaction Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: Tay Ray Chuan @ 2012-08-05 18:45 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Jeff King If suggestions are available (based on Levenshtein distance) and if the terminal isatty(), present a prompt to the user to select one of the computed suggestions. In the case where there is a single suggestion, present the prompt "[Y/n]", such that "" (ie. the default), "y" and "Y" as input leads git to proceed executing the suggestion, while everything else (possibly "n") leads git to terminate. In the case where there are multiple suggestions, number the suggestions 1 to <n> (the number of suggestions), and accept an integer as input, while everything else (possibly "n") leads git to terminate. In this case there is no default; an empty input leads git to terminate. A sample run: $ git sh --pretty=oneline git: 'sh' is not a git command. See 'git --help'. Did you mean one of these? 1: show 2: push [N/1/2/...] This prompt is enabled only if help.autocorrect is set to ask; if unset, advise the user about this ability. Helped-by: Thomas Rast <trast@student.ethz.ch> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> --- Changed in v3: - say do_* instead of shall_* - use new terminal interface Documentation/config.txt | 30 ++++++++++++++++----- advice.c | 2 ++ advice.h | 1 + help.c | 68 +++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 90 insertions(+), 11 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 0bcea8a..0bb175a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -177,6 +177,10 @@ advice.*:: Advice shown when you used linkgit:git-checkout[1] to move to the detach HEAD state, to instruct how to create a local branch after the fact. + typoPrompt:: + Upon a mistyped command, if 'help.autocorrect' is unset + advise that an interactive prompt can be displayed to + recover from the typo. -- core.fileMode:: @@ -1323,13 +1327,25 @@ help.format:: the default. 'web' and 'html' are the same. help.autocorrect:: - Automatically correct and execute mistyped commands after - waiting for the given number of deciseconds (0.1 sec). If more - than one command can be deduced from the entered text, nothing - will be executed. If the value of this option is negative, - the corrected command will be executed immediately. If the - value is 0 - the command will be just shown but not executed. - This is the default. + Specifies behaviour to recover from mistyped commands. ++ +When set to `ask`, an interactive prompt is displayed, allowing the user +to select a suggested command for execution. ++ +When set to `off`, no attempt to recover is made. ++ +If a number is given, it will be interpreted as the deciseconds (0.1 +sec) to wait before automatically correcting and executing the mistyped +command, with the following behaviour: ++ +* If more than one command can be deduced from the entered text, nothing + will be executed. +* If the value of this option is negative, the corrected command will be + executed immediately. +* If the value is 0 - the command will be just shown but not executed. ++ +The default is to display a message suggesting that this option be set +to `ask`, without attempting to recover (see `advice.typoPrompt`). http.proxy:: Override the HTTP proxy, normally configured using the 'http_proxy', diff --git a/advice.c b/advice.c index a492eea..d070a05 100644 --- a/advice.c +++ b/advice.c @@ -9,6 +9,7 @@ int advice_commit_before_merge = 1; int advice_resolve_conflict = 1; int advice_implicit_identity = 1; int advice_detached_head = 1; +int advice_typo_prompt = 1; static struct { const char *name; @@ -23,6 +24,7 @@ static struct { { "resolveconflict", &advice_resolve_conflict }, { "implicitidentity", &advice_implicit_identity }, { "detachedhead", &advice_detached_head }, + { "typoprompt", &advice_typo_prompt }, }; void advise(const char *advice, ...) diff --git a/advice.h b/advice.h index f3cdbbf..050068d 100644 --- a/advice.h +++ b/advice.h @@ -12,6 +12,7 @@ extern int advice_commit_before_merge; extern int advice_resolve_conflict; extern int advice_implicit_identity; extern int advice_detached_head; +extern int advice_typo_prompt; int git_default_advice_config(const char *var, const char *value); void advise(const char *advice, ...); diff --git a/help.c b/help.c index c4285a5..cc13b92 100644 --- a/help.c +++ b/help.c @@ -7,6 +7,7 @@ #include "string-list.h" #include "column.h" #include "version.h" +#include "compat/terminal.h" void add_cmdname(struct cmdnames *cmds, const char *name, int len) { @@ -233,12 +234,30 @@ int is_in_cmdlist(struct cmdnames *c, const char *s) } static int autocorrect; +static int do_advise = 1; +static int do_prompt; +static const char message_advice_prompt_ability[] = + N_("I can display an interactive prompt to proceed with one of the above\n" + "suggestions; if you wish me to do so, use\n" + "\n" + " git config --global help.autocorrect ask\n" + "\n" + "See 'git help config' and search for 'help.autocorrect' for further\n" + "information.\n"); static struct cmdnames aliases; static int git_unknown_cmd_config(const char *var, const char *value, void *cb) { - if (!strcmp(var, "help.autocorrect")) - autocorrect = git_config_int(var,value); + if (!strcmp(var, "help.autocorrect") && value) { + do_advise = 0; + if (!strcasecmp(value, "off")) + ; + else if (!strcasecmp(value, "ask")) + do_prompt = 1; + else + autocorrect = git_config_int(var, value); + } + /* Also use aliases for command lookup */ if (!prefixcmp(var, "alias.")) add_cmdname(&aliases, var + 6, strlen(var + 6)); @@ -366,13 +385,54 @@ const char *help_unknown_cmd(const char *cmd) fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd); if (SIMILAR_ENOUGH(best_similarity)) { + term_t term; + fprintf_ln(stderr, Q_("\nDid you mean this?", "\nDid you mean one of these?", n)); - for (i = 0; i < n; i++) - fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); + term = terminal_open(); + if (!term || !do_prompt) { + for (i = 0; i < n; i++) + fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); + if (isatty(2) && do_advise && advice_typo_prompt) { + fprintf(stderr, "\n"); + advise(_(message_advice_prompt_ability)); + } + } else if (n == 1) { + char *in; + const char *ret; + fprintf(stderr, "\t%s\n", main_cmds.names[0]->name); + in = terminal_prompt(term, "[Y/n] ", 1); + terminal_close(term); + switch (in[0]) { + case 'y': case 'Y': case 0: + ret = xstrdup(main_cmds.names[0]->name); + clean_cmdnames(&main_cmds); + return ret; + /* otherwise, don't do anything */ + } + } else { + char *in; + const char *ret; + int opt; + for (i = 0; i < n; i++) + fprintf(stderr, "%d:\t%s\n", i + 1, main_cmds.names[i]->name); + in = terminal_prompt(term, "[N/1/2/...] ", 1); + terminal_close(term); + switch (in[0]) { + case 'n': case 'N': case 0: + ; + default: + opt = atoi(in); + if (0 < opt && opt <= n) { + ret = xstrdup(main_cmds.names[opt - 1]->name); + clean_cmdnames(&main_cmds); + return ret; + } + } + } } exit(1); -- 1.7.12.rc1.187.g6dd9156 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v3 2/2] allow recovery from command name typos 2012-08-05 18:45 ` [PATCH v3 2/2] allow recovery from command name typos Tay Ray Chuan @ 2012-08-06 0:50 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-08-06 0:50 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Jeff King Tay Ray Chuan <rctay89@gmail.com> writes: > If suggestions are available (based on Levenshtein distance) and if the > terminal isatty(), present a prompt to the user to select one of the > computed suggestions. > > In the case where there is a single suggestion, present the prompt > "[Y/n]", such that "" (ie. the default), "y" and "Y" as input leads git > to proceed executing the suggestion, while everything else (possibly > "n") leads git to terminate. > > In the case where there are multiple suggestions, number the suggestions > 1 to <n> (the number of suggestions), and accept an integer as input, > while everything else (possibly "n") leads git to terminate. In this > case there is no default; an empty input leads git to terminate. A > sample run: > > $ git sh --pretty=oneline > git: 'sh' is not a git command. See 'git --help'. > > Did you mean one of these? > 1: show > 2: push > [N/1/2/...] > > This prompt is enabled only if help.autocorrect is set to ask; if unset, > advise the user about this ability. > > Helped-by: Thomas Rast <trast@student.ethz.ch> > Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> > --- > > Changed in v3: > - say do_* instead of shall_* > - use new terminal interface > > Documentation/config.txt | 30 ++++++++++++++++----- > advice.c | 2 ++ > advice.h | 1 + > help.c | 68 +++++++++++++++++++++++++++++++++++++++++++++--- > 4 files changed, 90 insertions(+), 11 deletions(-) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 0bcea8a..0bb175a 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -177,6 +177,10 @@ advice.*:: > Advice shown when you used linkgit:git-checkout[1] to > move to the detach HEAD state, to instruct how to create > a local branch after the fact. > + typoPrompt:: > + Upon a mistyped command, if 'help.autocorrect' is unset > + advise that an interactive prompt can be displayed to > + recover from the typo. > -- I have a moderately strong reaction against this; "advice" is for helping users out of common pitfalls, and we generally do not use the "advise" mechanism to advertise random shiny features. > @@ -1323,13 +1327,25 @@ help.format:: > ... > help.autocorrect:: > + Specifies behaviour to recover from mistyped commands. > ++ > +When set to `ask`, an interactive prompt is displayed, allowing the user > +to select a suggested command for execution. > ++ > +When set to `off`, no attempt to recover is made. I notice that with the current code, even if help.autocorrect is set to 0 to decline the guessing, we still get "did you mean one of these" as long as the typo is similar enough to existing command. I am guessing that this new value `off` is a way to remedy the situation so that users can choose to decline any guessing, and just get "no such subcommand". If that is the case, I think it is a vast improvement. > +If a number is given, it will be interpreted as the deciseconds (0.1 > +sec) to wait before automatically correcting and executing the mistyped > +command, with the following behaviour: > ++ > +* If more than one command can be deduced from the entered text, nothing > + will be executed. The above is from the original text, but I've always found the "can be deduced" part hard to understand. It is a quite roundabout way to say we cannot guess with confidence what the user meant and avoid committing to a wrong guess. We may want to think a better way to phrase the whole thing. Perhaps something along this line: help.autocorrect:: When you mistype the name of a subcommand during an interactive session, Git can try to guess which one of available subcommands you meant (Git does not waste cycles in a non-interactive session). This configuration variable specifies what happens when there are one or more subcommands that you are likely to have meant. - when set to 'ask', the choices are presented and you can pick one to execute. If the command is used non-interactively, - when set to `off`, ... This can be done after this patch series settles, of course. > +* If the value of this option is negative, the corrected command will be > + executed immediately. > +* If the value is 0 - the command will be just shown but not executed. > ++ > +The default is to display a message suggesting that this option be set > +to `ask`, without attempting to recover (see `advice.typoPrompt`). My comment to 'advice.typoPrompt' leads me to suggest not to change the default to `ask`, but leave it to 0, and remove the change to the following two files. > diff --git a/advice.c b/advice.c > diff --git a/advice.h b/advice.h > diff --git a/help.c b/help.c > index c4285a5..cc13b92 100644 > --- a/help.c > +++ b/help.c > @@ -7,6 +7,7 @@ > #include "string-list.h" > #include "column.h" > #include "version.h" > +#include "compat/terminal.h" > > void add_cmdname(struct cmdnames *cmds, const char *name, int len) > { > @@ -233,12 +234,30 @@ int is_in_cmdlist(struct cmdnames *c, const char *s) > } > > static int autocorrect; > +static int do_advise = 1; > +static int do_prompt; > +static const char message_advice_prompt_ability[] = > + N_("I can display an interactive prompt to proceed with one of the above\n" > + "suggestions; if you wish me to do so, use\n" > + "\n" > + " git config --global help.autocorrect ask\n" > + "\n" > + "See 'git help config' and search for 'help.autocorrect' for further\n" > + "information.\n"); > static struct cmdnames aliases; Nah. No unsolicited advertisement, please. > static int git_unknown_cmd_config(const char *var, const char *value, void *cb) > { > - if (!strcmp(var, "help.autocorrect")) > - autocorrect = git_config_int(var,value); > + if (!strcmp(var, "help.autocorrect") && value) { > + do_advise = 0; > + if (!strcasecmp(value, "off")) > + ; > + else if (!strcasecmp(value, "ask")) > + do_prompt = 1; > + else > + autocorrect = git_config_int(var, value); > + } I think the current code diagnoses [help] autocorrect that tries to say "true" as a syntax error. The above simply ignores such an entry, no? I was hoping "off" would be usable to bypass the whole levenstein thing, but the above code does not suggest that the remainder of this patch would be doing that X-<. > @@ -366,13 +385,54 @@ const char *help_unknown_cmd(const char *cmd) > fprintf_ln(stderr, _("git: '%s' is not a git command. See 'git --help'."), cmd); > > if (SIMILAR_ENOUGH(best_similarity)) { > + term_t term; > + > fprintf_ln(stderr, > Q_("\nDid you mean this?", > "\nDid you mean one of these?", > n)); > > - for (i = 0; i < n; i++) > - fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); > + term = terminal_open(); > + if (!term || !do_prompt) { > + for (i = 0; i < n; i++) > + fprintf(stderr, "\t%s\n", main_cmds.names[i]->name); It is the same as what is done with the current code, but if there is no terminal available, do we even want to give this list? > + if (isatty(2) && do_advise && advice_typo_prompt) { > + fprintf(stderr, "\n"); > + advise(_(message_advice_prompt_ability)); > + } Nah. No unsolicited advertisement, please. > + } else if (n == 1) { > + char *in; > + const char *ret; > + fprintf(stderr, "\t%s\n", main_cmds.names[0]->name); > + in = terminal_prompt(term, "[Y/n] ", 1); > + terminal_close(term); > + switch (in[0]) { > + case 'y': case 'Y': case 0: > + ret = xstrdup(main_cmds.names[0]->name); > + clean_cmdnames(&main_cmds); > + return ret; OK. > + /* otherwise, don't do anything */ > + } Indent the comment one level deeper? > + } else { > + char *in; > + const char *ret; > + int opt; > + for (i = 0; i < n; i++) Can we have too many choices for this "prompt" codepath to be practical? > + fprintf(stderr, "%d:\t%s\n", i + 1, main_cmds.names[i]->name); > + in = terminal_prompt(term, "[N/1/2/...] ", 1); Would it be too much trouble to spell the actual choices out here, instead of the ugly "/..."? > + terminal_close(term); > + switch (in[0]) { > + case 'n': case 'N': case 0: > + ; > + default: > + opt = atoi(in); > + if (0 < opt && opt <= n) { > + ret = xstrdup(main_cmds.names[opt - 1]->name); > + clean_cmdnames(&main_cmds); > + return ret; > + } When the user mistypes the choice (perhaps say '8' when there are only 7 choices available), it might be more helpful to loop here to give him another chance. Would such an enhancement be worth it? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3 1/2] add interface for /dev/tty interaction 2012-08-05 18:45 ` [PATCH v3 1/2] add interface for /dev/tty interaction Tay Ray Chuan 2012-08-05 18:45 ` [PATCH v3 2/2] allow recovery from command name typos Tay Ray Chuan @ 2012-08-05 20:11 ` Junio C Hamano 2012-08-06 19:45 ` Jeff King 1 sibling, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2012-08-05 20:11 UTC (permalink / raw) To: Tay Ray Chuan; +Cc: Git Mailing List, Jeff King Tay Ray Chuan <rctay89@gmail.com> writes: > Factor out the opening and closing of /dev/tty from > git_terminal_prompt(), so that callers may first test if a controlling > terminal is available before proceeding with prompting proper. > > When HAVE_DEV_TTY is not defined, terminal_open() falls back to checking > tty-ness of stdin and stderr, as getpass() uses them both. > > Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> > --- This is not your fault but seeing term_t made me go "eek, yuck". As far as I can see, use of "FILE *" in existing compat/terminal.c is not buying us anything useful. The stdio calls made on FILE *fh are only fopen(), fputs(), fflush() and fclose(), and everything else goes through fileno(fh). So perhaps it is a saner approach to fix that function first before this patch so that it works on file descriptors. > compat/terminal.c | 52 ++++++++++++++++++++++++++++++++++++++++++++-------- > compat/terminal.h | 10 ++++++++++ > 2 files changed, 54 insertions(+), 8 deletions(-) > > diff --git a/compat/terminal.c b/compat/terminal.c > index 6d16c8f..c85d5c7 100644 > --- a/compat/terminal.c > +++ b/compat/terminal.c > @@ -24,15 +24,21 @@ static void restore_term_on_signal(int sig) > raise(sig); > } > > -char *git_terminal_prompt(const char *prompt, int echo) > +term_t terminal_open(void) > +{ > + return fopen("/dev/tty", "w+"); > +} > + > +int terminal_close(term_t term) > +{ > + return fclose(term); > +} > + > +char *terminal_prompt(term_t term, const char *prompt, int echo) > { > static struct strbuf buf = STRBUF_INIT; > int r; > - FILE *fh; > - > - fh = fopen("/dev/tty", "w+"); > - if (!fh) > - return NULL; > + FILE *fh = term; > > if (!echo) { > struct termios t; > @@ -64,18 +70,48 @@ char *git_terminal_prompt(const char *prompt, int echo) > } > > restore_term(); > - fclose(fh); > > if (r == EOF) > return NULL; > return buf.buf; > } > > +char *git_terminal_prompt(const char *prompt, int echo) > +{ > + char *ret; > + term_t term; > + > + term = terminal_open(); > + if (!term) > + return NULL; > + > + ret = terminal_prompt(term, prompt, echo); > + > + terminal_close(term); > + > + return ret; > +} > + > #else > > -char *git_terminal_prompt(const char *prompt, int echo) > +term_t terminal_open() > +{ > + return isatty(0) && isatty(2); > +} > + > +int terminal_close(term_t term) > +{ > + return 0; > +} > + > +char *terminal_prompt(term_t term, const char *prompt, int echo) > { > return getpass(prompt); > } > > +char *git_terminal_prompt(const char *prompt, int echo) > +{ > + return terminal_prompt(prompt, echo); > +} > + > #endif > diff --git a/compat/terminal.h b/compat/terminal.h > index 97db7cd..cf2aa10 100644 > --- a/compat/terminal.h > +++ b/compat/terminal.h > @@ -1,6 +1,16 @@ > #ifndef COMPAT_TERMINAL_H > #define COMPAT_TERMINAL_H > > +#ifdef HAVE_DEV_TTY > +typedef FILE *term_t; > +#else > +typedef int term_t; > +#endif > + > +term_t terminal_open(); > +int terminal_close(term_t term); > +char *terminal_prompt(term_t term, const char *prompt, int echo); > + > char *git_terminal_prompt(const char *prompt, int echo); > > #endif /* COMPAT_TERMINAL_H */ ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3 1/2] add interface for /dev/tty interaction 2012-08-05 20:11 ` [PATCH v3 1/2] add interface for /dev/tty interaction Junio C Hamano @ 2012-08-06 19:45 ` Jeff King 2012-08-06 19:56 ` Jeff King 0 siblings, 1 reply; 37+ messages in thread From: Jeff King @ 2012-08-06 19:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Tay Ray Chuan, Git Mailing List On Sun, Aug 05, 2012 at 01:11:47PM -0700, Junio C Hamano wrote: > Tay Ray Chuan <rctay89@gmail.com> writes: > > > Factor out the opening and closing of /dev/tty from > > git_terminal_prompt(), so that callers may first test if a controlling > > terminal is available before proceeding with prompting proper. > > > > When HAVE_DEV_TTY is not defined, terminal_open() falls back to checking > > tty-ness of stdin and stderr, as getpass() uses them both. > > > > Signed-off-by: Tay Ray Chuan <rctay89@gmail.com> > > --- > > This is not your fault but seeing term_t made me go "eek, yuck". Agreed. > As far as I can see, use of "FILE *" in existing compat/terminal.c > is not buying us anything useful. The stdio calls made on FILE *fh > are only fopen(), fputs(), fflush() and fclose(), and everything > else goes through fileno(fh). > > So perhaps it is a saner approach to fix that function first before > this patch so that it works on file descriptors. Yeah, I think that is a good path. I think my original use of stdio was mostly because I started by paring down glibc's implementation of getpass. Since we have niceties like write_in_full, I don't think there's any reason not to just skip stdio. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3 1/2] add interface for /dev/tty interaction 2012-08-06 19:45 ` Jeff King @ 2012-08-06 19:56 ` Jeff King 2012-08-06 20:01 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Jeff King @ 2012-08-06 19:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Tay Ray Chuan, Git Mailing List On Mon, Aug 06, 2012 at 03:45:11PM -0400, Jeff King wrote: > > This is not your fault but seeing term_t made me go "eek, yuck". > > Agreed. > > > As far as I can see, use of "FILE *" in existing compat/terminal.c > > is not buying us anything useful. The stdio calls made on FILE *fh > > are only fopen(), fputs(), fflush() and fclose(), and everything > > else goes through fileno(fh). > > > > So perhaps it is a saner approach to fix that function first before > > this patch so that it works on file descriptors. > > Yeah, I think that is a good path. I think my original use of stdio > was mostly because I started by paring down glibc's implementation of > getpass. Since we have niceties like write_in_full, I don't think > there's any reason not to just skip stdio. I forgot to mention: even if we changed the HAVE_DEV_TTY code path to use an integer descriptor (which I think is a sane thing to do regardless), that may not be sufficient to solve this problem. Erik has looked into doing a Windows alternative in compat/terminal.c, and I think an integer would be insufficient there. In particular, I think Windows needs two descriptors to accomplish the same thing (one for CONIN$ and one for CONOUT$). So you'd need to turn term_t into a struct (and you'd probably not want to return it by value then). Maybe it would be better to keep the abstraction as non-leaky as possible, and just provide "terminal_can_prompt()" or similar? Returning the open descriptor (as Tay's patch does) avoids a race condition where /dev/tty can be opened when terminal_can_prompt runs, but not when we try to actually read from it. But we can either: 1. Not care. Even if the tty is opened, if a user has closed the terminal we are going to get a read error anyway. So you can never avoid that race condition in some form. 2. Open the terminal descriptor when either function is called, and never close it. I don't think there is any reason we can't just leak the descriptor. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v3 1/2] add interface for /dev/tty interaction 2012-08-06 19:56 ` Jeff King @ 2012-08-06 20:01 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2012-08-06 20:01 UTC (permalink / raw) To: Jeff King; +Cc: Tay Ray Chuan, Git Mailing List Jeff King <peff@peff.net> writes: > Erik has looked into doing a Windows alternative in compat/terminal.c, > and I think an integer would be insufficient there. In particular, I > think Windows needs two descriptors to accomplish the same thing (one > for CONIN$ and one for CONOUT$). So you'd need to turn term_t into a > struct (and you'd probably not want to return it by value then). > > Maybe it would be better to keep the abstraction as non-leaky as > possible, and just provide "terminal_can_prompt()" or similar? OK. As we won't be giving separate instances of terminals to different callers anyway, compat/terminal.c can keep a static variable of whatever type that is necessary for the implementation around. That sounds like a reasonable way to go. > Returning the open descriptor (as Tay's patch does) avoids a race > condition where /dev/tty can be opened when terminal_can_prompt runs, > but not when we try to actually read from it. But we can either: > > 1. Not care. Even if the tty is opened, if a user has closed the > terminal we are going to get a read error anyway. So you can never > avoid that race condition in some form. > > 2. Open the terminal descriptor when either function is called, and > never close it. I don't think there is any reason we can't just > leak the descriptor. > > -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2012-08-06 20:01 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-05-06 6:55 [PATCH 0/4] allow recovery from command name typos Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 1/4] help.c::uniq: plug a leak Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 2/4] help.c::exclude_cmds: " Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 3/4] help.c: plug a leak when help.autocorrect is set Tay Ray Chuan 2012-05-06 6:55 ` [PATCH 4/4] allow recovery from command name typos Tay Ray Chuan 2012-05-06 8:21 ` Jeff King 2012-05-06 16:07 ` Tay Ray Chuan 2012-05-07 9:43 ` Thomas Rast 2012-05-07 15:49 ` Tay Ray Chuan 2012-05-07 17:41 ` Junio C Hamano 2012-05-09 15:06 ` Tay Ray Chuan 2012-05-09 17:03 ` Junio C Hamano [not found] ` <CAOBOgRaDEgAqXWmdC6hrudkL5OwzeMffbj2RtKMxf2TsYWzotA@mail.gmail.com> 2012-05-06 16:04 ` Tay Ray Chuan 2012-05-06 8:12 ` [PATCH 1/4] help.c::uniq: plug a leak Jeff King 2012-05-06 15:54 ` Tay Ray Chuan 2012-05-07 7:30 ` Jeff King 2012-07-25 16:16 ` [PATCH v2 0/4] allow recovery from command name typos Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 1/4] help.c::uniq: plug a leak Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, " Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Tay Ray Chuan 2012-07-25 16:16 ` [PATCH v2 4/4] allow recovery from command name typos Tay Ray Chuan 2012-07-25 17:57 ` Junio C Hamano 2012-07-26 17:08 ` Tay Ray Chuan 2012-07-26 17:26 ` Jeff King 2012-07-26 17:59 ` Junio C Hamano 2012-07-26 18:37 ` Jeff King 2012-07-26 17:53 ` Junio C Hamano 2012-07-25 17:47 ` [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect Junio C Hamano 2012-07-25 17:39 ` [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak Junio C Hamano 2012-08-05 18:45 ` [PATCH v3 0/2] allow recovery from command name typos Tay Ray Chuan 2012-08-05 18:45 ` [PATCH v3 1/2] add interface for /dev/tty interaction Tay Ray Chuan 2012-08-05 18:45 ` [PATCH v3 2/2] allow recovery from command name typos Tay Ray Chuan 2012-08-06 0:50 ` Junio C Hamano 2012-08-05 20:11 ` [PATCH v3 1/2] add interface for /dev/tty interaction Junio C Hamano 2012-08-06 19:45 ` Jeff King 2012-08-06 19:56 ` Jeff King 2012-08-06 20:01 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).