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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 21, 2018, 19:58 UTC
Message-ID: <CAPig+cQZOjFuAHXqj6c8oP8JJaboLf3kL9heMA=jkXssr+gQ_Q@mail.gmail.com>
In-Reply-To: <xmqq4ll9p6oq.fsf@gitster-ct.c.googlers.com>

On Wed, Mar 21, 2018 at 3:30 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> 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.