[PATCH 0/6 v3] Add execvp failure diagnostics

[PATCH 0/6 v3] Add execvp failure diagnostics

[Frans Klaver]

This patch series replaces $gmane/187026. It aims to improve the
information provided after execvp fails in run-command.

This series takes a rather different approach than the previous
one. In the former the focus was on determining what might cause
an EACCES failure. This series focuses on trying to provide hints
on what went wrong on any error.

The resulting checking produces behavior rather like bash, with the
notable exception that we consider "Not found and PATH access issues"
a failure, where bash doesn't.

[PATCH 1/5] t0061: Fix incorrect indentation
[PATCH 2/5] t0061: Add tests
[PATCH 3/5] run-command: Elaborate execvp error checking
[PATCH 4/5] run-command: Warn if PATH entry cannot be searched
[PATCH 5/5] run-command: Error out if interpreter not found

run-command.c          |  140 +++++++++++++++++++++++++++++++++++++++++++++---
t/t0061-run-command.sh |   57 ++++++++++++++++++-
2 files changed, 186 insertions(+), 11 deletions(-)

[PATCH 1/5] t0061: Fix incorrect indentation

[Frans Klaver]

The hello.sh script started with <tab>#!, which is not considered a
correct hash-bang line. The default interpreter happened to be the one
required for this specific test.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 t/t0061-run-command.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 8d4938f..95e89bc 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -8,8 +8,8 @@ test_description='Test run command'
 . ./test-lib.sh
 
 cat >hello-script <<-EOF
-	#!$SHELL_PATH
-	cat hello-script
+#!$SHELL_PATH
+cat hello-script
 EOF
 >empty
 
-- 
1.7.8.1

[PATCH 2/5] t0061: Add tests

[Frans Klaver]

Capture failure behavior when running into
- EACCES caused by search path permissions
- ENOENT caused by interpreter not found

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 t/t0061-run-command.sh |   50 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 49 insertions(+), 1 deletions(-)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 95e89bc..31eb3c3 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -13,6 +13,24 @@ cat hello-script
 EOF
 >empty
 
+cat >someinterpreter <<-EOF
+#!$SHELL_PATH
+cat hello-script
+EOF
+>empty
+
+cat >incorrect-interpreter-script <<-EOF
+#!someinterpreter
+cat hello-script
+EOF
+>empty
+
+cat >non-existing-interpreter <<-EOF
+#!nonexisting_interpreter
+cat hello-script
+EOF
+>empty
+
 test_expect_success 'start_command reports ENOENT' '
 	test-run-command start-command-ENOENT ./does-not-exist
 '
@@ -26,7 +44,7 @@ test_expect_success 'run_command can run a command' '
 	test_cmp empty err
 '
 
-test_expect_success POSIXPERM 'run_command reports EACCES' '
+test_expect_success POSIXPERM 'run_command reports EACCES, file permissions' '
 	cat hello-script >hello.sh &&
 	chmod -x hello.sh &&
 	test_must_fail test-run-command run-command ./hello.sh 2>err &&
@@ -34,4 +52,34 @@ test_expect_success POSIXPERM 'run_command reports EACCES' '
 	grep "fatal: cannot exec.*hello.sh" err
 '
 
+test_expect_success POSIXPERM 'run_command reports EACCES, search path perms' '
+	mkdir -p inaccessible &&
+	PATH=$(pwd)/inaccessible:$PATH &&
+	export PATH &&
+
+	cat hello-script >inaccessible/hello.sh &&
+	chmod 400 inaccessible &&
+	test_must_fail test-run-command run-command hello.sh 2>err &&
+	chmod 755 inaccessible &&
+
+	grep "fatal: cannot exec.*hello.sh" err
+'
+
+test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
+	cat incorrect-interpreter-script >hello.sh &&
+	chmod +x hello.sh &&
+	chmod -x someinterpreter &&
+	test_must_fail test-run-command run-command ./hello.sh 2>err &&
+
+	grep "fatal: cannot exec.*hello.sh" err
+'
+
+test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
+	cat non-existing-interpreter >hello.sh &&
+	chmod +x hello.sh &&
+	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
+
+	grep "error: cannot exec.*hello.sh" err
+'
+
 test_done
-- 
1.7.8.1

[PATCH 3/5] run-command: Elaborate execvp error checking

[Frans Klaver]

The interpretation of errors from execvp was rather terse. For user
convenience communication of the nature of the error can be improved.

This patch introduces a function with more elaborate investigation of
the errors encountered. After a failure occurs on a program or script,
inspect_failure will run through the PATH entries if necessary, and if
the file exists, perform some tests on the file. If nothing is found,
ENOENT is returned.

The function inspect_file() will try to find out if the file itself or
an interpreter caused the issue. Some scripts don't have the hash-bang
and will not be reported as scripts. If the interpreter cannot be found,
the existing behavior is kept -- i.e. ENOENT will be reported to the
parent process.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |  144 +++++++++++++++++++++++++++++++++++++++++++++---
 t/t0061-run-command.sh |    6 +-
 2 files changed, 140 insertions(+), 10 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1c51043..17a65fe 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,6 +2,7 @@
 #include "run-command.h"
 #include "exec_cmd.h"
 #include "argv-array.h"
+#include "dir.h"
 
 static inline void close_pair(int fd[2])
 {
@@ -134,6 +135,140 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 	return code;
 }
 
