{"thread":{"id":"55019","subject":"[PATCH] refs: switch peel_ref() to peel_iterated_oid()","startedAt":"2021-01-20T19:52:52Z","lastAt":"2021-01-22T21:41:03Z","messageCount":3,"participants":["Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"414849","messageId":"YAiIK+/hSRxaUx/K@coredump.intra.peff.net","threadId":"55019","inReplyTo":null,"subject":"[PATCH] refs: switch peel_ref() to peel_iterated_oid()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-20T19:44:43Z","receivedAt":"2021-01-20T19:52:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This has been bugging me for years, and came up again in:\n\n  https://lore.kernel.org/git/YAhXw9Gvn5Pyvacq@coredump.intra.peff.net/\n\nso I decided to finally do something about it. There's a minor textual\nconflict with the caller being touched in that thread, but it's easy to\nresolve. So I think it's OK to consider the two independent.\n\nThe commit message turned out rather long. I hope that doesn't scare\nanybody away, but I wanted to cover all of the bases.\n\n-- >8 --\nSubject: refs: switch peel_ref() to peel_iterated_oid()\n\nThe peel_ref() interface is confusing and error-prone:\n\n  - it's typically used by ref iteration callbacks that have both a\n    refname and oid. But since they pass only the refname, we may load\n    the ref value from the filesystem again. This is inefficient, but\n    also means we are open to a race if somebody simultaneously updates\n    the ref. E.g., this:\n\n      int some_ref_cb(const char *refname, const struct object_id *oid, ...)\n      {\n              if (!peel_ref(refname, &peeled))\n                      printf(\"%s peels to %s\",\n                             oid_to_hex(oid), oid_to_hex(&peeled);\n      }\n\n    could print nonsense. It is correct to say \"refname peels to...\"\n    (you may see the \"before\" value or the \"after\" value, either of\n    which is consistent), but mentioning both oids may be mixing\n    before/after values.\n\n    Worse, whether this is possible depends on whether the optimization\n    to read from the current iterator value kicks in. So it is actually\n    not possible with:\n\n      for_each_ref(some_ref_cb);\n\n    but it _is_ possible with:\n\n      head_ref(some_ref_cb);\n\n    which does not use the iterator mechanism (though in practice, HEAD\n    should never peel to anything, so this may not be triggerable).\n\n  - it must take a fully-qualified refname for the read_ref_full() code\n    path to work. Yet we routinely pass it partial refnames from\n    callbacks to for_each_tag_ref(), etc. This happens to work when\n    iterating because there we do not call read_ref_full() at all, and\n    only use the passed refname to check if it is the same as the\n    iterator. But the requirements for the function parameters are quite\n    unclear.\n\nInstead of taking a refname, let's instead take an oid. That fixes both\nproblems. It's a little funny for a \"ref\" function not to involve refs\nat all. The key thing is that it's optimizing under the hood based on\nhaving access to the ref iterator. So let's change the name to make it\nclear why you'd want this function versus just peel_object().\n\nThere are two other directions I considered but rejected:\n\n  - we could pass the peel information into the each_ref_fn callback.\n    However, we don't know if the caller actually wants it or not. For\n    packed-refs, providing it is essentially free. But for loose refs,\n    we actually have to peel the object, which would be wasteful in most\n    cases. We could likewise pass in a flag to the callback indicating\n    whether the peeled information is known, but that complicates those\n    callbacks, as they then have to decide whether to manually peel\n    themselves. Plus it requires changing the interface of every\n    callback, whether they care about peeling or not, and there are many\n    of them.\n\n  - we could make a function to return the peeled value of the current\n    iterated ref (computing it if necessary), and BUG() otherwise. I.e.:\n\n      int peel_current_iterated_ref(struct object_id *out);\n\n    Each of the current callers is an each_ref_fn callback, so they'd\n    mostly be happy. But:\n\n      - we use those callbacks with functions like head_ref(), which do\n        not use the iteration code. So we'd need to handle the fallback\n        case there, anyway.\n\n      - it's possible that a caller would want to call into generic code\n        that sometimes is used during iteration and sometimes not. This\n        encapsulates the logic to do the fast thing when possible, and\n        fallback when necessary.\n\nThe implementation is mostly obvious, but I want to call out a few\nthings in the patch:\n\n  - the test-tool coverage for peel_ref() is now meaningless, as it all\n    collapses to a single peel_object() call (arguably they were pretty\n    uninteresting before; the tricky part of that function is the\n    fast-path we see during iteration, but these calls didn't trigger\n    that). I've just dropped it entirely, though note that some other\n    tests relied on the tags we created; I've moved that creation to the\n    tests where it matters.\n\n  - we no longer need to take a ref_store parameter, since we'd never\n    look up a ref now. We do still rely on a global \"current iterator\"\n    variable which _could_ be kept per-ref-store. But in practice this\n    is only useful if there are multiple recursive iterations, at which\n    point the more appropriate solution is probably a stack of\n    iterators. No caller used the actual ref-store parameter anyway\n    (they all call the wrapper that passes the_repository).\n\n  - the original only kicked in the optimization when the \"refname\"\n    pointer matched (i.e., not string comparison). We do likewise with\n    the \"oid\" parameter here, but fall back to doing an actual oideq()\n    call. This in theory lets us kick in the optimization more often,\n    though in practice no current caller cares. It should never be\n    wrong, though (peeling is a property of an object, so two refs\n    pointing to the same object would peel identically).\n\n  - the original took care not to touch the peeled out-parameter unless\n    we found something to put in it. But no caller cares about this, and\n    anyway, it is enforced by peel_object() itself (and even in the\n    optimized iterator case, that's where we eventually end up). We can\n    shorten the code and avoid an extra copy by just passing the\n    out-parameter through the stack.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/describe.c             |  2 +-\n builtin/gc.c                   |  2 +-\n builtin/pack-objects.c         |  4 ++--\n builtin/show-ref.c             |  2 +-\n commit-graph.c                 |  2 +-\n ls-refs.c                      |  2 +-\n refs.c                         | 29 ++++++-----------------------\n refs.h                         | 20 ++++++++++----------\n t/helper/test-ref-store.c      | 13 -------------\n t/t1405-main-ref-store.sh      |  8 +-------\n t/t1406-submodule-ref-store.sh | 10 ++--------\n upload-pack.c                  |  2 +-\n 12 files changed, 27 insertions(+), 69 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 7668591d57..40482d8e9f 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -194,7 +194,7 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi\n \t}\n \n \t/* Is it annotated? */\n-\tif (!peel_ref(path, &peeled)) {\n+\tif (!peel_iterated_oid(oid, &peeled)) {\n \t\tis_annotated = !oideq(oid, &peeled);\n \t} else {\n \t\toidcpy(&peeled, oid);\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex b315b2ad58..01e8dc9f70 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -769,7 +769,7 @@ static int dfs_on_ref(const char *refname,\n \tstruct commit_list *stack = NULL;\n \tstruct commit *commit;\n \n-\tif (!peel_ref(refname, &peeled))\n+\tif (!peel_iterated_oid(oid, &peeled))\n \t\toid = &peeled;\n \tif (oid_object_info(the_repository, oid, NULL) != OBJ_COMMIT)\n \t\treturn 0;\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2a00358f34..bd18139d7e 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -629,7 +629,7 @@ static int mark_tagged(const char *path, const struct object_id *oid, int flag,\n \n \tif (entry)\n \t\tentry->tagged = 1;\n-\tif (!peel_ref(path, &peeled)) {\n+\tif (!peel_iterated_oid(oid, &peeled)) {\n \t\tentry = packlist_find(&to_pack, &peeled);\n \t\tif (entry)\n \t\t\tentry->tagged = 1;\n@@ -2808,7 +2808,7 @@ static int add_ref_tag(const char *path, const struct object_id *oid, int flag,\n \tstruct object_id peeled;\n \n \tif (starts_with(path, \"refs/tags/\") && /* is a tag? */\n-\t    !peel_ref(path, &peeled)    && /* peelable? */\n+\t    !peel_iterated_oid(oid, &peeled)    && /* peelable? */\n \t    obj_is_packed(&peeled)) /* object packed? */\n \t\tadd_tag_chain(oid);\n \treturn 0;\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex ae60b4acf2..7f8a5332f8 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -40,7 +40,7 @@ static void show_one(const char *refname, const struct object_id *oid)\n \tif (!deref_tags)\n \t\treturn;\n \n-\tif (!peel_ref(refname, &peeled)) {\n+\tif (!peel_iterated_oid(oid, &peeled)) {\n \t\thex = find_unique_abbrev(&peeled, abbrev);\n \t\tprintf(\"%s %s^{}\\n\", hex, refname);\n \t}\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e9124d4a41..15c10fcc5a 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1458,7 +1458,7 @@ static int add_ref_to_set(const char *refname,\n \tstruct object_id peeled;\n \tstruct refs_cb_data *data = (struct refs_cb_data *)cb_data;\n \n-\tif (!peel_ref(refname, &peeled))\n+\tif (!peel_iterated_oid(oid, &peeled))\n \t\toid = &peeled;\n \tif (oid_object_info(the_repository, oid, NULL) == OBJ_COMMIT)\n \t\toidset_insert(data->commits, oid);\ndiff --git a/ls-refs.c b/ls-refs.c\nindex a1e0b473e4..7b9ae6498b 100644\n--- a/ls-refs.c\n+++ b/ls-refs.c\n@@ -63,7 +63,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \n \tif (data->peel) {\n \t\tstruct object_id peeled;\n-\t\tif (!peel_ref(refname, &peeled))\n+\t\tif (!peel_iterated_oid(oid, &peeled))\n \t\t\tstrbuf_addf(&refline, \" peeled:%s\", oid_to_hex(&peeled));\n \t}\n \ndiff --git a/refs.c b/refs.c\nindex 13dc2c3291..8a4780c201 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1898,31 +1898,14 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n \treturn refs->be->pack_refs(refs, flags);\n }\n \n-int refs_peel_ref(struct ref_store *refs, const char *refname,\n-\t\t  struct object_id *oid)\n+int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\n {\n-\tint flag;\n-\tstruct object_id base;\n-\n-\tif (current_ref_iter && current_ref_iter->refname == refname) {\n-\t\tstruct object_id peeled;\n-\n-\t\tif (ref_iterator_peel(current_ref_iter, &peeled))\n-\t\t\treturn -1;\n-\t\toidcpy(oid, &peeled);\n-\t\treturn 0;\n-\t}\n+\tif (current_ref_iter &&\n+\t    (current_ref_iter->oid == base ||\n+\t     oideq(current_ref_iter->oid, base)))\n+\t\treturn ref_iterator_peel(current_ref_iter, peeled);\n \n-\tif (refs_read_ref_full(refs, refname,\n-\t\t\t       RESOLVE_REF_READING, &base, &flag))\n-\t\treturn -1;\n-\n-\treturn peel_object(&base, oid);\n-}\n-\n-int peel_ref(const char *refname, struct object_id *oid)\n-{\n-\treturn refs_peel_ref(get_main_ref_store(the_repository), refname, oid);\n+\treturn peel_object(base, peeled);\n }\n \n int refs_create_symref(struct ref_store *refs,\ndiff --git a/refs.h b/refs.h\nindex ff05d2e9fe..71d56cb4b2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -118,16 +118,16 @@ int is_branch(const char *refname);\n int refs_init_db(struct strbuf *err);\n \n /*\n- * If refname is a non-symbolic reference that refers to a tag object,\n- * and the tag can be (recursively) dereferenced to a non-tag object,\n- * store the object ID of the referred-to object to oid and return 0.\n- * If any of these conditions are not met, return a non-zero value.\n- * Symbolic references are considered unpeelable, even if they\n- * ultimately resolve to a peelable tag.\n- */\n-int refs_peel_ref(struct ref_store *refs, const char *refname,\n-\t\t  struct object_id *oid);\n-int peel_ref(const char *refname, struct object_id *oid);\n+ * Return the peeled value of the oid currently being iterated via\n+ * for_each_ref(), etc. This is equivalent to calling:\n+ *\n+ *   peel_object(oid, &peeled);\n+ *\n+ * with the \"oid\" value given to the each_ref_fn callback, except\n+ * that some ref storage may be able to answer the query without\n+ * actually loading the object in memory.\n+ */\n+int peel_iterated_oid(const struct object_id *base, struct object_id *peeled);\n \n /**\n  * Resolve refname in the nested \"gitlink\" repository in the specified\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 759e69dc54..bba5f841c6 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -72,18 +72,6 @@ static int cmd_pack_refs(struct ref_store *refs, const char **argv)\n \treturn refs_pack_refs(refs, flags);\n }\n \n-static int cmd_peel_ref(struct ref_store *refs, const char **argv)\n-{\n-\tconst char *refname = notnull(*argv++, \"refname\");\n-\tstruct object_id oid;\n-\tint ret;\n-\n-\tret = refs_peel_ref(refs, refname, &oid);\n-\tif (!ret)\n-\t\tputs(oid_to_hex(&oid));\n-\treturn ret;\n-}\n-\n static int cmd_create_symref(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n@@ -255,7 +243,6 @@ struct command {\n \n static struct command commands[] = {\n \t{ \"pack-refs\", cmd_pack_refs },\n-\t{ \"peel-ref\", cmd_peel_ref },\n \t{ \"create-symref\", cmd_create_symref },\n \t{ \"delete-refs\", cmd_delete_refs },\n \t{ \"rename-ref\", cmd_rename_ref },\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 74af927fba..5f69ef8519 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -14,13 +14,6 @@ test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '\n \tN=`find .git/refs -type f | wc -l`\n '\n \n-test_expect_success 'peel_ref(new-tag)' '\n-\tgit rev-parse HEAD >expected &&\n-\tgit tag -a -m new-tag new-tag HEAD &&\n-\t$RUN peel-ref refs/tags/new-tag >actual &&\n-\ttest_cmp expected actual\n-'\n-\n test_expect_success 'create_symref(FOO, refs/heads/master)' '\n \t$RUN create-symref FOO refs/heads/master nothing &&\n \techo refs/heads/master >expected &&\n@@ -29,6 +22,7 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' '\n '\n \n test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '\n+\tgit tag -a -m new-tag new-tag HEAD &&\n \tgit rev-parse FOO -- &&\n \tgit rev-parse refs/tags/new-tag -- &&\n \tm=$(git rev-parse master) &&\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 36b7ef5046..110ac40a98 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -11,21 +11,15 @@ test_expect_success 'setup' '\n \t(\n \t\tcd sub &&\n \t\ttest_commit first &&\n-\t\tgit checkout -b new-master\n+\t\tgit checkout -b new-master &&\n+\t\tgit tag -a -m new-tag new-tag HEAD\n \t)\n '\n \n test_expect_success 'pack_refs() not allowed' '\n \ttest_must_fail $RUN pack-refs 3\n '\n \n-test_expect_success 'peel_ref(new-tag)' '\n-\tgit -C sub rev-parse HEAD >expected &&\n-\tgit -C sub tag -a -m new-tag new-tag HEAD &&\n-\t$RUN peel-ref refs/tags/new-tag >actual &&\n-\ttest_cmp expected actual\n-'\n-\n test_expect_success 'create_symref() not allowed' '\n \ttest_must_fail $RUN create-symref FOO refs/heads/master nothing\n '\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 3b66bf92ba..4ab55ce2b5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1232,7 +1232,7 @@ static int send_ref(const char *refname, const struct object_id *oid,\n \t\tpacket_write_fmt(1, \"%s %s\\n\", oid_to_hex(oid), refname_nons);\n \t}\n \tcapabilities = NULL;\n-\tif (!peel_ref(refname, &peeled))\n+\tif (!peel_iterated_oid(oid, &peeled))\n \t\tpacket_write_fmt(1, \"%s %s^{}\\n\", oid_to_hex(&peeled), refname_nons);\n \treturn 0;\n }\n-- \n2.30.0.661.gdf8f046147\n"},{"id":"414855","messageId":"YAiOPzOT52v+dOgL@nand.local","threadId":"55019","inReplyTo":"YAiIK+/hSRxaUx/K@coredump.intra.peff.net","subject":"Re: [PATCH] refs: switch peel_ref() to peel_iterated_oid()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-01-20T20:10:39Z","receivedAt":"2021-01-20T20:11:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jan 20, 2021 at 02:44:43PM -0500, Jeff King wrote:\n> This has been bugging me for years, and came up again in:\n>\n>   https://lore.kernel.org/git/YAhXw9Gvn5Pyvacq@coredump.intra.peff.net/\n>\n> so I decided to finally do something about it. There's a minor textual\n> conflict with the caller being touched in that thread, but it's easy to\n> resolve. So I think it's OK to consider the two independent.\n>\n> The commit message turned out rather long. I hope that doesn't scare\n> anybody away, but I wanted to cover all of the bases.\n\nThanks for all of the detail. Suffice it to say, I was not scared away,\nand am in fact quite comfortable with this direction.\n\nSorry for sending such a short response to such a long email, but I\nagree with the discussion in the patch message, and I don't have\nanything to add other than my approval and a:\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"415004","messageId":"xmqqk0s47iuo.fsf@gitster.c.googlers.com","threadId":"55019","inReplyTo":"YAiOPzOT52v+dOgL@nand.local","subject":"Re: [PATCH] refs: switch peel_ref() to peel_iterated_oid()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-22T21:38:23Z","receivedAt":"2021-01-22T21:41:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Wed, Jan 20, 2021 at 02:44:43PM -0500, Jeff King wrote:\n>> This has been bugging me for years, and came up again in:\n>>\n>>   https://lore.kernel.org/git/YAhXw9Gvn5Pyvacq@coredump.intra.peff.net/\n>>\n>> so I decided to finally do something about it. There's a minor textual\n>> conflict with the caller being touched in that thread, but it's easy to\n>> resolve. So I think it's OK to consider the two independent.\n>>\n>> The commit message turned out rather long. I hope that doesn't scare\n>> anybody away, but I wanted to cover all of the bases.\n>\n> Thanks for all of the detail. Suffice it to say, I was not scared away,\n> and am in fact quite comfortable with this direction.\n>\n> Sorry for sending such a short response to such a long email, but I\n> agree with the discussion in the patch message, and I don't have\n> anything to add other than my approval and a:\n>\n>   Reviewed-by: Taylor Blau <me@ttaylorr.com>\n>\n> Thanks,\n> Taylor\n\nThanks, both.\n"}]}