From 1c993fbeb5d88d7e9ec370faf19a44e185f6e28b Mon Sep 17 00:00:00 2001 From: Nicolas Arnaud-Cormos Date: Tue, 24 Mar 2026 13:17:57 +0100 Subject: [PATCH] fix(update): handle inverted merge parent ordering in weave topology Merge commits with inverted parent ordering (feature as 1st parent, main as 2nd) caused walk_first_parent_line to follow the feature branch back to root instead of staying on the integration line. This produced a rebase todo replaying the entire repo history, leading to conflicts. Fix walk_first_parent_line to detect which parent leads to the merge base (via graph_descendant_of) and swap if needed. Fix walk_branch_commits to use merge_base(tip, stop) as the actual stop point for branches forked before the merge base. Together, these changes correctly identify the feature branch commits as already upstream, drop them, and produce clean linear history. Co-Authored-By: Claude Opus 4.6 --- src/update_test.rs | 173 +++++++++++++++++++++++++++++++++++++++++++++ src/weave.rs | 89 ++++++++++++++++++----- 2 files changed, 244 insertions(+), 18 deletions(-) diff --git a/src/update_test.rs b/src/update_test.rs index feda2f6..d34df4b 100644 --- a/src/update_test.rs +++ b/src/update_test.rs @@ -777,3 +777,176 @@ fn update_handles_fully_cherry_picked_branch() { ); } } + +/// When the integration branch has a merge with inverted parent ordering +/// (feature as 1st parent, main as 2nd parent) and the feature branch was +/// already merged upstream, the update should drop the redundant merge and +/// produce linear history — not replay all feature commits and conflict. +/// +/// Remote topology: +/// R ── C1 ────────────── merge_up(C1,F1) ── C2(feature.txt="feat v2") ── C3 +/// └── F1(feature.txt="feat") ──┘ +/// +/// Integration topology (before update): +/// merge_local(F1, C1) ← HEAD [inverted: 1st=F1, 2nd=C1] +/// +/// Without the fix, walk_first_parent_line follows F1 → R → root (never +/// reaching the merge base). The fallback rebase replays F1 on top of C3, +/// which CONFLICTS because F1 writes feature.txt="feat" while C3 already +/// has feature.txt="feat v2" (modified in C2 after the upstream merge). +/// +/// With the fix, inverted parents are detected and swapped. F1 goes to a +/// branch section. filter_upstream_commits sees F1 is already in upstream +/// (via merge_up) and drops it. Empty section → merge removed → linear. +#[test] +fn update_handles_inverted_parent_merge_on_integration() { + let test_repo = TestRepo::new_with_remote(); + let remote_path = test_repo.remote_path().unwrap(); + let sig = Signature::now("Test", "test@test.com").unwrap(); + + // Build remote: R ── C1 ── merge_up(C1,F1) ── C2 + // └── F1 ──┘ + let (c1_oid, f1_oid) = { + let rr = Repository::open_bare(&remote_path).unwrap(); + let root_oid = rr + .find_branch("main", BranchType::Local) + .unwrap() + .get() + .target() + .unwrap(); + let root = rr.find_commit(root_oid).unwrap(); + + // C1: adds main.txt + let mut b = rr.treebuilder(Some(&root.tree().unwrap())).unwrap(); + b.insert("main.txt", rr.blob(b"main").unwrap(), 0o100644) + .unwrap(); + let c1_tree = rr.find_tree(b.write().unwrap()).unwrap(); + let c1_oid = rr + .commit( + Some("refs/heads/main"), + &sig, + &sig, + "C1", + &c1_tree, + &[&root], + ) + .unwrap(); + let c1 = rr.find_commit(c1_oid).unwrap(); + + // F1: adds feature.txt (forked from root, before C1) + let mut b = rr.treebuilder(Some(&root.tree().unwrap())).unwrap(); + b.insert("feature.txt", rr.blob(b"feat").unwrap(), 0o100644) + .unwrap(); + let f1_tree = rr.find_tree(b.write().unwrap()).unwrap(); + let f1_oid = rr + .commit(None, &sig, &sig, "F1", &f1_tree, &[&root]) + .unwrap(); + let f1 = rr.find_commit(f1_oid).unwrap(); + + // merge_up: normal merge of F1 into C1 + let mut b = rr.treebuilder(Some(&c1_tree)).unwrap(); + b.insert("feature.txt", rr.blob(b"feat").unwrap(), 0o100644) + .unwrap(); + let mt = rr.find_tree(b.write().unwrap()).unwrap(); + let mu_oid = rr + .commit(None, &sig, &sig, "Merge F1", &mt, &[&c1, &f1]) + .unwrap(); + rr.reference("refs/heads/main", mu_oid, true, "merge") + .unwrap(); + + // C2: post-merge work — modifies feature.txt so that replaying + // F1 (which writes "feat") on top of C2 (which has "feat v2") + // would CONFLICT without the fix. + let mu = rr.find_commit(mu_oid).unwrap(); + let mut b = rr.treebuilder(Some(&mu.tree().unwrap())).unwrap(); + b.insert("feature.txt", rr.blob(b"feat v2").unwrap(), 0o100644) + .unwrap(); + let c2_tree = rr.find_tree(b.write().unwrap()).unwrap(); + rr.commit(Some("refs/heads/main"), &sig, &sig, "C2", &c2_tree, &[&mu]) + .unwrap(); + + (c1_oid, f1_oid) + }; + + // Fetch remote state + test_repo + .repo + .find_remote("origin") + .unwrap() + .fetch(&["main"], None, None) + .unwrap(); + + let c1 = test_repo.repo.find_commit(c1_oid).unwrap(); + let f1 = test_repo.repo.find_commit(f1_oid).unwrap(); + + // Set integration to C1 (merge base) + test_repo + .repo + .reference("refs/heads/integration", c1_oid, true, "to C1") + .unwrap(); + + // Create INVERTED merge on integration: F1 (1st) + C1 (2nd) + let mut b = test_repo + .repo + .treebuilder(Some(&c1.tree().unwrap())) + .unwrap(); + b.insert( + "feature.txt", + test_repo.repo.blob(b"feat").unwrap(), + 0o100644, + ) + .unwrap(); + let mt = test_repo.repo.find_tree(b.write().unwrap()).unwrap(); + let merge_oid = test_repo + .repo + .commit( + None, + &sig, + &sig, + "Merge main into feature", + &mt, + &[&f1, &c1], + ) + .unwrap(); + + test_repo + .repo + .reference("refs/heads/integration", merge_oid, true, "merge") + .unwrap(); + test_repo.repo.set_head("refs/heads/integration").unwrap(); + test_repo + .repo + .checkout_head(Some(git2::build::CheckoutBuilder::new().force())) + .unwrap(); + + // Verify inverted parents + let head = test_repo.repo.head().unwrap().peel_to_commit().unwrap(); + assert_eq!(head.parent_count(), 2); + assert_eq!(head.parent_id(0).unwrap(), f1_oid); + + // Add one more remote commit + test_repo.add_remote_commits(&["C3"]); + + // Run update — should succeed and flatten (no conflicts) + let result = test_repo.in_dir(|| super::run(false)); + assert!( + result.is_ok(), + "update should handle inverted-parent merges: {:?}", + result.err() + ); + + // Verify the rebase fully completed (not paused on conflicts) + let git_dir = test_repo.repo.path().to_path_buf(); + assert!( + !crate::git_commands::git_rebase::is_in_progress(&git_dir), + "rebase should not be in progress — expected clean completion, not conflict pause" + ); + + // HEAD should be linear (redundant merge dropped) + let new_head = test_repo.repo.head().unwrap().peel_to_commit().unwrap(); + assert_eq!( + new_head.parent_count(), + 1, + "HEAD should be linear after update (redundant merge dropped)" + ); +} diff --git a/src/weave.rs b/src/weave.rs index 42b8d58..2781ad5 100644 --- a/src/weave.rs +++ b/src/weave.rs @@ -905,6 +905,11 @@ struct FirstParentEntry { /// /// Returns entries in oldest-first order (reversed from the walk direction). /// Includes both regular and merge commits. +/// +/// For merge commits with inverted parent ordering (feature branch as first +/// parent instead of the integration line), the parents are swapped so that +/// `merge_parent` always points to the branch side and the walk continues +/// along the line that leads back to `stop`. fn walk_first_parent_line( repo: &Repository, head: Oid, @@ -925,25 +930,60 @@ fn walk_first_parent_line( let message = commit.summary().unwrap_or("").to_string(); let is_merge = commit.parent_count() > 1; - let merge_parent = if is_merge { - Some(commit.parent_id(1)?) - } else { - None - }; - entries.push(FirstParentEntry { - oid: current, - short_hash, - message, - is_merge, - merge_parent, - }); + if is_merge { + let p0 = commit.parent_id(0)?; + let p1 = commit.parent_id(1)?; + + // Determine which parent leads back to `stop` (the integration + // line) and which is the branch side. Loom merges always have + // p0 = integration, p1 = branch, but upstream merges may have + // inverted parent ordering. + let (continue_parent, branch_parent) = + if p0 == stop || repo.graph_descendant_of(p0, stop).unwrap_or(false) { + // Normal: first parent leads to stop + (p0, p1) + } else if p1 == stop || repo.graph_descendant_of(p1, stop).unwrap_or(false) { + // Inverted: second parent leads to stop, swap + (p1, p0) + } else { + bail!( + "Neither parent of merge {} leads to merge-base {}", + current, + stop + ); + }; - // Follow first parent - current = match commit.parent_id(0) { - Ok(oid) => oid, - Err(_) => break, // reached root - }; + entries.push(FirstParentEntry { + oid: current, + short_hash, + message, + is_merge, + merge_parent: Some(branch_parent), + }); + + current = continue_parent; + } else { + entries.push(FirstParentEntry { + oid: current, + short_hash, + message, + is_merge, + merge_parent: None, + }); + + // Follow first parent + current = match commit.parent_id(0) { + Ok(oid) => oid, + Err(_) => { + bail!( + "First-parent walk from {} did not reach merge-base {}", + head, + stop + ); + } + }; + } } // Reverse to oldest-first @@ -954,11 +994,24 @@ fn walk_first_parent_line( /// Walk branch commits from `tip` back to `stop` (exclusive), skipping merges. /// /// Returns entries in newest-first order (like a revwalk). +/// +/// When the branch was forked from before `stop` (e.g. an upstream merge with +/// inverted parent ordering), the actual fork point is computed via +/// `merge_base(tip, stop)` and used as the stop instead. fn walk_branch_commits(repo: &Repository, tip: Oid, stop: Oid) -> Result> { + // For loom-woven branches, tip descends from stop (the branch was forked + // from the merge base). For branches forked earlier, compute the actual + // fork point so we don't walk past it into shared history. + let actual_stop = if tip == stop { + stop + } else { + repo.merge_base(tip, stop).unwrap_or(stop) + }; + let mut entries = Vec::new(); let mut current = tip; - while current != stop { + while current != actual_stop { let commit = repo.find_commit(current)?; // Skip merge commits