git: Fix a bug where BranchDiff shows incorrect diffs (#46433)

Anthony Eid created 2 days ago

This happens when a user's default branch has different remote and local
versions on their system. Because we would get a diff using just a
branch name without including it's remote name as well

This is a better default because it's far less confusing from a user's
perspective to debug what's going on.

Release Notes:

- git: Fix BranchDiff showing incorrect diffs when default local branch
is out of sync with the remote default branch

Change summary

crates/fs/src/fake_git_repo.rs       | 14 ++++++++++++--
crates/git/src/repository.rs         | 26 ++++++++++++++++++++++----
crates/git_ui/src/branch_picker.rs   |  6 +++---
crates/git_ui/src/project_diff.rs    |  2 +-
crates/git_ui/src/worktree_picker.rs |  6 +++---
crates/project/src/git_store.rs      |  9 ++++++---
6 files changed, 47 insertions(+), 16 deletions(-)

Detailed changes

crates/fs/src/fake_git_repo.rs

@@ -720,8 +720,18 @@ impl GitRepository for FakeGitRepository {
         unimplemented!()
     }
 
-    fn default_branch(&self) -> BoxFuture<'_, Result<Option<SharedString>>> {
-        async { Ok(Some("main".into())) }.boxed()
+    fn default_branch(
+        &self,
+        include_remote_name: bool,
+    ) -> BoxFuture<'_, Result<Option<SharedString>>> {
+        async move {
+            Ok(Some(if include_remote_name {
+                "origin/main".into()
+            } else {
+                "main".into()
+            }))
+        }
+        .boxed()
     }
 
     fn create_remote(&self, name: String, url: String) -> BoxFuture<'_, Result<()>> {

crates/git/src/repository.rs

@@ -649,7 +649,10 @@ pub trait GitRepository: Send + Sync {
         target_checkpoint: GitRepositoryCheckpoint,
     ) -> BoxFuture<'_, Result<String>>;
 
-    fn default_branch(&self) -> BoxFuture<'_, Result<Option<SharedString>>>;
+    fn default_branch(
+        &self,
+        include_remote_name: bool,
+    ) -> BoxFuture<'_, Result<Option<SharedString>>>;
 }
 
 pub enum DiffType {
@@ -2305,7 +2308,10 @@ impl GitRepository for RealGitRepository {
             .boxed()
     }
 
-    fn default_branch(&self) -> BoxFuture<'_, Result<Option<SharedString>>> {
+    fn default_branch(
+        &self,
+        include_remote_name: bool,
+    ) -> BoxFuture<'_, Result<Option<SharedString>>> {
         let working_directory = self.working_directory();
         let git_binary_path = self.any_git_binary_path.clone();
 
@@ -2315,19 +2321,31 @@ impl GitRepository for RealGitRepository {
                 let working_directory = working_directory?;
                 let git = GitBinary::new(git_binary_path, working_directory, executor);
 
+                let strip_prefix = if include_remote_name {
+                    "refs/remotes/"
+                } else {
+                    "refs/remotes/upstream/"
+                };
+
                 if let Ok(output) = git
                     .run(&["symbolic-ref", "refs/remotes/upstream/HEAD"])
                     .await
                 {
                     let output = output
-                        .strip_prefix("refs/remotes/upstream/")
+                        .strip_prefix(strip_prefix)
                         .map(|s| SharedString::from(s.to_owned()));
                     return Ok(output);
                 }
 
+                let strip_prefix = if include_remote_name {
+                    "refs/remotes/"
+                } else {
+                    "refs/remotes/origin/"
+                };
+
                 if let Ok(output) = git.run(&["symbolic-ref", "refs/remotes/origin/HEAD"]).await {
                     return Ok(output
-                        .strip_prefix("refs/remotes/origin/")
+                        .strip_prefix(strip_prefix)
                         .map(|s| SharedString::from(s.to_owned())));
                 }
 

crates/git_ui/src/branch_picker.rs

@@ -128,9 +128,9 @@ impl BranchList {
         let all_branches_request = repository
             .clone()
             .map(|repository| repository.update(cx, |repository, _| repository.branches()));
-        let default_branch_request = repository
-            .clone()
-            .map(|repository| repository.update(cx, |repository, _| repository.default_branch()));
+        let default_branch_request = repository.clone().map(|repository| {
+            repository.update(cx, |repository, _| repository.default_branch(false))
+        });
 
         cx.spawn_in(window, async move |this, cx| {
             let mut all_branches = all_branches_request

crates/git_ui/src/project_diff.rs

@@ -199,7 +199,7 @@ impl ProjectDiff {
         let Some(repo) = project.read(cx).git_store().read(cx).active_repository() else {
             return Task::ready(Err(anyhow!("No active repository")));
         };
-        let main_branch = repo.update(cx, |repo, _| repo.default_branch());
+        let main_branch = repo.update(cx, |repo, _| repo.default_branch(true));
         window.spawn(cx, async move |cx| {
             let main_branch = main_branch
                 .await??

crates/git_ui/src/worktree_picker.rs

@@ -60,9 +60,9 @@ impl WorktreeList {
             .clone()
             .map(|repository| repository.update(cx, |repository, _| repository.worktrees()));
 
-        let default_branch_request = repository
-            .clone()
-            .map(|repository| repository.update(cx, |repository, _| repository.default_branch()));
+        let default_branch_request = repository.clone().map(|repository| {
+            repository.update(cx, |repository, _| repository.default_branch(false))
+        });
 
         cx.spawn_in(window, async move |this, cx| {
             let all_worktrees = all_worktrees_request

crates/project/src/git_store.rs

@@ -2229,7 +2229,7 @@ impl GitStore {
 
         let branch = repository_handle
             .update(&mut cx, |repository_handle, _| {
-                repository_handle.default_branch()
+                repository_handle.default_branch(false)
             })
             .await??
             .map(Into::into);
@@ -5189,12 +5189,15 @@ impl Repository {
         )
     }
 
-    pub fn default_branch(&mut self) -> oneshot::Receiver<Result<Option<SharedString>>> {
+    pub fn default_branch(
+        &mut self,
+        include_remote_name: bool,
+    ) -> oneshot::Receiver<Result<Option<SharedString>>> {
         let id = self.id;
         self.send_job(None, move |repo, _| async move {
             match repo {
                 RepositoryState::Local(LocalRepositoryState { backend, .. }) => {
-                    backend.default_branch().await
+                    backend.default_branch(include_remote_name).await
                 }
                 RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => {
                     let response = client