2018-03-16 1:48 GMT+03:00 Junio C Hamano <gits...@pobox.com>:
> Olga Telezhnaya <olyatelezhn...@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.
