Doesn't disambiguate between 'external command failed' and 'command not found'

Doesn't disambiguate between 'external command failed' and 'command not found'

[Alex Vandiver]

An external git-whatever command which fails to run gets reported as not
having been found, and suggests ... exactly what you just ran.  If you
miss the first line of the output, this can be extremely confusing --
and "is not a git command" is somewhat misleading:

        umgah ~ $ cat bin/git-bogus
        #!/usr/bin/env nonexistant
        echo "yay"
        
        umgah ~ $ git bogus
        /usr/bin/env: nonexistant: No such file or directory
        git: 'bogus' is not a git command. See 'git --help'.
        
        Did you mean this?
        	bogus

I've no patch right now, but I thought I'd report the frustration, in
case someone else wanted to get to it first.
 - Alex

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Michael Schubert]

Hi,

here is a tiny patch; maybe there is a cleaner way doing this.?

-- >8 --

Subject: [PATCH] help_unknown_cmd: do not propose an "unknown" cmd

When executing an external shell script like `git foo` with the following
shebang "#!/usr/bin/not/existing", execvp returns 127 (ENOENT). Since
help_unknown_cmd proposes the use of all external commands similar to
the name of the "unknown" command, it suggests the just failed command
again. Stop it.

Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
 help.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/help.c b/help.c
index 7654f1b..10b98ba 100644
--- a/help.c
+++ b/help.c
@@ -383,12 +383,24 @@ const char *help_unknown_cmd(const char *cmd)
 
 	fprintf(stderr, "git: '%s' is not a git command. See 'git --help'.\n", cmd);
 
-	if (SIMILAR_ENOUGH(best_similarity)) {
+	if (n==1 && !strcmp(cmd, main_cmds.names[0]->name))
+		;
+		/*
+		 * This avoids proposing the use of a command
+		 * which apparently just didn't work, e.g.
+		 * when executing a shell script git-foo with
+		 * the following shebang:
+		 *
+		 * 	#!/usr/bin/not/here
+		 *
+		 */
+	else if (SIMILAR_ENOUGH(best_similarity)) {
 		fprintf(stderr, "\nDid you mean %s?\n",
 			n < 2 ? "this": "one of these");
 
 		for (i = 0; i < n; i++)
-			fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
+			if (strcmp(cmd, main_cmds.names[i]->name))
+				fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);
 	}
 
 	exit(1);
-- 
1.7.6.132.gdca5

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Jeff King]

On Tue, Jul 05, 2011 at 10:41:37PM +0200, Michael Schubert wrote:

> Subject: [PATCH] help_unknown_cmd: do not propose an "unknown" cmd
> 
> When executing an external shell script like `git foo` with the following
> shebang "#!/usr/bin/not/existing", execvp returns 127 (ENOENT). Since
> help_unknown_cmd proposes the use of all external commands similar to
> the name of the "unknown" command, it suggests the just failed command
> again. Stop it.

Yeah, I don't think we can distinguish "not there" versus "bad
interpreter" just from the exec return. We would have to then search the
PATH to see if the file actually exists.

