Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,11 @@ docs/static/.well-known/agent-skills/index.json linguist-generated=true
# Shell templates are embedded into the binary at compile time (askama); a
# CRLF checkout would leak \r into the shell code Windows-built binaries emit.
templates/* text eol=lf
# A fixture's object store and index are git's own binary formats, but git only
# calls a file binary when it finds a NUL in the first 8000 bytes, and a small
# enough zlib object may have none. Those diff as text, so `git diff` writes raw
# deflate into its output and any tool that decodes that output as a string
# fails on it — which is how `cargo affected` came to abort before running a
# test. The rest of `_git/` (HEAD, config, refs) is text worth reading.
tests/fixtures/*/_git/objects/** binary
tests/fixtures/*/_git/index binary
28 changes: 28 additions & 0 deletions src/commands/picker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2930,6 +2930,34 @@ pub mod tests {
);
}

/// A branch-only row whose branch has since acquired a worktree — someone
/// ran `wt switch` elsewhere while the picker sat open. The row's signal
/// still decodes to `Branch`, and removing it would take out a worktree
/// nobody selected, so validation refuses.
#[test]
fn test_prepare_removal_refuses_branch_that_gained_a_worktree() {
let mut test = worktrunk::testing::TestRepo::with_initial_commit();
let worktree_path = test.add_worktree("feature");
let repo = worktrunk::git::Repository::at(test.path()).unwrap();

let remover = test_remover(Arc::new(Mutex::new(Vec::new())), repo);

let target = PickerRemovalTarget::from_signal("feature").unwrap();
let err = remover
.prepare_removal(&target)
.map(|_| ())
.expect_err("a branch with a worktree should not delete as branch-only");
let rendered = err.to_string();
assert!(
rendered.contains("feature") && rendered.contains("wt remove"),
"error should name the branch and the command that removes its worktree: {err:#}"
);
assert!(
worktree_path.exists(),
"the worktree nobody selected should survive"
);
}

/// A selection that names neither a worktree nor a local branch fails the
/// `prepare_worktree_removal` validation, so `prepare_removal` returns the
/// error rather than touching the picker list.
Expand Down
112 changes: 54 additions & 58 deletions src/commands/repository_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,21 @@ use worktrunk::git::{
parse_porcelain_z, parse_untracked_files,
};
use worktrunk::path::format_path_for_display;
use worktrunk::styling::{eprintln, format_with_gutter, progress_message, warning_message};
use worktrunk::styling::{
eprintln, format_with_gutter, progress_message, suggest_command, warning_message,
};

