refs: ref_iterator_peel returns boolean, rather than peel_status
PATCH 1/1 refs: ref_iterator_peel returns boolean, rather than peel_status
From: Han-Wen Nienhuys <[email protected]>
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.
---
refs: ref_iterator_peel returns boolean, rather than peel_status
Before, the cached ref_iterator would return peel_object() output directly. This led to spurious differences in the GIT_TRACE_REFS output, depending ref storage backend.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1006%2Fhanwen%2Fpeel_return-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1006/hanwen/peel_return-v1
Pull-Request: https://github.com/git/git/pull/1006
refs.c | 2 +- refs/ref-cache.c | 2 +- refs/refs-internal.h | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-)
refs.c +1 -1
@@ -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,
refs/ref-cache.c +1 -1
@@ -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)
refs/refs-internal.h +3 -0
@@ -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);
base-commit: 54a391711554ed41b4b0792cfef004abc74893bd
Signature · 1 line
gitgitgadget
PATCH v2 1/1 refs: make explicit that ref_iterator_peel returns boolean
From: Han-Wen Nienhuys <[email protected]>
Use -1 as error return value throughout.
This removes spurious differences in the GIT_TRACE_REFS output, depending on the ref storage backend active.
Before, the cached ref_iterator (but only that iterator!) would return peel_object() output directly. No callers relied on the peel_status values beyond success/failure. All calls to these functions go through peel_iterated_oid(), which returns peel_object() as a fallback, but also squashing the error values.
The iteration interface already passes REF_ISSYMREF and REF_ISBROKEN through the flags argument, so the additional error values in enum peel_status provide no value.
The ref iteration interface provides a separate peel() function because certain formats (eg. packed-refs and reftable) can store the peeled object next to the tag SHA1. Passing the peeled SHA1 as an optional argument to each_ref_fn maps more naturally to the implementation of ref databases. Changing the code in this way is left for a future refactoring.
---
refs: ref_iterator_peel returns boolean, rather than peel_status
v2: expand commit message.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1006%2Fhanwen%2Fpeel_return-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1006/hanwen/peel_return-v2
Pull-Request: https://github.com/git/git/pull/1006
Range-diff vs v1:
1: 241e0ad1954b ! 1: f1dc6c2d7fea refs: ref_iterator_peel returns boolean, rather than peel_status
@@ Metadata
Author: Han-Wen Nienhuys <[email protected]>
## Commit message ##
- refs: ref_iterator_peel returns boolean, rather than peel_status
+ refs: make explicit that ref_iterator_peel returns boolean
- 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.
+ Use -1 as error return value throughout.
+
+ This removes spurious differences in the GIT_TRACE_REFS output, depending on the
+ ref storage backend active.
+
+ Before, the cached ref_iterator (but only that iterator!) would return
+ peel_object() output directly. No callers relied on the peel_status values
+ beyond success/failure. All calls to these functions go through
+ peel_iterated_oid(), which returns peel_object() as a fallback, but also
+ squashing the error values.
+
+ The iteration interface already passes REF_ISSYMREF and REF_ISBROKEN through the
+ flags argument, so the additional error values in enum peel_status provide no
+ value.
+
+ The ref iteration interface provides a separate peel() function because certain
+ formats (eg. packed-refs and reftable) can store the peeled object next to the
+ tag SHA1. Passing the peeled SHA1 as an optional argument to each_ref_fn maps
+ more naturally to the implementation of ref databases. Changing the code in this
+ way is left for a future refactoring.
@@ refs.c: int peel_iterated_oid(const struct object_id *base, struct object_id *pe
return ref_iterator_peel(current_ref_iter, peeled);
- return peel_object(base, peeled);
-+ return !!peel_object(base, peeled);
++ return peel_object(base, peeled) ? -1 : 0;
}
int refs_create_symref(struct ref_store *refs,
+ ## refs/packed-backend.c ##
+@@ refs/packed-backend.c: static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,
+ } else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {
+ return -1;
+ } else {
+- return !!peel_object(&iter->oid, peeled);
++ return peel_object(&iter->oid, peeled) ? -1 : 0;
+ }
+ }
+
+
## refs/ref-cache.c ##
@@ refs/ref-cache.c: 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);
++ return peel_object(ref_iterator->oid, peeled) ? -1 : 0;
}
static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)
@@ refs/refs-internal.h: 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.
++ * Peels the current ref, returning 0 for success or -1 for failure.
+ */
typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,
struct object_id *peeled);
refs.c | 2 +- refs/packed-backend.c | 2 +- refs/ref-cache.c | 2 +- refs/refs-internal.h | 3 +++ 4 files changed, 6 insertions(+), 3 deletions(-)
refs.c +1 -1
@@ -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) ?-1 :0 ; } int refs_create_symref(struct ref_store *refs,
refs/packed-backend.c +1 -1
@@ -889,7 +889,7 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator, }else if ((iter->base.flags & (REF_ISBROKEN |REF_ISSYMREF ))) {return -1 ; }else { -return !!peel_object (&iter->oid, peeled); +return peel_object (&iter->oid, peeled) ?-1 :0 ; } }
refs/ref-cache.c +1 -1
@@ -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) ?-1 :0 ; }static int cache_ref_iterator_abort (struct ref_iterator *ref_iterator)
refs/refs-internal.h +3 -0
@@ -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 or -1 for failure. +*/ typedef int ref_iterator_peel_fn (struct ref_iterator *ref_iterator,struct object_id *peeled);
base-commit: bf949ade81106fbda068c1fdb2c6fd1cb1babe7e
Signature · 1 line
gitgitgadget
Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
Quoted 3 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
Use -1 as error return value throughout.
So this corresponds to the bottommost step in the series that is queued as hn/reftable, with the difference at the end of this message.
Do you want me to replace that single step with this version and rebuild the remainder of the topic on top?
Thanks.
refs.c +1 -1
@@ -2020,7 +2020,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) ?-1 :0 ; } int refs_create_symref(struct ref_store *refs,
refs/packed-backend.c +1 -1
@@ -889,7 +889,7 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator, }else if ((iter->base.flags & (REF_ISBROKEN |REF_ISSYMREF ))) {return -1 ; }else { -return !!peel_object (&iter->oid, peeled); +return peel_object (&iter->oid, peeled) ?-1 :0 ; } }
refs/ref-cache.c +1 -1
@@ -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) ?-1 :0 ; }static int cache_ref_iterator_abort (struct ref_iterator *ref_iterator)
refs/refs-internal.h +1 -1
@@ -454,7 +454,7 @@ 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. +* Peels the current ref, returning 0 for success or -1 for failure. */ typedef int ref_iterator_peel_fn (struct ref_iterator *ref_iterator,struct object_id *peeled);
Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
Quoted 8 lines · Junio C Hamano
Quoted 1 line
Use -1 as error return value throughout.
So this corresponds to the bottommost step in the series that is queued as hn/reftable, with the difference at the end of this message.
Do you want me to replace that single step with this version and rebuild the remainder of the topic on top?
Yes.
I'm hoping this can graduate soon, so the hn/reftable topic becomes smaller (I don't know what that means for your organization of the seen branch).
Signature · 11 lines
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
Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
Quoted 4 lines · Han-Wen Nienhuys
Quoted 4 lines
So this corresponds to the bottommost step in the series that is queued as hn/reftable, with the difference at the end of this message.
Quoted 8 lines
Quoted 2 lines
Do you want me to replace that single step with this version and rebuild the remainder of the topic on top?
Yes.
I'm hoping this can graduate soon, so the hn/reftable topic becomes smaller (I don't know what that means for your organization of the seen branch).
Any update on this? This wasn't queued for next, but I believe it should be.
Signature · 11 lines
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
Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
Quoted 8 lines · Han-Wen Nienhuys
Quoted 7 lines · Han-Wen Nienhuys
Quoted 6 lines
So this corresponds to the bottommost step in the series that is queued as hn/reftable, with the difference at the end of this message.
Do you want me to replace that single step with this version and rebuild the remainder of the topic on top?
Quoted 7 lines
Quoted 5 lines
Yes.
I'm hoping this can graduate soon, so the hn/reftable topic becomes smaller (I don't know what that means for your organization of the seen branch).
Any update on this? This wasn't queued for next, but I believe it should be.
Sorry, I thought you meant you wanted to fix up the bottommost commit in that series, which I think was already done at 617480d7 (refs: make explicit that ref_iterator_peel returns boolean, 2021-05-19), and did not realize that you wanted a separate single patch topic for this step, on top of which the remainder of the other topic builds.
Will do. Thanks.