From 5709e0363a891b72eb9e9756d7fb121d9bf6a7c7 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 4 Jul 2009 21:26:39 +0200 Subject: run_command: return exit code as positive value 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 Signed-off-by: Junio C Hamano --- git.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'git.c') diff --git a/git.c b/git.c index 65ed733fd..d223eab62 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 */ -- cgit v1.2.1 From 0ac77ec3150f43a5c2a6b1e47e9db5aafe53fb72 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 4 Jul 2009 21:26:40 +0200 Subject: run_command: report system call errors instead of returning error codes 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 Signed-off-by: Junio C Hamano --- git.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'git.c') diff --git a/git.c b/git.c index d223eab62..03726eee5 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]); + if (status >= 0 || errno != ENOENT) exit(status); - } - errno = ENOENT; /* as if we called execvp */ argv[0] = tmp; -- cgit v1.2.1 From c024beb56da679839d61f352d088b9a86823233a Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 4 Jul 2009 21:26:42 +0200 Subject: run_command: report failure to execute the program, but optionally don't 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 Signed-off-by: Junio C Hamano --- git.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'git.c') diff --git a/git.c b/git.c index 03726eee5..18240280e 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); -- cgit v1.2.1