On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya
<olyatelezhn...@gmail.com> wrote:
> Continue removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
> [...]
> Signed-off-by: Olga Telezhnaia <olyatelezhn...@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.
