Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Eric Sunshine]

On Wed, Mar 21, 2018 at 3:30 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> strbuf_error() was a possibility proposed in [1], and it does take a
>> strbuf. Failure to pass in a strbuf here is just a typo.
>
> I've seen it; I just thought it was a joke and not a serious
> suggestion.
> A macro or helper function that is local to the file might be OK,

My thought all along was that this convenience helper (in whatever
form) should start life local to ref-filter.c, and only be published
more widely if found to be generally useful. Unfortunately, I forgot
to state so explicitly when writing the review. Suggesting the name
strbuf_error() didn't help to convey that thought either. My bad.

> but I do not think "strbuf_error()" is a useful abstraction that is
> generic enough in the first place (the questions to ask yourself to
> think about it are: Why should it be limited to return -1?  Why
> should it be limited to always do the addf() to a strbuf?).

There is some precedent in the existing error() function. As with
error(), as a _convenience_ function, it does not necessarily have to
be universally general. That it simplifies a reasonably large body of
code may be justification enough, despite it shortcomings. I don't
feel strongly about it, though, and, as noted above, agree that it can
be local to ref-filter.c.

Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Junio C Hamano]

Eric Sunshine  writes:

>> I have no idea what strbuf_error() that does not take any strbuf is
>> doing,...
>
> strbuf_error() was a possibility proposed in [1], and it does take a
> strbuf. Failure to pass in a strbuf here is just a typo.

I've seen it; I just thought it was a joke and not a serious
suggestion.

A macro or helper function that is local to the file might be OK,
but I do not think "strbuf_error()" is a useful abstraction that is
generic enough in the first place (the questions to ask yourself to
think about it are: Why should it be limited to return -1?  Why
should it be limited to always do the addf() to a strbuf?).

Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Eric Sunshine]

On Tue, Mar 20, 2018 at 6:30 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> int ret = 0;
>> void *buf = get_obj(oid, obj, , );
>> if (!buf)
>> ret = strbuf_error(_("missing object %s for %s"),
>> oid_to_hex(oid), ref->refname);
>> else if (!*obj)
>> ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
>> oid_to_hex(oid), ref->refname);
>> else
>> grab_values(ref->value, deref, *obj, buf, size);
>>if (!eaten)
>> free(buf);
>> return ret;
>
> I have no idea what strbuf_error() that does not take any strbuf is
> doing,...

strbuf_error() was a possibility proposed in [1], and it does take a
strbuf. Failure to pass in a strbuf here is just a typo.

> ... but I think you can initialize ret to -1 (i.e. assume the
> worst at the beginning), and then make the "ok, we didn't get any
> errors" case do
>
> else {
> grab_values(...);
> ret = 0;
> }

Yes, that also works.

[1]: 
https://public-inbox.org/git/capig+ct5jh0y9rmw0e6ns0k5mswaxaqdan8owcayce8v+jy...@mail.gmail.com/

Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Junio C Hamano]

Eric Sunshine  writes:

> Overall, with the need for resource cleanup, this function becomes
> unusually noisy after this change. It could be tamed by doing
> something like this:
>
> int ret = 0;
> void *buf = get_obj(oid, obj, , );
> if (!buf)
> ret = strbuf_error(_("missing object %s for %s"),
> oid_to_hex(oid), ref->refname);
> else if (!*obj)
> ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
> oid_to_hex(oid), ref->refname);
> else
> grab_values(ref->value, deref, *obj, buf, size);
>if (!eaten)
> free(buf);
> return ret;

I have no idea what strbuf_error() that does not take any strbuf is
doing, but I think you can initialize ret to -1 (i.e. assume the
worst at the beginning), and then make the "ok, we didn't get any
errors" case do

else {
grab_values(...);
ret = 0;
}

Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Eric Sunshine]

On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya
 wrote:
> ref-filter: get_ref_atom_value() error handling

This doesn't tell us much about what this patch is doing. Perhaps a
better subject would be:

ref-filter: libify get_ref_atom_value()

