Merge branch 'ab/pager-exit-log'

When a pager spawned by us exited, the trace log did not record its
exit status correctly, which has been corrected.

* ab/pager-exit-log:
  pager: properly log pager exit code when signalled
  run-command: add braces for "if" block in wait_or_whine()
  pager: test for exit code with and without SIGPIPE
  pager: refactor wait_for_pager() function

Author: gitster

pager.c

@@ -11,29 +11,25 @@
 static struct child_process pager_process = CHILD_PROCESS_INIT;
 static const char *pager_program;
 
-static void wait_for_pager(int in_signal)
+static void close_pager_fds(void)
 {
-	if (!in_signal) {
-		fflush(stdout);
-		fflush(stderr);
-	}
 	/* signal EOF to pager */
 	close(1);
 	close(2);
-	if (in_signal)
-		finish_command_in_signal(&pager_process);
-	else
-		finish_command(&pager_process);
 }
 
 static void wait_for_pager_atexit(void)
 {
-	wait_for_pager(0);
+	fflush(stdout);
+	fflush(stderr);
+	close_pager_fds();
+	finish_command(&pager_process);
 }
 
 static void wait_for_pager_signal(int signo)
 {
-	wait_for_pager(1);
+	close_pager_fds();
+	finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
 	raise(signo);
 }

run-command.c

@@ -551,8 +551,11 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
 
 	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
 		;	/* nothing */
-	if (in_signal)
-		return 0;
+	if (in_signal) {
+		if (WIFEXITED(status))
+			code = WEXITSTATUS(status);
+		return code;
+	}
 
 	if (waiting < 0) {
 		failed_errno = errno;

t/t7006-pager.sh

@@ -656,4 +656,134 @@ test_expect_success TTY 'git tag with auto-columns ' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup trace2' '
+	GIT_TRACE2_BRIEF=1 &&
+	export GIT_TRACE2_BRIEF
+'
+
+test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
+	test_when_finished "rm pager-used trace.normal" &&
+	test_config core.pager ">pager-used; head -n 1; exit 0" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:0 " child-exits &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
+	test_when_finished "rm pager-used trace.normal" &&
+	test_config core.pager ">pager-used; head -n 1; exit 1" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:1 " child-exits &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
+	test_when_finished "rm pager-used trace.normal" &&
+	test_config core.pager "wc >pager-used; exit 1" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test "$OUT" -eq 0
+	else
+		test_terminal git log
+	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:1 " child-exits &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
+	test_when_finished "rm pager-used trace.normal" &&
+	test_config core.pager "wc >pager-used; does-not-exist" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test "$OUT" -eq 0
+	else
+		test_terminal git log
+	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:127 " child-exits &&
+	test_path_is_file pager-used
+'
+
+test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '
+	test_when_finished "rm trace.normal" &&
+	test_config core.pager "does-not-exist" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:-1 " child-exits
+'
+
+test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
+	test_when_finished "rm pager-used trace.normal" &&
+	test_config core.pager ">pager-used; test-tool sigchain" &&
+	GIT_TRACE2="$(pwd)/trace.normal" &&
+	export GIT_TRACE2 &&
+	test_when_finished "unset GIT_TRACE2" &&
+
+	if test_have_prereq !MINGW
+	then
+		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
+		test_match_signal 13 "$OUT"
+	else
+		test_terminal git log
+	fi &&
+
+	grep child_exit trace.normal >child-exits &&
+	test_line_count = 1 child-exits &&
+	grep " code:143 " child-exits &&
+	test_path_is_file pager-used
+'
+
 test_done