Re: [PATCH 2/5 v2] unpack_trees: group errors by type

[Diane Gasselin <diane.gasselin@ensimag.imag.fr>]

Le 15 juin 2010 14:58, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> 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/
>