From: Diane Gasselin Date: Tue, 15 Jun 2010 13:15:36 GMT Subject: Re: [PATCH 2/5 v2] unpack_trees: group errors by type Message-ID: In-Reply-To: Le 15 juin 2010 14:58, Matthieu Moy a écrit : > Diane Gasselin 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 and action >> + * corresponding to the type with the message >> + * 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/ >