Han-Wen Nienhuys <hanwen@xxxxxxxxxx> writes:
> All callers of peel_iterated_oid use it as a boolean exclusively, i.e.
>
> if (!peel_iterated_oid( .. )) { .. }
>
> Aside from these considerations, it is also odd to return peel_status.
> For example, PEEL_IS_SYMREF is unnecessary, because the symref status
> is already returned from iterator_next().
>
>> I haven't seen a good justification given to help convince me that
>> this is a good change (and I presume it is, as I trust you or any
>> other contributores enough not to knowingly make the system worse),
I am not sure I agree 100% with
if (!do_this()) {
...
}
i.e. "try doing it and do one thing upon success" is a sign that all
error conditions will ever be treated equally and justifies to squash
all the error codes the current code tries to return, but whether I
agree with it or not, I'd want to see it recorded in the proposed
log message. It would help future developers and allow them to take
into account your motivation behind this change, when they need to
update the implementation.
>> > +/*
>> > + * Peels the current ref, returning 0 for success.
>> > + */
>>
>> And if it does make sense to squash the peel status down to "bool",
>> then the comment should mention the single acceptable value for
>> failure, not just "0 for success" which implies "different negative
>> values depending on the nature of failure".
>
> I can make it return -1 instead.
I do care more about _documenting_ it here, not just ending the
sentence with ", returning 0 for success", it should also say
"returns X upon failure", whether the value of X is 1 or -1 (and
obviously our codebase prefers -1 for an error, but that is
secondary).
Thanks.
