[PATCH 0/7] rework run_command error reporting

[PATCH 0/7] rework run_command error reporting

[Johannes Sixt]

I got tired of thinking about which return values of run_command functions
are exit codes and which are error codes of system call failures.

Furthermore, it is a pitty that almost no run_command callers correctly
report system call failures.

But on top of all I find it unacceptable that recent git-daemon logs
"unable to run 'git-pack-objects'" if a client terminates the connection
early.

This series addresses these issues, and also fixes an error in git-bisect
on Windows.

1/7 Truncate result of run_command that is used in exit() to lowest 8 bits

    This one fixes a breakage of git-bisect on Windows and should be
    applied at any rate.

2/7 MinGW: simplify waitpid() emulation macros
3/7 run_command: return exit code as positive value
4/7 run_command: report system call errors instead of returning error codes

    These address the mentioned issues; 4/7 is the important change.

5/7 run_command: encode deadly signal number in the return value
6/7 run_command: report failure to execute the program, but optionally don't

    These two are nice to have.

7/7 receive-pack: remove unnecessary run_status report

    This one is actually RFC because it removes a status report that users
    might be used to see, or even depend on.


 bisect.c                     |    4 +-
 builtin-add.c                |    2 +-
 builtin-merge.c              |    2 +-
 builtin-receive-pack.c       |   38 ++--------------
 compat/mingw.h               |    5 +-
 convert.c                    |    2 +-
 git.c                        |   10 +---
 ll-merge.c                   |    4 --
 run-command.c                |  105 +++++++++++++++++++++++------------------
 run-command.h                |   13 +----
 t/t5530-upload-pack-error.sh |    5 ++-
 transport.c                  |   16 +------
 12 files changed, 81 insertions(+), 125 deletions(-)

[PATCH 1/7] Truncate result of run_command that is used in exit() to lowest 8 bits

[Johannes Sixt]

On Windows, negative return values are not detected reliably by the parent
process as failure. This fixes it.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 bisect.c      |    4 ++--
 builtin-add.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/bisect.c b/bisect.c
index 52220df..b395c47 100644
--- a/bisect.c
+++ b/bisect.c
@@ -760,7 +760,7 @@ static int bisect_checkout(char *bisect_rev_hex)
 	argv_checkout[2] = bisect_rev_hex;
 	res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
 	if (res)
-		exit(res);
+		exit(res & 0xff);
 
 	argv_show_branch[1] = bisect_rev_hex;
 	return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
@@ -850,7 +850,7 @@ static void check_merge_bases(void)
 			handle_skipped_merge_base(mb);
 		} else {
 			printf("Bisecting: a merge base must be tested\n");
-			exit(bisect_checkout(sha1_to_hex(mb)));
+			exit(bisect_checkout(sha1_to_hex(mb)) & 0xff);
 		}
 	}
 
diff --git a/builtin-add.c b/builtin-add.c
index 4e44148..d83e67e 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -305,7 +305,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (patch_interactive)
 		add_interactive = 1;
 	if (add_interactive)
-		exit(interactive_add(argc - 1, argv + 1, prefix));
+		exit(interactive_add(argc - 1, argv + 1, prefix) & 0xff);
 
 	if (edit_interactive)
 		return(edit_patch(argc, argv, prefix));
-- 
1.6.3.17.g1665f

[Replacement PATCH 1/7] MinGW: truncate exit()'s argument to lowest 8 bits

[Johannes Sixt]

For some reason, MinGW's bash cannot reliably detect failure of the child
process if a negative value is passed to exit(). This fixes it by
truncating the exit code in all calls of exit().

This issue was worked around in run_builtin() of git.c (2488df84 builtin
run_command: do not exit with -1, 2007-11-15). This workaround is no longer
necessary and is reverted.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 compat/mingw.h |    2 ++
 git.c          |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 4f7ba4c..c1859c5 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -92,6 +92,8 @@ static inline int fcntl(int fd, int cmd, long arg)
 	errno = EINVAL;
 	return -1;
 }
