On Tue, Dec 20, 2016 at 12:12 PM, Johannes Sixt <j6t@xxxxxxxx> 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