From 3b10369a51326c4a3a231fe30ae0ebba93752b3a Mon Sep 17 00:00:00 2001 From: Nicolas Arnaud-Cormos Date: Sun, 22 Mar 2026 19:51:26 +0100 Subject: [PATCH] fix(merge): handle conflicts in loom branch merge with continue/abort support Co-Authored-By: Claude Sonnet 4.6 --- specs/005-branch.md | 31 ++++++++ src/branch/merge.rs | 46 +++++++++-- src/branch/merge_test.rs | 126 +++++++++++++++++++++++++++++++ src/git_commands/git_merge.rs | 50 +++++++++++- src/test_helpers.rs | 17 ++++- src/transaction.rs | 13 +++- tests/integration/helpers.sh | 3 + tests/integration/test_branch.sh | 49 ++++++++++++ 8 files changed, 323 insertions(+), 12 deletions(-) diff --git a/specs/005-branch.md b/specs/005-branch.md index 1f0135f..abeb227 100644 --- a/specs/005-branch.md +++ b/specs/005-branch.md @@ -64,6 +64,7 @@ git-loom branch merge [branch] [--all] - Uses `git merge --no-ff` to create the merge topology - If a remote branch is selected (with `--all`), a local tracking branch is created automatically before weaving +- On merge conflict, saves state and pauses for `loom continue` / `loom abort` - Errors if the branch is already woven or doesn't exist ### `branch unmerge` @@ -179,6 +180,36 @@ a new feature from the integration point. File targets are rejected with an error message. +## Conflict Recovery + +### `branch new` (weaving) + +When `branch new` triggers a weave operation that encounters conflicts, it uses +hard-fail: the rebase is automatically aborted and an error is reported. No +state is saved — the user must resolve the situation and retry. + +### `branch merge` + +`branch merge` supports resumable conflict handling via `loom continue` and +`loom abort` (see Spec 014). + +When the merge encounters a conflict: + +1. The state is saved to `.git/loom/state.json` +2. loom reports that the operation is paused and exits + +The saved state contains: +- `branch_name`: the branch being woven into integration + +After the user resolves the conflict: + +- `loom continue` — completes the merge and prints the success message +- `loom abort` — calls `git merge --abort` and restores the original state + +While the operation is paused, most other loom commands are blocked. Only +`loom show`, `loom diff`, `loom trace`, `loom continue`, and `loom abort` +are permitted. + ## Prerequisites - Git 2.38 or later diff --git a/src/branch/merge.rs b/src/branch/merge.rs index 45fee41..bf0ddbb 100644 --- a/src/branch/merge.rs +++ b/src/branch/merge.rs @@ -1,9 +1,17 @@ -use anyhow::{Result, bail}; +use anyhow::{Context, Result, bail}; use git2::{BranchType, Repository}; +use serde::{Deserialize, Serialize}; use crate::git; use crate::git_commands::git_branch; +use crate::git_commands::git_merge::MergeOutcome; use crate::msg; +use crate::transaction::{self, LoomState, Rollback}; + +#[derive(Serialize, Deserialize)] +struct MergeContext { + branch_name: String, +} /// Weave an existing branch into the integration branch. /// @@ -13,6 +21,7 @@ use crate::msg; pub fn run(branch: Option, all: bool) -> Result<()> { let repo = git::open_repo()?; let workdir = git::require_workdir(&repo, "merge")?; + let git_dir = repo.path().to_path_buf(); let info = git::gather_repo_info(&repo, false, 1)?; let branch_name = match branch { @@ -22,7 +31,7 @@ pub fn run(branch: Option, all: bool) -> Result<()> { // If this is a remote branch (contains '/'), create a local tracking branch let local_name = if branch_name.contains('/') { - let local = branch_name.split('/').skip(1).collect::>().join("/"); + let local = git::upstream_local_branch(&branch_name); git_branch::create(workdir, &local, &branch_name)?; // Set up tracking let mut local_branch = repo.find_branch(&local, BranchType::Local)?; @@ -33,10 +42,34 @@ pub fn run(branch: Option, all: bool) -> Result<()> { }; // Merge the branch into integration (--no-ff) so it appears in the topology - crate::git_commands::git_merge::merge_no_ff(workdir, &local_name)?; + match crate::git_commands::git_merge::merge_no_ff(workdir, &git_dir, &local_name)? { + MergeOutcome::Completed => { + msg::success(&format!("Woven `{}` into integration branch", local_name)); + } + MergeOutcome::Conflicted => { + let state = LoomState { + command: "merge".to_string(), + rollback: Rollback::default(), + context: serde_json::to_value(MergeContext { + branch_name: local_name, + })?, + }; + transaction::save(&git_dir, &state)?; + transaction::warn_conflict_paused("merge"); + } + } - msg::success(&format!("Woven `{}` into integration branch", local_name)); + Ok(()) +} +/// Resume a `loom branch merge` after conflicts have been resolved. +pub fn after_continue(context: &serde_json::Value) -> anyhow::Result<()> { + let ctx: MergeContext = + serde_json::from_value(context.clone()).context("Failed to parse merge resume context")?; + msg::success(&format!( + "Woven `{}` into integration branch", + ctx.branch_name + )); Ok(()) } @@ -87,7 +120,7 @@ fn pick_branch(repo: &Repository, info: &git::RepoInfo, include_remote: bool) -> // Remote branches without a local counterpart if include_remote { - let local_names: Vec = repo + let local_names: std::collections::HashSet = repo .branches(Some(BranchType::Local))? .filter_map(|b| b.ok()) .filter_map(|(b, _)| b.name().ok().flatten().map(|n| n.to_string())) @@ -107,8 +140,7 @@ fn pick_branch(repo: &Repository, info: &git::RepoInfo, include_remote: bool) -> continue; } // Skip if a local branch with the same short name exists - let short_name = name.split('/').skip(1).collect::>().join("/"); - if !local_names.contains(&short_name) { + if !local_names.contains(&git::upstream_local_branch(name)) { items.push(name.to_string()); } } diff --git a/src/branch/merge_test.rs b/src/branch/merge_test.rs index 46b9570..cd1e43e 100644 --- a/src/branch/merge_test.rs +++ b/src/branch/merge_test.rs @@ -114,3 +114,129 @@ fn merge_nonexistent_branch_errors() { test_repo.in_dir(|| super::merge::run(Some("nonexistent-branch".to_string()), false)); assert!(result.is_err(), "merging nonexistent branch should error"); } + +/// When a merge conflicts, the operation pauses: state file saved, HEAD unchanged. +/// Resolving and running continue completes the merge and weaves the branch. +#[test] +fn merge_conflict_continue_weaves_branch() { + let test_repo = TestRepo::new_with_remote(); + + // Both branches modify the same file differently → merge conflict. + let base_oid = test_repo.find_remote_branch_target("origin/main"); + test_repo.create_branch_at("feature-conflict", &base_oid.to_string()); + + test_repo.write_file("shared.txt", "integration"); + test_repo.stage_files(&["shared.txt"]); + test_repo.commit_staged("Integration change"); + + test_repo.switch_branch("feature-conflict"); + test_repo.write_file("shared.txt", "feature"); + test_repo.stage_files(&["shared.txt"]); + test_repo.commit_staged("Feature change"); + test_repo.switch_branch("integration"); + + let pre_merge_head = test_repo.head_oid(); + + // Merge pauses — must not return Err. + let result = + test_repo.in_dir(|| super::merge::run(Some("feature-conflict".to_string()), false)); + assert!( + result.is_ok(), + "merge should return Ok on conflict: {:?}", + result + ); + + let state_path = test_repo.repo.path().join("loom/state.json"); + assert!( + state_path.exists(), + "state file must exist when merge is paused" + ); + assert_eq!( + test_repo.head_oid(), + pre_merge_head, + "HEAD must not move on conflict" + ); + + // Resolve conflict and continue. + test_repo.write_file("shared.txt", "resolved"); + test_repo.stage_files(&["shared.txt"]); + crate::transaction::continue_cmd(&test_repo.workdir(), test_repo.repo.path()).unwrap(); + + assert!( + !state_path.exists(), + "state file must be deleted after continue" + ); + assert_eq!( + test_repo.head_commit().parent_count(), + 2, + "HEAD must be a merge commit after continue" + ); + + let info = git::gather_repo_info(&test_repo.repo, false, 1).unwrap(); + let branch_names: Vec<&str> = info.branches.iter().map(|b| b.name.as_str()).collect(); + assert!( + branch_names.contains(&"feature-conflict"), + "branch must be woven after continue, got: {:?}", + branch_names + ); +} + +/// Aborting a paused merge restores HEAD to its pre-merge state and leaves the +/// branch unwoven. +#[test] +fn merge_conflict_abort_restores_state() { + let test_repo = TestRepo::new_with_remote(); + + let base_oid = test_repo.find_remote_branch_target("origin/main"); + test_repo.create_branch_at("feature-abort", &base_oid.to_string()); + + test_repo.write_file("shared.txt", "integration"); + test_repo.stage_files(&["shared.txt"]); + test_repo.commit_staged("Integration change"); + + test_repo.switch_branch("feature-abort"); + test_repo.write_file("shared.txt", "feature"); + test_repo.stage_files(&["shared.txt"]); + test_repo.commit_staged("Feature change"); + test_repo.switch_branch("integration"); + + let pre_merge_head = test_repo.head_oid(); + + let result = test_repo.in_dir(|| super::merge::run(Some("feature-abort".to_string()), false)); + assert!( + result.is_ok(), + "merge should return Ok on conflict: {:?}", + result + ); + + let state_path = test_repo.repo.path().join("loom/state.json"); + assert!( + state_path.exists(), + "state file must exist when merge is paused" + ); + + crate::transaction::abort_cmd(&test_repo.workdir(), test_repo.repo.path()).unwrap(); + + assert!( + !state_path.exists(), + "state file must be deleted after abort" + ); + assert_eq!( + test_repo.head_oid(), + pre_merge_head, + "HEAD must be restored after abort" + ); + assert_eq!( + test_repo.head_commit().parent_count(), + 1, + "HEAD must not be a merge commit after abort" + ); + + let info = git::gather_repo_info(&test_repo.repo, false, 1).unwrap(); + let branch_names: Vec<&str> = info.branches.iter().map(|b| b.name.as_str()).collect(); + assert!( + !branch_names.contains(&"feature-abort"), + "branch must NOT be woven after abort, got: {:?}", + branch_names + ); +} diff --git a/src/git_commands/git_merge.rs b/src/git_commands/git_merge.rs index 6e09461..0199c15 100644 --- a/src/git_commands/git_merge.rs +++ b/src/git_commands/git_merge.rs @@ -1,7 +1,51 @@ +use std::path::Path; + +use anyhow::Result; + +/// Outcome of a merge operation. +pub enum MergeOutcome { + Completed, + Conflicted, +} + /// Merge a branch into the current branch, forcing a merge commit. /// /// Wraps `git merge --no-ff --no-edit`. -/// Use this when a fast-forward merge would skip creating the merge topology. -pub fn merge_no_ff(workdir: &std::path::Path, branch: &str) -> anyhow::Result<()> { - super::run_git(workdir, &["merge", "--no-ff", branch, "--no-edit"]) +/// Returns `Conflicted` if the merge stopped due to conflicts, +/// `Completed` on success, or `Err` on any other failure. +pub fn merge_no_ff(workdir: &Path, git_dir: &Path, branch: &str) -> Result { + run_merge_cmd(workdir, git_dir, &["merge", "--no-ff", branch, "--no-edit"]) +} + +/// Continue an in-progress merge (equivalent to `git merge --continue`). +/// +/// Note: `--continue` does not accept extra flags like `--no-edit`. The merge +/// commit message is taken from `MERGE_MSG` without opening an editor. +pub fn continue_merge(workdir: &Path, git_dir: &Path) -> Result { + run_merge_cmd(workdir, git_dir, &["merge", "--continue"]) +} + +fn run_merge_cmd(workdir: &Path, git_dir: &Path, args: &[&str]) -> Result { + match super::run_git(workdir, args) { + Ok(()) => Ok(MergeOutcome::Completed), + Err(e) => { + if is_in_progress(git_dir) { + Ok(MergeOutcome::Conflicted) + } else { + Err(e) + } + } + } +} + +/// Abort an in-progress merge. +pub fn abort(workdir: &Path) -> Result<()> { + super::run_git(workdir, &["merge", "--abort"]) +} + +/// Check whether a merge is currently in progress. +/// +/// Detects the presence of `MERGE_HEAD`, which git creates when a merge is paused. +pub fn is_in_progress(git_dir: &Path) -> bool { + git_dir.join("MERGE_HEAD").exists() } diff --git a/src/test_helpers.rs b/src/test_helpers.rs index fc7b747..be42fe9 100644 --- a/src/test_helpers.rs +++ b/src/test_helpers.rs @@ -135,6 +135,9 @@ impl TestRepo { let mut config = repo.config().unwrap(); config.set_str("user.name", "Test").unwrap(); config.set_str("user.email", "test@test.com").unwrap(); + // Prevent git from opening an interactive editor in tests (e.g. for + // `git merge --continue` which is equivalent to `git commit`). + config.set_str("core.editor", "true").unwrap(); } /// Get the signature used for commits. @@ -610,7 +613,19 @@ impl TestRepo { /// Merge a branch into the current branch with --no-ff. pub fn merge_no_ff(&self, branch: &str) { - crate::git_commands::git_merge::merge_no_ff(self.workdir().as_path(), branch).unwrap(); + let outcome = crate::git_commands::git_merge::merge_no_ff( + self.workdir().as_path(), + self.repo.path(), + branch, + ) + .unwrap(); + assert!( + matches!( + outcome, + crate::git_commands::git_merge::MergeOutcome::Completed + ), + "merge_no_ff: expected Completed, got Conflicted" + ); } /// Stage files in the working directory. diff --git a/src/transaction.rs b/src/transaction.rs index f7d0c86..65eef7b 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; -use crate::git_commands::{self, git_branch, git_commit, git_rebase}; +use crate::git_commands::{self, git_branch, git_commit, git_merge, git_rebase}; /// Persistent state saved when a loom command is paused due to a rebase conflict. #[derive(Debug, Serialize, Deserialize)] @@ -151,6 +151,14 @@ pub fn continue_cmd(workdir: &Path, git_dir: &Path) -> Result<()> { } git_rebase::RebaseOutcome::Completed => {} } + } else if git_merge::is_in_progress(git_dir) { + match git_merge::continue_merge(workdir, git_dir)? { + git_merge::MergeOutcome::Conflicted => { + crate::msg::warn("Conflicts remain — resolve them and run `loom continue` again"); + return Ok(()); + } + git_merge::MergeOutcome::Completed => {} + } } dispatch_after_continue(workdir, &state)?; @@ -171,6 +179,8 @@ pub fn abort_cmd(workdir: &Path, git_dir: &Path) -> Result<()> { if git_rebase::is_in_progress(git_dir) { let _ = git_rebase::abort(workdir); + } else if git_merge::is_in_progress(git_dir) { + let _ = git_merge::abort(workdir); } state.rollback.apply_abort(workdir)?; @@ -192,6 +202,7 @@ fn dispatch_after_continue(workdir: &Path, state: &LoomState) -> Result<()> { "drop" => crate::drop::after_continue(workdir, &state.context), "fold" => crate::fold::after_continue(workdir, &state.context), "swap" => crate::swap::after_continue(workdir, &state.context), + "merge" => crate::branch::merge::after_continue(&state.context), other => bail!("Unknown command '{}' in loom state file", other), } } diff --git a/tests/integration/helpers.sh b/tests/integration/helpers.sh index 66bba87..01c64ba 100644 --- a/tests/integration/helpers.sh +++ b/tests/integration/helpers.sh @@ -61,6 +61,9 @@ setup_repo_with_remote() { git -C "$WORK" config user.email "test@test.com" git -C "$WORK" config user.name "Test" git -C "$WORK" config core.autocrlf false + # Prevent git from opening an interactive editor in tests (e.g. for + # `git merge --continue` which is equivalent to `git commit`). + git -C "$WORK" config core.editor "true" # Integration branch tracking origin/ git -C "$WORK" checkout -q -b integration diff --git a/tests/integration/test_branch.sh b/tests/integration/test_branch.sh index 1f4afa4..2d90487 100644 --- a/tests/integration/test_branch.sh +++ b/tests/integration/test_branch.sh @@ -154,6 +154,55 @@ setup_repo_with_remote gl_capture branch merge g-does-not-exist assert_exit_fail "$CODE" "merge_nonexistent_fail" +# ══════════════════════════════════════════════════════════════════════════════ +# BRANCH MERGE — CONFLICT RECOVERY +# ══════════════════════════════════════════════════════════════════════════════ +# Both branches modify the same file to trigger a conflict on merge. + +describe "branch merge: conflict → continue (resolving conflict) → success" +setup_repo_with_remote +create_feature_branch "g-conflict-cont" +switch_to g-conflict-cont +echo "feature content" > "$WORK/shared.txt" +git -C "$WORK" add shared.txt +git -C "$WORK" commit -q -m "Feature change" +switch_to integration +echo "integration content" > "$WORK/shared.txt" +git -C "$WORK" add shared.txt +git -C "$WORK" commit -q -m "Integration change" +gl_capture branch merge g-conflict-cont +assert_exit_ok "$CODE" "merge_cont_exit" +assert_state_file "merge_cont_state" +assert_contains "$OUT" "loom continue" "merge_cont_hint" +echo "resolved" > "$WORK/shared.txt" +git -C "$WORK" add shared.txt +gl_capture continue +assert_exit_ok "$CODE" "merge_cont_ok" +assert_no_state_file "merge_cont_state_removed" +assert_contains "$OUT" "Woven" "merge_cont_msg" +assert_head_parent_count 2 "merge_cont_merge_commit" + +describe "branch merge: conflict → abort → HEAD restored" +setup_repo_with_remote +create_feature_branch "g-conflict-abort" +switch_to g-conflict-abort +echo "feature content" > "$WORK/shared.txt" +git -C "$WORK" add shared.txt +git -C "$WORK" commit -q -m "Feature change" +switch_to integration +echo "integration content" > "$WORK/shared.txt" +git -C "$WORK" add shared.txt +git -C "$WORK" commit -q -m "Integration change" +old_head=$(head_hash) +gl_capture branch merge g-conflict-abort +assert_state_file "merge_abort_state" +gl_capture abort +assert_exit_ok "$CODE" "merge_abort_ok" +assert_contains "$OUT" "Aborted" "merge_abort_msg" +assert_no_state_file "merge_abort_state_removed" +assert_eq "$old_head" "$(head_hash)" "merge_abort_head_restored" +assert_head_parent_count 1 "merge_abort_not_merged" + # ══════════════════════════════════════════════════════════════════════════════ # BRANCH UNMERGE # ══════════════════════════════════════════════════════════════════════════════