Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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?
>
This is curious one indeed, we only need to close the file descriptor
when we set `cp.out = -1`, otherwise there is no need to close `cp.out`
since `start_command()` takes care of it internally. So the
`close(cp.out)` can really be removed here.
Wonder if this was copied over from other parts of the submodule code,
where we actually set `cp.out = -1`.
>
> * 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.
>
If you see the documentation for run_command.h::child_process, it states
/*
* Using .in, .out, .err:
* - Specify 0 for no redirections. No new file descriptor is allocated.
* (child inherits stdin, stdout, stderr from parent).
* - Specify -1 to have a pipe allocated as follows:
* .in: returns the writable pipe end; parent writes to it,
* the readable pipe end becomes child's stdin
* .out, .err: returns the readable pipe end; parent reads from
* it, the writable pipe end becomes child's stdout/stderr
* The caller of start_command() must close the returned FDs
* after it has completed reading from/writing to it!
* - Specify > 0 to set a channel to a particular FD as follows:
* .in: a readable FD, becomes child's stdin
* .out: a writable FD, becomes child's stdout/stderr
* .err: a writable FD, becomes child's stderr
* The specified FD is closed by start_command(), even in case
* of errors!
*/
int in;
int out;
int err;
So this makes sense right? run_command() doesn't support the pipe
options, meaning clients have to call start_command() + finish_command()
manually. When clients do call start_command() with '-1' set to these
options, they are in charge of closing the created pipes.
> 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.
Attachment:
signature.asc
Description: PGP signature
