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

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> 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.