Preserve diverged local refs in SafelyAdvanceLocalRef#1256
Closed
Soph wants to merge 1 commit into
Closed
Conversation
The previous contract overwrote any local ref the target couldn't reach, which silently discarded unpushed local commits whenever a fetch landed sibling commits — e.g. checkpoint metadata produced on another machine sharing the same orphan-style refs (entire/checkpoints/v1, the V2 main ref). The objects survived in the loose-objects pool until git gc, but the branch ref no longer pointed at them. Tighten the helper so it only sets the ref when the move is non-destructive: missing → create, local at/ahead → no-op (existing protection), local strictly behind → fast-forward, diverged or unrelated history → no-op with a debug log. All three production callers (promoteRemoteTrackingMetadataBranch, FetchMetadataBranch, PromoteTmpRefSafely) sync orphan branches where this is the correct semantic; resume falls through to remote-tracking-tree reads in the rare divergent case. Update TestFetchV2MainFromURL_UpdatesExistingRef to use the existing advanceV2MainOnTop helper — the prior setup advanced the remote via a second call to createV2MainRef, which always produces an unrelated orphan commit. That setup was only "updating" via the now-removed diverged-overwrite path; a descendant commit on top of the previous tip is what the real condensation flow produces. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4197ff1b1da6
Collaborator
Author
|
Folding into #1252 since the changes are tightly coupled — the global helper tightening makes the resume fix safer, but each fix is small on its own. Branch will be deleted. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens git ref promotion/advancement for Entire’s orphan-style metadata refs by ensuring SafelyAdvanceLocalRef only performs non-destructive updates (create / no-op / fast-forward), and skips updates on divergence to avoid losing unpushed local checkpoint commits.
Changes:
- Update
SafelyAdvanceLocalRefto no-op (with debug logging) when local and target refs have diverged/unrelated history. - Add a dedicated unit test suite covering missing/equal/ahead/behind/diverged/unrelated scenarios for
SafelyAdvanceLocalRef. - Fix
TestFetchV2MainFromURL_UpdatesExistingRefsetup to advance the remote ref via a true descendant commit (fast-forward scenario).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cmd/entire/cli/strategy/common.go |
Tightens ref-advance semantics to avoid destructive overwrites and adds debug logging on divergence. |
cmd/entire/cli/strategy/safely_advance_local_ref_test.go |
Adds focused unit coverage for the new SafelyAdvanceLocalRef decision matrix. |
cmd/entire/cli/strategy/checkpoint_remote_test.go |
Updates the v2 /main fetch test to exercise the intended fast-forward path. |
Comments suppressed due to low confidence (1)
cmd/entire/cli/strategy/common.go:151
repo.Reference(localRefName, true)errors other thanplumbing.ErrReferenceNotFoundare currently treated the same as “ref missing” (the function falls through and unconditionally sets the ref). That can mask repository I/O/corruption errors and potentially overwrite a ref when we should instead return an error. Consider explicitly handlingErrReferenceNotFoundas the only create-path, and returning a wrapped error for all otherlocalErrvalues.
func SafelyAdvanceLocalRef(ctx context.Context, repo *git.Repository, localRefName plumbing.ReferenceName, targetHash plumbing.Hash) error {
currentLocal, localErr := repo.Reference(localRefName, true)
if localErr == nil {
if currentLocal.Hash() == targetHash {
Comment on lines
146
to
+159
| @@ -143,6 +154,14 @@ func SafelyAdvanceLocalRef(ctx context.Context, repo *git.Repository, localRefNa | |||
| if IsAncestorOf(ctx, repo, targetHash, currentLocal.Hash()) { | |||
| return nil | |||
| } | |||
| if !IsAncestorOf(ctx, repo, currentLocal.Hash(), targetHash) { | |||
| logging.Debug(ctx, "skipping advance: local ref has diverged from target", | |||
| slog.String("ref", string(localRefName)), | |||
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
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.
https://entire.io/gh/entireio/cli/trails/419
Summary
SafelyAdvanceLocalRefcontract overwrote any local ref the target couldn't reach, which silently discarded unpushed local commits whenever a fetch landed sibling commits — e.g. checkpoint metadata produced on another machine sharing the same orphan-style refs (entire/checkpoints/v1, the V2 main ref). The objects survived in the loose-objects pool until git gc, but the branch ref no longer pointed at them.promoteRemoteTrackingMetadataBranch,FetchMetadataBranch,PromoteTmpRefSafely) sync orphan branches where "skip on divergence" is the correct semantic. In the rare divergent case, resume falls through to remote-tracking-tree reads (v1) or the V1 fallback (DualCheckpointReader → v1).TestFetchV2MainFromURL_UpdatesExistingRefto use the existingadvanceV2MainOnTophelper. The prior setup advanced the remote via a second call tocreateV2MainRef(which always produces an unrelated orphan commit). That setup was only "updating" the local ref via the now-removed diverged-overwrite path; a descendant commit on top of the previous tip is what the real condensation flow produces.Companion PR
Stacks naturally with #1252 (which fixes the immediate user-visible "session log not available" bug). This PR is the broader hardening that came out of reviewing that fix — protects unpushed local checkpoint commits against destructive overwrite across all fetch paths.
Test plan
mise run checkpasses (fmt, lint, unit, integration, canary)strategy/safely_advance_local_ref_test.gocover all six branches:LocalMissing_SetsToTargetLocalEqualsTarget_NoOpLocalAhead_NoOp(existing protection)LocalBehind_FastForwardsDiverged_PreservesLocal(new behavior — failed onmain)UnrelatedHistory_PreservesLocal(new behavior — failed onmain)TestFetchMetadataBranch_DoesNotRewindLocalAheadstill passesTestFetchV2MainFromURL_DoesNotRewindLocalAheadstill passes🤖 Generated with Claude Code
Note
Medium Risk
Changes git ref promotion logic used by metadata/v2 ref fetch paths; incorrect ancestor detection could cause refs to stop updating (or to remain diverged) and impact resume behavior, though it reduces risk of losing unpushed local commits.
Overview
Prevents fetch/promotion code from overwriting orphan-style local refs when the fetched target has diverged or unrelated history, preserving unpushed local checkpoint commits.
SafelyAdvanceLocalRefnow only creates, no-ops, or fast-forwards refs, and logs a debug message when it detects divergence instead of updating.Adds a dedicated unit test suite covering missing/equal/ahead/behind/diverged/unrelated scenarios, and updates
TestFetchV2MainFromURL_UpdatesExistingRefto advance the remote v2 ref via a true descendant commit so the test exercises the intended fast-forward path.Reviewed by Cursor Bugbot for commit ca40dd0. Configure here.