+/* bash cannot reliably detect negative return codes as failure */
+#define exit(code) exit((code) & 0xff)
 
 /*
  * simple adaptors
diff --git a/git.c b/git.c
index f4d53f4..65ed733 100644
--- a/git.c
+++ b/git.c
@@ -245,7 +245,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 	status = p->fn(argc, argv, prefix);
 	if (status)
-		return status & 0xff;
+		return status;
 
 	/* Somebody closed stdout? */
 	if (fstat(fileno(stdout), &st))
-- 
1.6.3.3.393.g6b649

[PATCH 2/7] MinGW: simplify waitpid() emulation macros

[Johannes Sixt]

Windows does not have signals. At least they cannot be diagnosed by the
parent process; all that the parent process can observe is the exit code.

This also adds a dummy definition of WTERMSIG.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 The dummy definition WTERMSIG will be used in a subsequent patch.

 compat/mingw.h |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 4f7ba4c..21ab124 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -17,9 +17,10 @@ typedef int pid_t;
 #define S_IROTH 0
 #define S_IXOTH 0
 
-#define WIFEXITED(x) ((unsigned)(x) < 259)	/* STILL_ACTIVE */
+#define WIFEXITED(x) 1
+#define WIFSIGNALED(x) 0
 #define WEXITSTATUS(x) ((x) & 0xff)
-#define WIFSIGNALED(x) ((unsigned)(x) > 259)
+#define WTERMSIG(x) SIGTERM
 
 #define SIGHUP 1
 #define SIGQUIT 3
-- 
1.6.3.17.g1665f

[PATCH 3/7] run_command: return exit code as positive value

[Johannes Sixt]

As a general guideline, functions in git's code return zero to indicate
success and negative values to indicate failure. The run_command family of
functions followed this guideline. But there are actually two different
kinds of failure:

- failures of system calls;

- non-zero exit code of the program that was run.

Usually, a non-zero exit code of the program is a failure and means a
failure to the caller. Except that sometimes it does not. For example, the
exit code of merge programs (e.g. external merge drivers) conveys
information about how the merge failed, and not all exit calls are
actually failures.

Furthermore, the return value of run_command is sometimes used as exit
code by the caller.

This change arranges that the exit code of the program is returned as a
positive value, which can now be regarded as the "result" of the function.
System call failures continue to be reported as negative values.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-merge.c        |    2 +-
 builtin-receive-pack.c |    4 ++--
 convert.c              |    2 +-
 git.c                  |    4 ++--
 ll-merge.c             |    4 ----
 run-command.c          |    9 +--------
 run-command.h          |    1 -
 7 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index af9adab..96ecaf4 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -594,7 +594,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		discard_cache();
 		if (read_cache() < 0)
 			die("failed to read the cache");
-		return -ret;
+		return ret;
 	}
 }
 
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 6ec1d05..6235903 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -143,8 +143,8 @@ static int run_status(int code, const char *cmd_name)
 	case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
 		return error("%s died strangely", cmd_name);
 	default:
-		error("%s exited with error code %d", cmd_name, -code);
-		return -code;
+		error("%s exited with error code %d", cmd_name, code);
+		return code;
 	}
 }
 
diff --git a/convert.c b/convert.c
index 1816e97..491e714 100644
--- a/convert.c
+++ b/convert.c
@@ -267,7 +267,7 @@ static int filter_buffer(int fd, void *data)
 
 	status = finish_command(&child_process);
 	if (status)
-		error("external filter %s failed %d", params->cmd, -status);
+		error("external filter %s failed %d", params->cmd, status);
 	return (write_err || status);
 }
 
diff --git a/git.c b/git.c
index f4d53f4..662f21e 100644
--- a/git.c
+++ b/git.c
@@ -418,9 +418,9 @@ static void execv_dashed_external(const char **argv)
 	 */
 	status = run_command_v_opt(argv, 0);
 	if (status != -ERR_RUN_COMMAND_EXEC) {
-		if (IS_RUN_COMMAND_ERR(status))
+		if (status < 0)
 			die("unable to run '%s'", argv[0]);
-		exit(-status);
+		exit(status);
 	}
 	errno = ENOENT; /* as if we called execvp */
 
diff --git a/ll-merge.c b/ll-merge.c
index a2c13c4..31c7457 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -192,10 +192,6 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 
 	args[2] = cmd.buf;
 	status = run_command_v_opt(args, 0);
