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

> Am 28.10.22 um 18:11 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Fri, Oct 28 2022, René Scharfe wrote:
>>
>>> Am 27.10.22 um 23:46 schrieb Ævar Arnfjörð Bjarmason:
>>>>
>>>> - I wish C had a nicer syntax for not just declaring but squashing
>>>>   together compile_time bracketed lists (think set operations). But the
>>>>   below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version.
>>>>
>>>>   I see gcc/clang nicely give us safety rails for that with
>>>>   "-Woverride-init", for this sort of "opts struct with internal stuff,
>>>>   but also user options" I think it works out very nicely.
>>>>
>>>
>>> That's a nice and simple macro.  I played with a gross variant à la
>>>
>>>   #define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }
>>>
>>> which would allow e.g.
>>>
>>>   struct child_process cmd = CHILD_PROCESS_INIT_EX(.git_cmd = 1);
>>>
>>> Yours is better,
>>
>> I actually think yours is better, anyway...
>>
>>> but they share the downside of not actually saving any lines of code..
>>
>> To me it's not about saving code, but that it's immediately obvious when
>> reading the code that this set of options can be determined and set at
>> function or scope entry.
>>
>> We tend to otherwise have creep where the decl and option init drifts
>> apart over time, and with complex init's you might stare at it for 30s,
>> before realizing that between the decl and fully init ing it often 50
>> lines later nothing actually changed vis-a-vis the state, we could have
>> just done it earlier.
>
> Hmm, we could do that by collecting the flag setting parts at the top,
> without the need for a new macro.

>> I think that's worth it in general, whether it's worth the churn in this
>> case...
>>
>>>> - We have quite a few callers that want "on error, die", so maybe we
>>>>   should have something like that "on_error" sooner than later.
>>>
>>> We could add a die_on_error bit for that, or start_command_or_die() and
>>> run_command_or_die() variants (there I go again, multiplying APIs..).
>>> They could report the failed command, which a caller can't do because
>>> the internal strvec is already cleared once it learns of the failure.
>>
>> *nod*
>
> Wait a second: Does it even make sense to mention the command in a die()
> message after start_command() failed?  Unless .silent_exec_failure is
> set, start_command() already reports it.  E.g. archive-tar.c has:
>
> 	if (start_command(&filter) < 0)
> 		die_errno(_("unable to start '%s' filter"), cmd.buf);
>
> ... and the result looks like this upon failure:
>
>    $ git -c tar.tgz.command=nonsense archive --format=tgz HEAD
>    error: cannot run nonsense: No such file or directory
>    fatal: unable to start 'nonsense' filter: No such file or directory
>
> The second message is mostly redundant, but it mentions that the failed
> command was a filter.  Probably .silent_exec_failure should be set here,
> then the die() message is no longer redundant.  This requires args[0] to
> be stored outside of struct child_process, though, which is already done
> here, but may be a bit tedious in other cases.

> So for start_command(), would it be a generally useful to support a
> scenario where upon failure
>
> - the program terminates,
> - but before that prints a single message,
> - which includes the command that could not be started

> - and some kind of hint why we tried to start it?
>
> For run_command() we'd need to distinguish between not being able to
> run the command and getting an error code from it after a successful
> start.