8a4caa17

Refactor patch merge to use git's native merge path; dedup DAG tree-building

a73x   2026-03-22 08:21

Replace manual ref-update + reset(Hard) in patch merge with git's native
merge when HEAD is on the target branch. Fall back to plumbing for bare
ref updates when on a different branch. Extract build_event_tree helper
in dag.rs to consolidate 4 repeated tree-building blocks.

Closes issues 8ade92ed, 3a5a87af.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---

diff --git a/src/dag.rs b/src/dag.rs
index 9e7b577..e9e42be 100644
--- a/src/dag.rs
+++ b/src/dag.rs
@@ -11,6 +11,24 @@ const MANIFEST_JSON: &[u8] = br#"{"version":1,"format":"git-collab"}"#;
/// Maximum allowed size for an event.json blob (1 MB).
pub const MAX_EVENT_BLOB_SIZE: usize = 1_048_576;

/// Build an event tree containing event.json, signature, pubkey, and manifest.json blobs.
/// Returns the tree OID.
fn build_event_tree(
    repo: &Repository,
    event_blob: Oid,
    sig_blob: Oid,
    pubkey_blob: Oid,
    manifest_blob: Oid,
) -> Result<Oid, Error> {
    let mut tb = repo.treebuilder(None)?;
    tb.insert("event.json", event_blob, 0o100644)?;
    tb.insert("signature", sig_blob, 0o100644)?;
    tb.insert("pubkey", pubkey_blob, 0o100644)?;
    tb.insert("manifest.json", manifest_blob, 0o100644)?;
    let tree_oid = tb.write()?;
    Ok(tree_oid)
}

