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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 28, 2021, 22:35 UTC
Message-ID: <xmqqlf3gib0p.fsf@gitster.g>
In-Reply-To: <ec153eff7b0d5ca3188ec6f43bc40c38609f6a80.1632859148.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 30 quoted lines

> 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 <jonathantanmy@google.com>
> ---
>  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.

Show 11 quoted lines

> 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;
>  };

Show 7 quoted lines

> @@ -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.