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

From: Rubén Justo <rjusto@gmail.com>
Date: Jul 15, 2024, 20:04 UTC
Message-ID: <1e38a2f0-623c-46cf-b5c5-9e3a4b153cac@gmail.com>
In-Reply-To: <384f0147-d611-493b-a3d4-d83c65bd1114@gmail.com>

On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote:

Show 19 quoted lines

> Hi Rubén
> 
> On 14/07/2024 17:04, Rubén Justo wrote:
> > Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
> > 2006-02-28) we have the machinery to send our output to a pager.
> > 
> > That machinery, once set up, does not allow us to regain the original
> > stdio streams.
> > 
> > In the interactive commands (i.e.: add -p) we want to use the pager for
> > some output, while maintaining the interaction with the user.
> > 
> > Modify the pager machinery so that we can use setup_pager and, once
> > we've finished sending the desired output for the pager, wait for the
> > pager termination using a new function wait_for_pager.   Make this
> > function reset the pager machinery before returning.
> 
> Do you have any comments on my thoughts in
> <8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ?

Oops! I thought I had responded, but somehow I must not have.

For reference, these are the points you indicated:

Show 7 quoted lines

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

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.

Sorry for not responding sooner.