Re: [PATCH 2/5 v2] unpack_trees: group errors by type
- From
- Diane Gasselin <diane.gasselin@ensimag.imag.fr>
- Date
- Jun 15, 2010, 13:15 UTC
- Message-ID
- <AANLkTin381eyaDabz3-z_8jB05N4CVKGmLOqVOprJMW2@mail.gmail.com>
- In-Reply-To
- <vpqljagzc39.fsf@bauges.imag.fr>
Le 15 juin 2010 14:58, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
Show 76 quoted lines
> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
>
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>> }
>>
>> /*
>> + * add error messages on path <path> and action <action>
>> + * corresponding to the type <e> with the message <msg>
>> + * indicating if it should be display in porcelain or not
>> + */
>> +static int add_rejected_path(struct unpack_trees_options *o,
>> + enum unpack_trees_error e,
>> + const char *path,
>> + const char *action,
>> + int porcelain,
>> + const char *msg)
>> +{
>> + struct rejected_paths_list *newentry;
>> + struct rejected_paths **rp;
>> + /*
>> + * simply display the given error message if in plumbing mode
>> + */
>> + if (!porcelain)
>> + o->show_all_errors = 0;
>> + if (!o->show_all_errors)
>> + return error(msg, path, action);
>
> I don't fully understand what you're doing with show_all_errors and
> porcelain here. From the caller, "porcelain" is true iff the
> corresponding error message has been set in o. But if you can infer
> whether you're in porcelain from the error messages, why do you need
> show_all_errors in addition?
>
>> static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)
>> {
>> - return error(ERRORMSG(o, would_overwrite), ce->name);
>> + return add_rejected_path(o, would_overwrite, ce->name, NULL,
>> + (o && (o)->msgs.would_overwrite),
>
> Parenthesis around (o) are distracting and useless. I guess you
> copy-pasted from a macro (for which parentheses should definitely be
> used in case the macro is called on an arbitrary expression).
>
>> @@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,
>> }
>> if (errno == ENOENT)
>> return 0;
>> - return o->gently ? -1 :
>> - error(error_msg, ce->name);
>> + if (error == sparse_not_uptodate_file)
>> + return o->gently ? -1 :
>> + add_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,
>> + (o && (o)->msgs.sparse_not_uptodate_file),
>> + ERRORMSG(o, sparse_not_uptodate_file));
>> + else
>> + return o->gently ? -1 :
>> + add_rejected_path(o, not_uptodate_file, ce->name, NULL,
>> + (o && (o)->msgs.not_uptodate_file),
>> + ERRORMSG(o, not_uptodate_file));
>> }
>
> Isn't that a complex way of saying
>
> int porcelain;
> if (error == sparse_not_uptodate_file)
> porcelain = o && o->msgs.sparse_not_uptodate_file;
> else
> porcelain = o && o->msgs.not_uptodate_file;
> return o->gently ? -1 :
> add_rejected_path(o, error, ce->name, NULL,
> porcelain, ERRORMSG(o, error));
>
> ?
>The problem is that "error" is an enum unpack_trees_error, and ERRORMSG takes the name of the field from unpack_trees_error_msgs. If I try to do ERRORMSG(o, error), the compilator would say that the "error" is not a field of unpack_trees_error_msgs.
> Also, I'm not sure I understand why you're attaching the error message > string to each rejected_paths entry. Wouldn't it be more sensible to > use o->msg in display_error_msgs() instead? >
In display_error_msgs(), I cannot access o->msg because I would not know which error I am treating. In the same way as previously, I cannot use the enum unpack_trees_error to access it.
I know it makes the code a bit "heavy" but I did not see a better way to do it.
> -- > Matthieu Moy > http://www-verimag.imag.fr/~moy/ >