pager: properly log pager exit code when signalled
When git invokes a pager that exits with non-zero the common case is that we'll already return the correct SIGPIPE failure from git itself, but the exit code logged in trace2 has always been incorrectly reported[1]. Fix that and log the correct exit code in the logs. Since this gives us something to test outside of our recently-added tests needing a !MINGW prerequisite, let's refactor the test to run on MINGW and actually check for SIGPIPE outside of MINGW. The wait_or_whine() is only called with a true "in_signal" from from finish_command_in_signal(), which in turn is only used in pager.c. The "in_signal && !WIFEXITED(status)" case is not covered by tests. Let's log the default -1 in that case for good measure. 1. The incorrect logging of the exit code in was seemingly copy/pasted into finish_command_in_signal() in ee4512ed481 (trace2: create new combined trace facility, 2019-02-22) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ævar Arnfjörð Bjarmason committed
Feb 2, 2021 at 03:00 UTC
be8fc53e364211856cca7affa4472855f96f8fa9
2 files changed
+57
-7
run-command.c
+3
-1
@@ -552,7 +552,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
552
while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
553
; /* nothing */
554
if (in_signal) {
555
- return 0;
555
+ if (WIFEXITED(status))
556
+ code = WEXITSTATUS(status);
557
+ return code;
558
}
559
560
if (waiting < 0) {
t/t7006-pager.sh
+54
-6
@@ -656,9 +656,17 @@ test_expect_success TTY 'git tag with auto-columns ' '
656
test_cmp expect actual
657
'
658
659
+test_expect_success 'setup trace2' '
660
+ GIT_TRACE2_BRIEF=1 &&
661
+ export GIT_TRACE2_BRIEF
662
+'
663
+
664
test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
660
- test_when_finished "rm pager-used" &&
665
+ test_when_finished "rm pager-used trace.normal" &&
666
test_config core.pager ">pager-used; head -n 1; exit 0" &&
667
+ GIT_TRACE2="$(pwd)/trace.normal" &&
668
+ export GIT_TRACE2 &&
669
+ test_when_finished "unset GIT_TRACE2" &&
670
671
if test_have_prereq !MINGW
672
then
@@ -667,12 +675,19 @@ test_expect_success TTY 'git returns SIGPIPE on early pager exit' '
675
else
676
test_terminal git log
677
fi &&
678
+
679
+ grep child_exit trace.normal >child-exits &&
680
+ test_line_count = 1 child-exits &&
681
+ grep " code:0 " child-exits &&
682
test_path_is_file pager-used
683
'
684
685
test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
674
- test_when_finished "rm pager-used" &&
686
+ test_when_finished "rm pager-used trace.normal" &&
687
test_config core.pager ">pager-used; head -n 1; exit 1" &&
688
+ GIT_TRACE2="$(pwd)/trace.normal" &&
689
+ export GIT_TRACE2 &&
690
+ test_when_finished "unset GIT_TRACE2" &&
691
692
if test_have_prereq !MINGW
693
then
@@ -681,12 +696,19 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
696
else
697
test_terminal git log
698
fi &&
699
+
700
+ grep child_exit trace.normal >child-exits &&
701
+ test_line_count = 1 child-exits &&
702
+ grep " code:1 " child-exits &&
703
test_path_is_file pager-used
704
'
705
706
test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
688
- test_when_finished "rm pager-used" &&
707
+ test_when_finished "rm pager-used trace.normal" &&
708
test_config core.pager "wc >pager-used; exit 1" &&
709
+ GIT_TRACE2="$(pwd)/trace.normal" &&
710
+ export GIT_TRACE2 &&
711
+ test_when_finished "unset GIT_TRACE2" &&
712
713
if test_have_prereq !MINGW
714
then
@@ -695,12 +717,19 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
717
else
718
test_terminal git log
719
fi &&
720
+
721
+ grep child_exit trace.normal >child-exits &&
722
+ test_line_count = 1 child-exits &&
723
+ grep " code:1 " child-exits &&
724
test_path_is_file pager-used
725
'
726
727
test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
702
- test_when_finished "rm pager-used" &&
728
+ test_when_finished "rm pager-used trace.normal" &&
729
test_config core.pager "wc >pager-used; does-not-exist" &&
730
+ GIT_TRACE2="$(pwd)/trace.normal" &&
731
+ export GIT_TRACE2 &&
732
+ test_when_finished "unset GIT_TRACE2" &&
733
734
if test_have_prereq !MINGW
735
then
@@ -709,11 +738,19 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
738
else
739
test_terminal git log
740
fi &&
741
+
742
+ grep child_exit trace.normal >child-exits &&
743
+ test_line_count = 1 child-exits &&
744
+ grep " code:127 " child-exits &&
745
test_path_is_file pager-used
746
'
747
748
test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' '
749
+ test_when_finished "rm trace.normal" &&
750
test_config core.pager "does-not-exist" &&
751
+ GIT_TRACE2="$(pwd)/trace.normal" &&
752
+ export GIT_TRACE2 &&
753
+ test_when_finished "unset GIT_TRACE2" &&
754
755
if test_have_prereq !MINGW
756
then
@@ -721,12 +758,19 @@ test_expect_success TTY 'git attempts to page to nonexisting pager command, gets
758
test_match_signal 13 "$OUT"
759
else
760
test_terminal git log
724
- fi
761
+ fi &&
762
+
763
+ grep child_exit trace.normal >child-exits &&
764
+ test_line_count = 1 child-exits &&
765
+ grep " code:-1 " child-exits
766
'
767
768
test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
728
- test_when_finished "rm pager-used" &&
769
+ test_when_finished "rm pager-used trace.normal" &&
770
test_config core.pager ">pager-used; test-tool sigchain" &&
771
+ GIT_TRACE2="$(pwd)/trace.normal" &&
772
+ export GIT_TRACE2 &&
773
+ test_when_finished "unset GIT_TRACE2" &&
774
775
if test_have_prereq !MINGW
776
then
@@ -735,6 +779,10 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
779
else
780
test_terminal git log
781
fi &&
782
+
783
+ grep child_exit trace.normal >child-exits &&
784
+ test_line_count = 1 child-exits &&
785
+ grep " code:143 " child-exits &&
786
test_path_is_file pager-used
787
'
788