Bad DWIM response when git gui cannot start

Bad DWIM response when git gui cannot start

[Peter Krefting]

If git cannot start one of the external commands (at least some of them), 
the DWIM engine is a bit flaky:

   $ git citool
   /usr/local/libexec/git-core/git-citool: line 10: exec: wish: not found
   git: 'citool' is not a git-command. See 'git --help'.

   Did you mean this?
       citool

   $ git gui
   /usr/local/libexec/git-core/git-gui: line 10: exec: wish: not found
   git: 'gui' is not a git-command. See 'git --help'.

   Did you mean this?
       gui

-- 
\\// Peter - http://www.softwolves.pp.se/

[PATCH] help.c: don't blame an user's typo when the system is at fault

[Michele Ballabio]

As reported by Peter Krefting:

  If git cannot start one of the external commands (at least some of them),
  the DWIM engine is a bit flaky:

     $ git citool
     /usr/local/libexec/git-core/git-citool: line 10: exec: wish: not found
     git: 'citool' is not a git-command. See 'git --help'.

     Did you mean this?
         citool

Now we check whether the best bet found by levenshtein() differs from
the command line or not before proceeding.

The new error is:

$ git citool
/usr/local/libexec/git-core/git-citool: line 10: exec: wish: not found
fatal: Failed to run command 'citool': No such file or directory

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---

Is the call to strerror() useless anyway?

 help.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/help.c b/help.c
index fd87bb5..eec62a3 100644
--- a/help.c
+++ b/help.c
@@ -325,6 +325,9 @@ const char *help_unknown_cmd(const char *cmd)
 
 	if (!main_cmds.cnt)
 		die ("Uh oh. Your system reports no Git commands at all.");
+	if (!strcmp(cmd, main_cmds.names[0]->name))
+		die("Failed to run command '%s': %s\n",
+			cmd, strerror(errno));
 
 	best_similarity = main_cmds.names[0]->len;
 	n = 1;
-- 
1.6.3.1.17.g076c3

Re: [PATCH] help.c: don't blame an user's typo when the system is at fault

[Thomas Rast]

Michele Ballabio wrote:
> Is the call to strerror() useless anyway?
[...]
> +	if (!strcmp(cmd, main_cmds.names[0]->name))
> +		die("Failed to run command '%s': %s\n",
> +			cmd, strerror(errno));

The invocation of help_unknown_cmd comes from

	while (1) {
		// ...
		was_alias = run_argv(&argc, &argv);
		if (errno != ENOENT)
			break;
		// ... side branch with an exit() ...
		if (!done_help) {
			cmd = argv[0] = help_unknown_cmd(cmd);

so errno is always ENOENT when help_unknown_cmd() is called.
(Furthermore, the function itself uses git_config() and
load_command_list(), both of which _probably_ clobber errno, I don't
really have the time for an in-depth check.)

It also seems that the 'errno != ENOENT' check was intended to catch
the case where the command failed for any reason other than that it
does not exist, but this collides with the kernel reporting ENOENT if
the _interpreter_ does not exist.  Perhaps run_argv should
differentiate the case where a command executable exists but cannot be
run?


[I started writing a reply because I wanted to ask for a conversion to
die_errno() in the spirit of d824cbb (Convert existing die(...,
strerror(errno)) to die_errno(), 2009-06-27).  Please keep that in
mind if you put in another die() that mentions errno.]

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] help.c: don't blame an user's typo when the system is at fault

[Jeff King]

On Mon, Jul 20, 2009 at 04:17:47PM +0200, Thomas Rast wrote:

> The invocation of help_unknown_cmd comes from
> 
> 	while (1) {
> 		// ...
> 		was_alias = run_argv(&argc, &argv);
> 		if (errno != ENOENT)
> 			break;
> 		// ... side branch with an exit() ...
> 		if (!done_help) {
> 			cmd = argv[0] = help_unknown_cmd(cmd);
> 
> so errno is always ENOENT when help_unknown_cmd() is called.
> (Furthermore, the function itself uses git_config() and
> load_command_list(), both of which _probably_ clobber errno, I don't
> really have the time for an in-depth check.)
> 
> It also seems that the 'errno != ENOENT' check was intended to catch
> the case where the command failed for any reason other than that it
> does not exist, but this collides with the kernel reporting ENOENT if
> the _interpreter_ does not exist.  Perhaps run_argv should
> differentiate the case where a command executable exists but cannot be
> run?

Yes, I think double-checking the suggested commands list is only half of
it; it still says "citool is not a git command" which is wrong. Getting
it totally right means differentiating the two ENOENT cases, which I
think would require searching the PATH.

Something like the patch below should work, though I didn't think
terribly long about it, so there might be a corner case that isn't
covered, or some easier helper functions for accomplishing this.

---
diff --git a/git.c b/git.c
index 5da6c65..4e44c98 100644
--- a/git.c
+++ b/git.c
@@ -3,6 +3,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "run-command.h"
+#include "help.h"
 
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [--html-path] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
@@ -449,6 +450,16 @@ static int run_argv(int *argcp, const char ***argv)
 	return done_alias;
 }
 
+static int command_exists(const char *s)
+{
+	static struct cmdnames main_cmds, other_cmds;
+	static int loaded;
+	if (!loaded) {
+		load_command_list("git-", &main_cmds, &other_cmds);
+		loaded = 1;
+	}
+	return is_in_cmdlist(&main_cmds, s) || is_in_cmdlist(&other_cmds, s);
+}
 
 int main(int argc, const char **argv)
 {
@@ -504,7 +515,7 @@ int main(int argc, const char **argv)
 		static int done_help = 0;
 		static int was_alias = 0;
 		was_alias = run_argv(&argc, &argv);
-		if (errno != ENOENT)
+		if (errno != ENOENT || command_exists(argv[0]))
 			break;
 		if (was_alias) {
 			fprintf(stderr, "Expansion of alias '%s' failed; "