Re: [PATCH 3/3] pager: properly log pager exit code when signalled

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/run-command.c b/run-command.c
> index ea4d0fb4b15..10e1c96c2bd 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -551,8 +551,12 @@ 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 && WIFEXITED(status))
> +		return WEXITSTATUS(status);
> +	if (in_signal) {
> +		BUG("was not expecting waitpid() status %d", status);
> +		return -1;
> +	}

This starts reporting exit status of the pager back to
finish_command() and finish_command_in_signal().  But the code in
pager.c that call the finish_command*() ignore the returned value.

So, is the net result of these three patches just that the trace2
output gives the exit status of the pager, but "git" itself is not
affected otherwise (not a complaint; trying to understand the
intention) ?

> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index c60886f43e6..1424466caf5 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -656,31 +656,74 @@ test_expect_success TTY 'git tag with auto-columns ' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success TTY,!MINGW 'git returns SIGPIPE on early pager exit' '
> +test_expect_success TTY 'git returns SIGPIPE on early pager exit' '

I somehow find 2/3 of this change belongs to the previous step.
That is, shouldn't this new test added in the previous step without
the !MINGW prerequisite, but without the trace2 bits (i.e. the first
three added lines and the last "grep" of trace.normal), and the change
made in this step limited only to those trace2 bits?

>  	test_when_finished "rm pager-used" &&
>  	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" &&
>  
> -	OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
> -	test_match_signal 13 "$OUT" &&
> +	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.* code:0 " trace.normal &&
>  	test_path_is_file pager-used
>  '

The same comment applies to this new one added in this step.
Shouldn't the bulk of the test (i.e. under !MINGW we get killed with
signal 13) be introduced in the previous step?


> +test_expect_success TTY 'git logs nonexisting pager invocation' '
> +	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.* code:-1 " trace.normal
> +'
> +
>  test_done