Re: [PATCH v2 3/5] ref-filter: change parsing function error handling

[Оля Тележная]

2018-03-16 1:48 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Continue removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>
> Hmm.
>
>> Change the signature of parse_ref_filter_atom() by changing return value,
>> adding previous return value to function parameter and also adding
>> strbuf parameter for error message.
>
> This says what the patch changes, but it does not explain why it is
> a good idea to return something with different meaning to the
> callers (which of course forces us to update all callers so that
> they pass  pointer and check the value returned in the
> variable), or more importantly what meaning the return value has and
> how the callers are expected to use it.  While at it, it probably is
> a good idea to explain what the original return value means.
>
> The return value from parse_ref_filter_atom() used to be the
> position the atom is found in the used_atom[] array; that
> information is now returned in an integer pointed at by the
> *res parameter.  The function now returns 0 for success and
> -1 for failure.
>
> or something like that.
>
> Having said that, I wonder if a calling convention that does not
> force callers to pass in a pointer-to-int may make more sense.
> Because the original return value is an index into an array, we know
> the normal return values are not negative.  An updated caller could
> become like this instead:
>
> pos = parse_ref_filter_atom(format, atom, ep, );
> if (pos < 0)
> die("%s", err.buf);
> ... original code that used 'pos' can stay as before ...
>

Martin also mentioned that, but I was not sure which solution is
better. Great, thanks, I will fix that.

Re: [PATCH v2 3/5] ref-filter: change parsing function error handling

[Junio C Hamano]

Olga Telezhnaya  writes:

> Continue removing any printing from ref-filter formatting logic,
> so that it could be more general.

Hmm.

> Change the signature of parse_ref_filter_atom() by changing return value,
> adding previous return value to function parameter and also adding
> strbuf parameter for error message.

This says what the patch changes, but it does not explain why it is
a good idea to return something with different meaning to the
callers (which of course forces us to update all callers so that
they pass  pointer and check the value returned in the
variable), or more importantly what meaning the return value has and
how the callers are expected to use it.  While at it, it probably is
a good idea to explain what the original return value means.

The return value from parse_ref_filter_atom() used to be the
position the atom is found in the used_atom[] array; that
information is now returned in an integer pointed at by the
*res parameter.  The function now returns 0 for success and
-1 for failure.

or something like that.

Having said that, I wonder if a calling convention that does not
force callers to pass in a pointer-to-int may make more sense.
Because the original return value is an index into an array, we know
the normal return values are not negative.  An updated caller could
become like this instead:

pos = parse_ref_filter_atom(format, atom, ep, );
if (pos < 0)
die("%s", err.buf);
... original code that used 'pos' can stay as before ...

[PATCH v2 3/5] ref-filter: change parsing function error handling

[Olga Telezhnaya]

Continue removing any printing from ref-filter formatting logic,
so that it could be more general.

Change the signature of parse_ref_filter_atom() by changing return value,
adding previous return value to function parameter and also adding
strbuf parameter for error message.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 43 ++-
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index d120360104806..dd83ef326511d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -397,7 +397,8 @@ struct atom_value {
  * Used to parse format string and sort specifiers
  */
 static int parse_ref_filter_atom(const struct ref_format *format,
-const char *atom, const char *ep)
+const char *atom, const char *ep, int *res,
+struct strbuf *err)
 {
const char *sp;
const char *arg;
@@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
sp = atom;
if (*sp == '*' && sp < ep)
sp++; /* deref */
-   if (ep <= sp)
-   die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+   if (ep <= sp) {
+   strbuf_addf(err, _("malformed field name: %.*s"), 
(int)(ep-atom), atom);
+   return -1;
+   }
 
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
int len = strlen(used_atom[i].name);
-   if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
-   return i;
+   if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) {
+   *res = i;
+   return 0;
+   }
}
 
/*
@@ -432,8 +437,10 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
break;
}
 
-   if (ARRAY_SIZE(valid_atom) <= i)
-   die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+   if (ARRAY_SIZE(valid_atom) <= i) {
+   strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), 
atom);
+   return -1;
+   }
 
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
-   return at;
+   *res = at;
+   return 0;
 }
 
 static void quote_formatting(struct strbuf *s, const char *str, int 
quote_style)
@@ -725,17 +733,20 @@ int verify_ref_format(struct ref_format *format)
 
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+   struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
 
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
-   at = parse_ref_filter_atom(format, sp + 2, ep);
+   if (parse_ref_filter_atom(format, sp + 2, ep, , ))
+   die("%s", err.buf);
cp = ep + 1;
 
if (skip_prefix(used_atom[at].name, "color:", ))
format->need_color_reset_at_eol = !!strcmp(color, 
"reset");
+   strbuf_release();
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2154,13 +2165,14 @@ int format_ref_array_item(struct ref_array_item *info,
 
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+   int pos;
 
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, );
-   get_ref_atom_value(info,
-  parse_ref_filter_atom(format, sp + 2, ep),
-  );
+   if (parse_ref_filter_atom(format, sp + 2, ep, , error_buf))
+   return -1;
+   get_ref_atom_value(info, pos, );
if (atomv->handler(atomv, , error_buf))
return -1;
}
@@ -2215,7 +2227,12 @@ static int parse_sorting_atom(const char *atom)
 */
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
-   return parse_ref_filter_atom(, atom, end);
+   struct strbuf err = STRBUF_INIT;
+   int res;
+   if (parse_ref_filter_atom(, atom, end, , ))
+   die("%s", err.buf);
+   strbuf_release();
+   return res;
 }
 
 /*  If no sorting option is given, use refname to sort as