Re: [PATCH v2 2/9] refs: teach arbitrary repo...

Re: [PATCH v2 2/9] refs: teach arbitrary repo support to iterators

From: Junio C Hamano <hidden>
Date: 2021-09-28 22:35:39

Jonathan Tan [off-list ref] writes:

quoted hunk

Note that should_pack_ref() is called when writing refs, which is only
supported for the_repository, hence the_repository is hardcoded there.

Signed-off-by: Jonathan Tan <redacted>
---
 refs.c                | 3 ++-
 refs/files-backend.c  | 5 ++++-
 refs/packed-backend.c | 6 ++++--
 refs/refs-internal.h  | 1 +
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 6f7b3447a7..5163e064ae 100644
--- a/refs.c
+++ b/refs.c

@@ -255,12 +255,13 @@ int refname_is_safe(const char *refname)
  * does not exist, emit a warning and return false.
  */
 int ref_resolves_to_object(const char *refname,
+			   struct repository *repo,
 			   const struct object_id *oid,
 			   unsigned int flags)
 {
 	if (flags & REF_ISBROKEN)
 		return 0;
-	if (!has_object_file(oid)) {
+	if (!repo_has_object_file(repo, oid)) {
 		error(_("%s does not point to a valid object!"), refname);
 		return 0;
 	}

OK.

quoted hunk

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f0cbea41c9..4d883d9a89 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -730,6 +730,7 @@ struct files_ref_iterator {
 	struct ref_iterator base;
 
 	struct ref_iterator *iter0;
+	struct repository *repo;
 	unsigned int flags;
 };

quoted hunk

@@ -776,6 +776,7 @@ struct packed_ref_iterator {
 	struct object_id oid, peeled;
 	struct strbuf refname_buf;
 
+	struct repository *repo;
 	unsigned int flags;
 };

The two steps so far seems to give the necessary information to code
paths that want them, so it is not wrong per-se, but this makes me
wonder a few things.

 - There may be multiple ref backends and iterators corresponding to
   them.  Is it reasonable to assume that there are backends that do
   not need "repo"?  Otherwise, shouldn't this be added to the base
   class "struct ref_iterator base"?

 - The iterator_begin() and other functions have been taught to take
   the repository in addition to the ref_store in the previous step,
   but

   . Doesn't iterator iterate over a single ref_store?  Shouldn't it
     have a pointer to the ref_store it is iterating over?

   . Doesn't a ref_store belong to a single repository?  Shouldn't
     it have a pointer to the repository it is part of?

   If the answers to both are 'yes', then we wouldn't need to add a
   repository pointer as a new parameter to functions that already
   took a ref store.

In other words, I am wondering if the right pieces of information
are stored in the right structure.

Thanks.

Re: [PATCH v2 2/9] refs: teach arbitrary repo support to iterators

From: Jonathan Tan <hidden>
Date: 2021-09-29 17:04:20

The two steps so far seems to give the necessary information to code
paths that want them, so it is not wrong per-se, but this makes me
wonder a few things.

 - There may be multiple ref backends and iterators corresponding to
   them.  Is it reasonable to assume that there are backends that do
   not need "repo"?  Otherwise, shouldn't this be added to the base
   class "struct ref_iterator base"?

All backends need repos, but not all iterators need backends - there is
a merge_ref_iterator and a prefix_ref_iterator, for example.

 - The iterator_begin() and other functions have been taught to take
   the repository in addition to the ref_store in the previous step,
   but

   . Doesn't iterator iterate over a single ref_store?  Shouldn't it
     have a pointer to the ref_store it is iterating over?

No - as above, merge_ref_iterator, for example, does not iterate over a
ref_store but combines the results of 2 other iterators.

   . Doesn't a ref_store belong to a single repository?  Shouldn't
     it have a pointer to the repository it is part of?

   If the answers to both are 'yes', then we wouldn't need to add a
   repository pointer as a new parameter to functions that already
   took a ref store.

In other words, I am wondering if the right pieces of information
are stored in the right structure.

Thanks.

A ref_store does belong to a single repository. The reason why it
doesn't have a pointer to that repository is probably because struct
ref_store (00eebe351c ("refs: create a base class "ref_store" for
files_ref_store", 2016-09-09)) predates struct repository (359efeffc1
("repository: introduce the repository object", 2017-06-23)). I've been
avoiding introducing a pointer to the repository in struct ref_store to
avoid unnecessary coupling, but it is looking more and more necessary,
as you mention in your reply [1] to another patch about how this would
eliminate certain other "user" codepaths needing to know about the repo.
I'll take a look at introducing a pointer to the repo in struct
ref_store and report back with my findings.

[1] https://lore.kernel.org/git/xmqqh7e4iacw.fsf@gitster.g/

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help