On Tue, Apr 20, 2021 at 8:47 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "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).
No, it should not. The whole point of the trace output is to see
where and how the files backend and the reftable backend return
different results. By having the trace layer paper over differences, I
will miss regressions of the reftable backend.
> * 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.
All these functions go into ref_iterator_peel, which is only called
from peel_iterated_oid().
That function calls !!peel_object(base, peeled) if there is no
iterator. That leads me to conclude
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),
>
> > +/*
> > + * 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.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
