git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 6, 2012, 00:50 UTC
Message-ID
<7vehnk3kti.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1344192340-19415-3-git-send-email-rctay89@gmail.com>
Tay Ray Chuan <rctay89@gmail.com> writes:
Show 53 quoted lines
> 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.

Show 9 quoted lines
> @@ -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.

Show 6 quoted lines
> +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.
Show 6 quoted lines
> +* 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
Show 27 quoted lines
> 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.
Show 13 quoted lines
>  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-<.

Show 17 quoted lines
> @@ -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.
Show 11 quoted lines
> +		} 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?
Show 5 quoted lines
> +		} 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 "/..."?

Show 11 quoted lines
> +			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?

Previous: Tay Ray ChuanNext: Junio C Hamano
Message 32 of 37 in “allow recovery from command name typos”
  1. 0/4 allow recovery from command name typosTay Ray Chuan, May 6, 2012
  2. 1/4 help.c::uniq: plug a leakTay Ray Chuan, May 6, 2012
  3. 2/4 help.c::exclude_cmds: plug a leakTay Ray Chuan, May 6, 2012
  4. 3/4 help.c: plug a leak when help.autocorrect is setTay Ray Chuan, May 6, 2012
  5. 4/4 allow recovery from command name typosTay Ray Chuan, May 6, 2012
  6. Jeff KingMay 6, 2012
  7. Tay Ray ChuanMay 6, 2012
  8. Thomas RastMay 7, 2012
  9. Tay Ray ChuanMay 7, 2012
  10. Junio C HamanoMay 7, 2012
  11. Tay Ray ChuanMay 9, 2012
  12. Junio C HamanoMay 9, 2012
  13. Jeff KingMay 6, 2012
  14. Tay Ray ChuanMay 6, 2012
  15. Jeff KingMay 7, 2012
  16. 0/4 allow recovery from command name typosTay Ray Chuan, Jul 25, 2012
  17. 1/4 help.c::uniq: plug a leakTay Ray Chuan, Jul 25, 2012
  18. 2/4 help.c::exclude_cmds: realloc() before copy, plug a leakTay Ray Chuan, Jul 25, 2012
  19. 3/4 help.c: plug leaks with(out) help.autocorrectTay Ray Chuan, Jul 25, 2012
  20. 4/4 allow recovery from command name typosTay Ray Chuan, Jul 25, 2012
  21. Junio C HamanoJul 25, 2012
  22. Tay Ray ChuanJul 26, 2012
  23. Jeff KingJul 26, 2012
  24. Junio C HamanoJul 26, 2012
  25. Jeff KingJul 26, 2012
  26. Junio C HamanoJul 26, 2012
  27. Junio C HamanoJul 25, 2012
  28. Junio C HamanoJul 25, 2012
  29. 0/2 allow recovery from command name typosTay Ray Chuan, Aug 5, 2012
  30. 1/2 add interface for /dev/tty interactionTay Ray Chuan, Aug 5, 2012
  31. 2/2 allow recovery from command name typosTay Ray Chuan, Aug 5, 2012
  32. Junio C HamanoAug 6, 2012
  33. Junio C HamanoAug 5, 2012
  34. Jeff KingAug 6, 2012
  35. Jeff KingAug 6, 2012
  36. Junio C HamanoAug 6, 2012
  37. Tay Ray ChuanMay 6, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.