+#ifndef WIN32
+static int is_executable_file(const char *path)
+{
+	return access(path, X_OK) == 0 && !is_directory(path);
+}
+
+static int is_searchable(const char *path)
+{
+	return access(path, X_OK) == 0 && is_directory(path);
+}
+
+static void die_file_error(const char *file, int err)
+{
+	die("cannot exec '%s': %s", file, strerror(err));
+}
+
+static char *get_interpreter(const char *first_line)
+{
+	struct strbuf sb = STRBUF_INIT;
+	size_t start = strspn(first_line + 2, " \t") + 2;
+	size_t end = strcspn(first_line + start, " \t\r\n") + start;
+
+	if (start >= end)
+		return NULL;
+
+	strbuf_add(&sb, first_line + start, end - start);
+	return strbuf_detach(&sb, NULL);
+}
+
+static void inspect_file(struct strbuf *fn, int err, const char *argv0)
+{
+	/*
+	 * Typical line length of 80. BSD only allows 32 bytes, but that
+	 * won't really make a difference.
+	 */
+	char buf[80];
+	size_t read;
+	FILE *file;
+	if (!is_executable_file(fn->buf)) {
+		strbuf_release(fn);
+		die_file_error(argv0, err);
+	}
+
+	file = fopen(fn->buf, "r");
+	if (!file) {
+		strbuf_release(fn);
+		die_file_error(argv0, err);
+	}
+
+	read = fread(buf, 1, sizeof(buf), file);
+	fclose(file);
+	if (read > 2 && buf[0] == '#' && buf[1] == '!') {
+		char *i = get_interpreter(buf);
+		char es[1024];
+		if (i) {
+			sprintf(es, "cannot exec '%s': "
+					"bad interpreter '%s': %s",
+					argv0, i,
+					strerror(err));
+		} else {
+			sprintf(es, "cannot exec '%s': "
+					"bad interpreter: %s",
+					argv0, strerror(err));
+		}
+		free(i);
+		strbuf_release(fn);
+		if (err == ENOENT) {
+			error("%s", es);
+			exit(127);
+		} else {
+			die("%s", es);
+		}
+	}
+
+	strbuf_release(fn);
+	die_file_error(argv0, err);
+}
+
+static void inspect_failure(const char *argv0, int silent_exec_failure)
+{
+	int err = errno;
+	struct strbuf sb = STRBUF_INIT;
+
+	/* errors not related to path */
+	if (errno == E2BIG || errno == ENOMEM)
+		die_file_error(argv0, err);
+
+	if (strchr(argv0, '/')) {
+		if (file_exists(argv0)) {
+			strbuf_add(&sb, argv0, strlen(argv0));
+			inspect_file(&sb, err, argv0);
+		}
+	} else {
+		char *path, *next;
+		path = getenv("PATH");
+		while (path) {
+			next = strchrnul(path, ':');
+			if (path < next)
+				strbuf_add(&sb, path, next - path);
+			else
+				strbuf_addch(&sb, '.');
+
+			if (!*next)
+				path = NULL;
+			else
+				path = next + 1;
+
+			if (!is_searchable(sb.buf)) {
+				strbuf_release(&sb);
+				continue;
+			}
+
+			if (sb.len && sb.buf[sb.len - 1] != '/')
+				strbuf_addch(&sb, '/');
+			strbuf_addstr(&sb, argv0);
+
+			if (file_exists(sb.buf))
+				inspect_file(&sb, err, argv0);
+
+			strbuf_release(&sb);
+		}
+	}
+
+	if (err == ENOENT) {
+		if (!silent_exec_failure)
+			error("cannot exec '%s': %s", argv0,
+					strerror(ENOENT));
+		exit(127);
+	} else {
+		die_file_error(argv0, err);
+	}
+}
+#endif
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -280,14 +415,7 @@ fail_pipe:
 		} else {
 			execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
-		if (errno == ENOENT) {
-			if (!cmd->silent_exec_failure)
-				error("cannot run %s: %s", cmd->argv[0],
-					strerror(ENOENT));
-			exit(127);
-		} else {
-			die_errno("cannot exec '%s'", cmd->argv[0]);
-		}
+		inspect_failure(cmd->argv[0], cmd->silent_exec_failure);
 	}
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 31eb3c3..14b4ea6 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -71,7 +71,8 @@ test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
 	chmod -x someinterpreter &&
 	test_must_fail test-run-command run-command ./hello.sh 2>err &&
 
-	grep "fatal: cannot exec.*hello.sh" err
+	grep "fatal: cannot exec.*hello.sh" err &&
+	grep "bad interpreter" err
 '
 
 test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
@@ -79,7 +80,8 @@ test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
 	chmod +x hello.sh &&
 	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
 
-	grep "error: cannot exec.*hello.sh" err
+	grep "error: cannot exec.*hello.sh" err &&
+	grep "bad interpreter" err
 '
 
 test_done
-- 
1.7.8.1

[PATCH 4/5] run-command: Warn if PATH entry cannot be searched

[Frans Klaver]

execvp will return EACCES when PATH entries are encountered that cannot
be searched and the requested command isn't found. Since this error only
arises in that particular case, warning the user is more appropriate
than producing an error.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |    1 +
 t/t0061-run-command.sh |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 17a65fe..ab14910 100644
--- a/run-command.c
+++ b/run-command.c
@@ -243,6 +243,7 @@ static void inspect_failure(const char *argv0, int silent_exec_failure)
 				path = next + 1;
 
 			if (!is_searchable(sb.buf)) {
+				warning("cannot search '%s'", sb.buf);
 				strbuf_release(&sb);
 				continue;
 			}
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 14b4ea6..a4585b0 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -62,7 +62,8 @@ test_expect_success POSIXPERM 'run_command reports EACCES, search path perms' '
 	test_must_fail test-run-command run-command hello.sh 2>err &&
 	chmod 755 inaccessible &&
 
-	grep "fatal: cannot exec.*hello.sh" err
+	grep "fatal: cannot exec.*hello.sh" err &&
+	grep "cannot search" err
 '
 
 test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
-- 
1.7.8.1

[PATCH 5/5] run-command: Error out if interpreter not found

[Frans Klaver]

If the interpreter wasn't found, execvp returns ENOENT. The existing
error checking did not differentiate between file not found and
interpreter not found. While the former may be an incentive to start
inspecting aliases, the latter is an error because the desired script is
actually found.

This patch explicitly makes the interpreter failure a fatal error.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
As far as I'm concerned, this is a bug fix. However, since it really is
a change in git's behavior, we can still consider postponing this patch.

 run-command.c          |    7 +------
 t/t0061-run-command.sh |    4 ++--
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/run-command.c b/run-command.c
index ab14910..9179daf 100644
--- a/run-command.c
+++ b/run-command.c
@@ -201,12 +201,7 @@ static void inspect_file(struct strbuf *fn, int err, const char *argv0)
 		}
 		free(i);
 		strbuf_release(fn);
-		if (err == ENOENT) {
-			error("%s", es);
-			exit(127);
-		} else {
-			die("%s", es);
-		}
+		die("%s", es);
 	}
 
 	strbuf_release(fn);
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index a4585b0..f08163f 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -76,12 +76,12 @@ test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
 	grep "bad interpreter" err
 '
 
-test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
+test_expect_success POSIXPERM 'run_command reports ENOENT, interpreter' '
 	cat non-existing-interpreter >hello.sh &&
 	chmod +x hello.sh &&
 	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
 
-	grep "error: cannot exec.*hello.sh" err &&
+	grep "fatal: cannot exec.*hello.sh" err &&
 	grep "bad interpreter" err
 '
 
-- 
1.7.8.1

Re: [PATCH 1/5] t0061: Fix incorrect indentation

[Junio C Hamano]

Frans Klaver <fransklaver@gmail.com> writes:

> The hello.sh script started with <tab>#!, which is not considered a
> correct hash-bang line.

Isn't that exactly the reason why we start the here text with "<<-EOF",
not the usual "<<EOF"?

> required for this specific test.
>
> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
> ---
>  t/t0061-run-command.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 8d4938f..95e89bc 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -8,8 +8,8 @@ test_description='Test run command'
>  . ./test-lib.sh
>  
>  cat >hello-script <<-EOF
> -	#!$SHELL_PATH
> -	cat hello-script
> +#!$SHELL_PATH
> +cat hello-script
>  EOF
>  >empty

Re: [PATCH 1/5] t0061: Fix incorrect indentation

[Jonathan Nieder]

Frans Klaver wrote:

