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

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

[Venkat Ragavan via GitGitGadget]

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.

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.

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.

Add regression tests for shared-prefix names, missing nested markers,
and markers beyond the end of the ref listing, using loose, packed,
and mixed refs.

Signed-off-by: Venkat Ragavan S <mail2venkat21@gmail.com>
---
    refs: fix loose-ref seeking across directory boundaries

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2258%2Fvenkatr21%2Fvenkatr21%2Ffix-loose-ref-seek-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2258/venkatr21/venkatr21/fix-loose-ref-seek-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2258

 refs/ref-cache.c               |  6 ++--
 t/t6302-for-each-ref-filter.sh | 55 ++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)

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
@@ -506,7 +506,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
 			sort_ref_dir(dir);
 
 			slash = strchr(slash, '/');
-			len = slash ? (size_t)(slash - refname) : strlen(refname);
+			len = slash ? (size_t)(slash - refname + 1) : strlen(refname);
 
 			for (idx = 0; idx < dir->nr; idx++) {
 				cmp = strncmp(refname, dir->entries[idx]->name, len);
@@ -520,6 +520,8 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
 				slash = slash + 1;
 
 			level->index = idx;
+			if (cmp > 0)
+				break;
 			if (dir->entries[idx]->flag & REF_DIR) {
 				/* push down a level */
 				dir = get_ref_dir(dir->entries[idx]);
@@ -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
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 &&
+			git update-ref --stdin <<-\EOF &&
+			create refs/tags/a-leaf HEAD
+			create refs/tags/a/nested HEAD
+			create refs/tags/a0 HEAD
+			create refs/tags/b HEAD
+			create refs/tags/releases-z HEAD
+			create refs/tags/releases/v1 HEAD
+			create refs/tags/releases/v2 HEAD
+			EOF
+			if test "$storage" = packed
+			then
+				git pack-refs --all || return 1
+			fi &&
+			echo refs/tags/releases/v2 >expect &&
+			git for-each-ref --no-sort --format="%(refname)" \
+				--start-after=refs/tags/releases/v1 >actual &&
+			test_cmp expect actual &&
+			cat >expect <<-\EOF &&
+			refs/tags/a0
+			refs/tags/b
+			refs/tags/packed
+			refs/tags/releases-z
+			refs/tags/releases/v1
+			refs/tags/releases/v2
+			EOF
+			git for-each-ref --no-sort --format="%(refname)" \
+				--start-after=refs/tags/a/next >actual &&
+			test_cmp expect actual &&
+			tail -n 5 expect >expect-later &&
+			git for-each-ref --no-sort --format="%(refname)" \
+				--start-after=refs/tags/aa/missing >actual &&
+			test_cmp expect-later actual &&
+			for marker in refs/tags/zzzz refs/zzzz zzzz
+			do
+				git for-each-ref --no-sort --format="%(refname)" \
+					--start-after="$marker" >actual &&
+				test_must_be_empty actual || return 1
+			done
+		)
+	'
+done
+
 test_expect_success 'start after used with custom sort order' '
 	cat >expect <<-\EOF &&
 	fatal: cannot use --start-after with custom sort options

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
-- 
gitgitgadget

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

[Harald Nordgren]

The general idea looks sound.

> +			if test "$storage" = mixed
> +			then
> +				git pack-refs --all || return 1
> +			fi &&

I don't see any reason why a test would need an if block, we know what data we
expect, no?


Harald

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

[D. Ben Knoble]

On Thu, Oct 15, 2026 at 10:20 AM Harald Nordgren
<haraldnordgren@gmail.com> wrote:
[snip]
> > +                     if test "$storage" = mixed
> > +                     then
> > +                             git pack-refs --all || return 1
> > +                     fi &&
>
> I don't see any reason why a test would need an if block, we know what data we
> expect, no?

I didn't examine too closely, but this test is wrapped in a loop over
the storage variable that appears to determine whether and when we
pack-refs, so I think the conditional makes sense for that?

-- 
D. Ben Knoble

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

[D. Ben Knoble]

[resending with corrected plain-text ':D]

> On Thu, 15 Oct 2026 at 10:38 PM, D. Ben Knoble <ben.knoble@gmail.com> wrote:
>>
>> On Thu, Oct 15, 2026 at 10:20 AM Harald Nordgren
>> <haraldnordgren@gmail.com> wrote:
>> [snip]
>> > > +                     if test "$storage" = mixed
>> > > +                     then
>> > > +                             git pack-refs --all || return 1
>> > > +                     fi &&
>> >
>> > I don't see any reason why a test would need an if block, we know what data we
>> > expect, no?
>>
>> I didn't examine too closely, but this test is wrapped in a loop over
>> the storage variable that appears to determine whether and when we
>> pack-refs, so I think the conditional makes sense for that?

On Thu, Oct 15, 2026 at 1:13 PM Venkat Ragavan <mail2venkat21@gmail.com> wrote:
>
> Yes it does, I interpreted Harald’s suggestion was to write separate tests instead of looping over a variable outside the test block, either ways the logic of the test is going to remain the same.

[we bottom-post here]

I don’t mind the loop, personally; it might be worth looking around
for other tests and see how they approach it.

You can also extract shared sections into functions if that makes
writing separate tests easier.

-- 
D. Ben Knoble

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

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

> The general idea looks sound.
>
>> +			if test "$storage" = mixed
>> +			then
>> +				git pack-refs --all || return 1
>> +			fi &&
>
> I don't see any reason why a test would need an if block, we know what data we
> expect, no?

This is three tests implemented as a single test with a for-loop
around it.  The variable instructs the single test_expect_success
to behave slightly differently depending on its setting.

+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 &&
+			git update-ref --stdin <<-\EOF &&
+			create refs/tags/a-leaf HEAD
+			...
+			EOF
+			if test "$storage" = packed
+			then
+				git pack-refs --all || return 1
+			fi &&
+			... real test in the repository begins ...
+		)
+	'
+done

It creates a tag "packed", and packs when storage is "mixed".  Then
we create many other refs, and packs the whole refs when storage is
"packed".  So during these three runs of the for-loop, the preparation
part quoted above behaves this way:

    storage=loose	no 'git pack-refs' run and every ref is loose
    storage=packed	the later 'git pack-refs' packs everything
    storage=mixed	'packed' tag is packed, everything else is loose

and the test checks what happens in this repository with refs packed
in three different ways.

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

[Harald Nordgren]

> > The general idea looks sound.
> >
> >> +                    if test "$storage" = mixed
> >> +                    then
> >> +                            git pack-refs --all || return 1
> >> +                    fi &&
> >
> > I don't see any reason why a test would need an if block, we know what data we
> > expect, no?
>
> This is three tests implemented as a single test with a for-loop
> around it.  The variable instructs the single test_expect_success
> to behave slightly differently depending on its setting.

I realize now that I didn't read the code properly. I didn't expect to
see a loop encapsulating a call to test_expect_success. That explains
the if case as well.


Harald

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

[Venkat Ragavan]

[Resending as plain text after the mailing list rejected the HTML version.]

> I realize now that I didn't read the code properly. I didn't expect to
> see a loop encapsulating a call to test_expect_success. That explains
> the if case as well.

Thanks for following up, and thanks Junio for explaining the setup. Since
the loop intentionally exercises loose, packed, and mixed ref layouts, I'll
leave the test unchanged.


On Fri, Oct 16, 2026 at 8:47 AM Venkat Ragavan <mail2venkat21@gmail.com> wrote:
>
> > I realize now that I didn't read the code properly. I didn't expect to
> > see a loop encapsulating a call to test_expect_success. That explains
> > the if case as well.
>
> Thanks for following up, and thanks Junio for explaining the setup. Since
> the loop intentionally exercises loose, packed, and mixed ref layouts, I’ll
> leave the test unchanged.
>
> On Fri, Oct 16, 2026 at 2:47 AM Harald Nordgren <haraldnordgren@gmail.com> wrote:
>>
>> > > The general idea looks sound.
>> > >
>> > >> +                    if test "$storage" = mixed
>> > >> +                    then
>> > >> +                            git pack-refs --all || return 1
>> > >> +                    fi &&
>> > >
>> > > I don't see any reason why a test would need an if block, we know what data we
>> > > expect, no?
>> >
>> > This is three tests implemented as a single test with a for-loop
>> > around it.  The variable instructs the single test_expect_success
>> > to behave slightly differently depending on its setting.
>>
>> I realize now that I didn't read the code properly. I didn't expect to
>> see a loop encapsulating a call to test_expect_success. That explains
>> the if case as well.
>>
>>
>> Harald

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

[Junio C Hamano]

"Venkat Ragavan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.
> ...
> Add regression tests for shared-prefix names, missing nested markers,
> and markers beyond the end of the ref listing, using loose, packed,
> and mixed refs.
>
> Signed-off-by: Venkat Ragavan S <mail2venkat21@gmail.com>
> ---
>     refs: fix loose-ref seeking across directory boundaries

I overlooked this topic, but re-reading the discussion, aside from a
test that confused a reader, it seems the topic was already ready to
go?

Any objections to the idea of making the topic ready for 'next'?

Thanks.

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

[Patrick Steinhardt]

On Tue, Oct 27, 2026 at 03:22:48PM -0700, Junio C Hamano wrote:
> "Venkat Ragavan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > 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.
> > ...
> > Add regression tests for shared-prefix names, missing nested markers,
> > and markers beyond the end of the ref listing, using loose, packed,
> > and mixed refs.
> >
> > Signed-off-by: Venkat Ragavan S <mail2venkat21@gmail.com>
> > ---
> >     refs: fix loose-ref seeking across directory boundaries
> 
> I overlooked this topic, but re-reading the discussion, aside from a
> test that confused a reader, it seems the topic was already ready to
> go?
> 
> Any objections to the idea of making the topic ready for 'next'?

Sorry, I lost track of this patch series, but I've reviewed it now.
Overall this looks good, but I wonder whether this really should be
split up into three patches because as far as I understand, this single
patch solves three separate bugs at once.

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Oct 27, 2026 at 03:22:48PM -0700, Junio C Hamano wrote:
>> "Venkat Ragavan via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>> > 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.
>> > ...
>> > Add regression tests for shared-prefix names, missing nested markers,
>> > and markers beyond the end of the ref listing, using loose, packed,
>> > and mixed refs.
>> >
>> > Signed-off-by: Venkat Ragavan S <mail2venkat21@gmail.com>
>> > ---
>> >     refs: fix loose-ref seeking across directory boundaries
>> 
>> I overlooked this topic, but re-reading the discussion, aside from a
>> test that confused a reader, it seems the topic was already ready to
>> go?
>> 
>> Any objections to the idea of making the topic ready for 'next'?
>
> Sorry, I lost track of this patch series, but I've reviewed it now.
> Overall this looks good, but I wonder whether this really should be
> split up into three patches because as far as I understand, this single
> patch solves three separate bugs at once.
>
> Patrick

Thanks.  Let's hear from the author, then.

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

[Patrick Steinhardt]

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