From ed2c08fc4ea972f962f0dbc36970b738561e81f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E1=B4=80=E1=B4=8D=E1=B4=9B=E1=B4=8F=E1=B4=80=E1=B4=87?= =?UTF-8?q?=CA=80?= Date: Wed, 18 Mar 2026 16:39:56 +0800 Subject: [PATCH] git_ui: Fix branch picker deleting remote instead of remote branch (#48338) Closes #48256 It appears that the current git remotes support was implemented in #42819, following the design described in https://github.com/zed-industries/zed/pull/42486#issuecomment-3524092306. That design does not include an interaction for deleting remotes, but the delete remote branch action was mistakenly implemented as deleting the remote itself. After this PR, there should be no code paths that use `remove_remote` anymore. I've kept it for now though, as it may be useful when we introduce the corresponding interaction in the future. Release Notes: - Fixed a bug where deleting a remote branch from the branch picker would incorrectly remove the entire remote configuration --- crates/fs/src/fake_git_repo.rs | 12 +++++++- crates/git/src/repository.rs | 8 ++++-- crates/git_ui/src/branch_picker.rs | 44 +++++++++++++----------------- crates/project/src/git_store.rs | 23 +++++++++++++--- crates/proto/proto/git.proto | 1 + 5 files changed, 55 insertions(+), 33 deletions(-) diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 0cb610f7dd2d4ccf809d907347bf3b3be2c82444..dd8442f7c01f0535354edc83106e0a97ed8949a2 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -569,6 +569,11 @@ impl GitRepository for FakeGitRepository { _base_branch: Option, ) -> BoxFuture<'_, Result<()>> { self.with_state_async(true, move |state| { + if let Some((remote, _)) = name.split_once('/') + && !state.remotes.contains_key(remote) + { + state.remotes.insert(remote.to_owned(), "".to_owned()); + } state.branches.insert(name); Ok(()) }) @@ -587,7 +592,7 @@ impl GitRepository for FakeGitRepository { }) } - fn delete_branch(&self, name: String) -> BoxFuture<'_, Result<()>> { + fn delete_branch(&self, _is_remote: bool, name: String) -> BoxFuture<'_, Result<()>> { self.with_state_async(true, move |state| { if !state.branches.remove(&name) { bail!("no such branch: {name}"); @@ -981,6 +986,11 @@ impl GitRepository for FakeGitRepository { fn remove_remote(&self, name: String) -> BoxFuture<'_, Result<()>> { self.with_state_async(true, move |state| { + state.branches.retain(|branch| { + branch + .split_once('/') + .is_none_or(|(remote, _)| remote != name) + }); state.remotes.remove(&name); Ok(()) }) diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 094e634c7ff9265ef60ad0a3b892ef1eebdbad4e..074c397cec90858601eec0d5ac8adc3bc16c345c 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -763,7 +763,7 @@ pub trait GitRepository: Send + Sync { -> BoxFuture<'_, Result<()>>; fn rename_branch(&self, branch: String, new_name: String) -> BoxFuture<'_, Result<()>>; - fn delete_branch(&self, name: String) -> BoxFuture<'_, Result<()>>; + fn delete_branch(&self, is_remote: bool, name: String) -> BoxFuture<'_, Result<()>>; fn worktrees(&self) -> BoxFuture<'_, Result>>; @@ -1866,12 +1866,14 @@ impl GitRepository for RealGitRepository { .boxed() } - fn delete_branch(&self, name: String) -> BoxFuture<'_, Result<()>> { + fn delete_branch(&self, is_remote: bool, name: String) -> BoxFuture<'_, Result<()>> { let git_binary = self.git_binary(); self.executor .spawn(async move { - git_binary?.run(&["branch", "-d", &name]).await?; + git_binary? + .run(&["branch", if is_remote { "-dr" } else { "-d" }, &name]) + .await?; anyhow::Ok(()) }) .boxed() diff --git a/crates/git_ui/src/branch_picker.rs b/crates/git_ui/src/branch_picker.rs index d1ab60b9042fb06a3f049625f7c0a809957a1543..cc5f70e2d0f355e94f402f74c0ceaa6121c448d7 100644 --- a/crates/git_ui/src/branch_picker.rs +++ b/crates/git_ui/src/branch_picker.rs @@ -486,28 +486,24 @@ impl BranchListDelegate { let workspace = self.workspace.clone(); cx.spawn_in(window, async move |picker, cx| { - let mut is_remote = false; + let is_remote; let result = match &entry { - Entry::Branch { branch, .. } => match branch.remote_name() { - Some(remote_name) => { - is_remote = true; - repo.update(cx, |repo, _| repo.remove_remote(remote_name.to_string())) - .await? - } - None => { - repo.update(cx, |repo, _| repo.delete_branch(branch.name().to_string())) - .await? - } - }, + Entry::Branch { branch, .. } => { + is_remote = branch.is_remote(); + repo.update(cx, |repo, _| { + repo.delete_branch(is_remote, branch.name().to_string()) + }) + .await? + } _ => { - log::error!("Failed to delete remote: wrong entry to delete"); + log::error!("Failed to delete entry: wrong entry to delete"); return Ok(()); } }; if let Err(e) = result { if is_remote { - log::error!("Failed to delete remote: {}", e); + log::error!("Failed to delete remote branch: {}", e); } else { log::error!("Failed to delete branch: {}", e); } @@ -517,7 +513,7 @@ impl BranchListDelegate { if is_remote { show_error_toast( workspace, - format!("remote remove {}", entry.name()), + format!("branch -dr {}", entry.name()), e, cx, ) @@ -1534,7 +1530,7 @@ mod tests { } #[gpui::test] - async fn test_delete_remote(cx: &mut TestAppContext) { + async fn test_delete_remote_branch(cx: &mut TestAppContext) { init_test(cx); let (_project, repository) = init_fake_repository(cx).await; let branches = vec![ @@ -1544,19 +1540,17 @@ mod tests { create_test_branch("develop", false, Some("private"), Some(700)), ]; - let remote_names = branches + let branch_names = branches .iter() - .filter_map(|branch| branch.remote_name().map(|r| r.to_string())) + .map(|branch| branch.name().to_string()) .collect::>(); let repo = repository.clone(); cx.spawn(async move |mut cx| { - for branch in remote_names { - repo.update(&mut cx, |repo, _| { - repo.create_remote(branch, String::from("test")) - }) - .await - .unwrap() - .unwrap(); + for branch in branch_names { + repo.update(&mut cx, |repo, _| repo.create_branch(branch, None)) + .await + .unwrap() + .unwrap(); } }) .await; diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index f1a777711d5300f3aae479ce8bf30b6c64425532..75bd6cb59ff51499facb1d8fb7328881298c19c1 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -2543,11 +2543,12 @@ impl GitStore { ) -> Result { let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; + let is_remote = envelope.payload.is_remote; let branch_name = envelope.payload.branch_name; repository_handle .update(&mut cx, |repository_handle, _| { - repository_handle.delete_branch(branch_name) + repository_handle.delete_branch(is_remote, branch_name) }) .await??; @@ -6044,18 +6045,32 @@ impl Repository { ) } - pub fn delete_branch(&mut self, branch_name: String) -> oneshot::Receiver> { + pub fn delete_branch( + &mut self, + is_remote: bool, + branch_name: String, + ) -> oneshot::Receiver> { let id = self.id; self.send_job( - Some(format!("git branch -d {branch_name}").into()), + Some( + format!( + "git branch {} {}", + if is_remote { "-dr" } else { "-d" }, + branch_name + ) + .into(), + ), move |repo, _cx| async move { match repo { - RepositoryState::Local(state) => state.backend.delete_branch(branch_name).await, + RepositoryState::Local(state) => { + state.backend.delete_branch(is_remote, branch_name).await + } RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { client .request(proto::GitDeleteBranch { project_id: project_id.0, repository_id: id.to_proto(), + is_remote, branch_name, }) .await?; diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index bb6b73ce3b89d51e9bf594c9e01254f5f0d579a4..4f6b74e9537ac4570c72c3c5319b9819f1e52d0c 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -210,6 +210,7 @@ message GitDeleteBranch { uint64 project_id = 1; uint64 repository_id = 2; string branch_name = 3; + bool is_remote = 4; } message GitDiff {