git_ui: Fix branch picker deleting remote instead of remote branch (#48338)

ᴀᴍᴛᴏᴀᴇʀ created

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

Change summary

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(-)

Detailed changes

crates/fs/src/fake_git_repo.rs 🔗

@@ -569,6 +569,11 @@ impl GitRepository for FakeGitRepository {
         _base_branch: Option<String>,
     ) -> 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(())
         })

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<Vec<Worktree>>>;
 
@@ -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()

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::<Vec<String>>();
         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;

crates/project/src/git_store.rs 🔗

@@ -2543,11 +2543,12 @@ impl GitStore {
     ) -> Result<proto::Ack> {
         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<Result<()>> {
+    pub fn delete_branch(
+        &mut self,
+        is_remote: bool,
+        branch_name: String,
+    ) -> oneshot::Receiver<Result<()>> {
         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?;

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 {