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