diff mbox series

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

Message ID 8103a80394aefdd4e8b5061dfbb6a6199fe5fcae.1618832276.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series reftable library | expand

Commit Message

Han-Wen Nienhuys April 19, 2021, 11:37 a.m. UTC
From: Han-Wen Nienhuys <hanwen@google.com>

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@google.com>
---
 refs.c               | 2 +-
 refs/ref-cache.c     | 2 +-
 refs/refs-internal.h | 3 +++
 3 files changed, 5 insertions(+), 2 deletions(-)

Comments

Junio C Hamano April 20, 2021, 6:47 p.m. UTC | #1
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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@google.com>
> ---
>  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.
Han-Wen Nienhuys April 21, 2021, 10:15 a.m. UTC | #2
On Tue, Apr 20, 2021 at 8:47 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > 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@google.com>
> > ---
> >  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.
Junio C Hamano April 21, 2021, 11:28 p.m. UTC | #3
Han-Wen Nienhuys <hanwen@google.com> 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.
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index 261fd82beb98..8873854a44fb 100644
--- a/refs.c
+++ b/refs.c
@@ -2010,7 +2010,7 @@  int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)
 	     oideq(current_ref_iter->oid, base)))
 		return ref_iterator_peel(current_ref_iter, peeled);
 
-	return peel_object(base, peeled);
+	return !!peel_object(base, peeled);
 }
 
 int refs_create_symref(struct ref_store *refs,
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 46f1e5428433..703a12959e1f 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -491,7 +491,7 @@  static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
 static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,
 				   struct object_id *peeled)
 {
-	return peel_object(ref_iterator->oid, peeled);
+	return !!peel_object(ref_iterator->oid, peeled);
 }
 
 static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 467f4b3c936d..546a6b965dcc 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -453,6 +453,9 @@  void base_ref_iterator_free(struct ref_iterator *iter);
  */
 typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator);
 
+/*
+ * Peels the current ref, returning 0 for success.
+ */
 typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,
 				 struct object_id *peeled);