| Message ID | 8103a80394aefdd4e8b5061dfbb6a6199fe5fcae.1618832276.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reftable library | expand |
"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.
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.
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 --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);