Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Vincent Lefevre" <vincent@vinc17.net>,
	"Chris Torek" <chris.torek@gmail.com>,
	"Denton Liu" <liu.denton@gmail.com>,
	"Jeff Hostetler" <jeffhost@microsoft.com>,
	"Johannes Sixt" <j6t@kdbg.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [WIP/PATCH v2 5/5] WIP pager: respect exit code of pager over SIGPIPE
Date: Tue,  2 Feb 2021 03:00:01 +0100
Message-ID: <20210202020001.31601-6-avarab@gmail.com> (raw)
In-Reply-To: <20210201144921.8664-1-avarab@gmail.com>

As discussed on-list starting with [1] I don't think this patch makes
sense, but this "passes tests", at least on Debian with glibc, and is
food for thought for those who like the approach of git not
propagating the pager-induced SIGPIPE in git's own exit code.

The exit() here in wait_for_pager_atexit() isn't portable though[2],
we could probably use _exit(1) instead, but then we're going to
abruptly put a stop to further atexit handler processing. We're far
from the only one, tempfile.c, run-command.c, gc.c etc. all rely on
it, and that's just the git.git code.

If we drop the "if (code)" condition we can see that our pager exit
code will override the exit code of other commands in t7006-pager.sh,
causing numerous tests to fail. Of course if we don't do that all
tests pass.

But that experiment suggests regressions introduced here that we just
don't have good test coverage for. I.e. we're running code before the
atexit() here which expects to exit() with a given status code, and
we're clobbering it with ours because the pager also happened to fail
as we were exiting.

So a real implementation of this would, I think, have to at least:

 A. Refactor all use of atexit() to use some git-specific registry,
    hard assert somehow that we're never going to have atexit() by
    anything else (a library we use might call it).

 B. Because we used some atexit() wrapper API we'd know if we were in
    the last atexit() handler, which would need to re-evaluate the
    decision about the "real" exit code.

 C. We could not call exit() anywhere, but would have to make a
    git_exit() wrapper. We'd then assign the desired exit code to a
    global variable, and then only override our "real" non-zero exit
    code with the pager's non-zero, in cases where the pager also
    failed.

 D. I haven't found whether calling _exit() in the atexit() handler
    even has defined behavior, but in any case using it would
    short-circuit the documented program exit behavior defined in the
    C standard, of which calling atexit() handlers is just the first
    step.

1. https://lore.kernel.org/git/8735yhq3lc.fsf@evledraar.gmail.com/
2. https://pubs.opengroup.org/onlinepubs/009695399/functions/exit.html

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pager.c          | 10 ++++++++--
 t/t7006-pager.sh |  8 ++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/pager.c b/pager.c
index 3d37dd7adaa..2e743bc0b1e 100644
--- a/pager.c
+++ b/pager.c
@@ -20,18 +20,24 @@ static void close_pager_fds(void)
 
 static void wait_for_pager_atexit(void)
 {
+	int code;
 	fflush(stdout);
 	fflush(stderr);
 	close_pager_fds();
-	finish_command(&pager_process);
+	code = finish_command(&pager_process);
+	if (code)
+		exit(code);
 }
 
 static void wait_for_pager_signal(int signo)
 {
+	int code;
 	close_pager_fds();
-	finish_command_in_signal(&pager_process);
+	code = finish_command_in_signal(&pager_process);
 	sigchain_pop(signo);
 	raise(signo);
+	if (signo == SIGPIPE)
+		exit(code);
 }
 
 static int core_pager_config(const char *var, const char *value, void *data)
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435e..69997fa48f2 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -703,7 +703,7 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' '
 	test_path_is_file pager-used
 '
 
-test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
+test_expect_success TTY 'git respects pager non-zero exit without SIGPIPE' '
 	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager "wc >pager-used; exit 1" &&
 	GIT_TRACE2="$(pwd)/trace.normal" &&
@@ -713,7 +713,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
 	if test_have_prereq !MINGW
 	then
 		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
-		test "$OUT" -eq 0
+		test "$OUT" -eq 1
 	else
 		test_terminal git log
 	fi &&
@@ -724,7 +724,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' '
 	test_path_is_file pager-used
 '
 
-test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
+test_expect_success TTY 'git respects nonexisting pager without SIGPIPE' '
 	test_when_finished "rm pager-used trace.normal" &&
 	test_config core.pager "wc >pager-used; does-not-exist" &&
 	GIT_TRACE2="$(pwd)/trace.normal" &&
@@ -734,7 +734,7 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' '
 	if test_have_prereq !MINGW
 	then
 		OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) &&
-		test "$OUT" -eq 0
+		test "$OUT" -eq 127
 	else
 		test_terminal git log
 	fi &&
