git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 1/9] refs: make _advance() check struct repo, not flag

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Sep 21, 2021, 16:51 UTC
Message-ID
<493fff7f4716d889da751b5f8c6740cc1e3aa360.1632242495.git.jonathantanmy@google.com>
In-Reply-To
<cover.1632242495.git.jonathantanmy@google.com>

Currently, ref iterators access the object store each time they advance if and only if the boolean flag DO_FOR_EACH_INCLUDE_BROKEN is unset. (The iterators access the object store because, if DO_FOR_EACH_INCLUDE_BROKEN is unset, they need to attempt to resolve each ref to determine that it is not broken.)

Also, the object store accessed is always that of the_repository, making it impossible to iterate over a submodule's refs without DO_FOR_EACH_INCLUDE_BROKEN (unless add_submodule_odb() is used).

As a first step in resolving both these problems, replace the DO_FOR_EACH_INCLUDE_BROKEN flag with a struct repository pointer. This commit is a mechanical conversion - whenever DO_FOR_EACH_INCLUDE_BROKEN is set, a NULL repository (representing access to no object store) is used instead, and whenever DO_FOR_EACH_INCLUDE_BROKEN is unset, a non-NULL repository (representing access to that repository's object store) is used instead. Right now, the locations in which non-the_repository support needs to be added are marked with BUG() statements - in a future patch, these will be replaced. (NEEDSWORK: in this RFC patch set, this has not been done)

I have considered and rejected the following design alternative:
- Change the _advance() callback to also have a repository object
  parameter, and either skip or not skip depending on whether that
  parameter is NULL. This burdens callers to have to carry this
  information along with the iterator, and such calling code may be
  unclear as to why that parameter can be NULL in some cases and cannot
  in others.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 refs.c                | 46 +++++++++++++++++++++++--------------------
 refs/files-backend.c  | 14 ++++---------
 refs/iterator.c       | 18 ++++++++++++++++-
 refs/packed-backend.c | 10 +---------
 refs/refs-internal.h  | 27 +++++++++++++++----------
 5 files changed, 64 insertions(+), 51 deletions(-)
diff --git a/refs.c b/refs.c
index 8b9f7c3a80..49ddcdac53 100644
--- a/refs.c
+++ b/refs.c
@@ -1413,16 +1413,16 @@ int head_ref(each_ref_fn fn, void *cb_data)
 
 struct ref_iterator *refs_ref_iterator_begin(
 		struct ref_store *refs,
-		const char *prefix, int trim, int flags)
+		const char *prefix, int trim, struct repository *repo,
+		int flags)
 {
 	struct ref_iterator *iter;
 
 	if (ref_paranoia < 0)
 		ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
-	if (ref_paranoia)
-		flags |= DO_FOR_EACH_INCLUDE_BROKEN;
 
 	iter = refs->be->iterator_begin(refs, prefix, flags);
+	iter->repo = ref_paranoia ? NULL : repo;
 
 	/*
 	 * `iterator_begin()` already takes care of prefix, but we
@@ -1442,13 +1442,16 @@ struct ref_iterator *refs_ref_iterator_begin(
  * Call fn for each reference in the specified submodule for which the
  * refname begins with prefix. If trim is non-zero, then trim that
  * many characters off the beginning of each refname before passing
- * the refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to
- * include broken references in the iteration. If fn ever returns a
+ * the refname to fn. If fn ever returns a
  * non-zero value, stop the iteration and return that value;
  * otherwise, return 0.
+ *
+ * See the documentation of refs_ref_iterator_begin() for more information on
+ * the repo parameter.
  */
 static int do_for_each_repo_ref(struct repository *r, const char *prefix,
-				each_repo_ref_fn fn, int trim, int flags,
+				each_repo_ref_fn fn, int trim,
+				struct repository *repo, int flags,
 				void *cb_data)
 {
 	struct ref_iterator *iter;
@@ -1457,7 +1460,7 @@ static int do_for_each_repo_ref(struct repository *r, const char *prefix,
 	if (!refs)
 		return 0;
 
-	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
+	iter = refs_ref_iterator_begin(refs, prefix, trim, repo, flags);
 
 	return do_for_each_repo_ref_iterator(r, iter, fn, cb_data);
 }
@@ -1479,7 +1482,8 @@ static int do_for_each_ref_helper(struct repository *r,
 }
 
 static int do_for_each_ref(struct ref_store *refs, const char *prefix,
-			   each_ref_fn fn, int trim, int flags, void *cb_data)
+			   each_ref_fn fn, int trim, struct repository *repo,
+			   int flags, void *cb_data)
 {
 	struct ref_iterator *iter;
 	struct do_for_each_ref_help hp = { fn, cb_data };
@@ -1487,7 +1491,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix,
 	if (!refs)
 		return 0;
 
-	iter = refs_ref_iterator_begin(refs, prefix, trim, flags);
+	iter = refs_ref_iterator_begin(refs, prefix, trim, repo, flags);
 
 	return do_for_each_repo_ref_iterator(the_repository, iter,
 					do_for_each_ref_helper, &hp);
@@ -1495,7 +1499,7 @@ static int do_for_each_ref(struct ref_store *refs, const char *prefix,
 
 int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(refs, "", fn, 0, 0, cb_data);
+	return do_for_each_ref(refs, "", fn, 0, the_repository, 0, cb_data);
 }
 
 int for_each_ref(each_ref_fn fn, void *cb_data)
@@ -1506,7 +1510,7 @@ int for_each_ref(each_ref_fn fn, void *cb_data)
 int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
 			 each_ref_fn fn, void *cb_data)
 {
-	return do_for_each_ref(refs, prefix, fn, strlen(prefix), 0, cb_data);
+	return do_for_each_ref(refs, prefix, fn, strlen(prefix), the_repository, 0, cb_data);
 }
 
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
@@ -1518,10 +1522,10 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsig
 {
 	unsigned int flag = 0;
 
-	if (broken)
-		flag = DO_FOR_EACH_INCLUDE_BROKEN;
 	return do_for_each_ref(get_main_ref_store(the_repository),
-			       prefix, fn, 0, flag, cb_data);
+			       prefix, fn, 0,
+			       broken ? NULL : the_repository,
+			       flag, cb_data);
 }
 
 int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
