Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> writes:
> On 26/06/24 23:05, Junio C Hamano wrote:
>> Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> writes:
>>
>>> To me, this looks much better. child_process_clear's name already
>>> suggests that is sort of like a destructor, so it makes sense to
>>> re-initialize everything here. I even wonder why it was not that way to
>>> begin with. I suppose no callers are assuming that it only clears args
>>> and env though?
>>
>> I guess that validating that supposition is a prerequisite to
>> declare the change as "much better" and "makes sense".
>
> OK. I found one: at the end of submodule.c:push_submodule()
>
> if (...) {
> ...some setup...
> if (run_command(&cp))
> return 0;
> close(cp.out);
> }
This is curious.
* What is this thing trying to do? When run_command() fails, it
wants to leave cp.out open, so that the caller this returns to
can write into it??? That cannot be the case, as cp itself is
internal. So does this "close(cp.out)" really matter?
* Even though we are running child_process_clear() to release the
resources in run_command() we are not closing the file descriptor
cp.out in the child_process_clear() and force the caller to close
it instead. An open file descriptor is a resource, and a file
descriptor opened but forgotten is considered a leak. I wonder
if child_process_clear() should be closing the file descriptor,
at least the ones it opened or dup2()ed.
In any case, you found a case where child_process_clear() may not
want to do the full re-initialization and at the same time it is not
doing its job sufficiently well. Let's decide, at least for now,
not to do the reinitialization from child_process_clear(), then.
Thanks.
