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

[phillip.wood123@gmail.com]

Hi Rubén

On 15/07/2024 21:04, Rubén Justo wrote:
> 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