{"thread":{"id":"56379","subject":"[RFC PATCH 0/2] First steps towards iterating over submodule refs","startedAt":"2021-08-25T23:23:39Z","lastAt":"2021-09-16T22:05:42Z","messageCount":11,"participants":["Jonathan Tan","Han-Wen Nienhuys","Glen Choo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"433780","messageId":"cover.1629933380.git.jonathantanmy@google.com","threadId":"56379","inReplyTo":null,"subject":"[RFC PATCH 0/2] First steps towards iterating over submodule refs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-25T23:23:29Z","receivedAt":"2021-08-25T23:23:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Here are more results from my submodule partial clone work - teaching\nref iteration to work with submodules by introducing struct repo\nparameters at different points. I'm sending this out early as RFC\n(before implementing the part that actually adds support for other\nrepositories) because there might be a better way to do this that I\nhaven't thought of. (This patch set contains my best idea for how this\ncould be implemented, but admittedly it is a bit unelegant that the repo\nparameter is injected differently into each iterator function.)\n\nThis will break any existing topic that introduces new ref store\nbackends (Han-Wen cc-ed for reftable), although I think that the changes\nintroduced here are not too significant.\n\nJonathan Tan (2):\n  refs: make _advance() check struct repo, not flag\n  refs: add repo paramater to _iterator_peel()\n\n refs.c                | 50 +++++++++++++++++++++++-------------------\n refs/debug.c          | 13 ++++++-----\n refs/files-backend.c  | 31 ++++++++++++++++++--------\n refs/iterator.c       | 38 ++++++++++++++++++++++++--------\n refs/packed-backend.c | 30 ++++++++++++++++---------\n refs/ref-cache.c      |  7 +++---\n refs/refs-internal.h  | 51 +++++++++++++++++++++++++++++--------------\n 7 files changed, 144 insertions(+), 76 deletions(-)\n\n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433781","messageId":"9187eab52552630863285ef5743a107ccc555495.1629933380.git.jonathantanmy@google.com","threadId":"56379","inReplyTo":"cover.1629933380.git.jonathantanmy@google.com","subject":"[RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-25T23:23:30Z","receivedAt":"2021-08-25T23:23:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, ref iterators access the object store each time they advance\nif and only if the boolean flag DO_FOR_EACH_INCLUDE_BROKEN is unset.\n(The iterators access the object store because, if\nDO_FOR_EACH_INCLUDE_BROKEN is unset, they need to attempt to resolve\neach ref to determine that it is not broken.)\n\nAlso, the object store accessed is always that of the_repository, making\nit impossible to iterate over a submodule's refs without\nDO_FOR_EACH_INCLUDE_BROKEN (unless add_submodule_odb() is used).\n\nAs a first step in resolving both these problems, replace the\nDO_FOR_EACH_INCLUDE_BROKEN flag with a struct repository pointer. This\ncommit is a mechanical conversion - whenever DO_FOR_EACH_INCLUDE_BROKEN\nis set, a NULL repository (representing access to no object store) is\nused instead, and whenever DO_FOR_EACH_INCLUDE_BROKEN is unset, a\nnon-NULL repository (representing access to that repository's object\nstore) is used instead. Right now, the locations in which\nnon-the_repository support needs to be added are marked with BUG()\nstatements - in a future patch, these will be replaced. (NEEDSWORK: in\nthis RFC patch set, this has not been done)\n\nI have considered and rejected the following design alternatives:\n\n- Making all ref stores not access the object store during their\n  _advance() callbacks, and making ref_iterator_advance() be responsible\n  for checking the object store - thus, simplifying the code in that the\n  logic of checking for the flag (current) or the pointer (after the\n  equivalent of this commit) is only in one place instead of in every\n  ref store's callback. However, the ref stores already make use of this\n  flag for another reason - for determining if refs are resolvable when\n  writing (search for \"REF_STORE_ODB\"). Thus, I decided to retain each\n  ref store's knowledge of this flag.\n\n- Teaching the ref iterator mechanism to never skip any ref. This has\n  the same problem as above, and furthermore, all callers now need to\n  handle unresolvable refs.\n\n- Change the _advance() callback to also have a repository object\n  parameter, and either skip or not skip depending on whether that\n  parameter is NULL. This burdens callers to have to carry this\n  information along with the iterator, and such calling code may be\n  unclear as to why that parameter can be NULL in some cases and cannot\n  in others.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n refs.c                | 48 +++++++++++++++++++++++--------------------\n refs/debug.c          |  4 ++--\n refs/files-backend.c  | 19 +++++++++++++----\n refs/packed-backend.c | 16 +++++++++++----\n refs/refs-internal.h  | 24 ++++++++++++----------\n 5 files changed, 68 insertions(+), 43 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8b9f7c3a80..35b85f3e79 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1413,16 +1413,16 @@ int head_ref(each_ref_fn fn, void *cb_data)\n \n struct ref_iterator *refs_ref_iterator_begin(\n \t\tstruct ref_store *refs,\n-\t\tconst char *prefix, int trim, int flags)\n+\t\tconst char *prefix, int trim, struct repository *repo,\n+\t\tint flags)\n {\n \tstruct ref_iterator *iter;\n \n \tif (ref_paranoia < 0)\n \t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 0);\n-\tif (ref_paranoia)\n-\t\tflags |= DO_FOR_EACH_INCLUDE_BROKEN;\n \n-\titer = refs->be->iterator_begin(refs, prefix, flags);\n+\titer = refs->be->iterator_begin(refs, prefix,\n+\t\t\t\t\tref_paranoia ? NULL : repo, flags);\n \n \t/*\n \t * `iterator_begin()` already takes care of prefix, but we\n@@ -1442,13 +1442,16 @@ struct ref_iterator *refs_ref_iterator_begin(\n  * Call fn for each reference in the specified submodule for which the\n  * refname begins with prefix. If trim is non-zero, then trim that\n  * many characters off the beginning of each refname before passing\n- * the refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to\n- * include broken references in the iteration. If fn ever returns a\n+ * the refname to fn. If fn ever returns a\n  * non-zero value, stop the iteration and return that value;\n  * otherwise, return 0.\n+ *\n+ * See the documentation of refs_ref_iterator_begin() for more information on\n+ * the repo parameter.\n  */\n static int do_for_each_repo_ref(struct repository *r, const char *prefix,\n-\t\t\t\teach_repo_ref_fn fn, int trim, int flags,\n+\t\t\t\teach_repo_ref_fn fn, int trim,\n+\t\t\t\tstruct repository *repo, int flags,\n \t\t\t\tvoid *cb_data)\n {\n \tstruct ref_iterator *iter;\n@@ -1457,7 +1460,7 @@ static int do_for_each_repo_ref(struct repository *r, const char *prefix,\n \tif (!refs)\n \t\treturn 0;\n \n-\titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n+\titer = refs_ref_iterator_begin(refs, prefix, trim, repo, flags);\n \n \treturn do_for_each_repo_ref_iterator(r, iter, fn, cb_data);\n }\n@@ -1479,7 +1482,8 @@ static int do_for_each_ref_helper(struct repository *r,\n }\n \n static int do_for_each_ref(struct ref_store *refs, const char *prefix,\n-\t\t\t   each_ref_fn fn, int trim, int flags, void *cb_data)\n+\t\t\t   each_ref_fn fn, int trim, struct repository *repo,\n+\t\t\t   int flags, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n \tstruct do_for_each_ref_help hp = { fn, cb_data };\n@@ -1487,7 +1491,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix,\n \tif (!refs)\n \t\treturn 0;\n \n-\titer = refs_ref_iterator_begin(refs, prefix, trim, flags);\n+\titer = refs_ref_iterator_begin(refs, prefix, trim, repo, flags);\n \n \treturn do_for_each_repo_ref_iterator(the_repository, iter,\n \t\t\t\t\tdo_for_each_ref_helper, &hp);\n@@ -1495,7 +1499,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix,\n \n int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(refs, \"\", fn, 0, 0, cb_data);\n+\treturn do_for_each_ref(refs, \"\", fn, 0, the_repository, 0, cb_data);\n }\n \n int for_each_ref(each_ref_fn fn, void *cb_data)\n@@ -1506,7 +1510,7 @@ int for_each_ref(each_ref_fn fn, void *cb_data)\n int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,\n \t\t\t each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(refs, prefix, fn, strlen(prefix), 0, cb_data);\n+\treturn do_for_each_ref(refs, prefix, fn, strlen(prefix), the_repository, 0, cb_data);\n }\n \n int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)\n@@ -1518,10 +1522,10 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsig\n {\n \tunsigned int flag = 0;\n \n-\tif (broken)\n-\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n \treturn do_for_each_ref(get_main_ref_store(the_repository),\n-\t\t\t       prefix, fn, 0, flag, cb_data);\n+\t\t\t       prefix, fn, 0,\n+\t\t\t       broken ? NULL : the_repository,\n+\t\t\t       flag, cb_data);\n }\n \n int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n@@ -1530,16 +1534,16 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,\n {\n \tunsigned int flag = 0;\n \n-\tif (broken)\n-\t\tflag = DO_FOR_EACH_INCLUDE_BROKEN;\n-\treturn do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);\n+\treturn do_for_each_ref(refs, prefix, fn, 0,\n+\t\t\t       broken ? NULL : the_repository,\n+\t\t\t       flag, cb_data);\n }\n \n int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)\n {\n \treturn do_for_each_repo_ref(r, git_replace_ref_base, fn,\n \t\t\t\t    strlen(git_replace_ref_base),\n-\t\t\t\t    DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n+\t\t\t\t    NULL, 0, cb_data);\n }\n \n int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\n@@ -1548,7 +1552,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\n \tint ret;\n \tstrbuf_addf(&buf, \"%srefs/\", get_git_namespace());\n \tret = do_for_each_ref(get_main_ref_store(the_repository),\n-\t\t\t      buf.buf, fn, 0, 0, cb_data);\n+\t\t\t      buf.buf, fn, 0, the_repository, 0, cb_data);\n \tstrbuf_release(&buf);\n \treturn ret;\n }\n@@ -1556,7 +1560,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)\n int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \treturn do_for_each_ref(refs, \"\", fn, 0,\n-\t\t\t       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n+\t\t\t       NULL, 0, cb_data);\n }\n \n int for_each_rawref(each_ref_fn fn, void *cb_data)\n@@ -2263,7 +2267,7 @@ int refs_verify_refname_available(struct ref_store *refs,\n \tstrbuf_addch(&dirname, '/');\n \n \titer = refs_ref_iterator_begin(refs, dirname.buf, 0,\n-\t\t\t\t       DO_FOR_EACH_INCLUDE_BROKEN);\n+\t\t\t\t       NULL, 0);\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n \t\tif (skip &&\n \t\t    string_list_has_string(skip, iter->refname))\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 1a7a9e11cf..753d5da893 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -224,11 +224,11 @@ static struct ref_iterator_vtable debug_ref_iterator_vtable = {\n \n static struct ref_iterator *\n debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,\n-\t\t\t unsigned int flags)\n+\t\t\t struct repository *repo, unsigned int flags)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n \tstruct ref_iterator *res =\n-\t\tdrefs->refs->be->iterator_begin(drefs->refs, prefix, flags);\n+\t\tdrefs->refs->be->iterator_begin(drefs->refs, prefix, repo, flags);\n \tstruct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));\n \tbase_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);\n \tditer->iter = res;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 677b7e4cdd..4c42db1092 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -730,6 +730,7 @@ struct files_ref_iterator {\n \tstruct ref_iterator base;\n \n \tstruct ref_iterator *iter0;\n+\tstruct repository *repo;\n \tunsigned int flags;\n };\n \n@@ -744,7 +745,13 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t    ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)\n \t\t\tcontinue;\n \n-\t\tif (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n+\t\tif (iter->repo && iter->repo != the_repository)\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: make ref_resolves_to_object() support\n+\t\t\t * arbitrary repositories\n+\t\t\t */\n+\t\t\tBUG(\"iter->repo must be NULL or the_repository\");\n+\t\tif (iter->repo &&\n \t\t    !ref_resolves_to_object(iter->iter0->refname,\n \t\t\t\t\t    iter->iter0->oid,\n \t\t\t\t\t    iter->iter0->flags))\n@@ -793,7 +800,7 @@ static struct ref_iterator_vtable files_ref_iterator_vtable = {\n \n static struct ref_iterator *files_ref_iterator_begin(\n \t\tstruct ref_store *ref_store,\n-\t\tconst char *prefix, unsigned int flags)\n+\t\tconst char *prefix, struct repository *repo, unsigned int flags)\n {\n \tstruct files_ref_store *refs;\n \tstruct ref_iterator *loose_iter, *packed_iter, *overlay_iter;\n@@ -801,7 +808,7 @@ static struct ref_iterator *files_ref_iterator_begin(\n \tstruct ref_iterator *ref_iterator;\n \tunsigned int required_flags = REF_STORE_READ;\n \n-\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))\n+\tif (repo)\n \t\trequired_flags |= REF_STORE_ODB;\n \n \trefs = files_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n@@ -836,10 +843,13 @@ static struct ref_iterator *files_ref_iterator_begin(\n \t * references, and (if needed) do our own check for broken\n \t * ones in files_ref_iterator_advance(), after we have merged\n \t * the packed and loose references.\n+\t *\n+\t * Do this by not supplying any repo, regardless of whether a repo was\n+\t * supplied to files_ref_iterator_begin().\n \t */\n \tpacked_iter = refs_ref_iterator_begin(\n \t\t\trefs->packed_ref_store, prefix, 0,\n-\t\t\tDO_FOR_EACH_INCLUDE_BROKEN);\n+\t\t\tNULL, 0);\n \n \toverlay_iter = overlay_ref_iterator_begin(loose_iter, packed_iter);\n \n@@ -848,6 +858,7 @@ static struct ref_iterator *files_ref_iterator_begin(\n \tbase_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable,\n \t\t\t       overlay_iter->ordered);\n \titer->iter0 = overlay_iter;\n+\titer->repo = repo;\n \titer->flags = flags;\n \n \treturn ref_iterator;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex f8aa97d799..bc2302a6e0 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -776,6 +776,7 @@ struct packed_ref_iterator {\n \tstruct object_id oid, peeled;\n \tstruct strbuf refname_buf;\n \n+\tstruct repository *repo;\n \tunsigned int flags;\n };\n \n@@ -863,7 +864,13 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t    ref_type(iter->base.refname) != REF_TYPE_PER_WORKTREE)\n \t\t\tcontinue;\n \n-\t\tif (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n+\t\tif (iter->repo && iter->repo != the_repository)\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: make ref_resolves_to_object() support\n+\t\t\t * arbitrary repositories\n+\t\t\t */\n+\t\t\tBUG(\"iter->repo must be NULL or the_repository\");\n+\t\tif (iter->repo &&\n \t\t    !ref_resolves_to_object(iter->base.refname, &iter->oid,\n \t\t\t\t\t    iter->flags))\n \t\t\tcontinue;\n@@ -913,7 +920,7 @@ static struct ref_iterator_vtable packed_ref_iterator_vtable = {\n \n static struct ref_iterator *packed_ref_iterator_begin(\n \t\tstruct ref_store *ref_store,\n-\t\tconst char *prefix, unsigned int flags)\n+\t\tconst char *prefix, struct repository *repo, unsigned int flags)\n {\n \tstruct packed_ref_store *refs;\n \tstruct snapshot *snapshot;\n@@ -922,7 +929,7 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \tstruct ref_iterator *ref_iterator;\n \tunsigned int required_flags = REF_STORE_READ;\n \n-\tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))\n+\tif (repo)\n \t\trequired_flags |= REF_STORE_ODB;\n \trefs = packed_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n@@ -954,6 +961,7 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \n \titer->base.oid = &iter->oid;\n \n+\titer->repo = repo;\n \titer->flags = flags;\n \n \tif (prefix && *prefix)\n@@ -1137,7 +1145,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t * of updates is exhausted, leave i set to updates->nr.\n \t */\n \titer = packed_ref_iterator_begin(&refs->base, \"\",\n-\t\t\t\t\t DO_FOR_EACH_INCLUDE_BROKEN);\n+\t\t\t\t\t NULL, 0);\n \tif ((ok = ref_iterator_advance(iter)) != ITER_OK)\n \t\titer = NULL;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 3155708345..b20fa1f5cd 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -245,9 +245,6 @@ int refs_rename_ref_available(struct ref_store *refs,\n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define SYMREF_MAXDEPTH 5\n \n-/* Include broken references in a do_for_each_ref*() iteration: */\n-#define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n-\n /*\n  * Reference iterators\n  *\n@@ -349,16 +346,19 @@ int is_empty_ref_iterator(struct ref_iterator *ref_iterator);\n  * Return an iterator that goes over each reference in `refs` for\n  * which the refname begins with prefix. If trim is non-zero, then\n  * trim that many characters off the beginning of each refname.\n- * The output is ordered by refname. The following flags are supported:\n+ * The output is ordered by refname.\n+ *\n+ * Pass NULL as repo to include broken references in the iteration, or non-NULL\n+ * to skip references that do not resolve to an object in the given repo.\n  *\n- * DO_FOR_EACH_INCLUDE_BROKEN: include broken references in\n- *         the iteration.\n+ * The following flags are supported:\n  *\n  * DO_FOR_EACH_PER_WORKTREE_ONLY: only produce REF_TYPE_PER_WORKTREE refs.\n  */\n struct ref_iterator *refs_ref_iterator_begin(\n \t\tstruct ref_store *refs,\n-\t\tconst char *prefix, int trim, int flags);\n+\t\tconst char *prefix, int trim, struct repository *repo,\n+\t\tint flags);\n \n /*\n  * A callback function used to instruct merge_ref_iterator how to\n@@ -446,8 +446,9 @@ void base_ref_iterator_free(struct ref_iterator *iter);\n /*\n  * backend-specific implementation of ref_iterator_advance. For symrefs, the\n  * function should set REF_ISSYMREF, and it should also dereference the symref\n- * to provide the OID referent. If DO_FOR_EACH_INCLUDE_BROKEN is set, symrefs\n- * with non-existent referents and refs pointing to non-existent object names\n+ * to provide the OID referent. If a NULL repo was passed to the _begin()\n+ * function that created this iterator, symrefs with non-existent referents and\n+ * refs pointing to non-existent object names\n  * should also be returned. If DO_FOR_EACH_PER_WORKTREE_ONLY, only\n  * REF_TYPE_PER_WORKTREE refs should be returned.\n  */\n@@ -504,7 +505,7 @@ int do_for_each_repo_ref_iterator(struct repository *r,\n  * where all reference backends will presumably store their\n  * per-worktree refs.\n  */\n-#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02\n+#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x01\n \n struct ref_store;\n \n@@ -569,7 +570,8 @@ typedef int copy_ref_fn(struct ref_store *ref_store,\n  */\n typedef struct ref_iterator *ref_iterator_begin_fn(\n \t\tstruct ref_store *ref_store,\n-\t\tconst char *prefix, unsigned int flags);\n+\t\tconst char *prefix, struct repository *repo,\n+\t\tunsigned int flags);\n \n /* reflog functions */\n \n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433782","messageId":"2e93cc60e959d18909405a1d22c650228bf2fcb9.1629933380.git.jonathantanmy@google.com","threadId":"56379","inReplyTo":"cover.1629933380.git.jonathantanmy@google.com","subject":"[RFC PATCH 2/2] refs: add repo paramater to _iterator_peel()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-25T23:23:31Z","receivedAt":"2021-08-25T23:23:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Split the ref_iterator_peel() function into two functions: one that\nreturns information solely based on what the ref store contains\n(success, failure, inconclusive), and one that takes a repo parameter\nand accesses the object store if need be. Update the ref store's\ncallbacks to not access the object store, and to return\nsuccess/failure/inconclusive instead of a binary success/failure.\n\nThis makes it explicit whether a peel attempt may access the object\nstore of a repository.\n\nThe approach taken in this commit for peeling is different from the\napproach taken in the parent commit for advancing:\n\n- It is complicated to reuse the repo field (which determines if an\n  object store is ever accessed during advancing, and if yes, which\n  object store) added to ref stores in the parent commit; the files ref\n  store wraps the packed ref store, and it does not want the packed ref\n  store to access any object store during advancing (as described in\n  files_ref_iterator_begin()) - thus repo is NULL - but it wants packed\n  ref store peeling.\n\n- Having the repo handy when peeling is not as cumbersome as it is when\n  advancing. Firstly, the repo in this case is always non-NULL, and\n  secondly, peeling is typically followed by reading the object, which\n  requires the repo anyway.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n refs.c                |  2 +-\n refs/debug.c          |  9 +++++----\n refs/files-backend.c  | 12 +++++++-----\n refs/iterator.c       | 38 +++++++++++++++++++++++++++++---------\n refs/packed-backend.c | 14 ++++++++------\n refs/ref-cache.c      |  7 ++++---\n refs/refs-internal.h  | 27 ++++++++++++++++++++++-----\n 7 files changed, 76 insertions(+), 33 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 35b85f3e79..7bc23bcc3f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2012,7 +2012,7 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled)\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+\t\treturn ref_iterator_peel(current_ref_iter, the_repository, peeled);\n \n \treturn peel_object(base, peeled) ? -1 : 0;\n }\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 753d5da893..7291a1a4fb 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -198,13 +198,14 @@ static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn res;\n }\n \n-static int debug_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t   struct object_id *peeled)\n+static enum ref_iterator_peel_result debug_ref_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n \tstruct debug_ref_iterator *diter =\n \t\t(struct debug_ref_iterator *)ref_iterator;\n-\tint res = diter->iter->vtable->peel(diter->iter, peeled);\n-\ttrace_printf_key(&trace_refs, \"iterator_peel: %s: %d\\n\", diter->iter->refname, res);\n+\tenum ref_iterator_peel_result res = diter->iter->vtable->peel(diter->iter, peeled);\n+\ttrace_printf_key(&trace_refs, \"iterator_peel: %s: %d\\n\", diter->iter->refname, (int) res);\n \treturn res;\n }\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4c42db1092..e1930848b1 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -770,13 +770,14 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn ok;\n }\n \n-static int files_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t   struct object_id *peeled)\n+static enum ref_iterator_peel_result files_ref_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n \tstruct files_ref_iterator *iter =\n \t\t(struct files_ref_iterator *)ref_iterator;\n \n-\treturn ref_iterator_peel(iter->iter0, peeled);\n+\treturn ref_iterator_peel_raw(iter->iter0, peeled);\n }\n \n static int files_ref_iterator_abort(struct ref_iterator *ref_iterator)\n@@ -2122,8 +2123,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn ok;\n }\n \n-static int files_reflog_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t   struct object_id *peeled)\n+static enum ref_iterator_peel_result files_reflog_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n \tBUG(\"ref_iterator_peel() called for reflog_iterator\");\n }\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex a89d132d4f..386e6310ed 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -13,10 +13,27 @@ int ref_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn ref_iterator->vtable->advance(ref_iterator);\n }\n \n+enum ref_iterator_peel_result ref_iterator_peel_raw(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n+{\n+\treturn ref_iterator->vtable->peel(ref_iterator, peeled);\n+}\n+\n int ref_iterator_peel(struct ref_iterator *ref_iterator,\n+\t\t      struct repository *repo,\n \t\t      struct object_id *peeled)\n {\n-\treturn ref_iterator->vtable->peel(ref_iterator, peeled);\n+\tenum ref_iterator_peel_result result =\n+\t\tref_iterator_peel_raw(ref_iterator, peeled);\n+\n+\tif (repo != the_repository)\n+\t\t/* NEEDSWORK: make peel_object() work with all repositories */\n+\t\tBUG(\"ref_iterator_peel() can only be used with the_repository\");\n+\tif (result == REF_ITERATOR_PEEL_INCONCLUSIVE)\n+\t\treturn peel_object(ref_iterator->oid, peeled) == PEEL_PEELED ?\n+\t\t\t0 : -1;\n+\treturn result == REF_ITERATOR_PEEL_SUCCESS ? 0 : -1;\n }\n \n int ref_iterator_abort(struct ref_iterator *ref_iterator)\n@@ -51,8 +68,9 @@ static int empty_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn ref_iterator_abort(ref_iterator);\n }\n \n-static int empty_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t   struct object_id *peeled)\n+static enum ref_iterator_peel_result empty_ref_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n \tBUG(\"peel called for empty iterator\");\n }\n@@ -170,8 +188,9 @@ static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn ITER_ERROR;\n }\n \n-static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t   struct object_id *peeled)\n+static enum ref_iterator_peel_result merge_ref_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n \tstruct merge_ref_iterator *iter =\n \t\t(struct merge_ref_iterator *)ref_iterator;\n@@ -179,7 +198,7 @@ static int merge_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \tif (!iter->current) {\n \t\tBUG(\"peel called before advance for merge iterator\");\n \t}\n-\treturn ref_iterator_peel(*iter->current, peeled);\n+\treturn ref_iterator_peel_raw(*iter->current, peeled);\n }\n \n static int merge_ref_iterator_abort(struct ref_iterator *ref_iterator)\n@@ -355,13 +374,14 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn ok;\n }\n \n-static int prefix_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t    struct object_id *peeled)\n+static enum ref_iterator_peel_result prefix_ref_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n \tstruct prefix_ref_iterator *iter =\n \t\t(struct prefix_ref_iterator *)ref_iterator;\n \n-\treturn ref_iterator_peel(iter->iter0, peeled);\n+\treturn ref_iterator_peel_raw(iter->iter0, peeled);\n }\n \n static int prefix_ref_iterator_abort(struct ref_iterator *ref_iterator)\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex bc2302a6e0..ef14390043 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -884,19 +884,21 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \treturn ok;\n }\n \n-static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t   struct object_id *peeled)\n+static enum ref_iterator_peel_result packed_ref_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n \tstruct packed_ref_iterator *iter =\n \t\t(struct packed_ref_iterator *)ref_iterator;\n \n \tif ((iter->base.flags & REF_KNOWS_PEELED)) {\n \t\toidcpy(peeled, &iter->peeled);\n-\t\treturn is_null_oid(&iter->peeled) ? -1 : 0;\n+\t\treturn is_null_oid(&iter->peeled) ?\n+\t\t\tREF_ITERATOR_PEEL_FAILURE : REF_ITERATOR_PEEL_SUCCESS;\n \t} else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {\n-\t\treturn -1;\n+\t\treturn REF_ITERATOR_PEEL_FAILURE;\n \t} else {\n-\t\treturn peel_object(&iter->oid, peeled) ? -1 : 0;\n+\t\treturn REF_ITERATOR_PEEL_INCONCLUSIVE;\n \t}\n }\n \n@@ -1226,7 +1228,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\t\t/* Pass the old reference through. */\n \n \t\t\tstruct object_id peeled;\n-\t\t\tint peel_error = ref_iterator_peel(iter, &peeled);\n+\t\t\tint peel_error = ref_iterator_peel(iter, the_repository, &peeled);\n \n \t\t\tif (write_packed_entry(out, iter->refname,\n \t\t\t\t\t       iter->oid,\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 49d732f6db..031b613bb2 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -488,10 +488,11 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t}\n }\n \n-static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t   struct object_id *peeled)\n+static enum ref_iterator_peel_result cache_ref_iterator_peel(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled)\n {\n-\treturn peel_object(ref_iterator->oid, peeled) ? -1 : 0;\n+\treturn REF_ITERATOR_PEEL_INCONCLUSIVE;\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 b20fa1f5cd..aea8b0844f 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -317,11 +317,26 @@ struct ref_iterator {\n  */\n int ref_iterator_advance(struct ref_iterator *ref_iterator);\n \n+enum ref_iterator_peel_result {\n+\tREF_ITERATOR_PEEL_SUCCESS,\n+\tREF_ITERATOR_PEEL_FAILURE,\n+\tREF_ITERATOR_PEEL_INCONCLUSIVE\n+};\n+\n+/*\n+ * Peel the reference currently being viewed by the iterator without\n+ * using any information from any object store.\n+ */\n+enum ref_iterator_peel_result ref_iterator_peel_raw(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled);\n+\n /*\n- * If possible, peel the reference currently being viewed by the\n- * iterator. Return 0 on success.\n+ * Peel the reference currently being viewed by the iterator, using the object\n+ * store if the ref store has insufficient information. Returns 0 upon success.\n  */\n int ref_iterator_peel(struct ref_iterator *ref_iterator,\n+\t\t      struct repository *repo,\n \t\t      struct object_id *peeled);\n \n /*\n@@ -455,10 +470,12 @@ 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 or -1 for failure.\n+ * Peels the current ref using only information from the ref store. If there is\n+ * not enough information, returns REF_ITERATOR_PEEL_INCONCLUSIVE.\n  */\n-typedef int ref_iterator_peel_fn(struct ref_iterator *ref_iterator,\n-\t\t\t\t struct object_id *peeled);\n+typedef enum ref_iterator_peel_result ref_iterator_peel_fn(\n+\t\tstruct ref_iterator *ref_iterator,\n+\t\tstruct object_id *peeled);\n \n /*\n  * Implementations of this function should free any resources specific\n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433848","messageId":"CAFQ2z_PKKZJY1kC1QJo8Zwq_yNh5QNGc3S5bq1jBfSfK3vQwRQ@mail.gmail.com","threadId":"56379","inReplyTo":"9187eab52552630863285ef5743a107ccc555495.1629933380.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-08-26T16:39:41Z","receivedAt":"2021-08-26T16:47:07Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Aug 26, 2021 at 1:23 AM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> Currently, ref iterators access the object store each time they advance\n> if and only if the boolean flag DO_FOR_EACH_INCLUDE_BROKEN is unset.\n> (The iterators access the object store because, if\n> DO_FOR_EACH_INCLUDE_BROKEN is unset, they need to attempt to resolve\n> each ref to determine that it is not broken.)\n>\n> Also, the object store accessed is always that of the_repository, making\n> it impossible to iterate over a submodule's refs without\n> DO_FOR_EACH_INCLUDE_BROKEN (unless add_submodule_odb() is used).\n>\n> As a first step in resolving both these problems, replace the\n> DO_FOR_EACH_INCLUDE_BROKEN flag with a struct repository pointer. This\n> commit is a mechanical conversion - whenever DO_FOR_EACH_INCLUDE_BROKEN\n> is set, a NULL repository (representing access to no object store) is\n> used instead, and whenever DO_FOR_EACH_INCLUDE_BROKEN is unset, a\n> non-NULL repository (representing access to that repository's object\n> store) is used instead. Right now, the locations in which\n> non-the_repository support needs to be added are marked with BUG()\n> statements - in a future patch, these will be replaced. (NEEDSWORK: in\n> this RFC patch set, this has not been done)\n\nfrom a design perspective, it would be nice if the ref backend\nwouldn't need to know about the object store. Can't this be hidden in\nthe layer in refs.c that calls into the backends?\n\nIf they have to know about the object store, have you considered\npassing the repository pointer\nin xxx_ref_store_create() ? Then there is no possibliity to mismatch\nthe repository pointers and with the ref store.\n\n> - Making all ref stores not access the object store during their\n>   _advance() callbacks, and making ref_iterator_advance() be responsible\n>   for checking the object store - thus, simplifying the code in that the\n>   logic of checking for the flag (current) or the pointer (after the\n>   equivalent of this commit) is only in one place instead of in every\n>   ref store's callback. However, the ref stores already make use of this\n>   flag for another reason - for determining if refs are resolvable when\n>   writing (search for \"REF_STORE_ODB\"). Thus, I decided to retain each\n\nI looked, but I couldn't figure out how this flag is used.\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nHan-Wen Nienhuys\nGoogle Munich\nhanwen@google.com\n"},{"id":"433886","messageId":"20210826222439.3915402-1-jonathantanmy@google.com","threadId":"56379","inReplyTo":"CAFQ2z_PKKZJY1kC1QJo8Zwq_yNh5QNGc3S5bq1jBfSfK3vQwRQ@mail.gmail.com","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-26T22:24:39Z","receivedAt":"2021-08-26T22:24:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> from a design perspective, it would be nice if the ref backend\n> wouldn't need to know about the object store. Can't this be hidden in\n> the layer in refs.c that calls into the backends?\n\nThanks for taking a look.\n\nThe answer requires additional context, so I'll answer this at the end\nof this email.\n\n> If they have to know about the object store, have you considered\n> passing the repository pointer\n> in xxx_ref_store_create() ? Then there is no possibliity to mismatch\n> the repository pointers and with the ref store.\n\nI thought about that, but didn't want to make things worse - the effort\nin this patch set is, after all, to attempt to increase the dissociation\nbetween the ref stores and a certain object store (that is,\nthe_repository's object store), and I thought that reintroducing an\nassociation (albeit to arbitrary object stores instead of a hardcoded\nobject store) would be a step back.\n\nBut this may be the way to go - the ref stores already have a gitdir\nfield that we could replace with a struct repository field.\n\n> > - Making all ref stores not access the object store during their\n> >   _advance() callbacks, and making ref_iterator_advance() be responsible\n> >   for checking the object store - thus, simplifying the code in that the\n> >   logic of checking for the flag (current) or the pointer (after the\n> >   equivalent of this commit) is only in one place instead of in every\n> >   ref store's callback. However, the ref stores already make use of this\n> >   flag for another reason - for determining if refs are resolvable when\n> >   writing (search for \"REF_STORE_ODB\"). Thus, I decided to retain each\n> \n> I looked, but I couldn't figure out how this flag is used.\n\nI was thinking of files_ref_iterator_begin() setting a local variable\nrequired_flags. Somehow I thought that files_pack_refs() relied on\nfiles_ref_iterator_begin() setting that variable, but now I see that\nthat's not true - both functions are independently checking that the\nunderlying ref store supports ODB access, so I can remove ODB from\nfiles_ref_iterator_begin() if I want to.\n\nTo go back to the question at the top, now I agree that hiding the ODB\naccess in _advance() in the layer in refs.c is possible. The last part\nstill accessing the ODB is files_pack_refs(), I think. Refactoring that\nis possible, but I'll leave that to another patch set.\n\nIf you or anyone else has more questions or comments, please reply - and\nin the meantime, I'll update this patch set to move the ODB access in\n_advance() to the layer in refs.c.\n"},{"id":"435936","messageId":"YUElL3RI0VTnjE5C@chooglen-macbookpro.roam.corp.google.com","threadId":"56379","inReplyTo":"20210826222439.3915402-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2021-09-14T22:41:51Z","receivedAt":"2021-09-14T22:41:57Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"On Thu, Aug 26, 2021 at 03:24:39PM -0700, Jonathan Tan wrote:\n> > If they have to know about the object store, have you considered\n> > passing the repository pointer\n> > in xxx_ref_store_create() ? Then there is no possibliity to mismatch\n> > the repository pointers and with the ref store.\n> \n> I thought about that, but didn't want to make things worse - the effort\n> in this patch set is, after all, to attempt to increase the dissociation\n> between the ref stores and a certain object store (that is,\n> the_repository's object store), and I thought that reintroducing an\n> association (albeit to arbitrary object stores instead of a hardcoded\n> object store) would be a step back.\n> \n> But this may be the way to go - the ref stores already have a gitdir\n> field that we could replace with a struct repository field.\n\nI'm curious about how we'd want to resolve the general problem of ref\nstores referencing odbs. \n\nA discussion I had with Jonathan Nieder suggests that ref stores are\ndoing two slightly related, but not equivalent things:\n\n- a logical ref database that preserves its own consistency\n- a layer of ref storage in such a ref database\n\nIn the current state of affairs, the files ref store and the packed ref\nstore seem to behave as a single logical ref database. An example of\nthis (that I care about in particular) is in refs/files-backend.c where\nthe files backend validates oids using the_repository's odb.\nrefs/packed-backend.c doesn't do any such validation, and presumably\njust relies on the correctness of refs/files-backend.c. I assume that\nthis also explains why some functions in refs_be_packed are stubs.\n\nThe answer to whether or not a ref store should refer to a certain\nobject store seems unresolved because a ref store is trying to do two\nseparate things. Perhaps it is reasonable to associate a ref database\nwith an object store (so that it can validate its refs), but we would\nprefer to dissociate the physical ref storage layer from the object\nstore. (I'm paraphrasing Johnathan Nieder here, this isn't an original\nthought).\n\nPerhaps this is a question we want to resolve when considering reftable\nand other ref databases.\n"},{"id":"435966","messageId":"CAFQ2z_ML_g6DTiG92srq8UCCu_D8bi6z1mP_znt24TJagVfi2g@mail.gmail.com","threadId":"56379","inReplyTo":"YUElL3RI0VTnjE5C@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-09-15T07:35:06Z","receivedAt":"2021-09-15T07:35:22Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Sep 15, 2021 at 12:41 AM Glen Choo <chooglen@google.com> wrote:\n> In the current state of affairs, the files ref store and the packed ref\n> store seem to behave as a single logical ref database. An example of\n> this (that I care about in particular) is in refs/files-backend.c where\n> the files backend validates oids using the_repository's odb.\n> refs/packed-backend.c doesn't do any such validation, and presumably\n> just relies on the correctness of refs/files-backend.c. I assume that\n> this also explains why some functions in refs_be_packed are stubs.\n\nThe loose/packed storage is implemented in terms of files backend (the\npublic entry point) that defers to a packed backend in some cases. The\nlatter is implemented as a ref backend, but for no good reason.\n\n> The answer to whether or not a ref store should refer to a certain\n> object store seems unresolved because a ref store is trying to do two\n> separate things. Perhaps it is reasonable to associate a ref database\n> with an object store (so that it can validate its refs), but we would\n> prefer to dissociate the physical ref storage layer from the object\n> store. (I'm paraphrasing Johnathan Nieder here, this isn't an original\n> thought).\n>\n> Perhaps this is a question we want to resolve when considering reftable\n> and other ref databases.\n\nWork on reftable shows that there are more egregious breaks of\nabstraction boundaries. For example, there are still parts of the code\nthat equate\n\n  (file under .git/ == ref)\n\nyou can find a good part of them if you run GIT_TEST_REFTABLE=1 with\nthe reftable support switched on. Another place where API contracts\nare unclear is resolving symrefs: on first sight, you'd think that a\nref backend should just provide storage for a refname => {symref,\ncommit SHA-1, tag + commit SHA-1} mapping. However, in some places it\nis currently necessary for the ref backend to resolve symrefs. You can\nfind these places by grepping for refs_resolve_ref_unsafe() in the\nfiles backend.\n\nI think Jonathan is right, but I also think that teasing apart the ref\nbackend and the ODB is premature until the ref backend itself is a\nstrongly enforced abstraction boundary.\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":"436135","messageId":"20210916172432.1073546-1-jonathantanmy@google.com","threadId":"56379","inReplyTo":"YUElL3RI0VTnjE5C@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-16T17:24:32Z","receivedAt":"2021-09-16T18:47:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> The answer to whether or not a ref store should refer to a certain\n> object store seems unresolved because a ref store is trying to do two\n> separate things. Perhaps it is reasonable to associate a ref database\n> with an object store (so that it can validate its refs), but we would\n> prefer to dissociate the physical ref storage layer from the object\n> store. (I'm paraphrasing Johnathan Nieder here, this isn't an original\n> thought).\n> \n> Perhaps this is a question we want to resolve when considering reftable\n> and other ref databases.\n\nEither adding an explicit dependency on an object store to a ref store\nor dissociating it would be an improvement over what we have now, which\nis an implicit dependency on the_repository's object store. Of the two,\nI also prefer dissociating it. In practice, if I remember correctly, the\npart that checks object existence during ref writing is the last\ndependency, so if we can eliminate that without a convoluted design, I\nthink it's worth dissociating.\n"},{"id":"436136","messageId":"20210916172609.1074157-1-jonathantanmy@google.com","threadId":"56379","inReplyTo":"CAFQ2z_ML_g6DTiG92srq8UCCu_D8bi6z1mP_znt24TJagVfi2g@mail.gmail.com","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-16T17:26:09Z","receivedAt":"2021-09-16T18:47:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Wed, Sep 15, 2021 at 12:41 AM Glen Choo <chooglen@google.com> wrote:\n> > In the current state of affairs, the files ref store and the packed ref\n> > store seem to behave as a single logical ref database. An example of\n> > this (that I care about in particular) is in refs/files-backend.c where\n> > the files backend validates oids using the_repository's odb.\n> > refs/packed-backend.c doesn't do any such validation, and presumably\n> > just relies on the correctness of refs/files-backend.c. I assume that\n> > this also explains why some functions in refs_be_packed are stubs.\n> \n> The loose/packed storage is implemented in terms of files backend (the\n> public entry point) that defers to a packed backend in some cases. The\n> latter is implemented as a ref backend, but for no good reason.\n\nYes, the packed backend doesn't need to be a ref backend.\n\n> I think Jonathan is right, but I also think that teasing apart the ref\n> backend and the ODB is premature until the ref backend itself is a\n> strongly enforced abstraction boundary.\n\nI think both efforts can proceed independently.\n"},{"id":"436172","messageId":"xmqqilz0rxpm.fsf@gitster.g","threadId":"56379","inReplyTo":"20210916172609.1074157-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-16T21:56:37Z","receivedAt":"2021-09-16T21:56:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> On Wed, Sep 15, 2021 at 12:41 AM Glen Choo <chooglen@google.com> wrote:\n>> > In the current state of affairs, the files ref store and the packed ref\n>> > store seem to behave as a single logical ref database. An example of\n>> > this (that I care about in particular) is in refs/files-backend.c where\n>> > the files backend validates oids using the_repository's odb.\n>> > refs/packed-backend.c doesn't do any such validation, and presumably\n>> > just relies on the correctness of refs/files-backend.c. I assume that\n>> > this also explains why some functions in refs_be_packed are stubs.\n>> \n>> The loose/packed storage is implemented in terms of files backend (the\n>> public entry point) that defers to a packed backend in some cases. The\n>> latter is implemented as a ref backend, but for no good reason.\n>\n> Yes, the packed backend doesn't need to be a ref backend.\n\nSorry, I do not follow.  Do you mean we cannot have a version of Git\nthat offers say a read-only access to the repository without any\nloose refs, with the default ref backend being the packed one?\n\nOr do you mean that we can ignore such a hypothetical use case and\ncould reimplement the files backend that can also understand the\n$GIT_DIR/packed-refs file directly without \"deferring to another ref\nbackend which is 'packed'\"?\n\n"},{"id":"436174","messageId":"20210916220536.1107783-1-jonathantanmy@google.com","threadId":"56379","inReplyTo":"xmqqilz0rxpm.fsf@gitster.g","subject":"Re: [RFC PATCH 1/2] refs: make _advance() check struct repo, not flag","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-16T22:05:36Z","receivedAt":"2021-09-16T22:05:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Yes, the packed backend doesn't need to be a ref backend.\n> \n> Sorry, I do not follow.  Do you mean we cannot have a version of Git\n> that offers say a read-only access to the repository without any\n> loose refs, with the default ref backend being the packed one?\n> \n> Or do you mean that we can ignore such a hypothetical use case and\n> could reimplement the files backend that can also understand the\n> $GIT_DIR/packed-refs file directly without \"deferring to another ref\n> backend which is 'packed'\"?\n\nI meant the latter - more specifically, the files backend could defer to\nfunctions that are not necessarily inside a struct ref_storage_be.\n"}]}