-- 
2.30.0.284.gd98b1dd5eaa7


  parent reply index

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-15 16:15 git fails with a broken pipe when one quits the pager Vincent Lefevre
2021-01-29 23:48 ` [PATCH] pager: exit without error on SIGPIPE Denton Liu
2021-01-30  8:29   ` Johannes Sixt
2021-01-30 12:52     ` Johannes Sixt
2021-02-01 15:03   ` Ævar Arnfjörð Bjarmason
2021-02-01 17:47     ` Junio C Hamano
2021-02-01 19:52       ` Ævar Arnfjörð Bjarmason
2021-02-01 20:55         ` Junio C Hamano
2021-02-02  2:05           ` Ævar Arnfjörð Bjarmason
2021-02-02  4:45             ` Junio C Hamano
2021-02-02  5:25               ` Junio C Hamano
2021-02-02  7:45                 ` Johannes Sixt
2021-02-02 20:13                   ` Junio C Hamano
2021-02-02 22:15                     ` Johannes Sixt
2021-02-02 22:21                       ` Junio C Hamano
2021-02-03 17:07                         ` Johannes Sixt
2021-02-03 18:12                           ` Junio C Hamano
2021-02-04 15:10                           ` Vincent Lefevre
2021-02-03  2:45                 ` Ævar Arnfjörð Bjarmason
2021-02-03  2:54                   ` Junio C Hamano
2021-02-03  3:36                     ` Ævar Arnfjörð Bjarmason
2021-02-03 17:19                     ` Johannes Sixt
2021-01-31  1:47 ` git fails with a broken pipe when one quits the pager Ævar Arnfjörð Bjarmason
2021-01-31  3:36   ` Vincent Lefevre
2021-01-31  3:47     ` Vincent Lefevre
2021-01-31 20:49     ` Ævar Arnfjörð Bjarmason
2021-02-01 10:34       ` Vincent Lefevre
2021-02-01 11:33         ` Chris Torek
2021-02-01 12:36           ` Vincent Lefevre
2021-02-01 12:53             ` Chris Torek
2021-02-01 15:17               ` Vincent Lefevre
2021-02-01 15:00           ` Ævar Arnfjörð Bjarmason
2021-02-01 12:10         ` Ævar Arnfjörð Bjarmason
2021-02-01 14:48           ` Vincent Lefevre
2021-02-01 15:44             ` Ævar Arnfjörð Bjarmason
2021-02-01 22:16               ` Johannes Sixt
2021-02-03  2:48                 ` Ævar Arnfjörð Bjarmason
2021-02-03 17:11                   ` Johannes Sixt
2021-02-03 15:26               ` Vincent Lefevre
2021-02-04  0:14                 ` Ævar Arnfjörð Bjarmason
2021-02-04 15:38                   ` Vincent Lefevre
2021-02-01 14:49           ` [PATCH 0/3] pager: test for exit behavior & trace2 bug fix Ævar Arnfjörð Bjarmason
2021-02-02  1:59             ` [PATCH v2 0/5] " Ævar Arnfjörð Bjarmason
2021-02-02  1:59             ` [PATCH v2 1/5] pager: refactor wait_for_pager() function Ævar Arnfjörð Bjarmason
2021-02-02  1:59             ` [PATCH v2 2/5] pager: test for exit code with and without SIGPIPE Ævar Arnfjörð Bjarmason
2021-02-02  8:50               ` Denton Liu
2021-02-05  7:47               ` Johannes Sixt
2021-02-02  1:59             ` [PATCH v2 3/5] run-command: add braces for "if" block in wait_or_whine() Ævar Arnfjörð Bjarmason
2021-02-02  2:00             ` [PATCH v2 4/5] pager: properly log pager exit code when signalled Ævar Arnfjörð Bjarmason
2021-02-05  7:58               ` Johannes Sixt
2021-02-05 11:37                 ` Junio C Hamano
2021-02-02  2:00             ` Ævar Arnfjörð Bjarmason [this message]
2021-02-01 14:49           ` [PATCH 1/3] pager: test for exit code Ævar Arnfjörð Bjarmason
2021-02-01 14:49           ` [PATCH 2/3] pager: refactor wait_for_pager() function Ævar Arnfjörð Bjarmason
2021-02-01 14:49           ` [PATCH 3/3] pager: properly log pager exit code when signalled Ævar Arnfjörð Bjarmason
2021-02-01 18:07             ` Junio C Hamano
2021-02-01 19:21               ` Ævar Arnfjörð Bjarmason
2021-02-01 18:15             ` Junio C Hamano
2021-02-01 19:23               ` Ævar Arnfjörð Bjarmason
2021-02-01 22:04       ` git fails with a broken pipe when one quits the pager Johannes Sixt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210202020001.31601-6-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=chris.torek@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=jeffhost@microsoft.com \
    --cc=liu.denton@gmail.com \
    --cc=vincent@vinc17.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git