Re: [PATCH] pager: exit without error on SIGPIPE

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 1, 2021, 15:03 UTC
Message-ID: <87czxjomn8.fsf@evledraar.gmail.com>
In-Reply-To: <bc88492979fee215d5be06ccbc246ae0171a9ced.1611910122.git.liu.denton@gmail.com>

On Sat, Jan 30 2021, Denton Liu wrote:

> [...]

The thread at large has enough about whether this approach even makes sense. I won't repeat that here. Just small notes on the patch itself:

Show 22 quoted lines

> diff --git a/Makefile b/Makefile
> index 4edfda3e00..38a1a20f31 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-mktemp.o
>  TEST_BUILTINS_OBJS += test-oid-array.o
>  TEST_BUILTINS_OBJS += test-oidmap.o
>  TEST_BUILTINS_OBJS += test-online-cpus.o
> +TEST_BUILTINS_OBJS += test-pager.o
>  TEST_BUILTINS_OBJS += test-parse-options.o
>  TEST_BUILTINS_OBJS += test-parse-pathspec-file.o
>  TEST_BUILTINS_OBJS += test-path-utils.o
> diff --git a/pager.c b/pager.c
> index ee435de675..5922d99dc8 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -34,6 +34,8 @@ static void wait_for_pager_atexit(void)
>  static void wait_for_pager_signal(int signo)
>  {
>  	wait_for_pager(1);
> +	if (signo == SIGPIPE)
> +		exit(0);

As shown in https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ this leaves us without guard rails where the pager dies/segfaults or whatever.

That's an existing bug, but by not carrying the SIGPIPE forward it changes from "most of the time we'd exit with SIGPIPE anyway" to "we'll never notice".

Show 6 quoted lines

> [...]
> +test_expect_success TTY 'SIGPIPE from pager returns success' '
> +	test_terminal env PAGER=true test-tool pager
> +'
> +
>  test_done

As noted in https://lore.kernel.org/git/20210201144921.8664-1-avarab@gmail.com/ I think this whole "test-tool pager" isn't needed. We can just use git itself with some trickery.