From: Justin Tobler <jltobler@gmail.com>
Date: Tue, 07 Oct 2025 20:19:58 GMT
Subject: Re: [PATCH 02/13] refs: introduce `.ref` field for the base iterator
Message-ID: <q6ti6bevxr4kbsi7pe7slwmvyqhc2sslma3tk3xshohnqadtuv@canofgr644do>
In-Reply-To: <20251007-b4-pks-ref-filter-skip-parsing-objects-v1-2-916cc7c6886b@pks.im>

On 25/10/07 12:58PM, Patrick Steinhardt wrote:
> The base iterator has a couple of fields that tracks the name, target,
> object ID and flags for the current reference. Due do this design we

s/Due do/Due to/

> have to create a new `struct reference` whenever we want to hand over
> that reference to the callback function, which is tedious and not very
> efficient.
> 
> Convert the structure to instead contain a `stuct reference` as member.
> This member is expected to be populated by the implementations of the
> iterator and is handed over to the callback directly.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 0ddcf22aed..d34fbe55d6 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -962,26 +962,23 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)
>  
>  	while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {
>  		if (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&
> -		    parse_worktree_ref(iter->iter0->refname, NULL, NULL,
> +		    parse_worktree_ref(iter->iter0->ref.name, NULL, NULL,
>  				       NULL) != REF_WORKTREE_CURRENT)
>  			continue;
>  
>  		if ((iter->flags & DO_FOR_EACH_OMIT_DANGLING_SYMREFS) &&
> -		    (iter->iter0->flags & REF_ISSYMREF) &&
> -		    (iter->iter0->flags & REF_ISBROKEN))
> +		    (iter->iter0->ref.flags & REF_ISSYMREF) &&
> +		    (iter->iter0->ref.flags & REF_ISBROKEN))
>  			continue;
>  
>  		if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
> -		    !ref_resolves_to_object(iter->iter0->refname,
> +		    !ref_resolves_to_object(iter->iter0->ref.name,
>  					    iter->repo,
> -					    iter->iter0->oid,
> -					    iter->iter0->flags))
> +					    iter->iter0->ref.oid,
> +					    iter->iter0->ref.flags))
>  			continue;
>  
> -		iter->base.refname = iter->iter0->refname;
> -		iter->base.oid = iter->iter0->oid;
> -		iter->base.flags = iter->iter0->flags;
> -		iter->base.referent = iter->iter0->referent;
> +		iter->base.ref = iter->iter0->ref;

Ok, so here we already have a `struct reference` setup and thus we
directly propagate to the base when advacing the iterator. Makes sense.

>  
>  		return ITER_OK;
>  	}
> @@ -1368,30 +1365,29 @@ static void prune_refs(struct files_ref_store *refs, struct ref_to_prune **refs_
>   * Return true if the specified reference should be packed.
>   */
>  static int should_pack_ref(struct files_ref_store *refs,
> -			   const char *refname,
> -			   const struct object_id *oid, unsigned int ref_flags,
> +			   const struct reference *ref,
>  			   struct pack_refs_opts *opts)

This hunk is simplifies the arguments required by should_pack_ref() by
using `struct reference`. The change seems sensible, it might be worth
mentioning in the commit message though.

>  {
>  	struct string_list_item *item;
>  
>  	/* Do not pack per-worktree refs: */
> -	if (parse_worktree_ref(refname, NULL, NULL, NULL) !=
> +	if (parse_worktree_ref(ref->name, NULL, NULL, NULL) !=
>  	    REF_WORKTREE_SHARED)
>  		return 0;
>  
>  	/* Do not pack symbolic refs: */
> -	if (ref_flags & REF_ISSYMREF)
> +	if (ref->flags & REF_ISSYMREF)
>  		return 0;
>  
>  	/* Do not pack broken refs: */
> -	if (!ref_resolves_to_object(refname, refs->base.repo, oid, ref_flags))
> +	if (!ref_resolves_to_object(ref->name, refs->base.repo, ref->oid, ref->flags))
>  		return 0;
>  
> -	if (ref_excluded(opts->exclusions, refname))
> +	if (ref_excluded(opts->exclusions, ref->name))
>  		return 0;
>  
>  	for_each_string_list_item(item, opts->includes)
> -		if (!wildmatch(item->string, refname, 0))
> +		if (!wildmatch(item->string, ref->name, 0))
>  			return 1;
>  
>  	return 0;
[snip]
> @@ -476,14 +468,7 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
>  
>  	current_ref_iter = iter;
>  	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
> -		struct reference ref = {
> -			.name = iter->refname,
> -			.target = iter->referent,
> -			.oid = iter->oid,
> -			.flags = iter->flags,
> -		};
> -
> -		retval = fn(&ref, cb_data);
> +		retval = fn(&iter->ref, cb_data);

Now we propagate `struct reference` directly when invoking the for each
callback. Makes sense.

>  		if (retval)
>  			goto out;
>  	}

-Justin