> +++ b/t/t0061-run-command.sh
> @@ -8,8 +8,8 @@ test_description='Test run command'
> . ./test-lib.sh
>
> cat >hello-script <<-EOF
> -	#!$SHELL_PATH
> -	cat hello-script
> +#!$SHELL_PATH
> +cat hello-script
>  EOF

Looks like a no-op --- the script already started with #! and no
leading tab for me.  Does it behave differently on your machine?

Re: [PATCH 2/5] t0061: Add tests

[Jonathan Nieder]

Hi,

Frans Klaver wrote:

> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,6 +13,24 @@ cat hello-script
>  EOF
>  >empty
>  
> +cat >someinterpreter <<-EOF
> +#!$SHELL_PATH
> +cat hello-script
> +EOF
> +>empty
> +
> +cat >incorrect-interpreter-script <<-EOF
> +#!someinterpreter
> +cat hello-script
> +EOF
> +>empty

Thanks for writing tests.  Some hints on mechanics below, and one on
strategy (see (*) below).

What is the point of repreatedly writing an empty file named "empty"?

I think this would be easier to read and maintain if scripts not
shared between multiple tests were written in the body of the relevant
tests.  For example, that way it is easier to remember to remove a
helper script if the relevant test assertion changes to no longer need
it.

[...]
> @@ -26,7 +44,7 @@ test_expect_success 'run_command can run a command' '
>  	test_cmp empty err
>  '
>  
>  test_expect_success POSIXPERM 'run_command reports EACCES' '
>  	cat hello-script >hello.sh &&
>  	chmod -x hello.sh &&
>  	test_must_fail test-run-command run-command ./hello.sh 2>err &&
[...]
> +test_expect_success POSIXPERM 'run_command reports EACCES, search path perms' '
> +	mkdir -p inaccessible &&
> +	PATH=$(pwd)/inaccessible:$PATH &&
> +	export PATH &&
> +
> +	cat hello-script >inaccessible/hello.sh &&
> +	chmod 400 inaccessible &&
> +	test_must_fail test-run-command run-command hello.sh 2>err &&
> +	chmod 755 inaccessible &&
> +
> +	grep "fatal: cannot exec.*hello.sh" err
> +'

(*) These tests would be easier to understand if squashed with the
relevant later patch in the series that changes the error message.

Maybe they could be less repetitive that way, too.

	test_expect_success POSIXPERM 'diagnose command in inaccessible part of $PATH' '
		mkdir -p subdir &&
		cat hello-script >subdir/hello.sh &&
		chmod +x subdir/hello.sh &&
		chmod -x subdir &&
		(
			PATH=$(pwd)/inaccessible:$PATH &&
			test_must_fail test-run-command run-command hello.sh 2>err
		) &&
		test_i18ngrep ...
	'

[...]
> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
> +	cat incorrect-interpreter-script >hello.sh &&
> +	chmod +x hello.sh &&
> +	chmod -x someinterpreter &&
> +	test_must_fail test-run-command run-command ./hello.sh 2>err &&
> +
> +	grep "fatal: cannot exec.*hello.sh" err
> +'

Is this the common case?  Why would my interpreter be in the designated
spot but not marked executable?  Is there some other motivating
example?  (I'm genuinely curious; it's ok if the answer is "no".)

[...]
> +
> +test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
> +	cat non-existing-interpreter >hello.sh &&
> +	chmod +x hello.sh &&
> +	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
> +
> +	grep "error: cannot exec.*hello.sh" err
> +'

Maybe:

	test_expect_success POSIXPERM 'diagnose missing interpreter' '
		echo "#!/nonexistent/interpreter" >hello.sh &&
		chmod +x hello.sh &&
		test_must_fail test-run-command run-command hello.sh 2>err &&
		test_i18ngrep ...
	'

Hope that helps,
Jonathan

Re: [PATCH 3/5] run-command: Elaborate execvp error checking

[Jonathan Nieder]

Klaver wrote:

> The interpretation of errors from execvp was rather terse. For user
> convenience communication of the nature of the error can be improved.

Could you give an example?

[...]
> --- a/run-command.c
> +++ b/run-command.c
> @@ -2,6 +2,7 @@
>  #include "run-command.h"
>  #include "exec_cmd.h"
>  #include "argv-array.h"
> +#include "dir.h"
>  
>  static inline void close_pair(int fd[2])
>  {
> @@ -134,6 +135,140 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
>  	return code;
>  }
>  
> +#ifndef WIN32

Not related to this patch, but I wonder if there should be a separate
run-command-unix.c file so these ifdefs would no longer be necessary.

What happens on Windows?

> +static void die_file_error(const char *file, int err)
> +{
> +	die("cannot exec '%s': %s", file, strerror(err));
> +}

I suspect it might be clearer to use die() inline in the two call
sites so the reader does not have to figure out the calling
convention.

> +
> +static char *get_interpreter(const char *first_line)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	size_t start = strspn(first_line + 2, " \t") + 2;
> +	size_t end = strcspn(first_line + start, " \t\r\n") + start;
> +
> +	if (start >= end)
> +		return NULL;
> +
> +	strbuf_add(&sb, first_line + start, end - start);
> +	return strbuf_detach(&sb, NULL);
> +}

What does this function do?  What happens if first_line doesn't start
with "#!"?  What should happen when there is a newline instead of a
command name?  How about commands with quoting characters like " and
backslash --- are the semantics portable in these cases?

No need to use a strbuf here: xmemdupz would take care of the
allocation and copy more simply.