> -	if (SIMILAR_ENOUGH(best_similarity)) {
> +	if (n==1 && !strcmp(cmd, main_cmds.names[0]->name))
> +		;
> +		/*
> +		 * This avoids proposing the use of a command
> +		 * which apparently just didn't work, e.g.
> +		 * when executing a shell script git-foo with
> +		 * the following shebang:
> +		 *
> +		 * 	#!/usr/bin/not/here
> +		 *
> +		 */
> +	else if (SIMILAR_ENOUGH(best_similarity)) {

This misses the "autocorrect" case just above, which should not
autocorrect a command to itself (and I didn't try, but I assume it makes
more a really slow infinite loop).

So if you are going to follow this strategy, you are probably better to
just skip the entry (or give it a high levenshtein distance) in the main
loop where we calculate candidates.

But I wonder if we can do even better than just omitting it from the
candidates list. I mentioned searching the PATH above; but that is
exactly what load_command_list does to create this candidate list. So I
think the only way we can have an exact match is one of:

  1. There is a race condition. We tried to exec the command, and it was
     missing; meanwhile, another process created the command.

  2. Exec'ing the command returned ENOENT because of a bad interpreter.

Option (1) seems fairly unlikely; so maybe we should give the user some
advice about (2)?

Something like:

diff --git a/help.c b/help.c
index e925ca1..522b2ba 100644
--- a/help.c
+++ b/help.c
@@ -305,6 +305,10 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
 #define SIMILARITY_FLOOR 7
 #define SIMILAR_ENOUGH(x) ((x) < SIMILARITY_FLOOR)
 
+static const char bad_interpreter_advice[] =
+"'%s' appears to be a git command, but we were not able to\n"
+"execute it. Check the #!-line of the git-%s script.";
+
 const char *help_unknown_cmd(const char *cmd)
 {
 	int i, n, best_similarity = 0;
@@ -329,6 +333,14 @@ const char *help_unknown_cmd(const char *cmd)
 		int cmp = 0; /* avoid compiler stupidity */
 		const char *candidate = main_cmds.names[i]->name;
 
+		/*
+		 * An exact match means we have the command, but
+		 * for some reason exec'ing it gave us ENOENT; probably
+		 * it's a bad interpreter in the #! line.
+		 */
+		if (!strcmp(candidate, cmd))
+			die(bad_interpreter_advice, cmd, cmd);
+
 		/* Does the candidate appear in common_cmds list? */
 		while (n < ARRAY_SIZE(common_cmds) &&
 		       (cmp = strcmp(common_cmds[n].name, candidate)) < 0)

I'm not all that happy with the advice, though. It's pretty technical
and specific. I'm not sure whether it would be helpful to most users or
not.

-Peff

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Jeff King]

On Tue, Jul 05, 2011 at 07:16:05PM -0400, Jeff King wrote:

> So if you are going to follow this strategy, you are probably better to
> just skip the entry (or give it a high levenshtein distance) in the main
> loop where we calculate candidates.

And here's what that would look like.

diff --git a/help.c b/help.c
index e925ca1..15e6f0b 100644
--- a/help.c
+++ b/help.c
@@ -329,6 +329,11 @@ const char *help_unknown_cmd(const char *cmd)
 		int cmp = 0; /* avoid compiler stupidity */
 		const char *candidate = main_cmds.names[i]->name;
 
+		if (!strcmp(candidate, cmd)) {
+			main_cmds.names[i]->len = SIMILARITY_FLOOR + 1;
+			continue;
+		}
+
 		/* Does the candidate appear in common_cmds list? */
 		while (n < ARRAY_SIZE(common_cmds) &&
 		       (cmp = strcmp(common_cmds[n].name, candidate)) < 0)

I suspect it can create its own brand of confusion, though:

  $ cat `which git-broken`
  #!/bin/bogus
  $ git broken
  git: 'broken' is not a git command. See 'git --help'.

At which point I search through my PATH and confirm that indeed,
"git-broken" _is_ a git command. And I'm left on my own to figure out
that it's a broken #!-line.

So I think I prefer giving some more specific advice. Even if we don't
mention "#!" lines explicitly, saying "This exists, but exec didn't
work" is probably more helpful than pretending it's not there. It gives
clueful people an idea of where to start looking for the problem.

-Peff

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Sverre Rabbelier]

Heya,

On Wed, Jul 6, 2011 at 01:22, Jeff King <peff@peff.net> wrote:
> So I think I prefer giving some more specific advice. Even if we don't
> mention "#!" lines explicitly, saying "This exists, but exec didn't
> work" is probably more helpful than pretending it's not there. It gives
> clueful people an idea of where to start looking for the problem.

Seconded. We should at least give the user enough information to
figure out next steps. I like the advice from bad_interpreter_advice,
although it might be phrased more as a suggestion "Try looking at
..."?

-- 
Cheers,

Sverre Rabbelier

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Michael Schubert]

> So if you are going to follow this strategy, you are probably better to
> just skip the entry (or give it a high levenshtein distance) in the main
> loop where we calculate candidates.

Yes.

