Skip to content

refactor: unify tree modification to use ApplyTreeChanges#573

Draft
dvydra wants to merge 2 commits intomainfrom
refactor/unify-tree-operations
Draft

refactor: unify tree modification to use ApplyTreeChanges#573
dvydra wants to merge 2 commits intomainfrom
refactor/unify-tree-operations

Conversation

@dvydra
Copy link
Contributor

@dvydra dvydra commented Mar 2, 2026

Summary

  • Adds DiffTrees helper to checkpoint/parse_tree.go that computes []TreeChange from two flat entry maps
  • Converts 2 call sites from FlattenTree + BuildTreeFromEntries (flatten entire tree, rebuild from scratch) to DiffTrees + ApplyTreeChanges (surgical tree modification):
    • fetchAndMergeSessionsCommon in push_common.go — separate flattens + surgical apply
    • DeleteOrphanedCheckpoints in cleanup.go — flatten to discover paths + surgical delete
  • All tree modification sites now use the same primitive as WriteCommitted, reducing cognitive overhead
  • cherryPickOnto in metadata_reconcile.go should be converted separately when PR fix: detect and repair disconnected metadata branches at read/write time #533 merges

Test plan

  • 7 new unit tests for DiffTrees including equivalence test with ApplyTreeChanges
  • mise run fmt && mise run lint passes clean
  • mise run test:ci passes (unit + integration)

🤖 Generated with Claude Code

Three call sites modified git trees using FlattenTree + BuildTreeFromEntries
(flatten entire tree, mutate map, rebuild from scratch). A fourth site
(WriteCommitted) already used the surgical ApplyTreeChanges primitive.

Add DiffTrees helper that computes []TreeChange from two flat entry maps,
then convert all three flatten+rebuild sites to use DiffTrees + ApplyTreeChanges:

- cherryPickOnto (metadata_reconcile.go): eliminates tip tree flatten entirely
- fetchAndMergeSessionsCommon (push_common.go): separate flattens + surgical apply
- DeleteOrphanedCheckpoints (cleanup.go): flatten to discover paths + surgical delete

All sites now use the same tree-modification primitive, reducing cognitive
overhead from having two different approaches for the same operation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 0e7526b4fd6b
Copilot AI review requested due to automatic review settings March 2, 2026 06:33
@cursor
Copy link

cursor bot commented Mar 2, 2026

PR Summary

Medium Risk
Moderate risk because it changes how git trees are merged/deleted in session log syncing, checkpoint cleanup, and metadata-branch repair; bugs here could drop or fail to merge metadata. Added unit tests reduce risk, but behavior differences around deletions/empty dirs remain the main concern.

Overview
Introduces checkpoint.DiffTrees to compute file-level []TreeChange deltas between two flattened trees.

Refactors session log merge (fetchAndMergeSessionsCommon) and orphaned checkpoint cleanup to stop rebuilding full trees and instead apply computed deltas/deletions via checkpoint.ApplyTreeChanges.

Adds ReconcileDisconnectedMetadataBranch to detect a disconnected local/remote entire/checkpoints/v1 history (via git merge-base) and repair it by cherry-picking local commit deltas onto the remote tip using DiffTrees + ApplyTreeChanges.

Adds focused unit tests for DiffTrees, including an equivalence test ensuring DiffTrees + ApplyTreeChanges matches a flatten+rebuild result (when no full-directory deletions are involved).

Written by Cursor Bugbot for commit 913f497. Configure here.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors session/checkpoint tree mutation to rely on a single “tree surgery” primitive (ApplyTreeChanges) and introduces DiffTrees to compute []TreeChange deltas from flattened entry maps, reducing full-tree rebuilds across strategy operations.

Changes:

  • Added checkpoint.DiffTrees(base, target) to compute file-level add/modify/delete changes from flattened trees.
  • Updated session-log merge (fetchAndMergeSessionsCommon) and cleanup (DeleteOrphanedCheckpoints) to apply targeted tree changes via ApplyTreeChanges.
  • Added metadata branch reconciliation logic to cherry-pick disconnected local metadata commits onto the remote tip using DiffTrees + ApplyTreeChanges, plus unit tests for DiffTrees.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cmd/entire/cli/strategy/push_common.go Switches remote/local session-log merging from flatten+rebuild to diff+apply tree surgery.
cmd/entire/cli/strategy/metadata_reconcile.go Adds disconnected metadata-branch reconciliation via cherry-pick using tree diffs and surgical apply.
cmd/entire/cli/strategy/cleanup.go Converts orphaned checkpoint deletion to generate TreeChange deletions and apply surgically.
cmd/entire/cli/checkpoint/parse_tree.go Introduces DiffTrees helper to produce []TreeChange from two flattened entry maps.
cmd/entire/cli/checkpoint/parse_tree_test.go Adds unit tests validating DiffTrees behavior and equivalence with ApplyTreeChanges.

Comment on lines +182 to +184
// DiffTrees computes changes to transform local into remote (remote wins on collision)
changes := checkpoint.DiffTrees(localEntries, remoteEntries)

Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this merge path we need to preserve local-only session log entries and add remote entries on top. Using DiffTrees(localEntries, remoteEntries) computes deletions for any path present locally but absent remotely, which can drop local session logs when the remote isn't a strict superset (common after a non-fast-forward push rejection). Consider diffing against a union map (local + remote, with remote overwriting collisions) or otherwise filtering out deletions so the result is an additive/overwrite merge rather than “make local identical to remote”.

Suggested change
// DiffTrees computes changes to transform local into remote (remote wins on collision)
changes := checkpoint.DiffTrees(localEntries, remoteEntries)
// Build a union of local and remote entries where remote overwrites on collision.
// This ensures we only add/update entries and never delete local-only session logs.
unionEntries := make(map[string]object.TreeEntry, len(localEntries)+len(remoteEntries))
for path, entry := range localEntries {
unionEntries[path] = entry
}
for path, entry := range remoteEntries {
unionEntries[path] = entry
}
// DiffTrees computes changes to transform local into the union (remote wins on collision),
// preserving local-only entries.
changes := checkpoint.DiffTrees(localEntries, unionEntries)

Copilot uses AI. Check for mistakes.
Comment on lines +295 to +302
for path, entry := range target {
baseEntry, exists := base[path]
if !exists || baseEntry.Hash != entry.Hash {
changes = append(changes, TreeChange{
Path: path,
Entry: &object.TreeEntry{Name: entry.Name, Mode: entry.Mode, Hash: entry.Hash},
})
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiffTrees only compares TreeEntry.Hash when deciding whether an entry changed. Git tree entries also encode the file mode (e.g., regular vs executable), so a pure mode change with identical blob content would be missed and not applied. Include entry.Mode (and any other relevant metadata) in the equality check so mode-only changes produce a TreeChange.

Copilot uses AI. Check for mistakes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 2df5313f9ab9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants