Re: [PATCH v2] bisect--helper: plug strvec leak

From: Jeff King <peff@peff.net>
Date: Oct 14, 2022, 19:44 UTC
Message-ID: <Y0m8AU+Yf7PREI2E@coredump.intra.peff.net>
In-Reply-To: <xmqq8rlmujcz.fsf@gitster.g>

On Tue, Oct 11, 2022 at 02:43:24PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> > -	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> > +	if (run_command_opt_l(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
> > +			      "set", NULL)) {
> 
> And this does give us protection from the "Programmers can give
> unterminated list to run_command_v_opt() by mistake", which is not
> really solved mechanically even if the list is prepared with the
> strvec API (because the compiler has to be smart enough to know that
> argv.v was prepared with proper use of the API), which is nice.

Yeah, I agree this addresses the point I raised (which I am somewhat regretting raising, as IMHO it was not worth the amount of discussion that has ensued).

Since nobody asked, my _real_ opinion is that I prefer René's original that used an actual struct, and its auto-freeing strvec. And I'd be happy to see all of the run_command_v() variants go away entirely. It does save a few lines, but with modern niceties, it's not very many, and it's much less flexible.

I resisted saying so earlier because I do not think it is even worth anybody's time to think about, let alone implement.

-Peff