Re: [PATCH v4 3/6] pager: introduce wait_for_pager

Phillip Wood [off-list ref] writes:

quoted

  +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?

Good point.

quoted

+	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()

Yeah, that needs fixing.

quoted

+	if (!once++) {

We only need to increment "once" when we enter this block, not every
time the code is run.

Running this 4 billion times and we'll be in a trouble ;-).

quoted

+		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().

Good point.