# [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling 30 messages from 2018-03-14 to 2018-03-21. Participants: Olga Telezhnaya, Martin Ågren, Eric Sunshine, Junio C Hamano, Оля Тележная. Thread: https://gitlist.dev/t/48044 ## Olga Telezhnaya, 2018-03-14 19:04 Subject: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: <0102016225e61cc2-0d0c05ef-b999-4bf2-88ac-db9840dd2a87-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016225e61cc2-0d0c05ef-b999-4bf2-88ac-db9840dd2a87-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com> ``` Finish removing any printing from ref-filter formatting logic, so that it could be more general. Change the signature of get_ref_atom_value() and underlying functions by adding return value and strbuf parameter for error message. It's important to mention that grab_objectname() returned 1 if it gets objectname atom and 0 otherwise. Now this logic changed: we return 0 if we have no error, -1 otherwise. If someone needs to know whether it's objectname atom or not, he/she could use starts_with() function. It duplicates this checking but it does not sound like a really big overhead. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 69 insertions(+), 40 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 62ea4adcd0ff1..3f0c3924273d5 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned } static int grab_objectname(const char *name, const unsigned char *sha1, - struct atom_value *v, struct used_atom *atom) + struct atom_value *v, struct used_atom *atom, + struct strbuf *err) { if (starts_with(name, "objectname")) { if (atom->u.objectname.option == O_SHORT) { v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV)); - return 1; } else if (atom->u.objectname.option == O_FULL) { v->s = xstrdup(sha1_to_hex(sha1)); - return 1; } else if (atom->u.objectname.option == O_LENGTH) { v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length)); - return 1; - } else - die("BUG: unknown %%(objectname) option"); + } else { + strbuf_addstr(err, "BUG: unknown %(objectname) option"); + return -1; + } } return 0; } /* See grab_values */ -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz) +static int grab_common_values(struct atom_value *val, int deref, struct object *obj, + void *buf, unsigned long sz, struct strbuf *err) { int i; @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object v->s = xstrfmt("%lu", sz); } else if (deref) - grab_objectname(name, obj->oid.hash, v, &used_atom[i]); + if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err)) + return -1; } + return 0; } /* See grab_values */ @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val) * pointed at by the ref itself; otherwise it is the object the * ref (which is a tag) refers to. */ -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz) +static int grab_values(struct atom_value *val, int deref, struct object *obj, + void *buf, unsigned long sz, struct strbuf *err) { - grab_common_values(val, deref, obj, buf, sz); + if (grab_common_values(val, deref, obj, buf, sz, err)) + return -1; switch (obj->type) { case OBJ_TAG: grab_tag_values(val, deref, obj, buf, sz); @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v /* grab_blob_values(val, deref, obj, buf, sz); */ break; default: - die("Eh? Object of type %d?", obj->type); + strbuf_addf(err, "Eh? Object of type %d?", obj->type); + return -1; } + return 0; } static inline char *copy_advance(char *dst, const char *src) @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname) return refname; } -static void fill_remote_ref_details(struct used_atom *atom, const char *refname, - struct branch *branch, const char **s) +static int fill_remote_ref_details(struct used_atom *atom, const char *refname, + struct branch *branch, const char **s, + struct strbuf *err) { int num_ours, num_theirs; if (atom->u.remote_ref.option == RR_REF) @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname, } else if (atom->u.remote_ref.option == RR_TRACKSHORT) { if (stat_tracking_info(branch, &num_ours, &num_theirs, NULL, AHEAD_BEHIND_FULL) < 0) - return; + return 0; if (!num_ours && !num_theirs) *s = "="; @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname, *s = xstrdup(merge); else *s = ""; - } else - die("BUG: unhandled RR_* enum"); + } else { + strbuf_addstr(err, "BUG: unhandled RR_* enum"); + return -1; + } + return 0; } char *get_head_description(void) @@ -1447,28 +1458,33 @@ 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; 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); + if (!buf) { + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), + ref->refname); + return -1; + } + if (!*obj) { + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), + oid_to_hex(oid), ref->refname); + return -1; + } + if (grab_values(ref->value, deref, *obj, buf, size, err)) + return -1; if (!eaten) free(buf); + return 0; } /* * 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; @@ -1513,8 +1529,9 @@ static void populate_value(struct ref_array_item *ref) branch = branch_get(branch_name); refname = branch_get_upstream(branch, NULL); - if (refname) - fill_remote_ref_details(atom, refname, branch, &v->s); + if (refname && + fill_remote_ref_details(atom, refname, branch, &v->s, err)) + return -1; continue; } else if (atom->u.remote_ref.push) { const char *branch_name; @@ -1530,7 +1547,8 @@ static void populate_value(struct ref_array_item *ref) if (!refname) continue; } - fill_remote_ref_details(atom, refname, branch, &v->s); + if (fill_remote_ref_details(atom, refname, branch, &v->s, err)) + return -1; continue; } else if (starts_with(name, "color:")) { v->s = atom->u.color; @@ -1548,7 +1566,9 @@ static void populate_value(struct ref_array_item *ref) v->s = xstrdup(buf + 1); } continue; - } else if (!deref && grab_objectname(name, ref->objectname.hash, v, atom)) { + } else if (!deref && starts_with(name, "objectname")) { + if (grab_objectname(name, ref->objectname.hash, v, atom, err)) + return -1; continue; } else if (!strcmp(name, "HEAD")) { if (atom->u.head && !strcmp(ref->refname, atom->u.head)) @@ -1590,16 +1610,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 @@ -1613,20 +1634,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; } /* @@ -2150,9 +2174,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); @@ -2231,7 +2259,8 @@ int format_ref_array_item(struct ref_array_item *info, append_literal(cp, sp, &state); if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf)) 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-14 19:04 Subject: [PATCH v2 3/5] ref-filter: change parsing function error handling Message-ID: <0102016225e61cb4-b59773de-d22e-42cd-909a-d7ce71646898-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016225e61cb4-b59773de-d22e-42cd-909a-d7ce71646898-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-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 | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index d120360104806..dd83ef326511d 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,13 +2165,14 @@ 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); + if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf)) + return -1; + get_ref_atom_value(info, pos, &atomv); if (atomv->handler(atomv, &state, error_buf)) return -1; } @@ -2215,7 +2227,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-14 19:04 Subject: [PATCH v2 1/5] ref-filter: start adding strbufs with errors Message-ID: <0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000%40eu-west-1.amazonses.com In-Reply-To: <010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000@eu-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 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 | 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-14 19:04 Subject: [PATCH v2 4/5] ref-filter: add return value to parsers Message-ID: <0102016225e61cc8-a13a29f9-605e-4703-bb87-5c2d6829f0aa-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016225e61cc8-a13a29f9-605e-4703-bb87-5c2d6829f0aa-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-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 dd83ef326511d..62ea4adcd0ff1 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-14 19:04 Subject: [PATCH v2 2/5] ref-filter: add return value && strbuf to handlers Message-ID: <0102016225e61cc3-67840882-64b6-4a44-8199-4384fea48850-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016225e61cc3-67840882-64b6-4a44-8199-4384fea48850-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-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..d120360104806 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,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-15 20:47 Subject: Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: <20180315204709.1900787-1-martin.agren@gmail.com> URL: https://gitlist.dev/e/20180315204709.1900787-1-martin.agren%40gmail.com In-Reply-To: <0102016225e61cc2-0d0c05ef-b999-4bf2-88ac-db9840dd2a87-000000@eu-west-1.amazonses.com> ``` I skimmed the first four patches of this v2. It seems that patches 1 and 4 are identical to v2. Patches 2 and 3 have very straightforward changes based on my earlier comments. Let's see what this patch is about. :-) On 14 March 2018 at 20:04, Olga Telezhnaya wrote: > Finish removing any printing from ref-filter formatting logic, > so that it could be more general. > > Change the signature of get_ref_atom_value() and underlying functions > by adding return value and strbuf parameter for error message. > > It's important to mention that grab_objectname() returned 1 if > it gets objectname atom and 0 otherwise. Now this logic changed: > we return 0 if we have no error, -1 otherwise. If someone needs to > know whether it's objectname atom or not, he/she could use > starts_with() function. It duplicates this checking but it does not > sound like a really big overhead. > > Signed-off-by: Olga Telezhnaia > --- > ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 69 insertions(+), 40 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 62ea4adcd0ff1..3f0c3924273d5 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned > } > > static int grab_objectname(const char *name, const unsigned char *sha1, > - struct atom_value *v, struct used_atom *atom) > + struct atom_value *v, struct used_atom *atom, > + struct strbuf *err) > { > if (starts_with(name, "objectname")) { > if (atom->u.objectname.option == O_SHORT) { > v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV)); > - return 1; > } else if (atom->u.objectname.option == O_FULL) { > v->s = xstrdup(sha1_to_hex(sha1)); > - return 1; > } else if (atom->u.objectname.option == O_LENGTH) { > v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length)); > - return 1; > - } else > - die("BUG: unknown %%(objectname) option"); > + } else { > + strbuf_addstr(err, "BUG: unknown %(objectname) option"); > + return -1; > + } > } > return 0; > } This is interesting. This die() is never ever supposed to actually trigger, except to allow a developer adding some new O_xxx-value to quickly notice that they have forgotten to add code here. > /* See grab_values */ > -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz) > +static int grab_common_values(struct atom_value *val, int deref, struct object *obj, > + void *buf, unsigned long sz, struct strbuf *err) > { > int i; > > @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object > v->s = xstrfmt("%lu", sz); > } > else if (deref) > - grab_objectname(name, obj->oid.hash, v, &used_atom[i]); > + if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err)) > + return -1; > } > + return 0; > } So if that conversion I commented on above had not happened, this would not have been necessary. Let's read on... > /* See grab_values */ > @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val) > * pointed at by the ref itself; otherwise it is the object the > * ref (which is a tag) refers to. > */ > -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz) > +static int grab_values(struct atom_value *val, int deref, struct object *obj, > + void *buf, unsigned long sz, struct strbuf *err) > { > - grab_common_values(val, deref, obj, buf, sz); > + if (grab_common_values(val, deref, obj, buf, sz, err)) > + return -1; > switch (obj->type) { > case OBJ_TAG: > grab_tag_values(val, deref, obj, buf, sz); > @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v > /* grab_blob_values(val, deref, obj, buf, sz); */ > break; > default: > - die("Eh? Object of type %d?", obj->type); > + strbuf_addf(err, "Eh? Object of type %d?", obj->type); > + return -1; > } > + return 0; > } This seems similar. The string here is quite sloppy, and I do not believe that the author intended this to be user-visible. I believe this is more like a very short way of saying "how could we possibly get here??". It could also be written as die("BUG: unknown object type %d", obj->type), or even better: BUG(...). > static inline char *copy_advance(char *dst, const char *src) > @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname) > return refname; > } > > -static void fill_remote_ref_details(struct used_atom *atom, const char *refname, > - struct branch *branch, const char **s) > +static int fill_remote_ref_details(struct used_atom *atom, const char *refname, > + struct branch *branch, const char **s, > + struct strbuf *err) > { > int num_ours, num_theirs; > if (atom->u.remote_ref.option == RR_REF) > @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname, > } else if (atom->u.remote_ref.option == RR_TRACKSHORT) { > if (stat_tracking_info(branch, &num_ours, &num_theirs, > NULL, AHEAD_BEHIND_FULL) < 0) > - return; > + return 0; > > if (!num_ours && !num_theirs) > *s = "="; > @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname, > *s = xstrdup(merge); > else > *s = ""; > - } else > - die("BUG: unhandled RR_* enum"); > + } else { > + strbuf_addstr(err, "BUG: unhandled RR_* enum"); > + return -1; > + } > + return 0; > } This one too.. I start to think it is overkill to wire through these strbufs just to collect messages that should never ever be produced. It almost seems to me like if 1) we want to collect errors using strbufs, and 2) we want to use BUG() for these sorts of assertions, then 3) we will be wiring error-strbufs through more or less all of our code. I am exaggerating, but there is something to it: A small change deep down a callstack where you want to add a BUG() "to be safe", and you might need to wire a strbuf all the way through. According to d8193743e0 (usage.c: add BUG() function, 2017-05-12), a good thing about BUG() is that we can get a core dump, a filename and a line number. That opportunity gets lost if we do these sort of transformations. Of course, these were only die("BUG: "), not BUG(), but my point is that they should perhaps have been BUG(). > char *get_head_description(void) > @@ -1447,28 +1458,33 @@ 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; > 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); > + if (!buf) { > + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), > + ref->refname); > + return -1; > + } > + if (!*obj) { > + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), > + oid_to_hex(oid), ref->refname); > + return -1; > + } > + if (grab_values(ref->value, deref, *obj, buf, size, err)) > + return -1; > if (!eaten) > free(buf); > + return 0; > } These are "real" errors and yield several more changes in the remainder. Ignoring those BUG-type messages at the beginning of this patch would give a patch like the one below. Maybe that would be a bit less intrusive for the same gain. Thanks for working on this. I feel that your patches are really interesting. They open up many possibilities for philosophical exercises about how errors should be collected and reported. ;-) I would be interested in knowing your thoughts, and others'. Martin --- ref-filter.c | 51 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 62ea4adcd0..e41505b3c0 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1447,28 +1447,32 @@ 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; 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); - + if (!buf) { + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), + ref->refname); + return -1; + } + if (!*obj) { + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), + oid_to_hex(oid), ref->refname); + return -1; + } grab_values(ref->value, deref, *obj, buf, size); if (!eaten) free(buf); + return 0; } /* * 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; @@ -1590,16 +1594,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 @@ -1613,20 +1618,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; } /* @@ -2150,9 +2158,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); @@ -2231,7 +2243,8 @@ int format_ref_array_item(struct ref_array_item *info, append_literal(cp, sp, &state); if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf)) 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; } -- 2.16.2.246.ga4ee44448f ``` ## Eric Sunshine, 2018-03-15 21:01 Subject: Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRbsDTTfbeQS5tXCCoDKvf2pXAe_suGs-yHY_qj%3DGo_1A%40mail.gmail.com In-Reply-To: <20180315204709.1900787-1-martin.agren@gmail.com> ``` On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren wrote: > These are "real" errors and yield several more changes in the remainder. > Ignoring those BUG-type messages at the beginning of this patch would > give a patch like the one below. > > +static int get_object(struct ref_array_item *ref, const struct object_id *oid, > + int deref, struct object **obj, struct strbuf *err) > { > 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); > - > + if (!buf) { > + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), > + ref->refname); > + return -1; > + } > + if (!*obj) { > + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), > + oid_to_hex(oid), ref->refname); > + return -1; Doesn't this leak 'buf'? > + } > grab_values(ref->value, deref, *obj, buf, size); > if (!eaten) > free(buf); > + return 0; > } ``` ## Junio C Hamano, 2018-03-15 21:05 Subject: Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/xmqqd1053v8c.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20180315204709.1900787-1-martin.agren@gmail.com> ``` Martin Ågren writes: >> static int grab_objectname(const char *name, const unsigned char *sha1, >> - struct atom_value *v, struct used_atom *atom) >> + struct atom_value *v, struct used_atom *atom, >> + struct strbuf *err) >> { >> ... >> + } else { >> + strbuf_addstr(err, "BUG: unknown %(objectname) option"); >> + return -1; >> + } >> } >> return 0; >> } > > This is interesting. This die() is never ever supposed to actually > trigger, except to allow a developer adding some new O_xxx-value to > quickly notice that they have forgotten to add code here. Yup, BUG() is meant for a case like this; the original code predates the BUG("message") which in turn predates the die("BUG: message") convention, I think. >> default: >> - die("Eh? Object of type %d?", obj->type); >> + strbuf_addf(err, "Eh? Object of type %d?", obj->type); >> + return -1; >> } >> + return 0; >> } > > This seems similar. The string here is quite sloppy, and I do not > believe that the author intended this to be user-visible. I believe this > is more like a very short way of saying "how could we possibly get > here??". It could also be written as die("BUG: unknown object type %d", > obj->type), or even better: BUG(...). Likewise. ``` ## Junio C Hamano, 2018-03-15 22:48 Subject: Re: [PATCH v2 3/5] ref-filter: change parsing function error handling Message-ID: URL: https://gitlist.dev/e/xmqq8tat3qgj.fsf%40gitster-ct.c.googlers.com In-Reply-To: <0102016225e61cb4-b59773de-d22e-42cd-909a-d7ce71646898-000000@eu-west-1.amazonses.com> ``` Olga Telezhnaya writes: > Continue removing any printing from ref-filter formatting logic, > so that it could be more general. Hmm. > 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. This says what the patch changes, but it does not explain why it is a good idea to return something with different meaning to the callers (which of course forces us to update all callers so that they pass &result pointer and check the value returned in the variable), or more importantly what meaning the return value has and how the callers are expected to use it. While at it, it probably is a good idea to explain what the original return value means. The return value from parse_ref_filter_atom() used to be the position the atom is found in the used_atom[] array; that information is now returned in an integer pointed at by the *res parameter. The function now returns 0 for success and -1 for failure. or something like that. Having said that, I wonder if a calling convention that does not force callers to pass in a pointer-to-int may make more sense. Because the original return value is an index into an array, we know the normal return values are not negative. An updated caller could become like this instead: pos = parse_ref_filter_atom(format, atom, ep, &err); if (pos < 0) die("%s", err.buf); ... original code that used 'pos' can stay as before ... ``` ## Оля Тележная, 2018-03-16 07:20 Subject: Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/CAL21BmmXz7wsq9kPzbvjo3b5ZEvmZA5qi-XqoXf6X8rRwOg_aQ%40mail.gmail.com In-Reply-To: ``` 2018-03-16 0:01 GMT+03:00 Eric Sunshine : > On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren wrote: >> These are "real" errors and yield several more changes in the remainder. >> Ignoring those BUG-type messages at the beginning of this patch would >> give a patch like the one below. >> >> +static int get_object(struct ref_array_item *ref, const struct object_id *oid, >> + int deref, struct object **obj, struct strbuf *err) >> { >> 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); >> - >> + if (!buf) { >> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), >> + ref->refname); >> + return -1; >> + } >> + if (!*obj) { >> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), >> + oid_to_hex(oid), ref->refname); >> + return -1; > > Doesn't this leak 'buf'? Yes. Thanks a lot. > >> + } >> grab_values(ref->value, deref, *obj, buf, size); >> if (!eaten) >> free(buf); >> + return 0; >> } ``` ## Оля Тележная, 2018-03-16 07:17 Subject: Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/CAL21BmkipcdT6M38PDJ6%3Dkk_E5F8T_OmYOF_N097UbmzbnjESA%40mail.gmail.com In-Reply-To: <20180315204709.1900787-1-martin.agren@gmail.com> ``` 2018-03-15 23:47 GMT+03:00 Martin Ågren : > I skimmed the first four patches of this v2. It seems that patches 1 and > 4 are identical to v2. Patches 2 and 3 have very straightforward changes > based on my earlier comments. Let's see what this patch is about. :-) Yes, you are right. > > On 14 March 2018 at 20:04, Olga Telezhnaya wrote: >> Finish removing any printing from ref-filter formatting logic, >> so that it could be more general. >> >> Change the signature of get_ref_atom_value() and underlying functions >> by adding return value and strbuf parameter for error message. >> >> It's important to mention that grab_objectname() returned 1 if >> it gets objectname atom and 0 otherwise. Now this logic changed: >> we return 0 if we have no error, -1 otherwise. If someone needs to >> know whether it's objectname atom or not, he/she could use >> starts_with() function. It duplicates this checking but it does not >> sound like a really big overhead. >> >> Signed-off-by: Olga Telezhnaia >> --- >> ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 69 insertions(+), 40 deletions(-) >> >> diff --git a/ref-filter.c b/ref-filter.c >> index 62ea4adcd0ff1..3f0c3924273d5 100644 >> --- a/ref-filter.c >> +++ b/ref-filter.c >> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned >> } >> >> static int grab_objectname(const char *name, const unsigned char *sha1, >> - struct atom_value *v, struct used_atom *atom) >> + struct atom_value *v, struct used_atom *atom, >> + struct strbuf *err) >> { >> if (starts_with(name, "objectname")) { >> if (atom->u.objectname.option == O_SHORT) { >> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV)); >> - return 1; >> } else if (atom->u.objectname.option == O_FULL) { >> v->s = xstrdup(sha1_to_hex(sha1)); >> - return 1; >> } else if (atom->u.objectname.option == O_LENGTH) { >> v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length)); >> - return 1; >> - } else >> - die("BUG: unknown %%(objectname) option"); >> + } else { >> + strbuf_addstr(err, "BUG: unknown %(objectname) option"); >> + return -1; >> + } >> } >> return 0; >> } > > This is interesting. This die() is never ever supposed to actually > trigger, except to allow a developer adding some new O_xxx-value to > quickly notice that they have forgotten to add code here. Oh, cool, so I can revert this change, OK. > >> /* See grab_values */ >> -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz) >> +static int grab_common_values(struct atom_value *val, int deref, struct object *obj, >> + void *buf, unsigned long sz, struct strbuf *err) >> { >> int i; >> >> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object >> v->s = xstrfmt("%lu", sz); >> } >> else if (deref) >> - grab_objectname(name, obj->oid.hash, v, &used_atom[i]); >> + if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err)) >> + return -1; >> } >> + return 0; >> } > > So if that conversion I commented on above had not happened, this would > not have been necessary. Let's read on... Of course, I will check and also revert functions that were touched only because of other functions. > >> /* See grab_values */ >> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val) >> * pointed at by the ref itself; otherwise it is the object the >> * ref (which is a tag) refers to. >> */ >> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz) >> +static int grab_values(struct atom_value *val, int deref, struct object *obj, >> + void *buf, unsigned long sz, struct strbuf *err) >> { >> - grab_common_values(val, deref, obj, buf, sz); >> + if (grab_common_values(val, deref, obj, buf, sz, err)) >> + return -1; >> switch (obj->type) { >> case OBJ_TAG: >> grab_tag_values(val, deref, obj, buf, sz); >> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v >> /* grab_blob_values(val, deref, obj, buf, sz); */ >> break; >> default: >> - die("Eh? Object of type %d?", obj->type); >> + strbuf_addf(err, "Eh? Object of type %d?", obj->type); >> + return -1; >> } >> + return 0; >> } > > This seems similar. The string here is quite sloppy, and I do not > believe that the author intended this to be user-visible. I believe this > is more like a very short way of saying "how could we possibly get > here??". It could also be written as die("BUG: unknown object type %d", > obj->type), or even better: BUG(...). OK, thanks! > >> static inline char *copy_advance(char *dst, const char *src) >> @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname) >> return refname; >> } >> >> -static void fill_remote_ref_details(struct used_atom *atom, const char *refname, >> - struct branch *branch, const char **s) >> +static int fill_remote_ref_details(struct used_atom *atom, const char *refname, >> + struct branch *branch, const char **s, >> + struct strbuf *err) >> { >> int num_ours, num_theirs; >> if (atom->u.remote_ref.option == RR_REF) >> @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname, >> } else if (atom->u.remote_ref.option == RR_TRACKSHORT) { >> if (stat_tracking_info(branch, &num_ours, &num_theirs, >> NULL, AHEAD_BEHIND_FULL) < 0) >> - return; >> + return 0; >> >> if (!num_ours && !num_theirs) >> *s = "="; >> @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname, >> *s = xstrdup(merge); >> else >> *s = ""; >> - } else >> - die("BUG: unhandled RR_* enum"); >> + } else { >> + strbuf_addstr(err, "BUG: unhandled RR_* enum"); >> + return -1; >> + } >> + return 0; >> } > > This one too.. I start to think it is overkill to wire through these > strbufs just to collect messages that should never ever be produced. It > almost seems to me like if 1) we want to collect errors using strbufs, > and 2) we want to use BUG() for these sorts of assertions, then 3) we > will be wiring error-strbufs through more or less all of our code. I am > exaggerating, but there is something to it: A small change deep down a > callstack where you want to add a BUG() "to be safe", and you might need > to wire a strbuf all the way through. I absolutely agree that we should not touch functions that produce errors for Git developers. It still means that sometimes we need to wire strbufs, but I will revert some of them. It's enough to handle only errors for users. > > According to d8193743e0 (usage.c: add BUG() function, 2017-05-12), a > good thing about BUG() is that we can get a core dump, a filename and a > line number. That opportunity gets lost if we do these sort of > transformations. Of course, these were only die("BUG: "), not BUG(), but > my point is that they should perhaps have been BUG(). > >> char *get_head_description(void) >> @@ -1447,28 +1458,33 @@ 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; >> 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); >> + if (!buf) { >> + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), >> + ref->refname); >> + return -1; >> + } >> + if (!*obj) { >> + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), >> + oid_to_hex(oid), ref->refname); >> + return -1; >> + } >> + if (grab_values(ref->value, deref, *obj, buf, size, err)) >> + return -1; >> if (!eaten) >> free(buf); >> + return 0; >> } > > These are "real" errors and yield several more changes in the remainder. > Ignoring those BUG-type messages at the beginning of this patch would > give a patch like the one below. Maybe that would be a bit less > intrusive for the same gain. > > Thanks for working on this. I feel that your patches are really > interesting. They open up many possibilities for philosophical exercises > about how errors should be collected and reported. ;-) I would be > interested in knowing your thoughts, and others'. Thank you! I was also thinking about other ways how to handle errors. I haven't invented anything new, so at least I decided to make these changes (for now). > > Martin > > --- > ref-filter.c | 51 ++++++++++++++++++++++++++++++++------------------- > 1 file changed, 32 insertions(+), 19 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 62ea4adcd0..e41505b3c0 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1447,28 +1447,32 @@ 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; > 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); > - > + if (!buf) { > + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), > + ref->refname); > + return -1; > + } > + if (!*obj) { > + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), > + oid_to_hex(oid), ref->refname); > + return -1; > + } > grab_values(ref->value, deref, *obj, buf, size); > if (!eaten) > free(buf); > + return 0; > } > > /* > * 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; > @@ -1590,16 +1594,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 > @@ -1613,20 +1618,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; > } > > /* > @@ -2150,9 +2158,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); > @@ -2231,7 +2243,8 @@ int format_ref_array_item(struct ref_array_item *info, > append_literal(cp, sp, &state); > if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf)) > 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; > } > -- > 2.16.2.246.ga4ee44448f > ``` ## Оля Тележная, 2018-03-16 07:22 Subject: Re: [PATCH v2 3/5] ref-filter: change parsing function error handling Message-ID: URL: https://gitlist.dev/e/CAL21Bmk03kSaO8h%2BuQ9nB7DomAL0Ym%2B7jygTo7M-rv5OM2duzA%40mail.gmail.com In-Reply-To: ``` 2018-03-16 1:48 GMT+03:00 Junio C Hamano : > Olga Telezhnaya writes: > >> Continue removing any printing from ref-filter formatting logic, >> so that it could be more general. > > Hmm. > >> 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. > > This says what the patch changes, but it does not explain why it is > a good idea to return something with different meaning to the > callers (which of course forces us to update all callers so that > they pass &result pointer and check the value returned in the > variable), or more importantly what meaning the return value has and > how the callers are expected to use it. While at it, it probably is > a good idea to explain what the original return value means. > > The return value from parse_ref_filter_atom() used to be the > position the atom is found in the used_atom[] array; that > information is now returned in an integer pointed at by the > *res parameter. The function now returns 0 for success and > -1 for failure. > > or something like that. > > Having said that, I wonder if a calling convention that does not > force callers to pass in a pointer-to-int may make more sense. > Because the original return value is an index into an array, we know > the normal return values are not negative. An updated caller could > become like this instead: > > pos = parse_ref_filter_atom(format, atom, ep, &err); > if (pos < 0) > die("%s", err.buf); > ... original code that used 'pos' can stay as before ... > Martin also mentioned that, but I was not sure which solution is better. Great, thanks, I will fix that. ``` ## Olga Telezhnaya, 2018-03-19 13:01 Subject: [PATCH v3 1/5] ref-filter: start adding strbufs with errors Message-ID: <010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000%40eu-west-1.amazonses.com In-Reply-To: <0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-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 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 | 17 ++++++++++++----- ref-filter.h | 7 ++++--- 3 files changed, 21 insertions(+), 10 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..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-19 13:01 Subject: [PATCH v3 3/5] ref-filter: change parsing function error handling Message-ID: <010201623e5944b9-8d44ad36-a6df-4bd3-97e7-de4ee54af3c0-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201623e5944b9-8d44ad36-a6df-4bd3-97e7-de4ee54af3c0-000000%40eu-west-1.amazonses.com In-Reply-To: <010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-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 adding strbuf parameter for error message. Return value means the same except negative values: they indicate errors (previous version could return only non-negative value). Signed-off-by: Olga Telezhnaia --- ref-filter.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index d120360104806..2313a33f0baa4 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; @@ -406,8 +407,10 @@ 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++) { @@ -432,8 +435,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; @@ -725,17 +730,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; @@ -2154,13 +2163,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; } @@ -2215,7 +2226,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-19 13:01 Subject: [PATCH v3 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: <010201623e5944b3-9397c866-0bd8-4a03-863a-5c87a486e7b2-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201623e5944b3-9397c866-0bd8-4a03-863a-5c87a486e7b2-000000%40eu-west-1.amazonses.com In-Reply-To: <010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com> ``` Finish removing any printing from ref-filter formatting logic, so that it could be more general. Change the signature of get_ref_atom_value() and underlying functions by adding return value and strbuf parameter for error message. Some die() calls are left; all of them are not for users, but for Git developers. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 55 ++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index b90cec1056954..85f971663a4aa 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1445,28 +1445,36 @@ 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; 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); - + if (!buf) { + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), + ref->refname); + if (!eaten) + free(buf); + return -1; + } + if (!*obj) { + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), + oid_to_hex(oid), ref->refname); + if (!eaten) + free(buf); + return -1; + } grab_values(ref->value, deref, *obj, buf, size); if (!eaten) free(buf); + return 0; } /* * 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; @@ -1588,16 +1596,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 @@ -1611,20 +1620,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; } /* @@ -2148,9 +2160,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); @@ -2230,7 +2246,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-19 13:01 Subject: [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers Message-ID: <010201623e59448f-2db7a920-fe77-481a-b105-de6bbff253eb-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201623e59448f-2db7a920-fe77-481a-b105-de6bbff253eb-000000%40eu-west-1.amazonses.com In-Reply-To: <010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-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..d120360104806 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,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 ``` ## Olga Telezhnaya, 2018-03-19 13:01 Subject: [PATCH v3 4/5] ref-filter: add return value to parsers Message-ID: <010201623e5944bd-e932aceb-2d30-4940-89e1-130b99f6ed81-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201623e5944bd-e932aceb-2d30-4940-89e1-130b99f6ed81-000000%40eu-west-1.amazonses.com In-Reply-To: <010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-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 2313a33f0baa4..b90cec1056954 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" }, @@ -457,8 +516,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 ``` ## Junio C Hamano, 2018-03-19 19:24 Subject: Re: [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers Message-ID: URL: https://gitlist.dev/e/xmqqbmfjvpgm.fsf%40gitster-ct.c.googlers.com In-Reply-To: <010201623e59448f-2db7a920-fe77-481a-b105-de6bbff253eb-000000@eu-west-1.amazonses.com> ``` Olga Telezhnaya writes: > Continue removing any printing from ref-filter formatting logic, > so that it could be more general. "more general" sounds overly broad. Is this to avoid calling die() so that the caller can decide what error messages it wants to give, abort operation or not, etc.? From a quick scan of the patch, I have a feeling that "any printing" is not the problem you are solving (calling die() is). > Change the signature of handlers by adding return value > and strbuf parameter for errors. Could you explain what the "return value" means? We can see from the patch that the function signature is being changed by the patch. What needs human explanation is why and what the values mean, e.g. "to allow the caller to notice an error, the function returns 0 upon success and non-0 upon failure, and an error message is appended to the strbuf *err" (don't pay too much attention to "0" or "non-0" in this example, as I do not know what you chose to assign the return values to signal what to the callers; this is merely to illustrate the degree of details I would expect). Thanks. ``` ## Junio C Hamano, 2018-03-19 19:41 Subject: Re: [PATCH v3 3/5] ref-filter: change parsing function error handling Message-ID: URL: https://gitlist.dev/e/xmqq7eq7von9.fsf%40gitster-ct.c.googlers.com In-Reply-To: <010201623e5944b9-8d44ad36-a6df-4bd3-97e7-de4ee54af3c0-000000@eu-west-1.amazonses.com> ``` Olga Telezhnaya writes: > 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 adding > strbuf parameter for error message. > Return value means the same except negative values: they indicate > errors (previous version could return only non-negative value). "The same" meaning? The same as before? It shouldn't be too much work to describe what it means instead (and mention that you are not changing the meaning as a sidenote) to help the readers, something like "The function returns the position in the used_atom[] array (as before) for the given atom, or a negative value to signal an error". ``` ## Olga Telezhnaya, 2018-03-20 16:05 Subject: [PATCH v4 1/5] ref-filter: start adding strbufs with errors Message-ID: <01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000%40eu-west-1.amazonses.com In-Reply-To: <010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-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 | 17 ++++++++++++----- ref-filter.h | 7 ++++--- 3 files changed, 21 insertions(+), 10 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..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-20 16:05 Subject: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: <010201624428199f-b4b3d8ce-222f-4966-9171-7fcf932ca220-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201624428199f-b4b3d8ce-222f-4966-9171-7fcf932ca220-000000%40eu-west-1.amazonses.com In-Reply-To: <01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-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 (there could be more error codes further if necessary). Upon failure, error message is appended to the strbuf. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 55 ++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index b90cec1056954..85f971663a4aa 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1445,28 +1445,36 @@ 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; 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); - + if (!buf) { + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), + ref->refname); + if (!eaten) + free(buf); + return -1; + } + if (!*obj) { + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), + oid_to_hex(oid), ref->refname); + if (!eaten) + free(buf); + return -1; + } grab_values(ref->value, deref, *obj, buf, size); if (!eaten) free(buf); + return 0; } /* * 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; @@ -1588,16 +1596,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 @@ -1611,20 +1620,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; } /* @@ -2148,9 +2160,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); @@ -2230,7 +2246,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-20 16:05 Subject: [PATCH v4 3/5] ref-filter: change parsing function error handling Message-ID: <010201624428192e-dc81bfca-3acb-4c31-9d4c-1cf2adaee49d-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201624428192e-dc81bfca-3acb-4c31-9d4c-1cf2adaee49d-000000%40eu-west-1.amazonses.com In-Reply-To: <01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-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 (there could be more negative error codes further if necessary). Upon failure, error message is appended to the strbuf. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index d120360104806..2313a33f0baa4 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; @@ -406,8 +407,10 @@ 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++) { @@ -432,8 +435,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; @@ -725,17 +730,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; @@ -2154,13 +2163,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; } @@ -2215,7 +2226,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-20 16:05 Subject: [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers Message-ID: <010201624428190c-441272b2-bc7d-42af-82f8-6a805bb27f6c-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/010201624428190c-441272b2-bc7d-42af-82f8-6a805bb27f6c-000000%40eu-west-1.amazonses.com In-Reply-To: <01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-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 (there could be more error codes further if necessary). Upon failure, error message is appended to the strbuf. 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..d120360104806 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,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 ``` ## Olga Telezhnaya, 2018-03-20 16:05 Subject: [PATCH v4 4/5] ref-filter: add return value to parsers Message-ID: <0102016244281955-6f111db0-ba55-46fc-8db9-7b328fed1372-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/0102016244281955-6f111db0-ba55-46fc-8db9-7b328fed1372-000000%40eu-west-1.amazonses.com In-Reply-To: <01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-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 (there could be more error codes further if necessary). Upon failure, error message is appended to the strbuf. 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 2313a33f0baa4..b90cec1056954 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" }, @@ -457,8 +516,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 ``` ## Eric Sunshine, 2018-03-20 18:18 Subject: Re: [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers Message-ID: URL: https://gitlist.dev/e/CAPig%2BcT5jh0y9Rmw0E6ns0k5mSwaxAqdaN8oWCayCE8V%2BjYZow%40mail.gmail.com In-Reply-To: <010201624428190c-441272b2-bc7d-42af-82f8-6a805bb27f6c-000000@eu-west-1.amazonses.com> ``` On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya wrote: > Continue removing die() calls from ref-filter formatting logic, > so that it could be used by other commands. > [...] > Signed-off-by: Olga Telezhnaia > --- > 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. ``` ## Eric Sunshine, 2018-03-20 18:19 Subject: Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSxGgG%3D1k95j%2Bn4DZHBdtN%2B%2B7F6_U5fu0gOcjwwPQ5GZw%40mail.gmail.com In-Reply-To: <010201624428199f-b4b3d8ce-222f-4966-9171-7fcf932ca220-000000@eu-west-1.amazonses.com> ``` On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya wrote: > ref-filter: get_ref_atom_value() error handling This doesn't tell us much about what this patch is doing. Perhaps a better subject would be: ref-filter: libify get_ref_atom_value() > 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 (there > could be more error codes further if necessary). > Upon failure, error message is appended to the strbuf. > > Signed-off-by: Olga Telezhnaia > --- > diff --git a/ref-filter.c b/ref-filter.c > @@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re > +static int get_object(struct ref_array_item *ref, const struct object_id *oid, > + int deref, struct object **obj, struct strbuf *err) > { > 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); > - > + if (!buf) { > + strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid), > + ref->refname); > + if (!eaten) > + free(buf); Since we are inside the 'if (!buf)' conditional, we _know_ that 'buf' is NULL, therefore this free(buff) is unnecessary. > + return -1; > + } > + if (!*obj) { > + strbuf_addf(err, _("parse_object_buffer failed on %s for %s"), > + oid_to_hex(oid), ref->refname); > + if (!eaten) > + free(buf); > + return -1; > + } > grab_values(ref->value, deref, *obj, buf, size); > if (!eaten) > free(buf); > + return 0; > } Overall, with the need for resource cleanup, this function becomes unusually noisy after this change. It could be tamed by doing something like this: int ret = 0; void *buf = get_obj(oid, obj, &size, &eaten); if (!buf) ret = strbuf_error(_("missing object %s for %s"), oid_to_hex(oid), ref->refname); else if (!*obj) ret = strbuf_error(_("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; ``` ## Junio C Hamano, 2018-03-20 22:30 Subject: Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/xmqqh8paqt0f.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Eric Sunshine writes: > Overall, with the need for resource cleanup, this function becomes > unusually noisy after this change. It could be tamed by doing > something like this: > > int ret = 0; > void *buf = get_obj(oid, obj, &size, &eaten); > if (!buf) > ret = strbuf_error(_("missing object %s for %s"), > oid_to_hex(oid), ref->refname); > else if (!*obj) > ret = strbuf_error(_("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; I have no idea what strbuf_error() that does not take any strbuf is doing, but I think you can initialize ret to -1 (i.e. assume the worst at the beginning), and then make the "ok, we didn't get any errors" case do else { grab_values(...); ret = 0; } ``` ## Eric Sunshine, 2018-03-20 22:50 Subject: Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ0cq9-cYuw8AJpk6maeT-T25SJDhWpb4TyN8rQJmP%3Dxg%40mail.gmail.com In-Reply-To: ``` On Tue, Mar 20, 2018 at 6:30 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> int ret = 0; >> void *buf = get_obj(oid, obj, &size, &eaten); >> if (!buf) >> ret = strbuf_error(_("missing object %s for %s"), >> oid_to_hex(oid), ref->refname); >> else if (!*obj) >> ret = strbuf_error(_("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; > > I have no idea what strbuf_error() that does not take any strbuf is > doing,... strbuf_error() was a possibility proposed in [1], and it does take a strbuf. Failure to pass in a strbuf here is just a typo. > ... but I think you can initialize ret to -1 (i.e. assume the > worst at the beginning), and then make the "ok, we didn't get any > errors" case do > > else { > grab_values(...); > ret = 0; > } Yes, that also works. [1]: https://public-inbox.org/git/CAPig+cT5jh0y9Rmw0E6ns0k5mSwaxAqdaN8oWCayCE8V+jYZow@mail.gmail.com/ ``` ## Junio C Hamano, 2018-03-21 19:30 Subject: Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/xmqq4ll9p6oq.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Eric Sunshine writes: >> I have no idea what strbuf_error() that does not take any strbuf is >> doing,... > > strbuf_error() was a possibility proposed in [1], and it does take a > strbuf. Failure to pass in a strbuf here is just a typo. I've seen it; I just thought it was a joke and not a serious suggestion. A macro or helper function that is local to the file might be OK, but I do not think "strbuf_error()" is a useful abstraction that is generic enough in the first place (the questions to ask yourself to think about it are: Why should it be limited to return -1? Why should it be limited to always do the addf() to a strbuf?). ``` ## Eric Sunshine, 2018-03-21 19:58 Subject: Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQZOjFuAHXqj6c8oP8JJaboLf3kL9heMA%3DjkXssr%2BgQ_Q%40mail.gmail.com In-Reply-To: ``` On Wed, Mar 21, 2018 at 3:30 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> strbuf_error() was a possibility proposed in [1], and it does take a >> strbuf. Failure to pass in a strbuf here is just a typo. > > I've seen it; I just thought it was a joke and not a serious > suggestion. > A macro or helper function that is local to the file might be OK, My thought all along was that this convenience helper (in whatever form) should start life local to ref-filter.c, and only be published more widely if found to be generally useful. Unfortunately, I forgot to state so explicitly when writing the review. Suggesting the name strbuf_error() didn't help to convey that thought either. My bad. > but I do not think "strbuf_error()" is a useful abstraction that is > generic enough in the first place (the questions to ask yourself to > think about it are: Why should it be limited to return -1? Why > should it be limited to always do the addf() to a strbuf?). There is some precedent in the existing error() function. As with error(), as a _convenience_ function, it does not necessarily have to be universally general. That it simplifies a reasonably large body of code may be justification enough, despite it shortcomings. I don't feel strongly about it, though, and, as noted above, agree that it can be local to ref-filter.c. ```