> +static void inspect_failure(const char *argv0, int silent_exec_failure)
> +{
> +	int err = errno;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	/* errors not related to path */
> +	if (errno == E2BIG || errno == ENOMEM)
> +		die_file_error(argv0, err);
> +
> +	if (strchr(argv0, '/')) {
> +		if (file_exists(argv0)) {
> +			strbuf_add(&sb, argv0, strlen(argv0));
> +			inspect_file(&sb, err, argv0);
> +		}
> +	} else {
> +		char *path, *next;
> +		path = getenv("PATH");

I wonder if it's possible to rearrange this code to avoid deep
nesting.  What does the function do, anyway?  (If the reader has to
ask, it needs a comment or to be renamed.)

I guess the idea is to diagnose after the fact why execvp failed.
Might be simplest like this:

	To diagnose execvp failure:
		if filename does not contain a '/':
			if we can't find it on the search path:
				That's the problem, dummy!
			replace filename with full path
		if file does not exist:
			just report strerror(errno)
		if not executable:
			...
		if interpreter does not exist:
			...
		if interpreter not executable:
			...
		otherwise, just report strerror(errno)

with a separate function to find a command on the PATH, complaining
when it encounters an unsearchable entry.

Thanks for a fun read.

Jonathan

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Jonathan Nieder]

Frans Klaver wrote:

> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -76,12 +76,12 @@ test_expect_success POSIXPERM 'run_command reports EACCES, interpreter fails' '
>  	grep "bad interpreter" err
>  '
>  
> -test_expect_failure POSIXPERM 'run_command reports ENOENT, interpreter' '
> +test_expect_success POSIXPERM 'run_command reports ENOENT, interpreter' '
>  	cat non-existing-interpreter >hello.sh &&
>  	chmod +x hello.sh &&
>  	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err &&
>  
> -	grep "error: cannot exec.*hello.sh" err &&
> +	grep "fatal: cannot exec.*hello.sh" err &&

Thanks.  I'd suggest using "test_expect_code" rather than the detailed
wording of the message, since that is what scripts might want to rely
on.

What happens on Windows?

Re: [PATCH 1/5] t0061: Fix incorrect indentation

[Frans Klaver]

On Tue, 24 Jan 2012 23:40:00 +0100, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

> Frans Klaver wrote:
>
>> +++ b/t/t0061-run-command.sh
>> @@ -8,8 +8,8 @@ test_description='Test run command'
>> . ./test-lib.sh
>>
>> cat >hello-script <<-EOF
>> -	#!$SHELL_PATH
>> -	cat hello-script
>> +#!$SHELL_PATH
>> +cat hello-script
>>  EOF
>
> Looks like a no-op --- the script already started with #! and no
> leading tab for me.  Does it behave differently on your machine?

Hurr? I'm fairly sure the script ended up being indented for me. I'll  
recheck.

Re: [PATCH 2/5] t0061: Add tests

[Frans Klaver]

On Tue, 24 Jan 2012 23:56:36 +0100, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

>> +>empty
>> +
>> +cat >incorrect-interpreter-script <<-EOF
>> +#!someinterpreter
>> +cat hello-script
>> +EOF
>> +>empty
>
> What is the point of repreatedly writing an empty file named "empty"?

There isn't. I copied it along with the hello-script lines and didn't pay  
it much heed. I'll remove the excessive '>empty's.


> I think this would be easier to read and maintain if scripts not
> shared between multiple tests were written in the body of the relevant
> tests.  For example, that way it is easier to remember to remove a
> helper script if the relevant test assertion changes to no longer need
> it.

Makes sense. I'll reorder.


>
> [...]
>> @@ -26,7 +44,7 @@ test_expect_success 'run_command can run a command' '
>>  	test_cmp empty err
>>  '
>>
>>  test_expect_success POSIXPERM 'run_command reports EACCES' '
>>  	cat hello-script >hello.sh &&
>>  	chmod -x hello.sh &&
>>  	test_must_fail test-run-command run-command ./hello.sh 2>err &&
> [...]
>> +test_expect_success POSIXPERM 'run_command reports EACCES, search path  
>> perms' '
>> +	mkdir -p inaccessible &&
>> +	PATH=$(pwd)/inaccessible:$PATH &&
>> +	export PATH &&
>> +
>> +	cat hello-script >inaccessible/hello.sh &&
>> +	chmod 400 inaccessible &&
>> +	test_must_fail test-run-command run-command hello.sh 2>err &&
>> +	chmod 755 inaccessible &&
>> +
>> +	grep "fatal: cannot exec.*hello.sh" err
>> +'
>
> (*) These tests would be easier to understand if squashed with the
> relevant later patch in the series that changes the error message.

You mean "Elaborate execvp error checking"?


> Maybe they could be less repetitive that way, too.
>
> 	test_expect_success POSIXPERM 'diagnose command in inaccessible part of  
> $PATH' '
> 		mkdir -p subdir &&
> 		cat hello-script >subdir/hello.sh &&
> 		chmod +x subdir/hello.sh &&
> 		chmod -x subdir &&
> 		(
> 			PATH=$(pwd)/inaccessible:$PATH &&
> 			test_must_fail test-run-command run-command hello.sh 2>err
> 		) &&
> 		test_i18ngrep ...
> 	'
>
> [...]
>> +test_expect_success POSIXPERM 'run_command reports EACCES, interpreter  
>> fails' '
>> +	cat incorrect-interpreter-script >hello.sh &&
>> +	chmod +x hello.sh &&
>> +	chmod -x someinterpreter &&
>> +	test_must_fail test-run-command run-command ./hello.sh 2>err &&
>> +
>> +	grep "fatal: cannot exec.*hello.sh" err
>> +'
>
> Is this the common case?  Why would my interpreter be in the designated
> spot but not marked executable?  Is there some other motivating
> example?  (I'm genuinely curious; it's ok if the answer is "no".)

I wouldn't think so. This particular one is addressing a concern raised by  
Johannes Sixt in reaction to a patch from Junio.

http://article.gmane.org/gmane.comp.version-control.git/171848


>
> [...]
>> +
>> +test_expect_failure POSIXPERM 'run_command reports ENOENT,  
>> interpreter' '
>> +	cat non-existing-interpreter >hello.sh &&
>> +	chmod +x hello.sh &&
>> +	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err  
>> &&
>> +
>> +	grep "error: cannot exec.*hello.sh" err
>> +'
>
> Maybe:
>
> 	test_expect_success POSIXPERM 'diagnose missing interpreter' '
> 		echo "#!/nonexistent/interpreter" >hello.sh &&
> 		chmod +x hello.sh &&
> 		test_must_fail test-run-command run-command hello.sh 2>err &&
> 		test_i18ngrep ...
> 	'

Will check.


> Hope that helps,
> Jonathan

Re: [PATCH 1/5] t0061: Fix incorrect indentation

[Junio C Hamano]

"Frans Klaver" <fransklaver@gmail.com> writes:

> On Tue, 24 Jan 2012 23:40:00 +0100, Jonathan Nieder
> <jrnieder@gmail.com> wrote:
>
>> Frans Klaver wrote:
>>
>>> +++ b/t/t0061-run-command.sh
>>> @@ -8,8 +8,8 @@ test_description='Test run command'
>>> . ./test-lib.sh
>>>
>>> cat >hello-script <<-EOF
>>> -	#!$SHELL_PATH
>>> -	cat hello-script
>>> +#!$SHELL_PATH
>>> +cat hello-script
>>>  EOF
>>
>> Looks like a no-op --- the script already started with #! and no
>> leading tab for me.  Does it behave differently on your machine?
>
> Hurr? I'm fairly sure the script ended up being indented for me. I'll
> recheck.

It could be that your shell is broken and does not understand the
distinction between the "<<-EOF" vs "<<EOF". What system are you on? If
the problem is real and widespread, we might want to mention it in INSTALL
or Makefile, just like we label Solaris /bin/sh as unusable and advise
people to use /usr/xpg[46]/bin variant.

Re: [PATCH 1/5] t0061: Fix incorrect indentation

[Frans Klaver]

On Wed, 25 Jan 2012 08:00:57 +0100, Junio C Hamano <gitster@pobox.com>  
wrote:

> "Frans Klaver" <fransklaver@gmail.com> writes:
>
>> On Tue, 24 Jan 2012 23:40:00 +0100, Jonathan Nieder
>> <jrnieder@gmail.com> wrote:
>>
>>> Frans Klaver wrote:
>>>
>>>> +++ b/t/t0061-run-command.sh
>>>> @@ -8,8 +8,8 @@ test_description='Test run command'
>>>> . ./test-lib.sh
>>>>
>>>> cat >hello-script <<-EOF
>>>> -	#!$SHELL_PATH
>>>> -	cat hello-script
>>>> +#!$SHELL_PATH
>>>> +cat hello-script
>>>>  EOF
>>>
>>> Looks like a no-op --- the script already started with #! and no
>>> leading tab for me.  Does it behave differently on your machine?
>>
>> Hurr? I'm fairly sure the script ended up being indented for me. I'll
>> recheck.
>
> It could be that your shell is broken and does not understand the
> distinction between the "<<-EOF" vs "<<EOF". What system are you on?

Gentoo Linux, using bash. Would be hard to believe bash on plain old linux  
mucks this up, wouldn't it?


> If
> the problem is real and widespread, we might want to mention it in  
> INSTALL
> or Makefile, just like we label Solaris /bin/sh as unusable and advise
> people to use /usr/xpg[46]/bin variant.

I'll check and notify.

Re: [PATCH 3/5] run-command: Elaborate execvp error checking

[Frans Klaver]

On Wed, 25 Jan 2012 00:22:39 +0100, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

> Klaver wrote:
>
>> The interpretation of errors from execvp was rather terse. For user
>> convenience communication of the nature of the error can be improved.
>
> Could you give an example?

The case that triggered me to work on this. I had an incorrect entry in my  
PATH and some aliasing tests failed. The generated command output was  
something like

fatal: script: Access Denied

while I was certain it didn't exist because I was expecting an alias to be  
executed. Also, access denied on a file that doesn't exist? That didn't  
make sense (it really doesn't if you don't know the execvp workings).  
Basically git takes no effort in actually pointing the user to the source  
of the problem. During the previous two versions I came to realize that  
this goes for all error codes execvp produces.


>
> [...]
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -2,6 +2,7 @@
>>  #include "run-command.h"
>>  #include "exec_cmd.h"
>>  #include "argv-array.h"
>> +#include "dir.h"
>>
>>  static inline void close_pair(int fd[2])
>>  {
>> @@ -134,6 +135,140 @@ static int wait_or_whine(pid_t pid, const char  
>> *argv0, int silent_exec_failure)
>>  	return code;
>>  }
>>
>> +#ifndef WIN32
>
> Not related to this patch, but I wonder if there should be a separate
> run-command-unix.c file so these ifdefs would no longer be necessary.

Might be useful indeed.


> What happens on Windows?

I haven't changed anything on the windows side, so that probably sticks to  
the old behavior.



>> +static void die_file_error(const char *file, int err)
>> +{
>> +	die("cannot exec '%s': %s", file, strerror(err));
>> +}
>
> I suspect it might be clearer to use die() inline in the two call
> sites so the reader does not have to figure out the calling
> convention.

Fair enough. I originally expected it to be used more than twice in the  
code.



>
>> +
>> +static char *get_interpreter(const char *first_line)
>> +{
>> +	struct strbuf sb = STRBUF_INIT;
>> +	size_t start = strspn(first_line + 2, " \t") + 2;
>> +	size_t end = strcspn(first_line + start, " \t\r\n") + start;
>> +
>> +	if (start >= end)
>> +		return NULL;
>> +
>> +	strbuf_add(&sb, first_line + start, end - start);
>> +	return strbuf_detach(&sb, NULL);
>> +}
>
> What does this function do?  What happens if first_line doesn't start
> with "#!"?  What should happen when there is a newline instead of a
> command name?  How about commands with quoting characters like " and
> backslash --- are the semantics portable in these cases?

This gets the interpreter from a #! line, assuming that it actually has a  
#!. You might call it naive, which would be fair enough. I didn't see much  
point in doing the same check twice, but inlining this code in its  
callsite felt messy.


> No need to use a strbuf here: xmemdupz would take care of the
> allocation and copy more simply.

Right.


>> +static void inspect_failure(const char *argv0, int silent_exec_failure)
>> +{
>> +	int err = errno;
>> +	struct strbuf sb = STRBUF_INIT;
>> +
>> +	/* errors not related to path */
>> +	if (errno == E2BIG || errno == ENOMEM)
>> +		die_file_error(argv0, err);
>> +
>> +	if (strchr(argv0, '/')) {
>> +		if (file_exists(argv0)) {
>> +			strbuf_add(&sb, argv0, strlen(argv0));
>> +			inspect_file(&sb, err, argv0);
>> +		}
>> +	} else {
>> +		char *path, *next;
>> +		path = getenv("PATH");
>
> I wonder if it's possible to rearrange this code to avoid deep
> nesting.  What does the function do, anyway?  (If the reader has to
> ask, it needs a comment or to be renamed.)
>
> I guess the idea is to diagnose after the fact why execvp failed.

Correct guess.

> Might be simplest like this:
>
> 	To diagnose execvp failure:
> 		if filename does not contain a '/':
> 			if we can't find it on the search path:
> 				That's the problem, dummy!
> 			replace filename with full path
> 		if file does not exist:
> 			just report strerror(errno)
> 		if not executable:
> 			...
> 		if interpreter does not exist:
> 			...
> 		if interpreter not executable:
> 			...
> 		otherwise, just report strerror(errno)
>
> with a separate function to find a command on the PATH, complaining
> when it encounters an unsearchable entry.

Well, this approach basically does the same thing, but this may have  
gotten a bit tunnel-visioned. I'll have a look at your approach and see  
what it gives us.

>
> Thanks for a fun read.

Thanks for an insightful review.

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Frans Klaver]

On Wed, 25 Jan 2012 00:24:21 +0100, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

> Frans Klaver wrote:
>
>> --- a/t/t0061-run-command.sh
>> +++ b/t/t0061-run-command.sh
>> @@ -76,12 +76,12 @@ test_expect_success POSIXPERM 'run_command reports  
>> EACCES, interpreter fails' '
>>  	grep "bad interpreter" err
>>  '
>>
>> -test_expect_failure POSIXPERM 'run_command reports ENOENT,  
>> interpreter' '
>> +test_expect_success POSIXPERM 'run_command reports ENOENT,  
>> interpreter' '
>>  	cat non-existing-interpreter >hello.sh &&
>>  	chmod +x hello.sh &&
>>  	test_must_fail test-run-command start-command-ENOENT ./hello.sh 2>err  
>> &&
>>
>> -	grep "error: cannot exec.*hello.sh" err &&
>> +	grep "fatal: cannot exec.*hello.sh" err &&
>
> Thanks.  I'd suggest using "test_expect_code" rather than the detailed
> wording of the message, since that is what scripts might want to rely
> on.

OK, makes sense.


> What happens on Windows?

I didn't plan anything to happen on windows. Doesn't POSIXPERM rule that  
OS out? I guess it could use similar code to this patch series to tackle  
all this.

Re: [PATCH 1/5] t0061: Fix incorrect indentation

[Frans Klaver]

On Wed, Jan 25, 2012 at 7:27 AM, Frans Klaver <fransklaver@gmail.com> wrote:

> Hurr? I'm fairly sure the script ended up being indented for me. I'll
> recheck.

Hm, that must have been my imagination back then. I'm sure I did this
for a reason. Ah well, we'll drop this patch. I'll put the tabs into
the other test scripts as well, because it does make them that much
more readable.

Thanks for catching.

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Johannes Sixt]

Am 25.01.2012 08:12, schrieb Frans Klaver:
> On Wed, 25 Jan 2012 00:24:21 +0100, Jonathan Nieder <jrnieder@gmail.com>

>> What happens on Windows?
> 
> I didn't plan anything to happen on windows. Doesn't POSIXPERM rule that
> OS out?

It does; these tests are skipped on Windows.

> I guess it could use similar code to this patch series to tackle
> all this.

No thanks. IMHO, this is already too much code for too little gain.

-- Hannes

Re: [PATCH 3/5] run-command: Elaborate execvp error checking

[Johannes Sixt]

Am 24.01.2012 23:32, schrieb Frans Klaver:
> +static void inspect_failure(const char *argv0, int silent_exec_failure)
> +{
> +	int err = errno;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	/* errors not related to path */
> +	if (errno == E2BIG || errno == ENOMEM)
> +		die_file_error(argv0, err);
> +
> +	if (strchr(argv0, '/')) {
> +		if (file_exists(argv0)) {
> +			strbuf_add(&sb, argv0, strlen(argv0));
> +			inspect_file(&sb, err, argv0);

Can we end up here if errno == ENOENT? If so, silent_exec_failure must
be checked. (inspect_file does not return.)

> +		}
> +	} else {
> +		char *path, *next;
> +		path = getenv("PATH");
> +		while (path) {
> +			next = strchrnul(path, ':');
> +			if (path < next)
> +				strbuf_add(&sb, path, next - path);
> +			else
> +				strbuf_addch(&sb, '.');
> +
> +			if (!*next)
> +				path = NULL;
> +			else
> +				path = next + 1;
> +
> +			if (!is_searchable(sb.buf)) {
> +				strbuf_release(&sb);
> +				continue;
> +			}
> +
> +			if (sb.len && sb.buf[sb.len - 1] != '/')
> +				strbuf_addch(&sb, '/');
> +			strbuf_addstr(&sb, argv0);
> +
> +			if (file_exists(sb.buf))
> +				inspect_file(&sb, err, argv0);
> +
> +			strbuf_release(&sb);
> +		}
> +	}
> +
> +	if (err == ENOENT) {
> +		if (!silent_exec_failure)
> +			error("cannot exec '%s': %s", argv0,
> +					strerror(ENOENT));
> +		exit(127);
> +	} else {
> +		die_file_error(argv0, err);
> +	}
> +}
> +#endif
> +
>  int start_command(struct child_process *cmd)
>  {
>  	int need_in, need_out, need_err;
> @@ -280,14 +415,7 @@ fail_pipe:
>  		} else {
>  			execvp(cmd->argv[0], (char *const*) cmd->argv);
>  		}
> -		if (errno == ENOENT) {
> -			if (!cmd->silent_exec_failure)
> -				error("cannot run %s: %s", cmd->argv[0],
> -					strerror(ENOENT));
> -			exit(127);
> -		} else {
> -			die_errno("cannot exec '%s'", cmd->argv[0]);
> -		}
> +		inspect_failure(cmd->argv[0], cmd->silent_exec_failure);

Isn't it important that this function calls exit(127) if we want
silent_exec_failure and errno == ENOENT? But I don't see that this
guaranteed by inspect_failure; see above.

>  	}
>  	if (cmd->pid < 0)
>  		error("cannot fork() for %s: %s", cmd->argv[0],

-- Hannes

Re: [PATCH 3/5] run-command: Elaborate execvp error checking

[Jonathan Nieder]

Frans Klaver wrote:
> Jonathan Nieder wrote:

>> Could you give an example?
>
> The case that triggered me to work on this. I had an incorrect entry
> in my PATH and some aliasing tests failed. The generated command
> output was something like
>
> fatal: script: Access Denied

Sorry for the lack of clarity.  I meant that a (precise) "before and
after" example could make the commit message a lot easier to
understand.

[...]
>> What happens on Windows?
>
> I haven't changed anything on the windows side, so that probably
> sticks to the old behavior.

This was mostly a comment on the change description --- unless I look
at the patch, if I try this out on Windows after reading the changelog
I would end up utterly confused.  For patch 5/5, it also brings up
worries about consistency --- if systems are going to be relying on a
missing #! interpreter being treated differently from a missing script
for the sake of silent_exec_failure, do the same considerations apply
on Windows, too?

Perhaps it's more along the lines of "this is not supposed to happen
in practice, and when it does, humans will find it easier to debug if
we error out hard instead of falling back to the 'if the command does
not exist' behavior (e.g., by trying an alias next)".  In other words,
maybe this is intended as an optional nicety rather than something
scripts would ever rely on.

Jonathan

Re: [PATCH 3/5] run-command: Elaborate execvp error checking

[Frans Klaver]

On Wed, 25 Jan 2012 20:22:22 +0100, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

> Frans Klaver wrote:
>> Jonathan Nieder wrote:
>
>>> Could you give an example?
>>
>> The case that triggered me to work on this. I had an incorrect entry
>> in my PATH and some aliasing tests failed. The generated command
>> output was something like
>>
>> fatal: script: Access Denied
>
> Sorry for the lack of clarity.  I meant that a (precise) "before and
> after" example could make the commit message a lot easier to
> understand.

Ah I see. I'll add something along those lines.


> [...]
>>> What happens on Windows?
>>
>> I haven't changed anything on the windows side, so that probably
>> sticks to the old behavior.
>
> This was mostly a comment on the change description --- unless I look
> at the patch, if I try this out on Windows after reading the changelog
> I would end up utterly confused.  For patch 5/5, it also brings up
> worries about consistency --- if systems are going to be relying on a
> missing #! interpreter being treated differently from a missing script
> for the sake of silent_exec_failure, do the same considerations apply
> on Windows, too?

I'm actually not sure if scripts would be relying on this. There is of  
course a good chance that people actually will rely on it, regardless of  
what we think. If there are consistency concerns on different platforms  
I'd probably have to work on that as well. Mentioning that windows isn't  
affected by these changes would be a start though.

> Perhaps it's more along the lines of "this is not supposed to happen
> in practice, and when it does, humans will find it easier to debug if
> we error out hard instead of falling back to the 'if the command does
> not exist' behavior (e.g., by trying an alias next)".  In other words,
> maybe this is intended as an optional nicety rather than something
> scripts would ever rely on.

Exactly. My concern was primarily the human interaction, so getting at  
least some pointer to the cause of the error. Would that be nice to have  
on windows as well? It probably would.

Re: [PATCH 3/5] run-command: Elaborate execvp error checking

[Frans Klaver]

On Wed, 25 Jan 2012 20:03:46 +0100, Johannes Sixt <j6t@kdbg.org> wrote:

> Am 24.01.2012 23:32, schrieb Frans Klaver:
>> +static void inspect_failure(const char *argv0, int silent_exec_failure)
>> +{
>> +	int err = errno;
>> +	struct strbuf sb = STRBUF_INIT;
>> +
>> +	/* errors not related to path */
>> +	if (errno == E2BIG || errno == ENOMEM)
>> +		die_file_error(argv0, err);
>> +
>> +	if (strchr(argv0, '/')) {
>> +		if (file_exists(argv0)) {
>> +			strbuf_add(&sb, argv0, strlen(argv0));
>> +			inspect_file(&sb, err, argv0);
>
> Can we end up here if errno == ENOENT? If so, silent_exec_failure must
> be checked. (inspect_file does not return.)

Hm, good catch. Yes, we can if the interpreter isn't found. I never  
intended to actually leave the "interpreter not found" ENOENT case in it's  
current shape, so it probably slipped through. Will fix inspect_failure  
here to guarantee silent_exec_failure is heeded. Patch 5/5 would probably  
remove it again.

Thanks.

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Frans Klaver]

On Wed, 25 Jan 2012 19:55:36 +0100, Johannes Sixt <j6t@kdbg.org> wrote:

>> I guess it could use similar code to this patch series to tackle
>> all this.
>
> No thanks. IMHO, this is already too much code for too little gain.

That's OK. If it's not only you who feels this way, I'd better spend my  
time on something that does add value.

Thanks for having a look in any case.

Frans

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> No thanks. IMHO, this is already too much code for too little gain.

Thanks for bringing a bit of sanity. You have already said it "In which
way is git different from other tools that execvp other programs?" earlier
(http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171848)

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Frans Klaver]

On Thu, Jan 26, 2012 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> No thanks. IMHO, this is already too much code for too little gain.
>
> Thanks for bringing a bit of sanity. You have already said it "In which
> way is git different from other tools that execvp other programs?" earlier
> (http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171848)

Well, one tool that it differs from is bash (although bash uses execve
directly I think). Personally I think this whole thing essentially a
lack of information from execv*. Also, I do agree that the code
required for this is quite more than I would have liked, but it will
reduce confusion when things go wrong. It's when things go wrong that
people get annoyed. Annoyed people look for greener grass. If that bit
of annoyance could be reduced, why not go the extra mile for that
little bit of gain?

Being as it is, I'll stop working on this. If this was pretty much
going to be /dev/null'ed from the beginning, I'd rather have heard it
after my first patches.

In any case, it has been an education so far. Thanks for that. And if
there's any issue you think I could start tackling, please don't
hesitate to cc me.

Cheers,
Frans

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Jonathan Nieder]

(+cc: Jeff because mentioning a pagination side-issue [*])
Frans Klaver wrote:

>                                            If this was pretty much
> going to be /dev/null'ed from the beginning, I'd rather have heard it
> after my first patches.

Almost always when a developer has an itch, it is _possible_ to
massage a patch that scratches it into something acceptable to others.
And whether it is worth the trouble in terms of time is something that
only that developer can decide.

So no, I would not say these patches were not doomed from the
beginning.  However, I certainly agree that in their current form they
are more complicated than the use case justifies.

There is a tension between requirements that leaves me oddly
uncomfortable with the series:

a. on one hand, it would be nice to preserve all the current features
   of execvp(), which makes the approach of only doing post-mortem
   analysis after a failed execvp appealing;

b. on the other hand, it would be nice [*] to avoid launching a pager
   only in order to call execvp for a command that does not exist when
   the fallback might be to an alias to a command that does not want a
   pager.  That would require figuring out in advance that execvp
   would fail with ENOENT and missing out on possible system extensions
   that allow execvp to run shell built-in commands not existing on
   the filesystem.

I want to like (b), but the downside seems unacceptable.  I honestly
don't know if something like (a) would be a good idea if well
executed, so I was happy to have the opportunity to try to help
massage these patches into a form that would make the answer more
obvious.

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Frans Klaver]

