Re: [PATCHv4 3/5] run-command: add {run,start,finish}_command_or_die

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 20, 2016, 21:47 UTC
Message-ID: <48970168-ba4b-e125-6624-e4c37b8a7180@kdbg.org>
In-Reply-To: <CAGZ79kYG-veuWNFh6G1g5MiQHBGk2i1qbH_NBtnMS6jFcoWocg@mail.gmail.com>

Am 20.12.2016 um 21:49 schrieb Stefan Beller:

Show 13 quoted lines

> On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> I have to ask, though: Is it so much necessary to report the exact command
>> line in an error message?
>
> Have a look at https://github.com/git/git/blob/master/submodule.c#L1122
>
>     die("Could not run 'git status --porcelain -uall \
>         --ignore-submodules=none' in submodule %s", path);
>
> There are 2 things:
> 1) The error message is not very informative, as your question hints at.
>     I consider changing it to add more meaning. I think the end user
>     would also be interested in "Why did we run this command?".

You don't have to. start_command() and finish_command() report any low-level errors (exec failed, signal received...). If the exit code of the program is non-zero, finish_command() reports nothing because the command *itself* will have written some error message ("working directory dirty", "object database corrupt", "xyz: no such file or directory"...). At this level, it only makes sense to leave a trace in which submodule an error occured. So I think it would be sufficient to just

     die("could not run 'git status' in submodule %s", path);
or
     die("'git status' in submodule %s failed", path);

> 2) We really want to report the correct command line here.

Why? We do not do this anywhere else.

>     Currently that is the case, but if you look at the prior patch that extends
>     the arguments conditionally (and then also uses the same condition
>     to format the error message... that hints at hard to maintain error
>     messages.)

This does not explain why the *complete and detailed* invocation must be reported. I haven't followed this topic at all, so I may be missing some cruical detail. (If you say "it must happen" one more time, then I will believe you, because for me that's simpler than to plough through a flock of submodule topics. ;-)

-- Hannes