Re: [PATCH v3] pager: fix crash when pager program doesn't exist

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 24, 2021, 15:15 UTC
Message-ID: <CAPig+cT0-bu2tcgJrRR+9J86bmGEOOpFZv0JygCp26gieP-2pg@mail.gmail.com>
In-Reply-To: <20211124145409.8779-1-ematsumiya@suse.de>

On Wed, Nov 24, 2021 at 9:54 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:

Show 15 quoted lines

> [...]
> Reproducer:
> $ git config pager.show INVALID_PAGER
> $ git show $VALID_COMMIT
> error: cannot run INVALID_PAGER: No such file or directory
> [1]    3619 segmentation fault (core dumped)  git show $VALID_COMMIT
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
> ---
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> @@ -786,4 +786,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' '
> +test_expect_success TTY 'handle attempt to run an invalid pager' '
> +       test_config pager.show invalid-pager &&
> +       test_terminal git show
> +'

This is a minor observation (so you decide what value it might have), but the terminology "handle ... invalid pager" in the test title doesn't convey very much information to some future reader of this test, and that person will be forced to consult the commit message -- which does a good job of explaining the problem -- to really understand what this test is checking. If you change the title to, for instance:

    non-existent pager doesn't cause crash

then the reader will have an easier time understanding what this test is about. It's true that the reader will still need to consult the commit message for a detailed picture of the problem, but won't be left head-scratching, as might be the case with the current test title.