Re: [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 20, 2018, 18:18 UTC
Message-ID: <CAPig+cT5jh0y9Rmw0E6ns0k5mSwaxAqdaN8oWCayCE8V+jYZow@mail.gmail.com>
In-Reply-To: <010201624428190c-441272b2-bc7d-42af-82f8-6a805bb27f6c-000000@eu-west-1.amazonses.com>

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

Show 31 quoted lines

> Continue removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
> [...]
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -596,19 +603,24 @@ static int is_empty(const char *s)
> +static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,
> +                            struct strbuf *err)
>  {
>         struct ref_formatting_stack *cur = state->stack;
>         struct if_then_else *if_then_else = NULL;
>
>         if (cur->at_end == if_then_else_handler)
>                 if_then_else = (struct if_then_else *)cur->at_end_data;
> -       if (!if_then_else)
> -               die(_("format: %%(then) atom used without an %%(if) atom"));
> -       if (if_then_else->then_atom_seen)
> -               die(_("format: %%(then) atom used more than once"));
> -       if (if_then_else->else_atom_seen)
> -               die(_("format: %%(then) atom used after %%(else)"));
> +       if (!if_then_else) {
> +               strbuf_addstr(err, _("format: %(then) atom used without an %(if) atom"));
> +               return -1;
> +       } else if (if_then_else->then_atom_seen) {
> +               strbuf_addstr(err, _("format: %(then) atom used more than once"));
> +               return -1;
> +       } else if (if_then_else->else_atom_seen) {
> +               strbuf_addstr(err, _("format: %(then) atom used after %(else)"));
> +               return -1;
> +       }

This pattern of transforming:

    if (cond)
        die("...");

to:

    if (cond) {
        strbuf_add*(...);
        return -1;
    }

is repeated many, many times throughout this patch series and makes for quite noisy diffs. Such repetition and noise suggests that this patch series (and other similar ones) could benefit from a convenience function similar to the existing error() function. For instance:

    int strbuf_error(struct strbuf *buf, const char *fmt, ...);

which appends the formatted message to 'buf' and unconditionally returns -1. Thus, the above transformation simplifies to:

    if (cond)
        return strbuf_error(&err, "...", ...);

which makes for a much less noisy diff, thus is easier to review.

A new patch introducing such a function at the head of this patch series might be welcome.