d34ff097
Auto-detect merged patches from git graph; remove patch merge command
a73x 2026-03-22 09:40
Instead of git-collab implementing its own merge, detect merges automatically: when materializing a PatchState that is still Open, check if the patch head is reachable from the base branch tip and the base has moved since patch creation. This follows Radicle's approach — users merge however they want (git merge, rebase, cherry-pick) and the tool picks it up. - Add base_commit field to PatchCreate to record base branch OID at creation time (with serde(default) for backward compat) - Add merge detection in PatchState::from_ref_uncached - Remove patch merge CLI command and all merge implementation code - Remove CollabConfig / require_approval_on_latest policy enforcement - Update tests to use git merge directly instead of patch merge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/benches/core_ops.rs b/benches/core_ops.rs index 02e3fce..5c5fd25 100644 --- a/benches/core_ops.rs +++ b/benches/core_ops.rs @@ -78,6 +78,7 @@ fn setup_patches(n: usize) -> (Repository, TempDir) { fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; @@ -219,6 +220,7 @@ fn bench_patch_from_ref(c: &mut Criterion) { fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; diff --git a/src/cli.rs b/src/cli.rs index 5063e3a..4f1486f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -360,11 +360,6 @@ pub enum PatchCmd { #[arg(long)] json: bool, }, /// Merge a patch into its base branch Merge { /// Patch ID (prefix match) id: String, }, /// Close a patch Close { /// Patch ID (prefix match) diff --git a/src/event.rs b/src/event.rs index 74ea16d..97083f9 100644 --- a/src/event.rs +++ b/src/event.rs @@ -67,6 +67,9 @@ pub enum Action { fixes: Option<String>, commit: String, tree: String, /// Base branch tip OID at creation time (for merge detection). #[serde(default, skip_serializing_if = "Option::is_none")] base_commit: Option<String>, }, #[serde(rename = "patch.revision")] PatchRevision { diff --git a/src/lib.rs b/src/lib.rs index 21c5b6b..2922efe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -364,11 +364,6 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { } Ok(()) } PatchCmd::Merge { id } => { let p = patch::merge(repo, &id)?; println!("Patch {:.8} merged into {}.", p.id, p.base_ref); Ok(()) } PatchCmd::Close { id, reason } => { patch::close(repo, &id, reason.as_deref())?; println!("Patch closed."); diff --git a/src/patch.rs b/src/patch.rs index 8846408..a75752a 100644 --- a/src/patch.rs +++ b/src/patch.rs @@ -104,6 +104,7 @@ pub fn create( // Get commit and tree OIDs for revision 1 let commit = repo.find_commit(tip_oid)?; let tree_oid = commit.tree()?.id(); let base_oid = repo.refname_to_id(&format!("refs/heads/{}", base_ref))?; let oid = dag::create_root_action( repo, @@ -115,6 +116,7 @@ pub fn create( fixes: fixes.map(|s| s.to_string()), commit: tip_oid.to_string(), tree: tree_oid.to_string(), base_commit: Some(base_oid.to_string()), }, )?; let id = oid.to_string(); @@ -346,169 +348,6 @@ pub fn revise( Ok(()) } pub fn merge(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::error::Error> { let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; let mut p = PatchState::from_ref(repo, &ref_name, &id)?; if p.status != PatchStatus::Open { return Err(git2::Error::from_str(&format!( "patch is {}, can only merge open patches", p.status )) .into()); } // Auto-detect revision before merge auto_detect_and_update(repo, &ref_name, &mut p, &sk)?; // Check merge policy let config = read_collab_config(repo); if config.require_approval_on_latest { let latest_rev = p.revisions.last().map(|r| r.number).unwrap_or(0); let has_approval = p.reviews.iter().any(|r| { r.verdict == ReviewVerdict::Approve && r.revision == Some(latest_rev) }); if !has_approval { return Err(Error::Cmd(format!( "merge requires approval on the latest revision (revision {})", latest_rev ))); } } // Resolve the head commit let head_oid = p.resolve_head(repo)?; let head_commit = repo.find_commit(head_oid) .map_err(|_| git2::Error::from_str("cannot resolve head commit in patch"))?; let base_ref = format!("refs/heads/{}", p.base_ref); let base_oid = repo.refname_to_id(&base_ref)?; let head_oid = head_commit.id(); // 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 { // 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)?; let mut index = repo.merge_commits(&base_commit, &head_commit, None)?; if index.has_conflicts() { return Err(git2::Error::from_str( "merge has conflicts; resolve manually then revise the patch", ) .into()); } let tree_oid = index.write_tree_to(repo)?; let tree = repo.find_tree(tree_oid)?; let msg = format!("Merge patch {:.8}: {}", id, p.title); repo.commit( Some(&base_ref), &sig, &sig, &msg, &tree, &[&base_commit, &head_commit], )?; } // Record the merge event in the patch DAG let author = get_author(repo)?; let event = Event { timestamp: chrono::Utc::now().to_rfc3339(), author: author.clone(), action: Action::PatchMerge, clock: 0, }; dag::append_event(repo, &ref_name, &event, &sk)?; // Archive the patch ref if ref_name.starts_with("refs/collab/patches/") { state::archive_patch_ref(repo, &id)?; } // Auto-close linked issue if present if let Some(ref fixes_id) = p.fixes { if let Ok((issue_ref, issue_id)) = state::resolve_issue_ref(repo, fixes_id) { let close_event = Event { timestamp: chrono::Utc::now().to_rfc3339(), author, action: Action::IssueClose { reason: Some(format!("Fixed by patch {:.8}", p.id)), }, clock: 0, }; dag::append_event(repo, &issue_ref, &close_event, &sk)?; // Archive the issue ref if issue_ref.starts_with("refs/collab/issues/") { state::archive_issue_ref(repo, &issue_id)?; } } } Ok(p) } /// Generate a unified diff between a patch's base branch and head commit. pub fn diff( @@ -715,24 +554,3 @@ pub fn close( Ok(()) } /// Collab config (from refs/collab/config). #[derive(Default)] struct CollabConfig { require_approval_on_latest: bool, } fn read_collab_config(repo: &Repository) -> CollabConfig { (|| -> Option<CollabConfig> { let tip = repo.refname_to_id("refs/collab/config").ok()?; let tree = repo.find_commit(tip).ok()?.tree().ok()?; let blob = repo.find_blob(tree.get_name("config.json")?.id()).ok()?; let val: serde_json::Value = serde_json::from_slice(blob.content()).ok()?; Some(CollabConfig { require_approval_on_latest: val .pointer("/merge/require_approval_on_latest") .and_then(|v| v.as_bool()) .unwrap_or(false), }) })() .unwrap_or_default() } diff --git a/src/state.rs b/src/state.rs index 6218375..baa0787 100644 --- a/src/state.rs +++ b/src/state.rs @@ -175,6 +175,9 @@ pub struct PatchState { pub base_ref: String, pub fixes: Option<String>, pub branch: String, /// Base branch tip OID at patch creation time (None for old patches). #[serde(default, skip_serializing_if = "Option::is_none")] pub base_commit: Option<String>, pub comments: Vec<Comment>, pub inline_comments: Vec<InlineComment>, pub reviews: Vec<Review>, @@ -425,6 +428,7 @@ impl PatchState { fixes, commit, tree, base_commit, } => { let revisions = vec![Revision { number: 1, @@ -441,6 +445,7 @@ impl PatchState { base_ref, fixes, branch, base_commit, comments: Vec::new(), inline_comments: Vec::new(), reviews: Vec::new(), @@ -524,6 +529,28 @@ impl PatchState { if let Some(ref mut s) = state { s.last_updated = max_timestamp; // Auto-detect merge: if the patch is still Open and its head // is reachable from the base branch tip, the user merged it // outside of git-collab. We compare the current base tip to // the base_commit recorded at creation time — if it has moved // to include the patch head, the patch is merged. if s.status == PatchStatus::Open { if let Ok(patch_head) = s.resolve_head(repo) { let base_ref = format!("refs/heads/{}", s.base_ref); if let Ok(base_tip) = repo.refname_to_id(&base_ref) { // Base must have moved from where it was at creation let base_moved = s.base_commit.as_ref() .map(|bc| bc != &base_tip.to_string()) .unwrap_or(false); let reachable = base_tip == patch_head || repo.graph_descendant_of(base_tip, patch_head).unwrap_or(false); if base_moved && reachable { s.status = PatchStatus::Merged; } } } } } state.ok_or_else(|| git2::Error::from_str("no PatchCreate event found in DAG").into()) } diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 51f03d6..2d2300a 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -79,6 +79,7 @@ mod tests { base_ref: "main".into(), fixes: None, branch: format!("feature/{}", id), base_commit: None, comments: vec![], inline_comments: vec![], reviews: vec![], @@ -274,6 +275,7 @@ mod tests { base_ref: "main".to_string(), fixes: None, branch: format!("feature/p{:07x}", i), base_commit: None, comments: Vec::new(), inline_comments: Vec::new(), reviews: Vec::new(), @@ -408,6 +410,7 @@ mod tests { fixes: None, commit: "abc123".to_string(), tree: "def456".to_string(), base_commit: None, }; assert_eq!(action_type_label(&action), "Patch Create"); } @@ -1032,6 +1035,7 @@ mod tests { base_ref: "main".into(), fixes: Some("i1".into()), branch: "feature/fix-thing".into(), base_commit: None, comments: vec![crate::state::Comment { author: make_author(), body: "Thread comment".into(), diff --git a/tests/adversarial_test.rs b/tests/adversarial_test.rs index 8b43512..c0e5d04 100644 --- a/tests/adversarial_test.rs +++ b/tests/adversarial_test.rs @@ -544,6 +544,7 @@ fn arb_action() -> impl Strategy<Value = Action> { fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, } }), proptest::option::of(".*").prop_map(|body| Action::PatchRevision { diff --git a/tests/cli_test.rs b/tests/cli_test.rs index 4495397..33952c8 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -489,7 +489,7 @@ fn test_patch_close() { } #[test] fn test_patch_merge_fast_forward() { fn test_patch_auto_detect_merge_on_git_merge() { let repo = TestRepo::new("Alice", "alice@example.com"); // Create a feature branch ahead of main @@ -501,36 +501,15 @@ fn test_patch_merge_fast_forward() { let out = repo.run_ok(&["patch", "create", "-t", "Add feature", "-B", "feature"]); let id = out.trim().strip_prefix("Created patch ").unwrap(); let out = repo.run_ok(&["patch", "merge", id]); assert!(out.contains("merged into main")); // Main should now point at the feature commit let main_head = repo.git(&["rev-parse", "main"]).trim().to_string(); let feature_head = repo.git(&["rev-parse", "feature"]).trim().to_string(); assert_eq!(main_head, feature_head); // Merge via git directly repo.git(&["merge", "feature"]); // Patch should auto-detect as merged let out = repo.run_ok(&["patch", "show", id]); assert!(out.contains("[merged]")); } #[test] fn test_patch_cannot_merge_closed() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feature"]); repo.commit_file("f.txt", "f", "feature commit"); repo.git(&["checkout", "main"]); let out = repo.run_ok(&["patch", "create", "-t", "Will close", "-B", "feature"]); let id = out.trim().strip_prefix("Created patch ").unwrap(); repo.run_ok(&["patch", "close", id]); let err = repo.run_err(&["patch", "merge", id]); assert!(err.contains("can only merge open patches")); } #[test] fn test_patch_list_empty() { let repo = TestRepo::new("Alice", "alice@example.com"); @@ -566,30 +545,6 @@ fn test_patch_create_with_fixes() { assert!(out.contains(&issue_id[..8]), "should show linked issue ID"); } #[test] fn test_patch_merge_auto_closes_linked_issue() { let repo = TestRepo::new("Alice", "alice@example.com"); let issue_id = repo.issue_open("Crash on startup"); repo.git(&["checkout", "-b", "fix"]); repo.commit_file("fix.rs", "fixed", "fix crash"); repo.git(&["checkout", "main"]); let out = repo.run_ok(&[ "patch", "create", "-t", "Fix crash", "-B", "fix", "--fixes", &issue_id, ]); let patch_id = out.trim().strip_prefix("Created patch ").unwrap(); repo.run_ok(&["patch", "merge", patch_id]); // Issue should now be closed let out = repo.run_ok(&["issue", "show", &issue_id]); assert!(out.contains("[closed]"), "linked issue should be auto-closed on merge"); } // =========================================================================== // Unread tracking @@ -759,10 +714,11 @@ fn test_full_patch_review_cycle() { // Approve repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM now"]); // Merge repo.run_ok(&["patch", "merge", &id]); // Merge via git repo.git(&["checkout", "main"]); repo.git(&["merge", "feature"]); // Verify final state // Verify final state — auto-detected as merged let out = repo.run_ok(&["patch", "show", &id]); assert!(out.contains("[merged]")); assert!(out.contains("Added documentation")); diff --git a/tests/collab_test.rs b/tests/collab_test.rs index 022742a..0f30678 100644 --- a/tests/collab_test.rs +++ b/tests/collab_test.rs @@ -643,6 +643,7 @@ fn create_branch_patch( fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; @@ -915,6 +916,7 @@ fn test_resolve_head_with_oid_string() { fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; @@ -929,131 +931,43 @@ fn test_resolve_head_with_oid_string() { } // --------------------------------------------------------------------------- // Phase 5: US3 — Merge (T024-T025) // Phase 5: US3 — Merge auto-detection // --------------------------------------------------------------------------- #[test] fn test_merge_branch_based_patch() { // T024: merging a branch-based patch performs git merge and updates DAG fn test_auto_detect_merged_patch_via_git_merge() { // When a user merges the patch branch into the base branch manually // (using git merge), PatchState should auto-detect that the patch // is merged without needing `patch merge`. 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", "f.rs", b"x"); 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, "Merge test", "", "main", "feat", None).unwrap(); // Merge the patch patch::merge(&repo, &id).unwrap(); // Check that the patch is now merged (archived after merge) let archive_ref = format!("refs/collab/archive/patches/{}", id); let state = PatchState::from_ref(&repo, &archive_ref, &id).unwrap(); assert_eq!(state.status, PatchStatus::Merged); // Check that the base branch now has the feature commit 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" ); } #[test] fn test_merge_conflicting_patch_reports_error() { // T025: merging a conflicting patch reports error 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", "conflict.rs", b"original"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); // Both branches modify the same file differently add_commit_on_branch(&repo, "feat", "conflict.rs", b"feature version"); add_commit_on_branch(&repo, "main", "conflict.rs", b"main version"); let id = patch::create(&repo, "Conflict test", "", "main", "feat", None).unwrap(); let result = patch::merge(&repo, &id); assert!(result.is_err(), "conflicting merge should fail"); let err_msg = result.unwrap_err().to_string(); 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(); // Create the patch let id = patch::create(&repo, "Auto-detect test", "", "main", "feat", None).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(); // Verify it's open let ref_name = format!("refs/collab/patches/{}", id); let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); assert_eq!(state.status, PatchStatus::Open); // 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" ); // Manually fast-forward main to feat (simulating `git merge feat`) let feat_tip = repo.refname_to_id("refs/heads/feat").unwrap(); repo.reference("refs/heads/main", feat_tip, true, "manual merge").unwrap(); // 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"); // Now PatchState should auto-detect that it's merged let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap(); assert_eq!(state.status, PatchStatus::Merged, "should auto-detect merge"); } #[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. fn test_auto_detect_merged_patch_deleted_branch() { // If the patch branch was deleted after a manual merge, // auto-detection should not crash — patch stays Open. let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); @@ -1062,27 +976,15 @@ fn test_merge_ff_does_not_touch_working_tree_when_on_different_branch() { 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(); let id = patch::create(&repo, "Deleted branch test", "", "main", "feat", None).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" ); // Delete the feature branch (simulating cleanup after merge) repo.find_reference("refs/heads/feat").unwrap().delete().unwrap(); // 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" ); // Should not crash, patch stays Open (can't verify merge without the branch) let ref_name = format!("refs/collab/patches/{}", id); let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap(); assert_eq!(state.status, PatchStatus::Open); } // --------------------------------------------------------------------------- diff --git a/tests/common/mod.rs b/tests/common/mod.rs index fdc2113..d8b2da7 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -141,6 +141,7 @@ pub fn create_patch(repo: &Repository, author: &Author, title: &str) -> (String, fixes: None, commit: commit_oid.to_string(), tree: tree_oid.to_string(), base_commit: None, }, clock: 0, }; diff --git a/tests/crdt_test.rs b/tests/crdt_test.rs index 4046c94..4853366 100644 --- a/tests/crdt_test.rs +++ b/tests/crdt_test.rs @@ -363,6 +363,7 @@ fn concurrent_patch_close_merge_higher_clock_wins() { fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; diff --git a/tests/revision_test.rs b/tests/revision_test.rs index fb2b5bf..123e20c 100644 --- a/tests/revision_test.rs +++ b/tests/revision_test.rs @@ -518,6 +518,7 @@ fn test_concurrent_revision_dedup_after_reconcile() { fixes: None, commit: head.to_string(), tree: tree_oid.to_string(), base_commit: None, }, clock: 0, }; @@ -586,182 +587,5 @@ fn test_concurrent_revision_dedup_after_reconcile() { assert_eq!(state.revisions[1].commit, new_commit.to_string()); } // =========================================================================== // D2: Merge policy enforcement (require_approval_on_latest) // =========================================================================== #[test] fn test_merge_policy_require_approval_on_latest() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); let sk = test_signing_key(); // Create a branch with a commit let head = repo.head().unwrap().target().unwrap(); let head_commit = repo.find_commit(head).unwrap(); repo.branch("feat-policy", &head_commit, false).unwrap(); // Add a commit on the branch so it differs from main let sig = git2::Signature::now("Alice", "alice@example.com").unwrap(); let blob = repo.blob(b"feature code").unwrap(); let mut tb = repo.treebuilder(Some(&head_commit.tree().unwrap())).unwrap(); tb.insert("feature.txt", blob, 0o100644).unwrap(); let feat_tree_oid = tb.write().unwrap(); let feat_tree = repo.find_tree(feat_tree_oid).unwrap(); let feat_commit = repo.commit( Some("refs/heads/feat-policy"), &sig, &sig, "add feature", &feat_tree, &[&head_commit], ).unwrap(); // Create patch (revision 1) let create_event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "Policy test".to_string(), body: "".to_string(), base_ref: "main".to_string(), branch: "feat-policy".to_string(), fixes: None, commit: feat_commit.to_string(), tree: feat_tree_oid.to_string(), }, clock: 0, }; let patch_oid = dag::create_root_event(&repo, &create_event, &sk).unwrap(); let patch_id = patch_oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", patch_id); repo.reference(&patch_ref, patch_oid, false, "test").unwrap(); // Approve on revision 1 let review_event = Event { timestamp: now(), author: bob(), action: Action::PatchReview { verdict: git_collab::event::ReviewVerdict::Approve, body: "LGTM".to_string(), revision: 1, }, clock: 0, }; dag::append_event(&repo, &patch_ref, &review_event, &sk).unwrap(); // Push revision 2 (new commit on branch) let blob2 = repo.blob(b"v2 code").unwrap(); let parent = repo.find_commit(feat_commit).unwrap(); let mut tb2 = repo.treebuilder(Some(&parent.tree().unwrap())).unwrap(); tb2.insert("v2.txt", blob2, 0o100644).unwrap(); let v2_tree_oid = tb2.write().unwrap(); let v2_tree = repo.find_tree(v2_tree_oid).unwrap(); let v2_commit = repo.commit( Some("refs/heads/feat-policy"), &sig, &sig, "v2", &v2_tree, &[&parent], ).unwrap(); // Record revision 2 let rev_event = Event { timestamp: now(), author: alice(), action: Action::PatchRevision { commit: v2_commit.to_string(), tree: v2_tree_oid.to_string(), body: None, }, clock: 0, }; dag::append_event(&repo, &patch_ref, &rev_event, &sk).unwrap(); // Write merge policy config: require_approval_on_latest = true let config_json = br#"{"merge":{"require_approval_on_latest":true}}"#; let config_blob = repo.blob(config_json).unwrap(); let mut ctb = repo.treebuilder(None).unwrap(); ctb.insert("config.json", config_blob, 0o100644).unwrap(); let config_tree_oid = ctb.write().unwrap(); let config_tree = repo.find_tree(config_tree_oid).unwrap(); repo.commit( Some("refs/collab/config"), &sig, &sig, "set merge policy", &config_tree, &[], ).unwrap(); // Try to merge via the library — should fail because approval is on revision 1, not 2 let result = git_collab::patch::merge(&repo, &patch_id[..8]); assert!(result.is_err(), "merge should fail when approval is not on latest revision"); let err_msg = result.unwrap_err().to_string(); assert!( err_msg.contains("merge requires approval on the latest revision"), "error message should mention latest revision requirement, got: {}", err_msg ); } #[test] fn test_merge_policy_passes_when_approved_on_latest() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); let sk = test_signing_key(); // Create a branch with a commit let head = repo.head().unwrap().target().unwrap(); let head_commit = repo.find_commit(head).unwrap(); repo.branch("feat-policy-ok", &head_commit, false).unwrap(); let sig = git2::Signature::now("Alice", "alice@example.com").unwrap(); let blob = repo.blob(b"feature code").unwrap(); let mut tb = repo.treebuilder(Some(&head_commit.tree().unwrap())).unwrap(); tb.insert("feature.txt", blob, 0o100644).unwrap(); let feat_tree_oid = tb.write().unwrap(); let feat_tree = repo.find_tree(feat_tree_oid).unwrap(); let feat_commit = repo.commit( Some("refs/heads/feat-policy-ok"), &sig, &sig, "add feature", &feat_tree, &[&head_commit], ).unwrap(); // Create patch let create_event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "Policy pass test".to_string(), body: "".to_string(), base_ref: "main".to_string(), branch: "feat-policy-ok".to_string(), fixes: None, commit: feat_commit.to_string(), tree: feat_tree_oid.to_string(), }, clock: 0, }; let patch_oid = dag::create_root_event(&repo, &create_event, &sk).unwrap(); let patch_id = patch_oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", patch_id); repo.reference(&patch_ref, patch_oid, false, "test").unwrap(); // Approve on revision 1 (which IS the latest) let review_event = Event { timestamp: now(), author: bob(), action: Action::PatchReview { verdict: git_collab::event::ReviewVerdict::Approve, body: "LGTM".to_string(), revision: 1, }, clock: 0, }; dag::append_event(&repo, &patch_ref, &review_event, &sk).unwrap(); // Write merge policy config let config_json = br#"{"merge":{"require_approval_on_latest":true}}"#; let config_blob = repo.blob(config_json).unwrap(); let mut ctb = repo.treebuilder(None).unwrap(); ctb.insert("config.json", config_blob, 0o100644).unwrap(); let config_tree_oid = ctb.write().unwrap(); let config_tree = repo.find_tree(config_tree_oid).unwrap(); repo.commit( Some("refs/collab/config"), &sig, &sig, "set merge policy", &config_tree, &[], ).unwrap(); // Merge should succeed — approval is on latest revision (1) let result = git_collab::patch::merge(&repo, &patch_id[..8]); assert!(result.is_ok(), "merge should succeed when approval is on latest revision: {:?}", result.err()); } // (Merge policy tests removed — policy enforcement dropped in favour // of auto-detect merge via git graph.) diff --git a/tests/signing_test.rs b/tests/signing_test.rs index 01bbc67..c36fc96 100644 --- a/tests/signing_test.rs +++ b/tests/signing_test.rs @@ -212,6 +212,7 @@ fn event_json_uses_namespaced_action_types() { fixes: Some("deadbeef".to_string()), commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; diff --git a/tests/sort_test.rs b/tests/sort_test.rs index bd46aa5..81c0c93 100644 --- a/tests/sort_test.rs +++ b/tests/sort_test.rs @@ -89,6 +89,7 @@ fn create_patch_at( fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; diff --git a/tests/sync_test.rs b/tests/sync_test.rs index e398705..9ac8e59 100644 --- a/tests/sync_test.rs +++ b/tests/sync_test.rs @@ -293,6 +293,7 @@ fn test_patch_review_across_repos() { fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, }; @@ -345,6 +346,7 @@ fn test_concurrent_review_and_revise() { fixes: None, commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), base_commit: None, }, clock: 0, };