git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

From
Оля Тележная <olyatelezhnaya@gmail.com>
Date
Mar 16, 2018, 07:17 UTC
Message-ID
<CAL21BmkipcdT6M38PDJ6=kk_E5F8T_OmYOF_N097UbmzbnjESA@mail.gmail.com>
In-Reply-To
<20180315204709.1900787-1-martin.agren@gmail.com>
2018-03-15 23:47 GMT+03:00 Martin Ågren <martin.agren@gmail.com>:
> I skimmed the first four patches of this v2. It seems that patches 1 and
> 4 are identical to v2. Patches 2 and 3 have very straightforward changes
> based on my earlier comments. Let's see what this patch is about. :-)
Yes, you are right.
Show 55 quoted lines
>
> On 14 March 2018 at 20:04, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:
>> Finish removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>>
>> Change the signature of get_ref_atom_value() and underlying functions
>> by adding return value and strbuf parameter for error message.
>>
>> It's important to mention that grab_objectname() returned 1 if
>> it gets objectname atom and 0 otherwise. Now this logic changed:
>> we return 0 if we have no error, -1 otherwise. If someone needs to
>> know whether it's objectname atom or not, he/she could use
>> starts_with() function. It duplicates this checking but it does not
>> sound like a really big overhead.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> ---
>>  ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 69 insertions(+), 40 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 62ea4adcd0ff1..3f0c3924273d5 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned
>>  }
>>
>>  static int grab_objectname(const char *name, const unsigned char *sha1,
>> -                          struct atom_value *v, struct used_atom *atom)
>> +                          struct atom_value *v, struct used_atom *atom,
>> +                          struct strbuf *err)
>>  {
>>         if (starts_with(name, "objectname")) {
>>                 if (atom->u.objectname.option == O_SHORT) {
>>                         v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>> -                       return 1;
>>                 } else if (atom->u.objectname.option == O_FULL) {
>>                         v->s = xstrdup(sha1_to_hex(sha1));
>> -                       return 1;
>>                 } else if (atom->u.objectname.option == O_LENGTH) {
>>                         v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
>> -                       return 1;
>> -               } else
>> -                       die("BUG: unknown %%(objectname) option");
>> +               } else {
>> +                       strbuf_addstr(err, "BUG: unknown %(objectname) option");
>> +                       return -1;
>> +               }
>>         }
>>         return 0;
>>  }
>
> This is interesting. This die() is never ever supposed to actually
> trigger, except to allow a developer adding some new O_xxx-value to
> quickly notice that they have forgotten to add code here.
Oh, cool, so I can revert this change, OK.
Show 21 quoted lines
>
>>  /* See grab_values */
>> -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
>> +static int grab_common_values(struct atom_value *val, int deref, struct object *obj,
>> +                             void *buf, unsigned long sz, struct strbuf *err)
>>  {
>>         int i;
>>
>> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
>>                         v->s = xstrfmt("%lu", sz);
>>                 }
>>                 else if (deref)
>> -                       grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
>> +                       if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err))
>> +                               return -1;
>>         }
>> +       return 0;
>>  }
>
> So if that conversion I commented on above had not happened, this would
> not have been necessary. Let's read on...

Of course, I will check and also revert functions that were touched only because of other functions.

Show 32 quoted lines
>
>>  /* See grab_values */
>> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)
>>   * pointed at by the ref itself; otherwise it is the object the
>>   * ref (which is a tag) refers to.
>>   */
>> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
>> +static int grab_values(struct atom_value *val, int deref, struct object *obj,
>> +                      void *buf, unsigned long sz, struct strbuf *err)
>>  {
>> -       grab_common_values(val, deref, obj, buf, sz);
>> +       if (grab_common_values(val, deref, obj, buf, sz, err))
>> +               return -1;
>>         switch (obj->type) {
>>         case OBJ_TAG:
>>                 grab_tag_values(val, deref, obj, buf, sz);
>> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
>>                 /* grab_blob_values(val, deref, obj, buf, sz); */
>>                 break;
>>         default:
>> -               die("Eh?  Object of type %d?", obj->type);
>> +               strbuf_addf(err, "Eh?  Object of type %d?", obj->type);
>> +               return -1;
>>         }
>> +       return 0;
>>  }
>
> This seems similar. The string here is quite sloppy, and I do not
> believe that the author intended this to be user-visible. I believe this
> is more like a very short way of saying "how could we possibly get
> here??". It could also be written as die("BUG: unknown object type %d",
> obj->type), or even better: BUG(...).
OK, thanks!
Show 44 quoted lines
>
>>  static inline char *copy_advance(char *dst, const char *src)
>> @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)
>>                 return refname;
>>  }
>>
>> -static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> -                                   struct branch *branch, const char **s)
>> +static int fill_remote_ref_details(struct used_atom *atom, const char *refname,
>> +                                  struct branch *branch, const char **s,
>> +                                  struct strbuf *err)
>>  {
>>         int num_ours, num_theirs;
>>         if (atom->u.remote_ref.option == RR_REF)
>> @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>>         } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
>>                 if (stat_tracking_info(branch, &num_ours, &num_theirs,
>>                                        NULL, AHEAD_BEHIND_FULL) < 0)
>> -                       return;
>> +                       return 0;
>>
>>                 if (!num_ours && !num_theirs)
>>                         *s = "=";
>> @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
>>                         *s = xstrdup(merge);
>>                 else
>>                         *s = "";
>> -       } else
>> -               die("BUG: unhandled RR_* enum");
>> +       } else {
>> +               strbuf_addstr(err, "BUG: unhandled RR_* enum");
>> +               return -1;
>> +       }
>> +       return 0;
>>  }
>
> This one too.. I start to think it is overkill to wire through these
> strbufs just to collect messages that should never ever be produced.  It
> almost seems to me like if 1) we want to collect errors using strbufs,
> and 2) we want to use BUG() for these sorts of assertions, then 3) we
> will be wiring error-strbufs through more or less all of our code. I am
> exaggerating, but there is something to it: A small change deep down a
> callstack where you want to add a BUG() "to be safe", and you might need
> to wire a strbuf all the way through.

