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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Oct 28 2022, René Scharfe wrote:

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

You mean just to pinky promise to always try to set the flags right
after we declare variables. Yes, we can try to aim for that, but
sometimes you need to declare quite a few of them (e.g. that difftools
caller), so not having distance between the decl & setting can help.
>> 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.

Yes, maybe these messages aren't all that useful. But note that that
"error" is emittted by thy "#ifndef ...WINDOWS..." part of the code. See
also the recent 255a6f91ae4 (t1800: correct test to handle Cygwin,
2022-09-15).

But I suspect some of the "die" messaging is just boilerplate/legacy.

> 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

Isn't this just the "cannot spawn"/"cannot fork" etc. messaging from
start_command() now?

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

*nod*, it would be nice if that messaging was portable. Part of that
just seems to be that we need to do the same (or share) the whole "set
die message" part we do on *nix.

Some of the reasons a command fails aren't portable, but we should be
able to portably emit e.g. "can't execute this command because it
doesn't exist" etc.