{"thread":{"id":"48183","subject":"[PATCH v6 4/6] ref-filter: change parsing function error handling","startedAt":"2018-03-29T12:49:52Z","lastAt":"2018-03-29T12:50:07Z","messageCount":6,"participants":["Olga Telezhnaya"],"isPatch":true,"patchVersion":6,"patchTotal":6},"messages":[{"id":"343315","messageId":"0102016271ce919c-7976644e-d024-4f15-a634-2df2a1c633db-000000@eu-west-1.amazonses.com","threadId":"48183","inReplyTo":"0102016271ce90fc-1bd75012-add6-49ee-bb32-66eeeb1cc3df-000000@eu-west-1.amazonses.com","subject":"[PATCH v6 4/6] ref-filter: change parsing function error handling","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-29T12:49:45Z","receivedAt":"2018-03-29T12:49:52Z","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.\nUpon failure, error message is appended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>\n---\n ref-filter.c | 32 ++++++++++++++++++++++++--------\n 1 file changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a18c86961f08c..93fa6b4e5e63d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -410,7 +410,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@@ -420,7 +421,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\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+\t\treturn strbuf_addf_ret(err, -1, _(\"malformed field name: %.*s\"),\n+\t\t\t\t       (int)(ep-atom), atom);\n \n \t/* Do we have the atom already used elsewhere? */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n@@ -446,7 +448,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t}\n \n \tif (ARRAY_SIZE(valid_atom) <= i)\n-\t\tdie(_(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n+\t\treturn strbuf_addf_ret(err, -1, _(\"unknown field name: %.*s\"),\n+\t\t\t\t       (int)(ep-atom), atom);\n \n \t/* Add it in, including the deref prefix */\n \tat = used_atom_cnt;\n@@ -728,17 +731,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@@ -2157,13 +2164,17 @@ 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\tpop_stack_element(&state.stack);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tget_ref_atom_value(info, pos, &atomv);\n \t\tif (atomv->handler(atomv, &state, error_buf)) {\n \t\t\tpop_stack_element(&state.stack);\n \t\t\treturn -1;\n@@ -2222,7 +2233,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":"343316","messageId":"0102016271ce90fc-1bd75012-add6-49ee-bb32-66eeeb1cc3df-000000@eu-west-1.amazonses.com","threadId":"48183","inReplyTo":"0102016249d21c40-0edf6647-4d26-46fc-8cfd-5a446b93a5e2-000000@eu-west-1.amazonses.com","subject":"[PATCH v6 1/6] ref-filter: add shortcut to work with strbufs","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-29T12:49:45Z","receivedAt":"2018-03-29T12:49:55Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Add function strbuf_addf_ret() that helps to save a few lines of code.\nFunction expands fmt with placeholders, append resulting message\nto strbuf *sb, and return error code ret.\n\nSigned-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>\n---\n ref-filter.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 45fc56216aaa8..0c8d1589cf316 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -101,6 +101,19 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n \n+/*\n+ * Expand string, append it to strbuf *sb, then return error code ret.\n+ * Allow to save few lines of code.\n+ */\n+static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(sb, fmt, ap);\n+\tva_end(ap);\n+\treturn ret;\n+}\n+\n static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)\n {\n \tif (!color_value)\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"343317","messageId":"0102016271ce916b-f246ef25-49df-456c-9e76-7b1ae0de3c96-000000@eu-west-1.amazonses.com","threadId":"48183","inReplyTo":"0102016271ce90fc-1bd75012-add6-49ee-bb32-66eeeb1cc3df-000000@eu-west-1.amazonses.com","subject":"[PATCH v6 2/6] ref-filter: start adding strbufs with errors","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-29T12:49:45Z","receivedAt":"2018-03-29T12:50:01Z","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 <olyatelezhnaya@gmail.com>\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 0c8d1589cf316..9833709dbefe3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2131,9 +2131,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@@ -2161,19 +2162,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\tpop_stack_element(&state.stack);\n+\t\treturn strbuf_addf_ret(error_buf, -1, _(\"format: %%(end) atom missing\"));\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":"343318","messageId":"0102016271ce91a9-07b0e717-34fd-46a4-a475-58715a1a038b-000000@eu-west-1.amazonses.com","threadId":"48183","inReplyTo":"0102016271ce90fc-1bd75012-add6-49ee-bb32-66eeeb1cc3df-000000@eu-west-1.amazonses.com","subject":"[PATCH v6 5/6] ref-filter: add return value to parsers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-29T12:49:45Z","receivedAt":"2018-03-29T12:50:03Z","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.\nUpon failure, error message is appended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>\n---\n ref-filter.c | 138 ++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 89 insertions(+), 49 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 93fa6b4e5e63d..3f85ef64267d9 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -114,22 +114,25 @@ static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)\n \treturn ret;\n }\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:<color>)\"));\n+\t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(color:<color>)\"));\n \tif (color_parse(color_value, atom->u.color) < 0)\n-\t\tdie(_(\"unrecognized color: %%(color:%s)\"), color_value);\n+\t\treturn strbuf_addf_ret(err, -1, _(\"unrecognized color: %%(color:%s)\"),\n+\t\t\t\t       color_value);\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@@ -139,16 +142,18 @@ static void refname_atom_parser_internal(struct refname_atom *atom,\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\t\treturn strbuf_addf_ret(err, -1, _(\"Integer value expected refname:lstrip=%s\"), arg);\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\t\treturn strbuf_addf_ret(err, -1, _(\"Integer value expected refname:rstrip=%s\"), arg);\n \t} else\n-\t\tdie(_(\"unrecognized %%(%s) argument: %s\"), name, arg);\n+\t\treturn strbuf_addf_ret(err, -1, _(\"unrecognized %%(%s) argument: %s\"), name, arg);\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@@ -158,9 +163,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@@ -183,29 +187,38 @@ 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\tstring_list_clear(&params, 0);\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t}\n \n \tstring_list_clear(&params, 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+\t\treturn strbuf_addf_ret(err, -1, _(\"%%(body) does not take arguments\"));\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+\t\treturn strbuf_addf_ret(err, -1, _(\"%%(subject) does not take arguments\"));\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@@ -218,15 +231,20 @@ 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\tstring_list_clear(&params, 0);\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(&params, 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@@ -238,16 +256,19 @@ 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\t\treturn strbuf_addf_ret(err, -1, _(\"positive value expected contents:lines=%s\"), arg);\n \t} else\n-\t\tdie(_(\"unrecognized %%(contents) argument: %s\"), arg);\n+\t\treturn strbuf_addf_ret(err, -1, _(\"unrecognized %%(contents) argument: %s\"), arg);\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@@ -257,16 +278,18 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_\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\treturn strbuf_addf_ret(err, -1, _(\"positive value expected objectname:short=%s\"), arg);\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\treturn strbuf_addf_ret(err, -1, _(\"unrecognized %%(objectname) argument: %s\"), arg);\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@@ -280,7 +303,8 @@ 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@@ -288,7 +312,7 @@ static void align_atom_parser(const struct ref_format *format, struct used_atom\n \tunsigned int width = ~0U;\n \n \tif (!arg)\n-\t\tdie(_(\"expected format: %%(align:<width>,<position>)\"));\n+\t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(align:<width>,<position>)\"));\n \n \talign->position = ALIGN_LEFT;\n \n@@ -299,49 +323,65 @@ 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\tstring_list_clear(&params, 0);\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\tstring_list_clear(&params, 0);\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\tstring_list_clear(&params, 0);\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\tstring_list_clear(&params, 0);\n+\t\treturn strbuf_addf_ret(err, -1, _(\"positive width expected with the %%(align) atom\"));\n+\t}\n \talign->width = width;\n \tstring_list_clear(&params, 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}\n+\t} else\n+\t\treturn strbuf_addf_ret(err, -1, _(\"unrecognized %%(if) argument: %s\"), arg);\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 *unused_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@@ -468,8 +508,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":"343319","messageId":"0102016271ce91a8-81115fbe-a57e-40ba-be4b-a5af72dd2763-000000@eu-west-1.amazonses.com","threadId":"48183","inReplyTo":"0102016271ce90fc-1bd75012-add6-49ee-bb32-66eeeb1cc3df-000000@eu-west-1.amazonses.com","subject":"[PATCH v6 6/6] ref-filter: libify get_ref_atom_value()","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-29T12:49:45Z","receivedAt":"2018-03-29T12:50:05Z","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.\nUpon failure, error message is appended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>\n---\n ref-filter.c | 54 ++++++++++++++++++++++++++++++------------------------\n 1 file changed, 30 insertions(+), 24 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3f85ef64267d9..3bc65e49358ee 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1427,28 +1427,30 @@ 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+\tint ret = 0;\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+\t\tret = strbuf_addf_ret(err, -1, _(\"missing object %s for %s\"),\n+\t\t\t\t      oid_to_hex(oid), ref->refname);\n+\telse if (!*obj)\n+\t\tret = strbuf_addf_ret(err, -1, _(\"parse_object_buffer failed on %s for %s\"),\n+\t\t\t\t      oid_to_hex(oid), ref->refname);\n+\telse\n+\t\tgrab_values(ref->value, deref, *obj, buf, size);\n \tif (!eaten)\n \t\tfree(buf);\n+\treturn ret;\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@@ -1570,16 +1572,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@@ -1593,20 +1596,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@@ -2130,9 +2136,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@@ -2210,12 +2220,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n \t\tpos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);\n-\t\tif (pos < 0) {\n-\t\t\tpop_stack_element(&state.stack);\n-\t\t\treturn -1;\n-\t\t}\n-\t\tget_ref_atom_value(info, pos, &atomv);\n-\t\tif (atomv->handler(atomv, &state, error_buf)) {\n+\t\tif (pos < 0 || get_ref_atom_value(info, pos, &atomv, error_buf) ||\n+\t\t    atomv->handler(atomv, &state, error_buf)) {\n \t\t\tpop_stack_element(&state.stack);\n \t\t\treturn -1;\n \t\t}\n\n--\nhttps://github.com/git/git/pull/466\n"},{"id":"343320","messageId":"0102016271ce91a2-4925634f-9399-4f98-8d13-0a15b92d7cdc-000000@eu-west-1.amazonses.com","threadId":"48183","inReplyTo":"0102016271ce90fc-1bd75012-add6-49ee-bb32-66eeeb1cc3df-000000@eu-west-1.amazonses.com","subject":"[PATCH v6 3/6] ref-filter: add return value && strbuf to handlers","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-03-29T12:49:45Z","receivedAt":"2018-03-29T12:50:07Z","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.\nUpon failure, error message is appended to the strbuf.\n\nSigned-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>\n---\n ref-filter.c | 51 +++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 35 insertions(+), 16 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 9833709dbefe3..a18c86961f08c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -400,7 +400,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@@ -494,7 +495,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@@ -506,6 +508,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@@ -540,7 +543,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@@ -548,6 +552,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@@ -585,7 +590,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@@ -597,6 +603,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@@ -609,7 +616,8 @@ 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@@ -617,11 +625,11 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st\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+\t\treturn strbuf_addf_ret(err, -1, _(\"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+\t\treturn strbuf_addf_ret(err, -1, _(\"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+\t\treturn strbuf_addf_ret(err, -1, _(\"format: %%(then) atom used after %%(else)\"));\n \tif_then_else->then_atom_seen = 1;\n \t/*\n \t * If the 'equals' or 'notequals' attribute is used then\n@@ -637,9 +645,11 @@ 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@@ -647,24 +657,26 @@ static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_st\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+\t\treturn strbuf_addf_ret(err, -1, _(\"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+\t\treturn strbuf_addf_ret(err, -1, _(\"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+\t\treturn strbuf_addf_ret(err, -1, _(\"format: %%(else) atom used more than once\"));\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+\t\treturn strbuf_addf_ret(err, -1, _(\"format: %%(end) atom used without corresponding atom\"));\n \tcurrent->at_end(&state->stack);\n \n \t/*  Stack may have been popped within at_end(), hence reset the current pointer */\n@@ -681,6 +693,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@@ -2151,7 +2164,10 @@ 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\tpop_stack_element(&state.stack);\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n@@ -2160,7 +2176,10 @@ 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\tpop_stack_element(&state.stack);\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \tif (state.stack->prev) {\n \t\tpop_stack_element(&state.stack);\n\n--\nhttps://github.com/git/git/pull/466\n"}]}