Re: [PATCH v7 01/28] refs: ref_iterator_peel returns boolean, rather than peel_status

Han-Wen Nienhuys [off-list ref] 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().

quoted

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.

quoted

quoted

+/*
+ * 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.