Am 27.10.22 um 22:11 schrieb Jeff King: > 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@xxxxxx/ > > 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é
