Re: Doesn't disambiguate between 'external command failed' and 'command not found'
- From
Jeff King <peff@peff.net>
- Date
- Jul 5, 2011, 23:16 UTC
- Message-ID
- <20110705231604.GC12085@sigill.intra.peff.net>
- In-Reply-To
- <4E137701.1020007@elegosoft.com>
On Tue, Jul 05, 2011 at 10:41:37PM +0200, Michael Schubert wrote:
Show 7 quoted lines
> 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.
Show 13 quoted lines
> - 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