Le 15 juin 2010 14:58, Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> a écrit :
> Diane Gasselin <diane.gasselin@xxxxxxxxxxxxxxx> 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/
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html