I absolutely agree that we should not touch functions that produce errors for Git developers. It still means that sometimes we need to wire strbufs, but I will revert some of them. It's enough to handle only errors for users.

Show 54 quoted lines
>
> According to d8193743e0 (usage.c: add BUG() function, 2017-05-12), a
> good thing about BUG() is that we can get a core dump, a filename and a
> line number. That opportunity gets lost if we do these sort of
> transformations. Of course, these were only die("BUG: "), not BUG(), but
> my point is that they should perhaps have been BUG().
>
>>  char *get_head_description(void)
>> @@ -1447,28 +1458,33 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>>         return show_ref(&atom->u.refname, ref->refname);
>>  }
>>
>> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,
>> -                      int deref, struct object **obj)
>> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
>> +                      int deref, struct object **obj, struct strbuf *err)
>>  {
>>         int eaten;
>>         unsigned long size;
>>         void *buf = get_obj(oid, obj, &size, &eaten);
>> -       if (!buf)
>> -               die(_("missing object %s for %s"),
>> -                   oid_to_hex(oid), ref->refname);
>> -       if (!*obj)
>> -               die(_("parse_object_buffer failed on %s for %s"),
>> -                   oid_to_hex(oid), ref->refname);
>> -
>> -       grab_values(ref->value, deref, *obj, buf, size);
>> +       if (!buf) {
>> +               strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
>> +                           ref->refname);
>> +               return -1;
>> +       }
>> +       if (!*obj) {
>> +               strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
>> +                           oid_to_hex(oid), ref->refname);
>> +               return -1;
>> +       }
>> +       if (grab_values(ref->value, deref, *obj, buf, size, err))
>> +               return -1;
>>         if (!eaten)
>>                 free(buf);
>> +       return 0;
>>  }
>
> These are "real" errors and yield several more changes in the remainder.
> Ignoring those BUG-type messages at the beginning of this patch would
> give a patch like the one below. Maybe that would be a bit less
> intrusive for the same gain.
>
> Thanks for working on this. I feel that your patches are really
> interesting. They open up many possibilities for philosophical exercises
> about how errors should be collected and reported. ;-) I would be
> interested in knowing your thoughts, and others'.

Thank you! I was also thinking about other ways how to handle errors. I haven't invented anything new, so at least I decided to make these changes (for now).