On Fri, Jan 27, 2012 at 9:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>                                            If this was pretty much
>> going to be /dev/null'ed from the beginning, I'd rather have heard it
>> after my first patches.
>
> Almost always when a developer has an itch, it is _possible_ to
> massage a patch that scratches it into something acceptable to others.
> And whether it is worth the trouble in terms of time is something that
> only that developer can decide.

Hannes' reaction and Junio's to his didn't give me the impression they
even saw a possibility.


> So no, I would not say these patches were not doomed from the
> beginning.  However, I certainly agree that in their current form they
> are more complicated than the use case justifies.

Good. That's something we can work on.


> There is a tension between requirements that leaves me oddly
> uncomfortable with the series:
>
> a. on one hand, it would be nice to preserve all the current features
>   of execvp(), which makes the approach of only doing post-mortem
>   analysis after a failed execvp appealing;
>
> b. on the other hand, it would be nice [*] to avoid launching a pager
>   only in order to call execvp for a command that does not exist when
>   the fallback might be to an alias to a command that does not want a
>   pager.  That would require figuring out in advance that execvp
>   would fail with ENOENT and missing out on possible system extensions
>   that allow execvp to run shell built-in commands not existing on
>   the filesystem.

Just for my understanding: before a command is executed, a pager
(less/more or so) is started? We want to avoid starting the pager if
we won't be able to execute the command?