@@ -1530,16 +1534,16 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
 {
 	unsigned int flag = 0;
 
-	if (broken)
-		flag = DO_FOR_EACH_INCLUDE_BROKEN;
-	return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
+	return do_for_each_ref(refs, prefix, fn, 0,
+			       broken ? NULL : the_repository,
+			       flag, cb_data);
 }
 
 int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data)
 {
 	return do_for_each_repo_ref(r, git_replace_ref_base, fn,
 				    strlen(git_replace_ref_base),
-				    DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
+				    NULL, 0, cb_data);
 }
 
 int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
@@ -1548,7 +1552,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
 	int ret;
 	strbuf_addf(&buf, "%srefs/", get_git_namespace());
 	ret = do_for_each_ref(get_main_ref_store(the_repository),
-			      buf.buf, fn, 0, 0, cb_data);
+			      buf.buf, fn, 0, the_repository, 0, cb_data);
 	strbuf_release(&buf);
 	return ret;
 }
@@ -1556,7 +1560,7 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
 int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	return do_for_each_ref(refs, "", fn, 0,
-			       DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
+			       NULL, 0, cb_data);
 }
 
 int for_each_rawref(each_ref_fn fn, void *cb_data)
@@ -2263,7 +2267,7 @@ int refs_verify_refname_available(struct ref_store *refs,
 	strbuf_addch(&dirname, '/');
 
 	iter = refs_ref_iterator_begin(refs, dirname.buf, 0,
-				       DO_FOR_EACH_INCLUDE_BROKEN);
+				       NULL, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		if (skip &&
 		    string_list_has_string(skip, iter->refname))
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 677b7e4cdd..cd145301d0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -744,12 +744,6 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)
 		    ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)
 			continue;
 