Show 131 quoted lines
>
> Martin
>
> ---
>  ref-filter.c | 51 ++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 19 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 62ea4adcd0..e41505b3c0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1447,28 +1447,32 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>         return show_ref(&atom->u.refname, ref->refname);
>  }
>
> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,
> -                      int deref, struct object **obj)
> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> +                      int deref, struct object **obj, struct strbuf *err)
>  {
>         int eaten;
>         unsigned long size;
>         void *buf = get_obj(oid, obj, &size, &eaten);
> -       if (!buf)
> -               die(_("missing object %s for %s"),
> -                   oid_to_hex(oid), ref->refname);
> -       if (!*obj)
> -               die(_("parse_object_buffer failed on %s for %s"),
> -                   oid_to_hex(oid), ref->refname);
> -
> +       if (!buf) {
> +               strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
> +                           ref->refname);
> +               return -1;
> +       }
> +       if (!*obj) {
> +               strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> +                           oid_to_hex(oid), ref->refname);
> +               return -1;
> +       }
>         grab_values(ref->value, deref, *obj, buf, size);
>         if (!eaten)
>                 free(buf);
> +       return 0;
>  }
>
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> -static void populate_value(struct ref_array_item *ref)
> +static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  {
>         struct object *obj;
>         int i;
> @@ -1590,16 +1594,17 @@ static void populate_value(struct ref_array_item *ref)
>                         break;
>         }
>         if (used_atom_cnt <= i)
> -               return;
> +               return 0;
>
> -       get_object(ref, &ref->objectname, 0, &obj);
> +       if (get_object(ref, &ref->objectname, 0, &obj, err))
> +               return -1;
>
>         /*
>          * If there is no atom that wants to know about tagged
>          * object, we are done.
>          */
>         if (!need_tagged || (obj->type != OBJ_TAG))
> -               return;
> +               return 0;
>
>         /*
>          * If it is a tag object, see if we use a value that derefs
> @@ -1613,20 +1618,23 @@ static void populate_value(struct ref_array_item *ref)
>          * is not consistent with what deref_tag() does
>          * which peels the onion to the core.
>          */
> -       get_object(ref, tagged, 1, &obj);
> +       return get_object(ref, tagged, 1, &obj, err);
>  }
>
>  /*
>   * Given a ref, return the value for the atom.  This lazily gets value
>   * out of the object by calling populate value.
>   */
> -static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
> +static int get_ref_atom_value(struct ref_array_item *ref, int atom,
> +                             struct atom_value **v, struct strbuf *err)
>  {
>         if (!ref->value) {
> -               populate_value(ref);
> +               if (populate_value(ref, err))
> +                       return -1;
>                 fill_missing_values(ref->value);
>         }
>         *v = &ref->value[atom];
> +       return 0;
>  }
>
>  /*
> @@ -2150,9 +2158,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
>         int cmp;
>         cmp_type cmp_type = used_atom[s->atom].type;
>         int (*cmp_fn)(const char *, const char *);
> +       struct strbuf err = STRBUF_INIT;
>
> -       get_ref_atom_value(a, s->atom, &va);
> -       get_ref_atom_value(b, s->atom, &vb);
> +       if (get_ref_atom_value(a, s->atom, &va, &err))
> +               die("%s", err.buf);
> +       if (get_ref_atom_value(b, s->atom, &vb, &err))
> +               die("%s", err.buf);
> +       strbuf_release(&err);
>         cmp_fn = s->ignore_case ? strcasecmp : strcmp;
>         if (s->version)
>                 cmp = versioncmp(va->s, vb->s);
> @@ -2231,7 +2243,8 @@ int format_ref_array_item(struct ref_array_item *info,
>                         append_literal(cp, sp, &state);
>                 if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))
>                         return -1;
> -               get_ref_atom_value(info, pos, &atomv);
> +               if (get_ref_atom_value(info, pos, &atomv, error_buf))
> +                       return -1;
>                 if (atomv->handler(atomv, &state, error_buf))
>                         return -1;
>         }
> --
> 2.16.2.246.ga4ee44448f
>
Previous: Junio C HamanoNext: Olga Telezhnaya
Message 7 of 30 in “ref-filter: start adding strbufs with errors”
  1. 1/5 ref-filter: start adding strbufs with errorsOlga Telezhnaya, Mar 14, 2018
  2. 5/5 ref-filter: get_ref_atom_value() error handlingOlga Telezhnaya, Mar 14, 2018
  3. Martin ÅgrenMar 15, 2018
  4. Eric SunshineMar 15, 2018
  5. Оля ТележнаяMar 16, 2018
  6. Junio C HamanoMar 15, 2018
  7. Оля ТележнаяMar 16, 2018
  8. 3/5 ref-filter: change parsing function error handlingOlga Telezhnaya, Mar 14, 2018
  9. Junio C HamanoMar 15, 2018
  10. Оля ТележнаяMar 16, 2018
  11. 4/5 ref-filter: add return value to parsersOlga Telezhnaya, Mar 14, 2018
  12. 2/5 ref-filter: add return value && strbuf to handlersOlga Telezhnaya, Mar 14, 2018
  13. 1/5 ref-filter: start adding strbufs with errorsOlga Telezhnaya, Mar 19, 2018
  14. 3/5 ref-filter: change parsing function error handlingOlga Telezhnaya, Mar 19, 2018
  15. Junio C HamanoMar 19, 2018
  16. 5/5 ref-filter: get_ref_atom_value() error handlingOlga Telezhnaya, Mar 19, 2018
  17. 2/5 ref-filter: add return value && strbuf to handlersOlga Telezhnaya, Mar 19, 2018
  18. Junio C HamanoMar 19, 2018
  19. 4/5 ref-filter: add return value to parsersOlga Telezhnaya, Mar 19, 2018
  20. 1/5 ref-filter: start adding strbufs with errorsOlga Telezhnaya, Mar 20, 2018
  21. 5/5 ref-filter: get_ref_atom_value() error handlingOlga Telezhnaya, Mar 20, 2018
  22. Eric SunshineMar 20, 2018
  23. Junio C HamanoMar 20, 2018
  24. Eric SunshineMar 20, 2018
  25. Junio C HamanoMar 21, 2018
  26. Eric SunshineMar 21, 2018
  27. 3/5 ref-filter: change parsing function error handlingOlga Telezhnaya, Mar 20, 2018
  28. 2/5 ref-filter: add return value && strbuf to handlersOlga Telezhnaya, Mar 20, 2018
  29. Eric SunshineMar 20, 2018
  30. 4/5 ref-filter: add return value to parsersOlga Telezhnaya, Mar 20, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.