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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 22, 2024, 10:18 UTC
Message-ID: <5360ab9d-6d3e-4da0-b1c4-2ff381372c1a@gmail.com>
In-Reply-To: <a2ea00e2-08e4-4e6b-b81c-ef3ba02b4b1f@gmail.com>

Hi Rubén

On 20/07/2024 23:29, Rubén Justo wrote:

Show 12 quoted lines

> On Wed, Jul 17, 2024 at 08:39:12PM +0100, phillip.wood123@gmail.com wrote:
> 
>> On 17/07/2024 18:20, Rubén Justo wrote:
>>
>>> However, this error has exposed a problem: calling `wait_for_pager` if
>>> `setup_pager` hasn't worked is an issue that needs to be addressed in this
>>> series: `setup_pager` should return a result.  I was planning to do that
>>> in a future series, for the other commented command: `|[cmd]`.
>>
>> What was causing setup pager to fail in this test?
> 
> Because GIT_PAGER is not being set correctly in the test,

Oh I see, I had assumed that the shell would report an error if it didn't support setting environment variables like this when running a function but instead it silently ignored it. This highlights the importance of testing the output of "git add -p" in this test so we can be sure the pager is doing what we think it should. Using

	GIT_PAGER="sed -e s/^/PAGER\ / -e q"

would make it clear what the pager is printing while also causing SIGPIPE

Show 12 quoted lines

> "git add -p"
> can use the values defined in the environment where the test is running.
> Usually PAGER is empty or contains "less", but in the environment where
> the fault occurs, it happens to be: "PAGER=cat".
> 
> Since we have an optimization to avoid forking if the pager is "cat",
> courtesy of caef71a535 (Do not fork PAGER=cat, 2006-04-16), then we fail
> in `wait_for_pager()` because we are calling `finish_command()` with an
> uninitialized `pager_process`.
> 
> That's why I thought, aligned with what we are already doing in
> `wait_for_pager_at_exit()`, that this is a sensible approach:

That extra information is important. When I said [1]

 > Isn't it a bug to call this with old_fd1 == -1 or have I missed
 > something?

What I'd missed was that we can return early without executing anything. We cannot do

	if (!git_pager(asatty(1))
		return

at the beginning of wait_for_pager() because if we're running a pager isatty(1) will return false so I think the old_fd as you suggested is the easiest fix. The existing callers do not need to know if setup_pager() applied the "cat" optimization because they only setup the pager once. For "add -p" this no-longer applies so we should think about returning a flag to say "there was an error"/"there is no pager or the pager is 'cat'"/"the pager has been started"

Best Wishes

Phillip

[1] https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com

Show 19 quoted lines

>>> I'm wondering if the best way to proceed here is to revert to:
>>>
>>> diff --git a/pager.c b/pager.c
>>> index 5f0c1e9cce..5586e751dc 100644
>>> --- a/pager.c
>>> +++ b/pager.c
>>> @@ -46,6 +46,8 @@ static void wait_for_pager_atexit(void)
>>>
>>>    void wait_for_pager(void)
>>>    {
>>> +	if (old_fd1 == -1)
>>> +		return;
>>>    	finish_pager();
>>>    	sigchain_pop_common();
>>>    	unsetenv("GIT_PAGER_IN_USE");
>>>
>>> This was a change already commented here:
>>>
>>> https://lore.kernel.org/git/3f085795-79bd-4a56-9df8-659e32179925@gmail.com/