Handle trailing slash in ref list prefix filtering#1936
Handle trailing slash in ref list prefix filtering#1936moroten wants to merge 5 commits intoGitoxideLabs:mainfrom
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-1936Please disregard this if it is not convenient or if make_ref_repository.sh shall be changed again.
461b374 to
23cdf31
Compare
|
Thank you @EliahKagan. It turned out that I missed the binary files when using |
|
I've rewritten the PR to use a 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.
23cdf31 to
3ca6811
Compare
Byron
left a comment
There was a problem hiding this comment.
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.
| 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<'_, '_>> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The tests are now more explicitly named to test that.
| 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<'_, '_>> { |
There was a problem hiding this comment.
This should be &BStr if i twas &Path before.
gix-ref/src/store/file/loose/iter.rs
Outdated
| /// | ||
| /// 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<'_, '_>> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I put impl Into<Cow<...>> in most places. Not sure if some should just be &BStr or not.
|
|
||
| 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 |
There was a problem hiding this comment.
I think these names should be changed so packed isn't included. This is because symbolic refs are never packed.
There was a problem hiding this comment.
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.
|
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. |
Previously,
refs/heads/foo/barwould be listed when runningrepo.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 matchingrefs/heads/**/b*.This commit fixes that bug but also allows to use a trailing
/in the prefix, allowing to filter forrefs/heads/foo/and not getrefs/heads/foo-baras a result.Fixes #1934.