From ca40dd00d14a3262156c4ae061f6912f7b97eab7 Mon Sep 17 00:00:00 2001 From: Stefan Haubold Date: Fri, 22 May 2026 23:18:43 +0200 Subject: [PATCH] Preserve diverged local refs in SafelyAdvanceLocalRef MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Entire-Checkpoint: 4197ff1b1da6 --- .../cli/strategy/checkpoint_remote_test.go | 8 +- cmd/entire/cli/strategy/common.go | 31 ++- .../strategy/safely_advance_local_ref_test.go | 183 ++++++++++++++++++ 3 files changed, 213 insertions(+), 9 deletions(-) create mode 100644 cmd/entire/cli/strategy/safely_advance_local_ref_test.go diff --git a/cmd/entire/cli/strategy/checkpoint_remote_test.go b/cmd/entire/cli/strategy/checkpoint_remote_test.go index 08e815cb3a..4bdd6fceaf 100644 --- a/cmd/entire/cli/strategy/checkpoint_remote_test.go +++ b/cmd/entire/cli/strategy/checkpoint_remote_test.go @@ -918,10 +918,12 @@ func TestFetchV2MainFromURL_UpdatesExistingRef(t *testing.T) { require.NoError(t, err) hash1 := strings.TrimSpace(string(hash1Out)) - // Add a second commit on the remote's v2 ref - createV2MainRef(ctx, t, remoteDir) // Creates a new orphan commit, updating the ref + // Advance the remote's v2 /main with a child commit so the new tip is a + // strict descendant of hash1 (the realistic CLI flow — condensation + // appends a new checkpoint commit on top of the previous tip). + advanceV2MainOnTop(ctx, t, remoteDir, hash1) - // Fetch again — should update + // Fetch again — should fast-forward require.NoError(t, FetchV2MainFromURL(ctx, remoteDir)) hashCmd = exec.CommandContext(ctx, "git", "rev-parse", paths.V2MainRefName) diff --git a/cmd/entire/cli/strategy/common.go b/cmd/entire/cli/strategy/common.go index ce49de7d1e..104a138427 100644 --- a/cmd/entire/cli/strategy/common.go +++ b/cmd/entire/cli/strategy/common.go @@ -126,13 +126,24 @@ func PromoteTmpRefSafely(ctx context.Context, tmpRefName, destRefName plumbing.R return nil } -// SafelyAdvanceLocalRef updates localRefName to point at targetHash, except -// when the existing local ref is already at or ahead of targetHash. In that -// case it leaves the local ref unchanged to avoid rewinding locally-ahead -// work. Otherwise (local missing, behind, or diverged) it updates the ref to -// targetHash. +// SafelyAdvanceLocalRef updates localRefName to point at targetHash only when +// the move is non-destructive: // -// The ancestry check walks from the local ref (which has full history), so +// - local missing → create at target +// - local == target → no-op +// - local at or ahead of target (target is an ancestor of local) → no-op +// - local strictly behind target (local is an ancestor of target) → fast-forward +// - diverged or unrelated history (neither ref is an ancestor of the other) +// → no-op, logged at debug level +// +// The diverged case is the dangerous one this guard exists for. The CLI +// maintains orphan-style refs (entire/checkpoints/v1, the V2 main ref) where +// unpushed local commits encode user work — a fetch that landed sibling +// commits from another machine must not silently rewind that work. In the +// diverged case readers fall through to the remote-tracking tree or to v1, +// so resume keeps working while local-only commits stay reachable. +// +// The ancestry checks walk from the local ref (which has full history), so // callers that fetched with --depth=1 do not break the check. func SafelyAdvanceLocalRef(ctx context.Context, repo *git.Repository, localRefName plumbing.ReferenceName, targetHash plumbing.Hash) error { currentLocal, localErr := repo.Reference(localRefName, true) @@ -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)), + slog.String("local", currentLocal.Hash().String()), + slog.String("target", targetHash.String()), + ) + return nil + } } newRef := plumbing.NewHashReference(localRefName, targetHash) diff --git a/cmd/entire/cli/strategy/safely_advance_local_ref_test.go b/cmd/entire/cli/strategy/safely_advance_local_ref_test.go new file mode 100644 index 0000000000..57d650a815 --- /dev/null +++ b/cmd/entire/cli/strategy/safely_advance_local_ref_test.go @@ -0,0 +1,183 @@ +package strategy + +import ( + "context" + "testing" + "time" + + "github.com/entireio/cli/cmd/entire/cli/testutil" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/object" +) + +const safelyAdvanceTestRef plumbing.ReferenceName = "refs/heads/safely-advance-test" + +// newSafelyAdvanceTestRepo opens an empty git repository for ref-manipulation +// tests. It uses testutil.InitRepo so author/GPG config matches the rest of +// the suite, but the tests themselves operate purely via plumbing — no +// worktree state needed. +func newSafelyAdvanceTestRepo(t *testing.T) *git.Repository { + t.Helper() + dir := t.TempDir() + testutil.InitRepo(t, dir) + repo, err := git.PlainOpen(dir) + if err != nil { + t.Fatalf("PlainOpen: %v", err) + } + return repo +} + +// makeEmptyTreeCommit writes a commit with an empty tree and the given parents +// and message. Different messages produce different hashes, so callers can +// create distinct commits (including divergent siblings) without touching +// files. +func makeEmptyTreeCommit(t *testing.T, repo *git.Repository, parents []plumbing.Hash, msg string) plumbing.Hash { + t.Helper() + emptyTree := object.Tree{} + emptyTreeObj := repo.Storer.NewEncodedObject() + if err := emptyTree.Encode(emptyTreeObj); err != nil { + t.Fatalf("encode empty tree: %v", err) + } + treeHash, err := repo.Storer.SetEncodedObject(emptyTreeObj) + if err != nil { + t.Fatalf("store empty tree: %v", err) + } + sig := object.Signature{Name: "T", Email: "t@example.com", When: time.Unix(0, 0).UTC()} + commit := &object.Commit{ + TreeHash: treeHash, + Message: msg, + Author: sig, + Committer: sig, + ParentHashes: parents, + } + obj := repo.Storer.NewEncodedObject() + if err := commit.Encode(obj); err != nil { + t.Fatalf("encode commit %q: %v", msg, err) + } + hash, err := repo.Storer.SetEncodedObject(obj) + if err != nil { + t.Fatalf("store commit %q: %v", msg, err) + } + return hash +} + +// forceSetTestRef points safelyAdvanceTestRef at hash unconditionally — +// tests deliberately rewind/diverge the ref to set up the scenarios under +// test. +func forceSetTestRef(t *testing.T, repo *git.Repository, hash plumbing.Hash) { + t.Helper() + if err := repo.Storer.SetReference(plumbing.NewHashReference(safelyAdvanceTestRef, hash)); err != nil { + t.Fatalf("SetReference %s: %v", safelyAdvanceTestRef, err) + } +} + +// readTestRef reads safelyAdvanceTestRef and fatals if it is missing. Used +// after a call to SafelyAdvanceLocalRef to inspect the resulting ref state. +func readTestRef(t *testing.T, repo *git.Repository) plumbing.Hash { + t.Helper() + ref, err := repo.Reference(safelyAdvanceTestRef, true) + if err != nil { + t.Fatalf("read %s: %v", safelyAdvanceTestRef, err) + } + return ref.Hash() +} + +func TestSafelyAdvanceLocalRef_LocalMissing_SetsToTarget(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, a); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != a { + t.Errorf("local ref = %s, want %s", got, a) + } +} + +func TestSafelyAdvanceLocalRef_LocalEqualsTarget_NoOp(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + forceSetTestRef(t, repo, a) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, a); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != a { + t.Errorf("local ref = %s, want %s (unchanged)", got, a) + } +} + +func TestSafelyAdvanceLocalRef_LocalAhead_NoOp(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + b := makeEmptyTreeCommit(t, repo, []plumbing.Hash{a}, "B") + forceSetTestRef(t, repo, b) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, a); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != b { + t.Errorf("locally-ahead ref must not rewind: got %s, want %s", got, b) + } +} + +func TestSafelyAdvanceLocalRef_LocalBehind_FastForwards(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + a := makeEmptyTreeCommit(t, repo, nil, "A") + b := makeEmptyTreeCommit(t, repo, []plumbing.Hash{a}, "B") + forceSetTestRef(t, repo, a) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, b); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != b { + t.Errorf("local ref should have fast-forwarded: got %s, want %s", got, b) + } +} + +// Before this fix, SafelyAdvanceLocalRef overwrote any local ref whose history +// the target couldn't reach. That destroyed unpushed local commits on orphan +// refs the CLI maintains (entire/checkpoints/v1, the V2 main ref) whenever a +// fetch landed sibling commits — e.g. checkpoint metadata produced on another +// machine. The objects survived in the loose-objects pool until git gc, but +// the branch ref no longer pointed at them. +func TestSafelyAdvanceLocalRef_Diverged_PreservesLocal(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + base := makeEmptyTreeCommit(t, repo, nil, "base") + localTip := makeEmptyTreeCommit(t, repo, []plumbing.Hash{base}, "local-only-work") + targetTip := makeEmptyTreeCommit(t, repo, []plumbing.Hash{base}, "remote-only-work") + forceSetTestRef(t, repo, localTip) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, targetTip); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != localTip { + t.Errorf("diverged local ref must be preserved: got %s, want %s (overwrote to target %s?)", + got, localTip, targetTip) + } +} + +// Unrelated histories (no common ancestor at all) are a strict subset of +// "diverged" — neither ref is an ancestor of the other. Confirms the +// protection extends to that case. +func TestSafelyAdvanceLocalRef_UnrelatedHistory_PreservesLocal(t *testing.T) { + t.Parallel() + repo := newSafelyAdvanceTestRepo(t) + localOnly := makeEmptyTreeCommit(t, repo, nil, "local-orphan") + targetOnly := makeEmptyTreeCommit(t, repo, nil, "target-orphan") + forceSetTestRef(t, repo, localOnly) + + if err := SafelyAdvanceLocalRef(context.Background(), repo, safelyAdvanceTestRef, targetOnly); err != nil { + t.Fatalf("SafelyAdvanceLocalRef: %v", err) + } + if got := readTestRef(t, repo); got != localOnly { + t.Errorf("unrelated-history local ref must be preserved: got %s, want %s", got, localOnly) + } +}