> I want to like (b), but the downside seems unacceptable.

The downside being: having to figure out what execvp is going to do?
That would be tantamount to writing your own execvp.

> I honestly
> don't know if something like (a) would be a good idea if well
> executed, so I was happy to have the opportunity to try to help
> massage these patches into a form that would make the answer more
> obvious.

Given the above information, I'm happy to work on this to see if we
can mould it into something usable. Since the impact seems to go
beyond figuring out why execvp failed, I'm probably going to need some
help.

For now, I'll go through your suggestions and see what it produces.
We'll go from there.

Thanks for the heads-up.

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Jonathan Nieder]

Frans Klaver wrote:

> Just for my understanding: before a command is executed, a pager
> (less/more or so) is started? We want to avoid starting the pager if
> we won't be able to execute the command?

See [1] for an example of a recent patch touching the relevant
code path.

For example: if I run "git --paginate foo", foo is an alias for bar,
and the "[pager] bar" configuration is set to point to "otherpager",
then without this safety git launches the default pager in preparation
for running git-foo, receives ENOENT from execvp("git-foo"), and then
the pager has already been launched and it is too late to launch
otherpager instead.

> On Fri, Jan 27, 2012 at 9:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> I want to like (b), but the downside seems unacceptable.
>
> The downside being: having to figure out what execvp is going to do?
> That would be tantamount to writing your own execvp.

