Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling
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
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
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, &size, &eaten); >> 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
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, &size, &eaten); > 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
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, &size, &eaten); > - 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, &size, &eaten); 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;