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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 20, 2018, 18:19 UTC
Message-ID: <CAPig+cSxGgG=1k95j+n4DZHBdtN++7F6_U5fu0gOcjwwPQ5GZw@mail.gmail.com>
In-Reply-To: <010201624428199f-b4b3d8ce-222f-4966-9171-7fcf932ca220-000000@eu-west-1.amazonses.com>

On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:

> ref-filter: get_ref_atom_value() error handling

This doesn't tell us much about what this patch is doing. Perhaps a better subject would be:

    ref-filter: libify get_ref_atom_value()

Show 29 quoted lines

> Finish removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
>
> Change the signature of get_ref_atom_value() and underlying functions
> by adding return value and strbuf parameter for error message.
> Return value equals 0 upon success and -1 upon failure (there
> could be more error codes further if necessary).
> Upon failure, error message is appended to the strbuf.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> +                      int deref, struct object **obj, struct strbuf *err)
>  {
>         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);
> +               if (!eaten)
> +                       free(buf);

Since we are inside the 'if (!buf)' conditional, we _know_ that 'buf' is NULL, therefore this free(buff) is unnecessary.

Show 14 quoted lines

> +               return -1;
> +       }
> +       if (!*obj) {
> +               strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> +                           oid_to_hex(oid), ref->refname);
> +               if (!eaten)
> +                       free(buf);
> +               return -1;
> +       }
>         grab_values(ref->value, deref, *obj, buf, size);
>         if (!eaten)
>                 free(buf);
> +       return 0;
>  }

Overall, with the need for resource cleanup, this function becomes unusually noisy after this change. It could be tamed by doing something like this:

    int ret = 0;
    void *buf = get_obj(oid, obj, &size, &eaten);
    if (!buf)
        ret = strbuf_error(_("missing object %s for %s"),
            oid_to_hex(oid), ref->refname);
    else if (!*obj)
        ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
            oid_to_hex(oid), ref->refname);
    else
        grab_values(ref->value, deref, *obj, buf, size);
   if (!eaten)
        free(buf);
    return ret;