Re: [RFC 2/4] ref-filter: add return value && strbuf to handlers

From: Martin Ågren <martin.agren@gmail.com>
Date: Mar 13, 2018, 19:13 UTC
Message-ID: <CAN0heSokC8twry_4_2N42tswZ_vhrDp8svo-MUk-dg-HyQKtQA@mail.gmail.com>
In-Reply-To: <010201621edc9811-4197d53b-2ebd-4543-b316-22b58804c6b4-000000@eu-west-1.amazonses.com>

On 13 March 2018 at 11:16, Olga Telezhnaya <olyatelezhnaya@gmail.com> wrote:

Show 12 quoted lines

> -static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
> +static int append_atom(struct atom_value *v, struct ref_formatting_state *state,
> +                      struct strbuf *err)
>  {
>         /*
>          * Quote formatting is only done when the stack has a single
> @@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state
>                 quote_formatting(&state->stack->output, v->s, state->quote_style);
>         else
>                 strbuf_addstr(&state->stack->output, v->s);
> +       return 0;
>  }

Maybe "unused_err" instead of "err", to document that we are aware that "err" is not being used and that it is ok. Something similar is done in builtin/difftool.c.

Show 25 quoted lines

> -static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +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;
> +       }

I slowly start to wonder if we want a function (or macro) error_strbuf_addstr(), which returns -1. That could probably wait, though.

Martin