Handle trailing slash in ref list prefix filtering #1936

moroten · 2025-04-06T21:31:04Z

Previously, refs/heads/foo/bar would be listed when running repo.references()?.prefixed("refs/heads/b"). The code identified that the last component was not a directory and started to match it as a filename prefix for all files in all recursive directories, effectively matching refs/heads/**/b*.

This commit fixes that bug but also allows to use a trailing / in the prefix, allowing to filter for refs/heads/foo/ and not get refs/heads/foo-bar as a result.

Fixes #1934.

Byron

Thanks a lot for tackling this!

I think what would be needed here is a test that shows the problem this is solving, i.e. the iteration failing to properly deduplicate results from packed-refs and loose refs when a prefix is used.

Such an example should include deep nesting as well.

Prefixed search is necessary to help to quickly bypass a possibly large list of results, and it's essential to do quick listing (without reftable) during fetches in repositories where a lot of references exist.

Edit: I also left a comment here which may be relevant for this PR as well.

gix-ref/src/store/file/loose/iter.rs

EliahKagan

The current test failure is due to the need to commit a regenerated gix-ref/tests/fixtures/generated-archives/make_ref_repository.tar. This is needed because the script that generates it, gix-ref/tests/fixtures/make_ref_repository.sh, is modified here. To verify that this was the only cause of failure so far, I've regenerated that archive in ek/run-ci/regenerate-archives-for-pr-1936 (b6ab162).

If you like, you can fast-forward this PR to include that commit, though it might not be useful to do so if the further changes for #1936 (review) will include modifying make_ref_repository.sh again.

git fetch --all
git cherry-pick upstream/ek/run-ci/regenerate-archives-for-pr-1936

Please disregard this if it is not convenient or if make_ref_repository.sh shall be changed again.

moroten · 2025-04-07T19:26:58Z

Thank you @EliahKagan. It turned out that I missed the binary files when using git add -p.

moroten · 2025-04-07T19:28:26Z

I've rewritten the PR to use a BStr as prefix instead of Path. This allows for trailing slash.

Is there any use case where Windows backslash need to be translated to forward slash that I may have missed?

Previously, `refs/heads/foo/bar` would be listed when running `repo.references()?.prefixed("refs/heads/b")`. The code identified that the last component was not a directory and started to match it as a filename prefix for all files in all recursive directories, effectively matching `refs/heads/**/b*`. This commit fixes that bug but also allows to use a trailing `/` in the prefix, allowing to filter for `refs/heads/foo/` and not get `refs/heads/foo-bar` as a result. Fixes GitoxideLabs#1934.

Byron

Thanks a lot for taking this PR further, it looks great.

I'd have to take it into another round to improve a couple of things that are mentioned below. Most importantly, a test is needed to show that \\ isn't special anymore. Unfortunately, there isn't a breaking that that would have tested for it being treated specifically either, so it might be that the claim was never true.

From there I can promise no more rounds, I'd take it from there with an in-IDE review, fixing whatever small things that come up myself.

If you feel like it's enough, please let me know and I will happily pick it up as soon as I can to get it merged.

Byron · 2025-04-08T01:30:36Z

gix-ref/src/store/file/overlay_iter.rs

