diff --git a/Cargo.lock b/Cargo.lock index cf8e3941c8c..e3143d325e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3825,8 +3825,7 @@ dependencies = [ [[package]] name = "git2" version = "0.21.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ddddbf932745a6be37109b6112d3ee09696106f848449069d3a57bba937ab82e" +source = "git+https://github.com/jonathantanmy2/git2-rs?rev=9e2ec81c8d37ba5142e5fb97c5afbc2f43c6d7eb#9e2ec81c8d37ba5142e5fb97c5afbc2f43c6d7eb" dependencies = [ "bitflags 2.11.1", "libc", @@ -6322,8 +6321,7 @@ dependencies = [ [[package]] name = "libgit2-sys" version = "0.18.5+1.9.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "005d6ae6eac1912906073e069f7db60b1fa98e052a68227824afe3e3a1c59ca2" +source = "git+https://github.com/jonathantanmy2/git2-rs?rev=9e2ec81c8d37ba5142e5fb97c5afbc2f43c6d7eb#9e2ec81c8d37ba5142e5fb97c5afbc2f43c6d7eb" dependencies = [ "cc", "libc", diff --git a/Cargo.toml b/Cargo.toml index f250645150f..8d0c9b461ef 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -219,6 +219,7 @@ gix-testtools = { version = "0.19.0", git = "https://github.com/GitoxideLabs/git insta = { version = "1.47.2", features = ["json"] } git2 = { version = "0.21.0", features = [ + "unstable-sha256", "vendored-openssl", "vendored-libgit2", ] } @@ -313,6 +314,7 @@ smallvec = "1.15.1" [patch.crates-io] # Keep workspace crates that use the `gix 0.84` line on the same git revision. gix = { git = "https://github.com/GitoxideLabs/gitoxide", rev = "66f70f3063724b4145b21c6691c4f57892c7a74e" } +git2 = { git = "https://github.com/jonathantanmy2/git2-rs", rev = "9e2ec81c8d37ba5142e5fb97c5afbc2f43c6d7eb" } [workspace.lints.clippy] # Note that `all` doesn't include everything, so extra lints should be added here. diff --git a/crates/but-agentlog/src/gitmeta/tests.rs b/crates/but-agentlog/src/gitmeta/tests.rs index e86eeada183..6497058553b 100644 --- a/crates/but-agentlog/src/gitmeta/tests.rs +++ b/crates/but-agentlog/src/gitmeta/tests.rs @@ -10,7 +10,7 @@ use crate::projection::{ project_pr, }; use crate::transcript::TranscriptBatch; -use but_core::worktree::safe_checkout; +use but_core::worktree::checkout_commit; use git_meta_lib::{MetaEdit, MetaValue, Session, Target}; use gix::object::tree::EntryKind; use std::{fs, path::Path}; @@ -88,8 +88,7 @@ fn commit_file(repo: &Path, path: &str, body: &str) { let commit = repo .new_commit("test commit", tree_id, parents) .expect("write test commit"); - safe_checkout(current_tree_id, commit.id, &repo, Default::default()) - .expect("checkout test commit"); + checkout_commit(&repo, commit.id, Some(current_tree_id)).expect("checkout test commit"); } fn write_transcript(repo: &Path, body: &str) -> std::path::PathBuf { diff --git a/crates/but-api/src/branch.rs b/crates/but-api/src/branch.rs index 6046b90d490..f6eb6c4edc0 100644 --- a/crates/but-api/src/branch.rs +++ b/crates/but-api/src/branch.rs @@ -11,7 +11,7 @@ use but_core::{ sync::RepoExclusive, ui::TreeChanges, update_head_reference, - worktree::{checkout, safe_checkout}, + worktree::checkout_tree, }; use but_ctx::Context; use but_oplog::legacy::{OperationKind, SnapshotDetails, Trailer}; @@ -929,16 +929,7 @@ fn checkout_ref_with_perm( ) })?; - safe_checkout( - current_head, - target, - &repo, - checkout::Options { - skip_head_update: true, - ..Default::default() - }, - ) - .with_context(|| { + checkout_tree(&repo, target, Some(current_head)).with_context(|| { format!( "Could not safely check out '{}' from {current_head} to {target}", reference_name.as_bstr() diff --git a/crates/but-core/src/worktree/checkout/function.rs b/crates/but-core/src/worktree/checkout/function.rs index 4558faa6f71..9c262fef344 100644 --- a/crates/but-core/src/worktree/checkout/function.rs +++ b/crates/but-core/src/worktree/checkout/function.rs @@ -13,6 +13,83 @@ use crate::update_head_reference; use super::{Options, Outcome, utils::merge_worktree_changes_into_destination_or_keep_snapshot}; +/// Update the index and working directory (but not any refs). If +/// `baseline_treeish` is None, `HEAD^{tree}` is used instead. +pub fn checkout_tree( + repo: &gix::Repository, + treeish: gix::ObjectId, + baseline_treeish: Option, +) -> anyhow::Result<()> { + let git2_repo = git2::Repository::open(repo.git_dir())?; + let baseline = if let Some(baseline_treeish) = baseline_treeish { + let baseline = git2_repo + .find_object(baseline_treeish.to_git2(), None)? + .peel_to_tree()?; + // libgit2 only pretends that HEAD's tree (and not the index) is the + // given baseline. We also need it to pretend that the index is the + // given baseline. That is not possible with the current API, so for + // now, write to the index to make it the given baseline. + git2_repo.index()?.read_tree(&baseline)?; + Some(baseline) + } else { + None + }; + + let mut opts = git2::build::CheckoutBuilder::new(); + if let Some(ref baseline) = baseline { + opts.baseline(baseline); + } + git2_repo.checkout_tree( + &git2_repo.find_object(treeish.to_git2(), None)?, + Some(&mut opts), + )?; + Ok(()) +} + +/// Same as [checkout_tree()], except that HEAD is updated as well. +pub fn checkout_commit( + repo: &gix::Repository, + commit: gix::ObjectId, + baseline_treeish: Option, +) -> anyhow::Result<()> { + checkout_tree(repo, commit, baseline_treeish)?; + let needs_update = repo + .head()? + .id() + .is_none_or(|actual_head_id| actual_head_id != commit); + if needs_update { + // We play it loose here, as we assume a repository lock so we won't interfere with ourselves. + // Git itself enforces no lock either, so we rely on basic locking ref-locking here. Good enough. + let new_object = commit.attach(repo).object()?; + update_head_reference( + repo, + Target::Object(commit), + true, + "safe checkout", + "GitButler".into(), + new_object.into_commit().parent_ids().count(), + )?; + } + Ok(()) +} + +/// Same as [checkout_commit()], except that only unconflicted commits are allowed. +/// +/// This function exists to preserve the behavior of existing code paths +/// that require unconflicted commits, especially when checking out workspace +/// commits. This should be removed once we support conflicted workspace +/// commits. +pub fn checkout_unconflicted_commit( + repo: &gix::Repository, + commit: gix::ObjectId, + baseline_treeish: Option, +) -> anyhow::Result<()> { + if crate::Commit::from_id(commit.attach(repo))?.is_conflicted() { + bail!("Refusing to check out conflicted commit {commit}"); + } + checkout_commit(repo, commit, baseline_treeish) +} + /// Like [`safe_checkout()`], but the current tree will always be fetched from pub fn safe_checkout_from_head( new_head_id: gix::ObjectId, diff --git a/crates/but-core/src/worktree/checkout/mod.rs b/crates/but-core/src/worktree/checkout/mod.rs index b645b30a5fa..49acd66f3be 100644 --- a/crates/but-core/src/worktree/checkout/mod.rs +++ b/crates/but-core/src/worktree/checkout/mod.rs @@ -8,7 +8,7 @@ pub struct Options { /// If set, use this tree instead of `HEAD^{tree}` as the merge base when /// resolving the worktree snapshot against the new HEAD. /// - /// Set this to `HEAD^{tree}` + consumed changes (additive-only) after a + /// Set this to `HEAD^{tree}` + consumed changes after a /// commit/amend so the consumed hunks cancel in the 3-way merge and don't /// reappear as uncommitted changes. pub merge_base_override: Option, diff --git a/crates/but-core/src/worktree/mod.rs b/crates/but-core/src/worktree/mod.rs index 937fd5bf1b7..2185ca27d41 100644 --- a/crates/but-core/src/worktree/mod.rs +++ b/crates/but-core/src/worktree/mod.rs @@ -4,7 +4,10 @@ pub mod checkout; use std::{io::Read, path::Path}; use bstr::BStr; -pub use checkout::function::{safe_checkout, safe_checkout_from_head}; +pub use checkout::function::{ + checkout_commit, checkout_tree, checkout_unconflicted_commit, safe_checkout, + safe_checkout_from_head, +}; use gix::filter::plumbing::pipeline::convert::ToGitOutcome; /// Read a worktree file into `buf` after converting it to what Git *would* store. diff --git a/crates/but-core/tests/core/worktree/checkout.rs b/crates/but-core/tests/core/worktree/checkout.rs index c3233174068..594df7769a8 100644 --- a/crates/but-core/tests/core/worktree/checkout.rs +++ b/crates/but-core/tests/core/worktree/checkout.rs @@ -1,5 +1,4 @@ -use bstr::ByteSlice; -use but_core::worktree::{checkout, safe_checkout}; +use but_core::worktree::{checkout_commit, checkout_unconflicted_commit}; use but_testsupport::{ CommandExt, git_at_dir, git_status, open_repo, read_only_in_memory_scenario, visualize_commit_graph_all, visualize_disk_tree_skip_dot_git, visualize_index, @@ -18,15 +17,7 @@ fn update_unborn_head() -> anyhow::Result<()> { let empty_tree = repo.empty_tree().id; let head_commit = repo.new_commit("init", empty_tree, None::)?; - let out = safe_checkout(empty_tree, head_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: None, - num_deleted_files: 0, - num_added_or_updated_files: 0, - head_update: "Update refs/heads/main to Some(Object(Sha1(31ec8eacfba4051fd673e4fe23c775e87896a463)))", - } - "#); + checkout_commit(&repo, head_commit.id, Some(empty_tree))?; insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @"* 31ec8ea (HEAD -> main) init"); insta::assert_snapshot!(git_status(&repo)?, @""); @@ -54,15 +45,7 @@ fn no_op_trees_never_touch_worktree() -> anyhow::Result<()> { let a_commit = repo.head_commit()?; let a_tree = a_commit.tree_id()?.detach(); - let out = safe_checkout(a_tree, a_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: None, - num_deleted_files: 0, - num_added_or_updated_files: 0, - head_update: "None", - } - "#); + checkout_commit(&repo, a_commit.id, Some(a_tree))?; // Nothing changed insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @"* 4e26689 (HEAD -> main) init"); @@ -88,23 +71,15 @@ fn conflicted_commits_cannot_be_checked_out() -> anyhow::Result<()> { let normal = repo.rev_parse_single("normal")?.detach(); let conflicted = repo.rev_parse_single("conflicted")?.detach(); - let err = safe_checkout(normal, conflicted, &repo, Default::default()) - .expect_err("safe_checkout must reject GitButler-conflicted commits"); + let err = checkout_unconflicted_commit(&repo, conflicted, Some(normal)) + .expect_err("must reject GitButler-conflicted commits"); assert_eq!( err.to_string(), "Refusing to check out conflicted commit 84503317a1e1464381fcff65ece14bc1f4315b7c", ); - safe_checkout( - repo.head_id()?.detach(), - conflicted, - &repo, - checkout::Options { - allow_conflicted_commit_checkout: true, - ..Default::default() - }, - ) - .expect("internal callers can explicitly opt into conflicted commit checkout"); + checkout_commit(&repo, conflicted, Some(repo.head_id()?.detach())) + .expect("internal callers can explicitly opt into conflicted commit checkout"); Ok(()) } @@ -136,15 +111,7 @@ fn pure_deletion_checkout_does_not_restore_unrelated_worktree_deletions() -> any "delete executable", )?; - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: None, - num_deleted_files: 1, - num_added_or_updated_files: 0, - head_update: "Update refs/heads/main to Some(Object(Sha1(5eedd314adfb480212989a303c7651717062a9b2)))", - } - "#); + checkout_commit(&repo, new_commit.id, Some(head_commit.id))?; insta::assert_snapshot!(visualize_index(&*repo.index()?), @r" 100644:3aac70f file 120000:c4c364c link @@ -171,12 +138,7 @@ fn pure_deletion_checkout_keeps_non_intersecting_worktree_deletion() -> anyhow:: editor.upsert("c.txt", EntryKind::Blob, blob_id)?; let initial_tree_id = editor.write()?.detach(); let initial_commit = repo.new_commit("init", initial_tree_id, None::)?; - safe_checkout( - repo.empty_tree().id, - initial_commit.id, - &repo, - Default::default(), - )?; + checkout_commit(&repo, initial_commit.id, Some(repo.empty_tree().id))?; std::fs::remove_file(repo.workdir_path("b.txt").expect("non-bare repository"))?; insta::assert_snapshot!(git_status(&repo)?, @" D b.txt"); @@ -189,11 +151,7 @@ fn pure_deletion_checkout_keeps_non_intersecting_worktree_deletion() -> anyhow:: }, "delete a.txt", )?; - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - assert!(out.snapshot_tree.is_none()); - assert_eq!(out.num_deleted_files, 1); - assert_eq!(out.num_added_or_updated_files, 0); - assert!(out.head_update.is_some()); + checkout_commit(&repo, new_commit.id, Some(head_commit.id))?; assert!(!repo.workdir_path("a.txt").unwrap().exists()); assert!(!repo.workdir_path("b.txt").unwrap().exists()); @@ -230,12 +188,7 @@ fn pure_deletion_checkout_keeps_empty_worktree_root() -> anyhow::Result<()> { editor.upsert("nested/only.txt", EntryKind::Blob, blob_id)?; let initial_tree_id = editor.write()?.detach(); let initial_commit = repo.new_commit("init", initial_tree_id, None::)?; - safe_checkout( - repo.empty_tree().id, - initial_commit.id, - &repo, - Default::default(), - )?; + checkout_commit(&repo, initial_commit.id, Some(repo.empty_tree().id))?; insta::assert_snapshot!(visualize_disk_tree_skip_dot_git(&worktree)?, @r" . @@ -251,9 +204,7 @@ fn pure_deletion_checkout_keeps_empty_worktree_root() -> anyhow::Result<()> { }, "delete only file", )?; - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - assert_eq!(out.num_deleted_files, 1); - assert_eq!(out.num_added_or_updated_files, 0); + checkout_commit(&repo, new_commit.id, Some(head_commit.id))?; assert!( worktree.is_dir(), "safe checkout must not delete the worktree root while cleaning up empty parents" @@ -281,7 +232,7 @@ fn worktree_and_index_deletions_are_ignored_in_snapshots() -> anyhow::Result<()> "); // Turn deleted files into directory - these won't conflict no matter what they were in the index. - let (head_commit, new_commit) = build_commit( + let (_head_commit, new_commit) = build_commit( &repo, |tree| { let empty_blob = repo.empty_blob(); @@ -295,15 +246,7 @@ fn worktree_and_index_deletions_are_ignored_in_snapshots() -> anyhow::Result<()> "turn changed file into a directory", )?; - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: None, - num_deleted_files: 1, - num_added_or_updated_files: 1, - head_update: "Update refs/heads/main to Some(Object(Sha1(24f802a1250d2f84e1f49094e3b8bb1e5c0d29ad)))", - } - "#); + checkout_commit(&repo, new_commit.id, None)?; // Nothing changed as the checkout was aborted. insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" @@ -374,10 +317,10 @@ this will cause a conflict "edited 'file' (add single line)", )?; - let err = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default()).unwrap_err(); + let err = checkout_commit(&repo, new_commit.id, Some(head_commit.id)).unwrap_err(); assert_eq!( err.to_string(), - "Uncommitted files would be overwritten by checkout: \"file\"", + "1 conflict prevents checkout; class=Checkout (20); code=Conflict (-13)", "we check for conflict markers, and fail." ); // Nothing else changes @@ -402,203 +345,8 @@ this will cause a conflict Ok(()) } -#[test] -fn worktree_snapshot_reapplies_with_hunk_granularity() -> anyhow::Result<()> { - let (repo, _tmp) = writable_scenario("mixed-hunk-modifications"); - insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @"* 647cc94 (HEAD -> main) init"); - insta::assert_snapshot!(visualize_index(&*repo.index()?), @r" - 100755:3d3b36f file - 100755:cb89473 file-in-index - 100644:3d3b36f file-renamed-in-index - 100644:3d3b36f file-to-be-renamed - "); - insta::assert_snapshot!(git_status(&repo)?, @r" - M file - M file-in-index - RM file-to-be-renamed-in-index -> file-renamed-in-index - D file-to-be-renamed - ?? file-renamed - "); - let file_path = repo.workdir_path("file").unwrap(); - let actual = std::fs::read_to_string(&file_path)?; - insta::assert_snapshot!(actual, @r" - 1 - 2 - 3 - 4 - 5 - 6-7 - 8 - 9 - ten - eleven - 12 - 20 - 21 - 22 - 15 - 16 - "); - - // In the target tree, make a surgical edit (one changed line) so the changes should still apply cleany - let (head_commit, new_commit) = build_commit( - &repo, - |tree| { - let blob_id = repo.write_blob( - b"5 -6 -7 -8 -inserted in new tree -9 -10 -11 -12 -13 -14 -15 -16 -17 -18 -", - )?; - tree.upsert("file", EntryKind::Blob, blob_id)?; - Ok(()) - }, - "edited 'file' (add single line)", - )?; - - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default()) - .expect("no error as we keep the snapshot for later"); - // File is still changed, after all we re-applied the worktree changes. - let actual = std::fs::read_to_string(&file_path)?; - insta::assert_snapshot!(actual, @r" - 1 - 2 - 3 - 4 - 5 - 6-7 - 8 - inserted in new tree - 9 - ten - eleven - 12 - 20 - 21 - 22 - 15 - 16 - "); - - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: Some( - Sha1(76de10879a78339980d6a33ecfd6f2f711960106), - ), - num_deleted_files: 0, - num_added_or_updated_files: 1, - head_update: "Update refs/heads/main to Some(Object(Sha1(89b113aeae66a3cb1116bb23a195422edbd6af27)))", - } - "#); - insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" - * 89b113a (HEAD -> main) edited 'file' (add single line) - * 647cc94 init - "); - // `file-to-be-renamed-in-index` is no longer restored by the checkout — the checkout - // only touches `file`, so the index rename (`RM file-to-be-renamed-in-index -> …`) is preserved. - insta::assert_snapshot!(visualize_index(&*repo.index()?), @r" - 100644:832f532 file - 100755:cb89473 file-in-index - 100644:3d3b36f file-renamed-in-index - 100644:3d3b36f file-to-be-renamed - "); - // Notably, 'file' is not in the index anymore, as that now always matches the worktree. - // The rename of `file-to-be-renamed-in-index` and deletion of `file-to-be-renamed` are - // preserved — the checkout only touched `file`. - insta::assert_snapshot!(git_status(&repo)?, @r" - M file - M file-in-index - RM file-to-be-renamed-in-index -> file-renamed-in-index - D file-to-be-renamed - ?? file-renamed - "); - - Ok(()) -} - -#[test] -fn worktree_snapshot_of_legacy_crlf_blob_merges_cleanly_with_independent_target_change() --> anyhow::Result<()> { - let (repo, _tmp) = writable_scenario_slow("legacy-crlf-blob-with-gitattributes"); - let file_path = repo.workdir_path("ImportOrdersJob.cs").unwrap(); - let legacy_blob = repo - .find_object(repo.rev_parse_single("@:ImportOrdersJob.cs")?)? - .into_blob(); - assert_eq!( - legacy_blob.data.as_bstr(), - "1\r\n2\r\n3\r\n", - "the tracked blob must start from digit-only CRLF content so the later spelled-out edits are clearly distinguishable" - ); - - // This write is with line-endings that are unchanged from the ones on disk, and from what's in Git (CRLF). - std::fs::write(&file_path, b"1\r\ntwo from worktree\r\n3\r\n")?; - assert_eq!( - git_status(&repo)?, - " M ImportOrdersJob.cs\n", - "the worktree edit must be visible before checkout" - ); - - let (head_commit, new_commit) = build_commit( - &repo, - |tree| { - // This commit also has the right line endings (CRLF) - let blob_id = repo.write_blob(b"1\r\n2\r\nthree from target\r\n")?; - tree.upsert("ImportOrdersJob.cs", EntryKind::Blob, blob_id)?; - Ok(()) - }, - "edit same legacy crlf file independently", - )?; - - // A lot happens here, but the significant part is that the overlapping worktree changes are cherry-picked - // onto the `new_commit` to be transferred by merge. That snapshot now normalizes line endings correctly, - // so the independent edits merge cleanly instead of being treated as a whole-file conflict. - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: Some( - Sha1(77d39e5c3dae5dde723f5be3c45e3525ef424447), - ), - num_deleted_files: 0, - num_added_or_updated_files: 1, - head_update: "Update refs/heads/main to Some(Object(Sha1(a530b145a2513ba5b2a4418bbb74920d3967f8fb)))", - } - "#); - - assert_eq!( - std::fs::read(&file_path)?.as_bstr(), - "1\r\ntwo from worktree\r\nthree from target\r\n", - "checkout keeps the worktree edit and applies the independent target change" - ); - assert_eq!( - repo.head_id()?, - new_commit.id, - "checkout updates HEAD to the target commit" - ); - - Ok(()) -} - #[test] fn checkout_handles_directory_and_file_replacements() -> anyhow::Result<()> { - if but_testsupport::gix_testtools::is_ci::cached() { - // TODO(gix): remove this once `gitoxide` unconditional reset/checkout is available. - // Fails on checkout on CI Linux as it can't deal with `file`. - // Probably the `git2` OS error code handling isn't working cross-platform? - eprintln!("SKIPPING TEST KNOWN TO FAIL ON CI ONLY"); - return Ok(()); - } let (repo, _tmp) = writable_scenario("merge-with-two-branches-line-offset"); insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" * 2a6d103 (HEAD -> merge) Merge branch 'A' into merge @@ -623,15 +371,7 @@ fn checkout_handles_directory_and_file_replacements() -> anyhow::Result<()> { }, "turn file into a directory", )?; - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: None, - num_deleted_files: 1, - num_added_or_updated_files: 3, - head_update: "Update refs/heads/merge to Some(Object(Sha1(df178e3012ac0862407185ae7dd8d634a6cde677)))", - } - "#); + checkout_commit(&repo, new_commit.id, Some(head_commit.id))?; insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" * df178e3 (HEAD -> merge) turn file into a directory @@ -658,15 +398,7 @@ fn checkout_handles_directory_and_file_replacements() -> anyhow::Result<()> { }, "turn a directory back into a file", )?; - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: None, - num_deleted_files: 3, - num_added_or_updated_files: 1, - head_update: "Update refs/heads/merge to Some(Object(Sha1(94cc54fa25411ad51e319a9895d031d8da97b7ab)))", - } - "#); + checkout_commit(&repo, new_commit.id, Some(head_commit.id))?; insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" * 94cc54f (HEAD -> merge) turn a directory back into a file @@ -710,15 +442,7 @@ fn unrelated_additions_do_not_affect_worktree_changes() -> anyhow::Result<()> { }, "add unrelated file", )?; - let out = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default())?; - insta::assert_debug_snapshot!(out, @r#" - Outcome { - snapshot_tree: None, - num_deleted_files: 0, - num_added_or_updated_files: 1, - head_update: "Update refs/heads/main to Some(Object(Sha1(7add6cadcf636e5b3a6c15c75e82abbec97d6eef)))", - } - "#); + checkout_commit(&repo, new_commit.id, Some(head_commit.id))?; insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r" * 7add6ca (HEAD -> main) add unrelated file @@ -768,10 +492,9 @@ fn partial_commit_with_adjacent_lines_conflicts_on_checkout() -> anyhow::Result< // (added-a) because both add at the same position. Without a merge-base // override that includes the consumed changes, the 3-way merge treats this // as a conflict. - let err = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default()).unwrap_err(); + let err = checkout_commit(&repo, new_commit.id, Some(head_commit.id)).unwrap_err(); assert!( - err.to_string() - .contains("Uncommitted files would be overwritten"), + err.to_string().contains("1 conflict prevents checkout"), "checkout must abort on partial-commit conflict: {err}" ); @@ -802,10 +525,9 @@ fn partial_commit_with_deletion_plus_insertion_conflicts_on_checkout() -> anyhow // The three-way merge sees ours deleting old-line and theirs replacing it // with new-line — both modify the same region. Same class of bug as the // adjacent-line case: commit_create avoids this by skipping checkout entirely. - let err = safe_checkout(head_commit.id, new_commit.id, &repo, Default::default()).unwrap_err(); + let err = checkout_commit(&repo, new_commit.id, Some(head_commit.id)).unwrap_err(); assert!( - err.to_string() - .contains("Uncommitted files would be overwritten"), + err.to_string().contains("1 conflict prevents checkout"), "checkout must abort on partial-commit conflict: {err}" ); diff --git a/crates/but-rebase/src/graph_rebase/materialize.rs b/crates/but-rebase/src/graph_rebase/materialize.rs index 653c4c945aa..432abf3b25a 100644 --- a/crates/but-rebase/src/graph_rebase/materialize.rs +++ b/crates/but-rebase/src/graph_rebase/materialize.rs @@ -1,9 +1,6 @@ //! Functions for materializing a rebase use anyhow::{Context, Result, bail}; -use but_core::{ - ObjectStorageExt as _, RefMetadata, - worktree::{checkout::Options, safe_checkout_from_head}, -}; +use but_core::{ObjectStorageExt as _, RefMetadata, worktree::checkout_tree}; use gix::refs::{ Target, transaction::{Change, LogChange, PreviousValue, RefEdit, RefLog}, @@ -48,15 +45,7 @@ impl<'ws, 'graph, M: RefMetadata> SuccessfulRebase<'ws, 'graph, M> { // If the head has changed (which means it's in the // commit mapping), perform a safe checkout. - safe_checkout_from_head( - new_head, - &repo, - Options { - skip_head_update: true, - merge_base_override, - allow_conflicted_commit_checkout: true, - }, - )?; + checkout_tree(&repo, new_head, merge_base_override)?; } } } diff --git a/crates/but-workspace/src/branch/apply.rs b/crates/but-workspace/src/branch/apply.rs index dc118123f39..80fd2547932 100644 --- a/crates/but-workspace/src/branch/apply.rs +++ b/crates/but-workspace/src/branch/apply.rs @@ -275,14 +275,10 @@ pub fn apply( let current_head_commit = ws.graph.entrypoint()?.commit().context( "The entrypoint must have a commit - it's equal to HEAD, and we skipped unborn earlier", )?; - but_core::worktree::safe_checkout( - current_head_commit.id, - commit_to_checkout, + but_core::worktree::checkout_commit( repo, - but_core::worktree::checkout::Options { - skip_head_update: false, - ..Default::default() - }, + commit_to_checkout, + Some(current_head_commit.id), )?; let applied_branches = vec![branch.to_owned()]; if !branch_has_applied_workspace_metadata(branch.as_ref(), &ws, meta)? { @@ -751,15 +747,7 @@ pub fn apply( storage.persist(repo)?; drop(in_memory_repo); } - but_core::worktree::safe_checkout( - prev_head_id, - new_head_id, - repo, - but_core::worktree::checkout::Options { - skip_head_update: true, - ..Default::default() - }, - )?; + but_core::worktree::checkout_tree(repo, new_head_id, Some(prev_head_id))?; persist_metadata_and_gitconfig( meta, &branches_to_apply, diff --git a/crates/but-workspace/src/commit/mod.rs b/crates/but-workspace/src/commit/mod.rs index 89b532b3e33..75731e7f992 100644 --- a/crates/but-workspace/src/commit/mod.rs +++ b/crates/but-workspace/src/commit/mod.rs @@ -5,38 +5,16 @@ use but_core::ref_metadata::MaybeDebug; use crate::WorkspaceCommit; -/// Build a merge-base override tree from `HEAD^{tree}` + `consumed` changes -/// (additive-only). During checkout, the 3-way snapshot merge uses this as its -/// base so consumed hunks cancel out and don't reappear as uncommitted changes. -/// -/// Two kinds of changes are excluded to keep the tree additive-only: -/// -/// * `previous_path` is stripped so rename-source deletions don't leak in. -/// * Full-file deletions (empty `hunk_headers`, file absent from worktree) are -/// skipped — including them would remove the path from the base, causing the -/// snapshot merge to misinterpret the worktree copy as a new addition. +/// Build a merge-base override tree from `HEAD^{tree}` + `consumed` changes. +/// During checkout, this is used as the baseline so consumed hunks don't +/// reappear as uncommitted changes. fn compute_merge_base_override( repo: &gix::Repository, consumed: Vec, context_lines: u32, ) -> anyhow::Result { let head_tree = repo.head_tree_id_or_empty()?; - let workdir = repo.workdir().context("non-bare repository")?; - let mut specs: Vec<_> = consumed - .into_iter() - .filter(|spec| { - if spec.hunk_headers.is_empty() { - return workdir - .join(gix::path::from_bstr(spec.path.as_bstr())) - .exists(); - } - true - }) - .map(|mut spec| { - spec.previous_path = None; - Ok(spec) - }) - .collect(); + let mut specs: Vec<_> = consumed.into_iter().map(Ok).collect(); if specs.is_empty() { return Ok(head_tree.detach()); } diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs index 722656ec32a..1829952e9e1 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs @@ -104,32 +104,14 @@ impl TestRepo { let repo = self.open(); let current_head = repo.head_id().expect("HEAD peels").detach(); let target = oid.unwrap_or(current_head); - but_core::worktree::safe_checkout( - current_head, - target, - &repo, - but_core::worktree::checkout::Options { - skip_head_update: true, - ..Default::default() - }, - ) - .unwrap(); + but_core::worktree::checkout_tree(&repo, target, Some(current_head)).unwrap(); update_current_head(&repo, target, "reset hard"); } pub fn checkout_commit(&self, commit_oid: gix::ObjectId) { let repo = self.open(); let current_head = repo.head_id().expect("HEAD peels").detach(); - but_core::worktree::safe_checkout( - current_head, - commit_oid, - &repo, - but_core::worktree::checkout::Options { - skip_head_update: true, - ..Default::default() - }, - ) - .unwrap(); + but_core::worktree::checkout_tree(&repo, commit_oid, Some(current_head)).unwrap(); set_head_detached(&repo, commit_oid); } @@ -151,16 +133,7 @@ impl TestRepo { current_head } }; - but_core::worktree::safe_checkout( - current_head, - target, - &repo, - but_core::worktree::checkout::Options { - skip_head_update: true, - ..Default::default() - }, - ) - .unwrap(); + but_core::worktree::checkout_tree(&repo, target, Some(current_head)).unwrap(); set_head_symbolic(&repo, branch_name.as_str()); } @@ -183,16 +156,7 @@ impl TestRepo { None, ) .expect("failed to commit"); - but_core::worktree::safe_checkout( - parent, - commit_id, - &repo, - but_core::worktree::checkout::Options { - skip_head_update: true, - ..Default::default() - }, - ) - .unwrap(); + but_core::worktree::checkout_tree(&repo, commit_id, Some(tree_id)).unwrap(); update_current_head(&repo, commit_id, "commit"); commit_id } diff --git a/crates/gitbutler-oplog/src/oplog.rs b/crates/gitbutler-oplog/src/oplog.rs index 19385fc6429..a040eb724bf 100644 --- a/crates/gitbutler-oplog/src/oplog.rs +++ b/crates/gitbutler-oplog/src/oplog.rs @@ -731,13 +731,12 @@ fn restore_snapshot( let workdir_tree_id = get_workdir_tree(None, snapshot_commit_id, &gix_repo, ctx)?; // Check out the snapshot's worktree while HEAD still points at the pre-restore commit: - // safe_checkout diffs from `before_restore_snapshot_workdir_tree_id`, so + // checkout_tree diffs from `before_restore_snapshot_workdir_tree_id`, so // the workspace ref is repointed only afterwards (below). - but_core::worktree::safe_checkout( - before_restore_snapshot_workdir_tree_id, - workdir_tree_id, + but_core::worktree::checkout_tree( &gix_repo, - but_core::worktree::checkout::Options::default(), + workdir_tree_id, + Some(before_restore_snapshot_workdir_tree_id), )?; // Tracked content now matches the snapshot (untracked files outside the restored diff are