# [RFC 3/4] ref-filter: change parsing function error handling 9 messages from 2018-03-13 to 2018-03-14. Participants: Olga Telezhnaya, Martin Ågren, Оля Тележная. Thread: https://gitlist.dev/t/48030 ## Olga Telezhnaya, 2018-03-13 10:16 Subject: [RFC 3/4] ref-filter: change parsing function error handling Message-ID: <010201621edc982f-92123a33-386b-4b94-be0c-f793856e6c16-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201621edc982f-92123a33-386b-4b94-be0c-f793856e6c16-000000%40eu-west-1.amazonses.com In-Reply-To: <010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000@eu-west-1.amazonses.com> ``` Continue removing any printing from ref-filter formatting logic, so that it could be more general. Change the signature of parse_ref_filter_atom() by changing return value, adding previous return value to function parameter and also adding strbuf parameter for error message. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 07bedc636398c..e146215bf1e64 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -397,7 +397,8 @@ struct atom_value { * Used to parse format string and sort specifiers */ static int parse_ref_filter_atom(const struct ref_format *format, - const char *atom, const char *ep) + const char *atom, const char *ep, int *res, + struct strbuf *err) { const char *sp; const char *arg; @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format *format, sp = atom; if (*sp == '*' && sp < ep) sp++; /* deref */ - if (ep <= sp) - die(_("malformed field name: %.*s"), (int)(ep-atom), atom); + if (ep <= sp) { + strbuf_addf(err, _("malformed field name: %.*s"), (int)(ep-atom), atom); + return -1; + } /* Do we have the atom already used elsewhere? */ for (i = 0; i < used_atom_cnt; i++) { int len = strlen(used_atom[i].name); - if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) - return i; + if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) { + *res = i; + return 0; + } } /* @@ -432,8 +437,10 @@ static int parse_ref_filter_atom(const struct ref_format *format, break; } - if (ARRAY_SIZE(valid_atom) <= i) - die(_("unknown field name: %.*s"), (int)(ep-atom), atom); + if (ARRAY_SIZE(valid_atom) <= i) { + strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), atom); + return -1; + } /* Add it in, including the deref prefix */ at = used_atom_cnt; @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format *format, need_tagged = 1; if (!strcmp(valid_atom[i].name, "symref")) need_symref = 1; - return at; + *res = at; + return 0; } static void quote_formatting(struct strbuf *s, const char *str, int quote_style) @@ -725,17 +733,20 @@ int verify_ref_format(struct ref_format *format) format->need_color_reset_at_eol = 0; for (cp = format->format; *cp && (sp = find_next(cp)); ) { + struct strbuf err = STRBUF_INIT; const char *color, *ep = strchr(sp, ')'); int at; if (!ep) return error(_("malformed format string %s"), sp); /* sp points at "%(" and ep points at the closing ")" */ - at = parse_ref_filter_atom(format, sp + 2, ep); + if (parse_ref_filter_atom(format, sp + 2, ep, &at, &err)) + die("%s", err.buf); cp = ep + 1; if (skip_prefix(used_atom[at].name, "color:", &color)) format->need_color_reset_at_eol = !!strcmp(color, "reset"); + strbuf_release(&err); } if (format->need_color_reset_at_eol && !want_color(format->use_color)) format->need_color_reset_at_eol = 0; @@ -2154,15 +2165,18 @@ int format_ref_array_item(struct ref_array_item *info, for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) { struct atom_value *atomv; + struct strbuf err = STRBUF_INIT; + int pos; ep = strchr(sp, ')'); if (cp < sp) append_literal(cp, sp, &state); - get_ref_atom_value(info, - parse_ref_filter_atom(format, sp + 2, ep), - &atomv); + if (parse_ref_filter_atom(format, sp + 2, ep, &pos, &err)) + return -1; + get_ref_atom_value(info, pos, &atomv); if (atomv->handler(atomv, &state, error_buf)) return -1; + strbuf_release(&err); } if (*cp) { sp = cp + strlen(cp); @@ -2215,7 +2229,12 @@ static int parse_sorting_atom(const char *atom) */ struct ref_format dummy = REF_FORMAT_INIT; const char *end = atom + strlen(atom); - return parse_ref_filter_atom(&dummy, atom, end); + struct strbuf err = STRBUF_INIT; + int res; + if (parse_ref_filter_atom(&dummy, atom, end, &res, &err)) + die("%s", err.buf); + strbuf_release(&err); + return res; } /* If no sorting option is given, use refname to sort as default */ -- https://github.com/git/git/pull/466 ``` ## Olga Telezhnaya, 2018-03-13 10:16 Subject: [RFC 1/4] ref-filter: start adding strbufs with errors Message-ID: <010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000%40eu-west-1.amazonses.com ``` This is a first step in removing any printing from ref-filter formatting logic, so that it could be more general. Everything would be the same for show_ref_array_item() users. But, if you want to deal with errors by your own, you could invoke format_ref_array_item(). It means that you need to print everything (the result and errors) on your side. This commit changes signature of format_ref_array_item() by adding return value and strbuf parameter for errors, and fixes its callers. Signed-off-by: Olga Telezhnaia --- builtin/branch.c | 7 +++++-- ref-filter.c | 17 ++++++++++++----- ref-filter.h | 7 ++++--- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 8dcc2ed058be6..f86709ca42d5e 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin struct ref_array array; int maxwidth = 0; const char *remote_prefix = ""; - struct strbuf out = STRBUF_INIT; char *to_free = NULL; /* @@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin ref_array_sort(sorting, &array); for (i = 0; i < array.nr; i++) { - format_ref_array_item(array.items[i], format, &out); + struct strbuf out = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + if (format_ref_array_item(array.items[i], format, &out, &err)) + die("%s", err.buf); if (column_active(colopts)) { assert(!filter->verbose && "--column and --verbose are incompatible"); /* format to a string_list to let print_columns() do its job */ @@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin fwrite(out.buf, 1, out.len, stdout); putchar('\n'); } + strbuf_release(&err); strbuf_release(&out); } diff --git a/ref-filter.c b/ref-filter.c index 45fc56216aaa8..54fae00bdd410 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting } } -void format_ref_array_item(struct ref_array_item *info, +int format_ref_array_item(struct ref_array_item *info, const struct ref_format *format, - struct strbuf *final_buf) + struct strbuf *final_buf, + struct strbuf *error_buf) { const char *cp, *sp, *ep; struct ref_formatting_state state = REF_FORMATTING_STATE_INIT; @@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info, resetv.s = GIT_COLOR_RESET; append_atom(&resetv, &state); } - if (state.stack->prev) - die(_("format: %%(end) atom missing")); + if (state.stack->prev) { + strbuf_addstr(error_buf, _("format: %(end) atom missing")); + return -1; + } strbuf_addbuf(final_buf, &state.stack->output); pop_stack_element(&state.stack); + return 0; } void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format) { struct strbuf final_buf = STRBUF_INIT; + struct strbuf error_buf = STRBUF_INIT; - format_ref_array_item(info, format, &final_buf); + if (format_ref_array_item(info, format, &final_buf, &error_buf)) + die("%s", error_buf.buf); fwrite(final_buf.buf, 1, final_buf.len, stdout); + strbuf_release(&error_buf); strbuf_release(&final_buf); putchar('\n'); } diff --git a/ref-filter.h b/ref-filter.h index 0d98342b34319..e13f8e6f8721a 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format); /* Sort the given ref_array as per the ref_sorting provided */ void ref_array_sort(struct ref_sorting *sort, struct ref_array *array); /* Based on the given format and quote_style, fill the strbuf */ -void format_ref_array_item(struct ref_array_item *info, - const struct ref_format *format, - struct strbuf *final_buf); +int format_ref_array_item(struct ref_array_item *info, + const struct ref_format *format, + struct strbuf *final_buf, + struct strbuf *error_buf); /* Print the ref using the given format and quote_style */ void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format); /* Parse a single sort specifier and add it to the list */ -- https://github.com/git/git/pull/466 ``` ## Olga Telezhnaya, 2018-03-13 10:16 Subject: [RFC 4/4] ref-filter: add return value to parsers Message-ID: <010201621edc9828-01c57fe9-023c-4932-879c-19eea9fc2fdc-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201621edc9828-01c57fe9-023c-4932-879c-19eea9fc2fdc-000000%40eu-west-1.amazonses.com In-Reply-To: <010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000@eu-west-1.amazonses.com> ``` Continue removing any printing from ref-filter formatting logic, so that it could be more general. Change the signature of parsers by adding return value and strbuf parameter for error message. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 177 +++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 118 insertions(+), 59 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index e146215bf1e64..06eb95e7c2c07 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -101,22 +101,28 @@ static struct used_atom { } *used_atom; static int used_atom_cnt, need_tagged, need_symref; -static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value) +static int color_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *color_value, struct strbuf *err) { - if (!color_value) - die(_("expected format: %%(color:)")); - if (color_parse(color_value, atom->u.color) < 0) - die(_("unrecognized color: %%(color:%s)"), color_value); + if (!color_value) { + strbuf_addstr(err, _("expected format: %(color:)")); + return -1; + } + if (color_parse(color_value, atom->u.color) < 0) { + strbuf_addf(err, _("unrecognized color: %%(color:%s)"), color_value); + return -1; + } /* * We check this after we've parsed the color, which lets us complain * about syntactically bogus color names even if they won't be used. */ if (!want_color(format->use_color)) color_parse("", atom->u.color); + return 0; } -static void refname_atom_parser_internal(struct refname_atom *atom, - const char *arg, const char *name) +static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg, + const char *name, struct strbuf *err) { if (!arg) atom->option = R_NORMAL; @@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct refname_atom *atom, else if (skip_prefix(arg, "lstrip=", &arg) || skip_prefix(arg, "strip=", &arg)) { atom->option = R_LSTRIP; - if (strtol_i(arg, 10, &atom->lstrip)) - die(_("Integer value expected refname:lstrip=%s"), arg); + if (strtol_i(arg, 10, &atom->lstrip)) { + strbuf_addf(err, _("Integer value expected refname:lstrip=%s"), arg); + return -1; + } } else if (skip_prefix(arg, "rstrip=", &arg)) { atom->option = R_RSTRIP; - if (strtol_i(arg, 10, &atom->rstrip)) - die(_("Integer value expected refname:rstrip=%s"), arg); - } else - die(_("unrecognized %%(%s) argument: %s"), name, arg); + if (strtol_i(arg, 10, &atom->rstrip)) { + strbuf_addf(err, _("Integer value expected refname:rstrip=%s"), arg); + return -1; + } + } else { + strbuf_addf(err, _("unrecognized %%(%s) argument: %s"), name, arg); + return -1; + } + return 0; } -static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { struct string_list params = STRING_LIST_INIT_DUP; int i; @@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_ if (!arg) { atom->u.remote_ref.option = RR_REF; - refname_atom_parser_internal(&atom->u.remote_ref.refname, - arg, atom->name); - return; + return refname_atom_parser_internal(&atom->u.remote_ref.refname, + arg, atom->name, err); } atom->u.remote_ref.nobracket = 0; @@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_ atom->u.remote_ref.push_remote = 1; } else { atom->u.remote_ref.option = RR_REF; - refname_atom_parser_internal(&atom->u.remote_ref.refname, - arg, atom->name); + if (refname_atom_parser_internal(&atom->u.remote_ref.refname, + arg, atom->name, err)) + return -1; } } string_list_clear(¶ms, 0); + return 0; } -static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int body_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { - if (arg) - die(_("%%(body) does not take arguments")); + if (arg) { + strbuf_addstr(err, _("%(body) does not take arguments")); + return -1; + } atom->u.contents.option = C_BODY_DEP; + return 0; } -static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { - if (arg) - die(_("%%(subject) does not take arguments")); + if (arg) { + strbuf_addstr(err, _("%(subject) does not take arguments")); + return -1; + } atom->u.contents.option = C_SUB; + return 0; } -static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { struct string_list params = STRING_LIST_INIT_DUP; int i; @@ -205,15 +229,19 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at atom->u.contents.trailer_opts.unfold = 1; else if (!strcmp(s, "only")) atom->u.contents.trailer_opts.only_trailers = 1; - else - die(_("unknown %%(trailers) argument: %s"), s); + else { + strbuf_addf(err, _("unknown %%(trailers) argument: %s"), s); + return -1; + } } } atom->u.contents.option = C_TRAILERS; string_list_clear(¶ms, 0); + return 0; } -static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { if (!arg) atom->u.contents.option = C_BARE; @@ -225,16 +253,23 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at atom->u.contents.option = C_SUB; else if (skip_prefix(arg, "trailers", &arg)) { skip_prefix(arg, ":", &arg); - trailers_atom_parser(format, atom, *arg ? arg : NULL); + if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err)) + return -1; } else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; - if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) - die(_("positive value expected contents:lines=%s"), arg); - } else - die(_("unrecognized %%(contents) argument: %s"), arg); + if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) { + strbuf_addf(err, _("positive value expected contents:lines=%s"), arg); + return -1; + } + } else { + strbuf_addf(err, _("unrecognized %%(contents) argument: %s"), arg); + return -1; + } + return 0; } -static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { if (!arg) atom->u.objectname.option = O_FULL; @@ -243,17 +278,23 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_ else if (skip_prefix(arg, "short=", &arg)) { atom->u.objectname.option = O_LENGTH; if (strtoul_ui(arg, 10, &atom->u.objectname.length) || - atom->u.objectname.length == 0) - die(_("positive value expected objectname:short=%s"), arg); + atom->u.objectname.length == 0) { + strbuf_addf(err, _("positive value expected objectname:short=%s"), arg); + return -1; + } if (atom->u.objectname.length < MINIMUM_ABBREV) atom->u.objectname.length = MINIMUM_ABBREV; - } else - die(_("unrecognized %%(objectname) argument: %s"), arg); + } else { + strbuf_addf(err, _("unrecognized %%(objectname) argument: %s"), arg); + return -1; + } + return 0; } -static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { - refname_atom_parser_internal(&atom->u.refname, arg, atom->name); + return refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err); } static align_type parse_align_position(const char *s) @@ -267,15 +308,18 @@ static align_type parse_align_position(const char *s) return -1; } -static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int align_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { struct align *align = &atom->u.align; struct string_list params = STRING_LIST_INIT_DUP; int i; unsigned int width = ~0U; - if (!arg) - die(_("expected format: %%(align:,)")); + if (!arg) { + strbuf_addstr(err, _("expected format: %(align:,)")); + return -1; + } align->position = ALIGN_LEFT; @@ -286,49 +330,64 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom if (skip_prefix(s, "position=", &s)) { position = parse_align_position(s); - if (position < 0) - die(_("unrecognized position:%s"), s); + if (position < 0) { + strbuf_addf(err, _("unrecognized position:%s"), s); + return -1; + } align->position = position; } else if (skip_prefix(s, "width=", &s)) { - if (strtoul_ui(s, 10, &width)) - die(_("unrecognized width:%s"), s); + if (strtoul_ui(s, 10, &width)) { + strbuf_addf(err, _("unrecognized width:%s"), s); + return -1; + } } else if (!strtoul_ui(s, 10, &width)) ; else if ((position = parse_align_position(s)) >= 0) align->position = position; - else - die(_("unrecognized %%(align) argument: %s"), s); + else { + strbuf_addf(err, _("unrecognized %%(align) argument: %s"), s); + return -1; + } } - if (width == ~0U) - die(_("positive width expected with the %%(align) atom")); + if (width == ~0U) { + strbuf_addstr(err, _("positive width expected with the %(align) atom")); + return -1; + } align->width = width; string_list_clear(¶ms, 0); + return 0; } -static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int if_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { if (!arg) { atom->u.if_then_else.cmp_status = COMPARE_NONE; - return; + return 0; } else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.str)) { atom->u.if_then_else.cmp_status = COMPARE_EQUAL; } else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.str)) { atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL; } else { - die(_("unrecognized %%(if) argument: %s"), arg); + strbuf_addf(err, _("unrecognized %%(if) argument: %s"), arg); + return -1; } + return 0; } -static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) +static int head_atom_parser(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) { atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL); + return 0; } static struct { const char *name; cmp_type cmp_type; - void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg); + int (*parser)(const struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err); } valid_atom[] = { { "refname" , FIELD_STR, refname_atom_parser }, { "objecttype" }, @@ -459,8 +518,8 @@ static int parse_ref_filter_atom(const struct ref_format *format, } } memset(&used_atom[at].u, 0, sizeof(used_atom[at].u)); - if (valid_atom[i].parser) - valid_atom[i].parser(format, &used_atom[at], arg); + if (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err)) + return -1; if (*atom == '*') need_tagged = 1; if (!strcmp(valid_atom[i].name, "symref")) -- https://github.com/git/git/pull/466 ``` ## Olga Telezhnaya, 2018-03-13 10:16 Subject: [RFC 2/4] ref-filter: add return value && strbuf to handlers Message-ID: <010201621edc9811-4197d53b-2ebd-4543-b316-22b58804c6b4-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201621edc9811-4197d53b-2ebd-4543-b316-22b58804c6b4-000000%40eu-west-1.amazonses.com In-Reply-To: <010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000@eu-west-1.amazonses.com> ``` Continue removing any printing from ref-filter formatting logic, so that it could be more general. Change the signature of handlers by adding return value and strbuf parameter for errors. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 71 ++++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 54fae00bdd410..07bedc636398c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -387,7 +387,8 @@ struct ref_formatting_state { struct atom_value { const char *s; - void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state); + int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state, + struct strbuf *err); uintmax_t value; /* used for sorting when not FIELD_STR */ struct used_atom *atom; }; @@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style) } } -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; } static void push_stack_element(struct ref_formatting_stack **stack) @@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack) strbuf_release(&s); } -static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state) +static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state, + struct strbuf *err) { struct ref_formatting_stack *new_stack; @@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s new_stack = state->stack; new_stack->at_end = end_align_handler; new_stack->at_end_data = &atomv->atom->u.align; + return 0; } static void if_then_else_handler(struct ref_formatting_stack **stack) @@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack) free(if_then_else); } -static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state) +static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state, + struct strbuf *err) { struct ref_formatting_stack *new_stack; struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1); @@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat new_stack = state->stack; new_stack->at_end = if_then_else_handler; new_stack->at_end_data = if_then_else; + return 0; } static int is_empty(const char *s) @@ -596,19 +603,24 @@ static int is_empty(const char *s) return 1; } -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; + } if_then_else->then_atom_seen = 1; /* * If the 'equals' or 'notequals' attribute is used then @@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st } else if (cur->output.len && !is_empty(cur->output.buf)) if_then_else->condition_satisfied = 1; strbuf_reset(&cur->output); + return 0; } -static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state) +static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state, + struct strbuf *err) { struct ref_formatting_stack *prev = state->stack; struct if_then_else *if_then_else = NULL; if (prev->at_end == if_then_else_handler) if_then_else = (struct if_then_else *)prev->at_end_data; - if (!if_then_else) - die(_("format: %%(else) atom used without an %%(if) atom")); - if (!if_then_else->then_atom_seen) - die(_("format: %%(else) atom used without a %%(then) atom")); - if (if_then_else->else_atom_seen) - die(_("format: %%(else) atom used more than once")); + if (!if_then_else) { + strbuf_addstr(err, _("format: %(else) atom used without an %(if) atom")); + return -1; + } else if (!if_then_else->then_atom_seen) { + strbuf_addstr(err, _("format: %(else) atom used without a %(then) atom")); + return -1; + } else if (if_then_else->else_atom_seen) { + strbuf_addstr(err, _("format: %(else) atom used more than once")); + return -1; + } if_then_else->else_atom_seen = 1; push_stack_element(&state->stack); state->stack->at_end_data = prev->at_end_data; state->stack->at_end = prev->at_end; + return 0; } -static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state) +static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state, + struct strbuf *err) { struct ref_formatting_stack *current = state->stack; struct strbuf s = STRBUF_INIT; - if (!current->at_end) - die(_("format: %%(end) atom used without corresponding atom")); + if (!current->at_end) { + strbuf_addstr(err, _("format: %(end) atom used without corresponding atom")); + return -1; + } current->at_end(&state->stack); /* Stack may have been popped within at_end(), hence reset the current pointer */ @@ -668,6 +690,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta } strbuf_release(&s); pop_stack_element(&state->stack); + return 0; } /* @@ -2138,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info, get_ref_atom_value(info, parse_ref_filter_atom(format, sp + 2, ep), &atomv); - atomv->handler(atomv, &state); + if (atomv->handler(atomv, &state, error_buf)) + return -1; } if (*cp) { sp = cp + strlen(cp); @@ -2147,7 +2171,8 @@ int format_ref_array_item(struct ref_array_item *info, if (format->need_color_reset_at_eol) { struct atom_value resetv; resetv.s = GIT_COLOR_RESET; - append_atom(&resetv, &state); + if (append_atom(&resetv, &state, error_buf)) + return -1; } if (state.stack->prev) { strbuf_addstr(error_buf, _("format: %(end) atom missing")); -- https://github.com/git/git/pull/466 ``` ## Martin Ågren, 2018-03-13 19:12 Subject: Re: [RFC 1/4] ref-filter: start adding strbufs with errors Message-ID: URL: https://gitlist.dev/e/CAN0heSoJKxqSb0q5SR2SVfEzh6QjeiNNnESorZyYHTSC71jZuQ%40mail.gmail.com In-Reply-To: <010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000@eu-west-1.amazonses.com> ``` On 13 March 2018 at 11:16, Olga Telezhnaya wrote: > This is a first step in removing any printing from > ref-filter formatting logic, so that it could be more general. > Everything would be the same for show_ref_array_item() users. > But, if you want to deal with errors by your own, you could invoke > format_ref_array_item(). It means that you need to print everything > (the result and errors) on your side. > > This commit changes signature of format_ref_array_item() by adding > return value and strbuf parameter for errors, and fixes > its callers. Minor nit: Maybe s/fixes its callers/adjusts its callers/. They are not broken or need to be fixed. They were simply playing the game according to the old rules, and now they need to learn the new ways. :-) > Signed-off-by: Olga Telezhnaia > --- > builtin/branch.c | 7 +++++-- > ref-filter.c | 17 ++++++++++++----- > ref-filter.h | 7 ++++--- > 3 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 8dcc2ed058be6..f86709ca42d5e 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin > struct ref_array array; > int maxwidth = 0; > const char *remote_prefix = ""; > - struct strbuf out = STRBUF_INIT; You move this variable into the loop to reduce its scope. At first I suspected that this might mean we now start allocating+releasing in each run of the loop, which might be a performance-regression. But it turns out, we already did that, so this tightening of the scope has no such downsides. :-) From the commit message, I wasn't expecting this move, though. Maybe "While at it, reduce the scope of the out-variable." > char *to_free = NULL; > > /* > @@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin > ref_array_sort(sorting, &array); > > for (i = 0; i < array.nr; i++) { > - format_ref_array_item(array.items[i], format, &out); > + struct strbuf out = STRBUF_INIT; > + struct strbuf err = STRBUF_INIT; > + if (format_ref_array_item(array.items[i], format, &out, &err)) > + die("%s", err.buf); Using "%s", good. > if (column_active(colopts)) { > assert(!filter->verbose && "--column and --verbose are incompatible"); > /* format to a string_list to let print_columns() do its job */ > @@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin > fwrite(out.buf, 1, out.len, stdout); > putchar('\n'); > } > + strbuf_release(&err); > strbuf_release(&out); > } > > diff --git a/ref-filter.c b/ref-filter.c > index 45fc56216aaa8..54fae00bdd410 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting > } > } > > -void format_ref_array_item(struct ref_array_item *info, > +int format_ref_array_item(struct ref_array_item *info, > const struct ref_format *format, > - struct strbuf *final_buf) > + struct strbuf *final_buf, > + struct strbuf *error_buf) > { > const char *cp, *sp, *ep; > struct ref_formatting_state state = REF_FORMATTING_STATE_INIT; > @@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info, > resetv.s = GIT_COLOR_RESET; > append_atom(&resetv, &state); > } > - if (state.stack->prev) > - die(_("format: %%(end) atom missing")); > + if (state.stack->prev) { > + strbuf_addstr(error_buf, _("format: %(end) atom missing")); > + return -1; > + } > strbuf_addbuf(final_buf, &state.stack->output); > pop_stack_element(&state.stack); > + return 0; > } > > void show_ref_array_item(struct ref_array_item *info, > const struct ref_format *format) > { > struct strbuf final_buf = STRBUF_INIT; > + struct strbuf error_buf = STRBUF_INIT; > > - format_ref_array_item(info, format, &final_buf); > + if (format_ref_array_item(info, format, &final_buf, &error_buf)) > + die("%s", error_buf.buf); > fwrite(final_buf.buf, 1, final_buf.len, stdout); > + strbuf_release(&error_buf); I think this `strbuf_release()` will never actually do anything. If we get here, we had no error. But it makes sense (to me) to always be clear about releasing this. In this case it is easy enough. Possible counterargument: We might want this sort of "error-handling by strbufs" to follow this simple rule: return an error if and only if you add some error-string to the buffer. If this rule is universal enough, it might be ok to skip releasing these sort of buffers if you do not have an error. Martin ``` ## Martin Ågren, 2018-03-13 19:13 Subject: Re: [RFC 2/4] ref-filter: add return value && strbuf to handlers Message-ID: URL: https://gitlist.dev/e/CAN0heSokC8twry_4_2N42tswZ_vhrDp8svo-MUk-dg-HyQKtQA%40mail.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 wrote: > -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. > -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 ``` ## Martin Ågren, 2018-03-13 19:18 Subject: Re: [RFC 3/4] ref-filter: change parsing function error handling Message-ID: URL: https://gitlist.dev/e/CAN0heSrogCMT-8R4nht%3D_9syMNUfJe21BvZSVD4AUcf9PwN7rQ%40mail.gmail.com In-Reply-To: <010201621edc982f-92123a33-386b-4b94-be0c-f793856e6c16-000000@eu-west-1.amazonses.com> ``` On 13 March 2018 at 11:16, Olga Telezhnaya wrote: > Continue removing any printing from ref-filter formatting logic, > so that it could be more general. > > Change the signature of parse_ref_filter_atom() by changing return value, > adding previous return value to function parameter and also adding > strbuf parameter for error message. I think the current return value is always non-negative. Maybe it would be easier to leave the return value as-is, except return negative on error? Unless I am missing something? > > Signed-off-by: Olga Telezhnaia > --- > ref-filter.c | 45 ++++++++++++++++++++++++++++++++------------- > 1 file changed, 32 insertions(+), 13 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 07bedc636398c..e146215bf1e64 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -397,7 +397,8 @@ struct atom_value { > * Used to parse format string and sort specifiers > */ > static int parse_ref_filter_atom(const struct ref_format *format, > - const char *atom, const char *ep) > + const char *atom, const char *ep, int *res, > + struct strbuf *err) > { > const char *sp; > const char *arg; > @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format *format, > sp = atom; > if (*sp == '*' && sp < ep) > sp++; /* deref */ > - if (ep <= sp) > - die(_("malformed field name: %.*s"), (int)(ep-atom), atom); > + if (ep <= sp) { > + strbuf_addf(err, _("malformed field name: %.*s"), (int)(ep-atom), atom); > + return -1; > + } > > /* Do we have the atom already used elsewhere? */ > for (i = 0; i < used_atom_cnt; i++) { > int len = strlen(used_atom[i].name); > - if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) > - return i; > + if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) { > + *res = i; > + return 0; > + } > } If you did so, this hunk above would not need to be changed ... > @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format *format, > need_tagged = 1; > if (!strcmp(valid_atom[i].name, "symref")) > need_symref = 1; > - return at; > + *res = at; > + return 0; > } ... nor this one above ... > if (!ep) > return error(_("malformed format string %s"), sp); > /* sp points at "%(" and ep points at the closing ")" */ > - at = parse_ref_filter_atom(format, sp + 2, ep); > + if (parse_ref_filter_atom(format, sp + 2, ep, &at, &err)) > + die("%s", err.buf); And this would be more like "if (at < 0) die(...)". > for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) { > struct atom_value *atomv; > + struct strbuf err = STRBUF_INIT; > + int pos; > > ep = strchr(sp, ')'); > if (cp < sp) > append_literal(cp, sp, &state); > - get_ref_atom_value(info, > - parse_ref_filter_atom(format, sp + 2, ep), > - &atomv); > + if (parse_ref_filter_atom(format, sp + 2, ep, &pos, &err)) > + return -1; > + get_ref_atom_value(info, pos, &atomv); > if (atomv->handler(atomv, &state, error_buf)) > return -1; > + strbuf_release(&err); This looks leaky: if we get an error, we've got something in the buffer but we do not release it because we return early. Stepping back a bit, I wonder why we do not do anything at all with "err". Stepping back a bit more :-) I wonder if you could get rid of "err" and pass "error_buf" to parse_ref_filter_atom() instead. Our caller would like to have access to the error string? This ties back to my comment on the first patch -- "return negative if and only if you add some error string to the buffer" might be a useful rule? Martin ``` ## Оля Тележная, 2018-03-14 13:30 Subject: Re: [RFC 1/4] ref-filter: start adding strbufs with errors Message-ID: URL: https://gitlist.dev/e/CAL21BmmQvjzpXGCjuHdPFiQ07P7VcV-E%3DmQbmM%2B35Y1u8acD6Q%40mail.gmail.com In-Reply-To: ``` 2018-03-13 22:12 GMT+03:00 Martin Ågren : > On 13 March 2018 at 11:16, Olga Telezhnaya wrote: >> This is a first step in removing any printing from >> ref-filter formatting logic, so that it could be more general. >> Everything would be the same for show_ref_array_item() users. >> But, if you want to deal with errors by your own, you could invoke >> format_ref_array_item(). It means that you need to print everything >> (the result and errors) on your side. >> >> This commit changes signature of format_ref_array_item() by adding >> return value and strbuf parameter for errors, and fixes >> its callers. > > Minor nit: Maybe s/fixes its callers/adjusts its callers/. They are not > broken or need to be fixed. They were simply playing the game according > to the old rules, and now they need to learn the new ways. :-) Agree. > >> Signed-off-by: Olga Telezhnaia >> --- >> builtin/branch.c | 7 +++++-- >> ref-filter.c | 17 ++++++++++++----- >> ref-filter.h | 7 ++++--- >> 3 files changed, 21 insertions(+), 10 deletions(-) >> >> diff --git a/builtin/branch.c b/builtin/branch.c >> index 8dcc2ed058be6..f86709ca42d5e 100644 >> --- a/builtin/branch.c >> +++ b/builtin/branch.c >> @@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin >> struct ref_array array; >> int maxwidth = 0; >> const char *remote_prefix = ""; >> - struct strbuf out = STRBUF_INIT; > > You move this variable into the loop to reduce its scope. At first I > suspected that this might mean we now start allocating+releasing in each > run of the loop, which might be a performance-regression. But it turns > out, we already did that, so this tightening of the scope has no such > downsides. :-) From the commit message, I wasn't expecting this move, > though. Maybe "While at it, reduce the scope of the out-variable." Added this to commit message. I just wanted to unify code style. I added another strbuf and tried to reduce its scope (not sure that it will cause a performance regression, I guess compiler is smart enough - but, who knows). I saw another strbuf, and I just decided to put them together. In my opinion, it's easier to read the code where variables are created in the smallest possible scope. But, anyway, you are right, I needed to mention that in the commit message. > >> char *to_free = NULL; >> >> /* >> @@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin >> ref_array_sort(sorting, &array); >> >> for (i = 0; i < array.nr; i++) { >> - format_ref_array_item(array.items[i], format, &out); >> + struct strbuf out = STRBUF_INIT; >> + struct strbuf err = STRBUF_INIT; >> + if (format_ref_array_item(array.items[i], format, &out, &err)) >> + die("%s", err.buf); > > Using "%s", good. > >> if (column_active(colopts)) { >> assert(!filter->verbose && "--column and --verbose are incompatible"); >> /* format to a string_list to let print_columns() do its job */ >> @@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin >> fwrite(out.buf, 1, out.len, stdout); >> putchar('\n'); >> } >> + strbuf_release(&err); >> strbuf_release(&out); >> } >> >> diff --git a/ref-filter.c b/ref-filter.c >> index 45fc56216aaa8..54fae00bdd410 100644 >> --- a/ref-filter.c >> +++ b/ref-filter.c >> @@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting >> } >> } >> >> -void format_ref_array_item(struct ref_array_item *info, >> +int format_ref_array_item(struct ref_array_item *info, >> const struct ref_format *format, >> - struct strbuf *final_buf) >> + struct strbuf *final_buf, >> + struct strbuf *error_buf) >> { >> const char *cp, *sp, *ep; >> struct ref_formatting_state state = REF_FORMATTING_STATE_INIT; >> @@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info, >> resetv.s = GIT_COLOR_RESET; >> append_atom(&resetv, &state); >> } >> - if (state.stack->prev) >> - die(_("format: %%(end) atom missing")); >> + if (state.stack->prev) { >> + strbuf_addstr(error_buf, _("format: %(end) atom missing")); >> + return -1; >> + } >> strbuf_addbuf(final_buf, &state.stack->output); >> pop_stack_element(&state.stack); >> + return 0; >> } >> >> void show_ref_array_item(struct ref_array_item *info, >> const struct ref_format *format) >> { >> struct strbuf final_buf = STRBUF_INIT; >> + struct strbuf error_buf = STRBUF_INIT; >> >> - format_ref_array_item(info, format, &final_buf); >> + if (format_ref_array_item(info, format, &final_buf, &error_buf)) >> + die("%s", error_buf.buf); >> fwrite(final_buf.buf, 1, final_buf.len, stdout); >> + strbuf_release(&error_buf); > > I think this `strbuf_release()` will never actually do anything. If we > get here, we had no error. But it makes sense (to me) to always be clear > about releasing this. In this case it is easy enough. I was thinking same as you (I want to be sure that we release everything). > > Possible counterargument: We might want this sort of "error-handling by > strbufs" to follow this simple rule: return an error if and only if you > add some error-string to the buffer. If this rule is universal enough, > it might be ok to skip releasing these sort of buffers if you do not > have an error. I just think that it's more intuitive to release everything in all cases, even if this line seems useless. Anyway, it's not super hard to remove this line in any moment, so I guess we could wait with this till the final review. > > Martin ``` ## Оля Тележная, 2018-03-14 13:36 Subject: Re: [RFC 3/4] ref-filter: change parsing function error handling Message-ID: URL: https://gitlist.dev/e/CAL21BmkPrgUtiuV%3DAMqnOwPX-aXdWU39CFLwGCAmMLvXBdLXWA%40mail.gmail.com In-Reply-To: ``` 2018-03-13 22:18 GMT+03:00 Martin Ågren : > On 13 March 2018 at 11:16, Olga Telezhnaya wrote: >> Continue removing any printing from ref-filter formatting logic, >> so that it could be more general. >> >> Change the signature of parse_ref_filter_atom() by changing return value, >> adding previous return value to function parameter and also adding >> strbuf parameter for error message. > > I think the current return value is always non-negative. Maybe it would > be easier to leave the return value as-is, except return negative on > error? Unless I am missing something? That's interesting. I like your idea, but let's see what other people think. If others agree with us, I am ready to implement your solution. > >> >> Signed-off-by: Olga Telezhnaia >> --- >> ref-filter.c | 45 ++++++++++++++++++++++++++++++++------------- >> 1 file changed, 32 insertions(+), 13 deletions(-) >> >> diff --git a/ref-filter.c b/ref-filter.c >> index 07bedc636398c..e146215bf1e64 100644 >> --- a/ref-filter.c >> +++ b/ref-filter.c >> @@ -397,7 +397,8 @@ struct atom_value { >> * Used to parse format string and sort specifiers >> */ >> static int parse_ref_filter_atom(const struct ref_format *format, >> - const char *atom, const char *ep) >> + const char *atom, const char *ep, int *res, >> + struct strbuf *err) >> { >> const char *sp; >> const char *arg; >> @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format *format, >> sp = atom; >> if (*sp == '*' && sp < ep) >> sp++; /* deref */ >> - if (ep <= sp) >> - die(_("malformed field name: %.*s"), (int)(ep-atom), atom); >> + if (ep <= sp) { >> + strbuf_addf(err, _("malformed field name: %.*s"), (int)(ep-atom), atom); >> + return -1; >> + } >> >> /* Do we have the atom already used elsewhere? */ >> for (i = 0; i < used_atom_cnt; i++) { >> int len = strlen(used_atom[i].name); >> - if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) >> - return i; >> + if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) { >> + *res = i; >> + return 0; >> + } >> } > > If you did so, this hunk above would not need to be changed ... > >> @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format *format, >> need_tagged = 1; >> if (!strcmp(valid_atom[i].name, "symref")) >> need_symref = 1; >> - return at; >> + *res = at; >> + return 0; >> } > > ... nor this one above ... > >> if (!ep) >> return error(_("malformed format string %s"), sp); >> /* sp points at "%(" and ep points at the closing ")" */ >> - at = parse_ref_filter_atom(format, sp + 2, ep); >> + if (parse_ref_filter_atom(format, sp + 2, ep, &at, &err)) >> + die("%s", err.buf); > > And this would be more like "if (at < 0) die(...)". > >> for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) { >> struct atom_value *atomv; >> + struct strbuf err = STRBUF_INIT; >> + int pos; >> >> ep = strchr(sp, ')'); >> if (cp < sp) >> append_literal(cp, sp, &state); >> - get_ref_atom_value(info, >> - parse_ref_filter_atom(format, sp + 2, ep), >> - &atomv); >> + if (parse_ref_filter_atom(format, sp + 2, ep, &pos, &err)) >> + return -1; >> + get_ref_atom_value(info, pos, &atomv); >> if (atomv->handler(atomv, &state, error_buf)) >> return -1; >> + strbuf_release(&err); > > This looks leaky: if we get an error, we've got something in the buffer > but we do not release it because we return early. Stepping back a bit, I > wonder why we do not do anything at all with "err". Stepping back a bit > more :-) I wonder if you could get rid of "err" and pass "error_buf" to > parse_ref_filter_atom() instead. Our caller would like to have access to > the error string? Fully agree, I don't know why I decided to create one more buffer. Fixed. > > This ties back to my comment on the first patch -- "return negative if > and only if you add some error string to the buffer" might be a useful > rule? > > Martin ```