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