mirror of
https://github.com/herdrdev/herdr.git
synced 2026-11-06 08:06:30 +00:00
fix: scope worktree leftover cleanup to failed removals
Require a prior partial-removal lease before cleaning a checkout whose .git is gone, and refuse a replacement repository at the path. Check submodule state without spawning git, and wrap the long force warning. refs #5478
This commit is contained in:
@@ -2694,8 +2694,12 @@ mod tests {
|
||||
respond_to,
|
||||
}),
|
||||
result: Err("simulated remove failure".into()),
|
||||
leftover_pending: true,
|
||||
});
|
||||
assert!(pane_updates.is_empty());
|
||||
assert!(app
|
||||
.pending_worktree_remove_leftovers
|
||||
.contains(&crate::worktree::canonical_or_original(&checkout)));
|
||||
|
||||
let response = response_rx
|
||||
.recv_timeout(std::time::Duration::from_secs(2))
|
||||
@@ -2781,6 +2785,7 @@ mod tests {
|
||||
respond_to,
|
||||
}),
|
||||
result: Ok(()),
|
||||
leftover_pending: false,
|
||||
});
|
||||
|
||||
let response = response_rx
|
||||
@@ -2851,6 +2856,7 @@ mod tests {
|
||||
respond_to,
|
||||
}),
|
||||
result: Ok(()),
|
||||
leftover_pending: false,
|
||||
});
|
||||
|
||||
let response = response_rx
|
||||
|
||||
@@ -310,10 +310,7 @@ impl App {
|
||||
);
|
||||
return;
|
||||
}
|
||||
if crate::worktree::checkout_has_initialized_submodules(
|
||||
&space.checkout_path,
|
||||
params.trust_repository,
|
||||
) {
|
||||
if crate::worktree::checkout_has_initialized_submodules(&space.checkout_path) {
|
||||
Self::send_api_response(
|
||||
respond_to,
|
||||
encode_error(
|
||||
@@ -329,6 +326,12 @@ impl App {
|
||||
|
||||
let workspace_internal_id = self.state.workspaces[ws_idx].id.clone();
|
||||
let checkout_key = crate::worktree::canonical_or_original(&space.checkout_path);
|
||||
// Only a path that an earlier forced attempt left behind may be cleaned
|
||||
// without its `.git` still proving ownership.
|
||||
let allow_unregistered_leftover = params.force
|
||||
&& self
|
||||
.pending_worktree_remove_leftovers
|
||||
.contains(&checkout_key);
|
||||
if self
|
||||
.pending_api_worktree_removes
|
||||
.contains_key(&workspace_internal_id)
|
||||
@@ -396,22 +399,22 @@ impl App {
|
||||
forced: params.force,
|
||||
api_request: Some(api_request),
|
||||
result: Ok(()),
|
||||
leftover_pending: false,
|
||||
};
|
||||
let spawned = crate::thread_spawn::spawn_named_with(
|
||||
"herdr-worktree-remove",
|
||||
finished,
|
||||
move |mut finished| {
|
||||
// The workspace membership was verified before spawning this
|
||||
// thread, so a forced request may clean a checkout that Git has
|
||||
// already unregistered but could not delete.
|
||||
finished.result = crate::worktree::run_worktree_remove_command_with_recovery(
|
||||
let outcome = crate::worktree::run_worktree_remove_command_with_recovery(
|
||||
&command,
|
||||
&repo_root,
|
||||
&finished.path,
|
||||
finished.forced,
|
||||
trust_repository,
|
||||
finished.forced,
|
||||
allow_unregistered_leftover,
|
||||
);
|
||||
finished.result = outcome.result;
|
||||
finished.leftover_pending = outcome.leftover_pending;
|
||||
let _ =
|
||||
event_tx.blocking_send(AppEvent::WorktreeRemoveFinished(Box::new(finished)));
|
||||
},
|
||||
@@ -566,6 +569,13 @@ impl App {
|
||||
.remove(&result.workspace_id);
|
||||
self.pending_api_worktree_remove_paths
|
||||
.remove(&api.checkout_key);
|
||||
if result.leftover_pending {
|
||||
self.pending_worktree_remove_leftovers
|
||||
.insert(api.checkout_key.clone());
|
||||
} else if result.result.is_ok() {
|
||||
self.pending_worktree_remove_leftovers
|
||||
.remove(&api.checkout_key);
|
||||
}
|
||||
|
||||
if let Err(message) = result.result {
|
||||
let pane_updates = self.restore_shutdown_worktree_panes(
|
||||
|
||||
+5
-1
@@ -30,7 +30,7 @@ mod theme_sync;
|
||||
mod window_title;
|
||||
mod worktrees;
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::collections::{HashMap, HashSet};
|
||||
#[cfg(unix)]
|
||||
use std::io;
|
||||
use std::sync::Arc;
|
||||
@@ -130,6 +130,9 @@ pub struct App {
|
||||
pub(crate) worktree_read_slots: std::sync::Arc<tokio::sync::Semaphore>,
|
||||
pub(crate) pending_api_worktree_removes: HashMap<String, u64>,
|
||||
pub(crate) pending_api_worktree_remove_paths: HashMap<std::path::PathBuf, u64>,
|
||||
/// Forced removals that left an unregistered checkout behind. A retry may
|
||||
/// finish deleting that specific leftover, but never a replacement repo.
|
||||
pub(crate) pending_worktree_remove_leftovers: HashSet<std::path::PathBuf>,
|
||||
pub(crate) pending_worktree_remove_runtime_exits: HashMap<crate::layout::PaneId, usize>,
|
||||
pub(crate) pending_worktree_remove_runtime_restores: HashMap<crate::layout::PaneId, u64>,
|
||||
pub(crate) next_api_worktree_operation_id: u64,
|
||||
@@ -639,6 +642,7 @@ impl App {
|
||||
worktree_read_slots: std::sync::Arc::new(tokio::sync::Semaphore::new(8)),
|
||||
pending_api_worktree_removes: HashMap::new(),
|
||||
pending_api_worktree_remove_paths: HashMap::new(),
|
||||
pending_worktree_remove_leftovers: HashSet::new(),
|
||||
pending_worktree_remove_runtime_exits: HashMap::new(),
|
||||
pending_worktree_remove_runtime_restores: HashMap::new(),
|
||||
next_api_worktree_operation_id: 1,
|
||||
|
||||
@@ -1487,7 +1487,7 @@ fn worktree_remove_escalates_recoverable_failure_to_force_confirmation() {
|
||||
continue;
|
||||
}
|
||||
assert!(remove.error.is_none());
|
||||
let frame = state.compose(106, 30).expect("force remove modal");
|
||||
let frame = state.compose(60, 20).expect("force remove modal");
|
||||
let text = frame
|
||||
.cells
|
||||
.chunks(frame.width as usize)
|
||||
|
||||
@@ -353,16 +353,25 @@ pub(super) fn render_worktree_remove_overlay(
|
||||
inner.x,
|
||||
inner.y + 4,
|
||||
inner.width,
|
||||
" Dirty files and unpushed submodule work will be permanently deleted.",
|
||||
" Uncommitted or unpushed work in this checkout",
|
||||
Style::default().fg(p.red).bg(p.panel_bg),
|
||||
);
|
||||
}
|
||||
if remove.removing {
|
||||
put_text(
|
||||
b,
|
||||
inner.x,
|
||||
inner.y + 5,
|
||||
inner.width,
|
||||
" or its submodules will be permanently deleted.",
|
||||
Style::default().fg(p.red).bg(p.panel_bg),
|
||||
);
|
||||
}
|
||||
let status_y = inner.y + if remove.force_confirmation { 6 } else { 5 };
|
||||
if remove.removing {
|
||||
put_text(
|
||||
b,
|
||||
inner.x,
|
||||
status_y,
|
||||
inner.width,
|
||||
" removing…",
|
||||
Style::default().fg(p.accent).bg(p.panel_bg),
|
||||
);
|
||||
@@ -370,7 +379,7 @@ pub(super) fn render_worktree_remove_overlay(
|
||||
put_text(
|
||||
b,
|
||||
inner.x,
|
||||
inner.y + 5,
|
||||
status_y,
|
||||
inner.width,
|
||||
&format!(" {error}"),
|
||||
Style::default().fg(p.red).bg(p.panel_bg),
|
||||
|
||||
@@ -50,6 +50,9 @@ pub struct WorktreeRemoveResult {
|
||||
pub forced: bool,
|
||||
pub api_request: Option<ApiWorktreeRemoveRequest>,
|
||||
pub result: Result<(), String>,
|
||||
/// Git unregistered the checkout but left files behind that could not be
|
||||
/// removed yet. A later forced retry may finish cleaning this path.
|
||||
pub leftover_pending: bool,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
|
||||
@@ -5357,6 +5357,7 @@ fn terminal_attach_client_exits_when_worktree_remove_succeeds() {
|
||||
respond_to,
|
||||
}),
|
||||
result: Ok(()),
|
||||
leftover_pending: false,
|
||||
}
|
||||
)))
|
||||
);
|
||||
|
||||
+97
-52
@@ -246,23 +246,13 @@ pub(crate) fn checkout_has_dirty_files(
|
||||
}
|
||||
|
||||
#[cfg(any(windows, test))]
|
||||
pub(crate) fn checkout_has_initialized_submodules(path: &Path, trust_repository: bool) -> bool {
|
||||
if !path.join(".gitmodules").is_file() {
|
||||
return false;
|
||||
}
|
||||
let Ok(output) = repository_git_command(path, trust_repository)
|
||||
.args(["submodule", "status", "--recursive"])
|
||||
.output()
|
||||
else {
|
||||
pub(crate) fn checkout_has_initialized_submodules(path: &Path) -> bool {
|
||||
let Some(gitdir) = checkout_gitdir(path) else {
|
||||
return false;
|
||||
};
|
||||
if !output.status.success() {
|
||||
return false;
|
||||
}
|
||||
// `git submodule status` prefixes uninitialized entries with `-`.
|
||||
String::from_utf8_lossy(&output.stdout)
|
||||
.lines()
|
||||
.any(|line| matches!(line.as_bytes().first(), Some(b' ' | b'+' | b'U')))
|
||||
std::fs::read_dir(gitdir.join("modules"))
|
||||
.map(|mut entries| entries.next().is_some())
|
||||
.unwrap_or(false)
|
||||
}
|
||||
|
||||
pub(crate) fn build_worktree_add_new_branch_command(
|
||||
@@ -372,6 +362,11 @@ pub(crate) fn run_worktree_command(command: &WorktreeCommand) -> Result<(), Stri
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) struct WorktreeRemoveOutcome {
|
||||
pub result: Result<(), String>,
|
||||
pub leftover_pending: bool,
|
||||
}
|
||||
|
||||
pub(crate) fn run_worktree_remove_command_with_recovery(
|
||||
command: &WorktreeCommand,
|
||||
repo_root: &Path,
|
||||
@@ -379,37 +374,74 @@ pub(crate) fn run_worktree_remove_command_with_recovery(
|
||||
force: bool,
|
||||
trust_repository: bool,
|
||||
allow_unregistered_leftover: bool,
|
||||
) -> Result<(), String> {
|
||||
) -> WorktreeRemoveOutcome {
|
||||
// Capture the checkout identity before Git removes `.git`. A forced removal
|
||||
// can unregister the worktree and then fail to delete the directory, at
|
||||
// which point the checkout no longer proves it belongs to this repository.
|
||||
let checkout_gitdir = force.then(|| checkout_gitdir(path)).flatten();
|
||||
match run_worktree_command(command) {
|
||||
Ok(()) => Ok(()),
|
||||
Err(err) if force => {
|
||||
if worktree_list_contains_path(repo_root, path, trust_repository)? {
|
||||
return Err(err);
|
||||
}
|
||||
if !path.exists() {
|
||||
return Ok(());
|
||||
}
|
||||
let known_checkout = checkout_gitdir
|
||||
let checkout_identity = force.then(|| checkout_gitdir(path)).flatten();
|
||||
let Err(err) = run_worktree_command(command) else {
|
||||
return WorktreeRemoveOutcome {
|
||||
result: Ok(()),
|
||||
leftover_pending: false,
|
||||
};
|
||||
};
|
||||
if !force {
|
||||
return WorktreeRemoveOutcome {
|
||||
result: Err(err),
|
||||
leftover_pending: false,
|
||||
};
|
||||
}
|
||||
match worktree_list_contains_path(repo_root, path, trust_repository) {
|
||||
Ok(true) => {
|
||||
return WorktreeRemoveOutcome {
|
||||
result: Err(err),
|
||||
leftover_pending: false,
|
||||
};
|
||||
}
|
||||
Ok(false) => {}
|
||||
Err(list_err) => {
|
||||
return WorktreeRemoveOutcome {
|
||||
result: Err(list_err),
|
||||
leftover_pending: false,
|
||||
};
|
||||
}
|
||||
}
|
||||
if !path.exists() {
|
||||
return WorktreeRemoveOutcome {
|
||||
result: Ok(()),
|
||||
leftover_pending: false,
|
||||
};
|
||||
}
|
||||
let current_identity = checkout_gitdir(path)
|
||||
.as_deref()
|
||||
.is_some_and(|gitdir| gitdir_belongs_to_repo(gitdir, repo_root, trust_repository));
|
||||
// A replacement repository at the checkout path has its own `.git`, which
|
||||
// does not identify this repository's linked worktree. Never delete it.
|
||||
let replacement_checkout = path.join(".git").exists() && !current_identity;
|
||||
let known_checkout = !replacement_checkout
|
||||
&& (current_identity
|
||||
|| checkout_identity
|
||||
.as_deref()
|
||||
.is_some_and(|gitdir| gitdir_belongs_to_repo(gitdir, repo_root, trust_repository))
|
||||
|| leftover_worktree_checkout_matches_repo(repo_root, path, trust_repository)
|
||||
|| allow_unregistered_leftover;
|
||||
if !known_checkout {
|
||||
return Err(err);
|
||||
}
|
||||
std::fs::remove_dir_all(path).map_err(|remove_err| {
|
||||
format!(
|
||||
"{err}; failed to remove leftover checkout {}: {remove_err}",
|
||||
path.display()
|
||||
)
|
||||
})?;
|
||||
Ok(())
|
||||
}
|
||||
Err(err) => Err(err),
|
||||
|| allow_unregistered_leftover);
|
||||
if !known_checkout {
|
||||
return WorktreeRemoveOutcome {
|
||||
result: Err(err),
|
||||
leftover_pending: false,
|
||||
};
|
||||
}
|
||||
match std::fs::remove_dir_all(path) {
|
||||
Ok(()) => WorktreeRemoveOutcome {
|
||||
result: Ok(()),
|
||||
leftover_pending: false,
|
||||
},
|
||||
Err(remove_err) => WorktreeRemoveOutcome {
|
||||
result: Err(format!(
|
||||
"{err}; failed to remove leftover checkout {}: {remove_err}",
|
||||
path.display()
|
||||
)),
|
||||
leftover_pending: true,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -434,15 +466,6 @@ fn gitdir_belongs_to_repo(gitdir: &Path, repo_root: &Path, trust_repository: boo
|
||||
canonical_or_original(gitdir).starts_with(canonical_or_original(&worktrees_dir))
|
||||
}
|
||||
|
||||
fn leftover_worktree_checkout_matches_repo(
|
||||
repo_root: &Path,
|
||||
path: &Path,
|
||||
trust_repository: bool,
|
||||
) -> bool {
|
||||
checkout_gitdir(path)
|
||||
.is_some_and(|gitdir| gitdir_belongs_to_repo(&gitdir, repo_root, trust_repository))
|
||||
}
|
||||
|
||||
fn git_common_worktrees_dir(repo_root: &Path, trust_repository: bool) -> Option<PathBuf> {
|
||||
let output = repository_git_command(repo_root, trust_repository)
|
||||
.args(["rev-parse", "--git-common-dir"])
|
||||
@@ -958,7 +981,7 @@ prunable stale
|
||||
false,
|
||||
);
|
||||
run_worktree_command(&add).unwrap();
|
||||
assert!(!checkout_has_initialized_submodules(&checkout, false));
|
||||
assert!(!checkout_has_initialized_submodules(&checkout));
|
||||
run_git(
|
||||
&checkout,
|
||||
&[
|
||||
@@ -970,7 +993,7 @@ prunable stale
|
||||
"--quiet",
|
||||
],
|
||||
);
|
||||
assert!(checkout_has_initialized_submodules(&checkout, false));
|
||||
assert!(checkout_has_initialized_submodules(&checkout));
|
||||
assert!(!checkout_has_dirty_files(&checkout, false).unwrap());
|
||||
|
||||
let remove = build_worktree_remove_command(&repo, &checkout, false, false);
|
||||
@@ -985,6 +1008,7 @@ prunable stale
|
||||
|
||||
let forced = build_worktree_remove_command(&repo, &checkout, true, false);
|
||||
run_worktree_remove_command_with_recovery(&forced, &repo, &checkout, true, false, false)
|
||||
.result
|
||||
.unwrap();
|
||||
assert!(!checkout.exists());
|
||||
assert!(!worktree_list_contains_path(&repo, &checkout, false).unwrap());
|
||||
@@ -1092,6 +1116,7 @@ prunable stale
|
||||
std::fs::write(checkout.join("leftover"), "leftover\n").unwrap();
|
||||
|
||||
run_worktree_remove_command_with_recovery(&remove, &repo, &checkout, true, false, false)
|
||||
.result
|
||||
.unwrap();
|
||||
|
||||
assert!(!checkout.exists());
|
||||
@@ -1115,12 +1140,31 @@ prunable stale
|
||||
// remove the directory. A confirmed forced removal of the workspace's
|
||||
// checkout may still clean up the leftover.
|
||||
run_worktree_remove_command_with_recovery(&remove, &repo, &checkout, true, false, true)
|
||||
.result
|
||||
.unwrap();
|
||||
|
||||
assert!(!checkout.exists());
|
||||
let _ = std::fs::remove_dir_all(repo);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn forced_worktree_remove_recovery_keeps_replacement_repository_with_leftover_lease() {
|
||||
let repo = create_committed_repo("worktree-recovery-replacement-repo");
|
||||
let checkout = unique_temp_path("worktree-recovery-replacement-checkout");
|
||||
std::fs::create_dir_all(&checkout).unwrap();
|
||||
run_git(&checkout, &["init", "--quiet"]);
|
||||
|
||||
let remove = build_worktree_remove_command(&repo, &checkout, true, false);
|
||||
let outcome =
|
||||
run_worktree_remove_command_with_recovery(&remove, &repo, &checkout, true, false, true);
|
||||
|
||||
assert!(outcome.result.is_err());
|
||||
assert!(!outcome.leftover_pending);
|
||||
assert!(checkout.join(".git").exists());
|
||||
let _ = std::fs::remove_dir_all(checkout);
|
||||
let _ = std::fs::remove_dir_all(repo);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn forced_worktree_remove_recovery_keeps_unrelated_replacement_directory() {
|
||||
let repo = create_committed_repo("worktree-recovery-unrelated-repo");
|
||||
@@ -1137,6 +1181,7 @@ prunable stale
|
||||
let err = run_worktree_remove_command_with_recovery(
|
||||
&remove, &repo, &checkout, true, false, false,
|
||||
)
|
||||
.result
|
||||
.expect_err("unrelated replacement directory should not be removed");
|
||||
|
||||
assert!(is_not_working_tree_remove_error(&err));
|
||||
|
||||
Reference in New Issue
Block a user