Re: [PATCH 0/8] run-command: remove run_command_v_*()
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 28, 2022, 14:23 UTC
- Message-ID
- <943608ab-340d-8cd3-8812-c0f65909000a@web.de>
- In-Reply-To
- <Y1rl9arDRGay0CHm@coredump.intra.peff.net>
Am 27.10.22 um 22:11 schrieb Jeff King:
Show 28 quoted lines
> 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. :)
Yes, almost, except that I think 5-7 are doing necessary pruning and 8 requires a small leap of faith in the value of simplicity. And I wanted to shrink down and simplify that last patch.
René