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

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> 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.

Up to this point, it is fine.

But ...

> For the tests, avoid the common construct to set and export one-shot
> variables within the scope of a command:
>
>     VAR=VAL command args
>
> It happens that when "command" is a shell function that in turn executes
> a "command", the behavior with "VAR" varies depending on the shell:
>
>  ** Bash 5.2.21 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=1
>
>  ** dash 0.5.12-9 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=1
>
>  ** dash 0.5.10.2-6 **
>
>     $ f () { bash -c 'echo A=$A'; }
>     $ A=1 f
>     A=
>
> POSIX explicitly says the effect of this construct is unspecified.

... unless the patch is about changing an existing

	GIT_PAGER=... test_terminal git add -p

into

	(
		GIT_PAGER=...; export GIT_PAGER
		test_terminal git add -p
	)

then all of the above is irrelevant noise that says "as the coding
guidelines tell us not to use the non-portable 'VAR=VAL shell_func'
construct, we don't."

Always write your proposed log message to those who will read "git
log -p" 6 months down the road.  To them, the trouble we had while
diagnosing the true cause of the breakage in the previous iteration
do not exist.  It is not part of the "log -p" output they see.

> One of our CI jobs on GitHub Actions uses Ubuntu 20.04 running dash
> 0.5.10.2-6, so avoid using the construct and use a subshell instead.

And it does not matter if all CI platforms are updated.  As long as
our coding guidelines say not to use this construct, we don't.

In any case, that is an appropriate thing to say in a commit that
fixes use of such a construct, but not a commit that uses the right
constuct from the get-go.

I have to say that the [4/4] in the previous round, i.e., fc87b2f7
(add-patch: render hunks through the pager, 2024-07-25) in my tree,
is better than this version.

Thanks.