Re: [PATCH] refs: fix loose-ref seeking across directory boundaries

[Patrick Steinhardt <ps@pks.im>]

On Thu, Oct 15, 2026 at 12:30:55PM +0000, Venkat Ragavan via GitGitGadget wrote:
> From: Venkat Ragavan S <mail2venkat21@gmail.com>
> 
> When seeking through loose refs, cache_ref_iterator_seek() compares
> each directory component without its trailing slash. This can confuse
> a directory with a similarly named ref that sorts before it.

This observes the status quo, but it doesn't give us any context _why_
we even did this. I'm sure there must've been a reason for this, but the
reader will have to figure it out by themselves.

> For example, with these refs:
> 
>     refs/tags/releases-z
>     refs/tags/releases/v1
>     refs/tags/releases/v2
> 
> Starting after refs/tags/releases/v1 can position the iterator at
> releases-z instead of descending into releases/. Iteration then
> returns v1 again, even though it should return only v2.

It would have been great to demonstrate how this is visible to the user.
E.g., can this be shown with a simple call to git-for-each-ref(1)?

> Include the trailing slash when comparing directory components so
> that the seek follows the correct directory.
> 
> Also handle two related positioning errors. If every entry in a
> directory precedes the cursor, stop searching that level rather than
> descending into its last entry. If a leaf sorts after the cursor,
> leave the iterator positioned to return it, even when the cursor has
> remaining path components.

Are thse separate bugs? If so, it would've made sense to split this
series into three separate commit so that we can fix the individual bugs
one by one, right?

> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index ffef01a597..a3bc2dc11b 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -532,7 +534,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>  				level->prefix_state = PREFIX_CONTAINS_DIR;
>  			} else {
>  				/* reduce the index so the leaf node is iterated over */
> -				if (cmp <= 0 && !slash)
> +				if (cmp < 0 || (cmp == 0 && !slash))
>  					level->index = idx - 1;
>  				/*
>  				 * while the seek path may not be exhausted, our

Tiny nit: we usually don't explicitly compare for equality (or
inequality) with 0, but rather write `cmp` or `!cmp`.

> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 7f060d97bf..826fc1b9f2 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -746,6 +746,61 @@ test_expect_success 'start after used with a pattern' '
>  	test_cmp expect actual
>  '
>  
> +for storage in loose packed mixed
> +do
> +	test_expect_success "start after respects directory boundaries ($storage)" '
> +		test_when_finished "rm -rf repo" &&
> +		git init repo &&
> +		(
> +			cd repo &&
> +			test_commit --no-tag base &&
> +			git update-ref refs/tags/packed HEAD &&
> +			if test "$storage" = mixed
> +			then
> +				git pack-refs --all || return 1
> +			fi &&

Okay. These are a bit funny in the context of the reftable backend, but
I agree it makes sense to have this anyway. Doesn't hurt much to have
extra coverage.

Thanks!

Patrick