Re: [PATCH 3/3] pager: properly log pager exit code when signalled
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 1, 2021, 19:23 UTC
- Message-ID
- <87zh0negnx.fsf@evledraar.gmail.com>
- In-Reply-To
- <xmqqbld3mz85.fsf@gitster.c.googlers.com>
On Mon, Feb 01 2021, Junio C Hamano wrote:
Show 27 quoted lines
> Æ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.
Show 16 quoted lines
>> 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...
Show 22 quoted lines
>> 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*
Show 17 quoted lines
>> +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