-		if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
-		    !ref_resolves_to_object(iter->iter0->refname,
-					    iter->iter0->oid,
-					    iter->iter0->flags))
-			continue;
-
 		iter->base.refname = iter->iter0->refname;
 		iter->base.oid = iter->iter0->oid;
 		iter->base.flags = iter->iter0->flags;
@@ -801,9 +795,6 @@ static struct ref_iterator *files_ref_iterator_begin(
 	struct ref_iterator *ref_iterator;
 	unsigned int required_flags = REF_STORE_READ;
 
-	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))
-		required_flags |= REF_STORE_ODB;
-
 	refs = files_downcast(ref_store, required_flags, "ref_iterator_begin");
 
 	/*
@@ -836,10 +827,13 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * references, and (if needed) do our own check for broken
 	 * ones in files_ref_iterator_advance(), after we have merged
 	 * the packed and loose references.
+	 *
+	 * Do this by not supplying any repo, regardless of whether a repo was
+	 * supplied to files_ref_iterator_begin().
 	 */
 	packed_iter = refs_ref_iterator_begin(
 			refs->packed_ref_store, prefix, 0,
-			DO_FOR_EACH_INCLUDE_BROKEN);
+			NULL, 0);
 
 	overlay_iter = overlay_ref_iterator_begin(loose_iter, packed_iter);
 
diff --git a/refs/iterator.c b/refs/iterator.c
index a89d132d4f..5af6554887 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -10,7 +10,23 @@
 
 int ref_iterator_advance(struct ref_iterator *ref_iterator)
 {
-	return ref_iterator->vtable->advance(ref_iterator);
+	int ok;
+
+	if (ref_iterator->repo && ref_iterator->repo != the_repository)
+		/*
+		 * NEEDSWORK: make ref_resolves_to_object() support
+		 * arbitrary repositories
+		 */
+		BUG("ref_iterator->repo must be NULL or the_repository");
+	while ((ok = ref_iterator->vtable->advance(ref_iterator)) == ITER_OK) {
+		if (ref_iterator->repo &&
+		    !ref_resolves_to_object(ref_iterator->refname,
+					    ref_iterator->oid,
+					    ref_iterator->flags))
+			continue;
+		return ITER_OK;
+	}
+	return ok;
 }
 
 int ref_iterator_peel(struct ref_iterator *ref_iterator,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f8aa97d799..f52d5488b8 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -863,11 +863,6 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
 		    ref_type(iter->base.refname) != REF_TYPE_PER_WORKTREE)
 			continue;
 
-		if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
-		    !ref_resolves_to_object(iter->base.refname, &iter->oid,
-					    iter->flags))
-			continue;
-
 		return ITER_OK;
 	}
 
@@ -922,8 +917,6 @@ static struct ref_iterator *packed_ref_iterator_begin(
 	struct ref_iterator *ref_iterator;
 	unsigned int required_flags = REF_STORE_READ;
 
-	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN))
-		required_flags |= REF_STORE_ODB;
 	refs = packed_downcast(ref_store, required_flags, "ref_iterator_begin");
 
 	/*
@@ -1136,8 +1129,7 @@ static int write_with_updates(struct packed_ref_store *refs,
 	 * list of refs is exhausted, set iter to NULL. When the list
 	 * of updates is exhausted, leave i set to updates->nr.
 	 */
