Re: [PATCH] help.autocorrect: do not run a command if the command given is junk

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 14, 2009, 20:08 UTC
Message-ID: <7v7hspjp3q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4B263797.5070808@viscovery.net>

Johannes Sixt <j.sixt@viscovery.net> writes:

Show 23 quoted lines

> From: Johannes Sixt <j6t@kdbg.org>
>
> If a given command is not found, then help.c tries to guess which one the
> user could have meant. If help.autocorrect is 0 or unset, then a list of
> suggestions is given as long as the dissimilarity between the given command
> and the candidates is not excessively high. But if help.autocorrect was
> non-zero (i.e., a delay after which the command is run automatically), the
> latter restriction on dissimilarity was not obeyed.
>
> In my case, this happened:
>
>  $ git ..daab02
>  WARNING: You called a Git command named '..daab02', which does not exist.
>  Continuing under the assumption that you meant 'read-tree'
>  in 4.0 seconds automatically...
>
> The similarity limit that this patch introduces is already used a few lines
> later where the list of suggested commands is printed.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  help.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Thanks.  Will apply to 'maint'.

But I am curious about and would prefer to see the story behind '6' someday.

This is not entirely a new problem that you are introducing, as the same comparison-with-six appears in a later part of the code, but this patch duplicates the magic number whose two instances need to match, so in that sense it makes it more necessary than before to document the choice of magic number somewhere in a comment in the code.

Did 8af84da (git wrapper: DWIM mistyped commands, 2008-08-31) made up just a random number out of thin-air? What bad things would happen if we used '600' (or '1') instead of '6'? What kind of correlation does the cut-off value we use here should have with some intrinsic numbers your git installation has (e.g. perhaps "must be at least 80% of average length of available commands" or something like that)?

In the meantime, I think squashing the following in would help us keep the two magic numbers in sync.

diff --git a/help.c b/help.c
index db888cf..fbf80d9 100644
--- a/help.c
+++ b/help.c
@@ -297,6 +297,9 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
 	old->names = NULL;
 }
 
+/* how did we decide this is a good cutoff??? */
+#define SIMILAR_ENOUGH(x) ((x) < 6)
+
 const char *help_unknown_cmd(const char *cmd)
 {
 	int i, n, best_similarity = 0;
@@ -331,7 +334,7 @@ const char *help_unknown_cmd(const char *cmd)
 	n = 1;
 	while (n < main_cmds.cnt && best_similarity == main_cmds.names[n]->len)
 		++n;
-	if (autocorrect && n == 1 && best_similarity < 6) {
+	if (autocorrect && n == 1 && SIMILAR_ENOUGH(best_similarity)) {
 		const char *assumed = main_cmds.names[0]->name;
 		main_cmds.names[0] = NULL;
 		clean_cmdnames(&main_cmds);
@@ -349,7 +352,7 @@ const char *help_unknown_cmd(const char *cmd)
 
 	fprintf(stderr, "git: '%s' is not a git-command. See 'git --help'.\n", cmd);
 
-	if (best_similarity < 6) {
+	if (SIMILAR_ENOUGH(best_similarity)) {
 		fprintf(stderr, "\nDid you mean %s?\n",
 			n < 2 ? "this": "one of these");