agent_ui: Fix worktree naming regression when using existing branches (#53669)
When creating a new git worktree from an existing branch (e.g. `main`), the worktree directory was incorrectly named after the branch instead of getting a random adjective-noun name. Additionally, if a directory with that name already existed, the rollback logic could destructively delete the pre-existing worktree. Changes: - Generate a random adjective-noun name for worktree directories when no explicit name is provided - Collect existing worktree directory names to avoid collisions - Check generated paths against known worktree paths before creating - Reduce random name retry count from 100 to 10 - Add regression test Release Notes: - Fixed a regression where creating a git worktree from an existing branch would name the worktree directory after the branch (instead of generating a random name)
This commit is contained in:
1 parent
e262608b97
commit
ee0c195ed0
2 files changed
+186
-8
No files matched your search
@@ -3102,11 +3102,11 @@ impl AgentPanel {
|
||||
branch_target: &NewWorktreeBranchTarget,
|
||||
existing_branches: &HashSet<String>,
|
||||
occupied_branches: &HashSet<String>,
|
||||
rng: &mut impl rand::Rng,
|
||||
) -> Result<(String, bool, Option<String>)> {
|
||||
let generate_branch_name = || -> Result<String> {
|
||||
let mut generate_branch_name = || {
|
||||
let refs: Vec<&str> = existing_branches.iter().map(|s| s.as_str()).collect();
|
||||
let mut rng = rand::rng();
|
||||
crate::branch_names::generate_branch_name(&refs, &mut rng)
|
||||
crate::branch_names::generate_branch_name(&refs, rng)
|
||||
.ok_or_else(|| anyhow!("Failed to generate a unique branch name"))
|
||||
};
|
||||
|
||||
@@ -3134,10 +3134,13 @@ impl AgentPanel {
|
||||
fn start_worktree_creations(
|
||||
git_repos: &[Entity<project::git_store::Repository>],
|
||||
worktree_name: Option<String>,
|
||||
existing_worktree_names: &[String],
|
||||
existing_worktree_paths: &HashSet<PathBuf>,
|
||||
branch_name: &str,
|
||||
use_existing_branch: bool,
|
||||
start_point: Option<String>,
|
||||
worktree_directory_setting: &str,
|
||||
rng: &mut impl rand::Rng,
|
||||
cx: &mut Context<Self>,
|
||||
) -> Result<(
|
||||
Vec<(
|
||||
@@ -3150,12 +3153,20 @@ impl AgentPanel {
|
||||
let mut creation_infos = Vec::new();
|
||||
let mut path_remapping = Vec::new();
|
||||
|
||||
let worktree_name = worktree_name.unwrap_or_else(|| branch_name.to_string());
|
||||
let worktree_name = worktree_name.unwrap_or_else(|| {
|
||||
let existing_refs: Vec<&str> =
|
||||
existing_worktree_names.iter().map(|s| s.as_str()).collect();
|
||||
crate::branch_names::generate_branch_name(&existing_refs, rng)
|
||||
.unwrap_or_else(|| branch_name.to_string())
|
||||
});
|
||||
|
||||
for repo in git_repos {
|
||||
let (work_dir, new_path, receiver) = repo.update(cx, |repo, _cx| {
|
||||
let new_path =
|
||||
repo.path_for_new_linked_worktree(&worktree_name, worktree_directory_setting)?;
|
||||
if existing_worktree_paths.contains(&new_path) {
|
||||
anyhow::bail!("A worktree already exists at {}", new_path.display());
|
||||
}
|
||||
let target = if use_existing_branch {
|
||||
debug_assert!(
|
||||
git_repos.len() == 1,
|
||||
@@ -3221,7 +3232,8 @@ impl AgentPanel {
|
||||
return Ok(created_paths);
|
||||
};
|
||||
|
||||
// Rollback all attempted worktrees (both successful and failed)
|
||||
// Rollback all attempted worktrees (both successful and failed,
|
||||
// since a failed creation may have left an orphan directory).
|
||||
let mut rollback_futures = Vec::new();
|
||||
for (rollback_repo, rollback_path) in &repos_and_paths {
|
||||
let receiver = cx
|
||||
@@ -3431,6 +3443,8 @@ impl AgentPanel {
|
||||
}
|
||||
|
||||
let mut occupied_branches = HashSet::default();
|
||||
let mut existing_worktree_names = Vec::new();
|
||||
let mut existing_worktree_paths = HashSet::default();
|
||||
for result in futures::future::join_all(worktree_receivers).await {
|
||||
match result {
|
||||
Ok(Ok(worktrees)) => {
|
||||
@@ -3438,6 +3452,15 @@ impl AgentPanel {
|
||||
if let Some(branch_name) = worktree.branch_name() {
|
||||
occupied_branches.insert(branch_name.to_string());
|
||||
}
|
||||
if let Some(name) = worktree
|
||||
.path
|
||||
.parent()
|
||||
.and_then(|p| p.file_name())
|
||||
.and_then(|n| n.to_str())
|
||||
{
|
||||
existing_worktree_names.push(name.to_string());
|
||||
}
|
||||
existing_worktree_paths.insert(worktree.path.clone());
|
||||
}
|
||||
}
|
||||
Ok(Err(err)) => {
|
||||
@@ -3447,11 +3470,14 @@ impl AgentPanel {
|
||||
}
|
||||
}
|
||||
|
||||
let mut rng = rand::rng();
|
||||
|
||||
let (branch_name, use_existing_branch, start_point) =
|
||||
match Self::resolve_worktree_branch_target(
|
||||
&branch_target,
|
||||
&existing_branches,
|
||||
&occupied_branches,
|
||||
&mut rng,
|
||||
) {
|
||||
Ok(target) => target,
|
||||
Err(err) => {
|
||||
@@ -3471,10 +3497,13 @@ impl AgentPanel {
|
||||
Self::start_worktree_creations(
|
||||
&git_repos,
|
||||
worktree_name,
|
||||
&existing_worktree_names,
|
||||
&existing_worktree_paths,
|
||||
&branch_name,
|
||||
use_existing_branch,
|
||||
start_point,
|
||||
&worktree_directory_setting,
|
||||
&mut rng,
|
||||
cx,
|
||||
)
|
||||
}) {
|
||||
@@ -5240,6 +5269,7 @@ mod tests {
|
||||
use fs::FakeFs;
|
||||
use gpui::{TestAppContext, VisualTestContext};
|
||||
use project::Project;
|
||||
use rand::rngs::StdRng;
|
||||
use serde_json::json;
|
||||
use std::path::Path;
|
||||
use std::time::Instant;
|
||||
@@ -6569,8 +6599,8 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_resolve_worktree_branch_target() {
|
||||
#[gpui::test(iterations = 10)]
|
||||
fn test_resolve_worktree_branch_target(mut rng: StdRng) {
|
||||
let existing_branches = HashSet::from_iter([
|
||||
"main".to_string(),
|
||||
"feature".to_string(),
|
||||
@@ -6584,6 +6614,7 @@ mod tests {
|
||||
},
|
||||
&existing_branches,
|
||||
&HashSet::from_iter(["main".to_string()]),
|
||||
&mut rng,
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
@@ -6597,6 +6628,7 @@ mod tests {
|
||||
},
|
||||
&existing_branches,
|
||||
&HashSet::default(),
|
||||
&mut rng,
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(resolved, ("feature".to_string(), true, None));
|
||||
@@ -6607,6 +6639,7 @@ mod tests {
|
||||
},
|
||||
&existing_branches,
|
||||
&HashSet::from_iter(["main".to_string()]),
|
||||
&mut rng,
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(resolved.1, false);
|
||||
@@ -6616,6 +6649,151 @@ mod tests {
|
||||
assert!(!existing_branches.contains(&resolved.0));
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_worktree_dir_name_is_random_when_using_existing_branch(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
let app_state = cx.update(|cx| {
|
||||
agent::ThreadStore::init_global(cx);
|
||||
language_model::LanguageModelRegistry::test(cx);
|
||||
|
||||
let app_state = workspace::AppState::test(cx);
|
||||
workspace::init(app_state.clone(), cx);
|
||||
app_state
|
||||
});
|
||||
|
||||
let fs = app_state.fs.as_fake();
|
||||
fs.insert_tree(
|
||||
"/project",
|
||||
json!({
|
||||
".git": {},
|
||||
"src": {
|
||||
"main.rs": "fn main() {}"
|
||||
}
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
// Put the main worktree on "develop" so that "main" is NOT
|
||||
// occupied by any worktree.
|
||||
fs.set_branch_name(Path::new("/project/.git"), Some("develop"));
|
||||
fs.insert_branches(Path::new("/project/.git"), &["main", "develop"]);
|
||||
|
||||
let project = Project::test(app_state.fs.clone(), [Path::new("/project")], cx).await;
|
||||
|
||||
let multi_workspace =
|
||||
cx.add_window(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
|
||||
multi_workspace
|
||||
.update(cx, |multi_workspace, _, cx| {
|
||||
multi_workspace.open_sidebar(cx);
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
let workspace = multi_workspace
|
||||
.read_with(cx, |multi_workspace, _cx| {
|
||||
multi_workspace.workspace().clone()
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
workspace.update(cx, |workspace, _cx| {
|
||||
workspace.set_random_database_id();
|
||||
});
|
||||
|
||||
cx.update(|cx| {
|
||||
cx.observe_new(
|
||||
|workspace: &mut Workspace,
|
||||
window: Option<&mut Window>,
|
||||
cx: &mut Context<Workspace>| {
|
||||
if let Some(window) = window {
|
||||
let panel = cx.new(|cx| AgentPanel::new(workspace, None, window, cx));
|
||||
workspace.add_panel(panel, window, cx);
|
||||
}
|
||||
},
|
||||
)
|
||||
.detach();
|
||||
});
|
||||
|
||||
let cx = &mut VisualTestContext::from_window(multi_workspace.into(), cx);
|
||||
cx.run_until_parked();
|
||||
|
||||
let panel = workspace.update_in(cx, |workspace, window, cx| {
|
||||
let panel = cx.new(|cx| AgentPanel::new(workspace, None, window, cx));
|
||||
workspace.add_panel(panel.clone(), window, cx);
|
||||
panel
|
||||
});
|
||||
|
||||
cx.run_until_parked();
|
||||
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
panel.open_external_thread_with_server(
|
||||
Rc::new(StubAgentServer::default_response()),
|
||||
window,
|
||||
cx,
|
||||
);
|
||||
});
|
||||
|
||||
cx.run_until_parked();
|
||||
|
||||
// Select "main" as an existing branch — this should NOT make the
|
||||
// worktree directory named "main"; it should get a random name.
|
||||
let content = vec![acp::ContentBlock::Text(acp::TextContent::new(
|
||||
"Hello from test",
|
||||
))];
|
||||
panel.update_in(cx, |panel, window, cx| {
|
||||
panel.handle_worktree_requested(
|
||||
content,
|
||||
WorktreeCreationArgs::New {
|
||||
worktree_name: None,
|
||||
branch_target: NewWorktreeBranchTarget::ExistingBranch {
|
||||
name: "main".to_string(),
|
||||
},
|
||||
},
|
||||
window,
|
||||
cx,
|
||||
);
|
||||
});
|
||||
|
||||
cx.run_until_parked();
|
||||
|
||||
// Find the new workspace and check its worktree path.
|
||||
let new_worktree_path = multi_workspace
|
||||
.read_with(cx, |multi_workspace, cx| {
|
||||
let new_workspace = multi_workspace
|
||||
.workspaces()
|
||||
.find(|ws| ws.entity_id() != workspace.entity_id())
|
||||
.expect("a new workspace should have been created");
|
||||
|
||||
let new_project = new_workspace.read(cx).project().clone();
|
||||
let worktree = new_project
|
||||
.read(cx)
|
||||
.visible_worktrees(cx)
|
||||
.next()
|
||||
.expect("new workspace should have a worktree");
|
||||
worktree.read(cx).abs_path().to_path_buf()
|
||||
})
|
||||
.unwrap();
|
||||
|
||||
// The worktree directory path should contain a random adjective-noun
|
||||
// name, NOT the branch name "main".
|
||||
let path_str = new_worktree_path.to_string_lossy();
|
||||
assert!(
|
||||
!path_str.contains("/main/"),
|
||||
"worktree directory should use a random name, not the branch name. \
|
||||
Got path: {path_str}",
|
||||
);
|
||||
// Verify it looks like an adjective-noun pair (contains a hyphen in
|
||||
// the directory component above the project name).
|
||||
let parent = new_worktree_path
|
||||
.parent()
|
||||
.and_then(|p| p.file_name())
|
||||
.and_then(|n| n.to_str())
|
||||
.expect("should have a parent directory name");
|
||||
assert!(
|
||||
parent.contains('-'),
|
||||
"worktree parent directory should be an adjective-noun pair (e.g. 'swift-falcon'), \
|
||||
got: {parent}",
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_worktree_creation_preserves_selected_agent(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
@@ -61,7 +61,7 @@ const NOUNS: &[&str] = &[
|
||||
pub fn generate_branch_name(existing_branches: &[&str], rng: &mut impl Rng) -> Option<String> {
|
||||
let existing: HashSet<&str> = existing_branches.iter().copied().collect();
|
||||
|
||||
for _ in 0..100 {
|
||||
for _ in 0..10 {
|
||||
let adjective = ADJECTIVES[rng.random_range(0..ADJECTIVES.len())];
|
||||
let noun = NOUNS[rng.random_range(0..NOUNS.len())];
|
||||
let name = format!("{adjective}-{noun}");
|
||||
|
||||
Reference in new issue
Block a user