Fix ls-refs ref-prefix'ing by FintanH · Pull Request #2436 · GitoxideLabs/gitoxide

Fintan Halpenny (FintanH) · 2026-02-15T17:28:50Z

See discussion at #2429

I didn't quite have the correct environment set up, so I'm not sure if all the tests are passing, but let's see if CI catches anything I messed up :)

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 84d2daec19

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

chatgpt-codex-connector · 2026-02-15T17:34:50Z

        /// using `agent` information to identify ourselves.
        pub fn new(
-            prefix_refspecs: Option<&[gix_refspec::RefSpec]>,
+            prefix_refspecs: Option<RefPrefixes>,


Keep LsRefsCommand::new usable from downstream crates

LsRefsCommand::new now takes Option<RefPrefixes>, but RefPrefixes lives in gix_protocol::ls_refs::function and that module is pub(crate) (and not re-exported), so external callers cannot construct Some(...) anymore. In practice, downstream code can only pass None, which silently removes the public ability to configure ref-prefix filtering through this API and is a source-breaking regression for users that previously passed refspec-based filters.

Useful? React with 👍 / 👎.

Sebastian Thiel (Byron)

Thanks a lot for your help, it's much appreciated!

While I am happy to take it from here, I left a few questions hoping you can share more insights so we can keep this alongside the code.

Sebastian Thiel (Byron) · 2026-02-19T06:55:08Z

    fn full_names_have_a_prefix() {
-        assert_eq!(parse("refs/heads/main").to_ref().prefix().unwrap(), "refs/heads/");
-        assert_eq!(parse("refs/foo/bar").to_ref().prefix().unwrap(), "refs/foo/");
+        assert_eq!(parse("refs/heads/main").to_ref().prefix().unwrap(), "refs/heads/main");


The test now tests for the wrong thing, and I think it tries to test that a ref acts as prefix.
Probably that's wrong to start with and might have been added as convenience.
Is there some notes that we can add here to show why this is desired behaviour now?

PS: It seems this would affect prefix generation negatively, at least if one assumes prefixes really are prefixes. It seems more versatile to support exact matches though, i.e. the new behaviour here.

Ya, perhaps I can add more context here for the use case in Radicle.

We actually want to ask for exact references, e.g. refs/rad/id, refs/namespaces/me/refs/rad/sigrefs. This allows us to compute the references we want to ask for next.

If this is not expected or desired, then I think we can take a hit and ask for refs/rad/, and refs/namespaces/me/refs/rad/.

I forget what I found Git to do in these cases.

Sebastian Thiel (Byron) · 2026-02-19T06:56:26Z

+
+        let sans_refs_prefix = source.strip_prefix(b"refs/")?;
+        if let Some(star_pos) = sans_refs_prefix.find_byte(b'*') {
+            // Disallow `*` glob star components after `refs/`


Seeing this here makes me think that this should be documented, to know where this is coming from.
Do you have context to share?

This relates to https://github.com/GitoxideLabs/gitoxide/pull/2436/changes#r2826753347.

Does that answer it, or do you need more context?

Sebastian Thiel (Byron) · 2026-02-19T07:06:39Z

+            if star_pos == 0 {
+                return None;
+            }


It seems the return None might not match what Git does. Maybe there is a good reason to do this? Maybe the analysis is wrong?

Codex Analysis (unvalidated)

Look at #2436 and gather context on how Git is handling prefixes. Use the Git codebase at /Users/byron/dev/github.com/git/git if it helps.

From PR GitoxideLabs/gitoxide#2436, upstream Git context for ref-prefix is:

Prefix generation happens in refspec_ref_prefixes().

For fetch, Git uses src (except exact-OID refspecs); for push, it prefers dst and falls back to src (same function).

Pattern refspecs are truncated at * before sending ref-prefix (strchr(prefix, '*')).

Non-pattern refspecs are expanded through expand_ref_prefix(), which uses these rules in refs.c and implementation at refs.c#L631-L638.

Server-side behavior:

ls-refs treats each ref-prefix as a plain string prefix check (starts_with) in ls-refs.c.

Protocol docs say prefix filtering is an optimization and clients should still filter results locally: gitprotocol-v2.adoc.

Tests confirm exact full-ref prefixes are valid (for example, ref-prefix refs/heads/main) in t5701-git-serve.sh.

How this maps to #2436:

The PR’s change (moving away from “two path components only”) aligns with Git’s model of “full ref until *, or full ref if no *”.

This resolves namespace cases like refs/namespaces/<id>/... that were previously over-truncated.

One notable edge case: Git will emit broad prefixes like refs/ for patterns such as refs/*/main; if Fix ls-refs ref-prefix'ing #2436 intentionally rejects that case, that is a behavior divergence worth documenting in the PR.

Ya, so this is where I opted to try and reach some kind of hybrid approach. There were test examples, for example refs/*/main, that would expect None as the return value. So I was attempting to maintain behaviour on that front.

As the analysis points out, Git would drop back to a prefix that looks like refs/ which amounts to give me everything. Afaict, refs/ and None will result in the same behaviour for ls-refs, but perhaps I'm wrong or missing a case :)

The previous implementation would find the prefix that would include `refs/` and the next component. This can be too strict for namespaced references, i.e. `refs/namespaces/*`. This change follows the same approach as the C implementation of Git. It takes the prefix as the whole reference up to the `*`. If there is no `*`, if it starts with `refs/`, take the whole reference. Otherwise, there is no prefix.

- make docs match implementation - make tests match changes - add tests for the namespace case Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>

The `RefPrefixes` type captures the intent of building a set of `ref-prefix` arguments for the ls-refs protocol. Since refspecs are the natural type that these are built from, as seen in `handshake/mod.rs`, a constructor is provided in the form of `RefPrefixes::from_refspecs`. For other, external API users `RefSpecs::new` and `RefSpecs::extend` are provided. This is also a breaking change to `LsRefsCommand::new()` which now takes `Option<RefPrefixes>` as first parameter.

Fix clippy. Also: **Findings** 1. High: this is a public API break, not just a refactor. [`LsRefsCommand`] is part of the public crate surface at [gix-protocol/src/lib.rs:70](/Users/byron/dev/github.com/GitoxideLabs/gitoxide.p r-review/gix-protocol/src/lib.rs#L70), and its constructor now changed from `Option<&[RefSpec]>` to `Option<RefPrefixes>` at [gix-protocol/src/ls_refs.rs:129](/Users/byron/dev/github.com/GitoxideLabs/gitox ide.pr-review/gix-protocol/src/ls_refs.rs#L129). Any downstream caller passing `Some(&refspecs[..])` will stop compiling. Given the current commit message frames this as “Introduce RefPrefixes type”, this needs either a compatibility path or an explicit breaking-change callout. 2. Medium: the ref-prefix emission order became nondeterministic. `RefPrefixes` now stores prefixes in a `HashSet` at [gix-protocol/src/ls_refs.rs:68](/Users/byron/dev/github.com/GitoxideLabs/gitoxi de.pr-review/gix-protocol/src/ls_refs.rs#L68) and emits them by iterating that set at [gix-protocol/src/ls_refs.rs:100](/Users/byron/dev/github.com/GitoxideLabs/gitox ide.pr-review/gix-protocol/src/ls_refs.rs#L100). The previous code preserved refspec order. The protocol likely tolerates this, but it changes the wire payload and trace output unpredictably across runs, which is a regression in determinism and makes prefix-related tests/logging harder to stabilize. 3. Low: the new docs have a broken intra-doc link/typo. The comment references `RefsPrefixes::new` at [gix-protocol/src/ls_refs.rs:62](/Users/byron/dev/github.com/GitoxideLabs/gitoxi de.pr-review/gix-protocol/src/ls_refs.rs#L62), but the type is `RefPrefixes`. `cargo doc` warns about this. **Open Questions / Assumptions** If the constructor change is intentional, I’d still expect the commit message/changelog to call out that `LsRefsCommand::new` is now a different public API. Otherwise, I’d keep `new` accepting refspecs and add a second constructor for prebuilt `RefPrefixes`. **Verification** Feature-scoped `cargo check` for `gix-protocol` passed, and targeted `gix-protocol` protocol tests passed. I also ran `cargo doc` for that crate, which surfaced the broken doc link above. Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>

Sebastian Thiel (Byron) · 2026-03-23T09:59:56Z

Thanks a lot for the patience, it's finally done!

Meantime, I changed a lot which helps me do reviews on PRs that otherwise would take me a lot of time to research. One of the advantages is much higher-quality reviews, and a review-commit for each input commit if changes were needed.

Most notably, I had to change the message of your second commit to make it breaking via !, and I added a sentence about the actual breaking change (the parameter type change of a public method).

Fintan Halpenny (FintanH) · 2026-03-23T11:21:39Z

Thanks for getting around to it!

I'm not precious about my changes, especially when I'm not so familiar with the codebase, so I'm delighted that you messed around and changed things. Fwiw, my workflow is the same over on heartwood 😊

Fintan Halpenny (FintanH) force-pushed the fintohaps/fix-ls-refs-prefixing branch from 84d2dae to 485006d Compare February 15, 2026 17:33

chatgpt-codex-connector Bot reviewed Feb 15, 2026

View reviewed changes

Fintan Halpenny (FintanH) force-pushed the fintohaps/fix-ls-refs-prefixing branch from 485006d to 69159dd Compare February 15, 2026 20:38

LOCI-AI (loci-dev) mentioned this pull request Feb 16, 2026

UPSTREAM PR #2436: Fix ls-refs ref-prefix'ing auroralabs-loci/gitoxide#22

Open

Sebastian Thiel (Byron) reviewed Feb 19, 2026

View reviewed changes

Sebastian Thiel (Byron) self-assigned this Mar 23, 2026

Sebastian Thiel (Byron) force-pushed the fintohaps/fix-ls-refs-prefixing branch from 69159dd to 0a5ed89 Compare March 23, 2026 08:48

Codex (codex) and others added 2 commits March 23, 2026 17:23

refactor

de8dd0b

- make docs match implementation - make tests match changes - add tests for the namespace case Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>

Sebastian Thiel (Byron) force-pushed the fintohaps/fix-ls-refs-prefixing branch from 0a5ed89 to 98b3246 Compare March 23, 2026 09:49

Sebastian Thiel (Byron) force-pushed the fintohaps/fix-ls-refs-prefixing branch from 98b3246 to 82674cb Compare March 23, 2026 09:58

Sebastian Thiel (Byron) enabled auto-merge March 23, 2026 09:59

TBD

a28a147

Sebastian Thiel (Byron) merged commit 28f3402 into GitoxideLabs:main Mar 23, 2026
31 checks passed

Sebastian Thiel (Byron) mentioned this pull request Mar 23, 2026

fintohaps/fix ls refs prefixing #2485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix ls-refs ref-prefix'ing#2436

Fix ls-refs ref-prefix'ing#2436
Sebastian Thiel (Byron) merged 5 commits intoGitoxideLabs:mainfrom
FintanH:fintohaps/fix-ls-refs-prefixing

Fintan Halpenny (FintanH) commented Feb 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Feb 15, 2026

Uh oh!

Sebastian Thiel (Byron) left a comment

Uh oh!

Sebastian Thiel (Byron) Feb 19, 2026

Uh oh!

Fintan Halpenny (FintanH) Feb 19, 2026

Uh oh!

Sebastian Thiel (Byron) Feb 19, 2026

Uh oh!

Fintan Halpenny (FintanH) Feb 19, 2026

Uh oh!

Sebastian Thiel (Byron) Feb 19, 2026 •

edited

Loading

Uh oh!

Fintan Halpenny (FintanH) Feb 19, 2026

Uh oh!

Sebastian Thiel (Byron) commented Mar 23, 2026

Uh oh!

Fintan Halpenny (FintanH) commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Fintan Halpenny (FintanH) commented Feb 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

Sebastian Thiel (Byron) left a comment

Choose a reason for hiding this comment

Uh oh!

Sebastian Thiel (Byron) Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Fintan Halpenny (FintanH) Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Sebastian Thiel (Byron) Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Fintan Halpenny (FintanH) Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Sebastian Thiel (Byron) Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fintan Halpenny (FintanH) Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Sebastian Thiel (Byron) commented Mar 23, 2026

Uh oh!

Fintan Halpenny (FintanH) commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sebastian Thiel (Byron) Feb 19, 2026 •

edited

Loading