Skip to content

Commit 40035c7

Browse files
author
zhoupengwu
committed
fix(worktree):Before renaming, verify the root directory of the working tree.
Fixed the issue where the validity of the parent repository's working tree directory was not verified during renaming of the working tree. Now, the directory will be checked for existence and accessibility first, and the operation will fail early if it is invalid, to avoid inconsistencies in the state if the working tree fails to move after the branch has been renamed. New test cases have been added to verify this scenario.
1 parent cb4ccf4 commit 40035c7

2 files changed

Lines changed: 149 additions & 25 deletions

File tree

src-tauri/src/shared/workspaces_core/worktree.rs

Lines changed: 58 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -406,8 +406,6 @@ where
406406
return Err("Branch name is unchanged.".to_string());
407407
}
408408

409-
run_git_command(&parent_root, &["branch", "-m", &old_branch, &final_branch]).await?;
410-
411409
// Use the same priority logic as add_worktree_core:
412410
// per-workspace setting > global setting > default
413411
let worktree_root = if let Some(custom_folder) = &parent.settings.worktrees_folder {
@@ -430,44 +428,80 @@ where
430428
let current_path = PathBuf::from(&entry.path);
431429
let next_path = unique_worktree_path_for_rename(&worktree_root, &safe_name, &current_path)?;
432430
let next_path_string = next_path.to_string_lossy().to_string();
433-
if next_path_string != entry.path {
431+
let old_path_string = entry.path.clone();
432+
433+
run_git_command(&parent_root, &["branch", "-m", &old_branch, &final_branch]).await?;
434+
435+
let mut moved_worktree = false;
436+
if next_path_string != old_path_string {
434437
if let Err(error) = run_git_command(
435438
&parent_root,
436-
&["worktree", "move", &entry.path, &next_path_string],
439+
&["worktree", "move", &old_path_string, &next_path_string],
437440
)
438441
.await
439442
{
440443
let _ =
441444
run_git_command(&parent_root, &["branch", "-m", &final_branch, &old_branch]).await;
442445
return Err(error);
443446
}
447+
moved_worktree = true;
444448
}
445449

446-
let (entry_snapshot, list) = {
450+
let update_result: Result<(WorkspaceEntry, WorkspaceEntry, Vec<WorkspaceEntry>), String> = {
447451
let mut workspaces = workspaces.lock().await;
448-
let entry = match workspaces.get_mut(&id) {
449-
Some(entry) => entry,
450-
None => return Err("workspace not found".to_string()),
451-
};
452-
if entry.name.trim() == old_branch {
453-
entry.name = final_branch.clone();
454-
}
455-
entry.path = next_path_string.clone();
456-
match entry.worktree.as_mut() {
457-
Some(worktree) => {
458-
worktree.branch = final_branch.clone();
452+
if let Some(entry) = workspaces.get_mut(&id) {
453+
let old_snapshot = entry.clone();
454+
if entry.name.trim() == old_branch {
455+
entry.name = final_branch.clone();
459456
}
460-
None => {
461-
entry.worktree = Some(WorktreeInfo {
462-
branch: final_branch.clone(),
463-
});
457+
entry.path = next_path_string.clone();
458+
match entry.worktree.as_mut() {
459+
Some(worktree) => {
460+
worktree.branch = final_branch.clone();
461+
}
462+
None => {
463+
entry.worktree = Some(WorktreeInfo {
464+
branch: final_branch.clone(),
465+
});
466+
}
464467
}
468+
let snapshot = entry.clone();
469+
let list: Vec<_> = workspaces.values().cloned().collect();
470+
Ok((old_snapshot, snapshot, list))
471+
} else {
472+
Err("workspace not found".to_string())
465473
}
466-
let snapshot = entry.clone();
467-
let list: Vec<_> = workspaces.values().cloned().collect();
468-
(snapshot, list)
469474
};
470-
write_workspaces(storage_path, &list)?;
475+
let (old_snapshot, entry_snapshot, list) = match update_result {
476+
Ok(value) => value,
477+
Err(error) => {
478+
if moved_worktree {
479+
let _ = run_git_command(
480+
&parent_root,
481+
&["worktree", "move", &next_path_string, &old_path_string],
482+
)
483+
.await;
484+
}
485+
let _ =
486+
run_git_command(&parent_root, &["branch", "-m", &final_branch, &old_branch]).await;
487+
return Err(error);
488+
}
489+
};
490+
if let Err(error) = write_workspaces(storage_path, &list) {
491+
if moved_worktree {
492+
let _ = run_git_command(
493+
&parent_root,
494+
&["worktree", "move", &next_path_string, &old_path_string],
495+
)
496+
.await;
497+
}
498+
let _ = run_git_command(&parent_root, &["branch", "-m", &final_branch, &old_branch]).await;
499+
let mut workspaces = workspaces.lock().await;
500+
if let Some(entry) = workspaces.get_mut(&id) {
501+
*entry = old_snapshot;
502+
}
503+
return Err(error);
504+
}
471505

472506
if let Some(session) = sessions.lock().await.get(&entry_snapshot.id).cloned() {
473507
session

src-tauri/src/workspaces/tests.rs

Lines changed: 91 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use std::collections::HashMap;
22
use std::future::Future;
33
use std::path::PathBuf;
4-
use std::sync::Arc;
4+
use std::sync::{Arc, Mutex as StdMutex};
55

66
use super::settings::{apply_workspace_settings_update, sort_workspaces};
77
use super::worktree::{
@@ -56,6 +56,7 @@ fn workspace_with_id_and_kind(
5656
launch_script: None,
5757
launch_scripts: None,
5858
worktree_setup_script: None,
59+
worktrees_folder: None,
5960
},
6061
}
6162
}
@@ -390,6 +391,95 @@ fn rename_worktree_updates_name_when_unmodified() {
390391
});
391392
}
392393

394+
#[test]
395+
fn rename_worktree_validates_worktree_root_before_branch_rename() {
396+
run_async(async {
397+
let temp_dir = std::env::temp_dir().join(format!("codex-monitor-test-{}", Uuid::new_v4()));
398+
let repo_path = temp_dir.join("repo");
399+
std::fs::create_dir_all(&repo_path).expect("create repo path");
400+
let worktree_path = temp_dir.join("worktrees").join("parent").join("old");
401+
std::fs::create_dir_all(&worktree_path).expect("create worktree path");
402+
403+
let invalid_root = temp_dir.join("not-a-directory");
404+
std::fs::write(&invalid_root, "x").expect("create invalid root file");
405+
406+
let mut parent_settings = WorkspaceSettings::default();
407+
parent_settings.worktrees_folder = Some(invalid_root.to_string_lossy().to_string());
408+
let parent = WorkspaceEntry {
409+
id: "parent".to_string(),
410+
name: "Parent".to_string(),
411+
path: repo_path.to_string_lossy().to_string(),
412+
kind: WorkspaceKind::Main,
413+
parent_id: None,
414+
worktree: None,
415+
settings: parent_settings,
416+
};
417+
let worktree = WorkspaceEntry {
418+
id: "wt-3".to_string(),
419+
name: "feature/old".to_string(),
420+
path: worktree_path.to_string_lossy().to_string(),
421+
kind: WorkspaceKind::Worktree,
422+
parent_id: Some(parent.id.clone()),
423+
worktree: Some(WorktreeInfo {
424+
branch: "feature/old".to_string(),
425+
}),
426+
settings: WorkspaceSettings::default(),
427+
};
428+
let workspaces = Mutex::new(HashMap::from([
429+
(parent.id.clone(), parent.clone()),
430+
(worktree.id.clone(), worktree.clone()),
431+
]));
432+
let sessions: Mutex<HashMap<String, Arc<WorkspaceSession>>> = Mutex::new(HashMap::new());
433+
let app_settings = Mutex::new(AppSettings::default());
434+
let storage_path = temp_dir.join("workspaces.json");
435+
436+
let calls: Arc<StdMutex<Vec<Vec<String>>>> = Arc::new(StdMutex::new(Vec::new()));
437+
let result = rename_worktree_core(
438+
worktree.id.clone(),
439+
"feature/new".to_string(),
440+
&temp_dir,
441+
&workspaces,
442+
&sessions,
443+
&app_settings,
444+
&storage_path,
445+
|_| Ok(repo_path.clone()),
446+
|_root, branch| {
447+
let branch = branch.to_string();
448+
async move { Ok(branch) }
449+
},
450+
|value| sanitize_worktree_name(value),
451+
|_, _, current| Ok(current.to_path_buf()),
452+
|_root, args| {
453+
let calls = calls.clone();
454+
let args: Vec<String> = args.iter().map(|value| value.to_string()).collect();
455+
async move {
456+
calls
457+
.lock()
458+
.expect("lock")
459+
.push(args);
460+
Ok(())
461+
}
462+
},
463+
|_entry, _default_bin, _codex_args, _codex_home| async move {
464+
Err("spawn not expected".to_string())
465+
},
466+
)
467+
.await;
468+
469+
let error = result.expect_err("expected invalid worktree root to fail");
470+
assert!(error.contains("Failed to create worktree directory"));
471+
assert!(calls.lock().expect("lock").is_empty());
472+
473+
let stored = workspaces.lock().await;
474+
let entry = stored.get(&worktree.id).expect("stored entry");
475+
assert_eq!(
476+
entry.worktree.as_ref().map(|worktree| worktree.branch.as_str()),
477+
Some("feature/old")
478+
);
479+
assert_eq!(entry.path, worktree.path);
480+
});
481+
}
482+
393483
#[test]
394484
fn remove_workspace_succeeds_when_parent_repo_folder_is_missing() {
395485
run_async(async {

0 commit comments

Comments
 (0)