/// Walk the entire DAG reachable from `tip` and return the maximum clock value.
/// Returns 0 if the DAG is empty or all events have clock 0 (pre-migration).
pub fn max_clock(repo: &Repository, tip: Oid) -> Result<u64, Error> {
@@ -69,12 +87,7 @@ pub fn create_root_event(
    let pubkey_blob = repo.blob(detached.pubkey.as_bytes())?;
    let manifest_blob = repo.blob(MANIFEST_JSON)?;

    let mut tb = repo.treebuilder(None)?;
    tb.insert("event.json", event_blob, 0o100644)?;
    tb.insert("signature", sig_blob, 0o100644)?;
    tb.insert("pubkey", pubkey_blob, 0o100644)?;
    tb.insert("manifest.json", manifest_blob, 0o100644)?;
    let tree_oid = tb.write()?;
    let tree_oid = build_event_tree(repo, event_blob, sig_blob, pubkey_blob, manifest_blob)?;
    let tree = repo.find_tree(tree_oid)?;

    let sig = author_signature(&event.author)?;
@@ -106,12 +119,7 @@ pub fn append_event(
    let pubkey_blob = repo.blob(detached.pubkey.as_bytes())?;
    let manifest_blob = repo.blob(MANIFEST_JSON)?;

    let mut tb = repo.treebuilder(None)?;
    tb.insert("event.json", event_blob, 0o100644)?;
    tb.insert("signature", sig_blob, 0o100644)?;
    tb.insert("pubkey", pubkey_blob, 0o100644)?;
    tb.insert("manifest.json", manifest_blob, 0o100644)?;
    let tree_oid = tb.write()?;
    let tree_oid = build_event_tree(repo, event_blob, sig_blob, pubkey_blob, manifest_blob)?;
    let tree = repo.find_tree(tree_oid)?;

    let sig = author_signature(&event.author)?;
@@ -230,12 +238,7 @@ pub fn reconcile(
    let pubkey_blob = repo.blob(detached.pubkey.as_bytes())?;
    let manifest_blob = repo.blob(MANIFEST_JSON)?;

    let mut tb = repo.treebuilder(None)?;
    tb.insert("event.json", event_blob, 0o100644)?;
    tb.insert("signature", sig_blob, 0o100644)?;
    tb.insert("pubkey", pubkey_blob, 0o100644)?;
    tb.insert("manifest.json", manifest_blob, 0o100644)?;
    let tree_oid = tb.write()?;
    let tree_oid = build_event_tree(repo, event_blob, sig_blob, pubkey_blob, manifest_blob)?;
    let tree = repo.find_tree(tree_oid)?;

    let sig = author_signature(merge_author)?;
@@ -290,12 +293,7 @@ pub fn migrate_clocks(
        let pubkey_blob = repo.blob(detached.pubkey.as_bytes())?;
        let manifest_blob = repo.blob(MANIFEST_JSON)?;

        let mut tb = repo.treebuilder(None)?;
        tb.insert("event.json", event_blob, 0o100644)?;
        tb.insert("signature", sig_blob, 0o100644)?;
        tb.insert("pubkey", pubkey_blob, 0o100644)?;
        tb.insert("manifest.json", manifest_blob, 0o100644)?;
        let tree_oid = tb.write()?;
        let tree_oid = build_event_tree(repo, event_blob, sig_blob, pubkey_blob, manifest_blob)?;
        let tree = repo.find_tree(tree_oid)?;

        let sig = author_signature(&migrated.author)?;
diff --git a/src/patch.rs b/src/patch.rs
index 86528d9..1b545cc 100644
--- a/src/patch.rs
+++ b/src/patch.rs
@@ -416,15 +416,72 @@ pub fn merge(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::er

    let base_ref = format!("refs/heads/{}", p.base_ref);
    let base_oid = repo.refname_to_id(&base_ref)?;

    // Fast-forward: the base must be an ancestor of head
    let head_oid = head_commit.id();
    if repo.graph_descendant_of(head_oid, base_oid)? {
        repo.reference(&base_ref, head_oid, true, "collab: merge patch")?;
    } else if head_oid == base_oid {

    // Check if HEAD is on the target branch — determines whether we can
    // use git's native merge (which updates ref + index + working tree in
    // one shot) or must fall back to plumbing for a bare ref update.
    let on_base_branch = repo
        .head()
        .ok()
        .map(|r| r.is_branch() && r.shorthand() == Some(&p.base_ref))
        .unwrap_or(false);

    if head_oid == base_oid {
        // Already at the same point, nothing to do
    } else if repo.graph_descendant_of(head_oid, base_oid)? {
        // Fast-forward
        repo.reference(&base_ref, head_oid, true, "collab: fast-forward merge")?;
        if on_base_branch {
            repo.checkout_head(Some(
                git2::build::CheckoutBuilder::new().force(),
            ))?;
        }
    } else if on_base_branch {
        // Non-fast-forward with HEAD on base branch — use git's native merge.
        // repo.merge() updates ref, index, and working tree in one operation.
        let annotated = repo.find_annotated_commit(head_oid)?;
        let mut checkout = git2::build::CheckoutBuilder::new();
        checkout.force().allow_conflicts(true);
        if let Err(e) = repo.merge(&[&annotated], None, Some(&mut checkout)) {
            if e.code() == git2::ErrorCode::Conflict {
                let _ = repo.cleanup_state();
                return Err(git2::Error::from_str(
                    "merge has conflicts; resolve manually then revise the patch",
                )
                .into());
            }
            return Err(e.into());
        }
        {
            let index = repo.index()?;
            if index.has_conflicts() {
                repo.cleanup_state()?;
                return Err(git2::Error::from_str(
                    "merge has conflicts; resolve manually then revise the patch",
                )
                .into());
            }
        }
        // Create the merge commit
        let base_commit = repo.find_commit(base_oid)?;
        let author = get_author(repo)?;
        let sig = crate::identity::author_signature(&author)?;
        let mut index = repo.index()?;
        let tree_oid = index.write_tree()?;
        let tree = repo.find_tree(tree_oid)?;
        let msg = format!("Merge patch {:.8}: {}", id, p.title);
        repo.commit(
            Some("HEAD"),
            &sig,
            &sig,
            &msg,
            &tree,
            &[&base_commit, &head_commit],
        )?;
        repo.cleanup_state()?;
    } else {
        // Not a fast-forward — create a merge commit on the base branch
        // Non-fast-forward with HEAD on a different branch — plumbing only
        let base_commit = repo.find_commit(base_oid)?;
        let author = get_author(repo)?;
        let sig = crate::identity::author_signature(&author)?;
@@ -448,20 +505,6 @@ pub fn merge(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::er
        )?;
    }

    // Update working tree + index if HEAD is on the target branch
    if let Ok(head_ref) = repo.head() {
        if head_ref.is_branch()
            && head_ref.shorthand() == Some(&p.base_ref)
        {
            let target_commit = head_ref.peel_to_commit()?;
            repo.reset(
                target_commit.as_object(),
                git2::ResetType::Hard,
                None,
            )?;
        }
    }

    // Record the merge event in the patch DAG
    let author = get_author(repo)?;
    let event = Event {
diff --git a/tests/collab_test.rs b/tests/collab_test.rs
index 3f4d915..022742a 100644
--- a/tests/collab_test.rs
+++ b/tests/collab_test.rs
@@ -982,6 +982,109 @@ fn test_merge_conflicting_patch_reports_error() {
    assert!(err_msg.contains("conflict"), "error should mention conflicts");
}

#[test]
fn test_merge_ff_updates_working_tree_when_on_base_branch() {
    // When HEAD is on the base branch, a fast-forward merge should update
    // the working tree and index (not just the ref).
    let tmp = TempDir::new().unwrap();
    let repo = init_repo(tmp.path(), &alice());
    make_initial_commit(&repo, "main");

    // Set HEAD to main and checkout so working tree matches
    repo.set_head("refs/heads/main").unwrap();
    repo.checkout_head(Some(git2::build::CheckoutBuilder::new().force())).unwrap();

    let tip = add_commit_on_branch(&repo, "main", "base.rs", b"base");
    // Sync working tree to main
    repo.checkout_head(Some(git2::build::CheckoutBuilder::new().force())).unwrap();

    repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap();
    add_commit_on_branch(&repo, "feat", "feat.rs", b"feature code");

    let id = patch::create(&repo, "FF merge test", "", "main", "feat", None).unwrap();
    patch::merge(&repo, &id).unwrap();

    // Working tree should contain feat.rs
    assert!(
        tmp.path().join("feat.rs").exists(),
        "working tree should contain feat.rs after ff merge on current branch"
    );
}

#[test]
fn test_merge_non_ff_updates_working_tree_when_on_base_branch() {
    // When HEAD is on the base branch and branches have diverged,
    // the merge should update the working tree with the merged result.
    let tmp = TempDir::new().unwrap();
    let repo = init_repo(tmp.path(), &alice());
    make_initial_commit(&repo, "main");

    repo.set_head("refs/heads/main").unwrap();
    repo.checkout_head(Some(git2::build::CheckoutBuilder::new().force())).unwrap();

    let tip = add_commit_on_branch(&repo, "main", "base.rs", b"base");
    repo.checkout_head(Some(git2::build::CheckoutBuilder::new().force())).unwrap();

    repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap();
    add_commit_on_branch(&repo, "feat", "feat.rs", b"feature code");
    // Diverge main so it's not a fast-forward
    add_commit_on_branch(&repo, "main", "main_only.rs", b"main only");
    repo.checkout_head(Some(git2::build::CheckoutBuilder::new().force())).unwrap();

    let id = patch::create(&repo, "Non-FF merge test", "", "main", "feat", None).unwrap();
    patch::merge(&repo, &id).unwrap();

    // Working tree should contain both files
    assert!(
        tmp.path().join("feat.rs").exists(),
        "working tree should contain feat.rs after non-ff merge"
    );
    assert!(
        tmp.path().join("main_only.rs").exists(),
        "working tree should still contain main_only.rs after non-ff merge"
    );

    // Should be a merge commit (2 parents)
    let main_tip = repo.refname_to_id("refs/heads/main").unwrap();
    let main_commit = repo.find_commit(main_tip).unwrap();
    assert_eq!(main_commit.parent_count(), 2, "should be a merge commit");
}

#[test]
fn test_merge_ff_does_not_touch_working_tree_when_on_different_branch() {
    // When HEAD is NOT on the base branch, merge should update the ref
    // but not touch the working tree.
    let tmp = TempDir::new().unwrap();
    let repo = init_repo(tmp.path(), &alice());
    make_initial_commit(&repo, "main");

    let tip = add_commit_on_branch(&repo, "main", "base.rs", b"base");
    repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap();
    add_commit_on_branch(&repo, "feat", "feat.rs", b"feature code");

    // Create a separate branch and switch HEAD to it
    repo.branch("other", &repo.find_commit(tip).unwrap(), false).unwrap();
    repo.set_head("refs/heads/other").unwrap();
    repo.checkout_head(Some(git2::build::CheckoutBuilder::new().force())).unwrap();

    let id = patch::create(&repo, "Off-branch merge", "", "main", "feat", None).unwrap();
    patch::merge(&repo, &id).unwrap();

    // Ref should be updated
    let base_tip = repo.refname_to_id("refs/heads/main").unwrap();
    let feat_tip = repo.refname_to_id("refs/heads/feat").unwrap();
    assert!(
        repo.graph_descendant_of(base_tip, feat_tip).unwrap() || base_tip == feat_tip,
        "base should contain the feature branch"
    );

    // Working tree should NOT have feat.rs (we're on 'other')
    assert!(
        !tmp.path().join("feat.rs").exists(),
        "working tree should not be modified when HEAD is on a different branch"
    );
}

// ---------------------------------------------------------------------------
// Phase 6: US4 — Implicit Revision (T027)
// ---------------------------------------------------------------------------