On Mon, Feb 01 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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
