# [PATCH v5 4/6] ref-filter: change parsing function error handling 10 messages from 2018-03-21 to 2018-03-23. Participants: Olga Telezhnaya, Junio C Hamano, Оля Тележная. Thread: https://gitlist.dev/t/48108 ## Olga Telezhnaya, 2018-03-21 18:28 Subject: [PATCH v5 4/6] ref-filter: change parsing function error handling Message-ID: <0102016249d21c96-29f058b0-a43d-45b7-9c1d-5ea2382858a2-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016249d21c96-29f058b0-a43d-45b7-9c1d-5ea2382858a2-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com> ``` Continue removing die() calls from ref-filter formatting logic, so that it could be used by other commands. Change the signature of parse_ref_filter_atom() by adding strbuf parameter for error message. The function returns the position in the used_atom[] array (as before) for the given atom, or -1 to signal an error. Upon failure, error message is appended to the strbuf. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index f79c8c477f1dc..ca38728b4c998 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, + struct strbuf *err) { const char *sp; const char *arg; @@ -407,7 +408,8 @@ static int parse_ref_filter_atom(const struct ref_format *format, if (*sp == '*' && sp < ep) sp++; /* deref */ if (ep <= sp) - die(_("malformed field name: %.*s"), (int)(ep-atom), atom); + return strbuf_error(err, -1, _("malformed field name: %.*s"), + (int)(ep-atom), atom); /* Do we have the atom already used elsewhere? */ for (i = 0; i < used_atom_cnt; i++) { @@ -433,7 +435,8 @@ static int parse_ref_filter_atom(const struct ref_format *format, } if (ARRAY_SIZE(valid_atom) <= i) - die(_("unknown field name: %.*s"), (int)(ep-atom), atom); + return strbuf_error(err, -1, _("unknown field name: %.*s"), + (int)(ep-atom), atom); /* Add it in, including the deref prefix */ at = used_atom_cnt; @@ -715,17 +718,21 @@ 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); + at = parse_ref_filter_atom(format, sp + 2, ep, &err); + if (at < 0) + 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; @@ -2144,13 +2151,15 @@ 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; + 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); + pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf); + if (pos < 0) + return -1; + get_ref_atom_value(info, pos, &atomv); if (atomv->handler(atomv, &state, error_buf)) return -1; } @@ -2203,7 +2212,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 = parse_ref_filter_atom(&dummy, atom, end, &err); + if (res < 0) + 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-21 18:28 Subject: [PATCH v5 1/6] strbuf: add shortcut to work with error messages Message-ID: <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000%40eu-west-1.amazonses.com In-Reply-To: <01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000@eu-west-1.amazonses.com> ``` Add function strbuf_error() that helps to save few lines of code. Function expands fmt with placeholders, append resulting error message to strbuf *err, and return error code ret. Signed-off-by: Olga Telezhnaia --- strbuf.h | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/strbuf.h b/strbuf.h index e6cae5f4398c8..fa66d4835f1a7 100644 --- a/strbuf.h +++ b/strbuf.h @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap); __attribute__((format (printf, 1, 2))) char *xstrfmt(const char *fmt, ...); +/* + * Expand error message, append it to strbuf *err, then return error code ret. + * Allow to save few lines of code. + */ +static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...) +{ + va_list ap; + va_start(ap, fmt); + strbuf_vaddf(err, fmt, ap); + va_end(ap); + return ret; +} + #endif /* STRBUF_H */ -- https://github.com/git/git/pull/466 ``` ## Olga Telezhnaya, 2018-03-21 18:28 Subject: [PATCH v5 5/6] ref-filter: add return value to parsers Message-ID: <0102016249d21c9b-260decf6-b3ce-45ed-ba68-5107cf482832-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016249d21c9b-260decf6-b3ce-45ed-ba68-5107cf482832-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com> ``` Continue removing die() calls from ref-filter formatting logic, so that it could be used by other commands. Change the signature of parsers by adding return value and strbuf parameter for error message. Return value equals 0 upon success and -1 upon failure. Upon failure, error message is appended to the strbuf. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 112 ++++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 68 insertions(+), 44 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index ca38728b4c998..0ef7386b5fd20 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -101,22 +101,25 @@ 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:)")); + return strbuf_error(err, -1, _("expected format: %%(color:)")); if (color_parse(color_value, atom->u.color) < 0) - die(_("unrecognized color: %%(color:%s)"), color_value); + return strbuf_error(err, -1, _("unrecognized color: %%(color:%s)"), + color_value); /* * 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; @@ -126,16 +129,18 @@ static void refname_atom_parser_internal(struct refname_atom *atom, skip_prefix(arg, "strip=", &arg)) { atom->option = R_LSTRIP; if (strtol_i(arg, 10, &atom->lstrip)) - die(_("Integer value expected refname:lstrip=%s"), arg); + return strbuf_error(err, -1, _("Integer value expected refname:lstrip=%s"), arg); } 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); + return strbuf_error(err, -1, _("Integer value expected refname:rstrip=%s"), arg); } else - die(_("unrecognized %%(%s) argument: %s"), name, arg); + return strbuf_error(err, -1, _("unrecognized %%(%s) argument: %s"), name, arg); + 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 +150,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 +174,36 @@ 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")); + return strbuf_error(err, -1, _("%%(body) does not take arguments")); 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")); + return strbuf_error(err, -1, _("%%(subject) does not take arguments")); 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; @@ -206,14 +217,16 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at else if (!strcmp(s, "only")) atom->u.contents.trailer_opts.only_trailers = 1; else - die(_("unknown %%(trailers) argument: %s"), s); + return strbuf_error(err, -1, _("unknown %%(trailers) argument: %s"), s); } } 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 +238,19 @@ 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); + return strbuf_error(err, -1, _("positive value expected contents:lines=%s"), arg); } else - die(_("unrecognized %%(contents) argument: %s"), arg); + return strbuf_error(err, -1, _("unrecognized %%(contents) argument: %s"), arg); + 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; @@ -244,16 +260,18 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_ 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); + return strbuf_error(err, -1, _("positive value expected objectname:short=%s"), arg); if (atom->u.objectname.length < MINIMUM_ABBREV) atom->u.objectname.length = MINIMUM_ABBREV; } else - die(_("unrecognized %%(objectname) argument: %s"), arg); + return strbuf_error(err, -1, _("unrecognized %%(objectname) argument: %s"), arg); + 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,7 +285,8 @@ 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; @@ -275,7 +294,7 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom unsigned int width = ~0U; if (!arg) - die(_("expected format: %%(align:,)")); + return strbuf_error(err, -1, _("expected format: %%(align:,)")); align->position = ALIGN_LEFT; @@ -287,48 +306,53 @@ 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); + return strbuf_error(err, -1, _("unrecognized position:%s"), s); align->position = position; } else if (skip_prefix(s, "width=", &s)) { if (strtoul_ui(s, 10, &width)) - die(_("unrecognized width:%s"), s); + return strbuf_error(err, -1, _("unrecognized width:%s"), s); } 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); + return strbuf_error(err, -1, _("unrecognized %%(align) argument: %s"), s); } if (width == ~0U) - die(_("positive width expected with the %%(align) atom")); + return strbuf_error(err, -1, _("positive width expected with the %%(align) atom")); 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); - } + } else + return strbuf_error(err, -1, _("unrecognized %%(if) argument: %s"), arg); + 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" }, @@ -455,8 +479,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-21 18:28 Subject: [PATCH v5 2/6] ref-filter: start adding strbufs with errors Message-ID: <0102016249d21c87-2c0eb583-2766-42d1-9629-06563fa91820-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016249d21c87-2c0eb583-2766-42d1-9629-06563fa91820-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com> ``` This is a first step in removing die() calls from ref-filter formatting logic, so that it could be used by other commands that do not want to die during formatting process. die() calls related to bugs in code will not be touched in this patch. 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 adjusts its callers. While at it, reduce the scope of the out-variable. Signed-off-by: Olga Telezhnaia --- builtin/branch.c | 7 +++++-- ref-filter.c | 13 +++++++++---- ref-filter.h | 7 ++++--- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 6d0cea9d4bcc4..c21e5a04a0177 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..2c3fb2f003708 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; @@ -2149,18 +2150,22 @@ void format_ref_array_item(struct ref_array_item *info, append_atom(&resetv, &state); } if (state.stack->prev) - die(_("format: %%(end) atom missing")); + return strbuf_error(error_buf, -1, _("format: %%(end) atom missing")); 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-21 18:28 Subject: [PATCH v5 6/6] ref-filter: libify get_ref_atom_value() Message-ID: <0102016249d21ca3-4c6c5c4c-81c1-487d-8fc1-1bb6a2c83e56-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016249d21ca3-4c6c5c4c-81c1-487d-8fc1-1bb6a2c83e56-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com> ``` 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. Upon failure, error message is appended to the strbuf. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 49 ++++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 0ef7386b5fd20..52bb84398dc8d 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1398,28 +1398,30 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re return show_ref(&atom->u.refname, ref->refname); } -static void get_object(struct ref_array_item *ref, const struct object_id *oid, - int deref, struct object **obj) +static int get_object(struct ref_array_item *ref, const struct object_id *oid, + int deref, struct object **obj, struct strbuf *err) { int eaten; + int ret = 0; unsigned long size; 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); - - grab_values(ref->value, deref, *obj, buf, size); + ret = strbuf_error(err, -1, _("missing object %s for %s"), + oid_to_hex(oid), ref->refname); + else if (!*obj) + ret = strbuf_error(err, -1, _("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; } /* * Parse the object referred by ref, and grab needed value. */ -static void populate_value(struct ref_array_item *ref) +static int populate_value(struct ref_array_item *ref, struct strbuf *err) { struct object *obj; int i; @@ -1541,16 +1543,17 @@ static void populate_value(struct ref_array_item *ref) break; } if (used_atom_cnt <= i) - return; + return 0; - get_object(ref, &ref->objectname, 0, &obj); + if (get_object(ref, &ref->objectname, 0, &obj, err)) + return -1; /* * If there is no atom that wants to know about tagged * object, we are done. */ if (!need_tagged || (obj->type != OBJ_TAG)) - return; + return 0; /* * If it is a tag object, see if we use a value that derefs @@ -1564,20 +1567,23 @@ static void populate_value(struct ref_array_item *ref) * is not consistent with what deref_tag() does * which peels the onion to the core. */ - get_object(ref, tagged, 1, &obj); + return get_object(ref, tagged, 1, &obj, err); } /* * Given a ref, return the value for the atom. This lazily gets value * out of the object by calling populate value. */ -static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v) +static int get_ref_atom_value(struct ref_array_item *ref, int atom, + struct atom_value **v, struct strbuf *err) { if (!ref->value) { - populate_value(ref); + if (populate_value(ref, err)) + return -1; fill_missing_values(ref->value); } *v = &ref->value[atom]; + return 0; } /* @@ -2101,9 +2107,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru int cmp; cmp_type cmp_type = used_atom[s->atom].type; int (*cmp_fn)(const char *, const char *); + struct strbuf err = STRBUF_INIT; - get_ref_atom_value(a, s->atom, &va); - get_ref_atom_value(b, s->atom, &vb); + if (get_ref_atom_value(a, s->atom, &va, &err)) + die("%s", err.buf); + if (get_ref_atom_value(b, s->atom, &vb, &err)) + die("%s", err.buf); + strbuf_release(&err); cmp_fn = s->ignore_case ? strcasecmp : strcmp; if (s->version) cmp = versioncmp(va->s, vb->s); @@ -2183,7 +2193,8 @@ int format_ref_array_item(struct ref_array_item *info, pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf); if (pos < 0) return -1; - get_ref_atom_value(info, pos, &atomv); + if (get_ref_atom_value(info, pos, &atomv, error_buf)) + return -1; if (atomv->handler(atomv, &state, error_buf)) return -1; } -- https://github.com/git/git/pull/466 ``` ## Olga Telezhnaya, 2018-03-21 18:28 Subject: [PATCH v5 3/6] ref-filter: add return value && strbuf to handlers Message-ID: <0102016249d21c7f-c26a3b49-dbf3-4435-b1ef-e61917f0d08e-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016249d21c7f-c26a3b49-dbf3-4435-b1ef-e61917f0d08e-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com> ``` Continue removing die() calls from ref-filter formatting logic, so that it could be used by other commands. Change the signature of handlers by adding return value and strbuf parameter for errors. Return value equals 0 upon success and -1 upon failure. Upon failure, error message is appended to the strbuf. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 2c3fb2f003708..f79c8c477f1dc 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 *unused_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 *unused_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 *unused_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,7 +603,8 @@ 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; @@ -604,11 +612,11 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st 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")); + return strbuf_error(err, -1, _("format: %%(then) atom used without an %%(if) atom")); if (if_then_else->then_atom_seen) - die(_("format: %%(then) atom used more than once")); + return strbuf_error(err, -1, _("format: %%(then) atom used more than once")); if (if_then_else->else_atom_seen) - die(_("format: %%(then) atom used after %%(else)")); + return strbuf_error(err, -1, _("format: %%(then) atom used after %%(else)")); if_then_else->then_atom_seen = 1; /* * If the 'equals' or 'notequals' attribute is used then @@ -624,9 +632,11 @@ 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; @@ -634,24 +644,26 @@ static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_st 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")); + return strbuf_error(err, -1, _("format: %%(else) atom used without an %%(if) atom")); if (!if_then_else->then_atom_seen) - die(_("format: %%(else) atom used without a %%(then) atom")); + return strbuf_error(err, -1, _("format: %%(else) atom used without a %%(then) atom")); if (if_then_else->else_atom_seen) - die(_("format: %%(else) atom used more than once")); + return strbuf_error(err, -1, _("format: %%(else) atom used more than once")); 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")); + return strbuf_error(err, -1, _("format: %%(end) atom used without corresponding atom")); current->at_end(&state->stack); /* Stack may have been popped within at_end(), hence reset the current pointer */ @@ -668,6 +680,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 +2151,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 +2161,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) return strbuf_error(error_buf, -1, _("format: %%(end) atom missing")); -- https://github.com/git/git/pull/466 ``` ## Junio C Hamano, 2018-03-21 20:20 Subject: Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages Message-ID: URL: https://gitlist.dev/e/xmqqlgelnpsx.fsf%40gitster-ct.c.googlers.com In-Reply-To: <0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com> ``` Olga Telezhnaya writes: > Add function strbuf_error() that helps to save few lines of code. > Function expands fmt with placeholders, append resulting error message > to strbuf *err, and return error code ret. > > Signed-off-by: Olga Telezhnaia > --- > strbuf.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/strbuf.h b/strbuf.h > index e6cae5f4398c8..fa66d4835f1a7 100644 > --- a/strbuf.h > +++ b/strbuf.h > @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap); > __attribute__((format (printf, 1, 2))) > char *xstrfmt(const char *fmt, ...); > > +/* > + * Expand error message, append it to strbuf *err, then return error code ret. > + * Allow to save few lines of code. > + */ > +static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...) > +{ With this function, err does not have to be an error message, and ret does not have to be negative. Hence strbuf_error() is a wrong name for the wrapper. It somewhat is bothersome to see that this is inlined; if it is meant for error codepath, it probably shouldn't have to be. > + va_list ap; > + va_start(ap, fmt); > + strbuf_vaddf(err, fmt, ap); > + va_end(ap); > + return ret; > +} > + > #endif /* STRBUF_H */ Quite honestly, I am not sure if it is worth to be in strbuf.h; it feels a bit too specific to the immediate need for these five patches and nowhere else. Are there many existing calls to strbuf_addf() immediately followed by an "return" of an integer, that can be simplified by using this helper? I see quite a many instances of addf() soon followed by "return -1" in refs/files-backend.c, but they are not immediately adjacent to each other, and won't be helped. ``` ## Junio C Hamano, 2018-03-21 20:36 Subject: Re: [PATCH v5 4/6] ref-filter: change parsing function error handling Message-ID: URL: https://gitlist.dev/e/xmqqh8p9np2a.fsf%40gitster-ct.c.googlers.com In-Reply-To: <0102016249d21c96-29f058b0-a43d-45b7-9c1d-5ea2382858a2-000000@eu-west-1.amazonses.com> ``` Olga Telezhnaya writes: > @@ -2144,13 +2151,15 @@ 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; > + 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); > + pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf); > + if (pos < 0) > + return -1; > + get_ref_atom_value(info, pos, &atomv); > if (atomv->handler(atomv, &state, error_buf)) > return -1; > } These error returns leave the formatting state "state" on the stack holding onto its resources, no? The only thing the caller of format_ref_array_item() that notices an error return does is to die even after this series, so in that sense it does not matter (yet), but it still feels somewhat wrong. ``` ## Оля Тележная, 2018-03-23 06:48 Subject: Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages Message-ID: URL: https://gitlist.dev/e/CAL21Bm%3DM-X8pjnxKzXKZbwArF6gyoPLLLa8JBZFNrvwrsRsacg%40mail.gmail.com In-Reply-To: ``` 2018-03-21 23:20 GMT+03:00 Junio C Hamano : > Olga Telezhnaya writes: > >> Add function strbuf_error() that helps to save few lines of code. >> Function expands fmt with placeholders, append resulting error message >> to strbuf *err, and return error code ret. >> >> Signed-off-by: Olga Telezhnaia >> --- >> strbuf.h | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/strbuf.h b/strbuf.h >> index e6cae5f4398c8..fa66d4835f1a7 100644 >> --- a/strbuf.h >> +++ b/strbuf.h >> @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap); >> __attribute__((format (printf, 1, 2))) >> char *xstrfmt(const char *fmt, ...); >> >> +/* >> + * Expand error message, append it to strbuf *err, then return error code ret. >> + * Allow to save few lines of code. >> + */ >> +static inline int strbuf_error(struct strbuf *err, int ret, const char *fmt, ...) >> +{ > > With this function, err does not have to be an error message, and > ret does not have to be negative. Hence strbuf_error() is a wrong > name for the wrapper. > > It somewhat is bothersome to see that this is inlined; if it is > meant for error codepath, it probably shouldn't have to be. > >> + va_list ap; >> + va_start(ap, fmt); >> + strbuf_vaddf(err, fmt, ap); >> + va_end(ap); >> + return ret; >> +} >> + >> #endif /* STRBUF_H */ > > Quite honestly, I am not sure if it is worth to be in strbuf.h; it > feels a bit too specific to the immediate need for these five > patches and nowhere else. Are there many existing calls to > strbuf_addf() immediately followed by an "return" of an integer, > that can be simplified by using this helper? I see quite a many > instances of addf() soon followed by "return -1" in > refs/files-backend.c, but they are not immediately adjacent to each > other, and won't be helped. Summarizing all that we discussed: I have 2 options how to continue this patch. I can revert to v4, or I can replace new function in strbuf.h with similar macro in ref-filter.c (I mean, there would be no changes in strbuf.h and one new macro in ref-filter.c). What do you like more? Thanks! Olga ``` ## Оля Тележная, 2018-03-23 06:56 Subject: Re: [PATCH v5 4/6] ref-filter: change parsing function error handling Message-ID: URL: https://gitlist.dev/e/CAL21BmnJ8ktMxt2-sLBE-US-j8p_MFGOYjdoCHPmdL0-NG%3Domg%40mail.gmail.com In-Reply-To: ``` 2018-03-21 23:36 GMT+03:00 Junio C Hamano : > Olga Telezhnaya writes: > >> @@ -2144,13 +2151,15 @@ 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; >> + 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); >> + pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf); >> + if (pos < 0) >> + return -1; >> + get_ref_atom_value(info, pos, &atomv); >> if (atomv->handler(atomv, &state, error_buf)) >> return -1; >> } > > These error returns leave the formatting state "state" on the stack > holding onto its resources, no? Yes, you are right, thanks a lot! > > The only thing the caller of format_ref_array_item() that notices an > error return does is to die even after this series, so in that sense > it does not matter (yet), but it still feels somewhat wrong. > I am sure I need to fix it anyway, thanks. ```