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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Vincent Lefevre <vincent@vinc17.net>,
	Chris Torek <chris.torek@gmail.com>,
	Denton Liu <liu.denton@gmail.com>,
	"Jeff Hostetler" <jeffhost@microsoft.com>
Subject: Re: [PATCH 3/3] pager: properly log pager exit code when signalled
Date: Mon, 01 Feb 2021 20:23:14 +0100	[thread overview]
Message-ID: <87zh0negnx.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqbld3mz85.fsf@gitster.c.googlers.com>

On Mon, Feb 01 2021, Junio C Hamano wrote:

> Æ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) ?

Yes, it's just a bug fix for the trace2 output.

>> 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?

Sure, I can re-arrange it like that. I figured 1/3 shouldn't assume 3/3,
there wouldn't be a reason not to use !MINGW except because 3/3 is going
to remove it later, but I guess it makes the overall history easier to
read...

>>  	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?

*nod*

>> +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