address review feedback for streamline-mirroring refactor · Entire

address review feedback for streamline-mirroring refactor

ab023ac→main·

pfleidi·1mo ago·12 files·+99 added/-52 removed

Required: - explain.loadPrimaryMetadataRootTree: drop the Read != Primary clause from the origin-fallback gate. The function reads Primary's tree, so whether reads bootstrap from origin is irrelevant. - strategy: inline getCommittedReadStore (one-line wrapper) into its sole caller in manual_commit_condensation.

Improvements: - CommittedRefs.PrimaryFetchableFromOrigin: require Primary.IsBranch(). Non-branch refs don't get refs/remotes/origin/ shadows. - CommittedRefs.ReadBootstrappableFromOrigin: extract the predicate used at three sites (resume bootstrap gate, resume remote-metadata check, GitStore fallback) so the gate has a name. - attach, explain: drop the redundant outer HasMirror() guard around MirrorCommittedMetadataRef; the function already short-circuits. - push_common: inline setLocalAfterSync into fetchAndRebaseSessionsCommon and resolve refs once at the top instead of per advance step. - cleanup: pass already-resolved refs into the final AdvanceCommittedPrimary call instead of re-resolving. - v1_custom_ref_mirror_test: populate Push in v1CustomRefsForTest so the helper matches what the resolver actually produces. - push_common_test: add TestFetchAndRebase_NonBranchRef. - attach.checkpointPresentLocally: route the read through a Primary-bound store so the result reflects the write target, not the configured read ref. - push_common.pushRefIfNeeded: log a Debug when OpenRepository fails instead of silently returning nil.

Sessions

600afaa4321fView transcript

Changes

12

`` 314 unmodified lines

315 316 317 318 319 320 321 318 319 320 321 322 91 unmodified lines

414 415 416 419 417 418 419 420 421 422 423 1 unmodified line

425 426 427 427 428 429 430 431 432 433

314 unmodified lines

