[PATCH] run-command: write full error message in die_child

[Jonathan Nieder <jrnieder@gmail.com>]

The run_command facility writes a truncated error message when the
command is present but cannot be executed for some other reason.  For
example, if I add a 'hello' command to git:

	$ echo 'echo hello' >git-hello
	$ chmod +x git-hello
	$ PATH=.:$PATH git hello
	hello

and then make it non-executable, this is what I get from 'maint':

	$ chmod a-x git-hello
	$ git hello
	fatal: cannot exec 'git-hello': Permission denied

But with 'master', we get disturbing output:

	$ PATH=.:$PATH git hello
	fatal: $

That is a regression introduced by v1.7.5-rc0~29^2 (run-command:
prettify -D_FORTIFY_SOURCE workaround, 2011-03-16), which uses the
construct "if (write(...) || write(...) || write(...))" to perform
some writes in sequence, with the "if" body acknowledging errors from
them once.  write does not return 0 on success, so only the first
write succeeds.  Oops.

While fixing the above, let's actually pay attention to the return
value and handle partial writes.  write_in_full has the desired
semantics --- it loops until the desired number of bytes have been
written and on error it returns -1 to let us handle the error.

The "if" to appease warn_unused_result is no longer necessary after
this patch since xwrite and write_in_full check the return value from
write(2), but we leave it in for clarity and for robustness against
future static analyzers.

Reported-by: Junio C Hamano <gitster@pobox.com>
Analysis-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

> I'm reverting it for now, but am open to a submission of a proper fix
> after 1.7.5.

Knowing myself, I'm likely to forget to submit a fix later.  So here's
a patch to consider applying after 1.7.5.

Based directly against ebec84277 (run-command: prettify
-D_FORTIFY_SOURCE workaround, 2011-03-16).  The "grep" in the test
case should be test_i18ngrep if applying to a gettextized git.

Sorry for the breakage.

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

diff --git a/run-command.c b/run-command.c
index 8619c76..3e2ce2a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -72,7 +72,7 @@ static void notify_parent(void)
 	 * know, so failures like ENOENT can be handled right away; but
 	 * otherwise, finish_command will still report the error.
 	 */
-	if (write(child_notifier, "", 1))
+	if (xwrite(child_notifier, "", 1) < 0)
 		; /* yes, dear gcc -D_FORTIFY_SOURCE, there was an error. */
 }
 
@@ -83,9 +83,9 @@ static NORETURN void die_child(const char *err, va_list params)
 	if (len > sizeof(msg))
 		len = sizeof(msg);
 
-	if (write(child_err, "fatal: ", 7) ||
-	    write(child_err, msg, len) ||
-	    write(child_err, "\n", 1))
+	if (write_in_full(child_err, "fatal: ", 7) < 0 ||
+	    write_in_full(child_err, msg, len) < 0 ||
+	    write_in_full(child_err, "\n", 1) < 0)
 		; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */
 	exit(128);
 }
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 10b26e4..be602fd 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -7,8 +7,32 @@ test_description='Test run command'
 
 . ./test-lib.sh
 
+cat >hello-script <<-EOF
+	#!$SHELL_PATH
+	echo hello
+EOF
+>empty
+
 test_expect_success 'start_command reports ENOENT' '
 	test-run-command start-command-ENOENT ./does-not-exist
 '
 
+test_expect_success 'run_command can run a command' '
+	echo hello >expect &&
+	cat hello-script >hello.sh &&
+	chmod +x hello.sh &&
+	test-run-command run-command ./hello.sh >actual 2>err &&
+
+	test_cmp expect actual &&
+	test_cmp empty err
+'
+
+test_expect_success POSIXPERM,SANITY '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 &&
+
+	grep "fatal: cannot exec.*hello.sh" err
+'
+
 test_done
diff --git a/test-run-command.c b/test-run-command.c
index 0612bfa..37918e1 100644
--- a/test-run-command.c
+++ b/test-run-command.c
@@ -29,6 +29,8 @@ int main(int argc, char **argv)
 		fprintf(stderr, "FAIL %s\n", argv[1]);
 		return 1;
 	}
+	if (!strcmp(argv[1], "run-command"))
+		exit(run_command(&proc));
 
 	fprintf(stderr, "check usage\n");
 	return 1;
-- 
1.7.5.rc2