Exactly.

>> I honestly
>> don't know if something like (a) would be a good idea if well
>> executed, so I was happy to have the opportunity to try to help
>> massage these patches into a form that would make the answer more
>> obvious.
>
> Given the above information, I'm happy to work on this

I see.

Well, as I said, I don't know. :)  And I don't want to give false
hopes --- it's perfectly possible and not even unlikely that this is a
dead end and any patch in this direction will turn out not to be a
good idea and not applied.

That's part of why I was really grateful to Hannes for the reminder to
take a step back for a moment and consider whether it's worth it.
Maybe there's another way or a more targetted way to take care of the
motivational original confusing scenario that leads to execvp errors.
(By the way, can you remind me which one that was?)

Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/179635

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Frans Klaver]

On Fri, Jan 27, 2012 at 10:41 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Frans Klaver wrote:
>
>> Just for my understanding: before a command is executed, a pager
>> (less/more or so) is started? We want to avoid starting the pager if
>> we won't be able to execute the command?
>
> See [1] for an example of a recent patch touching the relevant
> code path.

I'll have a look at that.


> For example: if I run "git --paginate foo", foo is an alias for bar,
> and the "[pager] bar" configuration is set to point to "otherpager",
> then without this safety git launches the default pager in preparation
> for running git-foo, receives ENOENT from execvp("git-foo"), and then
> the pager has already been launched and it is too late to launch
> otherpager instead.