> Finish removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
>
> Change the signature of get_ref_atom_value() and underlying functions
> by adding return value and strbuf parameter for error message.
> Return value equals 0 upon success and -1 upon failure (there
> could be more error codes further if necessary).
> Upon failure, error message is appended to the strbuf.
>
> Signed-off-by: Olga Telezhnaia 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom 
> *atom, struct ref_array_item *re
> +static int get_object(struct ref_array_item *ref, const struct object_id 
> *oid,
> +  int deref, struct object **obj, struct strbuf *err)
>  {
> void *buf = get_obj(oid, obj, , );
> -   if (!buf)
> -   die(_("missing object %s for %s"),
> -   oid_to_hex(oid), ref->refname);
> -   if (!*obj)
> -   die(_("parse_object_buffer failed on %s for %s"),
> -   oid_to_hex(oid), ref->refname);
> -
> +   if (!buf) {
> +   strbuf_addf(err, _("missing object %s for %s"), 
> oid_to_hex(oid),
> +   ref->refname);
> +   if (!eaten)
> +   free(buf);

Since we are inside the 'if (!buf)' conditional, we _know_ that 'buf'
is NULL, therefore this free(buff) is unnecessary.

> +   return -1;
> +   }
> +   if (!*obj) {
> +   strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> +   oid_to_hex(oid), ref->refname);
> +   if (!eaten)
> +   free(buf);
> +   return -1;
> +   }
> grab_values(ref->value, deref, *obj, buf, size);
> if (!eaten)
> free(buf);
> +   return 0;
>  }

Overall, with the need for resource cleanup, this function becomes
unusually noisy after this change. It could be tamed by doing
something like this:

int ret = 0;
void *buf = get_obj(oid, obj, , );
if (!buf)
ret = strbuf_error(_("missing object %s for %s"),
oid_to_hex(oid), ref->refname);
else if (!*obj)
ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
oid_to_hex(oid), ref->refname);
else
grab_values(ref->value, deref, *obj, buf, size);
   if (!eaten)
free(buf);
return ret;

[PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Olga Telezhnaya]

Finish removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure (there
could be more error codes further if necessary).
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 55 ---
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index b90cec1056954..85f971663a4aa 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, 
struct ref_array_item *re
return show_ref(>u.refname, ref->refname);
 }
 
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
-  int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+  int deref, struct object **obj, struct strbuf *err)
 {
int eaten;
unsigned long size;
void *buf = get_obj(oid, obj, , );
-   if (!buf)
-   die(_("missing object %s for %s"),
-   oid_to_hex(oid), ref->refname);
-   if (!*obj)
-   die(_("parse_object_buffer failed on %s for %s"),
-   oid_to_hex(oid), ref->refname);
-
+   if (!buf) {
+   strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
+   ref->refname);
+   if (!eaten)
+   free(buf);
+   return -1;
+   }
+   if (!*obj) {
+   strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
+   oid_to_hex(oid), ref->refname);
+   if (!eaten)
+   free(buf);
+   return -1;
+   }
grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+   return 0;
 }
 
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 {
struct object *obj;
int i;
@@ -1588,16 +1596,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
-   return;
+   return 0;
 
-   get_object(ref, >objectname, 0, );
+   if (get_object(ref, >objectname, 0, , err))
+   return -1;
 
/*
 * If there is no atom that wants to know about tagged
 * object, we are done.
 */
if (!need_tagged || (obj->type != OBJ_TAG))
-   return;
+   return 0;
 
/*
 * If it is a tag object, see if we use a value that derefs
@@ -1611,20 +1620,23 @@ static void populate_value(struct ref_array_item *ref)
 * is not consistent with what deref_tag() does
 * which peels the onion to the core.
 */
-   get_object(ref, tagged, 1, );
+   return get_object(ref, tagged, 1, , err);
 }
 
 /*
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct 
atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
 {
if (!ref->value) {
-   populate_value(ref);
+   if (populate_value(ref, err))
+   return -1;
fill_missing_values(ref->value);
}
*v = >value[atom];
+   return 0;
 }
 
 /*
@@ -2148,9 +2160,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct 
ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+   struct strbuf err = STRBUF_INIT;
 
-   get_ref_atom_value(a, s->atom, );
-   get_ref_atom_value(b, s->atom, );
+   if (get_ref_atom_value(a, s->atom, , ))
+   die("%s", err.buf);
+   if (get_ref_atom_value(b, s->atom, , ))
+   die("%s", err.buf);
+   strbuf_release();
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2230,7 +2246,8 @@ int format_ref_array_item(struct ref_array_item *info,
pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
if (pos < 0)
return -1;
-   get_ref_atom_value(info, pos, );
+   if (get_ref_atom_value(info, pos, , error_buf))
+   return -1;