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. :) 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
