Re: [PATCH 0/8] run-command: remove run_command_v_*()

[Jeff King <peff@peff.net>]

On Thu, Oct 27, 2022 at 06:30:36PM +0200, René Scharfe wrote:

> Replace the convenience functions run_command_v_opt() et. al. and use
> struct child_process and run_command() directly instead, for an overall
> code reduction and a simpler and more flexible API that allows creating
> argument lists without magic numbers and reduced risk of memory leaks.
> 
> This is a broken-out and polished version of the original scratch at
> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/

I read through this and it all looks fine to me. I was a bit puzzled at
the layout of your series at first. In particular, the difference
between cases in patch 4 versus the later ones.

I think it is that in patch 4, these are all unambiguously positive
because we are getting rid of magic numbers (or magically-sized arrays).
Whereas in patches 5-8, there's nothing inherently wrong with the
call-sites; but as we get rid of the API wrappers, we convert them. So
they are collateral damage, so to speak, from the simplification of the
API.

That makes sense to me, though I could point out that most of the sites
cleaned up in patch 4 _could_ be converted to look like the ones that
are converted in 5-8. Obviously that doesn't make sense to do, knowing
that 5-8 are coming. But if the point in splitting it this way is to
show that we could stop at patch 4, cleaning up call sites but not
shrinking the run-command API, then I just want to point out that there
is another way to do those cleanups. :)

All that said, you will be unsurprised to hear that I am on board with
the direction of 5-8, so this all looks good to me. I'm just laying out
my thought process while reviewing.

I also think it would have been fine to just drop the whole API in one
patch (i.e., squashing 5-8), which makes the intermediate diffs a bit
shorter. But I do not mind at all having the cases broken out.

Thanks for cleaning this up.

-Peff