fix: build_pack handles force-push when local lacks the remote tip#26
Merged
Conversation
Closes #21. ## Problem `build_pack` invoked `git rev-list ^<have> <want>` blindly. When `<have>` is the remote tip reported by the contract state but the local repo has never seen it (canonical case: force-pushing an orphan commit, exactly what snapshot-mode mirroring does), rev-list bailed with `fatal: bad object <sha>` and the entire push failed. This was the root cause behind the freenet-core mirror failing 5/5 runs after merge. PR #20 worked around it at the workflow layer with a `git fetch freenet $BRANCH` before each push; that fix keeps working but adds bandwidth and only helps callers of the reusable workflow. ## Fix Two pieces: 1. `parse_push_spec` now returns `(force, src, dst)` instead of silently discarding the leading `+`. 2. `build_pack` takes the new `force: bool` and pre-validates `<have>` with `git cat-file -e`. If `<have>` is missing locally and the push is a force, drop `<have>` from the rev-list args and send everything reachable from `<want>` -- the right semantic for "replace whatever was there." If `<have>` is missing and the push is non-force, return a directed error pointing the user at `git fetch` or `+` rather than the cryptic `fatal: bad object` from rev-list. ## Tests Six new unit tests in `git-remote-freenet.rs` covering: - `parse_push_spec_force_flag` -- `+` is parsed as force, plain spec is non-force. - `build_pack_no_have_succeeds` -- empty have is the first-push case. - `build_pack_have_present_succeeds` -- existing fast-forward path. - `build_pack_missing_have_non_force_fails_with_directed_error` -- the non-force surface, directed error message. - `build_pack_missing_have_force_drops_have_and_succeeds` -- the exact case that was broken before. - `object_exists_distinguishes_real_from_bogus` -- pins the `cat-file -e` helper. ## End-to-end verification Reproduced the original failure with the patched binary against the live demo URL `freenet:3GEERif5ihbf/freenet-core`: $ # Fresh orphan repo, force push, no fetch: $ git push freenet +main:main ==> reading repo state from Freenet ==> publishing 226 B pack as a single bundle ==> updating repo state on Freenet ==> done + b2a8836...5d4a733 main -> main (forced update) $ # Non-force from fresh orphan -- gets the directed error: $ git push freenet main:main ! [remote rejected] main -> main (remote tip 5d4a7334... is not present in the local repo; run `git fetch` to populate it, or use `git push +`/`--force` to overwrite (which will discard the remote tip's history)) The freenet-core demo was clobbered briefly during testing and re-published to its proper snapshot (`b2a8836`) after. ## Follow-up Once this ships in a release and the caller workflow's pin is bumped, drop the `git fetch` workaround from `.github/workflows/mirror-repo.yml`. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses all four internal reviewers + Codex on PR #26. Codex P2 / Code-first #1 / Skeptical M1: object existence check now distinguishes "missing", "non-commit", and "tooling failure" cases. Renamed `object_exists` -> `commit_exists` and reimplemented via `git rev-parse --verify --quiet <sha>^{commit}`. The new check: - Returns Ok(true) only if <sha> is present and is a commit object. cat-file -e succeeded on blobs and trees, which would have left the cryptic `fatal: not a commit object` rev-list error reachable for corrupted local stores. - Distinguishes missing from corrupt-tooling via exit code 1 vs 128; the latter bubbles up rather than silently widening the pack (force) or producing a misleading "run git fetch" message (non-force). - New test `commit_exists_rejects_blob_and_tree` pins this against blob and tree SHAs from the same repo. Skeptical M2: directed error message reordered to lead with --force since snapshot/orphan-style pushes are the canonical case driving this PR. `git fetch` is still listed for the regular fast-forward recovery scenario but no longer the first remediation. Code-first #5: error message wording improved -- "use `git push +`" isn't a real invocation; rephrased as "use `git push --force` (or prefix the refspec with `+`)". The prior bail!(...) string was preserved but rephrased. Code-first #7 / Skeptical L3: test setup now sets per-repo commit.gpgsign=false and tag.gpgsign=false, so a developer with GPG signing on globally without a working key won't trip a prompt in tests. Code-first #8: `build_pack_have_present_succeeds` was a no-op (let _ = pack). Renamed to `build_pack_have_equal_to_want_emits_ empty_pack` and now asserts the resulting pack is smaller than the no-have pack -- pinning that rev-list short-circuits when have == want. Testing must-add #1: new test `build_pack_fast_forward_have_is_real_ancestor` exercises the production happy path -- have is the parent commit, want is the child. Asserts the incremental pack is non-empty AND smaller than the full pack, so a future regression that drops `^<have>` from the rev-list args silently would be caught. Testing should-add #3: directed-error wording now pinned via substring assertions on "not present", "local repo", "git fetch", "--force", and the bogus SHA itself. Big-picture LOW: docstring "Phase 1.0 caveats" block in the helper gained a "Push semantics" bullet documenting non-force vs force expectations. Pushed back / not addressing in this PR: - Code-first #2 (silent SinglePack/ChunkedPack flip when force drops have for large `want` history): real concern but architectural; for snapshot mode the orphan commits are small so not a current practical issue. Worth a follow-up if a future caller pushes a large-history force. - Code-first #4 (parse_push_spec allows force=true with empty src): caller rejects empty src before reading force; future-proofing with a defensive parser check is unnecessary today. - Skeptical L2 (race between cat-file and rev-list under concurrent gc): theoretical, two-Command::status() window. The helper is invoked synchronously by git push; concurrent gc on the same repo is rare. Not in scope. - Skeptical L4 (git init -b main needs git >= 2.28): all current CI runners have a modern git. Not in scope. - Skeptical L5 (pack uploaded before contract validation runs): pre-existing ordering, not introduced here. - Testing should-add #2 (parse-spec -> build-pack glue test): the existing test surface is sufficient given each function is unit-tested. A full handle_push test would require WS-API scaffolding that's overkill for this scope. - Testing should-add #4 (bogus `want`): the existing `git_resolve_ref` call path catches that earlier in handle_push. - Big-picture LOW (file follow-up issue for workflow workaround cleanup): tracked in issue body as "follow-up"; will file a separate ticket once this lands and the caller pin is bumped. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed reviewer findings in c7d821f. Headlines: Codex P2 / Code-first #1 / Skeptical M1: Skeptical M2: directed error reordered to lead with Code-first #5: error wording fixed -- Code-first #7 / Skeptical L3: tests force Code-first #8: weak Testing must-add #1: Testing should-add #3: directed-error wording pinned via substring assertions on Big-picture LOW: docstring 'Phase 1.0 caveats' gained a 'Push semantics' bullet. Pushed back on (out of scope, see commit body for reasoning): Code-first #2/#4, Skeptical L2/L4/L5, Testing should-add #2/#4. Most are architectural concerns or pre-existing issues this PR doesn't widen. 8 tests passing, clippy clean, fmt clean. [AI-assisted - Claude] |
sanity
added a commit
that referenced
this pull request
May 7, 2026
freenet-git 0.1.14 (PR #26 / issue #21) fixed the underlying build_pack bug -- force pushes from a fresh orphan repo no longer fail when the local lacks the remote tip. The workflow-side workaround added in #20 is now redundant and would just waste bandwidth on every snapshot push (it pulls the entire previous chunked pack into local objects only to immediately overwrite it). Caller workflows should bump their pinned SHA of mirror-repo.yml to whatever this commit lands as. [AI-assisted - Claude] Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
build_packinvokedgit rev-list ^<have> <want>blindly. When<have>is the remote tip reported by the contract state but the local repo has never seen it (canonical case: force-pushing an orphan commit, exactly what snapshot-mode mirroring does), rev-list bailed withfatal: bad object <sha>and the entire push failed.This was the root cause behind the freenet-core mirror failing 5/5 runs after merge. PR #20 worked around it at the workflow layer with
git fetch freenet $BRANCHbefore each push; that fix keeps working but adds bandwidth and only helps callers of the reusable workflow.Approach
parse_push_specnow returns(force, src, dst)instead of silently discarding the leading+.build_packtakes the newforce: booland pre-validates<have>withgit cat-file -e. If<have>is missing locally and the push is a force, drop<have>from the rev-list args. If non-force, return a directed error pointing atgit fetchor+instead of the cryptic rev-list output.Testing
git-remote-freenet.rscovering the parse, thecat-filehelper, and fourbuild_packcases (no have, have-present, missing-have non-force = directed error, missing-have force = succeeds).b2a8836after testing.Follow-up
Once released and caller pin is bumped, drop the
git fetchworkaround from.github/workflows/mirror-repo.yml.Closes #21.
[AI-assisted - Claude]