\t return fmt.Errorf("failed to write checkpoint: %w", err) \t}\n \t\n \tif refs := opts.committedRefs(ctx); refs.HasMirror() { \t\tif err := strategy.MirrorCommittedMetadataRef(ctx, repo, refs); err != nil { \t\t\treturn fmt.Errorf("checkpoint was written to %s, but failed to mirror to %s: %w", refs.Primary, refs.Mirror, err) \t}\n \tif err := strategy.MirrorCommittedMetadataRef(ctx, repo, refs); err != nil { \t\treturn fmt.Errorf("checkpoint was written to %s, but failed to mirror to %s: %w", refs.Primary, refs.Mirror, err) \t}\n

// Create or update session state. 91 unmodified lines

// checkpointPresentLocally reports whether the checkpoint already exists on // the local primary ref we would write to. Remote-tracking alone is not // enough; see ensureCheckpointAvailable. // enough; see ensureCheckpointAvailable. Reads target Primary directly even // when the configured Read ref differs (e.g. v1.1 mirror) because the // question is "is the write target up to date," not "what's visible to // readers." func checkpointPresentLocally(ctx context.Context, repo *git.Repository, refs cpkg.CommittedRefs, checkpointID id.CheckpointID) (bool, error) { \tif _, err := repo.Reference(refs.Primary, true); err != nil { \t\t// Local ref doesn't exist — treat as "not present locally". We 1 unmodified line

\t\t// ensureCheckpointAvailable's docstring. \t\treturn false, nil //nolint:nilerr // Missing ref is the "absent" signal, not an error. \t}\n \tsummary, err := cpkg.NewGitStore(repo, refs).ReadCommitted(ctx, checkpointID) \tprimaryRefs := refs \tprimaryRefs.Read = refs.Primary \tsummary, err := cpkg.NewGitStore(repo, primaryRefs).ReadCommitted(ctx, checkpointID) \tif err != nil { \t\treturn false, err //nolint:wrapcheck // Caller wraps with checkpoint ID context \t} ``

Mcmd/entire/cli/attach.go+9/-6

`` 1781 unmodified lines

1782 1783 1784 1785 1786 1787 1785 1786 1787 1788 1789 1790 1791 1791 1792 1793 1794

1781 unmodified lines

// getSessionsBranchTree returns the tree object at the configured read ref. // Falls back to origin's remote-tracking ref when Read equals Primary and // Primary is in Push (origin tracks it). When reads target the local-only // mirror, the fallback skips because origin doesn't track the mirror. // Falls back to origin's remote-tracking ref when reads are bootstrappable // from origin. When reads target a local-only mirror, the fallback skips // because origin doesn't track the mirror. func (s *GitStore) getSessionsBranchTree() (*object.Tree, error) { \tref, err := s.repo.Reference(s.refs.Read, true) \tif err != nil { \t\tif s.refs.Read != s.refs.Primary || !s.refs.PrimaryFetchableFromOrigin() { \t\t\tif !s.refs.ReadBootstrappableFromOrigin() { \t\t\t\treturn nil, fmt.Errorf("sessions ref %s not found: %w", s.refs.Read, err) \t\t\t} \t\t\tremoteRefName := plumbing.NewRemoteReferenceName("origin", s.refs.Primary.Short()) ``

Mcmd/entire/cli/checkpoint/committed.go+4/-4

`` 34 unmodified lines

35 36 37 38 39 38 39 40 41 41 42 43 44 45 46 47 48 49 50 51 52 53

34 unmodified lines

// PrimaryFetchableFromOrigin reports whether Primary has an origin-tracking // shadow — i.e. whether bootstrap-from-origin paths can fetch it. True when // Primary appears in Push: we push it, so origin tracks it. // shadow — i.e. whether bootstrap-from-origin paths can fetch it. Only branch // refs in Push get a refs/remotes/origin/ shadow; non-branch refs are // pushed without remote-tracking. func (r CommittedRefs) PrimaryFetchableFromOrigin() bool { \treturn slices.Contains(r.Push, r.Primary) \treturn r.Primary.IsBranch() && slices.Contains(r.Push, r.Primary) }

// ReadBootstrappableFromOrigin reports whether the read ref can be bootstrapped // from origin. True when reads target Primary AND Primary is fetchable from // origin. False when reads target a local-only mirror (origin doesn't track // it), even if Primary is itself fetchable. func (r CommittedRefs) ReadBootstrappableFromOrigin() bool { \treturn r.Read == r.Primary && r.PrimaryFetchableFromOrigin() }

// ResolveCommittedRefs returns the topology for the settings on disk, falling ``

Mcmd/entire/cli/checkpoint/committed_refs.go+12/-3

`` 78 unmodified lines

79 80 81 82 83 84 85

78 unmodified lines

\t{"v1 in push", CommittedRefs{Primary: v1, Push: []plumbing.ReferenceName{v1}}, true}, \t{"primary not in push", CommittedRefs{Primary: custom, Push: []plumbing.ReferenceName{v1}}, false}, \t{"empty push", CommittedRefs{Primary: v1, Push: nil}, false}, \t{"non-branch primary in push", CommittedRefs{Primary: custom, Push: []plumbing.ReferenceName{custom}}, false}, \t} for _, tt := range tests { \tt.Run(tt.name, func(t *testing.T) { ``

Mcmd/entire/cli/checkpoint/committed_refs_test.go+1

`` 825 unmodified lines

826 827 828 829 830 829 830 831 832 833 835 834 835 836 837 94 unmodified lines

932 933 934 936 937 938 939 935 936 937 938 939 940

825 unmodified lines

// loadPrimaryMetadataRootTree reads the tree at refs.Primary, falling back to // origin's remote-tracking ref when reads are bootstrappable from origin // (Read == Primary && Primary in Push). // origin's remote-tracking ref when Primary is pushed. func loadPrimaryMetadataRootTree(ctx context.Context, repo *git.Repository, refs checkpoint.CommittedRefs) (*object.Tree, error) { \tif tree, err := strategy.GetMetadataRefTree(repo, refs.Primary); err == nil { \t\treturn tree, nil \t} \tif refs.Read != refs.Primary || !refs.PrimaryFetchableFromOrigin() { \tif !refs.PrimaryFetchableFromOrigin() { \t\treturn nil, fmt.Errorf("read primary metadata tree %s: ref not found locally", refs.Primary) \t} \ttree, err := strategy.GetRemotePrimaryTree(ctx, repo) 94 unmodified lines

\treturn fmt.Errorf("failed to save summary: %w", err) \n \tif refs := checkpoint.ResolveCommittedRefs(ctx); refs.HasMirror() { \t\tif err := strategy.MirrorCommittedMetadataRef(ctx, store.Repository(), refs); err != nil { \t\t\treturn fmt.Errorf("summary was written to %s, but failed to mirror to %s: %w", refs.Primary, refs.Mirror, err) \t}\n \trefs := checkpoint.ResolveCommittedRefs(ctx) if err := strategy.MirrorCommittedMetadataRef(ctx, store.Repository(), refs); err != nil { \t\treturn fmt.Errorf("summary was written to %s, but failed to mirror to %s: %w", refs.Primary, refs.Mirror, err) \t}

styles := newStatusStyles(w) ``

Mcmd/entire/cli/explain.go+5/-7

`` 190 unmodified lines

191 192 193 194 194 195 196 197 409 unmodified lines

607 608 609 610 610 611 612 613

190 unmodified lines

\tstore.SetBlobFetcher(FetchBlobsByHash)

\trefs := store.Refs() \tif refs.Read == refs.Primary && refs.PrimaryFetchableFromOrigin() { \tif refs.ReadBootstrappableFromOrigin() { \tpromoteRemoteTrackingPrimary(ctx, repo, refs) \t}

409 unmodified lines

) error { \tlogCtx := logging.WithComponent(ctx, "resume.checkRemoteMetadata")

\tif refs.Read != refs.Primary || !refs.PrimaryFetchableFromOrigin() { \tif !refs.ReadBootstrappableFromOrigin() { \t\tfmt.Fprintf(errW, "Checkpoint '%s' found in commit but metadata is not available in %s.\n", checkpointID, refs.Read) \tfmt.Fprintf(errW, "This ref is local-only. Try: entire explain %s\n", checkpointID) \treturn nil ``

Mcmd/entire/cli/resume.go+2/-2

`` 333 unmodified lines

334 335 336 337 337 338 339 340

333 unmodified lines

\t\n \t// Update branch reference and best-effort mirror if err := AdvanceCommittedPrimary(ctx, repo, checkpoint.ResolveCommittedRefs(ctx), commitHash); err != nil { if err := AdvanceCommittedPrimary(ctx, repo, refs, commitHash); err != nil { return nil, nil, fmt.Errorf("failed to update branch: %w", err) } ``

Mcmd/entire/cli/strategy/cleanup.go+1/-1

`` 54 unmodified lines

55 56 57 58 59 60 61 62 63 64 58 59 60

54 unmodified lines

\treturn s.withBlobFetcher(checkpoint.NewGitStore(repo, checkpoint.ResolveCommittedRefs(ctx)))

// getCommittedReadStore returns a store for reading committed checkpoints. // Equivalent to getCheckpointStore today; kept as a separate name to mark // read-path call sites. func (s *ManualCommitStrategy) getCommittedReadStore(ctx context.Context, repo *git.Repository) *checkpoint.GitStore { \treturn s.getCheckpointStore(ctx, repo) }

// NewManualCommitStrategy creates a new manual-commit strategy instance. func NewManualCommitStrategy() *ManualCommitStrategy { \treturn &ManualCommitStrategy{} } ``

Mcmd/entire/cli/strategy/manual_commit.go-7

`` 48 unmodified lines

49 50 51 52 52 53 54 55

48 unmodified lines

\t\tdefer repo.Close()

\t\tWarnIfMetadataDisconnected() \t\tstore := s.getCommittedReadStore(ctx, repo) store := s.getCheckpointStore(ctx, repo)

\tcmitted, err := store.ListCommitted(ctx) if err != nil { ``

Mcmd/entire/cli/strategy/manual_commit_condensation.go+1/-1

`` 31 unmodified lines

32 33 34 35 36 37 38 39 40 374 unmodified lines

415 416 417 415 418 419 420 421 417 418 422 423 424 425 426 427 428 429 430 431 432 51 unmodified lines

484 485 486 476 477 478 479 480 481 482 483 484 485 486 487 488 489 490 491 492 487 488 489

31 unmodified lines

func pushRefIfNeeded(ctx context.Context, target string, ref plumbing.ReferenceName) error { \trepo, err := OpenRepository(ctx) if err != nil { logging.Debug(ctx, "push skipped: open repository failed", slog.String("ref", ref.String()), slog.String("error", err.Error())) return nil }\n\tdefer repo.Close() 374 unmodified lines

}

// Helper to advance ref and clean up the temp fetch ref. When ref is the // configured Primary, the mirror is advanced too (best-effort). // configured Primary, the mirror is advanced too (best-effort); otherwise // the mirror step is skipped because origin doesn't track this ref. refs := checkpoint.ResolveCommittedRefs(ctx) advance := func(hash plumbing.Hash) error { if err := setLocalAfterSync(ctx, repo, ref, hash); err != nil { return err var setErr error if ref == refs.Primary { setErr = AdvanceCommittedPrimary(ctx, repo, refs, hash) } else { setErr = repo.Storer.SetReference(plumbing.NewHashReference(ref, hash)) } if setErr != nil { return fmt.Errorf("failed to update ref %s: %w", ref, setErr) } if usedTempRef { _ = repo.Storer.RemoveReference(fetchedRefName) //nolint:errcheck // cleanup is best-effort } }

// setLocalAfterSync points ref at hash and, when ref is the configured // Primary, best-effort-advances refs.Mirror. Returns an error only if the // local ref update fails; mirror failures are logged. func setLocalAfterSync(ctx context.Context, repo *git.Repository, ref plumbing.ReferenceName, hash plumbing.Hash) error { refs := checkpoint.ResolveCommittedRefs(ctx) if ref == refs.Primary { return AdvanceCommittedPrimary(ctx, repo, refs, hash) } if err := repo.Storer.SetReference(plumbing.NewHashReference(ref, hash)); err != nil { return fmt.Errorf("failed to update ref %s: %w", ref, err) } logging.Debug(ctx, "committed-ref mirror skipped after sync: ref is not the configured primary", slog.String("ref", ref.String()), slog.String("primary", refs.Primary.String())) return nil }

// getMergeBase returns the merge base hash of two commits, or an error if they // have no common ancestor. func getMergeBase(ctx context.Context, repoPath, hashA, hashB string) (plumbing.Hash, error) { ``

Mcmd/entire/cli/strategy/push_common.go+14/-20

`` 221 unmodified lines

222 223 224 225 226 227 228 229 230 231 232 233 234 235 236 237 238 239 240 241 242 243 244 245 246 247 248 249 250 251 252 253 254 255 256 257 258 259 260 261 262 263 264 265 266 267 268 269 270 271 272 273 274

221 unmodified lines

}\n }

// TestFetchAndRebase_NonBranchRef verifies the fetch+rebase wiring accepts a // non-branch ref (e.g. refs/entire/checkpoints/v1.1). Today's resolver doesn't // emit non-branch refs in CommittedRefs.Push, but the helper must remain // correct when one is wired in. // // Not parallel: uses t.Chdir() (required for OpenRepository). func TestFetchAndRebase_NonBranchRef(t *testing.T) { ctx := context.Background()

tmpDir := setupRepoWithCheckpointBranch(t)

// Point a non-branch ref at HEAD locally. repo, err := git.PlainOpen(tmpDir) require.NoError(t, err) head, err := repo.Head() require.NoError(t, err) customRef := plumbing.ReferenceName("refs/entire/checkpoints/synthetic") require.NoError(t, repo.Storer.SetReference(plumbing.NewHashReference(customRef, head.Hash())))

// Bare remote that has the same ref at the same hash so the fetch+rebase // resolves to a no-op fast-forward (no rebase work required). bareDir := t.TempDir(); for _, args := range [][]string{{"init", "--bare"},} { c := exec.CommandContext(ctx, "git", args...); c.Dir = bareDir; c.Env = testutil.GitIsolatedEnv(); if out, err := c.CombinedOutput(); err != nil { t.Fatalf("git %v failed: %v\n%s", args, err, out); } } bareRepo, err := git.PlainOpen(bareDir); require.NoError(t, err); require.NoError(t, bareRepo.Storer.SetReference(plumbing.NewHashReference(customRef, head.Hash())));

t.Chdir(tmpDir);

require.NoError(t, fetchAndRebaseSessionsCommon(ctx, "file://"+bareDir, customRef), "fetchAndRebaseSessionsCommon should accept a non-branch ref");

// The local ref should remain at the same hash. got, err := repo.Reference(customRef, true); require.NoError(t, err); assert.Equal(t, head.Hash(), got.Hash()); }

// TestFetchAndRebase_DivergedBranches verifies that when local and remote // metadata branches have diverged (shared ancestor, different commits on each), // fetchAndRebaseSessionsCommon produces a linear history (no merge commits)

``

Mcmd/entire/cli/strategy/push_common_test.go+47

`` 100 unmodified lines

101 102 103 104 105 105 106 107 108 109 110 111 112

100 unmodified lines

}

func v1CustomRefsForTest() checkpoint.CommittedRefs { v1Branch := plumbing.NewBranchReferenceName(paths.MetadataBranchName); return checkpoint.CommittedRefs{ Primary: plumbing.NewBranchReferenceName(paths.MetadataBranchName), Primary: v1Branch, Read: plumbing.ReferenceName(paths.MetadataRefName), Mirror: plumbing.ReferenceName(paths.MetadataRefName), Push: []plumbing.ReferenceName{v1Branch}, } } ``

Mcmd/entire/cli/strategy/v1_custom_ref_mirror_test.go+3/-1