Re: [RFC 3/4] ref-filter: change parsing function error handling
2018-03-13 22:18 GMT+03:00 Martin Ågren : > On 13 March 2018 at 11:16, Olga Telezhnaya wrote: >> Continue removing any printing from ref-filter formatting logic, >> so that it could be more general. >> >> Change the signature of parse_ref_filter_atom() by changing return value, >> adding previous return value to function parameter and also adding >> strbuf parameter for error message. > > I think the current return value is always non-negative. Maybe it would > be easier to leave the return value as-is, except return negative on > error? Unless I am missing something? That's interesting. I like your idea, but let's see what other people think. If others agree with us, I am ready to implement your solution. > >> >> Signed-off-by: Olga Telezhnaia >> --- >> ref-filter.c | 45 - >> 1 file changed, 32 insertions(+), 13 deletions(-) >> >> diff --git a/ref-filter.c b/ref-filter.c >> index 07bedc636398c..e146215bf1e64 100644 >> --- a/ref-filter.c >> +++ b/ref-filter.c >> @@ -397,7 +397,8 @@ struct atom_value { >> * Used to parse format string and sort specifiers >> */ >> static int parse_ref_filter_atom(const struct ref_format *format, >> -const char *atom, const char *ep) >> +const char *atom, const char *ep, int *res, >> +struct strbuf *err) >> { >> const char *sp; >> const char *arg; >> @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct >> ref_format *format, >> sp = atom; >> if (*sp == '*' && sp < ep) >> sp++; /* deref */ >> - if (ep <= sp) >> - die(_("malformed field name: %.*s"), (int)(ep-atom), atom); >> + if (ep <= sp) { >> + strbuf_addf(err, _("malformed field name: %.*s"), >> (int)(ep-atom), atom); >> + return -1; >> + } >> >> /* Do we have the atom already used elsewhere? */ >> for (i = 0; i < used_atom_cnt; i++) { >> int len = strlen(used_atom[i].name); >> - if (len == ep - atom && !memcmp(used_atom[i].name, atom, >> len)) >> - return i; >> + if (len == ep - atom && !memcmp(used_atom[i].name, atom, >> len)) { >> + *res = i; >> + return 0; >> + } >> } > > If you did so, this hunk above would not need to be changed ... > >> @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format >> *format, >> need_tagged = 1; >> if (!strcmp(valid_atom[i].name, "symref")) >> need_symref = 1; >> - return at; >> + *res = at; >> + return 0; >> } > > ... nor this one above ... > >> if (!ep) >> return error(_("malformed format string %s"), sp); >> /* sp points at "%(" and ep points at the closing ")" */ >> - at = parse_ref_filter_atom(format, sp + 2, ep); >> + if (parse_ref_filter_atom(format, sp + 2, ep, &at, &err)) >> + die("%s", err.buf); > > And this would be more like "if (at < 0) die(...)". > >> for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) { >> struct atom_value *atomv; >> + struct strbuf err = STRBUF_INIT; >> + int pos; >> >> ep = strchr(sp, ')'); >> if (cp < sp) >> append_literal(cp, sp, &state); >> - get_ref_atom_value(info, >> - parse_ref_filter_atom(format, sp + 2, ep), >> - &atomv); >> + if (parse_ref_filter_atom(format, sp + 2, ep, &pos, &err)) >> + return -1; >> + get_ref_atom_value(info, pos, &atomv); >> if (atomv->handler(atomv, &state, error_buf)) >> return -1; >> + strbuf_release(&err); > > This looks leaky: if we get an error, we've got something in the buffer > but we do not release it because we return early. Stepping back a bit, I > wonder why we do not do anything at all with "err". Stepping back a bit > more :-) I wonder if you could get rid of "err" and pass "error_buf" to > parse_ref_filter_atom() instead. Our caller would like to have access to > the error string? Fully agree, I don't know why I decided to create one more buffer. Fixed. > > This ties back to my comment on the first patch -- "return negative if > and only if you add some error string to the buffer" might be a useful > rule? > > Martin
Re: [RFC 3/4] ref-filter: change parsing function error handling
On 13 March 2018 at 11:16, Olga Telezhnaya wrote: > Continue removing any printing from ref-filter formatting logic, > so that it could be more general. > > Change the signature of parse_ref_filter_atom() by changing return value, > adding previous return value to function parameter and also adding > strbuf parameter for error message. I think the current return value is always non-negative. Maybe it would be easier to leave the return value as-is, except return negative on error? Unless I am missing something? > > Signed-off-by: Olga Telezhnaia > --- > ref-filter.c | 45 - > 1 file changed, 32 insertions(+), 13 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 07bedc636398c..e146215bf1e64 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -397,7 +397,8 @@ struct atom_value { > * Used to parse format string and sort specifiers > */ > static int parse_ref_filter_atom(const struct ref_format *format, > -const char *atom, const char *ep) > +const char *atom, const char *ep, int *res, > +struct strbuf *err) > { > const char *sp; > const char *arg; > @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct > ref_format *format, > sp = atom; > if (*sp == '*' && sp < ep) > sp++; /* deref */ > - if (ep <= sp) > - die(_("malformed field name: %.*s"), (int)(ep-atom), atom); > + if (ep <= sp) { > + strbuf_addf(err, _("malformed field name: %.*s"), > (int)(ep-atom), atom); > + return -1; > + } > > /* Do we have the atom already used elsewhere? */ > for (i = 0; i < used_atom_cnt; i++) { > int len = strlen(used_atom[i].name); > - if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) > - return i; > + if (len == ep - atom && !memcmp(used_atom[i].name, atom, > len)) { > + *res = i; > + return 0; > + } > } If you did so, this hunk above would not need to be changed ... > @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format > *format, > need_tagged = 1; > if (!strcmp(valid_atom[i].name, "symref")) > need_symref = 1; > - return at; > + *res = at; > + return 0; > } ... nor this one above ... > if (!ep) > return error(_("malformed format string %s"), sp); > /* sp points at "%(" and ep points at the closing ")" */ > - at = parse_ref_filter_atom(format, sp + 2, ep); > + if (parse_ref_filter_atom(format, sp + 2, ep, &at, &err)) > + die("%s", err.buf); And this would be more like "if (at < 0) die(...)". > for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) { > struct atom_value *atomv; > + struct strbuf err = STRBUF_INIT; > + int pos; > > ep = strchr(sp, ')'); > if (cp < sp) > append_literal(cp, sp, &state); > - get_ref_atom_value(info, > - parse_ref_filter_atom(format, sp + 2, ep), > - &atomv); > + if (parse_ref_filter_atom(format, sp + 2, ep, &pos, &err)) > + return -1; > + get_ref_atom_value(info, pos, &atomv); > if (atomv->handler(atomv, &state, error_buf)) > return -1; > + strbuf_release(&err); This looks leaky: if we get an error, we've got something in the buffer but we do not release it because we return early. Stepping back a bit, I wonder why we do not do anything at all with "err". Stepping back a bit more :-) I wonder if you could get rid of "err" and pass "error_buf" to parse_ref_filter_atom() instead. Our caller would like to have access to the error string? This ties back to my comment on the first patch -- "return negative if and only if you add some error string to the buffer" might be a useful rule? Martin
[RFC 3/4] ref-filter: change parsing function error handling
Continue removing any printing from ref-filter formatting logic, so that it could be more general. Change the signature of parse_ref_filter_atom() by changing return value, adding previous return value to function parameter and also adding strbuf parameter for error message. Signed-off-by: Olga Telezhnaia --- ref-filter.c | 45 - 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 07bedc636398c..e146215bf1e64 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -397,7 +397,8 @@ struct atom_value { * Used to parse format string and sort specifiers */ static int parse_ref_filter_atom(const struct ref_format *format, -const char *atom, const char *ep) +const char *atom, const char *ep, int *res, +struct strbuf *err) { const char *sp; const char *arg; @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format *format, sp = atom; if (*sp == '*' && sp < ep) sp++; /* deref */ - if (ep <= sp) - die(_("malformed field name: %.*s"), (int)(ep-atom), atom); + if (ep <= sp) { + strbuf_addf(err, _("malformed field name: %.*s"), (int)(ep-atom), atom); + return -1; + } /* Do we have the atom already used elsewhere? */ for (i = 0; i < used_atom_cnt; i++) { int len = strlen(used_atom[i].name); - if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) - return i; + if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) { + *res = i; + return 0; + } } /* @@ -432,8 +437,10 @@ static int parse_ref_filter_atom(const struct ref_format *format, break; } - if (ARRAY_SIZE(valid_atom) <= i) - die(_("unknown field name: %.*s"), (int)(ep-atom), atom); + if (ARRAY_SIZE(valid_atom) <= i) { + strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), atom); + return -1; + } /* Add it in, including the deref prefix */ at = used_atom_cnt; @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format *format, need_tagged = 1; if (!strcmp(valid_atom[i].name, "symref")) need_symref = 1; - return at; + *res = at; + return 0; } static void quote_formatting(struct strbuf *s, const char *str, int quote_style) @@ -725,17 +733,20 @@ int verify_ref_format(struct ref_format *format) format->need_color_reset_at_eol = 0; for (cp = format->format; *cp && (sp = find_next(cp)); ) { + struct strbuf err = STRBUF_INIT; const char *color, *ep = strchr(sp, ')'); int at; if (!ep) return error(_("malformed format string %s"), sp); /* sp points at "%(" and ep points at the closing ")" */ - at = parse_ref_filter_atom(format, sp + 2, ep); + if (parse_ref_filter_atom(format, sp + 2, ep, &at, &err)) + die("%s", err.buf); cp = ep + 1; if (skip_prefix(used_atom[at].name, "color:", &color)) format->need_color_reset_at_eol = !!strcmp(color, "reset"); + strbuf_release(&err); } if (format->need_color_reset_at_eol && !want_color(format->use_color)) format->need_color_reset_at_eol = 0; @@ -2154,15 +2165,18 @@ int format_ref_array_item(struct ref_array_item *info, for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) { struct atom_value *atomv; + struct strbuf err = STRBUF_INIT; + int pos; ep = strchr(sp, ')'); if (cp < sp) append_literal(cp, sp, &state); - get_ref_atom_value(info, - parse_ref_filter_atom(format, sp + 2, ep), - &atomv); + if (parse_ref_filter_atom(format, sp + 2, ep, &pos, &err)) + return -1; + get_ref_atom_value(info, pos, &atomv); if (atomv->handler(atomv, &state, error_buf)) return -1; + strbuf_release(&err); } if (*cp) { sp = cp + strlen(cp); @@ -2215,7 +2229,12 @@ static int parse_sorting_atom(const char *atom) */ struct ref_format dummy = REF_FORMAT_INIT; const char *end = atom + strlen(atom); - return parse_ref_filter_atom(&dummy, atom, end); + struct strbuf err = STRBUF_INIT; + int res; + if (parse_ref_filter_atom(&d