-	if (status < -ERR_RUN_COMMAND_FORK)
-		; /* failure in run-command */
-	else
-		status = -status;
 	fd = open(temp[1], O_RDONLY);
 	if (fd < 0)
 		goto bad;
diff --git a/run-command.c b/run-command.c
index eb2efc3..a4e309e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -241,14 +241,7 @@ static int wait_or_whine(pid_t pid)
 		if (!WIFEXITED(status))
 			return -ERR_RUN_COMMAND_WAITPID_NOEXIT;
 		code = WEXITSTATUS(status);
-		switch (code) {
-		case 127:
-			return -ERR_RUN_COMMAND_EXEC;
-		case 0:
-			return 0;
-		default:
-			return -code;
-		}
+		return code == 127 ? -ERR_RUN_COMMAND_EXEC : code;
 	}
 }
 
diff --git a/run-command.h b/run-command.h
index e345502..0211e1d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -10,7 +10,6 @@ enum {
 	ERR_RUN_COMMAND_WAITPID_SIGNAL,
 	ERR_RUN_COMMAND_WAITPID_NOEXIT,
 };
-#define IS_RUN_COMMAND_ERR(x) (-(x) >= ERR_RUN_COMMAND_FORK)
 
 struct child_process {
 	const char **argv;
-- 
1.6.3.17.g1665f

[PATCH 4/7] run_command: report system call errors instead of returning error codes

[Johannes Sixt]

The motivation for this change is that system call failures are serious
errors that should be reported to the user, but only few callers took the
burden to decode the error codes that the functions returned into error
messages.

If at all, then only an unspecific error message was given. A prominent
example is this:

   $ git upload-pack . | :
   fatal: unable to run 'git-upload-pack'

In this example, git-upload-pack, the external command invoked through the
git wrapper, dies due to SIGPIPE, but the git wrapper does not bother to
report the real cause. In fact, this very error message is copied to the
syslog if git-daemon's client aborts the connection early.

With this change, system call failures are reported immediately after the
failure and only a generic failure code is returned to the caller. In the
above example the error is now to the point:

   $ git upload-pack . | :
   error: git-upload-pack died of signal

Note that there is no error report if the invoked program terminated with
a non-zero exit code, because it is reasonable to expect that the invoked
program has already reported an error. (But many run_command call sites
nevertheless write a generic error message.)

There was one special return code that was used to identify the case where
run_command failed because the requested program could not be exec'd. This
special case is now treated like a system call failure with errno set to
ENOENT. No error is reported in this case, because the call site in git.c
expects this as a normal result. Therefore, the callers that carefully
decoded the return value still check for this condition.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-receive-pack.c       |   22 +---------
 git.c                        |    8 +---
 run-command.c                |   89 +++++++++++++++++++++++-------------------
 run-command.h                |   10 -----
 t/t5530-upload-pack-error.sh |    5 ++-
 transport.c                  |   12 +-----
 6 files changed, 59 insertions(+), 87 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 6235903..1dcdb1a 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -125,27 +125,11 @@ static const char post_receive_hook[] = "hooks/post-receive";
 
 static int run_status(int code, const char *cmd_name)
 {
-	switch (code) {
-	case 0:
-		return 0;
-	case -ERR_RUN_COMMAND_FORK:
-		return error("fork of %s failed", cmd_name);
-	case -ERR_RUN_COMMAND_EXEC:
+	if (code < 0 && errno == ENOENT)
 		return error("execute of %s failed", cmd_name);
-	case -ERR_RUN_COMMAND_PIPE:
-		return error("pipe failed");
-	case -ERR_RUN_COMMAND_WAITPID:
-		return error("waitpid failed");
-	case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
-		return error("waitpid is confused");
-	case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
-		return error("%s died of signal", cmd_name);
-	case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
-		return error("%s died strangely", cmd_name);
-	default:
+	else if (code > 0)
 		error("%s exited with error code %d", cmd_name, code);
-		return code;
-	}
+	return code;
 }
 
 static int run_receive_hook(const char *hook_name)
diff --git a/git.c b/git.c
index 662f21e..2fb7093 100644
--- a/git.c
+++ b/git.c
@@ -417,12 +417,8 @@ static void execv_dashed_external(const char **argv)
 	 * OK to return. Otherwise, we just pass along the status code.
 	 */
 	status = run_command_v_opt(argv, 0);
-	if (status != -ERR_RUN_COMMAND_EXEC) {
-		if (status < 0)
-			die("unable to run '%s'", argv[0]);
-		exit(status);
-	}
-	errno = ENOENT; /* as if we called execvp */
+	if (status >= 0 || errno != ENOENT)
+		exit(status & 0xff);
 
 	argv[0] = tmp;
 
diff --git a/run-command.c b/run-command.c
index a4e309e..4b3fda0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -19,6 +19,7 @@ int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
 	int fdin[2], fdout[2], fderr[2];
+	int failed_errno;
 
 	/*
 	 * In case of errors we must keep the promise to close FDs
@@ -28,9 +29,10 @@ int start_command(struct child_process *cmd)
 	need_in = !cmd->no_stdin && cmd->in < 0;
 	if (need_in) {
 		if (pipe(fdin) < 0) {
+			failed_errno = errno;
 			if (cmd->out > 0)
 				close(cmd->out);
-			return -ERR_RUN_COMMAND_PIPE;
+			goto fail_pipe;
 		}
 		cmd->in = fdin[1];
 	}
@@ -40,11 +42,12 @@ int start_command(struct child_process *cmd)
 		&& cmd->out < 0;
 	if (need_out) {
 		if (pipe(fdout) < 0) {
+			failed_errno = errno;
 			if (need_in)
 				close_pair(fdin);
 			else if (cmd->in)
 				close(cmd->in);
-			return -ERR_RUN_COMMAND_PIPE;
+			goto fail_pipe;
 		}
 		cmd->out = fdout[0];
 	}
@@ -52,6 +55,7 @@ int start_command(struct child_process *cmd)
 	need_err = !cmd->no_stderr && cmd->err < 0;
 	if (need_err) {
 		if (pipe(fderr) < 0) {
+			failed_errno = errno;
 			if (need_in)
 				close_pair(fdin);
 			else if (cmd->in)
@@ -60,7 +64,11 @@ int start_command(struct child_process *cmd)
 				close_pair(fdout);
 			else if (cmd->out)
 				close(cmd->out);
-			return -ERR_RUN_COMMAND_PIPE;
+fail_pipe:
+			error("cannot create pipe for %s: %s",
+				cmd->argv[0], strerror(failed_errno));
+			errno = failed_errno;
+			return -1;
 		}
 		cmd->err = fderr[0];
 	}
@@ -122,6 +130,9 @@ int start_command(struct child_process *cmd)
 				strerror(errno));
 		exit(127);
 	}
+	if (cmd->pid < 0)
+		error("cannot fork() for %s: %s", cmd->argv[0],
+			strerror(failed_errno = errno));
 #else
 	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
 	const char **sargv = cmd->argv;
@@ -173,6 +184,9 @@ int start_command(struct child_process *cmd)
 	}
 
 	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
+	if (cmd->pid < 0 && errno != ENOENT)
+		error("cannot spawn %s: %s", cmd->argv[0],
+			strerror(failed_errno = errno));
 
 	if (cmd->env)
 		free_environ(env);
@@ -189,7 +203,6 @@ int start_command(struct child_process *cmd)
 #endif
 
 	if (cmd->pid < 0) {
-		int err = errno;
 		if (need_in)
 			close_pair(fdin);
 		else if (cmd->in)
@@ -200,9 +213,8 @@ int start_command(struct child_process *cmd)
 			close(cmd->out);
 		if (need_err)
 			close_pair(fderr);
-		return err == ENOENT ?
-			-ERR_RUN_COMMAND_EXEC :
-			-ERR_RUN_COMMAND_FORK;
+		errno = failed_errno;
+		return -1;
 	}
 
 	if (need_in)
@@ -221,33 +233,41 @@ int start_command(struct child_process *cmd)
 	return 0;
 }
 
-static int wait_or_whine(pid_t pid)
+static int wait_or_whine(pid_t pid, const char *argv0)
 {
-	for (;;) {
-		int status, code;
-		pid_t waiting = waitpid(pid, &status, 0);
-
-		if (waiting < 0) {
-			if (errno == EINTR)
-				continue;
-			error("waitpid failed (%s)", strerror(errno));
-			return -ERR_RUN_COMMAND_WAITPID;
-		}
-		if (waiting != pid)
-			return -ERR_RUN_COMMAND_WAITPID_WRONG_PID;
-		if (WIFSIGNALED(status))
-			return -ERR_RUN_COMMAND_WAITPID_SIGNAL;
-
-		if (!WIFEXITED(status))
-			return -ERR_RUN_COMMAND_WAITPID_NOEXIT;
+	int status, code = -1;
+	pid_t waiting;
+	int failed_errno = 0;
+
+	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
+		;	/* nothing */
+
+	if (waiting < 0) {
+		failed_errno = errno;
+		error("waitpid for %s failed: %s", argv0, strerror(errno));
+	} else if (waiting != pid) {
+		error("waitpid is confused (%s)", argv0);
+	} else if (WIFSIGNALED(status)) {
+		error("%s died of signal", argv0);
+	} else if (WIFEXITED(status)) {
 		code = WEXITSTATUS(status);
-		return code == 127 ? -ERR_RUN_COMMAND_EXEC : code;
+		/*
+		 * Convert special exit code when execvp failed.
+		 */
+		if (code == 127) {
+			code = -1;
+			failed_errno = ENOENT;
+		}
+	} else {
+		error("waitpid is confused (%s)", argv0);
 	}
+	errno = failed_errno;
+	return code;
 }
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid);
+	return wait_or_whine(cmd->pid, cmd->argv[0]);
 }
 
 int run_command(struct child_process *cmd)
