"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> Before, the cached ref_iterator would return peel_object() output directly. This
> led to spurious differences in the GIT_TRACE_REFS output, depending on the ref
> storage backend active.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> ---
> refs.c | 2 +-
> refs/ref-cache.c | 2 +-
> refs/refs-internal.h | 3 +++
> 3 files changed, 5 insertions(+), 2 deletions(-)
A few observations.
* The ref_iterator_peel() is defined to "Return 0 on success." in
refs/refs-internal.h and implication of the missing mention of
what is returned on failure is that it can give any non-zero
value back, and in this project a failure usually is signaled by
returning a negative value.
* If the trace output cares that all non-success trace entries to
look identical, I wonder if that layer should be the one that
normalizes "0 is success, any other value is failure" into "0 is
success, 1 is failure" (even though I find a positive 1 used as a
failure a bit odd).
* refs.c::peel_object() is defined to return "enum peel_status"
which is not even a simple yes/no.
PEEL_PEELED = 0
PEEL_INVALID = -1
PEEL_NON_TAG = -2
PEEL_IS_SYMREF = -3
PEEL_BROKEN = -4
The comment in refs/refs-internal.h about "Reference interators"
suggests me that ref_iterator_peel() is supposed to be a moral
equivalent of (recently gone) peel_ref() which was a thin wrapper
to still surviving peel_object(), so I expect ref_iterator_peel()
to allow the caller to differentiate various failure modes. And
as a way to debug into a running system, I am not sure if losing
the distinction in the trace output is even desirable.
packed_ref_iterator_peel() does use !!peel_object(), but I have a
feeling that that is what should be fixed and not the other way
around.
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),
> +/*
> + * 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".
> typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,
> struct object_id *peeled);
Thanks.
