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

From: René Scharfe <l.s.r@web.de>
Date: Oct 20, 2022, 18:34 UTC
Message-ID: <84971a3a-4c5c-5b79-582e-88b3bd8c624f@web.de>
In-Reply-To: <xmqq5ygfr48b.fsf@gitster.g>

Am 19.10.22 um 21:41 schrieb Junio C Hamano:

Show 7 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> 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...
>
> It makes two of us ;-)

I wrote that patch out of curiosity and was pleased by the total line count reduction, getting rid of the on-stack array construction sites with their magic numbers, not having to manage strvecs explicitly anymore and the callers staying readable.

One weak spot is the new helper builtin/gc.c::run_git_or_die() that I added because it was easier than replacing all those strvecs that are prepared before deciding whether their commands are even needed.

Stripping down the central API to a single shared object (a struct and functions that get it passed) simplifies it for programmers. It takes the idea of d3b2159712 (run-command API: remove "argv" member, always use "args", 2021-11-25) and c7c4bdeccf (run-command API: remove "env" member, always use "env_array", 2021-11-25) to its logical conclusion of going fully dynamic and using standard strvec functions everywhere. Local shortcuts like builtin/gc.c::run_git_or_die() may still be defensible.

But still: Is all of that code churn worth it?  Not sure.

René