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

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Mar 21, 2018 at 3:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> 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.