Re: [PATCH 4/8] use child_process member "args" instead of string array variable

[Taylor Blau <me@ttaylorr.com>]

On Fri, Oct 28, 2022 at 04:23:31PM +0200, René Scharfe wrote:
> > A lot of your 3-lines would be 1 lines if we just had e.g. (untested,
> > and could be a function not a macro, but you get the idea):
> >
> > 	#define run_command_git_simple(__VA_ARGS__) \
> > 		struct child_process cmd = CHILD_PROCESS_INIT; \
> > 		cmd.git_cmd = 1; \
> > 		strvec_pushl(&cmd.args, __VA_ARGS__); \
> > 		run_command(&cmd);
> >
> > But maybe nobody except me thinks that's worthwhile...
>
> I have similar temptations; you could see that in my scratch patch
> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/
> which added run_git_or_die() in builtin/gc.c.  Why, oh why?  Perhaps
> because taking a blank form (CHILD_PROCESS_INIT), ticking boxes
> (.git_cmd = 1), filling out text fields (strvec_push(...)) and
> submitting it (run_command()) feels tedious and bureaucratic, Java-esque
> even.  And some patterns appear again and again.
>
> How bad is that?  Is it bad at all?  I think overall we should try to
> reduce the number of external calls and make those we have to do
> self-documenting and leak-free.  A bit of tedium is OK; this API should
> be used rarely and sparingly.  Still I get the urge to search for
> patterns and define shortcuts when I see all those similar calls..
>
> run_command_git_simple as defined above wouldn't compile, but I get it.
> Reducing the number of lines feels good, but it also makes the code less
> flexible -- adding a conditional parameter requires converting back to
> run_command().

For what it's worth, I agree. I don't think there are or should be any
hard and fast rules about when extracting a pattern like this into a
function or macro is right. But here it feels wrong and
counterproductive to the goal of this series.

My main gripe with it is that it seems to be overfit to these small
handful of calls, and that changing it in the future would require us to
go and update existing callers to accommodate new functionality in other
callers.

So I'd prefer to see it left alone.

Thanks,
Taylor