fix(git): serve want-by-sha for unadvertised refs (PR merge commits) - #27
DanielHabenicht wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
+ Coverage 96.69% 96.80% +0.10%
==========================================
Files 6 6
Lines 1846 2065 +219
==========================================
+ Hits 1785 1999 +214
- Misses 61 66 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for your contribution @DanielHabenicht ! I left a few comments. You also need to fixup the commit msg/body. |
14a02b8 to
6e36d14
Compare
| // would be pruned straight away (see `prune_wants`), so fetching it is pure | ||
| // waste. The excess is dropped and upload-pack rejects those wants. | ||
| if self.cfg.max_wants > 0 && missing.len() > self.cfg.max_wants { | ||
| tracing::warn!( |
There was a problem hiding this comment.
The risk is that if we don't surface this to the user, they'll expect it to work and might be confused why the shas are not available. Someone will need to check the logs to see what's going on.
But, as long as it's a documented behavior, it would be ok.
|
|
||
| /// Collect an iterator of oids into a `HashSet` for order-insensitive assertions | ||
| /// against `parse_wants`. | ||
| fn want_set<I: IntoIterator<Item = String>>(oids: I) -> HashSet<String> { |
There was a problem hiding this comment.
Place helper fns at the bottom, after all tests.
| .take_while(u8::is_ascii_hexdigit) | ||
| .collect(); | ||
| // sha1 (40) or sha256 (64); ignore anything else (e.g. a stray token). | ||
| if oid.len() == 40 || oid.len() == 64 { |
There was a problem hiding this comment.
You could ignore parsing oids over the max_wants here. Then you don't need to truncate and check for missing in ensure_wanted_oids.
There was a problem hiding this comment.
I am actually not that deep into the code, but Claude told me that this is would break the new feature as it would lead to exceeding the max_wants:
- parse_wants collects all wants, including ones the mirror already has. The cap in ensure_wanted_oids is applied to missing — the subset the mirror lacks and must fetch/pin.
- A normal git clone sends a want for every advertised ref. A repo with >100 refs (common with many tags) already exceeds the default max_wants=100 on wants alone — but all of those are present, so missing is empty.
- If we capped at parse time, the one genuinely-missing unadvertised SHA (the PR merge commit) could be dropped as the 101st want, and the core feature would silently fail for large repos.
If you have time you can take over from here.
|
@DanielHabenicht do you think you'll have the time to address the comments? |
I'd be happy to pick this up and wrap it up if you're too busy. |
|
Sorry, so much todo. You can take it from here. |
actions/checkout on a pull_request event fetches the synthetic merge commit by bare SHA. That commit lives only under GitHub's unadvertised refs/pull/<n>/merge, so the mirror clone never captured it and upload-pack rejected the want with "not our ref", breaking CI behind the proxy. Before serving, ensure_fresh scans the upload-pack request for want lines and, for any SHA missing from the mirror, fetches it from upstream and pins it under a reserved refs/proxy-wants/<sha> ref. Pinning makes the object a valid want tip (so upload-pack serves it) and keeps it from git gc. The namespace is hidden from the advertisement yet still honored as a want tip, and excluded from prune so pins survive periodic refreshes. Because those pins are excluded from prune and gc, --max-wants bounds how many a mirror retains: the oldest are pruned beyond the cap so they cannot accumulate without bound. Relies on the upstream serving arbitrary SHAs (GitHub's allowAnySHA1InWant); an upstream that refuses leaves the want unsatisfied for upload-pack to reject. Ordinary branch/tag clones pay only a cheap cat-file check, never an extra upstream call. Assisted-by: Claude:claude-opus-4-8
Note in the README that per-request wants beyond --max-wants are left unserved with a warning, and move the want_set test helper below all tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1329ac2 to
238dbde
Compare
Add e2e coverage for unlimited pins (max_wants=0) and a tolerated local object-check failure, exercising the uncapped and best-effort error branches of the want-by-sha path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Before serving an upload-pack RPC, parse the client's
wantlines and, for any SHA missing from the mirror, fetch it from upstream on demand and pin it under a reservedrefs/proxy-wants/<sha>ref. Pinning makes the object a valid want tip (so upload-pack serves it) and keeps it fromgit gc. The namespace is hidden from the ref advertisement (uploadpack.hideRefs) yet still honored as a want tip, and excluded fromfetch --pruneso pins survive periodic refreshes.Relies on the upstream serving arbitrary SHAs (GitHub's allowAnySHA1InWant); an upstream that refuses leaves the request to fail as before - no regression. Ordinary branch/tag clones pay only a single cheap cat-file check, never an extra upstream call.
What kind of change does this PR introduce?
Summary
actions/checkout on a pull_request event fetches the synthetic merge commit by bare SHA. That commit lives only under GitHub's unadvertised refs/pull//merge, so
clone --mirrornever captured it and the mirror's upload-pack rejected the want with "fatal: not our ref ", breaking CI behind the proxy.Tests
Checklist
cargo fmt --all --check,cargo clippy --all-targets --all-features --locked -- -D warnings,cargo test --all-featuresAssisted-by:trailer (see CONTRIBUTING.md / AGENTS.md)Breaking change?
If yes, describe the impact and the migration path (flags / env, on-disk cache layout).