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

[Stefan Beller <sbeller@google.com>]

On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.12.2016 um 20:23 schrieb Stefan Beller:
>>
>> In a reroll I'll drop this patch and instead introduce
>> a function `char *get_child_command_line(struct child_process*);`, which
>> a caller can call before calling finish_command and then use the
>> resulting string
>> to assemble an error message without lego.
>
>
> That sounds a lot better, thank you. Note though, that the function would
> have to be called before start_command(), because when it fails, it would be
> too late.

Yes, the pattern to use it would be

    // first assemble the child process struct with conditions

    char *cmdline = get_child_command_line(&child)

    if (start_command(&child))
        // use cmdline here for error reporting.


>
> 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?".
2) We really want to report the correct command line here.
    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.)

So the new proposed function really only addresses 2) here.

> If someone is interested in which command failed,
> it would be "sufficient" to run the command under GIT_TRACE=2.
>

Yes, while at it, I may just move up the error reporting to the builtin
command giving a higher level message.

Thanks,
Stefan