refs: fix loose-ref seeking across directory boundaries
PATCH 1/1 refs: fix loose-ref seeking across directory boundaries
From: Venkat Ragavan S <[email protected]>
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.
---
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(-)
refs/ref-cache.c +4 -2
@@ -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
t/t6302-for-each-ref-filter.sh +55 -0
61 lines not on this page · raw message
base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
Signature · 1 line
gitgitgadget
The general idea looks sound.
t/t6302-for-each-ref-filter.sh test_expect_success 'start after used with a pattern' ' · line 761
Quoted 4 lines
t/t6302-for-each-ref-filter.sh +4
-0+ 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
On Thu, Oct 15, 2026 at 10:20 AM Harald Nordgren
<[email protected]> wrote:
[snip]
Quoted 7 lines
t/t6302-for-each-ref-filter.sh test_expect_success 'start after used with a pattern' ' · line 761
Quoted 4 lines
t/t6302-for-each-ref-filter.sh +4
-0+ 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?
Signature · 1 line
D. Ben Knoble
[resending with corrected plain-text ':D]
Quoted 16 lines · D. Ben Knoble
Quoted 15 lines
On Thu, Oct 15, 2026 at 10:20 AM Harald Nordgren
<[email protected]> wrote:
[snip]Quoted 7 lines
t/t6302-for-each-ref-filter.sh test_expect_success 'start after used with a pattern' ' · line 761
Quoted 4 lines
t/t6302-for-each-ref-filter.sh +4
-0+ 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?
Quoted 2 lines · Venkat Ragavan
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.
Signature · 1 line
D. Ben Knoble
Quoted 9 lines · Harald Nordgren
The general idea looks sound.
t/t6302-for-each-ref-filter.sh test_expect_success 'start after used with a pattern' ' · line 761
Quoted 4 lines
t/t6302-for-each-ref-filter.sh +4
-0+ 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.
Quoted 7 lines
Quoted 7 lines
The general idea looks sound.
Quoted 4 lines
t/t6302-for-each-ref-filter.sh +4
-0+ if test "$storage" = mixed + then + git pack-refs --all || return 1 + fi &&
Quoted 6 lines
Quoted 2 lines
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_successto 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
[Resending as plain text after the mailing list rejected the HTML version.]
Quoted 3 lines
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.
Quoted 31 lines · Venkat Ragavan
Quoted 4 lines
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.
Quoted 21 lines · Harald Nordgren
Quoted 8 lines
Quoted 8 lines
The general idea looks sound.
Quoted 4 lines
t/t6302-for-each-ref-filter.sh +4
-0+ if test "$storage" = mixed + then + git pack-refs --all || return 1 + fi &&Quoted 6 lines
Quoted 2 lines
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_successto 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
Quoted 10 lines · Venkat Ragavan via GitGitGadget
From: Venkat Ragavan S <[email protected]>
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.
Quoted 3 lines
Signed-off-by Venkat Ragavan S <[email protected]>---
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.
Quoted 5 lines · Venkat Ragavan via GitGitGadget
From: Venkat Ragavan S <[email protected]>
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.
Quoted 9 lines
For example, with these refs:
refs/tags/releases-z
refs/tags/releases/v1
refs/tags/releases/v2Starting 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)?
Quoted 8 lines
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?
refs/ref-cache.c static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator, · line 540
Quoted 13 lines
refs/ref-cache.c +1
-1@@ -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.
Quoted 11 lines
t/t6302-for-each-ref-filter.sh +3
-0@@ -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)" '
t/t6302-for-each-ref-filter.sh test_expect_success 'start after used with a pattern' ' · line 761
Quoted 10 lines
t/t6302-for-each-ref-filter.sh +10
-0+ 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
Quoted 16 lines · Junio C Hamano
Quoted 10 lines · Venkat Ragavan via GitGitGadget
From: Venkat Ragavan S <[email protected]>
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.Quoted 3 lines
Signed-off-by Venkat Ragavan S <[email protected]>---
refs: fix loose-ref seeking across directory boundaries
Quoted 5 lines
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
Quoted 21 lines · Patrick Steinhardt
Quoted 20 lines · Junio C Hamano
Quoted 10 lines · Venkat Ragavan via GitGitGadget
From: Venkat Ragavan S <[email protected]>
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.Quoted 3 lines
Signed-off-by Venkat Ragavan S <[email protected]>---
refs: fix loose-ref seeking across directory boundariesI 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?
Quoted 8 lines
Quoted 1 line
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.