Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
moroten opened this issue Apr 6, 2025 · 5 comments · May be fixed by #1936
Open

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

moroten opened this issue Apr 6, 2025 · 5 comments · May be fixed by #1936
Labels
acknowledged an issue is accepted as shortcoming to be fixed help wanted Extra attention is needed

Comments

@moroten
Copy link

moroten commented Apr 6, 2025

Current behavior 😯

Some refs reported by repo.references()?.prefixed(...) are outside the prefix scope. The refs that are reported are loose refs which are not present in .git/packed-refs.

This has been tested with gix=0.71.0 and overriding with gix-features=0.41.1.

Relates to #1928 with the PR #1931.

Expected behavior 🤔

Only refs that starts with the prefix are reported.

Git behavior

No response

Steps to reproduce 🕹

Create .git/refs/heads/foo/bar. That ref will be listed with repo.references()?.prefixed("refs/heads/b").

@Byron
Copy link
Member

Byron commented Apr 6, 2025

Thanks for reporting, I am looking forward to a reproduction.

While #1931 kind of 'winged' it, for a fix here we might have to go and check what Git really does. In theory, loose and packed refs 'just' have to be sorted the same way for this to work. And of course, in case of prefixed, they have to start at the same position.

Maybe one of these factors is still off.

@moroten
Copy link
Author

moroten commented Apr 6, 2025

In 293bfc0#diff-f8ebed64b52627e407b8ee19c33669e60975b015557005c518ac5a0c1c29e599R49-R57, the component of a ref is compared to the last component of the prefix. The full path is also compared to the prefix without the last component. This means that refs/heads/prefix will match refs/heads/some/path/prefix-suffix.

moroten added a commit to meroton/GitoxideLabs-gitoxide that referenced this issue Apr 6, 2025
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 makes sure to only list direct files and not walk the
directory tree recursively.

Fixes GitoxideLabs#1934.
@moroten
Copy link
Author

moroten commented Apr 6, 2025

It turns out that prefix refs/heads/foo matches refs/heads/foo-suffix and refs/heads/sub/dir/foo, but the documentation of [gix_ref::store_impl::file::overlay_iter::Platform::prefixed](https://github.com/GitoxideLabs/gitoxide/blob/1612c73a16c8d900e1b6ef35b25bd6b3e3f6652a/gix-ref/src/store/file/overlay_iter.rs#L200) says:

Please note that "refs/heads" or "refs\heads" is equivalent to "refs/heads/"

@moroten moroten linked a pull request Apr 6, 2025 that will close this issue
@moroten
Copy link
Author

moroten commented Apr 6, 2025

What is the contract for the Platform::prefixed()? 293bfc0 talks about letting the prefix refs/heads/foo match refs/heads/foo-suffix whereas the implementation for loose refs documents that refs/heads/foo behaves like refs/heads/foo/ as prefix. Should there be multiple API functions to reduce the confusion? Is there a git API that Gitoxide is trying to mimic?

I've created the PR #1936 to at least not match refs/heads/sub/dir/foo.

@Byron
Copy link
Member

Byron commented Apr 7, 2025

It turns out that prefix refs/heads/foo matches refs/heads/foo-suffix and refs/heads/sub/dir/foo, but the documentation of [gix_ref::store_impl::file::overlay_iter::Platform::prefixed](https://github.com/GitoxideLabs/gitoxide/blob/1612c73a16c8d900e1b6ef35b25bd6b3e3f6652a/gix-ref/src/store/file/overlay_iter.rs#L200) says:

Please note that "refs/heads" or "refs\heads" is equivalent to "refs/heads/"

Thanks so much for dissecting this!

I think we are onto something here, maybe a confusion due to dissimilarities between the prefixed loose reference iteration and prefixed packed-refs iterations.

refs/heads/foo from what I recall should match refs/heads/foo-suffix, but it shoudl definitely not match refs/heads/sub/dir/foo which it seems to do currently.

Maybe these simple tests could be added in #1936 both for packed and loose refs and see where we land, also for (prefixed) iteration.

If there are differences, it's clear why this happens, and yet another test using the overlay iterator could verify that the new implementation is indeed correct.

And… I'd really appreciate your help here, from what I can tell you are really close to a solution and with your familiarity with editing/creating tests I think it should be convenient to iterate (no pun intended 😅) towards a solution.

@Byron Byron added help wanted Extra attention is needed acknowledged an issue is accepted as shortcoming to be fixed labels Apr 7, 2025
moroten added a commit to meroton/GitoxideLabs-gitoxide that referenced this issue Apr 7, 2025
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.
moroten added a commit to meroton/GitoxideLabs-gitoxide that referenced this issue Apr 7, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged an issue is accepted as shortcoming to be fixed help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants