Re: [PATCH v3 4/4] add-patch: render hunks through the pager

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Rubén

On 14/07/2024 17:04, Rubén Justo wrote:
> Make the print command trigger the pager when invoked using a capital
> 'P', to make it easier for the user to review long hunks.
> 
> Note that if the PAGER ends unexpectedly before we've been able to send
> the payload, perhaps because the user is not interested in the whole
> thing, we might receive a SIGPIPE, which would abruptly and unexpectedly
> terminate the interactive session for the user.
> 
> Therefore, we need to ignore a possible SIGPIPE signal.  Add a test for
> this, in addition to the test for normal operation.
> 
> For the SIGPIPE test, we need to make sure that we completely fill the
> operating system's buffer, otherwise we might not trigger the SIGPIPE
> signal.  The normal size of this buffer in different OSs varies from a
> few KBs to 1MB.  Use a payload large enough to guarantee that we exceed
> this limit.

Thanks for updating the commit message to explain the purpose of the 
SIGPIPE test

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---

> +test_expect_success TTY 'P does not break if pager ends unexpectedly' '

I think it would be helpful to mention SIGPIPE in the title as this test 
is really checking "we don't die if we receive SIGPIPE". Maybe

     P handles SIGPIPE when writing to pager

> +	test_when_finished "rm -f huge_file; git reset" &&
> +	printf "\n%2500000s" Y >huge_file &&
> +	git add -N huge_file &&
> +	test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p >actual

If we're not going to look at the output we don't need to redirect it. 
I'm not sure if there is any benefit to comparing the actual output to 
what we expect here.

Best Wishes

Phillip

> +'
> +
>   test_expect_success 'split hunk "add -p (edit)"' '
>   	# Split, say Edit and do nothing.  Then:
>   	#