Re: [PATCH v3 3/4] pager: introduce wait_for_pager

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Jul 17, 2024, 14:58 UTC
Message-ID: <cf999478-b1fd-4c93-a11b-1b34d51767d3@gmail.com>
In-Reply-To: <1e38a2f0-623c-46cf-b5c5-9e3a4b153cac@gmail.com>

Hi Rubén

On 15/07/2024 21:04, Rubén Justo wrote:

Show 17 quoted lines

> On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote:
> For reference, these are the points you indicated:
> 
>>   - We ignore any errors when duplicating fds,
>>     "git grep '[^a-z_]dup2\{0,1\}(' shows that's not unusual in our
>>     code base, though if we cannot redirect the output to the pager or
>>     restore stdout when the pager exits that's a problem for "git add -p"
>>
>>   - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them
>>     being passed to the pager.
> 
> Both points are interesting and improve resilience to unexpected
> situations.  I remember that the first point was already suggested in
> the previous thread.
> 
> IMHO both points should be considered with a more global perspective
> than the scope of this series.

I'm not sure what you mean by this. It is true that we were ignoring dup2() errors in this function before but this patch the O_CLOEXEC issue is new.

> As I said in the first message of this thread, I have left out
> interesting points that may deserve to be addressed in future series,
> with the intention of not prolonging the discussion of the current
> changes too much.

I think there is a difference between adding new features which I agree should be left and getting the implementation details of this helper function right. Having said that ignoring the dup() errors isn't making anything worse than it was and the extra fds are for the terminal which the pager is accessing anyway.

> Sorry for not responding sooner.

No worries, thanks for your reply

Phillip