@@ -331,10 +351,7 @@ int start_async(struct async *async)
 int finish_async(struct async *async)
 {
 #ifndef __MINGW32__
-	int ret = 0;
-
-	if (wait_or_whine(async->pid))
-		ret = error("waitpid (async) failed");
+	int ret = wait_or_whine(async->pid, "child process");
 #else
 	DWORD ret = 0;
 	if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0)
@@ -378,15 +395,7 @@ int run_hook(const char *index_file, const char *name, ...)
 		hook.env = env;
 	}
 
-	ret = start_command(&hook);
+	ret = run_command(&hook);
 	free(argv);
-	if (ret) {
-		warning("Could not spawn %s", argv[0]);
-		return ret;
-	}
-	ret = finish_command(&hook);
-	if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL)
-		warning("%s exited due to uncaught signal", argv[0]);
-
 	return ret;
 }
diff --git a/run-command.h b/run-command.h
index 0211e1d..ac6c09c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -1,16 +1,6 @@
 #ifndef RUN_COMMAND_H
 #define RUN_COMMAND_H
 
-enum {
-	ERR_RUN_COMMAND_FORK = 10000,
-	ERR_RUN_COMMAND_EXEC,
-	ERR_RUN_COMMAND_PIPE,
-	ERR_RUN_COMMAND_WAITPID,
-	ERR_RUN_COMMAND_WAITPID_WRONG_PID,
-	ERR_RUN_COMMAND_WAITPID_SIGNAL,
-	ERR_RUN_COMMAND_WAITPID_NOEXIT,
-};
-
 struct child_process {
 	const char **argv;
 	pid_t pid;
diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
index f5102b9..82ca300 100755
--- a/t/t5530-upload-pack-error.sh
+++ b/t/t5530-upload-pack-error.sh
@@ -53,7 +53,10 @@ test_expect_success 'upload-pack fails due to error in rev-list' '
 	! echo "0032want $(git rev-parse HEAD)
 00000009done
 0000" | git upload-pack . > /dev/null 2> output.err &&
-	grep "waitpid (async) failed" output.err
+	# pack-objects survived
+	grep "Total.*, reused" output.err &&
+	# but there was an error, which must have been in rev-list
+	grep "bad tree object" output.err
 '
 
 test_expect_success 'create empty repository' '
diff --git a/transport.c b/transport.c
index 501a77b..0885801 100644
--- a/transport.c
+++ b/transport.c
@@ -417,18 +417,8 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
 		argv[argc++] = *refspec++;
 	argv[argc] = NULL;
 	err = run_command_v_opt(argv, RUN_GIT_CMD);
-	switch (err) {
-	case -ERR_RUN_COMMAND_FORK:
-		error("unable to fork for %s", argv[0]);
-	case -ERR_RUN_COMMAND_EXEC:
+	if (err < 0 && errno == ENOENT)
 		error("unable to exec %s", argv[0]);
-		break;
-	case -ERR_RUN_COMMAND_WAITPID:
-	case -ERR_RUN_COMMAND_WAITPID_WRONG_PID:
-	case -ERR_RUN_COMMAND_WAITPID_SIGNAL:
-	case -ERR_RUN_COMMAND_WAITPID_NOEXIT:
-		error("%s died with strange error", argv[0]);
-	}
 	return !!err;
 }
 
