Re: [PATCH] Make sure show_all_errors when using porcelain error messages

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 1, 2010, 17:04 UTC
Message-ID: <vpq39ttxumz.fsf@bauges.imag.fr>
In-Reply-To: <7vsk1t7bn2.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> opts.show_all_errors controls the number of errors that can be displayed,
>> and set_porcelain_error_msgs gives the format. But the formatting would
>> be incorrect if set_porcelain_error_msgs is called without setting
>> opts.show_all_errors.
>
> That makes it sound like a design bug of set_porcelain_error_msgs(), in
> that the caller _cannot_ choose to stop at the first error if it wants to
> use friendlier message than the plumbing one.

You're right that the caller cannot stop at the first error and get friendly message, but I don't think this is a bug. As a user, I don't see any reason not to want the complete list. We kept the "show_all_errors = false" behavior to allow the slight performance improvement of stopping at the first error, which may (but I'm not even sure) benefit to scripts.

Show 5 quoted lines

> But if that is (and I do not think it is such a bad thing if we decide
> that Porcelains have no option of stopping at the first error) then we
> should set opts.show_all_errors in the set_porcelain_error_msgs()
> function, and probably should rename the function to something
> saner.

Yes (actually, I send a quick patch to make sure this was the problem of the OP, but the intention was to have a better one when I have time. I forgot to add RFC in the title).

Show 6 quoted lines

> How about making it
>
> 	int setup_unpack_trees_porcelain(struct unpack_trees_options *opt,
> 						const char *cmd);
>
> and possibly moving it from merge-recursive.c to unpack-trees.c?

Sounds good, yes. Will do (probably tomorrow).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/