Skip to content

Check pushed tips even when the pack is empty - #42

Merged
tobi merged 1 commit into
tobi:mainfrom
0bserver07:fix/empty-pack-push-tip-check
Sep 11, 2026
Merged

tobi merged 1 commit into
tobi:mainfrom
0bserver07:fix/empty-pack-push-tip-check

Conversation

@0bserver07

Copy link
Copy Markdown
Contributor

Fixes #37. Thanks @czk-aa for the repro and for tracing both halves.

A ref-only push sends a 32-byte pack with zero objects, and receive_pack_process in crates/walgit-server/src/smart.rs only ran the connectivity check when ingest_pack returned Ok(Some(_)). So a command line pointing refs/heads/ghost at an object the server doesn't have got ok, the ref went into the WAL, and every clone walking it died with missing object.

The check now runs whenever unpack succeeded and any update names a non-zero tip, pack or no pack. With wal.check_connectivity on (the default) it's the existing walk, which stops at existing refs, so a legitimate ref-only push costs a few lookups. With it off, the tips are still looked up one by one with has_object before anything is published, so the hole is closed either way. I left verify_txn in publish.rs alone: the receive path is the one place a pusher's oid enters, and the walk there already has the objects in hand.

Test: empty_pack_push_to_a_missing_object_is_refused in tests/e2e.rs posts the exact 32-byte pack from the report against both settings and expects ng refs/heads/ghost with no ref published, then pushes main to a new branch (git sends the same zero-object pack) and expects it to land. It fails on main at the ng assertion and passes here. Full e2e suite: 44 passed.

A ref-only push carries a 32-byte pack with zero objects, and receive-pack
skipped the connectivity check whenever ingest returned Ok(None). A command
line naming an object the server does not have was therefore accepted, the
ref was published to the WAL, and every clone that walked it failed with
"missing object" (tobi#37, reported with a repro by czk-aa). The check now runs
whenever unpack succeeded and any update names a non-zero tip. With
wal.check_connectivity the existing walk covers the tips and everything new
under them and stops at existing refs, so a legitimate ref-only push costs a
few lookups; with it turned off the tips are still looked up one by one
before anything is published. The e2e test posts the exact empty pack from
the report against both settings and expects ng refs/heads/ghost, then pushes
main to a new branch, which sends the same zero-object pack, and expects it
to land. The test fails on main at the ng assertion and passes here.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CSbhRh6UEjFrgBYYzucvDe
@tobi
tobi merged commit 66e3e14 into tobi:main Sep 11, 2026
0 of 2 checks passed
@0bserver07
0bserver07 deleted the fix/empty-pack-push-tip-check branch September 11, 2026 05:58
@czk-aa

czk-aa commented Sep 11, 2026

Copy link
Copy Markdown

Quick note from a follower of #37: the current CI failures look unrelated to this PR.

  • e2e -> 'fetch_from_front_that_serves_the_base_remotely' panicked at e2e.rs:1224 on 'base carries a commit-graph layer'; the log shows 'commit-graph-chain.lock: File exists' (concurrent), and this test is already listed as known-flaky in AGENTS.md section 5 ("~1 in 3 under the full e2e suite: base published without has_commit_graph"). The new 'empty_pack_push_to_a_missing_object_is_refused' test passed (43 passed, 1 failed).
  • warnings + test -> 'too_small_closed_slots_are_recorded_and_skipped_not_remeasured' at walgit-bundle/tests/bundle.rs:1046 missed the 'yesterday' daily row (dailies: [(1788739200, Missing)]); the asserted row timestamp sits exactly on the run date (2026-09-07), so it looks like a date/threshold-sensitive existing test, unrelated to this change (the PR touches only smart.rs and tests/e2e.rs).

Both fail on code paths this PR does not touch. A workflow rerun should be enough to get a green check here. Thanks for the fix!

brightsparc added a commit to introspection-org/walgit that referenced this pull request Sep 12, 2026
Upstream main (tobi#42-tobi#45, tobi#49) plus the Azure and IRSA branches as
updated for PRs tobi#50 and tobi#48, plus the store-plugin and notify-transport
lines the production image was already built from (a6ce265). The two
conflicts were additive: the Event Grid handshake test and its doc note
now sit beside the loopback notify-transport test and its bullet.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6UZ2AaFQ6m7xgdZKRyHEV
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

receive-pack: empty-pack push skips the connectivity check, refs can end up pointing at objects that don't exist

3 participants