-- 
1.6.3.17.g1665f

Re: [PATCH 4/7] run_command: report system call errors instead of returning error codes

[Johannes Sixt]

On Samstag, 4. Juli 2009, Johannes Sixt wrote:
> +	if (status >= 0 || errno != ENOENT)
> +		exit(status & 0xff);

With my replacement patch 1/7 this '& 0xff' is no longer necessary. Would you
kindly squash this in?

-- Hannes

diff --git a/git.c b/git.c
--- a/git.c
+++ b/git.c
@@ -418,7 +418,7 @@ static void execv_dashed_external(const char **argv)
 	 */
 	status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
 	if (status >= 0 || errno != ENOENT)
-		exit(status & 0xff);
+		exit(status);
 
 	argv[0] = tmp;
 

Re: [PATCH 4/7] run_command: report system call errors instead of returning error codes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Samstag, 4. Juli 2009, Johannes Sixt wrote:
>> +	if (status >= 0 || errno != ENOENT)
>> +		exit(status & 0xff);
>
> With my replacement patch 1/7 this '& 0xff' is no longer necessary. Would you
> kindly squash this in?

Sounds sensible.  Thanks.

Re: [PATCH 4/7] run_command: report system call errors instead of returning error codes

[Johannes Sixt]

Johannes Sixt schrieb:
>  	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
> +	if (cmd->pid < 0 && errno != ENOENT)
> +		error("cannot spawn %s: %s", cmd->argv[0],
> +			strerror(failed_errno = errno));
>  
>  	if (cmd->env)
>  		free_environ(env);
> @@ -189,7 +203,6 @@ int start_command(struct child_process *cmd)
>  #endif
>  
>  	if (cmd->pid < 0) {
> -		int err = errno;
>  		if (need_in)
>  			close_pair(fdin);
>  		else if (cmd->in)
> @@ -200,9 +213,8 @@ int start_command(struct child_process *cmd)
>  			close(cmd->out);
>  		if (need_err)
>  			close_pair(fderr);
> -		return err == ENOENT ?
> -			-ERR_RUN_COMMAND_EXEC :
> -			-ERR_RUN_COMMAND_FORK;
> +		errno = failed_errno;
> +		return -1;
>  	}

