{"thread":{"id":"55498","subject":"[PATCH] refs: ref_iterator_peel returns boolean, rather than peel_status","startedAt":"2021-04-15T11:02:53Z","lastAt":"2021-07-06T19:55:00Z","messageCount":6,"participants":["Han-Wen Nienhuys via GitGitGadget","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422023","messageId":"pull.1006.git.git.1618484566164.gitgitgadget@gmail.com","threadId":"55498","inReplyTo":null,"subject":"[PATCH] refs: ref_iterator_peel returns boolean, rather than peel_status","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-15T11:02:45Z","receivedAt":"2021-04-15T11:02:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nBefore, the cached ref_iterator would return peel_object() output directly. This\nled to spurious differences in the GIT_TRACE_REFS output, depending on the ref\nstorage backend active.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n refs: ref_iterator_peel returns boolean, rather than peel_status\n \n Before, the cached ref_iterator would return peel_object() output\n directly. This led to spurious differences in the GIT_TRACE_REFS output,\n depending ref storage backend.\n \n Signed-off-by: Han-Wen Nienhuys hanwen@google.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1006%2Fhanwen%2Fpeel_return-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1006/hanwen/peel_return-v1\nPull-Request: https://github.com/git/git/pull/1006\n\n refs.c | 2 +-\n refs/ref-cache.c | 2 +-\n refs/refs-internal.h | 3 +++\n 3 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 261fd82beb98..8873854a44fb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2010,7 +2010,7 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n \t oideq(current_ref_iter->oid, base)))\n \t\treturn ref_iterator_peel(current_ref_iter, peeled);\n \n-\treturn peel_object(base, peeled);\n+\treturn !!peel_object(base, peeled);\n }\n \n int refs_create_symref(struct ref_store *refs,\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 46f1e5428433..703a12959e1f 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -491,7 +491,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled)\n {\n-\treturn peel_object(ref_iterator->oid, peeled);\n+\treturn !!peel_object(ref_iterator->oid, peeled);\n }\n \n static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 467f4b3c936d..546a6b965dcc 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -453,6 +453,9 @@ void base_ref_iterator_free(struct ref_iterator *iter);\n */\n typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator);\n \n+/*\n+ * Peels the current ref, returning 0 for success.\n+ */\n typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled);\n \n\nbase-commit: 54a391711554ed41b4b0792cfef004abc74893bd\n-- \ngitgitgadget\n"},{"id":"424967","messageId":"pull.1006.v2.git.git.1621438289354.gitgitgadget@gmail.com","threadId":"55498","inReplyTo":"pull.1006.git.git.1618484566164.gitgitgadget@gmail.com","subject":"[PATCH v2] refs: make explicit that ref_iterator_peel returns boolean","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-19T15:31:28Z","receivedAt":"2021-05-19T15:31:39Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nUse -1 as error return value throughout.\n\nThis removes spurious differences in the GIT_TRACE_REFS output, depending on the\nref storage backend active.\n\nBefore, the cached ref_iterator (but only that iterator!) would return\npeel_object() output directly. No callers relied on the peel_status values\nbeyond success/failure. All calls to these functions go through\npeel_iterated_oid(), which returns peel_object() as a fallback, but also\nsquashing the error values.\n\nThe iteration interface already passes REF_ISSYMREF and REF_ISBROKEN through the\nflags argument, so the additional error values in enum peel_status provide no\nvalue.\n\nThe ref iteration interface provides a separate peel() function because certain\nformats (eg. packed-refs and reftable) can store the peeled object next to the\ntag SHA1. Passing the peeled SHA1 as an optional argument to each_ref_fn maps\nmore naturally to the implementation of ref databases. Changing the code in this\nway is left for a future refactoring.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n refs: ref_iterator_peel returns boolean, rather than peel_status\n \n v2: expand commit message.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1006%2Fhanwen%2Fpeel_return-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1006/hanwen/peel_return-v2\nPull-Request: https://github.com/git/git/pull/1006\n\nRange-diff vs v1:\n\n 1: 241e0ad1954b ! 1: f1dc6c2d7fea refs: ref_iterator_peel returns boolean, rather than peel_status\n @@ Metadata\n Author: Han-Wen Nienhuys \n \n ## Commit message ##\n - refs: ref_iterator_peel returns boolean, rather than peel_status\n + refs: make explicit that ref_iterator_peel returns boolean\n \n - Before, the cached ref_iterator would return peel_object() output directly. This\n - led to spurious differences in the GIT_TRACE_REFS output, depending on the ref\n - storage backend active.\n + Use -1 as error return value throughout.\n +\n + This removes spurious differences in the GIT_TRACE_REFS output, depending on the\n + ref storage backend active.\n +\n + Before, the cached ref_iterator (but only that iterator!) would return\n + peel_object() output directly. No callers relied on the peel_status values\n + beyond success/failure. All calls to these functions go through\n + peel_iterated_oid(), which returns peel_object() as a fallback, but also\n + squashing the error values.\n +\n + The iteration interface already passes REF_ISSYMREF and REF_ISBROKEN through the\n + flags argument, so the additional error values in enum peel_status provide no\n + value.\n +\n + The ref iteration interface provides a separate peel() function because certain\n + formats (eg. packed-refs and reftable) can store the peeled object next to the\n + tag SHA1. Passing the peeled SHA1 as an optional argument to each_ref_fn maps\n + more naturally to the implementation of ref databases. Changing the code in this\n + way is left for a future refactoring.\n \n Signed-off-by: Han-Wen Nienhuys \n \n @@ refs.c: int peel_iterated_oid(const struct object_id *base, struct object_id *pe\n \t\treturn ref_iterator_peel(current_ref_iter, peeled);\n \n -\treturn peel_object(base, peeled);\n -+\treturn !!peel_object(base, peeled);\n ++\treturn peel_object(base, peeled) ? -1 : 0;\n }\n \n int refs_create_symref(struct ref_store *refs,\n \n + ## refs/packed-backend.c ##\n +@@ refs/packed-backend.c: static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n + \t} else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {\n + \t\treturn -1;\n + \t} else {\n +-\t\treturn !!peel_object(&iter->oid, peeled);\n ++\t\treturn peel_object(&iter->oid, peeled) ? -1 : 0;\n + \t}\n + }\n + \n +\n ## refs/ref-cache.c ##\n @@ refs/ref-cache.c: static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled)\n {\n -\treturn peel_object(ref_iterator->oid, peeled);\n -+\treturn !!peel_object(ref_iterator->oid, peeled);\n ++\treturn peel_object(ref_iterator->oid, peeled) ? -1 : 0;\n }\n \n static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\n @@ refs/refs-internal.h: void base_ref_iterator_free(struct ref_iterator *iter);\n typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator);\n \n +/*\n -+ * Peels the current ref, returning 0 for success.\n ++ * Peels the current ref, returning 0 for success or -1 for failure.\n + */\n typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled);\n\n\n refs.c | 2 +-\n refs/packed-backend.c | 2 +-\n refs/ref-cache.c | 2 +-\n refs/refs-internal.h | 3 +++\n 4 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8c9490235ea6..8b9f7c3a80a0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2010,7 +2010,7 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n \t oideq(current_ref_iter->oid, base)))\n \t\treturn ref_iterator_peel(current_ref_iter, peeled);\n \n-\treturn peel_object(base, peeled);\n+\treturn peel_object(base, peeled) ? -1 : 0;\n }\n \n int refs_create_symref(struct ref_store *refs,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex dfecdbc1db60..66cb90c79ee0 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -889,7 +889,7 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \t} else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {\n \t\treturn -1;\n \t} else {\n-\t\treturn !!peel_object(&iter->oid, peeled);\n+\t\treturn peel_object(&iter->oid, peeled) ? -1 : 0;\n \t}\n }\n \ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 46f1e5428433..49d732f6db96 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -491,7 +491,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled)\n {\n-\treturn peel_object(ref_iterator->oid, peeled);\n+\treturn peel_object(ref_iterator->oid, peeled) ? -1 : 0;\n }\n \n static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 467f4b3c936d..3155708345fc 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -453,6 +453,9 @@ void base_ref_iterator_free(struct ref_iterator *iter);\n */\n typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator);\n \n+/*\n+ * Peels the current ref, returning 0 for success or -1 for failure.\n+ */\n typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled);\n \n\nbase-commit: bf949ade81106fbda068c1fdb2c6fd1cb1babe7e\n-- \ngitgitgadget\n"},{"id":"424999","messageId":"xmqqzgwqwcu1.fsf@gitster.g","threadId":"55498","inReplyTo":"pull.1006.v2.git.git.1621438289354.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-19T22:58:14Z","receivedAt":"2021-05-19T22:58:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" writes:\n\n> From: Han-Wen Nienhuys \n>\n> Use -1 as error return value throughout.\n\nSo this corresponds to the bottommost step in the series that is\nqueued as hn/reftable, with the difference at the end of this\nmessage.\n\nDo you want me to replace that single step with this version and\nrebuild the remainder of the topic on top?\n\nThanks.\n\n\ndiff --git c/refs.c w/refs.c\nindex 7292c80a26..3b3478a2f1 100644\n--- c/refs.c\n+++ w/refs.c\n@@ -2020,7 +2020,7 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n \t oideq(current_ref_iter->oid, base)))\n \t\treturn ref_iterator_peel(current_ref_iter, peeled);\n \n-\treturn !!peel_object(base, peeled);\n+\treturn peel_object(base, peeled) ? -1 : 0;\n }\n \n int refs_create_symref(struct ref_store *refs,\ndiff --git c/refs/packed-backend.c w/refs/packed-backend.c\nindex dfecdbc1db..66cb90c79e 100644\n--- c/refs/packed-backend.c\n+++ w/refs/packed-backend.c\n@@ -889,7 +889,7 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \t} else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {\n \t\treturn -1;\n \t} else {\n-\t\treturn !!peel_object(&iter->oid, peeled);\n+\t\treturn peel_object(&iter->oid, peeled) ? -1 : 0;\n \t}\n }\n \ndiff --git c/refs/ref-cache.c w/refs/ref-cache.c\nindex 703a12959e..49d732f6db 100644\n--- c/refs/ref-cache.c\n+++ w/refs/ref-cache.c\n@@ -491,7 +491,7 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled)\n {\n-\treturn !!peel_object(ref_iterator->oid, peeled);\n+\treturn peel_object(ref_iterator->oid, peeled) ? -1 : 0;\n }\n \n static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\ndiff --git c/refs/refs-internal.h w/refs/refs-internal.h\nindex 7b427b58f6..2bdabf7b4a 100644\n--- c/refs/refs-internal.h\n+++ w/refs/refs-internal.h\n@@ -454,7 +454,7 @@ void base_ref_iterator_free(struct ref_iterator *iter);\n typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator);\n \n /*\n- * Peels the current ref, returning 0 for success.\n+ * Peels the current ref, returning 0 for success or -1 for failure.\n */\n typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled);\n"},{"id":"425075","messageId":"CAFQ2z_OUxrW_m5H_y62=Q_i0Kj3c4=2Kzf4hRwVwp-wGPcGayw@mail.gmail.com","threadId":"55498","inReplyTo":"xmqqzgwqwcu1.fsf@gitster.g","subject":"Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-05-20T09:04:20Z","receivedAt":"2021-05-20T09:04:34Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, May 20, 2021 at 12:58 AM Junio C Hamano wrote:\n> > Use -1 as error return value throughout.\n>\n> So this corresponds to the bottommost step in the series that is\n> queued as hn/reftable, with the difference at the end of this\n> message.\n>\n> Do you want me to replace that single step with this version and\n> rebuild the remainder of the topic on top?\n\nYes.\n\nI'm hoping this can graduate soon, so the hn/reftable topic becomes\nsmaller (I don't know what that means for your organization of the\nseen branch).\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"429304","messageId":"CAFQ2z_M-NmN2xVjoudzEqpAvugyRBQkKh+N4r_s_W=U99Uvwpg@mail.gmail.com","threadId":"55498","inReplyTo":"CAFQ2z_OUxrW_m5H_y62=Q_i0Kj3c4=2Kzf4hRwVwp-wGPcGayw@mail.gmail.com","subject":"Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-07-06T18:09:40Z","receivedAt":"2021-07-06T18:09:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, May 20, 2021 at 11:04 AM Han-Wen Nienhuys wrote:\n> > So this corresponds to the bottommost step in the series that is\n> > queued as hn/reftable, with the difference at the end of this\n> > message.\n> >\n> > Do you want me to replace that single step with this version and\n> > rebuild the remainder of the topic on top?\n>\n> Yes.\n>\n> I'm hoping this can graduate soon, so the hn/reftable topic becomes\n> smaller (I don't know what that means for your organization of the\n> seen branch).\n\nAny update on this? This wasn't queued for next, but I believe it should be.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"429333","messageId":"xmqqo8bfi5f5.fsf@gitster.g","threadId":"55498","inReplyTo":"CAFQ2z_M-NmN2xVjoudzEqpAvugyRBQkKh+N4r_s_W=U99Uvwpg@mail.gmail.com","subject":"Re: [PATCH v2] refs: make explicit that ref_iterator_peel returns boolean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-06T19:54:54Z","receivedAt":"2021-07-06T19:55:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n> On Thu, May 20, 2021 at 11:04 AM Han-Wen Nienhuys wrote:\n>> > So this corresponds to the bottommost step in the series that is\n>> > queued as hn/reftable, with the difference at the end of this\n>> > message.\n>> >\n>> > Do you want me to replace that single step with this version and\n>> > rebuild the remainder of the topic on top?\n>>\n>> Yes.\n>>\n>> I'm hoping this can graduate soon, so the hn/reftable topic becomes\n>> smaller (I don't know what that means for your organization of the\n>> seen branch).\n>\n> Any update on this? This wasn't queued for next, but I believe it should be.\n\nSorry, I thought you meant you wanted to fix up the bottommost\ncommit in that series, which I think was already done at 617480d7\n(refs: make explicit that ref_iterator_peel returns boolean,\n2021-05-19), and did not realize that you wanted a separate single\npatch topic for this step, on top of which the remainder of the\nother topic builds.\n\nWill do. Thanks.\n"}]}