/// Target for worktree removal.
#[derive(Debug)]
pub enum RemoveTarget<'a> {
/// Remove worktree by branch name
/// Delete a branch that has no worktree.
///
/// A branch names a worktree only while it has exactly one: let it name
/// two, which `git worktree add --force` allows, and the lookup silently
/// picks git's first-listed checkout. So callers resolve first and pass
/// [`Path`](Self::Path) for anything that has a worktree, and this variant
/// carries only the branch-only case. One that has since acquired a
/// worktree lost the race and errors rather than removing it unasked.
Branch(&'a str),
/// Remove the current worktree (supports detached HEAD)
Current,
Expand All @@ -29,7 +38,8 @@ pub trait RepositoryCliExt {
/// Warn about untracked files being auto-staged.
fn warn_if_auto_staging_untracked(&self) -> anyhow::Result<()>;

/// Prepare a worktree removal by branch name or current worktree.
/// Prepare the removal of whichever worktree or branch [`RemoveTarget`]
/// names.
///
/// Returns a `RemoveResult` describing what will be removed. The actual
/// removal is performed by the output handler.
Expand Down Expand Up @@ -133,70 +143,55 @@ impl RepositoryCliExt for Repository {
},
BranchOnly {
/// Path of the stale worktree entry this fell back from, when
/// the fallback was a prune. `None` when the branch never had a
/// worktree, which is also the only case with no sibling to
/// check — a branch whose worktree exists resolves to
/// `Worktree` above.
/// the fallback was a prune. `None` when the branch has no
/// worktree entry at all, which is also the only case with no
/// sibling to check — a branch that has one resolves to
/// `Worktree` above, or, named as a `Branch` target, errors.
pruned_from: Option<PathBuf>,
branch: String,
},
}

let resolved = match target {
RemoveTarget::Branch(branch) => {
match worktrees
// The caller established there was no worktree, so one here
// appeared in between. Falling through would remove it — the
// wrong operation, and on a worktree nobody named — so the
// race surfaces instead. `wt remove <path>` is the spelling
// that does mean "remove that worktree".
if let Some(wt) = worktrees
.iter()
.find(|wt| wt.branch.as_deref() == Some(branch))
{
Some(wt) => {
if !wt.path.exists() {
// Directory missing - prune and continue
self.prune_worktrees()?;
Resolved::BranchOnly {
pruned_from: Some(wt.path.clone()),
branch: branch.to_string(),
}
} else if wt.locked.is_some() {
return Err(GitError::WorktreeLocked {
branch: branch.into(),
path: wt.path.clone(),
reason: wt.locked.clone(),
}
.into());
} else {
let is_current = current_path == wt.path;
Resolved::Worktree {
path: wt.path.clone(),
branch: Some(branch.to_string()),
is_current,
}
let path = format_path_for_display(&wt.path);
bail!(cformat!(
"Branch <bold>{branch}</> gained a worktree @ <bold>{path}</> since it was selected; to remove that worktree, run <bold>{}</>",
suggest_command("remove", &[&path], &[])
));
}
// Check the branch exists locally, so a typo or a remote-only
// name reports itself rather than deleting nothing.
let branch_handle = self.branch(branch);
if !branch_handle.exists_locally()? {
let remotes = branch_handle.remotes()?;
if !remotes.is_empty() {
return Err(GitError::RemoteOnlyBranch {
branch: branch.into(),
remote: remotes[0].clone(),
}
.into());
}
None => {
// No worktree found - check if the branch exists locally
let branch_handle = self.branch(branch);
if !branch_handle.exists_locally()? {
let remotes = branch_handle.remotes()?;
if !remotes.is_empty() {
return Err(GitError::RemoteOnlyBranch {
branch: branch.into(),
remote: remotes[0].clone(),
}
.into());
}
return Err(GitError::BranchNotFound {
branch: branch.into(),
show_create_hint: false,
last_fetch_ago: None,
pr_mr_platform: None,
}
.into());
}
Resolved::BranchOnly {
pruned_from: None,
branch: branch.to_string(),
}
return Err(GitError::BranchNotFound {
branch: branch.into(),
show_create_hint: false,
last_fetch_ago: None,
pr_mr_platform: None,
}
.into());
}
Resolved::BranchOnly {
pruned_from: None,
branch: branch.to_string(),
}
}
RemoveTarget::Current | RemoveTarget::Path(_) => {
Expand Down Expand Up @@ -567,10 +562,6 @@ pub(crate) fn compute_integration_reason(
}
}

/// Reject removing the default branch unless force-delete is set.
///
/// The default branch is the integration target — checking it against itself is
/// tautological (same logic as `wt list`'s `is_main` guard in `check_integration_state`).
/// The worktree, other than the one being removed, whose checkout of `branch`
/// deleting the ref would orphan.
///
Expand Down Expand Up @@ -598,6 +589,11 @@ pub(crate) fn live_sibling_checkout<'a>(
})
}

/// Reject removing the default branch unless force-delete is set.
///
/// The default branch is the integration target — checking it against itself is
/// tautological (same logic as `wt list`'s `is_main` guard in
/// `check_integration_state`).
pub(crate) fn check_not_default_branch(
repo: &Repository,
branch: &str,
Expand Down
Loading
Loading