Sorry to bother you once again. 'gcc -O2' correctly points out that
failed_errno could be used uninitialized (MinGW code path only).
Would you please squash this in?

-- Hannes

diff --git a/run-command.c b/run-command.c
--- a/run-command.c
+++ b/run-command.c
@@ -184,9 +184,9 @@ fail_pipe:
 	}

 	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
+	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
-		error("cannot spawn %s: %s", cmd->argv[0],
-			strerror(failed_errno = errno));
+		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));

 	if (cmd->env)
 		free_environ(env);

[PATCH 5/7] run_command: encode deadly signal number in the return value

[Johannes Sixt]

We now write the signal number in the error message if the program
terminated by a signal. The negative return value is constructed such that
after truncation to 8 bits it looks like a POSIX shell's $?:

   $ echo 0000 | { git upload-pack .; echo $? >&2; } | :
   error: git-upload-pack died of signal 13
   141

Previously, the exit code was 255 instead of 141.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 run-command.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 4b3fda0..146a8ea 100644
--- a/run-command.c
+++ b/run-command.c
@@ -248,7 +248,14 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 	} else if (waiting != pid) {
 		error("waitpid is confused (%s)", argv0);
 	} else if (WIFSIGNALED(status)) {
-		error("%s died of signal", argv0);
+		code = WTERMSIG(status);
+		error("%s died of signal %d", argv0, code);
+		/*
+		 * This return value is chosen so that code & 0xff
+		 * mimics the exit code that a POSIX shell would report for
+		 * a program that died from this signal.
+		 */
+		code -= 128;
 	} else if (WIFEXITED(status)) {
 		code = WEXITSTATUS(status);
 		/*
-- 
1.6.3.17.g1665f

[PATCH 6/7] run_command: report failure to execute the program, but optionally don't

[Johannes Sixt]

In the case where a program was not found, it was still the task of the
caller to report an error to the user. Usually, this is an interesting case
but only few callers actually reported a specific error (though many call
sites report a generic error message regardless of the cause).

With this change the error is reported by run_command, but since there is
one call site in git.c that does not want that, an option is added to
struct child_process, which is used to turn the error off.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-receive-pack.c |    4 +---
 git.c                  |    2 +-
 run-command.c          |   12 ++++++++----
 run-command.h          |    2 ++
 transport.c            |    6 +-----
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 1dcdb1a..c85507b 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -125,9 +125,7 @@ static const char post_receive_hook[] = "hooks/post-receive";
 
 static int run_status(int code, const char *cmd_name)
 {
-	if (code < 0 && errno == ENOENT)
-		return error("execute of %s failed", cmd_name);
-	else if (code > 0)
+	if (code > 0)
 		error("%s exited with error code %d", cmd_name, code);
 	return code;
 }
diff --git a/git.c b/git.c
index 2fb7093..4a977f5 100644
--- a/git.c
+++ b/git.c
@@ -416,7 +416,7 @@ static void execv_dashed_external(const char **argv)
 	 * if we fail because the command is not found, it is
 	 * OK to return. Otherwise, we just pass along the status code.
 	 */
-	status = run_command_v_opt(argv, 0);
+	status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
 	if (status >= 0 || errno != ENOENT)
 		exit(status & 0xff);
 
diff --git a/run-command.c b/run-command.c
index 146a8ea..ddf3dd3 100644
--- a/run-command.c
+++ b/run-command.c
@@ -184,7 +184,7 @@ fail_pipe:
 	}
 
 	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env);
