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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2024, 16:05 UTC
Message-ID: <xmqqy16t6m8u.fsf@gitster.g>
In-Reply-To: <xmqq34p1813n.fsf@gitster.g>

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

Show 15 quoted lines

> (#leftoverbit)
>
> Outside the scope of this patch, I'd prefer to see somebody makes
> sure that it is truly equivalent to prepare a separate and new
> struct child_process for each run_command() call and to reuse the
> same struct child_process after calling child_process_clear() each
> time.  It is unclear if they are equivalent in general, even though
> in this particular case I think we should be OK.
>
> There _might_ be other things in the child_process structure that
> need to be reset to the initial state before it can be reused, but
> are not cleared by child_process_clear().  .git_cmd and other flags
> as well as in/out/err file descriptors do not seem to be cleared,
> and other callers of run_command() may even be depending on the
> current behaviour that they are kept.

Ahh, the reuse of the same struct came directly from Karthik's review on the second iteration. I guess Karthik volunteered himself into this #leftoverbit task? I am not convinced that

 (1) the selective clearing done by current child_process_clear() is
     the best thing we can do to make child_process reusable, and

 (2) among the current callers, there is nobody that depends on the
     state left by the previous use of child_process in another
     run_command() call that is left uncleared by child_process_clear().

If (1) is false, then reusing child_process structure is not quite safe, and if (2) is false, updating child_process_clear() to really clear everything will first need to adjust some callers.

Thanks.