-	iter = packed_ref_iterator_begin(&refs->base, "",
-					 DO_FOR_EACH_INCLUDE_BROKEN);
+	iter = refs_ref_iterator_begin(&refs->base, "", 0, NULL, 0);
 	if ((ok = ref_iterator_advance(iter)) != ITER_OK)
 		iter = NULL;
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3155708345..dc0ed65686 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -245,9 +245,6 @@ int refs_rename_ref_available(struct ref_store *refs,
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define SYMREF_MAXDEPTH 5
 
-/* Include broken references in a do_for_each_ref*() iteration: */
-#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
-
 /*
  * Reference iterators
  *
@@ -305,6 +302,12 @@ struct ref_iterator {
 	 */
 	unsigned int ordered : 1;
 
+	/*
+	 * See the documentation of refs_ref_iterator_begin() for more
+	 * information.
+	 */
+	struct repository *repo;
+
 	const char *refname;
 	const struct object_id *oid;
 	unsigned int flags;
@@ -349,16 +352,19 @@ int is_empty_ref_iterator(struct ref_iterator *ref_iterator);
  * Return an iterator that goes over each reference in `refs` for
  * which the refname begins with prefix. If trim is non-zero, then
  * trim that many characters off the beginning of each refname.
- * The output is ordered by refname. The following flags are supported:
+ * The output is ordered by refname.
+ *
+ * Pass NULL as repo to include broken references in the iteration, or non-NULL
+ * to skip references that do not resolve to an object in the given repo.
  *
- * DO_FOR_EACH_INCLUDE_BROKEN: include broken references in
- *         the iteration.
+ * The following flags are supported:
  *
  * DO_FOR_EACH_PER_WORKTREE_ONLY: only produce REF_TYPE_PER_WORKTREE refs.
  */
 struct ref_iterator *refs_ref_iterator_begin(
 		struct ref_store *refs,
-		const char *prefix, int trim, int flags);
+		const char *prefix, int trim, struct repository *repo,
+		int flags);
 
 /*
  * A callback function used to instruct merge_ref_iterator how to
@@ -446,8 +452,9 @@ void base_ref_iterator_free(struct ref_iterator *iter);
 /*
  * backend-specific implementation of ref_iterator_advance. For symrefs, the
  * function should set REF_ISSYMREF, and it should also dereference the symref
- * to provide the OID referent. If DO_FOR_EACH_INCLUDE_BROKEN is set, symrefs
- * with non-existent referents and refs pointing to non-existent object names
+ * to provide the OID referent. If a NULL repo was passed to the _begin()
+ * function that created this iterator, symrefs with non-existent referents and
+ * refs pointing to non-existent object names
  * should also be returned. If DO_FOR_EACH_PER_WORKTREE_ONLY, only
  * REF_TYPE_PER_WORKTREE refs should be returned.
  */
