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

[Junio C Hamano <gitster@pobox.com>]

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

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