git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] refs: ref_iterator_peel returns boolean, rather than peel_status
@ 2021-04-15 11:02 Han-Wen Nienhuys via GitGitGadget
  2021-05-19 15:31 ` [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean Han-Wen Nienhuys via GitGitGadget
  0 siblings, 1 reply; 6+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-15 11:02 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

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: 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.
    
    Signed-off-by: Han-Wen Nienhuys hanwen@google.com

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(-)

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);
 

base-commit: 54a391711554ed41b4b0792cfef004abc74893bd
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
  2021-04-15 11:02 [PATCH] refs: ref_iterator_peel returns boolean, rather than peel_status Han-Wen Nienhuys via GitGitGadget
@ 2021-05-19 15:31 ` Han-Wen Nienhuys via GitGitGadget
  2021-05-19 22:58   ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-05-19 15:31 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
    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 <hanwen@google.com>
      
       ## 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.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     @@ 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(-)

diff --git a/refs.c b/refs.c
index 8c9490235ea6..8b9f7c3a80a0 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) ? -1 : 0;
 }
 
 int refs_create_symref(struct ref_store *refs,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index dfecdbc1db60..66cb90c79ee0 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -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;
 	}
 }
 
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 46f1e5428433..49d732f6db96 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) ? -1 : 0;
 }
 
 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..3155708345fc 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 or -1 for failure.
+ */
 typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,
 				 struct object_id *peeled);
 

base-commit: bf949ade81106fbda068c1fdb2c6fd1cb1babe7e
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
  2021-05-19 15:31 ` [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean Han-Wen Nienhuys via GitGitGadget
@ 2021-05-19 22:58   ` Junio C Hamano
  2021-05-20  9:04     ` Han-Wen Nienhuys
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2021-05-19 22:58 UTC (permalink / raw)
  To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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.


diff --git c/refs.c w/refs.c
index 7292c80a26..3b3478a2f1 100644
--- c/refs.c
+++ w/refs.c
@@ -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,
diff --git c/refs/packed-backend.c w/refs/packed-backend.c
index dfecdbc1db..66cb90c79e 100644
--- c/refs/packed-backend.c
+++ w/refs/packed-backend.c
@@ -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;
 	}
 }
 
diff --git c/refs/ref-cache.c w/refs/ref-cache.c
index 703a12959e..49d732f6db 100644
--- c/refs/ref-cache.c
+++ w/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) ? -1 : 0;
 }
 
 static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)
diff --git c/refs/refs-internal.h w/refs/refs-internal.h
index 7b427b58f6..2bdabf7b4a 100644
--- c/refs/refs-internal.h
+++ w/refs/refs-internal.h
@@ -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);

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
  2021-05-19 22:58   ` Junio C Hamano
@ 2021-05-20  9:04     ` Han-Wen Nienhuys
  2021-07-06 18:09       ` Han-Wen Nienhuys
  0 siblings, 1 reply; 6+ messages in thread
From: Han-Wen Nienhuys @ 2021-05-20  9:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, May 20, 2021 at 12:58 AM Junio C Hamano <gitster@pobox.com> wrote:
> > 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).

-- 
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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
  2021-05-20  9:04     ` Han-Wen Nienhuys
@ 2021-07-06 18:09       ` Han-Wen Nienhuys
  2021-07-06 19:54         ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Han-Wen Nienhuys @ 2021-07-06 18:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, May 20, 2021 at 11:04 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> > 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).

Any update on this? This wasn't queued for next, but I believe it should be.

-- 
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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean
  2021-07-06 18:09       ` Han-Wen Nienhuys
@ 2021-07-06 19:54         ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2021-07-06 19:54 UTC (permalink / raw)
  To: Han-Wen Nienhuys; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

Han-Wen Nienhuys <hanwen@google.com> writes:

> On Thu, May 20, 2021 at 11:04 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
>> > 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).
>
> 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-06 19:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-15 11:02 [PATCH] refs: ref_iterator_peel returns boolean, rather than peel_status Han-Wen Nienhuys via GitGitGadget
2021-05-19 15:31 ` [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean Han-Wen Nienhuys via GitGitGadget
2021-05-19 22:58   ` Junio C Hamano
2021-05-20  9:04     ` Han-Wen Nienhuys
2021-07-06 18:09       ` Han-Wen Nienhuys
2021-07-06 19:54         ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).