Skip to content

feat(git): resolve pull conflicts with the shared ladder, never with markers - #1485

Draft
ocavuebot wants to merge 4 commits into
git_N2_ladder_totalfrom
git_P3.2_git_ladder
Draft

ocavuebot wants to merge 4 commits into
git_N2_ladder_totalfrom
git_P3.2_git_ladder

Conversation

@ocavuebot

@ocavuebot ocavuebot commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Stacked on #1480 (the ladder without markers), which is stacked on #1477.

A pull that hit a text conflict used to let libgit2 write the note with conflict markers and commit that. With this PR the Git side runs the same resolution ladder as the iCloud sweep (conflict::ladder), so a note both devices edited lands as merged text and no conflict markers are ever committed:

  • merge_remote checks the merge out with use_ours instead of marker-style conflicts, so a conflicted note is never on disk with markers, not even briefly. resolve_conflicts then reads the ancestor, our, and their blobs, runs the ladder (three-way merge, key-wise frontmatter, append-union, word-level total merge as the last resort), writes the result, and stages it. Nothing is added to conflicted_paths for text; the originals are in the merge commit's two parents.
  • The ladder orders the sides by (modified_ms, content), and modified_ms is the commit time here, shared data. The device that merges the mirror pair produces the same bytes.
  • Binary files still keep both copies, edit-vs-delete still restores the edit, and those still land in conflicted_paths (the message says "files to review").
  • docs/git-backup-safety.md S7 and two engine comments updated.

Tests

  • conflicting_edits_are_merged_without_markers (replaces the marker test): both lines present, nothing to review, the merge is staged as written, the repo is clean, the other device fast-forwards to the same bytes.
  • daily_appends_on_two_devices_merge_by_union, frontmatter_only_conflicts_merge_key_wise.
  • both_devices_merge_the_same_pair_to_the_same_bytes: B gets A's branch through an anonymous fetch so each device merges the pair from the opposite side; head_blob matches.
  • same_title_created_on_two_devices_merges_both_bodies (was a review-conflict test).
  • cargo test -p reflect-open: 414 passed. Clippy clean.

Written by Claude Fable 5.1.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by Sourcery

Merge Git pull conflicts through the shared resolution ladder and commit marker-free, deterministic note content.

New Features:

  • Resolve text conflicts during Git pulls with the shared sync resolution ladder, producing merged notes without conflict markers.
  • Ensure both devices deterministically converge on identical merged content.
  • Preserve binary conflict copies and edited files in edit-vs-delete conflicts for user review.

Bug Fixes:

  • Prevent Git pulls from committing conflict-marker content into notes.
  • Prevent text conflicts from being reported as unresolved review conflicts.

Enhancements:

  • Use atomic writes for resolved pull content while retaining both original versions in merge history.

Documentation:

  • Update Git backup safety guidance to describe marker-free text merging and deterministic conflict handling.

Tests:

  • Add coverage for text, append, frontmatter, same-title, and cross-device deterministic merges without markers.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@ocavuebot ocavuebot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review by Claude Fable 5.1 (Anthropic model claude-fable-5-1). Static review of the diff and the call chains it touches, against the sync design (total merge, archive before any non-exact result, one sync channel per graph). No local tests run; CI was green at review time.

Reviewed head d3fcfd8c75ae2359be48996483b63f7e114af52a (one commit on git_N2_ladder_total, no stale N2 commits).

Verdict: approve-worthy once #1480 is fixed (stacked on it). The notes below are cleanups.

What I confirmed: determinism holds across devices. commit_ms is seconds × 1000, the ladder orders by (modified_ms, content), and total::merge sorts by content again, so two commits in the same second still yield identical bytes; the test both_devices_merge_the_same_pair_to_the_same_bytes creates both commits within one second, so it exercises the tie. Blobs come from the index conflict entries, not the working tree. Non-UTF-8 on either side falls to the binary copy path. libgit2 GIT_CHECKOUT_USE_OURS writes ours for both-sides conflicts, so marker text never reaches disk. Edit-vs-delete and rename 1→2 are unchanged. conflicted_paths is empty for text, so the commit message is the plain merge message.

Inherits the #1480 rule-7 note: a line deleted on A while B appended right after it comes back after the pull. Both originals are in the merge commit’s parents, so it is recoverable, but it is a silent revert of a deletion.

Stale comments (unchanged lines, so not inline): git/mod.rs:201-202 still says “conflicts are committed into the notes as labeled markers”; git/tests.rs:986-987 still says “must surface it through the existing marker flow”.

}

fn write_file(root: &Path, rel: &str, bytes: &[u8]) -> AppResult<()> {
let target = root.join(rel);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[P3] write_file is a plain fs::write, and git_merge_remote takes no graph lock in this stack. Same as master, but merged note text now goes through here too, so the watcher or an iCloud sweep can observe a half-written note. Use crate::fs::atomic_write_bytes, and let #1471 wrap merge_remote in the graph lock.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done in d875ebd: write_file goes through crate::fs::atomic_write_bytes. The graph lock around merge_remote is #1471's.

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.

2 participants