-	if (cmd->pid < 0 && errno != ENOENT)
+	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
 
@@ -233,7 +233,7 @@ fail_pipe:
 	return 0;
 }
 
-static int wait_or_whine(pid_t pid, const char *argv0)
+static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 {
 	int status, code = -1;
 	pid_t waiting;
@@ -264,6 +264,9 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 		if (code == 127) {
 			code = -1;
 			failed_errno = ENOENT;
+			if (!silent_exec_failure)
+				error("cannot run %s: %s", argv0,
+					strerror(ENOENT));
 		}
 	} else {
 		error("waitpid is confused (%s)", argv0);
@@ -274,7 +277,7 @@ static int wait_or_whine(pid_t pid, const char *argv0)
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid, cmd->argv[0]);
+	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);
 }
 
 int run_command(struct child_process *cmd)
@@ -294,6 +297,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,
 	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
 	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
 	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 }
 
 int run_command_v_opt(const char **argv, int opt)
@@ -358,7 +362,7 @@ int start_async(struct async *async)
 int finish_async(struct async *async)
 {
 #ifndef __MINGW32__
-	int ret = wait_or_whine(async->pid, "child process");
+	int ret = wait_or_whine(async->pid, "child process", 0);
 #else
 	DWORD ret = 0;
 	if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0)
diff --git a/run-command.h b/run-command.h
index ac6c09c..0c00b25 100644
--- a/run-command.h
+++ b/run-command.h
@@ -31,6 +31,7 @@ struct child_process {
 	unsigned no_stdout:1;
 	unsigned no_stderr:1;
 	unsigned git_cmd:1; /* if this is to be git sub-command */
+	unsigned silent_exec_failure:1;
 	unsigned stdout_to_stderr:1;
 	void (*preexec_cb)(void);
 };
@@ -44,6 +45,7 @@ extern int run_hook(const char *index_file, const char *name, ...);
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
 #define RUN_COMMAND_STDOUT_TO_STDERR 4
+#define RUN_SILENT_EXEC_FAILURE 8
 int run_command_v_opt(const char **argv, int opt);
 
 /*
diff --git a/transport.c b/transport.c
index 0885801..802ce7f 100644
--- a/transport.c
+++ b/transport.c
@@ -396,7 +396,6 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
 {
 	const char **argv;
 	int argc;
-	int err;
 
 	if (flags & TRANSPORT_PUSH_MIRROR)
 		return error("http transport does not support mirror mode");
@@ -416,10 +415,7 @@ static int curl_transport_push(struct transport *transport, int refspec_nr, cons
 	while (refspec_nr--)
 		argv[argc++] = *refspec++;
 	argv[argc] = NULL;
-	err = run_command_v_opt(argv, RUN_GIT_CMD);
-	if (err < 0 && errno == ENOENT)
-		error("unable to exec %s", argv[0]);
-	return !!err;
+	return !!run_command_v_opt(argv, RUN_GIT_CMD);
 }
 
 static struct ref *get_refs_via_curl(struct transport *transport, int for_push)
-- 
1.6.3.17.g1665f

[PATCH/RFC 7/7] receive-pack: remove unnecessary run_status report

[Johannes Sixt]

The function run_status was used to report failures after a hook was run.
By now, the only thing that the function itself reported was the exit code
of the hook (if it was non-zero). But this is redundant because it can be
expected that the hook itself will have reported a suitable error.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-receive-pack.c |   20 ++++----------------
 1 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index c85507b..b771fe9 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -123,13 +123,6 @@ static struct command *commands;
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
-static int run_status(int code, const char *cmd_name)
-{
-	if (code > 0)
-		error("%s exited with error code %d", cmd_name, code);
-	return code;
-}
-
 static int run_receive_hook(const char *hook_name)
 {
 	static char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];
@@ -156,7 +149,7 @@ static int run_receive_hook(const char *hook_name)
 
 	code = start_command(&proc);
 	if (code)
-		return run_status(code, hook_name);
+		return code;
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string) {
 			size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n",
@@ -168,7 +161,7 @@ static int run_receive_hook(const char *hook_name)
 		}
 	}
 	close(proc.in);
-	return run_status(finish_command(&proc), hook_name);
+	return finish_command(&proc);
 }
 
 static int run_update_hook(struct command *cmd)
@@ -185,9 +178,8 @@ static int run_update_hook(struct command *cmd)
 	argv[3] = sha1_to_hex(cmd->new_sha1);
 	argv[4] = NULL;
 
-	return run_status(run_command_v_opt(argv, RUN_COMMAND_NO_STDIN |
-					RUN_COMMAND_STDOUT_TO_STDERR),
-			update_hook);
+	return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN |
+					RUN_COMMAND_STDOUT_TO_STDERR);
 }
 
 static int is_ref_checked_out(const char *ref)
@@ -401,7 +393,6 @@ static void run_update_post_hook(struct command *cmd)
 	argv[argc] = NULL;
 	status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
 			| RUN_COMMAND_STDOUT_TO_STDERR);
-	run_status(status, update_post_hook);
 }
 
 static void execute_commands(const char *unpacker_error)
@@ -519,7 +510,6 @@ static const char *unpack(void)
 		code = run_command_v_opt(unpacker, RUN_GIT_CMD);
 		if (!code)
 			return NULL;
-		run_status(code, unpacker[0]);
 		return "unpack-objects abnormal exit";
 	} else {
 		const char *keeper[7];
@@ -545,7 +535,6 @@ static const char *unpack(void)
 		ip.git_cmd = 1;
 		status = start_command(&ip);
 		if (status) {
-			run_status(status, keeper[0]);
 			return "index-pack fork failed";
 		}
 		pack_lockfile = index_pack_lockfile(ip.out);
@@ -555,7 +544,6 @@ static const char *unpack(void)
 			reprepare_packed_git();
 			return NULL;
 		}
-		run_status(status, keeper[0]);
 		return "index-pack abnormal exit";
 	}
 }
-- 
1.6.3.17.g1665f