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

["Оля Тележная" <olyatelezhnaya@gmail.com>]

2018-03-16 1:48 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> 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 &result 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, &err);
>         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.