> But I wonder if we can do even better than just omitting it from the
> candidates list. I mentioned searching the PATH above; but that is
> exactly what load_command_list does to create this candidate list. So I
> think the only way we can have an exact match is one of:
> 
>   1. There is a race condition. We tried to exec the command, and it was
>      missing; meanwhile, another process created the command.
> 
>   2. Exec'ing the command returned ENOENT because of a bad interpreter.
> 
> Option (1) seems fairly unlikely; so maybe we should give the user some
> advice about (2)?

Like this? I've replaced "Check the #!-line of the git-%s script." with
"Maybe git-%s is broken?" to be less technical and specific..

-- >8 --

Subject: [PATCH] help_unknown_cmd: do not propose an "unknown" cmd

When executing an external shell script like `git foo` with the following
shebang "#!/usr/bin/not/existing", execvp returns 127 (ENOENT). Since
help_unknown_cmd proposes the use of all external commands similar to
the name of the "unknown" command, it suggests the just failed command
again. Stop it and give some advice to the user.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
 help.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/help.c b/help.c
index 7654f1b..a5a0613 100644
--- a/help.c
+++ b/help.c
@@ -302,6 +302,10 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
 #define SIMILARITY_FLOOR 7
 #define SIMILAR_ENOUGH(x) ((x) < SIMILARITY_FLOOR)
 
+static const char bad_interpreter_advice[] =
+	"'%s' appears to be a git command, but we were not\n"
+	"able to execute it. Maybe git-%s is broken?";
+
 const char *help_unknown_cmd(const char *cmd)
 {
 	int i, n, best_similarity = 0;
@@ -326,6 +330,14 @@ const char *help_unknown_cmd(const char *cmd)
 		int cmp = 0; /* avoid compiler stupidity */
 		const char *candidate = main_cmds.names[i]->name;
 
+		/*
+		 * An exact match means we have the command, but
+		 * for some reason exec'ing it gave us ENOENT; probably
+		 * it's a bad interpreter in the #! line.
+		 */
+		if (!strcmp(candidate, cmd))
+			die(bad_interpreter_advice, cmd, cmd);
+
 		/* Does the candidate appear in common_cmds list? */
 		while (n < ARRAY_SIZE(common_cmds) &&
 		       (cmp = strcmp(common_cmds[n].name, candidate)) < 0)
-- 
1.7.6.132.g91c244

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Jeff King]

On Wed, Jul 06, 2011 at 01:47:33PM +0200, Michael Schubert wrote:

> Like this? I've replaced "Check the #!-line of the git-%s script." with
> "Maybe git-%s is broken?" to be less technical and specific..

Yeah, looks good to me (unless somebody wants to do something more
elaborate to catch other exec problems, but I personally don't think
it's worth the effort).

-Peff

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Jeff King]

On Wed, Jul 06, 2011 at 01:58:03PM -0400, Jeff King wrote:

