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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2024, 18:49 UTC
Message-ID: <xmqq8qyrzgi5.fsf@gitster.g>
In-Reply-To: <f7d0abce-b389-45ae-992a-adbc7ec10d50@gmail.com>

Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:

Show 20 quoted lines

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

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