Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

From: Jeff King <peff@peff.net>
Date: Oct 19, 2022, 18:57 UTC
Message-ID: <Y1BIfEN5ds2kKjjk@coredump.intra.peff.net>
In-Reply-To: <xmqq7d0vsngp.fsf@gitster.g>

On Wed, Oct 19, 2022 at 11:00:22AM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >> True. run_command() needs the RUN_* flags twiddling, too, so it is
> >> not a point against _l_opt() variant.
> >
> > Did you mean run_command_v() here? If so, then yes, it requires the
> > flags. But if we are going to drop it in favor of just run_command(),
> > then those flags go away (and moving to the _l() variant is a step in
> > the opposite direction).
> 
> Ah, but we'd still need to prepare individual bits in the child
> structure (e.g. RUN_GIT_CMD vs .git_cmd) anyway.  Even though we may
> not have to work with the flags, somehow we need to set it up.  So
> it is not all that strong argument against the _l_opt().

Right, to the callers it is no different; they must use the flags or the struct fields. What I meant is that we do not need to support _both_. I.e., the big mapping in run_command_v_opt_cd_env_tr2() goes away.

Show 5 quoted lines

> The above assessment is primarily because I do not have too much
> against RUN_GIT_CMD and friends, compared to setting the individual
> members in the child_process struct.  Setting individual members in
> the struct is more direct and it may be an improvement use
> run_command() directly over using _v_opt() and _l_opt() variants.

Yeah. I do not find RUN_GIT_CMD all that bad myself; I was mainly pointing out that we do not need to support both of them (and of the two, the struct fields are by far the more flexible, so that's what we should keep).

That said, I am not too sad to have both. I would not have bothered to do the work to remove the _v() versions with flags. But since René already did so...

-Peff