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

From: Jeff King <peff@peff.net>
Date: Oct 19, 2022, 17:06 UTC
Message-ID: <Y1AujKKPUJFepph5@coredump.intra.peff.net>
In-Reply-To: <xmqq7d0vu8cw.fsf@gitster.g>

On Wed, Oct 19, 2022 at 08:43:43AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I do think Junio saying "consensus" may have been premature. I expressed
> > my opinion and he agreed, but I think that is as far as it got. :)
> 
> Maybe.  This is a tangent, but as far as I am concerned, when Réne
> writes something that looks to me very straight-forward and correct,
> and it passes your taste buds, then that is enough consensus to move
> ahead.  As Linus often said and I concur, some people got good
> taste, that is hard to quantify and probably hard to teach, and
> there are a handful folks here with good taste.  And when two who
> have demonstrated they are with good taste agrees, that is good
> enough to me.

I pretty much agree with that worldview, but I try hard not to assume "my taste is good" when going into a technical discussion. If only because I've embarrassed myself a sufficient number of times. :)

Show 10 quoted lines

> >> I don't see how *_l_opt() is particularly error prone, I just had a
> >> stupid think-o in v1 of this, but that if/else if bug is something that
> >> could have snuck in with run_command() given the same stupidity :)
> >
> > I don't think it's error-prone. It just seems like it complicates an API
> > for little gain, and causes us to have a lot of boilerplate mapping
> > RUN_* flags into cmd.* fields.
> 
> 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).

-Peff