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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2024, 15:59 UTC
Message-ID: <xmqq34p1813n.fsf@gitster.g>
In-Reply-To: <20240625133534.223579-1-abhijeet.nkt@gmail.com>

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

Show 8 quoted lines

>  		if (broken) {
>  			struct child_process cp = CHILD_PROCESS_INIT;
> +			strvec_pushv(&cp.args, update_index_args);
> +			cp.git_cmd = 1;
> +			cp.no_stdin = 1;
> +			cp.no_stdout = 1;
> +			run_command(&cp);
> +			strvec_clear(&cp.args);

Why clear .args here?

Either "struct child_process" is reusable after finish_command() that is called as the last step of run_command() returns successfully, or it shouldn't be reused at all. And when finish_command() is called, .args as well as .env are cleared because it calls child_process_clear().

I am wondering if the last part need to be more like

	...
	cp.no_stdout = 1;
	if (run_command(&cp))
		child_process_clear(&cp);

> +
>  			strvec_pushv(&cp.args, diff_index_args);
>  			cp.git_cmd = 1;
>  			cp.no_stdin = 1;

Thanks.

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