Hi Rubén
On 03/06/2024 21:38, 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.
This makes sense, I've left a few comments below
Signed-off-by: Rubén Justo <rjusto@xxxxxxxxx>
---
static void wait_for_pager_atexit(void)
{
+ if (old_fd1 == -1)
+ return;
+
This is good - we'll return early if we've already cleaned up the pager.
fflush(stdout);
fflush(stderr);
close_pager_fds();
finish_command(&pager_process);
}
+void wait_for_pager(void)
+{
+ if (old_fd1 == -1)
+ return;
Isn't it a bug to call this with old_fd1 == -1 or have I missed something?
+ wait_for_pager_atexit();
+ unsetenv("GIT_PAGER_IN_USE");
+ dup2(old_fd1, 1);
+ old_fd1 = -1;
+ if (old_fd2 != -1) {
+ dup2(old_fd2, 2);
+ old_fd2 = -1;
We're leaking old_fd1 and old_fd2 here. wait_for_pager_atexit() flushes
stdout and stderr so this switching of fds should play nicely with code
that uses stdio.
@@ -113,6 +134,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
void setup_pager(void)
{
+ static int once = 0;
const char *pager = git_pager(isatty(1));
if (!pager)
@@ -142,16 +164,18 @@ void setup_pager(void)
return;
/* original process continues, but writes to the pipe */
+ old_fd1 = dup(1);
dup2(pager_process.in, 1);
if (isatty(2)) {
- close_fd2 = 1;
+ old_fd2 = dup(2);
dup2(pager_process.in, 2);
}
close(pager_process.in);
- /* this makes sure that the parent terminates after the pager */
- sigchain_push_common(wait_for_pager_signal);
- atexit(wait_for_pager_atexit);
+ if (!once++) {
We only need to increment "once" when we enter this block, not every
time the code is run.
+ sigchain_push_common(wait_for_pager_signal);
I think we should be calling this each time we setup the pager and pop
it in wait_for_pager(). Imagine a caller sets up a signal handler before
calling setup_pager() and wants to pop it after the pager has finished
sigchain_push(...)
setup_pager(...)
do_something()
wait_for_pager()
sigchain_pop(...)
With the changes here it will pop the signal handler added by
setup_pager() rather than the one it is expecting.
+ atexit(wait_for_pager_atexit);
It is a bit of a shame we have to leave this function active when the
pager has finished. We could add a wrapper around atexit() that allows
us to pop functions we no-longer want to call but I don't think it is
worth the effort here. wait_for_pager_atexit() is careful to return
early if it is not needed.
Best Wishes
Phillip
