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

From: Junio C Hamano <[email-masked]>
Date: Tue, 04 Jun 2024 09:29:30 -0700

Phillip Wood <[email-masked]> writes:

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

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

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

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