[PATCH 0/4] allow recovery from command name typos

[PATCH 0/4] allow recovery from command name typos

[Tay Ray Chuan]

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

[PATCH 1/4] help.c::uniq: plug a leak

[Tay Ray Chuan]

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

[PATCH 2/4] help.c::exclude_cmds: plug a leak

[Tay Ray Chuan]

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

[PATCH 3/4] help.c: plug a leak when help.autocorrect is set

[Tay Ray Chuan]

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

[PATCH 4/4] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH 4/4] allow recovery from command name typos

[Jeff King]

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

Re: [PATCH 4/4] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH 4/4] allow recovery from command name typos

[Thomas Rast]

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

Re: [PATCH 4/4] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH 4/4] allow recovery from command name typos

[Junio C Hamano]

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).

Re: [PATCH 4/4] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH 4/4] allow recovery from command name typos

[Junio C Hamano]

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.

Re: [PATCH 4/4] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH 1/4] help.c::uniq: plug a leak

[Jeff King]

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;
 }

Re: [PATCH 1/4] help.c::uniq: plug a leak

[Tay Ray Chuan]

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

Re: [PATCH 1/4] help.c::uniq: plug a leak

[Jeff King]

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

[PATCH v2 0/4] allow recovery from command name typos

[Tay Ray Chuan]

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

[PATCH v2 1/4] help.c::uniq: plug a leak

[Tay Ray Chuan]

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

[PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak

[Tay Ray Chuan]

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

[PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect

[Tay Ray Chuan]

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

[PATCH v2 4/4] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH v2 4/4] allow recovery from command name typos

[Junio C Hamano]

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).

Re: [PATCH v2 4/4] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH v2 4/4] allow recovery from command name typos

[Jeff King]

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

Re: [PATCH v2 4/4] allow recovery from command name typos

[Junio C Hamano]

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.

Re: [PATCH v2 4/4] allow recovery from command name typos

[Jeff King]

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

Re: [PATCH v2 4/4] allow recovery from command name typos

[Junio C Hamano]

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.

Re: [PATCH v2 3/4] help.c: plug leaks with(out) help.autocorrect

[Junio C Hamano]

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);
>  }

Re: [PATCH v2 2/4] help.c::exclude_cmds: realloc() before copy, plug a leak

[Junio C Hamano]

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++;
 	}
 

[PATCH v3 0/2] allow recovery from command name typos

[Tay Ray Chuan]

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

[PATCH v3 1/2] add interface for /dev/tty interaction

[Tay Ray Chuan]

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

[PATCH v3 2/2] allow recovery from command name typos

[Tay Ray Chuan]

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

Re: [PATCH v3 2/2] allow recovery from command name typos

[Junio C Hamano]

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?

Re: [PATCH v3 1/2] add interface for /dev/tty interaction

[Junio C Hamano]

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 */

Re: [PATCH v3 1/2] add interface for /dev/tty interaction

[Jeff King]

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

Re: [PATCH v3 1/2] add interface for /dev/tty interaction

[Jeff King]

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

Re: [PATCH v3 1/2] add interface for /dev/tty interaction

[Junio C Hamano]

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