@@ -504,7 +511,7 @@ int do_for_each_repo_ref_iterator(struct repository *r,
  * where all reference backends will presumably store their
  * per-worktree refs.
  */
-#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02
+#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x01
 
 struct ref_store;
 

-- 
2.33.0.464.g1972c5931b-goog
Previous: Jonathan TanNext: Junio C Hamano
Message 2 of 65 in “No more adding submodule ODB as alternate”
  1. 0/9 No more adding submodule ODB as alternateJonathan Tan, Sep 21, 2021
  2. 1/9 refs: make _advance() check struct repo, not flagJonathan Tan, Sep 21, 2021
  3. Junio C HamanoSep 23, 2021
  4. Jonathan TanSep 24, 2021
  5. Junio C HamanoSep 24, 2021
  6. Jeff KingSep 24, 2021
  7. Jonathan TanSep 24, 2021
  8. 2/9 refs: add repo paramater to _iterator_peel()Jonathan Tan, Sep 21, 2021
  9. 3/9 refs iterator: support non-the_repository advanceJonathan Tan, Sep 21, 2021
  10. 4/9 refs: teach refs_for_each_ref() arbitrary reposJonathan Tan, Sep 21, 2021
  11. 5/9 merge-{ort,recursive}: remove add_submodule_odb()Jonathan Tan, Sep 21, 2021
  12. Elijah NewrenSep 28, 2021
  13. 6/9 object-file: only register submodule ODB if neededJonathan Tan, Sep 21, 2021
  14. 7/9 submodule: pass repo to check_has_commit()Jonathan Tan, Sep 21, 2021
  15. 8/9 refs: change refs_for_each_ref_in() to take repoJonathan Tan, Sep 21, 2021
  16. 9/9 submodule: trace adding submodule ODB as alternateJonathan Tan, Sep 21, 2021
  17. Junio C HamanoSep 23, 2021
  18. 0/9 No more adding submodule ODB as alternateJonathan Tan, Sep 28, 2021
  19. 1/9 refs: plumb repo param in begin-iterator functionsJonathan Tan, Sep 28, 2021
  20. Junio C HamanoSep 28, 2021
  21. 2/9 refs: teach arbitrary repo support to iteratorsJonathan Tan, Sep 28, 2021
  22. Junio C HamanoSep 28, 2021
  23. Jonathan TanSep 29, 2021
  24. 3/9 refs: peeling non-the_repository iterators is BUGJonathan Tan, Sep 28, 2021
  25. 4/9 refs: teach refs_for_each_ref() arbitrary reposJonathan Tan, Sep 28, 2021
  26. Junio C HamanoSep 28, 2021
  27. 5/9 merge-{ort,recursive}: remove add_submodule_odb()Jonathan Tan, Sep 28, 2021
  28. 6/9 object-file: only register submodule ODB if neededJonathan Tan, Sep 28, 2021
  29. 7/9 submodule: pass repo to check_has_commit()Jonathan Tan, Sep 28, 2021
  30. 8/9 refs: change refs_for_each_ref_in() to take repoJonathan Tan, Sep 28, 2021
  31. 9/9 submodule: trace adding submodule ODB as alternateJonathan Tan, Sep 28, 2021
  32. 0/7 No more adding submodule ODB as alternateJonathan Tan, Sep 29, 2021
  33. 1/7 refs: plumb repo into ref storesJonathan Tan, Sep 29, 2021
  34. fixup! refs: plumb repo into ref storesCarlo Marcelo Arenas Belón, Sep 30, 2021
  35. fixup! refs: plumb repo into ref storesGlen Choo, Oct 6, 2021
  36. Jonathan TanOct 8, 2021
  37. Jonathan TanOct 8, 2021
  38. Josh SteadmonOct 7, 2021
  39. Jonathan TanOct 8, 2021
  40. 2/7 refs: teach arbitrary repo support to iteratorsJonathan Tan, Sep 29, 2021
  41. Glen ChooOct 7, 2021
  42. Jonathan TanOct 8, 2021
  43. 3/7 refs: peeling non-the_repository iterators is BUGJonathan Tan, Sep 29, 2021
  44. 5/7 object-file: only register submodule ODB if neededJonathan Tan, Sep 29, 2021
  45. Josh SteadmonOct 7, 2021
  46. Jonathan TanOct 8, 2021
  47. 4/7 merge-{ort,recursive}: remove add_submodule_odb()Jonathan Tan, Sep 29, 2021
  48. Josh SteadmonOct 7, 2021
  49. Jonathan TanOct 8, 2021
  50. 6/7 submodule: pass repo to check_has_commit()Jonathan Tan, Sep 29, 2021
  51. 7/7 submodule: trace adding submodule ODB as alternateJonathan Tan, Sep 29, 2021
  52. Josh SteadmonOct 7, 2021
  53. Jonathan TanOct 8, 2021
  54. Josh SteadmonOct 7, 2021
  55. 0/7 No more adding submodule ODB as alternateJonathan Tan, Oct 8, 2021
  56. 1/7 refs: plumb repo into ref storesJonathan Tan, Oct 8, 2021
  57. 2/7 refs: teach arbitrary repo support to iteratorsJonathan Tan, Oct 8, 2021
  58. 3/7 refs: peeling non-the_repository iterators is BUGJonathan Tan, Oct 8, 2021
  59. 4/7 merge-{ort,recursive}: remove add_submodule_odb()Jonathan Tan, Oct 8, 2021
  60. 5/7 object-file: only register submodule ODB if neededJonathan Tan, Oct 8, 2021
  61. 6/7 submodule: pass repo to check_has_commit()Jonathan Tan, Oct 8, 2021
  62. 7/7 submodule: trace adding submodule ODB as alternateJonathan Tan, Oct 8, 2021
  63. Glen ChooOct 12, 2021
  64. Josh SteadmonOct 12, 2021
  65. Junio C HamanoOct 12, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.