Something to be looked into then.


> Well, as I said, I don't know. :)  And I don't want to give false
> hopes --- it's perfectly possible and not even unlikely that this is a
> dead end and any patch in this direction will turn out not to be a
> good idea and not applied.

Hm don't worry about false hopes. I don't insist on having some of my
work actually in if there's no point in including it. Contributing to
the research is good enough for me if we can come to a conclusion that
we can present to people running into similar issues.


> That's part of why I was really grateful to Hannes for the reminder to
> take a step back for a moment and consider whether it's worth it.
> Maybe there's another way or a more targetted way to take care of the
> motivational original confusing scenario that leads to execvp errors.

I wonder.

> (By the way, can you remind me which one that was?)

I'll even summarize my thinking and motivation about this.

I was executing the test suite on my PC. Some test for aliases failed
-- git said EACCES, while for aliases one would expect ENOENT. For
users expecting an alias to be executed, "cannot execute git-frotz:
Access Denied" will be rather confusing. "Huh? Access denied? The file
doesn't even exist?!". It took me quite some debugging in git to track
this down to an inaccessible PATH entry. As someone who didn't know
anything of the git internal code it took quite a bit of learning as
well just to find out where we'd end up in the first place. It
bothered me (and still does) that execve uses EACCES for at least four
different errors:

...
    EACCES Search  permission is denied on a component of the path
prefix of filename or the
              name of a script interpreter.  (See also path_resolution(7).)
    EACCES The file or a script interpreter is not a regular file.
    EACCES Execute permission is denied for the file or a script or
ELF interpreter.
    EACCES The file system is mounted noexec.
...

Anyway, reading through that man page later on I found that a lot of
errors are only mentioned once, but do contain 'or' in the problem
description, like the first one of the EACCES items. ENOENT does that
as well:

    ENOENT The file filename or a script or ELF interpreter does  not
exist,  or  a  shared
              library needed for file or interpreter cannot be found.

I then additionally figured that always silently passing ENOENT is a
bad thing to do, because it simply can mean more than just "The file
you asked for cannot be found". It means "something required cannot be
found". My resulting view on this is basically that the execvp error
handling git currently has, is lacking a nuance that is necessary for
effective debugging. As I said earlier, it's when things go wrong
people get annoyed. Even more so if you don't provide them with
pointers to what might be wrong.

It also bothers me that to effectively debug program execution errors,
you have to know that git uses execvp, and you have to know how it
behaves. I would label that "implementation details" and as a user I
really don't want to be bothered by that, especially not as a new
user. For that reason I would have liked to end up with something like
bash does. I would rather see "Hey dummy, can't access /some/path" or
"Hey dummy, you ask for an interpreter that I have no acces to" than
"Well we got EACCES, so check the following things: Do we have search
permission on.... Is it a regular file? mounted noexec?..." or "We got
EACCES, check man execve(2) for possible reasons", although I'd agree
that even that would be slightly better than "We got EACCES".

So take of your git-guru hat and put on your new-git-user one and let
it simmer for a while.

Re: [PATCH 5/5] run-command: Error out if interpreter not found

[Frans Klaver]

On Fri, 27 Jan 2012 10:41:45 +0100, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

> Frans Klaver wrote:
>
>> Just for my understanding: before a command is executed, a pager
>> (less/more or so) is started? We want to avoid starting the pager if
>> we won't be able to execute the command?
>
> See [1] for an example of a recent patch touching the relevant
> code path.
>
> For example: if I run "git --paginate foo", foo is an alias for bar,
> and the "[pager] bar" configuration is set to point to "otherpager",
> then without this safety git launches the default pager in preparation
> for running git-foo, receives ENOENT from execvp("git-foo"), and then
> the pager has already been launched and it is too late to launch
> otherpager instead.

Took me a while to catch your drift, but if I understand correctly, you're  
thinking using some of the code to find out if starting the pager is a  
good idea or not. If I factor out the part that finds a command in PATH,  
there's the helper that with a fair amount of certainty, will predict  
whether 'git foo' will fail with ENOENT or not. It would fix a possible  
problem that is currently there. Obviously the only case we can catch, is  
the command not actually existing. Although it is just one of the cases  
ENOENT can be returned for, I think it is the only one git actually cares  
about when checking for it.


>> On Fri, Jan 27, 2012 at 9:48 AM, Jonathan Nieder <jrnieder@gmail.com>  
>> wrote:
>
>>> I want to like (b), but the downside seems unacceptable.
>>
>> The downside being: having to figure out what execvp is going to do?
>> That would be tantamount to writing your own execvp.
>
> Exactly.

So as it seems, there are a few cases where we can fairly reliably predict  
whether a command is or isn't going to be found. Unless I'm mistaken,  
dashed externals are never shell built-ins and so we don't have to be able  
to check for their existence. Then assuming that silent_exec_failure  
really only cares about commands actually not existing, we can be fairly  
naive about it. See if we can find it somewhere in PATH and if we can't  
bail out. If we can, start the pager and everything execvp then returns  
will be regarded a fatal error. In this case it would be a choice between  
spawning the wrong pager, or having a quick browse through the file system.


> That's part of why I was really grateful to Hannes for the reminder to
> take a step back for a moment and consider whether it's worth it.

It may be a sensible reminder. I didn't understand that comment as such.  
Maybe it's Hannes' style, I don't know.


> Maybe there's another way or a more targetted way to take care of the
> motivational original confusing scenario that leads to execvp errors.
> (By the way, can you remind me which one that was?)

Been thinking about it and I doubt it. To find out whether EACCES is  
returned due to a PATH issue, you have to go through all of those PATH  
entries. So while you're at it, there's a lot more you can check and most  
of those checks are fairly trivial to do.

I think I've worked through all your review comments. I'll address Hannes'  
comments, create an RFC series and see where we end up.

Junio, care to be CC'd in that?

Thanks,
Frans


> [1] http://thread.gmane.org/gmane.comp.version-control.git/179635