Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 26, 2024, 21:23 UTC
Message-ID: <CAOLa=ZR9qQNDRy_gRGxw-78=CkqbqV_6-AFtdJo7WEjQNqQyAw@mail.gmail.com>
In-Reply-To: <xmqq8qyrzgi5.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 30 quoted lines

> Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
>
>> On 26/06/24 23:05, Junio C Hamano wrote:
>>> Abhijeet Sonar <abhijeet.nkt@gmail.com> 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`.

Show 9 quoted lines

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

Show 6 quoted lines

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