{"thread":{"id":"48044","subject":"[PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling","startedAt":"2018-03-14T19:04:30Z","lastAt":"2018-03-21T19:58:42Z","messageCount":30,"participants":["Olga Telezhnaya","Martin Ågren","Eric Sunshine","Junio C Hamano","Оля Тележная"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"341687","messageId":"0102016225e61cc2-0d0c05ef-b999-4bf2-88ac-db9840dd2a87-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com","subject":"[PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-14T19:04:20Z","receivedAt":"2018-03-14T19:04:30Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Finish removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of get_ref_atom_value() and underlying functions\nby adding return value and strbuf parameter for error message.\n\nIt's important to mention that grab_objectname() returned 1 if\nit gets objectname atom and 0 otherwise. Now this logic changed:\nwe return 0 if we have no error, -1 otherwise. If someone needs to\nknow whether it's objectname atom or not, he/she could use\nstarts_with() function. It duplicates this checking but it does not\nsound like a really big overhead.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 69 insertions(+), 40 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 62ea4adcd0ff1..3f0c3924273d5 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned\n }\n \n static int grab_objectname(const char *name, const unsigned char *sha1,\n-\t\t\t struct atom_value *v, struct used_atom *atom)\n+\t\t\t struct atom_value *v, struct used_atom *atom,\n+\t\t\t struct strbuf *err)\n {\n \tif (starts_with(name, \"objectname\")) {\n \t\tif (atom->u.objectname.option == O_SHORT) {\n \t\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n-\t\t\treturn 1;\n \t\t} else if (atom->u.objectname.option == O_FULL) {\n \t\t\tv->s = xstrdup(sha1_to_hex(sha1));\n-\t\t\treturn 1;\n \t\t} else if (atom->u.objectname.option == O_LENGTH) {\n \t\t\tv->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));\n-\t\t\treturn 1;\n-\t\t} else\n-\t\t\tdie(\"BUG: unknown %%(objectname) option\");\n+\t\t} else {\n+\t\t\tstrbuf_addstr(err, \"BUG: unknown %(objectname) option\");\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \treturn 0;\n }\n \n /* See grab_values */\n-static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n+static int grab_common_values(struct atom_value *val, int deref, struct object *obj,\n+\t\t\t void *buf, unsigned long sz, struct strbuf *err)\n {\n \tint i;\n \n@@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \t\t\tv->s = xstrfmt(\"%lu\", sz);\n \t\t}\n \t\telse if (deref)\n-\t\t\tgrab_objectname(name, obj->oid.hash, v, &used_atom[i]);\n+\t\t\tif (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err))\n+\t\t\t\treturn -1;\n \t}\n+\treturn 0;\n }\n \n /* See grab_values */\n@@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)\n * pointed at by the ref itself; otherwise it is the object the\n * ref (which is a tag) refers to.\n */\n-static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n+static int grab_values(struct atom_value *val, int deref, struct object *obj,\n+\t\t void *buf, unsigned long sz, struct strbuf *err)\n {\n-\tgrab_common_values(val, deref, obj, buf, sz);\n+\tif (grab_common_values(val, deref, obj, buf, sz, err))\n+\t\treturn -1;\n \tswitch (obj->type) {\n \tcase OBJ_TAG:\n \t\tgrab_tag_values(val, deref, obj, buf, sz);\n@@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v\n \t\t/* grab_blob_values(val, deref, obj, buf, sz); */\n \t\tbreak;\n \tdefault:\n-\t\tdie(\"Eh? Object of type %d?\", obj->type);\n+\t\tstrbuf_addf(err, \"Eh? Object of type %d?\", obj->type);\n+\t\treturn -1;\n \t}\n+\treturn 0;\n }\n \n static inline char *copy_advance(char *dst, const char *src)\n@@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)\n \t\treturn refname;\n }\n \n-static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n-\t\t\t\t struct branch *branch, const char **s)\n+static int fill_remote_ref_details(struct used_atom *atom, const char *refname,\n+\t\t\t\t struct branch *branch, const char **s,\n+\t\t\t\t struct strbuf *err)\n {\n \tint num_ours, num_theirs;\n \tif (atom->u.remote_ref.option == RR_REF)\n@@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {\n \t\tif (stat_tracking_info(branch, &num_ours, &num_theirs,\n \t\t\t\t NULL, AHEAD_BEHIND_FULL) < 0)\n-\t\t\treturn;\n+\t\t\treturn 0;\n \n \t\tif (!num_ours && !num_theirs)\n \t\t\t*s = \"=\";\n@@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\t\t*s = xstrdup(merge);\n \t\telse\n \t\t\t*s = \"\";\n-\t} else\n-\t\tdie(\"BUG: unhandled RR_* enum\");\n+\t} else {\n+\t\tstrbuf_addstr(err, \"BUG: unhandled RR_* enum\");\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n char *get_head_description(void)\n@@ -1447,28 +1458,33 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n \treturn show_ref(&atom->u.refname, ref->refname);\n }\n \n-static void get_object(struct ref_array_item *ref, const struct object_id *oid,\n-\t\t int deref, struct object **obj)\n+static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n+\t\t int deref, struct object **obj, struct strbuf *err)\n {\n \tint eaten;\n \tunsigned long size;\n \tvoid *buf = get_obj(oid, obj, &size, &eaten);\n-\tif (!buf)\n-\t\tdie(_(\"missing object %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\tif (!*obj)\n-\t\tdie(_(\"parse_object_buffer failed on %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\n-\tgrab_values(ref->value, deref, *obj, buf, size);\n+\tif (!buf) {\n+\t\tstrbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n+\t\t\t ref->refname);\n+\t\treturn -1;\n+\t}\n+\tif (!*obj) {\n+\t\tstrbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n+\t\t\t oid_to_hex(oid), ref->refname);\n+\t\treturn -1;\n+\t}\n+\tif (grab_values(ref->value, deref, *obj, buf, size, err))\n+\t\treturn -1;\n \tif (!eaten)\n \t\tfree(buf);\n+\treturn 0;\n }\n \n /*\n * Parse the object referred by ref, and grab needed value.\n */\n-static void populate_value(struct ref_array_item *ref)\n+static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n {\n \tstruct object *obj;\n \tint i;\n@@ -1513,8 +1529,9 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tbranch = branch_get(branch_name);\n \n \t\t\trefname = branch_get_upstream(branch, NULL);\n-\t\t\tif (refname)\n-\t\t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n+\t\t\tif (refname &&\n+\t\t\t fill_remote_ref_details(atom, refname, branch, &v->s, err))\n+\t\t\t\treturn -1;\n \t\t\tcontinue;\n \t\t} else if (atom->u.remote_ref.push) {\n \t\t\tconst char *branch_name;\n@@ -1530,7 +1547,8 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tif (!refname)\n \t\t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tfill_remote_ref_details(atom, refname, branch, &v->s);\n+\t\t\tif (fill_remote_ref_details(atom, refname, branch, &v->s, err))\n+\t\t\t\treturn -1;\n \t\t\tcontinue;\n \t\t} else if (starts_with(name, \"color:\")) {\n \t\t\tv->s = atom->u.color;\n@@ -1548,7 +1566,9 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tv->s = xstrdup(buf + 1);\n \t\t\t}\n \t\t\tcontinue;\n-\t\t} else if (!deref && grab_objectname(name, ref->objectname.hash, v, atom)) {\n+\t\t} else if (!deref && starts_with(name, \"objectname\")) {\n+\t\t\tif (grab_objectname(name, ref->objectname.hash, v, atom, err))\n+\t\t\t\treturn -1;\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"HEAD\")) {\n \t\t\tif (atom->u.head && !strcmp(ref->refname, atom->u.head))\n@@ -1590,16 +1610,17 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tbreak;\n \t}\n \tif (used_atom_cnt <= i)\n-\t\treturn;\n+\t\treturn 0;\n \n-\tget_object(ref, &ref->objectname, 0, &obj);\n+\tif (get_object(ref, &ref->objectname, 0, &obj, err))\n+\t\treturn -1;\n \n \t/*\n \t * If there is no atom that wants to know about tagged\n \t * object, we are done.\n \t */\n \tif (!need_tagged || (obj->type != OBJ_TAG))\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If it is a tag object, see if we use a value that derefs\n@@ -1613,20 +1634,23 @@ static void populate_value(struct ref_array_item *ref)\n \t * is not consistent with what deref_tag() does\n \t * which peels the onion to the core.\n \t */\n-\tget_object(ref, tagged, 1, &obj);\n+\treturn get_object(ref, tagged, 1, &obj, err);\n }\n \n /*\n * Given a ref, return the value for the atom. This lazily gets value\n * out of the object by calling populate value.\n */\n-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)\n+static int get_ref_atom_value(struct ref_array_item *ref, int atom,\n+\t\t\t struct atom_value **v, struct strbuf *err)\n {\n \tif (!ref->value) {\n-\t\tpopulate_value(ref);\n+\t\tif (populate_value(ref, err))\n+\t\t\treturn -1;\n \t\tfill_missing_values(ref->value);\n \t}\n \t*v = &ref->value[atom];\n+\treturn 0;\n }\n \n /*\n@@ -2150,9 +2174,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tint cmp;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tint (*cmp_fn)(const char *, const char *);\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tget_ref_atom_value(a, s->atom, &va);\n-\tget_ref_atom_value(b, s->atom, &vb);\n+\tif (get_ref_atom_value(a, s->atom, &va, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tif (get_ref_atom_value(b, s->atom, &vb, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n \tif (s->version)\n \t\tcmp = versioncmp(va->s, vb->s);\n@@ -2231,7 +2259,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t\tappend_literal(cp, sp, &state);\n \t\tif (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))\n \t\t\treturn -1;\n-\t\tget_ref_atom_value(info, pos, &atomv);\n+\t\tif (get_ref_atom_value(info, pos, &atomv, error_buf))\n+\t\t\treturn -1;\n \t\tif (atomv->handler(atomv, &state, error_buf))\n \t\t\treturn -1;\n \t}\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"341688","messageId":"0102016225e61cb4-b59773de-d22e-42cd-909a-d7ce71646898-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com","subject":"[PATCH v2 3/5] ref-filter: change parsing function error handling","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-14T19:04:20Z","receivedAt":"2018-03-14T19:04:33Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of parse_ref_filter_atom() by changing return value,\nadding previous return value to function parameter and also adding\nstrbuf parameter for error message.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 43 ++++++++++++++++++++++++++++++-------------\n 1 file changed, 30 insertions(+), 13 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d120360104806..dd83ef326511d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -397,7 +397,8 @@ struct atom_value {\n * Used to parse format string and sort specifiers\n */\n static int parse_ref_filter_atom(const struct ref_format *format,\n-\t\t\t\t const char *atom, const char *ep)\n+\t\t\t\t const char *atom, const char *ep, int *res,\n+\t\t\t\t struct strbuf *err)\n {\n \tconst char *sp;\n \tconst char *arg;\n@@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tsp = atom;\n \tif (*sp == '*' && sp < ep)\n \t\tsp++; /* deref */\n-\tif (ep <= sp)\n-\t\tdie(_(\"malformed field name: %.*s\"), (int)(ep-atom), atom);\n+\tif (ep <= sp) {\n+\t\tstrbuf_addf(err, _(\"malformed field name: %.*s\"), (int)(ep-atom), atom);\n+\t\treturn -1;\n+\t}\n \n \t/* Do we have the atom already used elsewhere? */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n \t\tint len = strlen(used_atom[i].name);\n-\t\tif (len == ep - atom && !memcmp(used_atom[i].name, atom, len))\n-\t\t\treturn i;\n+\t\tif (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) {\n+\t\t\t*res = i;\n+\t\t\treturn 0;\n+\t\t}\n \t}\n \n \t/*\n@@ -432,8 +437,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\t\tbreak;\n \t}\n \n-\tif (ARRAY_SIZE(valid_atom) <= i)\n-\t\tdie(_(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n+\tif (ARRAY_SIZE(valid_atom) <= i) {\n+\t\tstrbuf_addf(err, _(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n+\t\treturn -1;\n+\t}\n \n \t/* Add it in, including the deref prefix */\n \tat = used_atom_cnt;\n@@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n \t\tneed_symref = 1;\n-\treturn at;\n+\t*res = at;\n+\treturn 0;\n }\n \n static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n@@ -725,17 +733,20 @@ int verify_ref_format(struct ref_format *format)\n \n \tformat->need_color_reset_at_eol = 0;\n \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n \t\tconst char *color, *ep = strchr(sp, ')');\n \t\tint at;\n \n \t\tif (!ep)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n-\t\tat = parse_ref_filter_atom(format, sp + 2, ep);\n+\t\tif (parse_ref_filter_atom(format, sp + 2, ep, &at, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t\tcp = ep + 1;\n \n \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n \t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n+\t\tstrbuf_release(&err);\n \t}\n \tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n \t\tformat->need_color_reset_at_eol = 0;\n@@ -2154,13 +2165,14 @@ int format_ref_array_item(struct ref_array_item *info,\n \n \tfor (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n+\t\tint pos;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n-\t\tget_ref_atom_value(info,\n-\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n-\t\t\t\t &atomv);\n+\t\tif (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))\n+\t\t\treturn -1;\n+\t\tget_ref_atom_value(info, pos, &atomv);\n \t\tif (atomv->handler(atomv, &state, error_buf))\n \t\t\treturn -1;\n \t}\n@@ -2215,7 +2227,12 @@ static int parse_sorting_atom(const char *atom)\n \t */\n \tstruct ref_format dummy = REF_FORMAT_INIT;\n \tconst char *end = atom + strlen(atom);\n-\treturn parse_ref_filter_atom(&dummy, atom, end);\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint res;\n+\tif (parse_ref_filter_atom(&dummy, atom, end, &res, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\treturn res;\n }\n \n /* If no sorting option is given, use refname to sort as default */\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"341689","messageId":"0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"010201621edc97a4-a4239b1e-86d2-4400-aaac-d81727710f4f-000000@eu-west-1.amazonses.com","subject":"[PATCH v2 1/5] ref-filter: start adding strbufs with errors","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-14T19:04:20Z","receivedAt":"2018-03-14T19:04:44Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"This is a first step in removing any printing from\nref-filter formatting logic, so that it could be more general.\nEverything would be the same for show_ref_array_item() users.\nBut, if you want to deal with errors by your own, you could invoke\nformat_ref_array_item(). It means that you need to print everything\n(the result and errors) on your side.\n\nThis commit changes signature of format_ref_array_item() by adding\nreturn value and strbuf parameter for errors, and adjusts\nits callers. While at it, reduce the scope of the out-variable.\n\nSigned-off-by: Olga Telezhnaia \n---\n builtin/branch.c | 7 +++++--\n ref-filter.c | 17 ++++++++++++-----\n ref-filter.h | 7 ++++---\n 3 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 8dcc2ed058be6..f86709ca42d5e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tstruct ref_array array;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n-\tstruct strbuf out = STRBUF_INIT;\n \tchar *to_free = NULL;\n \n \t/*\n@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tref_array_sort(sorting, &array);\n \n \tfor (i = 0; i < array.nr; i++) {\n-\t\tformat_ref_array_item(array.items[i], format, &out);\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tif (format_ref_array_item(array.items[i], format, &out, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t\tif (column_active(colopts)) {\n \t\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n \t\t\t /* format to a string_list to let print_columns() do its job */\n@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n \t\t\tputchar('\\n');\n \t\t}\n+\t\tstrbuf_release(&err);\n \t\tstrbuf_release(&out);\n \t}\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex 45fc56216aaa8..54fae00bdd410 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \n-void format_ref_array_item(struct ref_array_item *info,\n+int format_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format,\n-\t\t\t struct strbuf *final_buf)\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf)\n {\n \tconst char *cp, *sp, *ep;\n \tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n@@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\tresetv.s = GIT_COLOR_RESET;\n \t\tappend_atom(&resetv, &state);\n \t}\n-\tif (state.stack->prev)\n-\t\tdie(_(\"format: %%(end) atom missing\"));\n+\tif (state.stack->prev) {\n+\t\tstrbuf_addstr(error_buf, _(\"format: %(end) atom missing\"));\n+\t\treturn -1;\n+\t}\n \tstrbuf_addbuf(final_buf, &state.stack->output);\n \tpop_stack_element(&state.stack);\n+\treturn 0;\n }\n \n void show_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format)\n {\n \tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct strbuf error_buf = STRBUF_INIT;\n \n-\tformat_ref_array_item(info, format, &final_buf);\n+\tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n+\t\tdie(\"%s\", error_buf.buf);\n \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\tstrbuf_release(&error_buf);\n \tstrbuf_release(&final_buf);\n \tputchar('\\n');\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 0d98342b34319..e13f8e6f8721a 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n /* Based on the given format and quote_style, fill the strbuf */\n-void format_ref_array_item(struct ref_array_item *info,\n-\t\t\t const struct ref_format *format,\n-\t\t\t struct strbuf *final_buf);\n+int format_ref_array_item(struct ref_array_item *info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf);\n /* Print the ref using the given format and quote_style */\n void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n /* Parse a single sort specifier and add it to the list */\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"341690","messageId":"0102016225e61cc8-a13a29f9-605e-4703-bb87-5c2d6829f0aa-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com","subject":"[PATCH v2 4/5] ref-filter: add return value to parsers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-14T19:04:20Z","receivedAt":"2018-03-14T19:04:52Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of parsers by adding return value and\nstrbuf parameter for error message.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 177 +++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 118 insertions(+), 59 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex dd83ef326511d..62ea4adcd0ff1 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -101,22 +101,28 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n \n-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)\n+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *color_value, struct strbuf *err)\n {\n-\tif (!color_value)\n-\t\tdie(_(\"expected format: %%(color:)\"));\n-\tif (color_parse(color_value, atom->u.color) < 0)\n-\t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n+\tif (!color_value) {\n+\t\tstrbuf_addstr(err, _(\"expected format: %(color:)\"));\n+\t\treturn -1;\n+\t}\n+\tif (color_parse(color_value, atom->u.color) < 0) {\n+\t\tstrbuf_addf(err, _(\"unrecognized color: %%(color:%s)\"), color_value);\n+\t\treturn -1;\n+\t}\n \t/*\n \t * We check this after we've parsed the color, which lets us complain\n \t * about syntactically bogus color names even if they won't be used.\n \t */\n \tif (!want_color(format->use_color))\n \t\tcolor_parse(\"\", atom->u.color);\n+\treturn 0;\n }\n \n-static void refname_atom_parser_internal(struct refname_atom *atom,\n-\t\t\t\t\t const char *arg, const char *name)\n+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,\n+\t\t\t\t\t const char *name, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->option = R_NORMAL;\n@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct refname_atom *atom,\n \telse if (skip_prefix(arg, \"lstrip=\", &arg) ||\n \t\t skip_prefix(arg, \"strip=\", &arg)) {\n \t\tatom->option = R_LSTRIP;\n-\t\tif (strtol_i(arg, 10, &atom->lstrip))\n-\t\t\tdie(_(\"Integer value expected refname:lstrip=%s\"), arg);\n+\t\tif (strtol_i(arg, 10, &atom->lstrip)) {\n+\t\t\tstrbuf_addf(err, _(\"Integer value expected refname:lstrip=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n \t} else if (skip_prefix(arg, \"rstrip=\", &arg)) {\n \t\tatom->option = R_RSTRIP;\n-\t\tif (strtol_i(arg, 10, &atom->rstrip))\n-\t\t\tdie(_(\"Integer value expected refname:rstrip=%s\"), arg);\n-\t} else\n-\t\tdie(_(\"unrecognized %%(%s) argument: %s\"), name, arg);\n+\t\tif (strtol_i(arg, 10, &atom->rstrip)) {\n+\t\t\tstrbuf_addf(err, _(\"Integer value expected refname:rstrip=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(%s) argument: %s\"), name, arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\t const char *arg, struct strbuf *err)\n {\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_\n \n \tif (!arg) {\n \t\tatom->u.remote_ref.option = RR_REF;\n-\t\trefname_atom_parser_internal(&atom->u.remote_ref.refname,\n-\t\t\t\t\t arg, atom->name);\n-\t\treturn;\n+\t\treturn refname_atom_parser_internal(&atom->u.remote_ref.refname,\n+\t\t\t\t\t\t arg, atom->name, err);\n \t}\n \n \tatom->u.remote_ref.nobracket = 0;\n@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_\n \t\t\tatom->u.remote_ref.push_remote = 1;\n \t\t} else {\n \t\t\tatom->u.remote_ref.option = RR_REF;\n-\t\t\trefname_atom_parser_internal(&atom->u.remote_ref.refname,\n-\t\t\t\t\t\t arg, atom->name);\n+\t\t\tif (refname_atom_parser_internal(&atom->u.remote_ref.refname,\n+\t\t\t\t\t\t\t arg, atom->name, err))\n+\t\t\t\treturn -1;\n \t\t}\n \t}\n \n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\tif (arg)\n-\t\tdie(_(\"%%(body) does not take arguments\"));\n+\tif (arg) {\n+\t\tstrbuf_addstr(err, _(\"%(body) does not take arguments\"));\n+\t\treturn -1;\n+\t}\n \tatom->u.contents.option = C_BODY_DEP;\n+\treturn 0;\n }\n \n-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\tif (arg)\n-\t\tdie(_(\"%%(subject) does not take arguments\"));\n+\tif (arg) {\n+\t\tstrbuf_addstr(err, _(\"%(subject) does not take arguments\"));\n+\t\treturn -1;\n+\t}\n \tatom->u.contents.option = C_SUB;\n+\treturn 0;\n }\n \n-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n {\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n@@ -205,15 +229,19 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at\n \t\t\t\tatom->u.contents.trailer_opts.unfold = 1;\n \t\t\telse if (!strcmp(s, \"only\"))\n \t\t\t\tatom->u.contents.trailer_opts.only_trailers = 1;\n-\t\t\telse\n-\t\t\t\tdie(_(\"unknown %%(trailers) argument: %s\"), s);\n+\t\t\telse {\n+\t\t\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t}\n \tatom->u.contents.option = C_TRAILERS;\n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->u.contents.option = C_BARE;\n@@ -225,16 +253,23 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at\n \t\tatom->u.contents.option = C_SUB;\n \telse if (skip_prefix(arg, \"trailers\", &arg)) {\n \t\tskip_prefix(arg, \":\", &arg);\n-\t\ttrailers_atom_parser(format, atom, *arg ? arg : NULL);\n+\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n+\t\t\treturn -1;\n \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n \t\tatom->u.contents.option = C_LINES;\n-\t\tif (strtoul_ui(arg, 10, &atom->u.contents.nlines))\n-\t\t\tdie(_(\"positive value expected contents:lines=%s\"), arg);\n-\t} else\n-\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), arg);\n+\t\tif (strtoul_ui(arg, 10, &atom->u.contents.nlines)) {\n+\t\t\tstrbuf_addf(err, _(\"positive value expected contents:lines=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(contents) argument: %s\"), arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\t const char *arg, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->u.objectname.option = O_FULL;\n@@ -243,17 +278,23 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_\n \telse if (skip_prefix(arg, \"short=\", &arg)) {\n \t\tatom->u.objectname.option = O_LENGTH;\n \t\tif (strtoul_ui(arg, 10, &atom->u.objectname.length) ||\n-\t\t atom->u.objectname.length == 0)\n-\t\t\tdie(_(\"positive value expected objectname:short=%s\"), arg);\n+\t\t atom->u.objectname.length == 0) {\n+\t\t\tstrbuf_addf(err, _(\"positive value expected objectname:short=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n \t\tif (atom->u.objectname.length < MINIMUM_ABBREV)\n \t\t\tatom->u.objectname.length = MINIMUM_ABBREV;\n-\t} else\n-\t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), arg);\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(objectname) argument: %s\"), arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\trefname_atom_parser_internal(&atom->u.refname, arg, atom->name);\n+\treturn refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);\n }\n \n static align_type parse_align_position(const char *s)\n@@ -267,15 +308,18 @@ static align_type parse_align_position(const char *s)\n \treturn -1;\n }\n \n-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tstruct align *align = &atom->u.align;\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n \tunsigned int width = ~0U;\n \n-\tif (!arg)\n-\t\tdie(_(\"expected format: %%(align:,)\"));\n+\tif (!arg) {\n+\t\tstrbuf_addstr(err, _(\"expected format: %(align:,)\"));\n+\t\treturn -1;\n+\t}\n \n \talign->position = ALIGN_LEFT;\n \n@@ -286,49 +330,64 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom\n \n \t\tif (skip_prefix(s, \"position=\", &s)) {\n \t\t\tposition = parse_align_position(s);\n-\t\t\tif (position < 0)\n-\t\t\t\tdie(_(\"unrecognized position:%s\"), s);\n+\t\t\tif (position < 0) {\n+\t\t\t\tstrbuf_addf(err, _(\"unrecognized position:%s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\talign->position = position;\n \t\t} else if (skip_prefix(s, \"width=\", &s)) {\n-\t\t\tif (strtoul_ui(s, 10, &width))\n-\t\t\t\tdie(_(\"unrecognized width:%s\"), s);\n+\t\t\tif (strtoul_ui(s, 10, &width)) {\n+\t\t\t\tstrbuf_addf(err, _(\"unrecognized width:%s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t} else if (!strtoul_ui(s, 10, &width))\n \t\t\t;\n \t\telse if ((position = parse_align_position(s)) >= 0)\n \t\t\talign->position = position;\n-\t\telse\n-\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s);\n+\t\telse {\n+\t\t\tstrbuf_addf(err, _(\"unrecognized %%(align) argument: %s\"), s);\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \n-\tif (width == ~0U)\n-\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n+\tif (width == ~0U) {\n+\t\tstrbuf_addstr(err, _(\"positive width expected with the %(align) atom\"));\n+\t\treturn -1;\n+\t}\n \talign->width = width;\n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tif (!arg) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_NONE;\n-\t\treturn;\n+\t\treturn 0;\n \t} else if (skip_prefix(arg, \"equals=\", &atom->u.if_then_else.str)) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_EQUAL;\n \t} else if (skip_prefix(arg, \"notequals=\", &atom->u.if_then_else.str)) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;\n \t} else {\n-\t\tdie(_(\"unrecognized %%(if) argument: %s\"), arg);\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(if) argument: %s\"), arg);\n+\t\treturn -1;\n \t}\n+\treturn 0;\n }\n \n-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tatom->u.head = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, NULL, NULL);\n+\treturn 0;\n }\n \n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n-\tvoid (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);\n+\tint (*parser)(const struct ref_format *format, struct used_atom *atom,\n+\t\t const char *arg, struct strbuf *err);\n } valid_atom[] = {\n \t{ \"refname\" , FIELD_STR, refname_atom_parser },\n \t{ \"objecttype\" },\n@@ -459,8 +518,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\t}\n \t}\n \tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n-\tif (valid_atom[i].parser)\n-\t\tvalid_atom[i].parser(format, &used_atom[at], arg);\n+\tif (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))\n+\t\treturn -1;\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"341691","messageId":"0102016225e61cc3-67840882-64b6-4a44-8199-4384fea48850-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com","subject":"[PATCH v2 2/5] ref-filter: add return value && strbuf to handlers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-14T19:04:20Z","receivedAt":"2018-03-14T19:04:55Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of handlers by adding return value\nand strbuf parameter for errors.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 71 ++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 48 insertions(+), 23 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 54fae00bdd410..d120360104806 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -387,7 +387,8 @@ struct ref_formatting_state {\n \n struct atom_value {\n \tconst char *s;\n-\tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n+\tint (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t struct strbuf *err);\n \tuintmax_t value; /* used for sorting when not FIELD_STR */\n \tstruct used_atom *atom;\n };\n@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n \t}\n }\n \n-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,\n+\t\t struct strbuf *unused_err)\n {\n \t/*\n \t * Quote formatting is only done when the stack has a single\n@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state\n \t\tquote_formatting(&state->stack->output, v->s, state->quote_style);\n \telse\n \t\tstrbuf_addstr(&state->stack->output, v->s);\n+\treturn 0;\n }\n \n static void push_stack_element(struct ref_formatting_stack **stack)\n@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)\n \tstrbuf_release(&s);\n }\n \n-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *unused_err)\n {\n \tstruct ref_formatting_stack *new_stack;\n \n@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n \tnew_stack = state->stack;\n \tnew_stack->at_end = end_align_handler;\n \tnew_stack->at_end_data = &atomv->atom->u.align;\n+\treturn 0;\n }\n \n static void if_then_else_handler(struct ref_formatting_stack **stack)\n@@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)\n \tfree(if_then_else);\n }\n \n-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *unused_err)\n {\n \tstruct ref_formatting_stack *new_stack;\n \tstruct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);\n@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat\n \tnew_stack = state->stack;\n \tnew_stack->at_end = if_then_else_handler;\n \tnew_stack->at_end_data = if_then_else;\n+\treturn 0;\n }\n \n static int is_empty(const char *s)\n@@ -596,19 +603,24 @@ static int is_empty(const char *s)\n \treturn 1;\n }\n \n-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *cur = state->stack;\n \tstruct if_then_else *if_then_else = NULL;\n \n \tif (cur->at_end == if_then_else_handler)\n \t\tif_then_else = (struct if_then_else *)cur->at_end_data;\n-\tif (!if_then_else)\n-\t\tdie(_(\"format: %%(then) atom used without an %%(if) atom\"));\n-\tif (if_then_else->then_atom_seen)\n-\t\tdie(_(\"format: %%(then) atom used more than once\"));\n-\tif (if_then_else->else_atom_seen)\n-\t\tdie(_(\"format: %%(then) atom used after %%(else)\"));\n+\tif (!if_then_else) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used without an %(if) atom\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->then_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used more than once\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->else_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used after %(else)\"));\n+\t\treturn -1;\n+\t}\n \tif_then_else->then_atom_seen = 1;\n \t/*\n \t * If the 'equals' or 'notequals' attribute is used then\n@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st\n \t} else if (cur->output.len && !is_empty(cur->output.buf))\n \t\tif_then_else->condition_satisfied = 1;\n \tstrbuf_reset(&cur->output);\n+\treturn 0;\n }\n \n-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *prev = state->stack;\n \tstruct if_then_else *if_then_else = NULL;\n \n \tif (prev->at_end == if_then_else_handler)\n \t\tif_then_else = (struct if_then_else *)prev->at_end_data;\n-\tif (!if_then_else)\n-\t\tdie(_(\"format: %%(else) atom used without an %%(if) atom\"));\n-\tif (!if_then_else->then_atom_seen)\n-\t\tdie(_(\"format: %%(else) atom used without a %%(then) atom\"));\n-\tif (if_then_else->else_atom_seen)\n-\t\tdie(_(\"format: %%(else) atom used more than once\"));\n+\tif (!if_then_else) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used without an %(if) atom\"));\n+\t\treturn -1;\n+\t} else if (!if_then_else->then_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used without a %(then) atom\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->else_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used more than once\"));\n+\t\treturn -1;\n+\t}\n \tif_then_else->else_atom_seen = 1;\n \tpush_stack_element(&state->stack);\n \tstate->stack->at_end_data = prev->at_end_data;\n \tstate->stack->at_end = prev->at_end;\n+\treturn 0;\n }\n \n-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *current = state->stack;\n \tstruct strbuf s = STRBUF_INIT;\n \n-\tif (!current->at_end)\n-\t\tdie(_(\"format: %%(end) atom used without corresponding atom\"));\n+\tif (!current->at_end) {\n+\t\tstrbuf_addstr(err, _(\"format: %(end) atom used without corresponding atom\"));\n+\t\treturn -1;\n+\t}\n \tcurrent->at_end(&state->stack);\n \n \t/* Stack may have been popped within at_end(), hence reset the current pointer */\n@@ -668,6 +690,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\n \t}\n \tstrbuf_release(&s);\n \tpop_stack_element(&state->stack);\n+\treturn 0;\n }\n \n /*\n@@ -2138,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tget_ref_atom_value(info,\n \t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n \t\t\t\t &atomv);\n-\t\tatomv->handler(atomv, &state);\n+\t\tif (atomv->handler(atomv, &state, error_buf))\n+\t\t\treturn -1;\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n@@ -2147,7 +2171,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \tif (format->need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n \t\tresetv.s = GIT_COLOR_RESET;\n-\t\tappend_atom(&resetv, &state);\n+\t\tif (append_atom(&resetv, &state, error_buf))\n+\t\t\treturn -1;\n \t}\n \tif (state.stack->prev) {\n \t\tstrbuf_addstr(error_buf, _(\"format: %(end) atom missing\"));\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"341831","messageId":"20180315204709.1900787-1-martin.agren@gmail.com","threadId":"48044","inReplyTo":"0102016225e61cc2-0d0c05ef-b999-4bf2-88ac-db9840dd2a87-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-03-15T20:47:09Z","receivedAt":"2018-03-15T20:47:24Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"I skimmed the first four patches of this v2. It seems that patches 1 and\n4 are identical to v2. Patches 2 and 3 have very straightforward changes\nbased on my earlier comments. Let's see what this patch is about. :-)\n\nOn 14 March 2018 at 20:04, Olga Telezhnaya wrote:\n> Finish removing any printing from ref-filter formatting logic,\n> so that it could be more general.\n>\n> Change the signature of get_ref_atom_value() and underlying functions\n> by adding return value and strbuf parameter for error message.\n>\n> It's important to mention that grab_objectname() returned 1 if\n> it gets objectname atom and 0 otherwise. Now this logic changed:\n> we return 0 if we have no error, -1 otherwise. If someone needs to\n> know whether it's objectname atom or not, he/she could use\n> starts_with() function. It duplicates this checking but it does not\n> sound like a really big overhead.\n>\n> Signed-off-by: Olga Telezhnaia \n> ---\n> ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++----------------------\n> 1 file changed, 69 insertions(+), 40 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 62ea4adcd0ff1..3f0c3924273d5 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned\n> }\n>\n> static int grab_objectname(const char *name, const unsigned char *sha1,\n> - struct atom_value *v, struct used_atom *atom)\n> + struct atom_value *v, struct used_atom *atom,\n> + struct strbuf *err)\n> {\n> if (starts_with(name, \"objectname\")) {\n> if (atom->u.objectname.option == O_SHORT) {\n> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n> - return 1;\n> } else if (atom->u.objectname.option == O_FULL) {\n> v->s = xstrdup(sha1_to_hex(sha1));\n> - return 1;\n> } else if (atom->u.objectname.option == O_LENGTH) {\n> v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));\n> - return 1;\n> - } else\n> - die(\"BUG: unknown %%(objectname) option\");\n> + } else {\n> + strbuf_addstr(err, \"BUG: unknown %(objectname) option\");\n> + return -1;\n> + }\n> }\n> return 0;\n> }\n\nThis is interesting. This die() is never ever supposed to actually\ntrigger, except to allow a developer adding some new O_xxx-value to\nquickly notice that they have forgotten to add code here.\n\n> /* See grab_values */\n> -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n> +static int grab_common_values(struct atom_value *val, int deref, struct object *obj,\n> + void *buf, unsigned long sz, struct strbuf *err)\n> {\n> int i;\n>\n> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n> v->s = xstrfmt(\"%lu\", sz);\n> }\n> else if (deref)\n> - grab_objectname(name, obj->oid.hash, v, &used_atom[i]);\n> + if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err))\n> + return -1;\n> }\n> + return 0;\n> }\n\nSo if that conversion I commented on above had not happened, this would\nnot have been necessary. Let's read on...\n\n> /* See grab_values */\n> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)\n> * pointed at by the ref itself; otherwise it is the object the\n> * ref (which is a tag) refers to.\n> */\n> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n> +static int grab_values(struct atom_value *val, int deref, struct object *obj,\n> + void *buf, unsigned long sz, struct strbuf *err)\n> {\n> - grab_common_values(val, deref, obj, buf, sz);\n> + if (grab_common_values(val, deref, obj, buf, sz, err))\n> + return -1;\n> switch (obj->type) {\n> case OBJ_TAG:\n> grab_tag_values(val, deref, obj, buf, sz);\n> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v\n> /* grab_blob_values(val, deref, obj, buf, sz); */\n> break;\n> default:\n> - die(\"Eh? Object of type %d?\", obj->type);\n> + strbuf_addf(err, \"Eh? Object of type %d?\", obj->type);\n> + return -1;\n> }\n> + return 0;\n> }\n\nThis seems similar. The string here is quite sloppy, and I do not\nbelieve that the author intended this to be user-visible. I believe this\nis more like a very short way of saying \"how could we possibly get\nhere??\". It could also be written as die(\"BUG: unknown object type %d\",\nobj->type), or even better: BUG(...).\n\n> static inline char *copy_advance(char *dst, const char *src)\n> @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)\n> return refname;\n> }\n>\n> -static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> - struct branch *branch, const char **s)\n> +static int fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> + struct branch *branch, const char **s,\n> + struct strbuf *err)\n> {\n> int num_ours, num_theirs;\n> if (atom->u.remote_ref.option == RR_REF)\n> @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {\n> if (stat_tracking_info(branch, &num_ours, &num_theirs,\n> NULL, AHEAD_BEHIND_FULL) < 0)\n> - return;\n> + return 0;\n>\n> if (!num_ours && !num_theirs)\n> *s = \"=\";\n> @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n> *s = xstrdup(merge);\n> else\n> *s = \"\";\n> - } else\n> - die(\"BUG: unhandled RR_* enum\");\n> + } else {\n> + strbuf_addstr(err, \"BUG: unhandled RR_* enum\");\n> + return -1;\n> + }\n> + return 0;\n> }\n\nThis one too.. I start to think it is overkill to wire through these\nstrbufs just to collect messages that should never ever be produced. It\nalmost seems to me like if 1) we want to collect errors using strbufs,\nand 2) we want to use BUG() for these sorts of assertions, then 3) we\nwill be wiring error-strbufs through more or less all of our code. I am\nexaggerating, but there is something to it: A small change deep down a\ncallstack where you want to add a BUG() \"to be safe\", and you might need\nto wire a strbuf all the way through.\n\nAccording to d8193743e0 (usage.c: add BUG() function, 2017-05-12), a\ngood thing about BUG() is that we can get a core dump, a filename and a\nline number. That opportunity gets lost if we do these sort of\ntransformations. Of course, these were only die(\"BUG: \"), not BUG(), but\nmy point is that they should perhaps have been BUG().\n\n> char *get_head_description(void)\n> @@ -1447,28 +1458,33 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n> return show_ref(&atom->u.refname, ref->refname);\n> }\n>\n> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,\n> - int deref, struct object **obj)\n> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n> + int deref, struct object **obj, struct strbuf *err)\n> {\n> int eaten;\n> unsigned long size;\n> void *buf = get_obj(oid, obj, &size, &eaten);\n> - if (!buf)\n> - die(_(\"missing object %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> - if (!*obj)\n> - die(_(\"parse_object_buffer failed on %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> -\n> - grab_values(ref->value, deref, *obj, buf, size);\n> + if (!buf) {\n> + strbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n> + ref->refname);\n> + return -1;\n> + }\n> + if (!*obj) {\n> + strbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n> + oid_to_hex(oid), ref->refname);\n> + return -1;\n> + }\n> + if (grab_values(ref->value, deref, *obj, buf, size, err))\n> + return -1;\n> if (!eaten)\n> free(buf);\n> + return 0;\n> }\n\nThese are \"real\" errors and yield several more changes in the remainder.\nIgnoring those BUG-type messages at the beginning of this patch would\ngive a patch like the one below. Maybe that would be a bit less\nintrusive for the same gain.\n\nThanks for working on this. I feel that your patches are really\ninteresting. They open up many possibilities for philosophical exercises\nabout how errors should be collected and reported. ;-) I would be\ninterested in knowing your thoughts, and others'.\n\nMartin\n\n---\n ref-filter.c | 51 ++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 32 insertions(+), 19 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 62ea4adcd0..e41505b3c0 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1447,28 +1447,32 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n \treturn show_ref(&atom->u.refname, ref->refname);\n }\n \n-static void get_object(struct ref_array_item *ref, const struct object_id *oid,\n-\t\t int deref, struct object **obj)\n+static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n+\t\t int deref, struct object **obj, struct strbuf *err)\n {\n \tint eaten;\n \tunsigned long size;\n \tvoid *buf = get_obj(oid, obj, &size, &eaten);\n-\tif (!buf)\n-\t\tdie(_(\"missing object %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\tif (!*obj)\n-\t\tdie(_(\"parse_object_buffer failed on %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\n+\tif (!buf) {\n+\t\tstrbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n+\t\t\t ref->refname);\n+\t\treturn -1;\n+\t}\n+\tif (!*obj) {\n+\t\tstrbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n+\t\t\t oid_to_hex(oid), ref->refname);\n+\t\treturn -1;\n+\t}\n \tgrab_values(ref->value, deref, *obj, buf, size);\n \tif (!eaten)\n \t\tfree(buf);\n+\treturn 0;\n }\n \n /*\n * Parse the object referred by ref, and grab needed value.\n */\n-static void populate_value(struct ref_array_item *ref)\n+static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n {\n \tstruct object *obj;\n \tint i;\n@@ -1590,16 +1594,17 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tbreak;\n \t}\n \tif (used_atom_cnt <= i)\n-\t\treturn;\n+\t\treturn 0;\n \n-\tget_object(ref, &ref->objectname, 0, &obj);\n+\tif (get_object(ref, &ref->objectname, 0, &obj, err))\n+\t\treturn -1;\n \n \t/*\n \t * If there is no atom that wants to know about tagged\n \t * object, we are done.\n \t */\n \tif (!need_tagged || (obj->type != OBJ_TAG))\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If it is a tag object, see if we use a value that derefs\n@@ -1613,20 +1618,23 @@ static void populate_value(struct ref_array_item *ref)\n \t * is not consistent with what deref_tag() does\n \t * which peels the onion to the core.\n \t */\n-\tget_object(ref, tagged, 1, &obj);\n+\treturn get_object(ref, tagged, 1, &obj, err);\n }\n \n /*\n * Given a ref, return the value for the atom. This lazily gets value\n * out of the object by calling populate value.\n */\n-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)\n+static int get_ref_atom_value(struct ref_array_item *ref, int atom,\n+\t\t\t struct atom_value **v, struct strbuf *err)\n {\n \tif (!ref->value) {\n-\t\tpopulate_value(ref);\n+\t\tif (populate_value(ref, err))\n+\t\t\treturn -1;\n \t\tfill_missing_values(ref->value);\n \t}\n \t*v = &ref->value[atom];\n+\treturn 0;\n }\n \n /*\n@@ -2150,9 +2158,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tint cmp;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tint (*cmp_fn)(const char *, const char *);\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tget_ref_atom_value(a, s->atom, &va);\n-\tget_ref_atom_value(b, s->atom, &vb);\n+\tif (get_ref_atom_value(a, s->atom, &va, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tif (get_ref_atom_value(b, s->atom, &vb, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n \tif (s->version)\n \t\tcmp = versioncmp(va->s, vb->s);\n@@ -2231,7 +2243,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t\tappend_literal(cp, sp, &state);\n \t\tif (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))\n \t\t\treturn -1;\n-\t\tget_ref_atom_value(info, pos, &atomv);\n+\t\tif (get_ref_atom_value(info, pos, &atomv, error_buf))\n+\t\t\treturn -1;\n \t\tif (atomv->handler(atomv, &state, error_buf))\n \t\t\treturn -1;\n \t}\n-- \n2.16.2.246.ga4ee44448f\n\n"},{"id":"341832","messageId":"CAPig+cRbsDTTfbeQS5tXCCoDKvf2pXAe_suGs-yHY_qj=Go_1A@mail.gmail.com","threadId":"48044","inReplyTo":"20180315204709.1900787-1-martin.agren@gmail.com","subject":"Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-15T21:01:20Z","receivedAt":"2018-03-15T21:01:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren wrote:\n> These are \"real\" errors and yield several more changes in the remainder.\n> Ignoring those BUG-type messages at the beginning of this patch would\n> give a patch like the one below.\n>\n> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n> + int deref, struct object **obj, struct strbuf *err)\n> {\n> void *buf = get_obj(oid, obj, &size, &eaten);\n> - if (!buf)\n> - die(_(\"missing object %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> - if (!*obj)\n> - die(_(\"parse_object_buffer failed on %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> -\n> + if (!buf) {\n> + strbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n> + ref->refname);\n> + return -1;\n> + }\n> + if (!*obj) {\n> + strbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n> + oid_to_hex(oid), ref->refname);\n> + return -1;\n\nDoesn't this leak 'buf'?\n\n> + }\n> grab_values(ref->value, deref, *obj, buf, size);\n> if (!eaten)\n> free(buf);\n> + return 0;\n> }\n"},{"id":"341833","messageId":"xmqqd1053v8c.fsf@gitster-ct.c.googlers.com","threadId":"48044","inReplyTo":"20180315204709.1900787-1-martin.agren@gmail.com","subject":"Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T21:05:39Z","receivedAt":"2018-03-15T21:05:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n>> static int grab_objectname(const char *name, const unsigned char *sha1,\n>> - struct atom_value *v, struct used_atom *atom)\n>> + struct atom_value *v, struct used_atom *atom,\n>> + struct strbuf *err)\n>> {\n>> ...\n>> + } else {\n>> + strbuf_addstr(err, \"BUG: unknown %(objectname) option\");\n>> + return -1;\n>> + }\n>> }\n>> return 0;\n>> }\n>\n> This is interesting. This die() is never ever supposed to actually\n> trigger, except to allow a developer adding some new O_xxx-value to\n> quickly notice that they have forgotten to add code here.\n\nYup, BUG() is meant for a case like this; the original code predates\nthe BUG(\"message\") which in turn predates the die(\"BUG: message\")\nconvention, I think.\n\n>> default:\n>> - die(\"Eh? Object of type %d?\", obj->type);\n>> + strbuf_addf(err, \"Eh? Object of type %d?\", obj->type);\n>> + return -1;\n>> }\n>> + return 0;\n>> }\n>\n> This seems similar. The string here is quite sloppy, and I do not\n> believe that the author intended this to be user-visible. I believe this\n> is more like a very short way of saying \"how could we possibly get\n> here??\". It could also be written as die(\"BUG: unknown object type %d\",\n> obj->type), or even better: BUG(...).\n\nLikewise.\n"},{"id":"341838","messageId":"xmqq8tat3qgj.fsf@gitster-ct.c.googlers.com","threadId":"48044","inReplyTo":"0102016225e61cb4-b59773de-d22e-42cd-909a-d7ce71646898-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v2 3/5] ref-filter: change parsing function error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T22:48:44Z","receivedAt":"2018-03-15T22:48:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olga Telezhnaya writes:\n\n> Continue removing any printing from ref-filter formatting logic,\n> so that it could be more general.\n\nHmm.\n\n> Change the signature of parse_ref_filter_atom() by changing return value,\n> adding previous return value to function parameter and also adding\n> strbuf parameter for error message.\n\nThis says what the patch changes, but it does not explain why it is\na good idea to return something with different meaning to the\ncallers (which of course forces us to update all callers so that\nthey pass &result pointer and check the value returned in the\nvariable), or more importantly what meaning the return value has and\nhow the callers are expected to use it. While at it, it probably is\na good idea to explain what the original return value means.\n\n\tThe return value from parse_ref_filter_atom() used to be the\n\tposition the atom is found in the used_atom[] array; that\n\tinformation is now returned in an integer pointed at by the\n\t*res parameter. The function now returns 0 for success and\n\t-1 for failure.\n\nor something like that.\n\nHaving said that, I wonder if a calling convention that does not\nforce callers to pass in a pointer-to-int may make more sense.\nBecause the original return value is an index into an array, we know\nthe normal return values are not negative. An updated caller could\nbecome like this instead:\n\n\tpos = parse_ref_filter_atom(format, atom, ep, &err);\n\tif (pos < 0)\n\t\tdie(\"%s\", err.buf);\n\t... original code that used 'pos' can stay as before ...\n\n"},{"id":"341859","messageId":"CAL21BmmXz7wsq9kPzbvjo3b5ZEvmZA5qi-XqoXf6X8rRwOg_aQ@mail.gmail.com","threadId":"48044","inReplyTo":"CAPig+cRbsDTTfbeQS5tXCCoDKvf2pXAe_suGs-yHY_qj=Go_1A@mail.gmail.com","subject":"Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-16T07:20:01Z","receivedAt":"2018-03-16T07:20:10Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-03-16 0:01 GMT+03:00 Eric Sunshine :\n> On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren wrote:\n>> These are \"real\" errors and yield several more changes in the remainder.\n>> Ignoring those BUG-type messages at the beginning of this patch would\n>> give a patch like the one below.\n>>\n>> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n>> + int deref, struct object **obj, struct strbuf *err)\n>> {\n>> void *buf = get_obj(oid, obj, &size, &eaten);\n>> - if (!buf)\n>> - die(_(\"missing object %s for %s\"),\n>> - oid_to_hex(oid), ref->refname);\n>> - if (!*obj)\n>> - die(_(\"parse_object_buffer failed on %s for %s\"),\n>> - oid_to_hex(oid), ref->refname);\n>> -\n>> + if (!buf) {\n>> + strbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n>> + ref->refname);\n>> + return -1;\n>> + }\n>> + if (!*obj) {\n>> + strbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n>> + oid_to_hex(oid), ref->refname);\n>> + return -1;\n>\n> Doesn't this leak 'buf'?\n\nYes. Thanks a lot.\n>\n>> + }\n>> grab_values(ref->value, deref, *obj, buf, size);\n>> if (!eaten)\n>> free(buf);\n>> + return 0;\n>> }\n"},{"id":"341860","messageId":"CAL21BmkipcdT6M38PDJ6=kk_E5F8T_OmYOF_N097UbmzbnjESA@mail.gmail.com","threadId":"48044","inReplyTo":"20180315204709.1900787-1-martin.agren@gmail.com","subject":"Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-16T07:17:48Z","receivedAt":"2018-03-16T07:20:31Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-03-15 23:47 GMT+03:00 Martin Ågren :\n> I skimmed the first four patches of this v2. It seems that patches 1 and\n> 4 are identical to v2. Patches 2 and 3 have very straightforward changes\n> based on my earlier comments. Let's see what this patch is about. :-)\n\nYes, you are right.\n\n>\n> On 14 March 2018 at 20:04, Olga Telezhnaya wrote:\n>> Finish removing any printing from ref-filter formatting logic,\n>> so that it could be more general.\n>>\n>> Change the signature of get_ref_atom_value() and underlying functions\n>> by adding return value and strbuf parameter for error message.\n>>\n>> It's important to mention that grab_objectname() returned 1 if\n>> it gets objectname atom and 0 otherwise. Now this logic changed:\n>> we return 0 if we have no error, -1 otherwise. If someone needs to\n>> know whether it's objectname atom or not, he/she could use\n>> starts_with() function. It duplicates this checking but it does not\n>> sound like a really big overhead.\n>>\n>> Signed-off-by: Olga Telezhnaia \n>> ---\n>> ref-filter.c | 109 +++++++++++++++++++++++++++++++++++++----------------------\n>> 1 file changed, 69 insertions(+), 40 deletions(-)\n>>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 62ea4adcd0ff1..3f0c3924273d5 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, struct object **obj, unsigned\n>> }\n>>\n>> static int grab_objectname(const char *name, const unsigned char *sha1,\n>> - struct atom_value *v, struct used_atom *atom)\n>> + struct atom_value *v, struct used_atom *atom,\n>> + struct strbuf *err)\n>> {\n>> if (starts_with(name, \"objectname\")) {\n>> if (atom->u.objectname.option == O_SHORT) {\n>> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n>> - return 1;\n>> } else if (atom->u.objectname.option == O_FULL) {\n>> v->s = xstrdup(sha1_to_hex(sha1));\n>> - return 1;\n>> } else if (atom->u.objectname.option == O_LENGTH) {\n>> v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));\n>> - return 1;\n>> - } else\n>> - die(\"BUG: unknown %%(objectname) option\");\n>> + } else {\n>> + strbuf_addstr(err, \"BUG: unknown %(objectname) option\");\n>> + return -1;\n>> + }\n>> }\n>> return 0;\n>> }\n>\n> This is interesting. This die() is never ever supposed to actually\n> trigger, except to allow a developer adding some new O_xxx-value to\n> quickly notice that they have forgotten to add code here.\n\nOh, cool, so I can revert this change, OK.\n\n>\n>> /* See grab_values */\n>> -static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n>> +static int grab_common_values(struct atom_value *val, int deref, struct object *obj,\n>> + void *buf, unsigned long sz, struct strbuf *err)\n>> {\n>> int i;\n>>\n>> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n>> v->s = xstrfmt(\"%lu\", sz);\n>> }\n>> else if (deref)\n>> - grab_objectname(name, obj->oid.hash, v, &used_atom[i]);\n>> + if (grab_objectname(name, obj->oid.hash, v, &used_atom[i], err))\n>> + return -1;\n>> }\n>> + return 0;\n>> }\n>\n> So if that conversion I commented on above had not happened, this would\n> not have been necessary. Let's read on...\n\nOf course, I will check and also revert functions that were touched\nonly because of other functions.\n\n>\n>> /* See grab_values */\n>> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value *val)\n>> * pointed at by the ref itself; otherwise it is the object the\n>> * ref (which is a tag) refers to.\n>> */\n>> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)\n>> +static int grab_values(struct atom_value *val, int deref, struct object *obj,\n>> + void *buf, unsigned long sz, struct strbuf *err)\n>> {\n>> - grab_common_values(val, deref, obj, buf, sz);\n>> + if (grab_common_values(val, deref, obj, buf, sz, err))\n>> + return -1;\n>> switch (obj->type) {\n>> case OBJ_TAG:\n>> grab_tag_values(val, deref, obj, buf, sz);\n>> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v\n>> /* grab_blob_values(val, deref, obj, buf, sz); */\n>> break;\n>> default:\n>> - die(\"Eh? Object of type %d?\", obj->type);\n>> + strbuf_addf(err, \"Eh? Object of type %d?\", obj->type);\n>> + return -1;\n>> }\n>> + return 0;\n>> }\n>\n> This seems similar. The string here is quite sloppy, and I do not\n> believe that the author intended this to be user-visible. I believe this\n> is more like a very short way of saying \"how could we possibly get\n> here??\". It could also be written as die(\"BUG: unknown object type %d\",\n> obj->type), or even better: BUG(...).\n\nOK, thanks!\n\n>\n>> static inline char *copy_advance(char *dst, const char *src)\n>> @@ -1335,8 +1342,9 @@ static const char *show_ref(struct refname_atom *atom, const char *refname)\n>> return refname;\n>> }\n>>\n>> -static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n>> - struct branch *branch, const char **s)\n>> +static int fill_remote_ref_details(struct used_atom *atom, const char *refname,\n>> + struct branch *branch, const char **s,\n>> + struct strbuf *err)\n>> {\n>> int num_ours, num_theirs;\n>> if (atom->u.remote_ref.option == RR_REF)\n>> @@ -1362,7 +1370,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n>> } else if (atom->u.remote_ref.option == RR_TRACKSHORT) {\n>> if (stat_tracking_info(branch, &num_ours, &num_theirs,\n>> NULL, AHEAD_BEHIND_FULL) < 0)\n>> - return;\n>> + return 0;\n>>\n>> if (!num_ours && !num_theirs)\n>> *s = \"=\";\n>> @@ -1391,8 +1399,11 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n>> *s = xstrdup(merge);\n>> else\n>> *s = \"\";\n>> - } else\n>> - die(\"BUG: unhandled RR_* enum\");\n>> + } else {\n>> + strbuf_addstr(err, \"BUG: unhandled RR_* enum\");\n>> + return -1;\n>> + }\n>> + return 0;\n>> }\n>\n> This one too.. I start to think it is overkill to wire through these\n> strbufs just to collect messages that should never ever be produced. It\n> almost seems to me like if 1) we want to collect errors using strbufs,\n> and 2) we want to use BUG() for these sorts of assertions, then 3) we\n> will be wiring error-strbufs through more or less all of our code. I am\n> exaggerating, but there is something to it: A small change deep down a\n> callstack where you want to add a BUG() \"to be safe\", and you might need\n> to wire a strbuf all the way through.\n\nI absolutely agree that we should not touch functions that produce\nerrors for Git developers. It still means that sometimes we need to\nwire strbufs, but I will revert some of them. It's enough to handle\nonly errors for users.\n\n>\n> According to d8193743e0 (usage.c: add BUG() function, 2017-05-12), a\n> good thing about BUG() is that we can get a core dump, a filename and a\n> line number. That opportunity gets lost if we do these sort of\n> transformations. Of course, these were only die(\"BUG: \"), not BUG(), but\n> my point is that they should perhaps have been BUG().\n>\n>> char *get_head_description(void)\n>> @@ -1447,28 +1458,33 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n>> return show_ref(&atom->u.refname, ref->refname);\n>> }\n>>\n>> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,\n>> - int deref, struct object **obj)\n>> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n>> + int deref, struct object **obj, struct strbuf *err)\n>> {\n>> int eaten;\n>> unsigned long size;\n>> void *buf = get_obj(oid, obj, &size, &eaten);\n>> - if (!buf)\n>> - die(_(\"missing object %s for %s\"),\n>> - oid_to_hex(oid), ref->refname);\n>> - if (!*obj)\n>> - die(_(\"parse_object_buffer failed on %s for %s\"),\n>> - oid_to_hex(oid), ref->refname);\n>> -\n>> - grab_values(ref->value, deref, *obj, buf, size);\n>> + if (!buf) {\n>> + strbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n>> + ref->refname);\n>> + return -1;\n>> + }\n>> + if (!*obj) {\n>> + strbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n>> + oid_to_hex(oid), ref->refname);\n>> + return -1;\n>> + }\n>> + if (grab_values(ref->value, deref, *obj, buf, size, err))\n>> + return -1;\n>> if (!eaten)\n>> free(buf);\n>> + return 0;\n>> }\n>\n> These are \"real\" errors and yield several more changes in the remainder.\n> Ignoring those BUG-type messages at the beginning of this patch would\n> give a patch like the one below. Maybe that would be a bit less\n> intrusive for the same gain.\n>\n> Thanks for working on this. I feel that your patches are really\n> interesting. They open up many possibilities for philosophical exercises\n> about how errors should be collected and reported. ;-) I would be\n> interested in knowing your thoughts, and others'.\n\nThank you! I was also thinking about other ways how to handle errors.\nI haven't invented anything new, so at least I decided to make these\nchanges (for now).\n\n>\n> Martin\n>\n> ---\n> ref-filter.c | 51 ++++++++++++++++++++++++++++++++-------------------\n> 1 file changed, 32 insertions(+), 19 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 62ea4adcd0..e41505b3c0 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1447,28 +1447,32 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n> return show_ref(&atom->u.refname, ref->refname);\n> }\n>\n> -static void get_object(struct ref_array_item *ref, const struct object_id *oid,\n> - int deref, struct object **obj)\n> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n> + int deref, struct object **obj, struct strbuf *err)\n> {\n> int eaten;\n> unsigned long size;\n> void *buf = get_obj(oid, obj, &size, &eaten);\n> - if (!buf)\n> - die(_(\"missing object %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> - if (!*obj)\n> - die(_(\"parse_object_buffer failed on %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> -\n> + if (!buf) {\n> + strbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n> + ref->refname);\n> + return -1;\n> + }\n> + if (!*obj) {\n> + strbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n> + oid_to_hex(oid), ref->refname);\n> + return -1;\n> + }\n> grab_values(ref->value, deref, *obj, buf, size);\n> if (!eaten)\n> free(buf);\n> + return 0;\n> }\n>\n> /*\n> * Parse the object referred by ref, and grab needed value.\n> */\n> -static void populate_value(struct ref_array_item *ref)\n> +static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n> {\n> struct object *obj;\n> int i;\n> @@ -1590,16 +1594,17 @@ static void populate_value(struct ref_array_item *ref)\n> break;\n> }\n> if (used_atom_cnt <= i)\n> - return;\n> + return 0;\n>\n> - get_object(ref, &ref->objectname, 0, &obj);\n> + if (get_object(ref, &ref->objectname, 0, &obj, err))\n> + return -1;\n>\n> /*\n> * If there is no atom that wants to know about tagged\n> * object, we are done.\n> */\n> if (!need_tagged || (obj->type != OBJ_TAG))\n> - return;\n> + return 0;\n>\n> /*\n> * If it is a tag object, see if we use a value that derefs\n> @@ -1613,20 +1618,23 @@ static void populate_value(struct ref_array_item *ref)\n> * is not consistent with what deref_tag() does\n> * which peels the onion to the core.\n> */\n> - get_object(ref, tagged, 1, &obj);\n> + return get_object(ref, tagged, 1, &obj, err);\n> }\n>\n> /*\n> * Given a ref, return the value for the atom. This lazily gets value\n> * out of the object by calling populate value.\n> */\n> -static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)\n> +static int get_ref_atom_value(struct ref_array_item *ref, int atom,\n> + struct atom_value **v, struct strbuf *err)\n> {\n> if (!ref->value) {\n> - populate_value(ref);\n> + if (populate_value(ref, err))\n> + return -1;\n> fill_missing_values(ref->value);\n> }\n> *v = &ref->value[atom];\n> + return 0;\n> }\n>\n> /*\n> @@ -2150,9 +2158,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n> int cmp;\n> cmp_type cmp_type = used_atom[s->atom].type;\n> int (*cmp_fn)(const char *, const char *);\n> + struct strbuf err = STRBUF_INIT;\n>\n> - get_ref_atom_value(a, s->atom, &va);\n> - get_ref_atom_value(b, s->atom, &vb);\n> + if (get_ref_atom_value(a, s->atom, &va, &err))\n> + die(\"%s\", err.buf);\n> + if (get_ref_atom_value(b, s->atom, &vb, &err))\n> + die(\"%s\", err.buf);\n> + strbuf_release(&err);\n> cmp_fn = s->ignore_case ? strcasecmp : strcmp;\n> if (s->version)\n> cmp = versioncmp(va->s, vb->s);\n> @@ -2231,7 +2243,8 @@ int format_ref_array_item(struct ref_array_item *info,\n> append_literal(cp, sp, &state);\n> if (parse_ref_filter_atom(format, sp + 2, ep, &pos, error_buf))\n> return -1;\n> - get_ref_atom_value(info, pos, &atomv);\n> + if (get_ref_atom_value(info, pos, &atomv, error_buf))\n> + return -1;\n> if (atomv->handler(atomv, &state, error_buf))\n> return -1;\n> }\n> --\n> 2.16.2.246.ga4ee44448f\n>\n"},{"id":"341861","messageId":"CAL21Bmk03kSaO8h+uQ9nB7DomAL0Ym+7jygTo7M-rv5OM2duzA@mail.gmail.com","threadId":"48044","inReplyTo":"xmqq8tat3qgj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 3/5] ref-filter: change parsing function error handling","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-16T07:22:38Z","receivedAt":"2018-03-16T07:22:46Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-03-16 1:48 GMT+03:00 Junio C Hamano :\n> Olga Telezhnaya writes:\n>\n>> Continue removing any printing from ref-filter formatting logic,\n>> so that it could be more general.\n>\n> Hmm.\n>\n>> Change the signature of parse_ref_filter_atom() by changing return value,\n>> adding previous return value to function parameter and also adding\n>> strbuf parameter for error message.\n>\n> This says what the patch changes, but it does not explain why it is\n> a good idea to return something with different meaning to the\n> callers (which of course forces us to update all callers so that\n> they pass &result pointer and check the value returned in the\n> variable), or more importantly what meaning the return value has and\n> how the callers are expected to use it. While at it, it probably is\n> a good idea to explain what the original return value means.\n>\n> The return value from parse_ref_filter_atom() used to be the\n> position the atom is found in the used_atom[] array; that\n> information is now returned in an integer pointed at by the\n> *res parameter. The function now returns 0 for success and\n> -1 for failure.\n>\n> or something like that.\n>\n> Having said that, I wonder if a calling convention that does not\n> force callers to pass in a pointer-to-int may make more sense.\n> Because the original return value is an index into an array, we know\n> the normal return values are not negative. An updated caller could\n> become like this instead:\n>\n> pos = parse_ref_filter_atom(format, atom, ep, &err);\n> if (pos < 0)\n> die(\"%s\", err.buf);\n> ... original code that used 'pos' can stay as before ...\n>\n\nMartin also mentioned that, but I was not sure which solution is\nbetter. Great, thanks, I will fix that.\n"},{"id":"342141","messageId":"010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"0102016225e61c34-50ab78f0-6fb6-4ece-acfe-da253bdce616-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 1/5] ref-filter: start adding strbufs with errors","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-19T13:01:00Z","receivedAt":"2018-03-19T13:01:07Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"This is a first step in removing any printing from\nref-filter formatting logic, so that it could be more general.\nEverything would be the same for show_ref_array_item() users.\nBut, if you want to deal with errors by your own, you could invoke\nformat_ref_array_item(). It means that you need to print everything\n(the result and errors) on your side.\n\nThis commit changes signature of format_ref_array_item() by adding\nreturn value and strbuf parameter for errors, and adjusts\nits callers. While at it, reduce the scope of the out-variable.\n\nSigned-off-by: Olga Telezhnaia \n---\n builtin/branch.c | 7 +++++--\n ref-filter.c | 17 ++++++++++++-----\n ref-filter.h | 7 ++++---\n 3 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6d0cea9d4bcc4..c21e5a04a0177 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tstruct ref_array array;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n-\tstruct strbuf out = STRBUF_INIT;\n \tchar *to_free = NULL;\n \n \t/*\n@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tref_array_sort(sorting, &array);\n \n \tfor (i = 0; i < array.nr; i++) {\n-\t\tformat_ref_array_item(array.items[i], format, &out);\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tif (format_ref_array_item(array.items[i], format, &out, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t\tif (column_active(colopts)) {\n \t\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n \t\t\t /* format to a string_list to let print_columns() do its job */\n@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n \t\t\tputchar('\\n');\n \t\t}\n+\t\tstrbuf_release(&err);\n \t\tstrbuf_release(&out);\n \t}\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex 45fc56216aaa8..54fae00bdd410 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \n-void format_ref_array_item(struct ref_array_item *info,\n+int format_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format,\n-\t\t\t struct strbuf *final_buf)\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf)\n {\n \tconst char *cp, *sp, *ep;\n \tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n@@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\tresetv.s = GIT_COLOR_RESET;\n \t\tappend_atom(&resetv, &state);\n \t}\n-\tif (state.stack->prev)\n-\t\tdie(_(\"format: %%(end) atom missing\"));\n+\tif (state.stack->prev) {\n+\t\tstrbuf_addstr(error_buf, _(\"format: %(end) atom missing\"));\n+\t\treturn -1;\n+\t}\n \tstrbuf_addbuf(final_buf, &state.stack->output);\n \tpop_stack_element(&state.stack);\n+\treturn 0;\n }\n \n void show_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format)\n {\n \tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct strbuf error_buf = STRBUF_INIT;\n \n-\tformat_ref_array_item(info, format, &final_buf);\n+\tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n+\t\tdie(\"%s\", error_buf.buf);\n \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\tstrbuf_release(&error_buf);\n \tstrbuf_release(&final_buf);\n \tputchar('\\n');\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 0d98342b34319..e13f8e6f8721a 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n /* Based on the given format and quote_style, fill the strbuf */\n-void format_ref_array_item(struct ref_array_item *info,\n-\t\t\t const struct ref_format *format,\n-\t\t\t struct strbuf *final_buf);\n+int format_ref_array_item(struct ref_array_item *info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf);\n /* Print the ref using the given format and quote_style */\n void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n /* Parse a single sort specifier and add it to the list */\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342142","messageId":"010201623e5944b9-8d44ad36-a6df-4bd3-97e7-de4ee54af3c0-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 3/5] ref-filter: change parsing function error handling","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-19T13:01:00Z","receivedAt":"2018-03-19T13:01:09Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of parse_ref_filter_atom() by adding\nstrbuf parameter for error message.\nReturn value means the same except negative values: they indicate\nerrors (previous version could return only non-negative value).\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 36 ++++++++++++++++++++++++++----------\n 1 file changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d120360104806..2313a33f0baa4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -397,7 +397,8 @@ struct atom_value {\n * Used to parse format string and sort specifiers\n */\n static int parse_ref_filter_atom(const struct ref_format *format,\n-\t\t\t\t const char *atom, const char *ep)\n+\t\t\t\t const char *atom, const char *ep,\n+\t\t\t\t struct strbuf *err)\n {\n \tconst char *sp;\n \tconst char *arg;\n@@ -406,8 +407,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tsp = atom;\n \tif (*sp == '*' && sp < ep)\n \t\tsp++; /* deref */\n-\tif (ep <= sp)\n-\t\tdie(_(\"malformed field name: %.*s\"), (int)(ep-atom), atom);\n+\tif (ep <= sp) {\n+\t\tstrbuf_addf(err, _(\"malformed field name: %.*s\"), (int)(ep-atom), atom);\n+\t\treturn -1;\n+\t}\n \n \t/* Do we have the atom already used elsewhere? */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n@@ -432,8 +435,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\t\tbreak;\n \t}\n \n-\tif (ARRAY_SIZE(valid_atom) <= i)\n-\t\tdie(_(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n+\tif (ARRAY_SIZE(valid_atom) <= i) {\n+\t\tstrbuf_addf(err, _(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n+\t\treturn -1;\n+\t}\n \n \t/* Add it in, including the deref prefix */\n \tat = used_atom_cnt;\n@@ -725,17 +730,21 @@ int verify_ref_format(struct ref_format *format)\n \n \tformat->need_color_reset_at_eol = 0;\n \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n \t\tconst char *color, *ep = strchr(sp, ')');\n \t\tint at;\n \n \t\tif (!ep)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n-\t\tat = parse_ref_filter_atom(format, sp + 2, ep);\n+\t\tat = parse_ref_filter_atom(format, sp + 2, ep, &err);\n+\t\tif (at < 0)\n+\t\t\tdie(\"%s\", err.buf);\n \t\tcp = ep + 1;\n \n \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n \t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n+\t\tstrbuf_release(&err);\n \t}\n \tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n \t\tformat->need_color_reset_at_eol = 0;\n@@ -2154,13 +2163,15 @@ int format_ref_array_item(struct ref_array_item *info,\n \n \tfor (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n+\t\tint pos;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n-\t\tget_ref_atom_value(info,\n-\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n-\t\t\t\t &atomv);\n+\t\tpos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);\n+\t\tif (pos < 0)\n+\t\t\treturn -1;\n+\t\tget_ref_atom_value(info, pos, &atomv);\n \t\tif (atomv->handler(atomv, &state, error_buf))\n \t\t\treturn -1;\n \t}\n@@ -2215,7 +2226,12 @@ static int parse_sorting_atom(const char *atom)\n \t */\n \tstruct ref_format dummy = REF_FORMAT_INIT;\n \tconst char *end = atom + strlen(atom);\n-\treturn parse_ref_filter_atom(&dummy, atom, end);\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint res = parse_ref_filter_atom(&dummy, atom, end, &err);\n+\tif (res < 0)\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\treturn res;\n }\n \n /* If no sorting option is given, use refname to sort as default */\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342143","messageId":"010201623e5944b3-9397c866-0bd8-4a03-863a-5c87a486e7b2-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-19T13:01:00Z","receivedAt":"2018-03-19T13:01:15Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Finish removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of get_ref_atom_value() and underlying functions\nby adding return value and strbuf parameter for error message.\n\nSome die() calls are left; all of them are not for users, but for\nGit developers.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 55 ++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 36 insertions(+), 19 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b90cec1056954..85f971663a4aa 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n \treturn show_ref(&atom->u.refname, ref->refname);\n }\n \n-static void get_object(struct ref_array_item *ref, const struct object_id *oid,\n-\t\t int deref, struct object **obj)\n+static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n+\t\t int deref, struct object **obj, struct strbuf *err)\n {\n \tint eaten;\n \tunsigned long size;\n \tvoid *buf = get_obj(oid, obj, &size, &eaten);\n-\tif (!buf)\n-\t\tdie(_(\"missing object %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\tif (!*obj)\n-\t\tdie(_(\"parse_object_buffer failed on %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\n+\tif (!buf) {\n+\t\tstrbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n+\t\t\t ref->refname);\n+\t\tif (!eaten)\n+\t\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n+\tif (!*obj) {\n+\t\tstrbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n+\t\t\t oid_to_hex(oid), ref->refname);\n+\t\tif (!eaten)\n+\t\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n \tgrab_values(ref->value, deref, *obj, buf, size);\n \tif (!eaten)\n \t\tfree(buf);\n+\treturn 0;\n }\n \n /*\n * Parse the object referred by ref, and grab needed value.\n */\n-static void populate_value(struct ref_array_item *ref)\n+static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n {\n \tstruct object *obj;\n \tint i;\n@@ -1588,16 +1596,17 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tbreak;\n \t}\n \tif (used_atom_cnt <= i)\n-\t\treturn;\n+\t\treturn 0;\n \n-\tget_object(ref, &ref->objectname, 0, &obj);\n+\tif (get_object(ref, &ref->objectname, 0, &obj, err))\n+\t\treturn -1;\n \n \t/*\n \t * If there is no atom that wants to know about tagged\n \t * object, we are done.\n \t */\n \tif (!need_tagged || (obj->type != OBJ_TAG))\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If it is a tag object, see if we use a value that derefs\n@@ -1611,20 +1620,23 @@ static void populate_value(struct ref_array_item *ref)\n \t * is not consistent with what deref_tag() does\n \t * which peels the onion to the core.\n \t */\n-\tget_object(ref, tagged, 1, &obj);\n+\treturn get_object(ref, tagged, 1, &obj, err);\n }\n \n /*\n * Given a ref, return the value for the atom. This lazily gets value\n * out of the object by calling populate value.\n */\n-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)\n+static int get_ref_atom_value(struct ref_array_item *ref, int atom,\n+\t\t\t struct atom_value **v, struct strbuf *err)\n {\n \tif (!ref->value) {\n-\t\tpopulate_value(ref);\n+\t\tif (populate_value(ref, err))\n+\t\t\treturn -1;\n \t\tfill_missing_values(ref->value);\n \t}\n \t*v = &ref->value[atom];\n+\treturn 0;\n }\n \n /*\n@@ -2148,9 +2160,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tint cmp;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tint (*cmp_fn)(const char *, const char *);\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tget_ref_atom_value(a, s->atom, &va);\n-\tget_ref_atom_value(b, s->atom, &vb);\n+\tif (get_ref_atom_value(a, s->atom, &va, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tif (get_ref_atom_value(b, s->atom, &vb, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n \tif (s->version)\n \t\tcmp = versioncmp(va->s, vb->s);\n@@ -2230,7 +2246,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tpos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);\n \t\tif (pos < 0)\n \t\t\treturn -1;\n-\t\tget_ref_atom_value(info, pos, &atomv);\n+\t\tif (get_ref_atom_value(info, pos, &atomv, error_buf))\n+\t\t\treturn -1;\n \t\tif (atomv->handler(atomv, &state, error_buf))\n \t\t\treturn -1;\n \t}\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342144","messageId":"010201623e59448f-2db7a920-fe77-481a-b105-de6bbff253eb-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 2/5] ref-filter: add return value && strbuf to handlers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-19T13:01:00Z","receivedAt":"2018-03-19T13:01:17Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of handlers by adding return value\nand strbuf parameter for errors.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 71 ++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 48 insertions(+), 23 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 54fae00bdd410..d120360104806 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -387,7 +387,8 @@ struct ref_formatting_state {\n \n struct atom_value {\n \tconst char *s;\n-\tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n+\tint (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t struct strbuf *err);\n \tuintmax_t value; /* used for sorting when not FIELD_STR */\n \tstruct used_atom *atom;\n };\n@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n \t}\n }\n \n-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,\n+\t\t struct strbuf *unused_err)\n {\n \t/*\n \t * Quote formatting is only done when the stack has a single\n@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state\n \t\tquote_formatting(&state->stack->output, v->s, state->quote_style);\n \telse\n \t\tstrbuf_addstr(&state->stack->output, v->s);\n+\treturn 0;\n }\n \n static void push_stack_element(struct ref_formatting_stack **stack)\n@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)\n \tstrbuf_release(&s);\n }\n \n-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *unused_err)\n {\n \tstruct ref_formatting_stack *new_stack;\n \n@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n \tnew_stack = state->stack;\n \tnew_stack->at_end = end_align_handler;\n \tnew_stack->at_end_data = &atomv->atom->u.align;\n+\treturn 0;\n }\n \n static void if_then_else_handler(struct ref_formatting_stack **stack)\n@@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)\n \tfree(if_then_else);\n }\n \n-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *unused_err)\n {\n \tstruct ref_formatting_stack *new_stack;\n \tstruct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);\n@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat\n \tnew_stack = state->stack;\n \tnew_stack->at_end = if_then_else_handler;\n \tnew_stack->at_end_data = if_then_else;\n+\treturn 0;\n }\n \n static int is_empty(const char *s)\n@@ -596,19 +603,24 @@ static int is_empty(const char *s)\n \treturn 1;\n }\n \n-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *cur = state->stack;\n \tstruct if_then_else *if_then_else = NULL;\n \n \tif (cur->at_end == if_then_else_handler)\n \t\tif_then_else = (struct if_then_else *)cur->at_end_data;\n-\tif (!if_then_else)\n-\t\tdie(_(\"format: %%(then) atom used without an %%(if) atom\"));\n-\tif (if_then_else->then_atom_seen)\n-\t\tdie(_(\"format: %%(then) atom used more than once\"));\n-\tif (if_then_else->else_atom_seen)\n-\t\tdie(_(\"format: %%(then) atom used after %%(else)\"));\n+\tif (!if_then_else) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used without an %(if) atom\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->then_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used more than once\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->else_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used after %(else)\"));\n+\t\treturn -1;\n+\t}\n \tif_then_else->then_atom_seen = 1;\n \t/*\n \t * If the 'equals' or 'notequals' attribute is used then\n@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st\n \t} else if (cur->output.len && !is_empty(cur->output.buf))\n \t\tif_then_else->condition_satisfied = 1;\n \tstrbuf_reset(&cur->output);\n+\treturn 0;\n }\n \n-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *prev = state->stack;\n \tstruct if_then_else *if_then_else = NULL;\n \n \tif (prev->at_end == if_then_else_handler)\n \t\tif_then_else = (struct if_then_else *)prev->at_end_data;\n-\tif (!if_then_else)\n-\t\tdie(_(\"format: %%(else) atom used without an %%(if) atom\"));\n-\tif (!if_then_else->then_atom_seen)\n-\t\tdie(_(\"format: %%(else) atom used without a %%(then) atom\"));\n-\tif (if_then_else->else_atom_seen)\n-\t\tdie(_(\"format: %%(else) atom used more than once\"));\n+\tif (!if_then_else) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used without an %(if) atom\"));\n+\t\treturn -1;\n+\t} else if (!if_then_else->then_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used without a %(then) atom\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->else_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used more than once\"));\n+\t\treturn -1;\n+\t}\n \tif_then_else->else_atom_seen = 1;\n \tpush_stack_element(&state->stack);\n \tstate->stack->at_end_data = prev->at_end_data;\n \tstate->stack->at_end = prev->at_end;\n+\treturn 0;\n }\n \n-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *current = state->stack;\n \tstruct strbuf s = STRBUF_INIT;\n \n-\tif (!current->at_end)\n-\t\tdie(_(\"format: %%(end) atom used without corresponding atom\"));\n+\tif (!current->at_end) {\n+\t\tstrbuf_addstr(err, _(\"format: %(end) atom used without corresponding atom\"));\n+\t\treturn -1;\n+\t}\n \tcurrent->at_end(&state->stack);\n \n \t/* Stack may have been popped within at_end(), hence reset the current pointer */\n@@ -668,6 +690,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\n \t}\n \tstrbuf_release(&s);\n \tpop_stack_element(&state->stack);\n+\treturn 0;\n }\n \n /*\n@@ -2138,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tget_ref_atom_value(info,\n \t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n \t\t\t\t &atomv);\n-\t\tatomv->handler(atomv, &state);\n+\t\tif (atomv->handler(atomv, &state, error_buf))\n+\t\t\treturn -1;\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n@@ -2147,7 +2171,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \tif (format->need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n \t\tresetv.s = GIT_COLOR_RESET;\n-\t\tappend_atom(&resetv, &state);\n+\t\tif (append_atom(&resetv, &state, error_buf))\n+\t\t\treturn -1;\n \t}\n \tif (state.stack->prev) {\n \t\tstrbuf_addstr(error_buf, _(\"format: %(end) atom missing\"));\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342145","messageId":"010201623e5944bd-e932aceb-2d30-4940-89e1-130b99f6ed81-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 4/5] ref-filter: add return value to parsers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-19T13:01:00Z","receivedAt":"2018-03-19T13:01:23Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing any printing from ref-filter formatting logic,\nso that it could be more general.\n\nChange the signature of parsers by adding return value and\nstrbuf parameter for error message.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 177 +++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 118 insertions(+), 59 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 2313a33f0baa4..b90cec1056954 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -101,22 +101,28 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n \n-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)\n+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *color_value, struct strbuf *err)\n {\n-\tif (!color_value)\n-\t\tdie(_(\"expected format: %%(color:)\"));\n-\tif (color_parse(color_value, atom->u.color) < 0)\n-\t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n+\tif (!color_value) {\n+\t\tstrbuf_addstr(err, _(\"expected format: %(color:)\"));\n+\t\treturn -1;\n+\t}\n+\tif (color_parse(color_value, atom->u.color) < 0) {\n+\t\tstrbuf_addf(err, _(\"unrecognized color: %%(color:%s)\"), color_value);\n+\t\treturn -1;\n+\t}\n \t/*\n \t * We check this after we've parsed the color, which lets us complain\n \t * about syntactically bogus color names even if they won't be used.\n \t */\n \tif (!want_color(format->use_color))\n \t\tcolor_parse(\"\", atom->u.color);\n+\treturn 0;\n }\n \n-static void refname_atom_parser_internal(struct refname_atom *atom,\n-\t\t\t\t\t const char *arg, const char *name)\n+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,\n+\t\t\t\t\t const char *name, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->option = R_NORMAL;\n@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct refname_atom *atom,\n \telse if (skip_prefix(arg, \"lstrip=\", &arg) ||\n \t\t skip_prefix(arg, \"strip=\", &arg)) {\n \t\tatom->option = R_LSTRIP;\n-\t\tif (strtol_i(arg, 10, &atom->lstrip))\n-\t\t\tdie(_(\"Integer value expected refname:lstrip=%s\"), arg);\n+\t\tif (strtol_i(arg, 10, &atom->lstrip)) {\n+\t\t\tstrbuf_addf(err, _(\"Integer value expected refname:lstrip=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n \t} else if (skip_prefix(arg, \"rstrip=\", &arg)) {\n \t\tatom->option = R_RSTRIP;\n-\t\tif (strtol_i(arg, 10, &atom->rstrip))\n-\t\t\tdie(_(\"Integer value expected refname:rstrip=%s\"), arg);\n-\t} else\n-\t\tdie(_(\"unrecognized %%(%s) argument: %s\"), name, arg);\n+\t\tif (strtol_i(arg, 10, &atom->rstrip)) {\n+\t\t\tstrbuf_addf(err, _(\"Integer value expected refname:rstrip=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(%s) argument: %s\"), name, arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\t const char *arg, struct strbuf *err)\n {\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_\n \n \tif (!arg) {\n \t\tatom->u.remote_ref.option = RR_REF;\n-\t\trefname_atom_parser_internal(&atom->u.remote_ref.refname,\n-\t\t\t\t\t arg, atom->name);\n-\t\treturn;\n+\t\treturn refname_atom_parser_internal(&atom->u.remote_ref.refname,\n+\t\t\t\t\t\t arg, atom->name, err);\n \t}\n \n \tatom->u.remote_ref.nobracket = 0;\n@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_\n \t\t\tatom->u.remote_ref.push_remote = 1;\n \t\t} else {\n \t\t\tatom->u.remote_ref.option = RR_REF;\n-\t\t\trefname_atom_parser_internal(&atom->u.remote_ref.refname,\n-\t\t\t\t\t\t arg, atom->name);\n+\t\t\tif (refname_atom_parser_internal(&atom->u.remote_ref.refname,\n+\t\t\t\t\t\t\t arg, atom->name, err))\n+\t\t\t\treturn -1;\n \t\t}\n \t}\n \n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\tif (arg)\n-\t\tdie(_(\"%%(body) does not take arguments\"));\n+\tif (arg) {\n+\t\tstrbuf_addstr(err, _(\"%(body) does not take arguments\"));\n+\t\treturn -1;\n+\t}\n \tatom->u.contents.option = C_BODY_DEP;\n+\treturn 0;\n }\n \n-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\tif (arg)\n-\t\tdie(_(\"%%(subject) does not take arguments\"));\n+\tif (arg) {\n+\t\tstrbuf_addstr(err, _(\"%(subject) does not take arguments\"));\n+\t\treturn -1;\n+\t}\n \tatom->u.contents.option = C_SUB;\n+\treturn 0;\n }\n \n-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n {\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n@@ -205,15 +229,19 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at\n \t\t\t\tatom->u.contents.trailer_opts.unfold = 1;\n \t\t\telse if (!strcmp(s, \"only\"))\n \t\t\t\tatom->u.contents.trailer_opts.only_trailers = 1;\n-\t\t\telse\n-\t\t\t\tdie(_(\"unknown %%(trailers) argument: %s\"), s);\n+\t\t\telse {\n+\t\t\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t}\n \tatom->u.contents.option = C_TRAILERS;\n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->u.contents.option = C_BARE;\n@@ -225,16 +253,23 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at\n \t\tatom->u.contents.option = C_SUB;\n \telse if (skip_prefix(arg, \"trailers\", &arg)) {\n \t\tskip_prefix(arg, \":\", &arg);\n-\t\ttrailers_atom_parser(format, atom, *arg ? arg : NULL);\n+\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n+\t\t\treturn -1;\n \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n \t\tatom->u.contents.option = C_LINES;\n-\t\tif (strtoul_ui(arg, 10, &atom->u.contents.nlines))\n-\t\t\tdie(_(\"positive value expected contents:lines=%s\"), arg);\n-\t} else\n-\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), arg);\n+\t\tif (strtoul_ui(arg, 10, &atom->u.contents.nlines)) {\n+\t\t\tstrbuf_addf(err, _(\"positive value expected contents:lines=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(contents) argument: %s\"), arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\t const char *arg, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->u.objectname.option = O_FULL;\n@@ -243,17 +278,23 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_\n \telse if (skip_prefix(arg, \"short=\", &arg)) {\n \t\tatom->u.objectname.option = O_LENGTH;\n \t\tif (strtoul_ui(arg, 10, &atom->u.objectname.length) ||\n-\t\t atom->u.objectname.length == 0)\n-\t\t\tdie(_(\"positive value expected objectname:short=%s\"), arg);\n+\t\t atom->u.objectname.length == 0) {\n+\t\t\tstrbuf_addf(err, _(\"positive value expected objectname:short=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n \t\tif (atom->u.objectname.length < MINIMUM_ABBREV)\n \t\t\tatom->u.objectname.length = MINIMUM_ABBREV;\n-\t} else\n-\t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), arg);\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(objectname) argument: %s\"), arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\trefname_atom_parser_internal(&atom->u.refname, arg, atom->name);\n+\treturn refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);\n }\n \n static align_type parse_align_position(const char *s)\n@@ -267,15 +308,18 @@ static align_type parse_align_position(const char *s)\n \treturn -1;\n }\n \n-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tstruct align *align = &atom->u.align;\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n \tunsigned int width = ~0U;\n \n-\tif (!arg)\n-\t\tdie(_(\"expected format: %%(align:,)\"));\n+\tif (!arg) {\n+\t\tstrbuf_addstr(err, _(\"expected format: %(align:,)\"));\n+\t\treturn -1;\n+\t}\n \n \talign->position = ALIGN_LEFT;\n \n@@ -286,49 +330,64 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom\n \n \t\tif (skip_prefix(s, \"position=\", &s)) {\n \t\t\tposition = parse_align_position(s);\n-\t\t\tif (position < 0)\n-\t\t\t\tdie(_(\"unrecognized position:%s\"), s);\n+\t\t\tif (position < 0) {\n+\t\t\t\tstrbuf_addf(err, _(\"unrecognized position:%s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\talign->position = position;\n \t\t} else if (skip_prefix(s, \"width=\", &s)) {\n-\t\t\tif (strtoul_ui(s, 10, &width))\n-\t\t\t\tdie(_(\"unrecognized width:%s\"), s);\n+\t\t\tif (strtoul_ui(s, 10, &width)) {\n+\t\t\t\tstrbuf_addf(err, _(\"unrecognized width:%s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t} else if (!strtoul_ui(s, 10, &width))\n \t\t\t;\n \t\telse if ((position = parse_align_position(s)) >= 0)\n \t\t\talign->position = position;\n-\t\telse\n-\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s);\n+\t\telse {\n+\t\t\tstrbuf_addf(err, _(\"unrecognized %%(align) argument: %s\"), s);\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \n-\tif (width == ~0U)\n-\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n+\tif (width == ~0U) {\n+\t\tstrbuf_addstr(err, _(\"positive width expected with the %(align) atom\"));\n+\t\treturn -1;\n+\t}\n \talign->width = width;\n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tif (!arg) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_NONE;\n-\t\treturn;\n+\t\treturn 0;\n \t} else if (skip_prefix(arg, \"equals=\", &atom->u.if_then_else.str)) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_EQUAL;\n \t} else if (skip_prefix(arg, \"notequals=\", &atom->u.if_then_else.str)) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;\n \t} else {\n-\t\tdie(_(\"unrecognized %%(if) argument: %s\"), arg);\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(if) argument: %s\"), arg);\n+\t\treturn -1;\n \t}\n+\treturn 0;\n }\n \n-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tatom->u.head = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, NULL, NULL);\n+\treturn 0;\n }\n \n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n-\tvoid (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);\n+\tint (*parser)(const struct ref_format *format, struct used_atom *atom,\n+\t\t const char *arg, struct strbuf *err);\n } valid_atom[] = {\n \t{ \"refname\" , FIELD_STR, refname_atom_parser },\n \t{ \"objecttype\" },\n@@ -457,8 +516,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\t}\n \t}\n \tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n-\tif (valid_atom[i].parser)\n-\t\tvalid_atom[i].parser(format, &used_atom[at], arg);\n+\tif (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))\n+\t\treturn -1;\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342236","messageId":"xmqqbmfjvpgm.fsf@gitster-ct.c.googlers.com","threadId":"48044","inReplyTo":"010201623e59448f-2db7a920-fe77-481a-b105-de6bbff253eb-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 2/5] ref-filter: add return value && strbuf to handlers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-19T19:24:09Z","receivedAt":"2018-03-19T19:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olga Telezhnaya writes:\n\n> Continue removing any printing from ref-filter formatting logic,\n> so that it could be more general.\n\n\"more general\" sounds overly broad. Is this to avoid calling die()\nso that the caller can decide what error messages it wants to give,\nabort operation or not, etc.? From a quick scan of the patch, I\nhave a feeling that \"any printing\" is not the problem you are\nsolving (calling die() is).\n\n> Change the signature of handlers by adding return value\n> and strbuf parameter for errors.\n\nCould you explain what the \"return value\" means? We can see from\nthe patch that the function signature is being changed by the patch.\nWhat needs human explanation is why and what the values mean,\ne.g. \"to allow the caller to notice an error, the function returns 0\nupon success and non-0 upon failure, and an error message is\nappended to the strbuf *err\" (don't pay too much attention to \"0\" or\n\"non-0\" in this example, as I do not know what you chose to assign\nthe return values to signal what to the callers; this is merely to\nillustrate the degree of details I would expect).\n\nThanks.\n"},{"id":"342241","messageId":"xmqq7eq7von9.fsf@gitster-ct.c.googlers.com","threadId":"48044","inReplyTo":"010201623e5944b9-8d44ad36-a6df-4bd3-97e7-de4ee54af3c0-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 3/5] ref-filter: change parsing function error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-19T19:41:46Z","receivedAt":"2018-03-19T19:42:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olga Telezhnaya writes:\n\n> Continue removing any printing from ref-filter formatting logic,\n> so that it could be more general.\n>\n> Change the signature of parse_ref_filter_atom() by adding\n> strbuf parameter for error message.\n> Return value means the same except negative values: they indicate\n> errors (previous version could return only non-negative value).\n\n\"The same\" meaning? The same as before? It shouldn't be too much\nwork to describe what it means instead (and mention that you are not\nchanging the meaning as a sidenote) to help the readers, something\nlike \"The function returns the position in the used_atom[] array (as\nbefore) for the given atom, or a negative value to signal an error\".\n\n"},{"id":"342330","messageId":"01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"010201623e594401-906aa5ca-545b-405a-810a-607491fe09a7-000000@eu-west-1.amazonses.com","subject":"[PATCH v4 1/5] ref-filter: start adding strbufs with errors","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-20T16:05:01Z","receivedAt":"2018-03-20T16:05:17Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"This is a first step in removing die() calls from ref-filter\nformatting logic, so that it could be used by other commands\nthat do not want to die during formatting process.\ndie() calls related to bugs in code will not be touched in this patch.\n\nEverything would be the same for show_ref_array_item() users.\nBut, if you want to deal with errors by your own, you could invoke\nformat_ref_array_item(). It means that you need to print everything\n(the result and errors) on your side.\n\nThis commit changes signature of format_ref_array_item() by adding\nreturn value and strbuf parameter for errors, and adjusts\nits callers. While at it, reduce the scope of the out-variable.\n\nSigned-off-by: Olga Telezhnaia \n---\n builtin/branch.c | 7 +++++--\n ref-filter.c | 17 ++++++++++++-----\n ref-filter.h | 7 ++++---\n 3 files changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6d0cea9d4bcc4..c21e5a04a0177 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tstruct ref_array array;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n-\tstruct strbuf out = STRBUF_INIT;\n \tchar *to_free = NULL;\n \n \t/*\n@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tref_array_sort(sorting, &array);\n \n \tfor (i = 0; i < array.nr; i++) {\n-\t\tformat_ref_array_item(array.items[i], format, &out);\n+\t\tstruct strbuf out = STRBUF_INIT;\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tif (format_ref_array_item(array.items[i], format, &out, &err))\n+\t\t\tdie(\"%s\", err.buf);\n \t\tif (column_active(colopts)) {\n \t\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n \t\t\t /* format to a string_list to let print_columns() do its job */\n@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n \t\t\tputchar('\\n');\n \t\t}\n+\t\tstrbuf_release(&err);\n \t\tstrbuf_release(&out);\n \t}\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex 45fc56216aaa8..54fae00bdd410 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \n-void format_ref_array_item(struct ref_array_item *info,\n+int format_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format,\n-\t\t\t struct strbuf *final_buf)\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf)\n {\n \tconst char *cp, *sp, *ep;\n \tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n@@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\tresetv.s = GIT_COLOR_RESET;\n \t\tappend_atom(&resetv, &state);\n \t}\n-\tif (state.stack->prev)\n-\t\tdie(_(\"format: %%(end) atom missing\"));\n+\tif (state.stack->prev) {\n+\t\tstrbuf_addstr(error_buf, _(\"format: %(end) atom missing\"));\n+\t\treturn -1;\n+\t}\n \tstrbuf_addbuf(final_buf, &state.stack->output);\n \tpop_stack_element(&state.stack);\n+\treturn 0;\n }\n \n void show_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format)\n {\n \tstruct strbuf final_buf = STRBUF_INIT;\n+\tstruct strbuf error_buf = STRBUF_INIT;\n \n-\tformat_ref_array_item(info, format, &final_buf);\n+\tif (format_ref_array_item(info, format, &final_buf, &error_buf))\n+\t\tdie(\"%s\", error_buf.buf);\n \tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\tstrbuf_release(&error_buf);\n \tstrbuf_release(&final_buf);\n \tputchar('\\n');\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 0d98342b34319..e13f8e6f8721a 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n /* Based on the given format and quote_style, fill the strbuf */\n-void format_ref_array_item(struct ref_array_item *info,\n-\t\t\t const struct ref_format *format,\n-\t\t\t struct strbuf *final_buf);\n+int format_ref_array_item(struct ref_array_item *info,\n+\t\t\t const struct ref_format *format,\n+\t\t\t struct strbuf *final_buf,\n+\t\t\t struct strbuf *error_buf);\n /* Print the ref using the given format and quote_style */\n void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n /* Parse a single sort specifier and add it to the list */\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342331","messageId":"010201624428199f-b4b3d8ce-222f-4966-9171-7fcf932ca220-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000@eu-west-1.amazonses.com","subject":"[PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-20T16:05:01Z","receivedAt":"2018-03-20T16:05:22Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Finish removing die() calls from ref-filter formatting logic,\nso that it could be used by other commands.\n\nChange the signature of get_ref_atom_value() and underlying functions\nby adding return value and strbuf parameter for error message.\nReturn value equals 0 upon success and -1 upon failure (there\ncould be more error codes further if necessary).\nUpon failure, error message is appended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 55 ++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 36 insertions(+), 19 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b90cec1056954..85f971663a4aa 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n \treturn show_ref(&atom->u.refname, ref->refname);\n }\n \n-static void get_object(struct ref_array_item *ref, const struct object_id *oid,\n-\t\t int deref, struct object **obj)\n+static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n+\t\t int deref, struct object **obj, struct strbuf *err)\n {\n \tint eaten;\n \tunsigned long size;\n \tvoid *buf = get_obj(oid, obj, &size, &eaten);\n-\tif (!buf)\n-\t\tdie(_(\"missing object %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\tif (!*obj)\n-\t\tdie(_(\"parse_object_buffer failed on %s for %s\"),\n-\t\t oid_to_hex(oid), ref->refname);\n-\n+\tif (!buf) {\n+\t\tstrbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n+\t\t\t ref->refname);\n+\t\tif (!eaten)\n+\t\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n+\tif (!*obj) {\n+\t\tstrbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n+\t\t\t oid_to_hex(oid), ref->refname);\n+\t\tif (!eaten)\n+\t\t\tfree(buf);\n+\t\treturn -1;\n+\t}\n \tgrab_values(ref->value, deref, *obj, buf, size);\n \tif (!eaten)\n \t\tfree(buf);\n+\treturn 0;\n }\n \n /*\n * Parse the object referred by ref, and grab needed value.\n */\n-static void populate_value(struct ref_array_item *ref)\n+static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n {\n \tstruct object *obj;\n \tint i;\n@@ -1588,16 +1596,17 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tbreak;\n \t}\n \tif (used_atom_cnt <= i)\n-\t\treturn;\n+\t\treturn 0;\n \n-\tget_object(ref, &ref->objectname, 0, &obj);\n+\tif (get_object(ref, &ref->objectname, 0, &obj, err))\n+\t\treturn -1;\n \n \t/*\n \t * If there is no atom that wants to know about tagged\n \t * object, we are done.\n \t */\n \tif (!need_tagged || (obj->type != OBJ_TAG))\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If it is a tag object, see if we use a value that derefs\n@@ -1611,20 +1620,23 @@ static void populate_value(struct ref_array_item *ref)\n \t * is not consistent with what deref_tag() does\n \t * which peels the onion to the core.\n \t */\n-\tget_object(ref, tagged, 1, &obj);\n+\treturn get_object(ref, tagged, 1, &obj, err);\n }\n \n /*\n * Given a ref, return the value for the atom. This lazily gets value\n * out of the object by calling populate value.\n */\n-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)\n+static int get_ref_atom_value(struct ref_array_item *ref, int atom,\n+\t\t\t struct atom_value **v, struct strbuf *err)\n {\n \tif (!ref->value) {\n-\t\tpopulate_value(ref);\n+\t\tif (populate_value(ref, err))\n+\t\t\treturn -1;\n \t\tfill_missing_values(ref->value);\n \t}\n \t*v = &ref->value[atom];\n+\treturn 0;\n }\n \n /*\n@@ -2148,9 +2160,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \tint cmp;\n \tcmp_type cmp_type = used_atom[s->atom].type;\n \tint (*cmp_fn)(const char *, const char *);\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tget_ref_atom_value(a, s->atom, &va);\n-\tget_ref_atom_value(b, s->atom, &vb);\n+\tif (get_ref_atom_value(a, s->atom, &va, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tif (get_ref_atom_value(b, s->atom, &vb, &err))\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n \tcmp_fn = s->ignore_case ? strcasecmp : strcmp;\n \tif (s->version)\n \t\tcmp = versioncmp(va->s, vb->s);\n@@ -2230,7 +2246,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tpos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);\n \t\tif (pos < 0)\n \t\t\treturn -1;\n-\t\tget_ref_atom_value(info, pos, &atomv);\n+\t\tif (get_ref_atom_value(info, pos, &atomv, error_buf))\n+\t\t\treturn -1;\n \t\tif (atomv->handler(atomv, &state, error_buf))\n \t\t\treturn -1;\n \t}\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342332","messageId":"010201624428192e-dc81bfca-3acb-4c31-9d4c-1cf2adaee49d-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000@eu-west-1.amazonses.com","subject":"[PATCH v4 3/5] ref-filter: change parsing function error handling","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-20T16:05:01Z","receivedAt":"2018-03-20T16:05:24Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing die() calls from ref-filter formatting logic,\nso that it could be used by other commands.\n\nChange the signature of parse_ref_filter_atom() by adding\nstrbuf parameter for error message.\nThe function returns the position in the used_atom[] array\n(as before) for the given atom, or -1 to signal an error (there\ncould be more negative error codes further if necessary).\nUpon failure, error message is appended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 36 ++++++++++++++++++++++++++----------\n 1 file changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d120360104806..2313a33f0baa4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -397,7 +397,8 @@ struct atom_value {\n * Used to parse format string and sort specifiers\n */\n static int parse_ref_filter_atom(const struct ref_format *format,\n-\t\t\t\t const char *atom, const char *ep)\n+\t\t\t\t const char *atom, const char *ep,\n+\t\t\t\t struct strbuf *err)\n {\n \tconst char *sp;\n \tconst char *arg;\n@@ -406,8 +407,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tsp = atom;\n \tif (*sp == '*' && sp < ep)\n \t\tsp++; /* deref */\n-\tif (ep <= sp)\n-\t\tdie(_(\"malformed field name: %.*s\"), (int)(ep-atom), atom);\n+\tif (ep <= sp) {\n+\t\tstrbuf_addf(err, _(\"malformed field name: %.*s\"), (int)(ep-atom), atom);\n+\t\treturn -1;\n+\t}\n \n \t/* Do we have the atom already used elsewhere? */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n@@ -432,8 +435,10 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\t\tbreak;\n \t}\n \n-\tif (ARRAY_SIZE(valid_atom) <= i)\n-\t\tdie(_(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n+\tif (ARRAY_SIZE(valid_atom) <= i) {\n+\t\tstrbuf_addf(err, _(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n+\t\treturn -1;\n+\t}\n \n \t/* Add it in, including the deref prefix */\n \tat = used_atom_cnt;\n@@ -725,17 +730,21 @@ int verify_ref_format(struct ref_format *format)\n \n \tformat->need_color_reset_at_eol = 0;\n \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n \t\tconst char *color, *ep = strchr(sp, ')');\n \t\tint at;\n \n \t\tif (!ep)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n-\t\tat = parse_ref_filter_atom(format, sp + 2, ep);\n+\t\tat = parse_ref_filter_atom(format, sp + 2, ep, &err);\n+\t\tif (at < 0)\n+\t\t\tdie(\"%s\", err.buf);\n \t\tcp = ep + 1;\n \n \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n \t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n+\t\tstrbuf_release(&err);\n \t}\n \tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n \t\tformat->need_color_reset_at_eol = 0;\n@@ -2154,13 +2163,15 @@ int format_ref_array_item(struct ref_array_item *info,\n \n \tfor (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n+\t\tint pos;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n-\t\tget_ref_atom_value(info,\n-\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n-\t\t\t\t &atomv);\n+\t\tpos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);\n+\t\tif (pos < 0)\n+\t\t\treturn -1;\n+\t\tget_ref_atom_value(info, pos, &atomv);\n \t\tif (atomv->handler(atomv, &state, error_buf))\n \t\t\treturn -1;\n \t}\n@@ -2215,7 +2226,12 @@ static int parse_sorting_atom(const char *atom)\n \t */\n \tstruct ref_format dummy = REF_FORMAT_INIT;\n \tconst char *end = atom + strlen(atom);\n-\treturn parse_ref_filter_atom(&dummy, atom, end);\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint res = parse_ref_filter_atom(&dummy, atom, end, &err);\n+\tif (res < 0)\n+\t\tdie(\"%s\", err.buf);\n+\tstrbuf_release(&err);\n+\treturn res;\n }\n \n /* If no sorting option is given, use refname to sort as default */\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342333","messageId":"010201624428190c-441272b2-bc7d-42af-82f8-6a805bb27f6c-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000@eu-west-1.amazonses.com","subject":"[PATCH v4 2/5] ref-filter: add return value && strbuf to handlers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-20T16:05:01Z","receivedAt":"2018-03-20T16:05:25Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing die() calls from ref-filter formatting logic,\nso that it could be used by other commands.\n\nChange the signature of handlers by adding return value\nand strbuf parameter for errors.\nReturn value equals 0 upon success and -1 upon failure (there could be\nmore error codes further if necessary). Upon failure, error message is\nappended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 71 ++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 48 insertions(+), 23 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 54fae00bdd410..d120360104806 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -387,7 +387,8 @@ struct ref_formatting_state {\n \n struct atom_value {\n \tconst char *s;\n-\tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);\n+\tint (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t struct strbuf *err);\n \tuintmax_t value; /* used for sorting when not FIELD_STR */\n \tstruct used_atom *atom;\n };\n@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)\n \t}\n }\n \n-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n+static int append_atom(struct atom_value *v, struct ref_formatting_state *state,\n+\t\t struct strbuf *unused_err)\n {\n \t/*\n \t * Quote formatting is only done when the stack has a single\n@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct ref_formatting_state *state\n \t\tquote_formatting(&state->stack->output, v->s, state->quote_style);\n \telse\n \t\tstrbuf_addstr(&state->stack->output, v->s);\n+\treturn 0;\n }\n \n static void push_stack_element(struct ref_formatting_stack **stack)\n@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack **stack)\n \tstrbuf_release(&s);\n }\n \n-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *unused_err)\n {\n \tstruct ref_formatting_stack *new_stack;\n \n@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n \tnew_stack = state->stack;\n \tnew_stack->at_end = end_align_handler;\n \tnew_stack->at_end_data = &atomv->atom->u.align;\n+\treturn 0;\n }\n \n static void if_then_else_handler(struct ref_formatting_stack **stack)\n@@ -572,7 +577,8 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)\n \tfree(if_then_else);\n }\n \n-static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *unused_err)\n {\n \tstruct ref_formatting_stack *new_stack;\n \tstruct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);\n@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat\n \tnew_stack = state->stack;\n \tnew_stack->at_end = if_then_else_handler;\n \tnew_stack->at_end_data = if_then_else;\n+\treturn 0;\n }\n \n static int is_empty(const char *s)\n@@ -596,19 +603,24 @@ static int is_empty(const char *s)\n \treturn 1;\n }\n \n-static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *cur = state->stack;\n \tstruct if_then_else *if_then_else = NULL;\n \n \tif (cur->at_end == if_then_else_handler)\n \t\tif_then_else = (struct if_then_else *)cur->at_end_data;\n-\tif (!if_then_else)\n-\t\tdie(_(\"format: %%(then) atom used without an %%(if) atom\"));\n-\tif (if_then_else->then_atom_seen)\n-\t\tdie(_(\"format: %%(then) atom used more than once\"));\n-\tif (if_then_else->else_atom_seen)\n-\t\tdie(_(\"format: %%(then) atom used after %%(else)\"));\n+\tif (!if_then_else) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used without an %(if) atom\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->then_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used more than once\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->else_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(then) atom used after %(else)\"));\n+\t\treturn -1;\n+\t}\n \tif_then_else->then_atom_seen = 1;\n \t/*\n \t * If the 'equals' or 'notequals' attribute is used then\n@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st\n \t} else if (cur->output.len && !is_empty(cur->output.buf))\n \t\tif_then_else->condition_satisfied = 1;\n \tstrbuf_reset(&cur->output);\n+\treturn 0;\n }\n \n-static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *prev = state->stack;\n \tstruct if_then_else *if_then_else = NULL;\n \n \tif (prev->at_end == if_then_else_handler)\n \t\tif_then_else = (struct if_then_else *)prev->at_end_data;\n-\tif (!if_then_else)\n-\t\tdie(_(\"format: %%(else) atom used without an %%(if) atom\"));\n-\tif (!if_then_else->then_atom_seen)\n-\t\tdie(_(\"format: %%(else) atom used without a %%(then) atom\"));\n-\tif (if_then_else->else_atom_seen)\n-\t\tdie(_(\"format: %%(else) atom used more than once\"));\n+\tif (!if_then_else) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used without an %(if) atom\"));\n+\t\treturn -1;\n+\t} else if (!if_then_else->then_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used without a %(then) atom\"));\n+\t\treturn -1;\n+\t} else if (if_then_else->else_atom_seen) {\n+\t\tstrbuf_addstr(err, _(\"format: %(else) atom used more than once\"));\n+\t\treturn -1;\n+\t}\n \tif_then_else->else_atom_seen = 1;\n \tpush_stack_element(&state->stack);\n \tstate->stack->at_end_data = prev->at_end_data;\n \tstate->stack->at_end = prev->at_end;\n+\treturn 0;\n }\n \n-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+static int end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n+\t\t\t struct strbuf *err)\n {\n \tstruct ref_formatting_stack *current = state->stack;\n \tstruct strbuf s = STRBUF_INIT;\n \n-\tif (!current->at_end)\n-\t\tdie(_(\"format: %%(end) atom used without corresponding atom\"));\n+\tif (!current->at_end) {\n+\t\tstrbuf_addstr(err, _(\"format: %(end) atom used without corresponding atom\"));\n+\t\treturn -1;\n+\t}\n \tcurrent->at_end(&state->stack);\n \n \t/* Stack may have been popped within at_end(), hence reset the current pointer */\n@@ -668,6 +690,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\n \t}\n \tstrbuf_release(&s);\n \tpop_stack_element(&state->stack);\n+\treturn 0;\n }\n \n /*\n@@ -2138,7 +2161,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tget_ref_atom_value(info,\n \t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n \t\t\t\t &atomv);\n-\t\tatomv->handler(atomv, &state);\n+\t\tif (atomv->handler(atomv, &state, error_buf))\n+\t\t\treturn -1;\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n@@ -2147,7 +2171,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \tif (format->need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n \t\tresetv.s = GIT_COLOR_RESET;\n-\t\tappend_atom(&resetv, &state);\n+\t\tif (append_atom(&resetv, &state, error_buf))\n+\t\t\treturn -1;\n \t}\n \tif (state.stack->prev) {\n \t\tstrbuf_addstr(error_buf, _(\"format: %(end) atom missing\"));\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342334","messageId":"0102016244281955-6f111db0-ba55-46fc-8db9-7b328fed1372-000000@eu-west-1.amazonses.com","threadId":"48044","inReplyTo":"01020162442818b4-c153f9ce-3813-41a6-aebd-f5cb2b98b1fa-000000@eu-west-1.amazonses.com","subject":"[PATCH v4 4/5] ref-filter: add return value to parsers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-20T16:05:01Z","receivedAt":"2018-03-20T16:05:28Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue removing die() calls from ref-filter formatting logic,\nso that it could be used by other commands.\n\nChange the signature of parsers by adding return value and\nstrbuf parameter for error message.\nReturn value equals 0 upon success and -1 upon failure (there\ncould be more error codes further if necessary).\nUpon failure, error message is appended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia \n---\n ref-filter.c | 177 +++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 118 insertions(+), 59 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 2313a33f0baa4..b90cec1056954 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -101,22 +101,28 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n \n-static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)\n+static int color_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *color_value, struct strbuf *err)\n {\n-\tif (!color_value)\n-\t\tdie(_(\"expected format: %%(color:)\"));\n-\tif (color_parse(color_value, atom->u.color) < 0)\n-\t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n+\tif (!color_value) {\n+\t\tstrbuf_addstr(err, _(\"expected format: %(color:)\"));\n+\t\treturn -1;\n+\t}\n+\tif (color_parse(color_value, atom->u.color) < 0) {\n+\t\tstrbuf_addf(err, _(\"unrecognized color: %%(color:%s)\"), color_value);\n+\t\treturn -1;\n+\t}\n \t/*\n \t * We check this after we've parsed the color, which lets us complain\n \t * about syntactically bogus color names even if they won't be used.\n \t */\n \tif (!want_color(format->use_color))\n \t\tcolor_parse(\"\", atom->u.color);\n+\treturn 0;\n }\n \n-static void refname_atom_parser_internal(struct refname_atom *atom,\n-\t\t\t\t\t const char *arg, const char *name)\n+static int refname_atom_parser_internal(struct refname_atom *atom, const char *arg,\n+\t\t\t\t\t const char *name, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->option = R_NORMAL;\n@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct refname_atom *atom,\n \telse if (skip_prefix(arg, \"lstrip=\", &arg) ||\n \t\t skip_prefix(arg, \"strip=\", &arg)) {\n \t\tatom->option = R_LSTRIP;\n-\t\tif (strtol_i(arg, 10, &atom->lstrip))\n-\t\t\tdie(_(\"Integer value expected refname:lstrip=%s\"), arg);\n+\t\tif (strtol_i(arg, 10, &atom->lstrip)) {\n+\t\t\tstrbuf_addf(err, _(\"Integer value expected refname:lstrip=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n \t} else if (skip_prefix(arg, \"rstrip=\", &arg)) {\n \t\tatom->option = R_RSTRIP;\n-\t\tif (strtol_i(arg, 10, &atom->rstrip))\n-\t\t\tdie(_(\"Integer value expected refname:rstrip=%s\"), arg);\n-\t} else\n-\t\tdie(_(\"unrecognized %%(%s) argument: %s\"), name, arg);\n+\t\tif (strtol_i(arg, 10, &atom->rstrip)) {\n+\t\t\tstrbuf_addf(err, _(\"Integer value expected refname:rstrip=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(%s) argument: %s\"), name, arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\t const char *arg, struct strbuf *err)\n {\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_\n \n \tif (!arg) {\n \t\tatom->u.remote_ref.option = RR_REF;\n-\t\trefname_atom_parser_internal(&atom->u.remote_ref.refname,\n-\t\t\t\t\t arg, atom->name);\n-\t\treturn;\n+\t\treturn refname_atom_parser_internal(&atom->u.remote_ref.refname,\n+\t\t\t\t\t\t arg, atom->name, err);\n \t}\n \n \tatom->u.remote_ref.nobracket = 0;\n@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct ref_format *format, struct used_\n \t\t\tatom->u.remote_ref.push_remote = 1;\n \t\t} else {\n \t\t\tatom->u.remote_ref.option = RR_REF;\n-\t\t\trefname_atom_parser_internal(&atom->u.remote_ref.refname,\n-\t\t\t\t\t\t arg, atom->name);\n+\t\t\tif (refname_atom_parser_internal(&atom->u.remote_ref.refname,\n+\t\t\t\t\t\t\t arg, atom->name, err))\n+\t\t\t\treturn -1;\n \t\t}\n \t}\n \n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\tif (arg)\n-\t\tdie(_(\"%%(body) does not take arguments\"));\n+\tif (arg) {\n+\t\tstrbuf_addstr(err, _(\"%(body) does not take arguments\"));\n+\t\treturn -1;\n+\t}\n \tatom->u.contents.option = C_BODY_DEP;\n+\treturn 0;\n }\n \n-static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int subject_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\tif (arg)\n-\t\tdie(_(\"%%(subject) does not take arguments\"));\n+\tif (arg) {\n+\t\tstrbuf_addstr(err, _(\"%(subject) does not take arguments\"));\n+\t\treturn -1;\n+\t}\n \tatom->u.contents.option = C_SUB;\n+\treturn 0;\n }\n \n-static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n {\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n@@ -205,15 +229,19 @@ static void trailers_atom_parser(const struct ref_format *format, struct used_at\n \t\t\t\tatom->u.contents.trailer_opts.unfold = 1;\n \t\t\telse if (!strcmp(s, \"only\"))\n \t\t\t\tatom->u.contents.trailer_opts.only_trailers = 1;\n-\t\t\telse\n-\t\t\t\tdie(_(\"unknown %%(trailers) argument: %s\"), s);\n+\t\t\telse {\n+\t\t\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t}\n \tatom->u.contents.option = C_TRAILERS;\n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\tconst char *arg, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->u.contents.option = C_BARE;\n@@ -225,16 +253,23 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at\n \t\tatom->u.contents.option = C_SUB;\n \telse if (skip_prefix(arg, \"trailers\", &arg)) {\n \t\tskip_prefix(arg, \":\", &arg);\n-\t\ttrailers_atom_parser(format, atom, *arg ? arg : NULL);\n+\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n+\t\t\treturn -1;\n \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n \t\tatom->u.contents.option = C_LINES;\n-\t\tif (strtoul_ui(arg, 10, &atom->u.contents.nlines))\n-\t\t\tdie(_(\"positive value expected contents:lines=%s\"), arg);\n-\t} else\n-\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), arg);\n+\t\tif (strtoul_ui(arg, 10, &atom->u.contents.nlines)) {\n+\t\t\tstrbuf_addf(err, _(\"positive value expected contents:lines=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(contents) argument: %s\"), arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int objectname_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t\t const char *arg, struct strbuf *err)\n {\n \tif (!arg)\n \t\tatom->u.objectname.option = O_FULL;\n@@ -243,17 +278,23 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_\n \telse if (skip_prefix(arg, \"short=\", &arg)) {\n \t\tatom->u.objectname.option = O_LENGTH;\n \t\tif (strtoul_ui(arg, 10, &atom->u.objectname.length) ||\n-\t\t atom->u.objectname.length == 0)\n-\t\t\tdie(_(\"positive value expected objectname:short=%s\"), arg);\n+\t\t atom->u.objectname.length == 0) {\n+\t\t\tstrbuf_addf(err, _(\"positive value expected objectname:short=%s\"), arg);\n+\t\t\treturn -1;\n+\t\t}\n \t\tif (atom->u.objectname.length < MINIMUM_ABBREV)\n \t\t\tatom->u.objectname.length = MINIMUM_ABBREV;\n-\t} else\n-\t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), arg);\n+\t} else {\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(objectname) argument: %s\"), arg);\n+\t\treturn -1;\n+\t}\n+\treturn 0;\n }\n \n-static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int refname_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n-\trefname_atom_parser_internal(&atom->u.refname, arg, atom->name);\n+\treturn refname_atom_parser_internal(&atom->u.refname, arg, atom->name, err);\n }\n \n static align_type parse_align_position(const char *s)\n@@ -267,15 +308,18 @@ static align_type parse_align_position(const char *s)\n \treturn -1;\n }\n \n-static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int align_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tstruct align *align = &atom->u.align;\n \tstruct string_list params = STRING_LIST_INIT_DUP;\n \tint i;\n \tunsigned int width = ~0U;\n \n-\tif (!arg)\n-\t\tdie(_(\"expected format: %%(align:,)\"));\n+\tif (!arg) {\n+\t\tstrbuf_addstr(err, _(\"expected format: %(align:,)\"));\n+\t\treturn -1;\n+\t}\n \n \talign->position = ALIGN_LEFT;\n \n@@ -286,49 +330,64 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom\n \n \t\tif (skip_prefix(s, \"position=\", &s)) {\n \t\t\tposition = parse_align_position(s);\n-\t\t\tif (position < 0)\n-\t\t\t\tdie(_(\"unrecognized position:%s\"), s);\n+\t\t\tif (position < 0) {\n+\t\t\t\tstrbuf_addf(err, _(\"unrecognized position:%s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\talign->position = position;\n \t\t} else if (skip_prefix(s, \"width=\", &s)) {\n-\t\t\tif (strtoul_ui(s, 10, &width))\n-\t\t\t\tdie(_(\"unrecognized width:%s\"), s);\n+\t\t\tif (strtoul_ui(s, 10, &width)) {\n+\t\t\t\tstrbuf_addf(err, _(\"unrecognized width:%s\"), s);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t} else if (!strtoul_ui(s, 10, &width))\n \t\t\t;\n \t\telse if ((position = parse_align_position(s)) >= 0)\n \t\t\talign->position = position;\n-\t\telse\n-\t\t\tdie(_(\"unrecognized %%(align) argument: %s\"), s);\n+\t\telse {\n+\t\t\tstrbuf_addf(err, _(\"unrecognized %%(align) argument: %s\"), s);\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \n-\tif (width == ~0U)\n-\t\tdie(_(\"positive width expected with the %%(align) atom\"));\n+\tif (width == ~0U) {\n+\t\tstrbuf_addstr(err, _(\"positive width expected with the %(align) atom\"));\n+\t\treturn -1;\n+\t}\n \talign->width = width;\n \tstring_list_clear(¶ms, 0);\n+\treturn 0;\n }\n \n-static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int if_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tif (!arg) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_NONE;\n-\t\treturn;\n+\t\treturn 0;\n \t} else if (skip_prefix(arg, \"equals=\", &atom->u.if_then_else.str)) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_EQUAL;\n \t} else if (skip_prefix(arg, \"notequals=\", &atom->u.if_then_else.str)) {\n \t\tatom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;\n \t} else {\n-\t\tdie(_(\"unrecognized %%(if) argument: %s\"), arg);\n+\t\tstrbuf_addf(err, _(\"unrecognized %%(if) argument: %s\"), arg);\n+\t\treturn -1;\n \t}\n+\treturn 0;\n }\n \n-static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+static int head_atom_parser(const struct ref_format *format, struct used_atom *atom,\n+\t\t\t const char *arg, struct strbuf *err)\n {\n \tatom->u.head = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, NULL, NULL);\n+\treturn 0;\n }\n \n static struct {\n \tconst char *name;\n \tcmp_type cmp_type;\n-\tvoid (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);\n+\tint (*parser)(const struct ref_format *format, struct used_atom *atom,\n+\t\t const char *arg, struct strbuf *err);\n } valid_atom[] = {\n \t{ \"refname\" , FIELD_STR, refname_atom_parser },\n \t{ \"objecttype\" },\n@@ -457,8 +516,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\t}\n \t}\n \tmemset(&used_atom[at].u, 0, sizeof(used_atom[at].u));\n-\tif (valid_atom[i].parser)\n-\t\tvalid_atom[i].parser(format, &used_atom[at], arg);\n+\tif (valid_atom[i].parser && valid_atom[i].parser(format, &used_atom[at], arg, err))\n+\t\treturn -1;\n \tif (*atom == '*')\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"342347","messageId":"CAPig+cT5jh0y9Rmw0E6ns0k5mSwaxAqdaN8oWCayCE8V+jYZow@mail.gmail.com","threadId":"48044","inReplyTo":"010201624428190c-441272b2-bc7d-42af-82f8-6a805bb27f6c-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-20T18:18:42Z","receivedAt":"2018-03-20T18:18:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya\n wrote:\n> Continue removing die() calls from ref-filter formatting logic,\n> so that it could be used by other commands.\n> [...]\n> Signed-off-by: Olga Telezhnaia \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -596,19 +603,24 @@ static int is_empty(const char *s)\n> +static int then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n> + struct strbuf *err)\n> {\n> struct ref_formatting_stack *cur = state->stack;\n> struct if_then_else *if_then_else = NULL;\n>\n> if (cur->at_end == if_then_else_handler)\n> if_then_else = (struct if_then_else *)cur->at_end_data;\n> - if (!if_then_else)\n> - die(_(\"format: %%(then) atom used without an %%(if) atom\"));\n> - if (if_then_else->then_atom_seen)\n> - die(_(\"format: %%(then) atom used more than once\"));\n> - if (if_then_else->else_atom_seen)\n> - die(_(\"format: %%(then) atom used after %%(else)\"));\n> + if (!if_then_else) {\n> + strbuf_addstr(err, _(\"format: %(then) atom used without an %(if) atom\"));\n> + return -1;\n> + } else if (if_then_else->then_atom_seen) {\n> + strbuf_addstr(err, _(\"format: %(then) atom used more than once\"));\n> + return -1;\n> + } else if (if_then_else->else_atom_seen) {\n> + strbuf_addstr(err, _(\"format: %(then) atom used after %(else)\"));\n> + return -1;\n> + }\n\nThis pattern of transforming:\n\n if (cond)\n die(\"...\");\n\nto:\n\n if (cond) {\n strbuf_add*(...);\n return -1;\n }\n\nis repeated many, many times throughout this patch series and makes\nfor quite noisy diffs. Such repetition and noise suggests that this\npatch series (and other similar ones) could benefit from a convenience\nfunction similar to the existing error() function. For instance:\n\n int strbuf_error(struct strbuf *buf, const char *fmt, ...);\n\nwhich appends the formatted message to 'buf' and unconditionally\nreturns -1. Thus, the above transformation simplifies to:\n\n if (cond)\n return strbuf_error(&err, \"...\", ...);\n\nwhich makes for a much less noisy diff, thus is easier to review.\n\nA new patch introducing such a function at the head of this patch\nseries might be welcome.\n"},{"id":"342348","messageId":"CAPig+cSxGgG=1k95j+n4DZHBdtN++7F6_U5fu0gOcjwwPQ5GZw@mail.gmail.com","threadId":"48044","inReplyTo":"010201624428199f-b4b3d8ce-222f-4966-9171-7fcf932ca220-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-20T18:19:11Z","receivedAt":"2018-03-20T18:19:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya\n wrote:\n> ref-filter: get_ref_atom_value() error handling\n\nThis doesn't tell us much about what this patch is doing. Perhaps a\nbetter subject would be:\n\n ref-filter: libify get_ref_atom_value()\n\n> Finish removing die() calls from ref-filter formatting logic,\n> so that it could be used by other commands.\n>\n> Change the signature of get_ref_atom_value() and underlying functions\n> by adding return value and strbuf parameter for error message.\n> Return value equals 0 upon success and -1 upon failure (there\n> could be more error codes further if necessary).\n> Upon failure, error message is appended to the strbuf.\n>\n> Signed-off-by: Olga Telezhnaia \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n> +static int get_object(struct ref_array_item *ref, const struct object_id *oid,\n> + int deref, struct object **obj, struct strbuf *err)\n> {\n> void *buf = get_obj(oid, obj, &size, &eaten);\n> - if (!buf)\n> - die(_(\"missing object %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> - if (!*obj)\n> - die(_(\"parse_object_buffer failed on %s for %s\"),\n> - oid_to_hex(oid), ref->refname);\n> -\n> + if (!buf) {\n> + strbuf_addf(err, _(\"missing object %s for %s\"), oid_to_hex(oid),\n> + ref->refname);\n> + if (!eaten)\n> + free(buf);\n\nSince we are inside the 'if (!buf)' conditional, we _know_ that 'buf'\nis NULL, therefore this free(buff) is unnecessary.\n\n> + return -1;\n> + }\n> + if (!*obj) {\n> + strbuf_addf(err, _(\"parse_object_buffer failed on %s for %s\"),\n> + oid_to_hex(oid), ref->refname);\n> + if (!eaten)\n> + free(buf);\n> + return -1;\n> + }\n> grab_values(ref->value, deref, *obj, buf, size);\n> if (!eaten)\n> free(buf);\n> + return 0;\n> }\n\nOverall, with the need for resource cleanup, this function becomes\nunusually noisy after this change. It could be tamed by doing\nsomething like this:\n\n int ret = 0;\n void *buf = get_obj(oid, obj, &size, &eaten);\n if (!buf)\n ret = strbuf_error(_(\"missing object %s for %s\"),\n oid_to_hex(oid), ref->refname);\n else if (!*obj)\n ret = strbuf_error(_(\"parse_object_buffer failed on %s for %s\"),\n oid_to_hex(oid), ref->refname);\n else\n grab_values(ref->value, deref, *obj, buf, size);\n if (!eaten)\n free(buf);\n return ret;\n"},{"id":"342370","messageId":"xmqqh8paqt0f.fsf@gitster-ct.c.googlers.com","threadId":"48044","inReplyTo":"CAPig+cSxGgG=1k95j+n4DZHBdtN++7F6_U5fu0gOcjwwPQ5GZw@mail.gmail.com","subject":"Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-20T22:30:56Z","receivedAt":"2018-03-20T22:31:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Overall, with the need for resource cleanup, this function becomes\n> unusually noisy after this change. It could be tamed by doing\n> something like this:\n>\n> int ret = 0;\n> void *buf = get_obj(oid, obj, &size, &eaten);\n> if (!buf)\n> ret = strbuf_error(_(\"missing object %s for %s\"),\n> oid_to_hex(oid), ref->refname);\n> else if (!*obj)\n> ret = strbuf_error(_(\"parse_object_buffer failed on %s for %s\"),\n> oid_to_hex(oid), ref->refname);\n> else\n> grab_values(ref->value, deref, *obj, buf, size);\n> if (!eaten)\n> free(buf);\n> return ret;\n\nI have no idea what strbuf_error() that does not take any strbuf is\ndoing, but I think you can initialize ret to -1 (i.e. assume the\nworst at the beginning), and then make the \"ok, we didn't get any\nerrors\" case do\n\n\telse {\n\t\tgrab_values(...);\n\t\tret = 0;\n\t}\n\n"},{"id":"342372","messageId":"CAPig+cQ0cq9-cYuw8AJpk6maeT-T25SJDhWpb4TyN8rQJmP=xg@mail.gmail.com","threadId":"48044","inReplyTo":"xmqqh8paqt0f.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-20T22:50:11Z","receivedAt":"2018-03-20T22:50:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 20, 2018 at 6:30 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> int ret = 0;\n>> void *buf = get_obj(oid, obj, &size, &eaten);\n>> if (!buf)\n>> ret = strbuf_error(_(\"missing object %s for %s\"),\n>> oid_to_hex(oid), ref->refname);\n>> else if (!*obj)\n>> ret = strbuf_error(_(\"parse_object_buffer failed on %s for %s\"),\n>> oid_to_hex(oid), ref->refname);\n>> else\n>> grab_values(ref->value, deref, *obj, buf, size);\n>> if (!eaten)\n>> free(buf);\n>> return ret;\n>\n> I have no idea what strbuf_error() that does not take any strbuf is\n> doing,...\n\nstrbuf_error() was a possibility proposed in [1], and it does take a\nstrbuf. Failure to pass in a strbuf here is just a typo.\n\n> ... but I think you can initialize ret to -1 (i.e. assume the\n> worst at the beginning), and then make the \"ok, we didn't get any\n> errors\" case do\n>\n> else {\n> grab_values(...);\n> ret = 0;\n> }\n\nYes, that also works.\n\n[1]: https://public-inbox.org/git/CAPig+cT5jh0y9Rmw0E6ns0k5mSwaxAqdaN8oWCayCE8V+jYZow@mail.gmail.com/\n"},{"id":"342447","messageId":"xmqq4ll9p6oq.fsf@gitster-ct.c.googlers.com","threadId":"48044","inReplyTo":"CAPig+cQ0cq9-cYuw8AJpk6maeT-T25SJDhWpb4TyN8rQJmP=xg@mail.gmail.com","subject":"Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-21T19:30:45Z","receivedAt":"2018-03-21T19:30:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> I have no idea what strbuf_error() that does not take any strbuf is\n>> doing,...\n>\n> strbuf_error() was a possibility proposed in [1], and it does take a\n> strbuf. Failure to pass in a strbuf here is just a typo.\n\nI've seen it; I just thought it was a joke and not a serious\nsuggestion.\n\nA macro or helper function that is local to the file might be OK,\nbut I do not think \"strbuf_error()\" is a useful abstraction that is\ngeneric enough in the first place (the questions to ask yourself to\nthink about it are: Why should it be limited to return -1? Why\nshould it be limited to always do the addf() to a strbuf?).\n\n"},{"id":"342453","messageId":"CAPig+cQZOjFuAHXqj6c8oP8JJaboLf3kL9heMA=jkXssr+gQ_Q@mail.gmail.com","threadId":"48044","inReplyTo":"xmqq4ll9p6oq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-21T19:58:36Z","receivedAt":"2018-03-21T19:58:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 21, 2018 at 3:30 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> strbuf_error() was a possibility proposed in [1], and it does take a\n>> strbuf. Failure to pass in a strbuf here is just a typo.\n>\n> I've seen it; I just thought it was a joke and not a serious\n> suggestion.\n> A macro or helper function that is local to the file might be OK,\n\nMy thought all along was that this convenience helper (in whatever\nform) should start life local to ref-filter.c, and only be published\nmore widely if found to be generally useful. Unfortunately, I forgot\nto state so explicitly when writing the review. Suggesting the name\nstrbuf_error() didn't help to convey that thought either. My bad.\n\n> but I do not think \"strbuf_error()\" is a useful abstraction that is\n> generic enough in the first place (the questions to ask yourself to\n> think about it are: Why should it be limited to return -1? Why\n> should it be limited to always do the addf() to a strbuf?).\n\nThere is some precedent in the existing error() function. As with\nerror(), as a _convenience_ function, it does not necessarily have to\nbe universally general. That it simplifies a reasonably large body of\ncode may be justification enough, despite it shortcomings. I don't\nfeel strongly about it, though, and, as noted above, agree that it can\nbe local to ref-filter.c.\n"}]}