diff --git a/Cargo.lock b/Cargo.lock index a397aac9aee..cbc2a16f873 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1761,14 +1761,10 @@ dependencies = [ "bstr", "but-core", "but-ctx", - "but-meta", - "but-oxidize", + "but-graph", "but-rebase", + "but-serde", "but-testsupport", - "but-workspace", - "gitbutler-branch-actions", - "gitbutler-stack", - "gitbutler-workspace", "gix", "insta", "serde", diff --git a/crates/but-api/src/legacy/worktree.rs b/crates/but-api/src/legacy/worktree.rs index 57d9b7aaaf5..16f6c4987df 100644 --- a/crates/but-api/src/legacy/worktree.rs +++ b/crates/but-api/src/legacy/worktree.rs @@ -15,16 +15,18 @@ pub fn worktree_new( reference: gix::refs::FullName, ) -> Result { let guard = ctx.shared_worktree_access(); + let (repo, ws, _) = ctx.workspace_and_db_with_perm(guard.read_permission())?; - but_worktrees::new::worktree_new(ctx, guard.read_permission(), reference.as_ref()) + but_worktrees::new::worktree_new(&repo, &ws, &ctx.project_data_dir(), reference.as_ref()) } #[but_api] #[instrument(err(Debug))] pub fn worktree_list(ctx: &mut but_ctx::Context) -> Result { - let guard = ctx.shared_worktree_access(); + let _guard = ctx.shared_worktree_access(); + let repo = ctx.repo.get()?; - but_worktrees::list::worktree_list(ctx, guard.read_permission()) + but_worktrees::list::worktree_list(&repo) } #[but_api] @@ -34,11 +36,14 @@ pub fn worktree_integration_status( id: WorktreeId, target: gix::refs::FullName, ) -> Result { - let guard = ctx.exclusive_worktree_access(); + let mut guard = ctx.exclusive_worktree_access(); + let mut meta = ctx.meta()?; + let (repo, mut ws, _) = ctx.workspace_mut_and_db_with_perm(guard.write_permission())?; but_worktrees::integrate::worktree_integration_status( - ctx, - guard.read_permission(), + &repo, + &mut ws, + &mut meta, &id, target.as_ref(), ) @@ -52,13 +57,10 @@ pub fn worktree_integrate( target: gix::refs::FullName, ) -> Result<()> { let mut guard = ctx.exclusive_worktree_access(); + let mut meta = ctx.meta()?; + let (repo, mut ws, _) = ctx.workspace_mut_and_db_with_perm(guard.write_permission())?; - but_worktrees::integrate::worktree_integrate( - ctx, - guard.write_permission(), - &id, - target.as_ref(), - ) + but_worktrees::integrate::worktree_integrate(&repo, &mut ws, &mut meta, &id, target.as_ref()) } #[but_api] @@ -67,9 +69,10 @@ pub fn worktree_destroy_by_id( ctx: &mut but_ctx::Context, id: WorktreeId, ) -> Result { - let mut guard = ctx.exclusive_worktree_access(); + let _guard = ctx.exclusive_worktree_access(); + let repo = ctx.repo.get()?; - but_worktrees::destroy::worktree_destroy_by_id(ctx, guard.write_permission(), &id) + but_worktrees::destroy::worktree_destroy_by_id(&repo, &id) } #[but_api] @@ -78,11 +81,8 @@ pub fn worktree_destroy_by_reference( ctx: &mut but_ctx::Context, reference: gix::refs::FullName, ) -> Result { - let mut guard = ctx.exclusive_worktree_access(); + let _guard = ctx.exclusive_worktree_access(); + let repo = ctx.repo.get()?; - but_worktrees::destroy::worktree_destroy_by_reference( - ctx, - guard.write_permission(), - reference.as_ref(), - ) + but_worktrees::destroy::worktree_destroy_by_reference(&repo, reference.as_ref()) } diff --git a/crates/but-serde/src/lib.rs b/crates/but-serde/src/lib.rs index d788fcd6880..dcb27b227ac 100644 --- a/crates/but-serde/src/lib.rs +++ b/crates/but-serde/src/lib.rs @@ -33,7 +33,7 @@ pub use bstring::BStringForFrontend; /// ``` pub mod bstring_lossy { use bstr::{BString, ByteSlice}; - use serde::Serialize; + use serde::{Deserialize, Deserializer, Serialize}; pub fn serialize(v: &BString, s: S) -> Result where @@ -41,6 +41,13 @@ pub mod bstring_lossy { { v.to_str_lossy().serialize(s) } + + pub fn deserialize<'de, D>(d: D) -> Result + where + D: Deserializer<'de>, + { + Ok(String::deserialize(d)?.into()) + } } /// Use on `gix::refs::FullName` fields that should serialize as JSON strings. diff --git a/crates/but-worktrees/Cargo.toml b/crates/but-worktrees/Cargo.toml index 524d9a98af5..57d8fda5471 100644 --- a/crates/but-worktrees/Cargo.toml +++ b/crates/but-worktrees/Cargo.toml @@ -10,25 +10,20 @@ rust-version.workspace = true doctest = false [dependencies] -but-workspace.workspace = true -but-meta = { workspace = true, features = ["legacy"] } but-rebase.workspace = true but-core.workspace = true -but-ctx.workspace = true - -gitbutler-stack.workspace = true -gitbutler-workspace.workspace = true -gitbutler-branch-actions.workspace = true +but-graph.workspace = true anyhow.workspace = true gix.workspace = true serde.workspace = true +but-serde.workspace = true uuid.workspace = true bstr.workspace = true tracing.workspace = true [dev-dependencies] -but-oxidize.workspace = true +but-ctx.workspace = true but-testsupport.workspace = true insta.workspace = true diff --git a/crates/but-worktrees/src/destroy.rs b/crates/but-worktrees/src/destroy.rs index bc526b77590..591993d5676 100644 --- a/crates/but-worktrees/src/destroy.rs +++ b/crates/but-worktrees/src/destroy.rs @@ -1,5 +1,4 @@ use anyhow::Result; -use but_ctx::{Context, access::RepoExclusive}; use serde::Serialize; use crate::{WorktreeId, git::git_worktree_remove, list::worktree_list}; @@ -13,12 +12,11 @@ pub struct DestroyWorktreeOutcome { /// Destroys a worktree by its ID. pub fn worktree_destroy_by_id( - ctx: &mut Context, - _perm: &RepoExclusive, + repo: &gix::Repository, id: &WorktreeId, ) -> Result { // Remove the git worktree (force=true to handle uncommitted changes) - git_worktree_remove(ctx.repo.get()?.common_dir(), id, true)?; + git_worktree_remove(repo.common_dir(), id, true)?; Ok(DestroyWorktreeOutcome { destroyed_ids: vec![id.clone()], @@ -27,12 +25,11 @@ pub fn worktree_destroy_by_id( /// Destroys all worktrees created from a given reference. pub fn worktree_destroy_by_reference( - ctx: &mut Context, - perm: &RepoExclusive, + repo: &gix::Repository, reference: &gix::refs::FullNameRef, ) -> Result { // Use the existing list function to get all worktrees - let list_outcome = worktree_list(ctx, perm.read_permission())?; + let list_outcome = worktree_list(repo)?; // Filter for worktrees created from the specified reference let worktrees_to_destroy: Vec<_> = list_outcome @@ -51,7 +48,7 @@ pub fn worktree_destroy_by_reference( // Destroy each matching worktree for worktree in worktrees_to_destroy { // Remove the git worktree (force=true to handle uncommitted changes) - git_worktree_remove(ctx.repo.get()?.common_dir(), &worktree.id, true)?; + git_worktree_remove(repo.common_dir(), &worktree.id, true)?; destroyed_ids.push(worktree.id); } diff --git a/crates/but-worktrees/src/git.rs b/crates/but-worktrees/src/git.rs index c8d8ad1579d..6c47c5c1c0a 100644 --- a/crates/but-worktrees/src/git.rs +++ b/crates/but-worktrees/src/git.rs @@ -1,43 +1,38 @@ use std::path::Path; use anyhow::{Result, bail}; -use bstr::BString; use crate::WorktreeId; /// Creates a git worktree. -/// -/// Git does not accept fully qualified branch names. The given partial ref will -/// be written out under `refs/heads` -/// -/// Returns the full reference. pub(crate) fn git_worktree_add( project_path: &Path, path: &Path, - branch_name: &gix::refs::PartialNameRef, commit: gix::ObjectId, -) -> Result { +) -> Result<()> { let output = std::process::Command::from(gix::command::prepare(gix::path::env::exe_invocation())) .current_dir(project_path) .arg("worktree") .arg("add") - .args(["-B", &branch_name.to_string()]) + .arg("--detach") .arg(path.as_os_str()) .arg(commit.to_string()) + .stderr(std::process::Stdio::piped()) .output()?; - tracing::info!("{}", str::from_utf8(&output.stdout)?); - tracing::error!("{}", str::from_utf8(&output.stderr)?); + tracing::debug!( + stdout = %String::from_utf8_lossy(&output.stdout), + stderr = %String::from_utf8_lossy(&output.stderr), + "git worktree add" + ); if output.status.success() { - let mut out = BString::from(b"refs/heads/"); - out.extend_from_slice(branch_name.as_bstr()); - Ok(gix::refs::FullName::try_from(out)?) + Ok(()) } else { bail!( "Failed to create worktree\n\n{}", - str::from_utf8(&output.stderr).unwrap_or("") + String::from_utf8_lossy(&output.stderr) ) } } @@ -55,17 +50,20 @@ pub(crate) fn git_worktree_remove(project_path: &Path, id: &WorktreeId, force: b command.arg("--force"); } - let output = command.output()?; + let output = command.stderr(std::process::Stdio::piped()).output()?; - tracing::info!("{}", str::from_utf8(&output.stdout)?); - tracing::error!("{}", str::from_utf8(&output.stderr)?); + tracing::debug!( + stdout = %String::from_utf8_lossy(&output.stdout), + stderr = %String::from_utf8_lossy(&output.stderr), + "git worktree remove" + ); if output.status.success() { Ok(()) } else { bail!( - "Failed to create worktree\n\n{}", - str::from_utf8(&output.stderr).unwrap_or("") + "Failed to remove worktree\n\n{}", + String::from_utf8_lossy(&output.stderr) ) } } diff --git a/crates/but-worktrees/src/integrate.rs b/crates/but-worktrees/src/integrate.rs index 8a585482ecf..a9ec9fca075 100644 --- a/crates/but-worktrees/src/integrate.rs +++ b/crates/but-worktrees/src/integrate.rs @@ -1,20 +1,8 @@ use anyhow::{Context as _, Result, bail}; -use bstr::{BString, ByteSlice}; -use but_core::RepositoryExt; -use but_ctx::{ - Context, - access::{RepoExclusive, RepoShared}, -}; -use but_rebase::{Rebase, RebaseOutput, RebaseStep}; -use but_workspace::legacy::stack_ext::StackExt; -use gitbutler_branch_actions::update_workspace_commit; -#[expect( - deprecated, - reason = "VirtualBranchesHandle should be replaced with ctx.workspace_* helpers" -)] -use gitbutler_stack::{Stack, VirtualBranchesHandle}; -use gitbutler_workspace::branch_trees::{ - WorkspaceState, merge_workspace, move_tree_has_conflicts, update_uncommitted_changes, +use bstr::BString; +use but_core::{RefMetadata, RepositoryExt as _}; +use but_rebase::graph_rebase::{ + Editor, LookupStep as _, Step, SuccessfulRebase, mutate::InsertSide, }; use gix::prelude::ObjectIdExt as _; use serde::{Deserialize, Serialize}; @@ -22,11 +10,19 @@ use serde::{Deserialize, Serialize}; use crate::{WorktreeId, db::get_worktree_meta, git::git_worktree_remove}; #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] -#[serde(tag = "type", content = "data", rename_all = "camelCase")] +#[serde( + tag = "type", + content = "data", + rename_all = "camelCase", + rename_all_fields = "camelCase" +)] /// This gets used as a public API in the CLI so be careful when modifying. pub enum WorktreeIntegrationStatus { NoMergeBaseFound, WorktreeIsBare, + /// The worktree's tree is identical to its base; there are no changes to + /// bring back into the workspace. + NothingToIntegrate, /// If we were to integrate this worktree back into the project, it would /// cause the workspace to conflict. /// @@ -38,83 +34,88 @@ pub enum WorktreeIntegrationStatus { /// Commits above where this worktree will be cherry-picked are going to /// end up conflicted. commits_above_conflict: bool, - /// Whether the uncommitted changes in the main checkout will end up - /// conflicted + /// Whether the uncommitted changes in the main checkout would conflict + /// with the integration result. If this is true, integration aborts to + /// keep those changes intact. working_dir_conflicts: bool, }, } -/// Determines whether a worktree is integrated -/// -/// This function makes use of older APIs because there is not yet an -/// alternative to the rebase engine. -pub fn worktree_integration_status( - ctx: &mut Context, - perm: &RepoShared, +/// Determines whether a worktree can be integrated into `target`. +pub fn worktree_integration_status( + repo: &gix::Repository, + ws: &mut but_graph::Workspace, + meta: &mut M, id: &WorktreeId, target: &gix::refs::FullNameRef, ) -> Result { - Ok(worktree_integration_inner(ctx, perm, id, target)?.0) + Ok(worktree_integration_inner(repo, ws, meta, id, target)?.0) } -/// Integrates a worktree if it's integratable -/// -/// This function makes use of older APIs because there is not yet an -/// alternative to the rebase engine. -pub fn worktree_integrate( - ctx: &mut Context, - perm: &mut RepoExclusive, +/// Integrates a worktree if it's integratable: the worktree's state is +/// squashed into a single commit which becomes the new tip of `target`, +/// then the linked worktree checkout is removed. +pub fn worktree_integrate( + repo: &gix::Repository, + ws: &mut but_graph::Workspace, + meta: &mut M, id: &WorktreeId, target: &gix::refs::FullNameRef, ) -> Result<()> { - let before = WorkspaceState::create(ctx, perm.read_permission())?; - - let result = worktree_integration_inner(ctx, perm.read_permission(), id, target)?; - let (WorktreeIntegrationStatus::Integratable { .. }, Some(mut status)) = result else { - bail!("Worktree failed integration checks"); + let result = worktree_integration_inner(repo, ws, meta, id, target)?; + let (WorktreeIntegrationStatus::Integratable { .. }, Some(rebase)) = result else { + match result.0 { + WorktreeIntegrationStatus::NoMergeBaseFound => { + bail!("Cannot integrate worktree: no merge base found with {target}") + } + WorktreeIntegrationStatus::WorktreeIsBare => { + bail!("Cannot integrate worktree: it is bare") + } + WorktreeIntegrationStatus::NothingToIntegrate => bail!( + "Worktree has no changes to integrate. Use `but worktree destroy` to remove it" + ), + WorktreeIntegrationStatus::CausesWorkspaceConflicts => { + bail!("Cannot integrate worktree: it would cause conflicts in the workspace") + } + WorktreeIntegrationStatus::Integratable { .. } => { + bail!("Worktree failed integration checks") + } + } }; - status - .stack - .set_heads_from_rebase_output(ctx, status.rebase_output.references)?; - ctx.invalidate_workspace_cache()?; - let after = - WorkspaceState::create_from_heads(ctx, perm.read_permission(), &status.after_heads)?; - update_uncommitted_changes(ctx, before, after, perm)?; - update_workspace_commit(ctx, false)?; + // Persists the new commits, updates refs, and safely updates the main + // checkout. Uncommitted changes that would conflict abort the operation + // before any ref is touched. + rebase + .materialize() + .context("Failed to integrate worktree into the workspace")?; - git_worktree_remove(ctx.repo.get()?.common_dir(), id, true)?; + git_worktree_remove(repo.common_dir(), id, true)?; Ok(()) } -struct IntegrationResult { - rebase_output: RebaseOutput, - stack: Stack, - after_heads: Vec, -} - -/// Performs the workspace integration operations in memory, returning the -/// status, and output if it's integratable -#[expect( - deprecated, - reason = "VirtualBranchesHandle should be replaced with ctx.workspace_* helpers" -)] -fn worktree_integration_inner( - ctx: &mut Context, - perm: &RepoShared, +/// Performs the workspace integration in-memory using the graph editor, +/// returning the status, and the un-materialized rebase if it's integratable. +fn worktree_integration_inner<'ws, 'meta, M: RefMetadata>( + repo: &gix::Repository, + ws: &'ws mut but_graph::Workspace, + meta: &'meta mut M, id: &WorktreeId, target: &gix::refs::FullNameRef, -) -> Result<(WorktreeIntegrationStatus, Option)> { - let repo = ctx.repo.get()?.clone().for_tree_diffing()?; - - let target_ref = repo.find_reference(target)?; +) -> Result<( + WorktreeIntegrationStatus, + Option>, +)> { + if !ws.refname_is_segment(target) { + bail!("Branch {} not found in workspace", target.shorten()); + } let git_worktree = repo .worktrees()? .into_iter() .find(|w| w.id() == id.as_bstr()) - .expect("Health dictates this exists"); + .with_context(|| format!("Worktree '{id}' not found"))?; let worktree_repo = git_worktree.into_repo()?; let worktree_head = worktree_repo.head()?; let Some(worktree_head_id) = worktree_head.id() else { @@ -122,7 +123,8 @@ fn worktree_integration_inner( }; // Find the base which we will use for the "cherry pick". - let wt_meta = get_worktree_meta(&repo, id)?; + let target_tip = repo.find_reference(target)?.id().detach(); + let wt_meta = get_worktree_meta(repo, id)?; let base = { // If we have worktree metadata and the base hasn't been dropped entirely // from history, we will use that. @@ -131,26 +133,46 @@ fn worktree_integration_inner( { wt_meta.base } else { - let Ok(merge_base) = repo.merge_base(target_ref.id(), worktree_head_id) else { + let Ok(merge_base) = repo.merge_base(target_tip, worktree_head_id.detach()) else { return Ok((WorktreeIntegrationStatus::NoMergeBaseFound, None)); }; merge_base.detach() } }; - // Create a squash commit which we will then cherry pick into the - // target branch - #[expect(deprecated)] + // The squashed state of the worktree, including its uncommitted changes. + #[expect( + deprecated, + reason = "no alternative yet for snapshotting a worktree into a tree" + )] let wd_tree = worktree_repo.create_wd_tree(0)?; + if wd_tree == repo.find_commit(base)?.tree_id()? { + return Ok((WorktreeIntegrationStatus::NothingToIntegrate, None)); + } + + // State needed later which can't be read while the editor borrows `ws`. + let ws_commit_id = ws.graph.managed_entrypoint_commit(repo)?.map(|c| c.id); + let head_id = repo.head_id().ok().map(|id| id.detach()); + let head_tree_id = repo.head_tree_id_or_empty()?.detach(); + #[expect( + deprecated, + reason = "no alternative yet for snapshotting a worktree into a tree" + )] + let main_wd_tree = repo.create_wd_tree(0)?; + let author = repo .author() .transpose() .ok() .flatten() - .context("Failed to find author signautre")? + .context("Failed to find author signature")? .to_owned()?; - let commit = gix::objs::Commit { + let mut editor = Editor::create(ws, meta, repo)?; + + // Create the squash commit in the editor's in-memory repository; it is + // only persisted if the result gets materialized. + let squash_commit = gix::objs::Commit { tree: wd_tree, parents: [base].into(), author: author.clone(), @@ -159,117 +181,54 @@ fn worktree_integration_inner( message: BString::from("Integrated worktree"), extra_headers: vec![], }; - let commit_id = repo.write_object(commit)?; - - let vb_handle = VirtualBranchesHandle::new(ctx.project_data_dir()); - let stacks = vb_handle.list_stacks_in_workspace()?; - let stack = stacks - .iter() - .find(|s| { - s.branches() - .iter() - .any(|b| b.name.as_bytes() == target.shorten()) - }) - .context("Failed to find branch in vb state")? - .clone(); - - let mut steps = stack.as_rebase_steps(ctx)?; - let Some(to_insert_at) = steps.iter().enumerate().find_map(|(i, entry)| { - if let RebaseStep::Reference(reference) = entry { - let matches_target = match reference { - but_core::Reference::Git(reference) => reference.as_ref() == target, - but_core::Reference::Virtual(vref) => { - target.shorten() == BString::from(vref.clone()).as_bstr() - } - }; - - if matches_target { - return Some(i); - } - } - - None - }) else { - bail!("Failed to find point to insert at"); - }; - - steps.insert( - to_insert_at, - RebaseStep::Pick { - commit_id: commit_id.detach(), - new_message: None, - }, - ); - - let mut rebase = Rebase::new(&repo, stack.merge_base(ctx)?, None)?; - rebase.steps(steps)?; - rebase.rebase_noops(false); - let output = rebase.rebase()?; - - // Does the new stack tip conflict with any of the other stacks. - let tip_tree = repo.find_commit(output.top_commit)?.tree_id()?; - let cache = repo.commit_graph_if_enabled()?; - let mut graph = repo.revision_graph(cache.as_ref()); - for stack in stacks.iter().filter(|s| s.id != stack.id) { - let head_id = stack.head_oid(ctx)?; - let head_tree = repo.find_commit(head_id)?.tree_id()?; - let merge_base = repo.merge_base_with_graph(head_id, output.top_commit, &mut graph)?; - let merge_base_tree = repo.find_commit(merge_base)?.tree_id()?; - - if !repo.merges_cleanly( - merge_base_tree.detach(), - head_tree.detach(), - tip_tree.detach(), - )? { + let squash_id = editor.repo().write_object(&squash_commit)?.detach(); + + // The squash commit becomes the new tip of `target`; everything that + // pointed at the old tip (including the workspace commit) follows. + let squash_selector = editor.insert( + target, + Step::new_untracked_pick(squash_id), + InsertSide::Below, + )?; + + let rebase = match editor.rebase() { + Ok(rebase) => rebase, + Err(err) => { + // The workspace commit is the only non-conflictable pick in the + // graph, so failing to rebuild the workspace with the squash + // inserted means the workspace would conflict. + tracing::debug!("worktree integration rebase failed: {err:#}"); return Ok((WorktreeIntegrationStatus::CausesWorkspaceConflicts, None)); } - } - - let cherry_pick_conflicts = { - let Some(row) = output - .commit_mapping - .iter() - .find(|m| m.1 == commit_id.detach()) - else { - bail!("Cherry-pick did not end up in rebase output"); - }; - - let commit = but_core::Commit::from_id(row.2.attach(&repo))?; - commit.is_conflicted() }; - let commits_above_conflict = output - .commit_mapping - .iter() - .filter(|r| r.1 != commit_id.detach()) - .any(|row| { - if let Ok(commit) = but_core::Commit::from_id(row.2.attach(&repo)) { - commit.is_conflicted() - } else { - false + // Inspect the in-memory result for conflicts. + let in_memory_repo = rebase.repo(); + let new_squash_id = rebase.lookup_pick(squash_selector)?; + let cherry_pick_conflicts = + but_core::Commit::from_id(new_squash_id.attach(in_memory_repo))?.is_conflicted(); + + let mappings = rebase.history.commit_mappings(); + let commits_above_conflict = mappings.iter().any(|(old, new)| { + Some(*old) != ws_commit_id + && *new != new_squash_id + && but_core::Commit::from_id(new.attach(in_memory_repo)) + .is_ok_and(|c| c.is_conflicted()) + }); + + // Predict whether the safe checkout of the new workspace state would + // conflict with uncommitted changes in the main worktree. + let working_dir_conflicts = if main_wd_tree == head_tree_id { + false + } else { + match head_id.and_then(|head| mappings.get(&head).copied()) { + // HEAD is not rewritten, the checkout won't touch anything. + None => false, + Some(new_head_id) => { + let new_head_tree = in_memory_repo.find_commit(new_head_id)?.tree_id()?.detach(); + !in_memory_repo.merges_cleanly(head_tree_id, main_wd_tree, new_head_tree)? } - }); - - #[expect(deprecated)] - let wd_tree = repo.create_wd_tree(0)?; - - let before_heads = stacks - .iter() - .map(|s| s.head_oid(ctx)) - .collect::>>()?; - let mut after_heads = stacks - .iter() - .filter(|s| s.id != stack.id) - .map(|s| s.head_oid(ctx)) - .collect::>>()?; - after_heads.push(output.top_commit); - - let working_dir_conflicts = { - let before = WorkspaceState::create_from_heads(ctx, perm, &before_heads)?; - let before = merge_workspace(&repo, &before)?; - let after = WorkspaceState::create_from_heads(ctx, perm, &after_heads)?; - let after = merge_workspace(&repo, &after)?; - move_tree_has_conflicts(ctx, wd_tree, before, after)? + } }; Ok(( @@ -278,10 +237,6 @@ fn worktree_integration_inner( commits_above_conflict, working_dir_conflicts, }, - Some(IntegrationResult { - rebase_output: output, - stack, - after_heads, - }), + Some(rebase), )) } diff --git a/crates/but-worktrees/src/lib.rs b/crates/but-worktrees/src/lib.rs index a7e2c9dc803..494afeeb911 100644 --- a/crates/but-worktrees/src/lib.rs +++ b/crates/but-worktrees/src/lib.rs @@ -18,7 +18,7 @@ pub mod new; /// A worktree name. #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] -pub struct WorktreeId(BString); +pub struct WorktreeId(#[serde(with = "but_serde::bstring_lossy")] BString); impl WorktreeId { /// Create a new worktree ID using a random UUID. @@ -81,7 +81,9 @@ pub struct Worktree { /// The canonicalized filesystem path to the worktree. pub path: PathBuf, /// The git reference this worktree was created from. + #[serde(with = "but_serde::fullname_lossy_opt")] pub created_from_ref: Option, /// The base which we will use in a cherry-pick. + #[serde(with = "but_serde::object_id_opt")] pub base: Option, } diff --git a/crates/but-worktrees/src/list.rs b/crates/but-worktrees/src/list.rs index 5ecd1343cd8..cfd29fb9139 100644 --- a/crates/but-worktrees/src/list.rs +++ b/crates/but-worktrees/src/list.rs @@ -1,5 +1,4 @@ use anyhow::Result; -use but_ctx::{Context, access::RepoShared}; use serde::Serialize; use crate::{Worktree, WorktreeId, db::list_worktree_meta}; @@ -12,9 +11,7 @@ pub struct ListWorktreeOutcome { } /// Lists worktrees -pub fn worktree_list(ctx: &mut Context, _perm: &RepoShared) -> Result { - let repo = &*ctx.repo.get()?; - +pub fn worktree_list(repo: &gix::Repository) -> Result { let metas = list_worktree_meta(repo)?; let entries = repo diff --git a/crates/but-worktrees/src/new.rs b/crates/but-worktrees/src/new.rs index 55fee19a2d0..bc3af102874 100644 --- a/crates/but-worktrees/src/new.rs +++ b/crates/but-worktrees/src/new.rs @@ -1,7 +1,6 @@ use std::path::{Path, PathBuf}; use anyhow::{Result, bail}; -use but_ctx::{Context, access::RepoShared}; use serde::Serialize; use crate::{Worktree, WorktreeId, WorktreeMeta, db::save_worktree_meta, git::git_worktree_add}; @@ -13,14 +12,14 @@ pub struct NewWorktreeOutcome { pub created: Worktree, } -/// Creates a new worktree off of workspace branch with given `refname`. -// TODO: make this plumbing to take the `but_graph::Workspace` directly. +/// Creates a new worktree off of workspace branch with given `refname`, +/// checked out under `data_dir`. pub fn worktree_new( - ctx: &mut Context, - perm: &RepoShared, + repo: &gix::Repository, + ws: &but_graph::Workspace, + data_dir: &Path, refname: &gix::refs::FullNameRef, ) -> Result { - let (repo, ws, _) = ctx.workspace_and_db_with_perm(perm)?; if !ws.refname_is_segment(refname) { bail!("Branch not found in workspace"); } @@ -30,16 +29,8 @@ pub fn worktree_new( // Generate a new worktree ID let id = WorktreeId::generate(); - let path = worktree_workdir(&ctx.project_data_dir(), &id); - let branch_name = - gix::refs::PartialName::try_from(format!("gitbutler/worktree/{}", id.as_bstr()))?; - - git_worktree_add( - repo.common_dir(), - &path, - branch_name.as_ref(), - to_checkout.detach(), - )?; + let path = worktree_workdir(data_dir, &id); + git_worktree_add(repo.common_dir(), &path, to_checkout.detach())?; let path = path.canonicalize()?; @@ -49,7 +40,7 @@ pub fn worktree_new( base: to_checkout.detach(), }; - save_worktree_meta(&repo, meta)?; + save_worktree_meta(repo, meta)?; Ok(NewWorktreeOutcome { created: Worktree { diff --git a/crates/but-worktrees/tests/worktree/main.rs b/crates/but-worktrees/tests/worktree/main.rs index 41aa2b91760..248cc4af06f 100644 --- a/crates/but-worktrees/tests/worktree/main.rs +++ b/crates/but-worktrees/tests/worktree/main.rs @@ -1,48 +1,88 @@ -#![expect( - deprecated, - reason = "VirtualBranchesHandle should be replaced with ctx.workspace_* helpers" -)] - /// Tests for worktree creation and management mod util { use but_ctx::Context; use but_testsupport::gix_testtools::tempfile::TempDir; - use gitbutler_stack::VirtualBranchesHandle; + use but_worktrees::{ + WorktreeId, + destroy::DestroyWorktreeOutcome, + integrate::{WorktreeIntegrationStatus, worktree_integrate, worktree_integration_status}, + list::ListWorktreeOutcome, + new::NewWorktreeOutcome, + }; pub fn test_ctx(name: &str) -> anyhow::Result { let (repo, tmpdir) = but_testsupport::writable_scenario(name); - // TODO: all this should work without `Context` once it's switched to the new rebase engine, - // making this crate either obsolete or proper plumbing. - let mut ctx = Context::from_repo(repo)?; - // update the vb-toml metadata - trigger reconciliation and write the vb.toml according to what's there. - { - let _guard = ctx.exclusive_worktree_access(); - let meta = ctx.legacy_meta()?; - meta.write_reconciled(&*ctx.repo.get()?)?; - } - let handle = VirtualBranchesHandle::new(ctx.project_data_dir()); - - Ok(TestContext { - ctx, - handle, - tmpdir, - }) + let ctx = Context::from_repo(repo)?; + + Ok(TestContext { ctx, tmpdir }) } - #[expect(unused)] pub struct TestContext { pub ctx: Context, - pub handle: VirtualBranchesHandle, + #[expect(unused)] pub tmpdir: TempDir, } + + /// Derive the narrow inputs `worktree_new` needs from `ctx`. + pub fn worktree_new( + ctx: &Context, + perm: &but_ctx::access::RepoShared, + refname: &gix::refs::FullNameRef, + ) -> anyhow::Result { + let (repo, ws, _) = ctx.workspace_and_db_with_perm(perm)?; + but_worktrees::new::worktree_new(&repo, &ws, &ctx.project_data_dir(), refname) + } + + pub fn worktree_list(ctx: &Context) -> anyhow::Result { + let repo = ctx.repo.get()?; + but_worktrees::list::worktree_list(&repo) + } + + pub fn worktree_destroy_by_id( + ctx: &Context, + id: &WorktreeId, + ) -> anyhow::Result { + let repo = ctx.repo.get()?; + but_worktrees::destroy::worktree_destroy_by_id(&repo, id) + } + + pub fn worktree_destroy_by_reference( + ctx: &Context, + reference: &gix::refs::FullNameRef, + ) -> anyhow::Result { + let repo = ctx.repo.get()?; + but_worktrees::destroy::worktree_destroy_by_reference(&repo, reference) + } + + /// Derive the narrow inputs the integration functions need from `ctx`. + pub fn integration_status( + ctx: &Context, + perm: &but_ctx::access::RepoExclusive, + id: &WorktreeId, + target: &gix::refs::FullNameRef, + ) -> anyhow::Result { + let mut meta = ctx.meta()?; + let (repo, mut ws, _) = ctx.workspace_mut_and_db_with_perm(perm)?; + worktree_integration_status(&repo, &mut ws, &mut meta, id, target) + } + + /// Derive the narrow inputs the integration functions need from `ctx`. + pub fn integrate( + ctx: &Context, + perm: &but_ctx::access::RepoExclusive, + id: &WorktreeId, + target: &gix::refs::FullNameRef, + ) -> anyhow::Result<()> { + let mut meta = ctx.meta()?; + let (repo, mut ws, _) = ctx.workspace_mut_and_db_with_perm(perm)?; + worktree_integrate(&repo, &mut ws, &mut meta, id, target) + } } mod worktree_new; mod worktree_list { - use but_worktrees::{list::worktree_list, new::worktree_new}; - - use crate::util::test_ctx; + use crate::util::{test_ctx, worktree_list, worktree_new}; #[test] fn can_list_worktrees() -> anyhow::Result<()> { @@ -53,17 +93,17 @@ mod worktree_list { let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; let feature_c_name = gix::refs::FullName::try_from("refs/heads/feature-c")?; - let a = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; - let b = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; - let c = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; - let d = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; - let e = worktree_new(&mut ctx, guard.read_permission(), feature_c_name.as_ref())?; + let a = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + let b = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + let c = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + let d = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + let e = worktree_new(&ctx, guard.read_permission(), feature_c_name.as_ref())?; let all = &[&a, &b, &c, &d, &e]; // All should start normal assert!( - worktree_list(&mut ctx, guard.read_permission())? + worktree_list(&ctx)? .entries .iter() .all(|e| all.iter().any(|a| a.created == *e)) diff --git a/crates/but-worktrees/tests/worktree/various.rs b/crates/but-worktrees/tests/worktree/various.rs index 1df5f4a0113..4c590a052fb 100644 --- a/crates/but-worktrees/tests/worktree/various.rs +++ b/crates/but-worktrees/tests/worktree/various.rs @@ -1,11 +1,8 @@ use bstr::ByteSlice; use but_testsupport::{git, invoke_bash_at_dir}; -use but_worktrees::{ - integrate::{WorktreeIntegrationStatus, worktree_integrate, worktree_integration_status}, - new::worktree_new, -}; +use but_worktrees::integrate::WorktreeIntegrationStatus; -use crate::util::test_ctx; +use crate::util::{integrate, integration_status, test_ctx, worktree_new}; #[test] fn create_unrelated_change_and_reintroduce() -> anyhow::Result<()> { @@ -16,7 +13,7 @@ fn create_unrelated_change_and_reintroduce() -> anyhow::Result<()> { let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; let feature_b_name = gix::refs::FullName::try_from("refs/heads/feature-b")?; - let a = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; + let a = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; invoke_bash_at_dir( r#"echo "foo" > qux.txt && git add . && git commit -am "added qux!""#, @@ -24,9 +21,9 @@ fn create_unrelated_change_and_reintroduce() -> anyhow::Result<()> { ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &a.created.id, feature_a_name.as_ref() )?, @@ -38,9 +35,9 @@ fn create_unrelated_change_and_reintroduce() -> anyhow::Result<()> { "We should be able to integrate the unrelated change back into the original reference" ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &a.created.id, feature_b_name.as_ref() )?, @@ -52,8 +49,8 @@ fn create_unrelated_change_and_reintroduce() -> anyhow::Result<()> { "We should also be able to integrate the unrelated change back into the above reference" ); - worktree_integrate( - &mut ctx, + integrate( + &ctx, guard.write_permission(), &a.created.id, feature_a_name.as_ref(), @@ -70,7 +67,6 @@ fn create_unrelated_change_and_reintroduce() -> anyhow::Result<()> { "#); // cannot show hashes as these aren't controllable yet. - // TODO: when making this 'modern', ensure we fully isolate it. let unstable_log = git(&repo) .args(["log", "--graph", "--pretty=format:%s %d"]) .output()? @@ -99,7 +95,7 @@ fn causes_conflicts_above() -> anyhow::Result<()> { let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; let feature_b_name = gix::refs::FullName::try_from("refs/heads/feature-b")?; - let a = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; + let a = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; invoke_bash_at_dir( r#"echo "foo" > foo.txt && git add . && git commit -am "added conflicts above!""#, @@ -107,9 +103,9 @@ fn causes_conflicts_above() -> anyhow::Result<()> { ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &a.created.id, feature_a_name.as_ref() )?, @@ -121,9 +117,9 @@ fn causes_conflicts_above() -> anyhow::Result<()> { "When integrating into feature-a, it should cause the commits above which touch foo.txt to conflict" ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &a.created.id, feature_b_name.as_ref() )?, @@ -135,8 +131,8 @@ fn causes_conflicts_above() -> anyhow::Result<()> { "When integrating into feature-b, the resulting commit should end up conflicted" ); - worktree_integrate( - &mut ctx, + integrate( + &ctx, guard.write_permission(), &a.created.id, feature_a_name.as_ref(), @@ -175,13 +171,12 @@ fn causes_conflicts_above() -> anyhow::Result<()> { fn causes_workdir_conflicts_simple() -> anyhow::Result<()> { let test_ctx = test_ctx("stacked-branches")?; let mut ctx = test_ctx.ctx; - ctx.settings.feature_flags.cv3 = false; let main_worktree_dir = ctx.workdir()?.expect("non-bare"); let mut guard = ctx.exclusive_worktree_access(); let feature_b_name = gix::refs::FullName::try_from("refs/heads/feature-b")?; - let b = worktree_new(&mut ctx, guard.read_permission(), feature_b_name.as_ref())?; + let b = worktree_new(&ctx, guard.read_permission(), feature_b_name.as_ref())?; invoke_bash_at_dir(r#"echo "qux" > foo.txt"#, &main_worktree_dir); invoke_bash_at_dir( @@ -190,9 +185,9 @@ fn causes_workdir_conflicts_simple() -> anyhow::Result<()> { ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &b.created.id, feature_b_name.as_ref() )?, @@ -204,24 +199,35 @@ fn causes_workdir_conflicts_simple() -> anyhow::Result<()> { "In this case, we're putting a new commit on the top of the stack - the thing that should conflict is the working directory" ); - worktree_integrate( - &mut ctx, + let feature_b_tip_before = ctx + .repo + .get()? + .find_reference(feature_b_name.as_ref())? + .id() + .detach(); + let err = integrate( + &ctx, guard.write_permission(), &b.created.id, feature_b_name.as_ref(), ) - .expect("it works"); + .expect_err("integration aborts instead of clobbering uncommitted changes"); + assert!( + format!("{err:#}").contains("Failed to integrate worktree"), + "unexpected error: {err:#}" + ); let foo = std::fs::read_to_string(main_worktree_dir.join("foo.txt"))?; - insta::assert_snapshot!(foo, @r" - <<<<<<< ours - qux - ||||||| ancestor - feature-b line 1 - ======= - foo - >>>>>>> theirs - "); + insta::assert_snapshot!(foo, @"qux"); + assert_eq!( + ctx.repo + .get()? + .find_reference(feature_b_name.as_ref())? + .id() + .detach(), + feature_b_tip_before, + "an aborted integration must not move any refs" + ); Ok(()) } @@ -230,14 +236,13 @@ fn causes_workdir_conflicts_simple() -> anyhow::Result<()> { fn causes_workdir_conflicts_complex() -> anyhow::Result<()> { let test_ctx = test_ctx("stacked-branches")?; let mut ctx = test_ctx.ctx; - ctx.settings.feature_flags.cv3 = false; let main_worktree_dir = ctx.workdir()?.expect("non-bare"); let mut guard = ctx.exclusive_worktree_access(); let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; let feature_b_name = gix::refs::FullName::try_from("refs/heads/feature-b")?; - let a = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; + let a = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; std::fs::write(main_worktree_dir.join("foo.txt"), "qux\n")?; invoke_bash_at_dir( @@ -245,9 +250,9 @@ fn causes_workdir_conflicts_complex() -> anyhow::Result<()> { &a.created.path, ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &a.created.id, feature_a_name.as_ref() )?, @@ -259,9 +264,9 @@ fn causes_workdir_conflicts_complex() -> anyhow::Result<()> { "When integrating into feature-a, it should cause the commits above which touch foo.txt and the worktree to conflict" ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &a.created.id, feature_b_name.as_ref() )?, @@ -273,24 +278,47 @@ fn causes_workdir_conflicts_complex() -> anyhow::Result<()> { "When integrating into feature-b, because the thing that commits is the cherry on top of the source, it auto-resolves to what was originally there, resulting in the working_dir not conflicting" ); - worktree_integrate( - &mut ctx, + integrate( + &ctx, guard.write_permission(), &a.created.id, feature_a_name.as_ref(), ) - .expect("it works"); + .expect_err("integration aborts instead of clobbering uncommitted changes"); let foo = std::fs::read_to_string(main_worktree_dir.join("foo.txt"))?; - insta::assert_snapshot!(foo, @r" - <<<<<<< ours - qux - ||||||| ancestor - feature-b line 1 - ======= - foo - >>>>>>> theirs - "); + insta::assert_snapshot!(foo, @"qux"); + + Ok(()) +} + +#[test] +fn fresh_worktree_does_not_obscure_its_branch() -> anyhow::Result<()> { + let test_ctx = test_ctx("stacked-branches")?; + let mut ctx = test_ctx.ctx; + + let mut guard = ctx.exclusive_worktree_access(); + + let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; + let a = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + + // Recompute the workspace like a fresh process would; the linked worktree + // metadata must not obscure the source branch's segment identity. + ctx.invalidate_workspace_cache()?; + + assert_eq!( + integration_status( + &ctx, + guard.write_permission(), + &a.created.id, + feature_a_name.as_ref() + )?, + WorktreeIntegrationStatus::NothingToIntegrate, + "the branch is still resolvable in the workspace, and the untouched worktree has nothing to integrate" + ); + + // Creating another worktree off the same branch also still works. + worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; Ok(()) } @@ -300,12 +328,12 @@ fn causes_workspace_conflict() -> anyhow::Result<()> { let test_ctx = test_ctx("stacked-and-parallel")?; let mut ctx = test_ctx.ctx; - let guard = ctx.exclusive_worktree_access(); + let mut guard = ctx.exclusive_worktree_access(); let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; let feature_b_name = gix::refs::FullName::try_from("refs/heads/feature-b")?; let feature_c_name = gix::refs::FullName::try_from("refs/heads/feature-c")?; - let c = worktree_new(&mut ctx, guard.read_permission(), feature_c_name.as_ref())?; + let c = worktree_new(&ctx, guard.read_permission(), feature_c_name.as_ref())?; invoke_bash_at_dir( r#"echo "foo" >> file.txt && git add . && git commit -am "added conflicts above!""#, @@ -313,9 +341,9 @@ fn causes_workspace_conflict() -> anyhow::Result<()> { ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &c.created.id, feature_c_name.as_ref() )?, @@ -323,9 +351,9 @@ fn causes_workspace_conflict() -> anyhow::Result<()> { "When integrating into feature-c, because we modified a file that sits in feature a & b, it causes the workspace to conflict" ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &c.created.id, feature_b_name.as_ref() )?, @@ -337,9 +365,9 @@ fn causes_workspace_conflict() -> anyhow::Result<()> { "When integrating into feature-b, we can cherry pick, but it will conflict" ); assert_eq!( - worktree_integration_status( - &mut ctx, - guard.read_permission(), + integration_status( + &ctx, + guard.write_permission(), &c.created.id, feature_a_name.as_ref() )?, diff --git a/crates/but-worktrees/tests/worktree/worktree_destroy.rs b/crates/but-worktrees/tests/worktree/worktree_destroy.rs index 1c5a8b8332e..0b1a98c35b8 100644 --- a/crates/but-worktrees/tests/worktree/worktree_destroy.rs +++ b/crates/but-worktrees/tests/worktree/worktree_destroy.rs @@ -1,35 +1,30 @@ -use but_worktrees::{ - destroy::{worktree_destroy_by_id, worktree_destroy_by_reference}, - list::worktree_list, - new::worktree_new, +use crate::util::{ + test_ctx, worktree_destroy_by_id, worktree_destroy_by_reference, worktree_list, worktree_new, }; -use crate::util::test_ctx; - #[test] fn can_destroy_worktree_by_id() -> anyhow::Result<()> { let test_ctx = test_ctx("stacked-and-parallel")?; let mut ctx = test_ctx.ctx; - let mut guard = ctx.exclusive_worktree_access(); + let guard = ctx.exclusive_worktree_access(); let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; - let outcome = worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; + let outcome = worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; // Verify it was created - let list_before = worktree_list(&mut ctx, guard.read_permission())?; + let list_before = worktree_list(&ctx)?; assert_eq!(list_before.entries.len(), 1); assert_eq!(list_before.entries[0].path, outcome.created.path); // Destroy it - let destroy_outcome = - worktree_destroy_by_id(&mut ctx, guard.write_permission(), &outcome.created.id)?; + let destroy_outcome = worktree_destroy_by_id(&ctx, &outcome.created.id)?; assert_eq!(destroy_outcome.destroyed_ids.len(), 1); assert_eq!(destroy_outcome.destroyed_ids[0], outcome.created.id); // Verify it was destroyed - let list_after = worktree_list(&mut ctx, guard.read_permission())?; + let list_after = worktree_list(&ctx)?; assert_eq!(list_after.entries.len(), 0); Ok(()) @@ -40,30 +35,29 @@ fn can_destroy_worktrees_by_reference() -> anyhow::Result<()> { let test_ctx = test_ctx("stacked-and-parallel")?; let mut ctx = test_ctx.ctx; - let mut guard = ctx.exclusive_worktree_access(); + let guard = ctx.exclusive_worktree_access(); let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; let feature_c_name = gix::refs::FullName::try_from("refs/heads/feature-c")?; // Create 3 worktrees from feature-a and 2 from feature-c - worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; - worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; - worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; - worktree_new(&mut ctx, guard.read_permission(), feature_c_name.as_ref())?; - worktree_new(&mut ctx, guard.read_permission(), feature_c_name.as_ref())?; + worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; + worktree_new(&ctx, guard.read_permission(), feature_c_name.as_ref())?; + worktree_new(&ctx, guard.read_permission(), feature_c_name.as_ref())?; // Verify all 5 were created - let list_before = worktree_list(&mut ctx, guard.read_permission())?; + let list_before = worktree_list(&ctx)?; assert_eq!(list_before.entries.len(), 5); // Destroy all feature-a worktrees - let destroy_outcome = - worktree_destroy_by_reference(&mut ctx, guard.write_permission(), feature_a_name.as_ref())?; + let destroy_outcome = worktree_destroy_by_reference(&ctx, feature_a_name.as_ref())?; assert_eq!(destroy_outcome.destroyed_ids.len(), 3); // Verify only feature-c worktrees remain - let list_after = worktree_list(&mut ctx, guard.read_permission())?; + let list_after = worktree_list(&ctx)?; assert_eq!(list_after.entries.len(), 2); assert!( list_after @@ -80,22 +74,21 @@ fn destroy_by_reference_returns_empty_when_no_matches() -> anyhow::Result<()> { let test_ctx = test_ctx("stacked-and-parallel")?; let mut ctx = test_ctx.ctx; - let mut guard = ctx.exclusive_worktree_access(); + let guard = ctx.exclusive_worktree_access(); let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; let feature_b_name = gix::refs::FullName::try_from("refs/heads/feature-b")?; // Create worktrees from feature-a - worktree_new(&mut ctx, guard.read_permission(), feature_a_name.as_ref())?; + worktree_new(&ctx, guard.read_permission(), feature_a_name.as_ref())?; // Try to destroy worktrees from feature-b (which don't exist) - let destroy_outcome = - worktree_destroy_by_reference(&mut ctx, guard.write_permission(), feature_b_name.as_ref())?; + let destroy_outcome = worktree_destroy_by_reference(&ctx, feature_b_name.as_ref())?; assert_eq!(destroy_outcome.destroyed_ids.len(), 0); // Verify feature-a worktree is still there - let list_after = worktree_list(&mut ctx, guard.read_permission())?; + let list_after = worktree_list(&ctx)?; assert_eq!(list_after.entries.len(), 1); Ok(()) diff --git a/crates/but-worktrees/tests/worktree/worktree_new.rs b/crates/but-worktrees/tests/worktree/worktree_new.rs index 8ecee339ede..34e25920674 100644 --- a/crates/but-worktrees/tests/worktree/worktree_new.rs +++ b/crates/but-worktrees/tests/worktree/worktree_new.rs @@ -1,159 +1,39 @@ -#![expect(deprecated, reason = "calls stacks_v3")] - -use anyhow::Context as _; -use but_meta::VirtualBranchesTomlMetadata; -use but_workspace::legacy::{StacksFilter, stacks_v3}; -use but_worktrees::new::worktree_new; - -use crate::util::test_ctx; +use crate::util::{test_ctx, worktree_new}; #[test] fn can_create_worktree_from_feature_a() -> anyhow::Result<()> { - let feature_a_name = gix::refs::FullName::try_from("refs/heads/feature-a")?; - let mut test_ctx = test_ctx("stacked-and-parallel")?; - - let guard = test_ctx.ctx.exclusive_worktree_access(); - let meta = VirtualBranchesTomlMetadata::from_path( - test_ctx - .ctx - .project_data_dir() - .join("virtual_branches.toml"), - )?; - let feature_a = { - let stacks = stacks_v3( - &*test_ctx.ctx.repo.get()?, - &meta, - &test_ctx.ctx.project_meta()?, - StacksFilter::InWorkspace, - None, - )?; - stacks - .into_iter() - .flat_map(|s| s.heads) - .find(|h| h.name == b"feature-a") - .context("Expect to find feature-a")? - }; - - let outcome = worktree_new( - &mut test_ctx.ctx, - guard.read_permission(), - feature_a_name.as_ref(), - )?; - - assert_eq!( - outcome.created.base, - Some(feature_a.tip), - "The base should the the same as the tip of feature-a" - ); - let repo = test_ctx.ctx.repo.get()?; - let worktree = repo.worktrees()?[0].clone(); - let worktree_repo = worktree.clone().into_repo()?; - assert_eq!( - worktree.base()?, - outcome.created.path.canonicalize()?, - "Worktree should be created where we say" - ); - assert_eq!( - Some(worktree_repo.head()?.id().unwrap().detach()), - outcome.created.base, - "Worktree should have base checked out" - ); - - Ok(()) + can_create_worktree_from("refs/heads/feature-a") } #[test] fn can_create_worktree_from_feature_b() -> anyhow::Result<()> { - let feature_b_name = gix::refs::FullName::try_from("refs/heads/feature-b")?; - let mut test_ctx = test_ctx("stacked-and-parallel")?; - - let guard = test_ctx.ctx.exclusive_worktree_access(); - let meta = VirtualBranchesTomlMetadata::from_path( - test_ctx - .ctx - .project_data_dir() - .join("virtual_branches.toml"), - )?; - let feature_b = { - let stacks = stacks_v3( - &*test_ctx.ctx.repo.get()?, - &meta, - &test_ctx.ctx.project_meta()?, - StacksFilter::InWorkspace, - None, - )?; - stacks - .into_iter() - .flat_map(|s| s.heads) - .find(|h| h.name == b"feature-b") - .context("Expect to find feature-b")? - }; - - let outcome = worktree_new( - &mut test_ctx.ctx, - guard.read_permission(), - feature_b_name.as_ref(), - )?; - - assert_eq!( - outcome.created.base, - Some(feature_b.tip), - "The base should the the same as the tip of feature-b" - ); - let repo = test_ctx.ctx.repo.get()?; - let worktree = repo.worktrees()?[0].clone(); - let worktree_repo = worktree.clone().into_repo()?; - assert_eq!( - worktree.base()?, - outcome.created.path.canonicalize()?, - "Worktree should be created where we say" - ); - assert_eq!( - Some(worktree_repo.head()?.id().unwrap().detach()), - outcome.created.base, - "Worktree should have base checked out" - ); - - Ok(()) + can_create_worktree_from("refs/heads/feature-b") } #[test] fn can_create_worktree_from_feature_c() -> anyhow::Result<()> { - let feature_c_name = gix::refs::FullName::try_from("refs/heads/feature-c")?; + can_create_worktree_from("refs/heads/feature-c") +} + +fn can_create_worktree_from(refname: &str) -> anyhow::Result<()> { + let branch_name = gix::refs::FullName::try_from(refname)?; let mut test_ctx = test_ctx("stacked-and-parallel")?; let guard = test_ctx.ctx.exclusive_worktree_access(); - let meta = VirtualBranchesTomlMetadata::from_path( - test_ctx - .ctx - .project_data_dir() - .join("virtual_branches.toml"), - )?; - let feature_c = { - let stacks = stacks_v3( - &*test_ctx.ctx.repo.get()?, - &meta, - &test_ctx.ctx.project_meta()?, - StacksFilter::InWorkspace, - None, - )?; - stacks - .into_iter() - .flat_map(|s| s.heads) - .find(|h| h.name == b"feature-c") - .context("Expect to find feature-c")? - }; + let tip = test_ctx + .ctx + .repo + .get()? + .find_reference(branch_name.as_ref())? + .id() + .detach(); - let outcome = worktree_new( - &mut test_ctx.ctx, - guard.read_permission(), - feature_c_name.as_ref(), - )?; + let outcome = worktree_new(&test_ctx.ctx, guard.read_permission(), branch_name.as_ref())?; assert_eq!( outcome.created.base, - Some(feature_c.tip), - "The base should the the same as the tip of feature-c" + Some(tip), + "The base should be the same as the tip of {refname}" ); let repo = test_ctx.ctx.repo.get()?; let worktree = repo.worktrees()?[0].clone(); diff --git a/crates/but/src/command/legacy/worktree.rs b/crates/but/src/command/legacy/worktree.rs index 3f01956b05f..368bd3be07f 100644 --- a/crates/but/src/command/legacy/worktree.rs +++ b/crates/but/src/command/legacy/worktree.rs @@ -116,6 +116,12 @@ pub fn handle(cmd: Subcommands, ctx: &mut Context, out: &mut OutputChannel) -> R IntegrationStatus::WorktreeIsBare => { writeln!(out, "Status: Cannot integrate - worktree is bare")?; } + IntegrationStatus::NothingToIntegrate => { + writeln!( + out, + "Status: Nothing to integrate - the worktree has no changes" + )?; + } IntegrationStatus::CausesWorkspaceConflicts => { writeln!( out, diff --git a/crates/but/src/lib.rs b/crates/but/src/lib.rs index 96a0dd32939..f110ce99903 100644 --- a/crates/but/src/lib.rs +++ b/crates/but/src/lib.rs @@ -715,7 +715,7 @@ async fn match_subcommand( let mut ctx = setup::init_ctx(&args, InitCtxOptions::default(), out)?; command::legacy::worktree::handle(cmd, &mut ctx, out) .emit_metrics(metrics_ctx) - .show_root_cause_error_then_exit_without_destructors(output) + .map_err(CliError::from) } #[cfg(feature = "legacy")] Subcommands::Status { diff --git a/crates/but/tests/but/command/mod.rs b/crates/but/tests/but/command/mod.rs index 607438a128b..9c49559e6b2 100644 --- a/crates/but/tests/but/command/mod.rs +++ b/crates/but/tests/but/command/mod.rs @@ -49,6 +49,8 @@ mod status; mod teardown; #[cfg(feature = "legacy")] mod undo; +#[cfg(feature = "legacy")] +mod worktree; #[cfg(feature = "legacy")] mod util { diff --git a/crates/but/tests/but/command/worktree.rs b/crates/but/tests/but/command/worktree.rs new file mode 100644 index 00000000000..ad0b83701ed --- /dev/null +++ b/crates/but/tests/but/command/worktree.rs @@ -0,0 +1,191 @@ +use snapbox::str; + +use crate::utils::Sandbox; + +/// The typical journey: create an isolated worktree off a workspace branch, do +/// work there, then squash-integrate the result back into the workspace. +#[test] +fn journey_new_list_integrate() -> anyhow::Result<()> { + let env = Sandbox::init_scenario_with_target_and_default_settings("two-stacks")?; + insta::assert_snapshot!(env.git_log()?, @r" + * c128bce (HEAD -> gitbutler/workspace) GitButler Workspace Commit + |\ + | * 9477ae7 (A) add A + * | d3e2ba3 (B) add B + |/ + * 0dc3733 (origin/main, origin/HEAD, main) add M + "); + env.setup_metadata(&["A", "B"])?; + + env.but("worktree new A") + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +Created worktree at: [..] +Reference: refs/heads/A + +"#]]); + + env.but("worktree list") + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +Path: [..] +Reference: refs/heads/A +Base: [..] + + +"#]]); + + // Do some work in the worktree, like an agent would. + let wt_id = single_worktree_id(&env); + but_testsupport::invoke_bash_at_dir( + r#"echo "from worktree" > wt-file.txt && git add . && git commit -qm "worktree work""#, + &worktrees_dir(&env).join(&wt_id), + ); + + env.but(format!("worktree integrate {wt_id} --dry")) + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +Integration status for worktree: [..] +Target: refs/heads/A +Status: Integratable + No conflicts expected + +"#]]); + + env.but(format!("worktree integrate {wt_id}")) + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +Successfully integrated worktree: [..] +Target: refs/heads/A + +"#]]); + + let log = env.git_log()?; + assert!( + log.contains("(A) Integrated worktree"), + "the worktree work is squashed into a commit on the branch it was created from: {log}" + ); + assert!( + env.projects_root().join("wt-file.txt").exists(), + "the integrated change is checked out in the main worktree" + ); + let remaining_worktrees = + std::fs::read_dir(worktrees_dir(&env))?.collect::, std::io::Error>>()?; + assert_eq!( + remaining_worktrees.len(), + 0, + "the worktree checkout is removed after integration" + ); + assert_eq!( + worktree_private_branches(&env)?, + Vec::::new(), + "worktree creation should not leave hidden branches in the main repository" + ); + + Ok(()) +} + +#[test] +fn destroy_by_name_and_by_reference() -> anyhow::Result<()> { + let env = Sandbox::init_scenario_with_target_and_default_settings("two-stacks")?; + env.setup_metadata(&["A", "B"])?; + + env.but("worktree new A").assert().success(); + let a_id = single_worktree_id(&env); + env.but("worktree new B").assert().success(); + + env.but(format!("worktree destroy {a_id}")) + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +Destroyed worktree: [..] + +"#]]); + + env.but("worktree destroy B --reference") + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +Destroyed 1 worktree(s) for reference: refs/heads/B + - [..] + +"#]]); + + env.but("worktree list") + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +No worktrees found + +"#]]); + assert_eq!( + worktree_private_branches(&env)?, + Vec::::new(), + "destroy should not have hidden branches to clean up" + ); + + Ok(()) +} + +#[test] +fn integrate_dry_run_reports_worktree_without_changes() -> anyhow::Result<()> { + let env = Sandbox::init_scenario_with_target_and_default_settings("two-stacks")?; + env.setup_metadata(&["A", "B"])?; + + env.but("worktree new A").assert().success(); + let wt_id = single_worktree_id(&env); + + env.but(format!("worktree integrate {wt_id} --dry")) + .assert() + .success() + .stderr_eq(str![]) + .stdout_eq(str![[r#" +Integration status for worktree: [..] +Target: refs/heads/A +Status: Nothing to integrate - the worktree has no changes + +"#]]); + + Ok(()) +} + +fn worktrees_dir(env: &Sandbox) -> std::path::PathBuf { + env.projects_root().join(".git/gitbutler/worktrees") +} + +/// The id of the only worktree that currently exists. +fn single_worktree_id(env: &Sandbox) -> String { + let mut entries: Vec<_> = std::fs::read_dir(worktrees_dir(env)) + .expect("worktrees directory exists") + .map(|e| e.expect("readable directory entry")) + .collect(); + assert_eq!(entries.len(), 1, "exactly one worktree is expected"); + entries + .pop() + .expect("one entry") + .file_name() + .to_string_lossy() + .into_owned() +} + +/// All local branches under the private worktree namespace. +fn worktree_private_branches(env: &Sandbox) -> anyhow::Result> { + let repo = env.open_repo()?; + let refs = repo.references()?; + Ok(refs + .prefixed(b"refs/heads/gitbutler/worktree/".as_ref())? + .filter_map(Result::ok) + .map(|r| r.name().as_bstr().to_string()) + .collect()) +}