-    pub fn prefixed(&self, prefix: &Path) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads/" or
+    /// "refs/heads/feature-".
+    pub fn prefixed(&self, prefix: Cow<'_, BStr>) -> std::io::Result<LooseThenPacked<'_, '_>> {


I think it's fair to have this breaking change, as this prefix is essentially a RepositoryPath, i.e. a slash-separated relative path.
Something I think that should come with it is tests that show that behaviour.

The tests are now more explicitly named to test that.

Byron · 2025-04-08T01:32:28Z

gix-ref/src/store/file/overlay_iter.rs

-    pub fn prefixed(&self, prefix: &Path) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads/" or
+    /// "refs/heads/feature-".
+    pub fn prefixed(&self, prefix: Cow<'_, BStr>) -> std::io::Result<LooseThenPacked<'_, '_>> {


This should be &BStr if i twas &Path before.

Byron · 2025-04-08T01:33:24Z

gix-ref/src/store/file/loose/iter.rs

@@ -93,7 +87,7 @@ impl file::Store {
    /// Return an iterator over all loose references that start with the given `prefix`.
    ///
    /// Otherwise it's similar to [`loose_iter()`][file::Store::loose_iter()].
-    pub fn loose_iter_prefixed(&self, prefix: &Path) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    pub fn loose_iter_prefixed(&self, prefix: Cow<'_, BStr>) -> std::io::Result<LooseThenPacked<'_, '_>> {


This should be &BStr if it was &Path before.
It could also be impl Into<…> if that makes it more convenient to call, even for the test-suite.

I put impl Into<Cow<...>> in most places. Not sure if some should just be &BStr or not.

Byron · 2025-04-08T01:40:38Z

gix-ref/tests/fixtures/make_packed_ref_repository_for_overlay.sh


 cp .git/refs/heads/main .git/refs/remotes/origin/

 echo "ref: refs/remotes/origin/main" > .git/refs/remotes/origin/HEAD
+echo "ref: refs/heads/main" > .git/refs/heads-packed
+echo "ref: refs/heads/main" > .git/refs/heads/sub/dir/packed


I think these names should be changed so packed isn't included. This is because symbolic refs are never packed.

I just wanted to make sure that both the loose and packed refs are handled correctly and not accidentally testing the wrong thing. Changed the names to refs/feature-suffix and refs/feature/sub/dir/algo to make the test more explicit.

Byron · 2025-04-13T16:12:05Z

Thanks again for tackling this!

I am closing this PR as I wasn't able to push my changes into it. Instead, I opened #1954.

moroten mentioned this pull request Apr 6, 2025

refs.references().prefixed() lets too many refs through #1934

Closed

Byron requested changes Apr 7, 2025

View reviewed changes

gix-ref/src/store/file/loose/iter.rs Outdated Show resolved Hide resolved

EliahKagan reviewed Apr 7, 2025

View reviewed changes

moroten force-pushed the fix-recursive-list-refs-prefix branch from 461b374 to 23cdf31 Compare April 7, 2025 19:24

moroten changed the title ~~List direct files only for ref name prefix~~ Handle trailing slash in ref list prefix filtering Apr 7, 2025

moroten force-pushed the fix-recursive-list-refs-prefix branch from 23cdf31 to 3ca6811 Compare April 7, 2025 20:14

Byron reviewed Apr 8, 2025

View reviewed changes

moroten added 4 commits April 8, 2025 11:32

Rename prefix test branches

94faf51

Use Into<Cow<>>

507d682

Fix ci failures

a138b38

Fix ci failures

44f4f8c

moroten requested a review from Byron April 9, 2025 09:48

Byron mentioned this pull request Apr 13, 2025

PoC: add RepositoryPathBuf #1921

Draft

Byron self-assigned this Apr 13, 2025

Byron mentioned this pull request Apr 13, 2025

Fix recursive list refs prefix #1954

Merged

Byron closed this Apr 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle trailing slash in ref list prefix filtering #1936

Handle trailing slash in ref list prefix filtering #1936

moroten commented Apr 6, 2025 •

edited

Loading

Byron left a comment •

edited

Loading

EliahKagan left a comment •

edited

Loading

moroten commented Apr 7, 2025

moroten commented Apr 7, 2025

Byron left a comment

Byron Apr 8, 2025

moroten Apr 8, 2025

Byron Apr 8, 2025

Byron Apr 8, 2025

moroten Apr 8, 2025

Byron Apr 8, 2025

moroten Apr 8, 2025

Byron commented Apr 13, 2025

Handle trailing slash in ref list prefix filtering #1936

Handle trailing slash in ref list prefix filtering #1936

Conversation

moroten commented Apr 6, 2025 • edited Loading

Byron left a comment • edited Loading

Choose a reason for hiding this comment

EliahKagan left a comment • edited Loading

Choose a reason for hiding this comment

moroten commented Apr 7, 2025

moroten commented Apr 7, 2025

Byron left a comment

Choose a reason for hiding this comment

Byron Apr 8, 2025

Choose a reason for hiding this comment

moroten Apr 8, 2025

Choose a reason for hiding this comment

Byron Apr 8, 2025

Choose a reason for hiding this comment

Byron Apr 8, 2025

Choose a reason for hiding this comment

moroten Apr 8, 2025

Choose a reason for hiding this comment

Byron Apr 8, 2025

Choose a reason for hiding this comment

moroten Apr 8, 2025

Choose a reason for hiding this comment

Byron commented Apr 13, 2025

moroten commented Apr 6, 2025 •

edited

Loading

Byron left a comment •

edited

Loading

EliahKagan left a comment •

edited

Loading