> > Like this? I've replaced "Check the #!-line of the git-%s script." with
> > "Maybe git-%s is broken?" to be less technical and specific..
> 
> Yeah, looks good to me (unless somebody wants to do something more
> elaborate to catch other exec problems, but I personally don't think
> it's worth the effort).

One minor nit, though. I haven't been paying attention to the progress
of the gettext topics, but should this message:

> +static const char bad_interpreter_advice[] =
> +	"'%s' appears to be a git command, but we were not\n"
> +	"able to execute it. Maybe git-%s is broken?";

Actually be inside _() for gettext?

-Peff

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 06, 2011 at 01:58:03PM -0400, Jeff King wrote:
>
>> > Like this? I've replaced "Check the #!-line of the git-%s script." with
>> > "Maybe git-%s is broken?" to be less technical and specific..
>> 
>> Yeah, looks good to me (unless somebody wants to do something more
>> elaborate to catch other exec problems, but I personally don't think
>> it's worth the effort).
>
> One minor nit, though. I haven't been paying attention to the progress
> of the gettext topics, but should this message:
>
>> +static const char bad_interpreter_advice[] =
>> +	"'%s' appears to be a git command, but we were not\n"
>> +	"able to execute it. Maybe git-%s is broken?";
>
> Actually be inside _() for gettext?

I would mark it with N_() and then the calling site inside die() with _()
if I were doing this.

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Michael Schubert]

On 07/07/2011 01:25 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>> One minor nit, though. I haven't been paying attention to the progress
>> of the gettext topics, but should this message:
>>
>>> +static const char bad_interpreter_advice[] =
>>> +	"'%s' appears to be a git command, but we were not\n"
>>> +	"able to execute it. Maybe git-%s is broken?";
>>
>> Actually be inside _() for gettext?
> 
> I would mark it with N_() and then the calling site inside die() with _()
> if I were doing this.

Sorry for the delay.

-- >8 --

Subject: [PATCH] help_unknown_cmd: do not propose an "unknown" cmd

When executing an external shell script like `git foo` with the following
shebang "#!/usr/bin/not/existing", execvp returns 127 (ENOENT). Since
help_unknown_cmd proposes the use of all external commands similar to
the name of the "unknown" command, it suggests the just failed command
again. Stop it and give some advice to the user.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
 help.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/help.c b/help.c
index 7654f1b..4219355 100644
--- a/help.c
+++ b/help.c
@@ -302,6 +302,10 @@ static void add_cmd_list(struct cmdnames *cmds, struct cmdnames *old)
 #define SIMILARITY_FLOOR 7
 #define SIMILAR_ENOUGH(x) ((x) < SIMILARITY_FLOOR)
 
+static const char bad_interpreter_advice[] =
+	N_("'%s' appears to be a git command, but we were not\n"
+	"able to execute it. Maybe git-%s is broken?");
+
 const char *help_unknown_cmd(const char *cmd)
 {
 	int i, n, best_similarity = 0;
@@ -326,6 +330,14 @@ const char *help_unknown_cmd(const char *cmd)
 		int cmp = 0; /* avoid compiler stupidity */
 		const char *candidate = main_cmds.names[i]->name;
 
+		/*
+		 * An exact match means we have the command, but
+		 * for some reason exec'ing it gave us ENOENT; probably
+		 * it's a bad interpreter in the #! line.
+		 */
+		if (!strcmp(candidate, cmd))
+			die(_(bad_interpreter_advice), cmd, cmd);
+
 		/* Does the candidate appear in common_cmds list? */
 		while (n < ARRAY_SIZE(common_cmds) &&
 		       (cmp = strcmp(common_cmds[n].name, candidate)) < 0)
-- 
1.7.6.132.g91c244.dirty

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Junio C Hamano]

Michael Schubert <mschub@elegosoft.com> writes:

> Sorry for the delay.

Thanks.

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not all that happy with the advice, though. It's pretty technical
> and specific. I'm not sure whether it would be helpful to most users or
> not.

Yeah, Michael's rewording makes it fuzzier by saying "exists, unable to
execute, maybe git-%s is broken?".

I notice that we do not give the path to the file that implements the
command. Perhaps we should walk the $PATH after we see this failure to
pinpoint which one is to be inspected (I vaguely recall a weatherbaloon
patch to a similar effect)?

Re: Doesn't disambiguate between 'external command failed' and 'command not found'

[Jeff King]

On Wed, Jul 06, 2011 at 10:24:57AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm not all that happy with the advice, though. It's pretty technical
> > and specific. I'm not sure whether it would be helpful to most users or
> > not.
> 
> Yeah, Michael's rewording makes it fuzzier by saying "exists, unable to
> execute, maybe git-%s is broken?".

Yeah, I like his better.

> I notice that we do not give the path to the file that implements the
> command. Perhaps we should walk the $PATH after we see this failure to
> pinpoint which one is to be inspected (I vaguely recall a weatherbaloon
> patch to a similar effect)?

That would be better still. But I don't know how much effort this is
really worth. It is about catching one specific uncommon
misconfiguration. If it were part of a more general exec wrapper that
gave better output (which I think is the weatherballoon you mean, that
you did a month or three ago), I think it might be more worthwhile.

But even then, I seem to remember the discussion fizzling out to "is
this really that common a problem?"

So I'm happy with just taking Michael's patch.

-Peff