Abhijeet Sonar <abhijeet.nkt@xxxxxxxxx> 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.
