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

From: René Scharfe <l.s.r@web.de>
Date: Oct 28, 2022, 17:16 UTC
Message-ID: <b200477c-f856-f107-b897-63721c805093@web.de>
In-Reply-To: <221028.8635b87wo6.gmgdl@evledraar.gmail.com>

Am 28.10.22 um 18:11 schrieb Ævar Arnfjörð Bjarmason:

Show 37 quoted lines

>
> 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. Or at the bottom, before the run_command() call.

Show 12 quoted lines

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

René