From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 12 Jun 2026 11:48:11 GMT
Subject: Re: [PATCH v3] ref-filter: restore prefix-scoped iteration
Message-ID: <aivx-7VOKE_TC50R@pks.im>
In-Reply-To: <20260610-fix-git-branch-regression-v3-1-6fd48fad7a53@gmail.com>

On Wed, Jun 10, 2026 at 05:29:49AM -0700, Tamir Duberstein wrote:
> dabecb9db2 (for-each-ref: introduce a '--start-after' option,
> 2025-07-15) changed branch, remote-tracking branch, and tag enumeration
> from constructing an iterator with the namespace prefix to constructing
> an unscoped iterator and seeking to the prefix.
> 
> The files backend constructs its loose-ref iterator with cache priming
> enabled. cache_ref_iterator_begin() immediately applies the construction
> prefix through cache_ref_iterator_set_prefix(), reading loose refs
> beneath it before packed refs are opened. An empty prefix therefore
> reads every loose ref, and a later seek cannot undo that I/O.
> 
> For these single-kind filters, construct the iterator with the namespace
> prefix when start_after is not set. Keep the existing unscoped
> construction for start_after, whose seek position may differ from the
> namespace prefix.
> 
> With 10,000 unrelated loose refs, the p6300 tests improve as follows:
> 
>                          before   after
>   branch                  2.74 s   0.11 s
>   branch --remotes        2.81 s   0.12 s
>   tag                     3.01 s   0.11 s
> 
> Link: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/
> Link: https://lore.kernel.org/git/xmqqikjq7s16.fsf@gitster.g/
> Link: https://lore.kernel.org/r/CAOLa=ZRHKNNymXGk31YgECjUmF9nZ8GsPUdQb7aKBH5DKMz7=w@mail.gmail.com

I honestly have no idea what you want to say with these links, as they
seem to just link to random reviews mails when the above mentioned
commit was reviewed. In general, we typically try to embed references
like this into the explanation, like:

    In [1], we discussed... and this is relevant because of ...

    [1]: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/

Just dropping the links as-is without much of an explanation isn't
helpful.

> diff --git a/ref-filter.c b/ref-filter.c
> index 1da4c0e60d..9b04e3af85 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3316,15 +3316,14 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for
>  
>  	if (prefix) {
>  		struct ref_iterator *iter;
> +		struct ref_store *store = get_main_ref_store(the_repository);
>  
> -		iter = refs_ref_iterator_begin(get_main_ref_store(the_repository),
> -					       "", NULL, 0, 0);
> -
> -		if (filter->start_after)
> +		if (filter->start_after) {
> +			iter = refs_ref_iterator_begin(store, "", NULL, 0, 0);
>  			ret = start_ref_iterator_after(iter, filter->start_after);
> -		else
> -			ret = ref_iterator_seek(iter, prefix,
> -						REF_ITERATOR_SEEK_SET_PREFIX);
> +		} else {
> +			iter = refs_ref_iterator_begin(store, prefix, NULL, 0, 0);
> +		}
>  
>  		if (!ret)
>  			ret = do_for_each_ref_iterator(iter, fn, cb_data);

The patch itself seems sensible to me.

> diff --git a/t/perf/p6300-for-each-ref.sh b/t/perf/p6300-for-each-ref.sh
> index fa7289c752..ed9c1c6a19 100755
> --- a/t/perf/p6300-for-each-ref.sh
> +++ b/t/perf/p6300-for-each-ref.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -test_description='performance of for-each-ref'
> +test_description='performance of ref-filter users'
>  . ./perf-lib.sh
>  
>  test_perf_fresh_repo
> @@ -84,4 +84,41 @@ test_expect_success 'pack refs' '
>  '
>  run_tests "packed"
>  
> +test_expect_success REFFILES 'setup many unrelated loose refs' '
> +	git init scoped &&
> +	test_commit -C scoped --no-tag base &&
> +	test_seq $ref_count_per_type |
> +		sed "s,.*,update refs/custom/unrelated_& HEAD," |
> +		git -C scoped update-ref --stdin &&
> +	git -C scoped update-ref refs/remotes/origin/main HEAD &&
> +	git -C scoped update-ref refs/tags/only HEAD
> +'

I've already called this out before on other patches, but the REFFILES
prerequisite just doesn't make any sense here as this test logic is
generic.

Patrick