From e1e3f2e423b2f527ea60be4696be2f9ed99073ac Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Thu, 1 May 2025 21:24:26 -0400 Subject: [PATCH] Improve handling of remote-tracking branches in the picker (#29744) Release Notes: - Changed the git branch picker to make remote-tracking branches less prominent --------- Co-authored-by: Anthony Eid --- crates/agent/src/thread.rs | 2 +- crates/collab/src/tests/integration_tests.rs | 8 +- .../remote_editing_collaboration_tests.rs | 6 +- crates/fs/src/fake_git_repo.rs | 2 +- crates/git/src/repository.rs | 141 ++++++++++-------- crates/git_ui/src/branch_picker.rs | 50 +++++-- crates/git_ui/src/commit_modal.rs | 4 +- crates/git_ui/src/git_panel.rs | 33 ++-- crates/git_ui/src/project_diff.rs | 6 +- crates/project/src/git_store.rs | 12 +- crates/proto/proto/git.proto | 2 +- .../remote_server/src/remote_editing_tests.rs | 6 +- crates/title_bar/src/title_bar.rs | 2 +- 13 files changed, 150 insertions(+), 124 deletions(-) diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index abe0fefe3c..61cae185b0 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -2110,7 +2110,7 @@ impl Thread { .map(|repo| { repo.update(cx, |repo, _| { let current_branch = - repo.branch.as_ref().map(|branch| branch.name.to_string()); + repo.branch.as_ref().map(|branch| branch.name().to_owned()); repo.send_job(None, |state, _| async move { let RepositoryState::Local { backend, .. } = state else { return GitState { diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index 9655fcbcb8..9eb9eb5a3c 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/crates/collab/src/tests/integration_tests.rs @@ -2902,7 +2902,7 @@ async fn test_git_branch_name( .read(cx) .branch .as_ref() - .map(|branch| branch.name.to_string()), + .map(|branch| branch.name().to_owned()), branch_name ) } @@ -6864,7 +6864,7 @@ async fn test_remote_git_branches( let branches_b = branches_b .into_iter() - .map(|branch| branch.name.to_string()) + .map(|branch| branch.name().to_string()) .collect::>(); assert_eq!(branches_b, branches_set); @@ -6895,7 +6895,7 @@ async fn test_remote_git_branches( }) }); - assert_eq!(host_branch.name, branches[2]); + assert_eq!(host_branch.name(), branches[2]); // Also try creating a new branch cx_b.update(|cx| { @@ -6933,5 +6933,5 @@ async fn test_remote_git_branches( }) }); - assert_eq!(host_branch.name, "totally-new-branch"); + assert_eq!(host_branch.name(), "totally-new-branch"); } diff --git a/crates/collab/src/tests/remote_editing_collaboration_tests.rs b/crates/collab/src/tests/remote_editing_collaboration_tests.rs index cfc7df7d5b..70fc6bd516 100644 --- a/crates/collab/src/tests/remote_editing_collaboration_tests.rs +++ b/crates/collab/src/tests/remote_editing_collaboration_tests.rs @@ -293,7 +293,7 @@ async fn test_ssh_collaboration_git_branches( let branches_b = branches_b .into_iter() - .map(|branch| branch.name.to_string()) + .map(|branch| branch.name().to_string()) .collect::>(); assert_eq!(&branches_b, &branches_set); @@ -326,7 +326,7 @@ async fn test_ssh_collaboration_git_branches( }) }); - assert_eq!(server_branch.name, branches[2]); + assert_eq!(server_branch.name(), branches[2]); // Also try creating a new branch cx_b.update(|cx| { @@ -366,7 +366,7 @@ async fn test_ssh_collaboration_git_branches( }) }); - assert_eq!(server_branch.name, "totally-new-branch"); + assert_eq!(server_branch.name(), "totally-new-branch"); // Remove the git repository and check that all participants get the update. remote_fs diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 18c39c83da..414338bd1d 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -322,7 +322,7 @@ impl GitRepository for FakeGitRepository { .iter() .map(|branch_name| Branch { is_head: Some(branch_name) == current_branch.as_ref(), - name: branch_name.into(), + ref_name: branch_name.into(), most_recent_commit: None, upstream: None, }) diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 14fade1074..02b8e1cf89 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -37,12 +37,24 @@ pub const REMOTE_CANCELLED_BY_USER: &str = "Operation cancelled by user"; #[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct Branch { pub is_head: bool, - pub name: SharedString, + pub ref_name: SharedString, pub upstream: Option, pub most_recent_commit: Option, } impl Branch { + pub fn name(&self) -> &str { + self.ref_name + .as_ref() + .strip_prefix("refs/heads/") + .or_else(|| self.ref_name.as_ref().strip_prefix("refs/remotes/")) + .unwrap_or(self.ref_name.as_ref()) + } + + pub fn is_remote(&self) -> bool { + self.ref_name.starts_with("refs/remotes/") + } + pub fn tracking_status(&self) -> Option { self.upstream .as_ref() @@ -71,6 +83,10 @@ impl Upstream { .strip_prefix("refs/remotes/") .and_then(|stripped| stripped.split("/").next()) } + + pub fn stripped_ref_name(&self) -> Option<&str> { + self.ref_name.strip_prefix("refs/remotes/") + } } #[derive(Clone, Copy, Default)] @@ -803,68 +819,69 @@ impl GitRepository for RealGitRepository { fn branches(&self) -> BoxFuture>> { let working_directory = self.working_directory(); let git_binary_path = self.git_binary_path.clone(); - async move { - let fields = [ - "%(HEAD)", - "%(objectname)", - "%(parent)", - "%(refname)", - "%(upstream)", - "%(upstream:track)", - "%(committerdate:unix)", - "%(contents:subject)", - ] - .join("%00"); - let args = vec![ - "for-each-ref", - "refs/heads/**/*", - "refs/remotes/**/*", - "--format", - &fields, - ]; - let working_directory = working_directory?; - let output = new_smol_command(&git_binary_path) - .current_dir(&working_directory) - .args(args) - .output() - .await?; - - if !output.status.success() { - return Err(anyhow!( - "Failed to git git branches:\n{}", - String::from_utf8_lossy(&output.stderr) - )); - } - - let input = String::from_utf8_lossy(&output.stdout); - - let mut branches = parse_branch_input(&input)?; - if branches.is_empty() { - let args = vec!["symbolic-ref", "--quiet", "--short", "HEAD"]; - + self.executor + .spawn(async move { + let fields = [ + "%(HEAD)", + "%(objectname)", + "%(parent)", + "%(refname)", + "%(upstream)", + "%(upstream:track)", + "%(committerdate:unix)", + "%(contents:subject)", + ] + .join("%00"); + let args = vec![ + "for-each-ref", + "refs/heads/**/*", + "refs/remotes/**/*", + "--format", + &fields, + ]; + let working_directory = working_directory?; let output = new_smol_command(&git_binary_path) .current_dir(&working_directory) .args(args) .output() .await?; - // git symbolic-ref returns a non-0 exit code if HEAD points - // to something other than a branch - if output.status.success() { - let name = String::from_utf8_lossy(&output.stdout).trim().to_string(); - - branches.push(Branch { - name: name.into(), - is_head: true, - upstream: None, - most_recent_commit: None, - }); + if !output.status.success() { + return Err(anyhow!( + "Failed to git git branches:\n{}", + String::from_utf8_lossy(&output.stderr) + )); } - } - Ok(branches) - } - .boxed() + let input = String::from_utf8_lossy(&output.stdout); + + let mut branches = parse_branch_input(&input)?; + if branches.is_empty() { + let args = vec!["symbolic-ref", "--quiet", "HEAD"]; + + let output = new_smol_command(&git_binary_path) + .current_dir(&working_directory) + .args(args) + .output() + .await?; + + // git symbolic-ref returns a non-0 exit code if HEAD points + // to something other than a branch + if output.status.success() { + let name = String::from_utf8_lossy(&output.stdout).trim().to_string(); + + branches.push(Branch { + ref_name: name.into(), + is_head: true, + upstream: None, + most_recent_commit: None, + }); + } + } + + Ok(branches) + }) + .boxed() } fn change_branch(&self, name: String) -> BoxFuture> { @@ -1691,15 +1708,7 @@ fn parse_branch_input(input: &str) -> Result> { let is_current_branch = fields.next().context("no HEAD")? == "*"; let head_sha: SharedString = fields.next().context("no objectname")?.to_string().into(); let parent_sha: SharedString = fields.next().context("no parent")?.to_string().into(); - let raw_ref_name = fields.next().context("no refname")?; - let ref_name: SharedString = - if let Some(ref_name) = raw_ref_name.strip_prefix("refs/heads/") { - ref_name.to_string().into() - } else if let Some(ref_name) = raw_ref_name.strip_prefix("refs/remotes/") { - ref_name.to_string().into() - } else { - return Err(anyhow!("unexpected format for refname")); - }; + let ref_name = fields.next().context("no refname")?.to_string().into(); let upstream_name = fields.next().context("no upstream")?.to_string(); let upstream_tracking = parse_upstream_track(fields.next().context("no upstream:track")?)?; let commiterdate = fields.next().context("no committerdate")?.parse::()?; @@ -1711,7 +1720,7 @@ fn parse_branch_input(input: &str) -> Result> { branches.push(Branch { is_head: is_current_branch, - name: ref_name, + ref_name: ref_name, most_recent_commit: Some(CommitSummary { sha: head_sha, subject, @@ -1974,7 +1983,7 @@ mod tests { parse_branch_input(&input).unwrap(), vec![Branch { is_head: true, - name: "zed-patches".into(), + ref_name: "refs/heads/zed-patches".into(), upstream: Some(Upstream { ref_name: "refs/remotes/origin/zed-patches".into(), tracking: UpstreamTracking::Tracked(UpstreamTrackingStatus { diff --git a/crates/git_ui/src/branch_picker.rs b/crates/git_ui/src/branch_picker.rs index 2c619cbd6f..2530bacf8d 100644 --- a/crates/git_ui/src/branch_picker.rs +++ b/crates/git_ui/src/branch_picker.rs @@ -1,6 +1,7 @@ use anyhow::{Context as _, anyhow}; use fuzzy::StringMatchCandidate; +use collections::HashSet; use git::repository::Branch; use gpui::{ App, Context, DismissEvent, Entity, EventEmitter, FocusHandle, Focusable, InteractiveElement, @@ -95,12 +96,28 @@ impl BranchList { .context("No active repository")? .await??; - all_branches.sort_by_key(|branch| { - branch - .most_recent_commit - .as_ref() - .map(|commit| 0 - commit.commit_timestamp) - }); + let all_branches = cx + .background_spawn(async move { + let upstreams: HashSet<_> = all_branches + .iter() + .filter_map(|branch| { + let upstream = branch.upstream.as_ref()?; + Some(upstream.ref_name.clone()) + }) + .collect(); + + all_branches.retain(|branch| !upstreams.contains(&branch.ref_name)); + + all_branches.sort_by_key(|branch| { + branch + .most_recent_commit + .as_ref() + .map(|commit| 0 - commit.commit_timestamp) + }); + + all_branches + }) + .await; this.update_in(cx, |this, window, cx| { this.picker.update(cx, |picker, cx| { @@ -266,6 +283,7 @@ impl PickerDelegate for BranchListDelegate { let mut matches: Vec = if query.is_empty() { all_branches .into_iter() + .filter(|branch| !branch.is_remote()) .take(RECENT_BRANCHES_COUNT) .map(|branch| BranchEntry { branch, @@ -277,7 +295,7 @@ impl PickerDelegate for BranchListDelegate { let candidates = all_branches .iter() .enumerate() - .map(|(ix, command)| StringMatchCandidate::new(ix, &command.name.clone())) + .map(|(ix, branch)| StringMatchCandidate::new(ix, branch.name())) .collect::>(); fuzzy::match_strings( &candidates, @@ -303,11 +321,11 @@ impl PickerDelegate for BranchListDelegate { if !query.is_empty() && !matches .first() - .is_some_and(|entry| entry.branch.name == query) + .is_some_and(|entry| entry.branch.name() == query) { matches.push(BranchEntry { branch: Branch { - name: query.clone().into(), + ref_name: format!("refs/heads/{query}").into(), is_head: false, upstream: None, most_recent_commit: None, @@ -335,19 +353,19 @@ impl PickerDelegate for BranchListDelegate { return; }; if entry.is_new { - self.create_branch(entry.branch.name.clone(), window, cx); + self.create_branch(entry.branch.name().to_owned().into(), window, cx); return; } let current_branch = self.repo.as_ref().map(|repo| { repo.update(cx, |repo, _| { - repo.branch.as_ref().map(|branch| branch.name.clone()) + repo.branch.as_ref().map(|branch| branch.ref_name.clone()) }) }); if current_branch .flatten() - .is_some_and(|current_branch| current_branch == entry.branch.name) + .is_some_and(|current_branch| current_branch == entry.branch.ref_name) { cx.emit(DismissEvent); return; @@ -368,7 +386,7 @@ impl PickerDelegate for BranchListDelegate { anyhow::Ok(async move { repo.update(&mut cx, |repo, _| { - repo.change_branch(branch.name.to_string()) + repo.change_branch(branch.name().to_string()) })? .await? }) @@ -443,13 +461,13 @@ impl PickerDelegate for BranchListDelegate { if entry.is_new { Label::new(format!( "Create branch \"{}\"…", - entry.branch.name + entry.branch.name() )) .single_line() .into_any_element() } else { HighlightedLabel::new( - entry.branch.name.clone(), + entry.branch.name().to_owned(), entry.positions.clone(), ) .truncate() @@ -470,7 +488,7 @@ impl PickerDelegate for BranchListDelegate { let message = if entry.is_new { if let Some(current_branch) = self.repo.as_ref().and_then(|repo| { - repo.read(cx).branch.as_ref().map(|b| b.name.clone()) + repo.read(cx).branch.as_ref().map(|b| b.name()) }) { format!("based off {}", current_branch) diff --git a/crates/git_ui/src/commit_modal.rs b/crates/git_ui/src/commit_modal.rs index 3d4987b083..6f4e67ddb9 100644 --- a/crates/git_ui/src/commit_modal.rs +++ b/crates/git_ui/src/commit_modal.rs @@ -321,8 +321,8 @@ impl CommitModal { let branch = active_repo .as_ref() .and_then(|repo| repo.read(cx).branch.as_ref()) - .map(|b| b.name.clone()) - .unwrap_or_else(|| "".into()); + .map(|b| b.name().to_owned()) + .unwrap_or_else(|| "".to_owned()); let branch_picker_button = panel_button(branch) .icon(IconName::GitBranch) diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index 6d2cc2542c..e35a25e025 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -1953,7 +1953,12 @@ impl GitPanel { })?; let pull = repo.update(cx, |repo, cx| { - repo.pull(branch.name.clone(), remote.name.clone(), askpass, cx) + repo.pull( + branch.name().to_owned().into(), + remote.name.clone(), + askpass, + cx, + ) })?; let remote_message = pull.await?; @@ -2020,7 +2025,7 @@ impl GitPanel { let push = repo.update(cx, |repo, cx| { repo.push( - branch.name.clone(), + branch.name().to_owned().into(), remote.name.clone(), options, askpass_delegate, @@ -2030,7 +2035,7 @@ impl GitPanel { let remote_output = push.await?; - let action = RemoteAction::Push(branch.name, remote); + let action = RemoteAction::Push(branch.name().to_owned().into(), remote); this.update(cx, |this, cx| match remote_output { Ok(remote_message) => this.show_remote_output(action, remote_message, cx), Err(e) => { @@ -2092,7 +2097,7 @@ impl GitPanel { return Err(anyhow::anyhow!("No active branch")); }; - Ok(repo.get_remotes(Some(current_branch.name.to_string()))) + Ok(repo.get_remotes(Some(current_branch.name().to_string()))) })?? .await??; @@ -4363,19 +4368,17 @@ impl RenderOnce for PanelRepoFooter { let branch_name = self .branch .as_ref() - .map(|branch| branch.name.clone()) + .map(|branch| branch.name().to_owned()) .or_else(|| { self.head_commit.as_ref().map(|commit| { - SharedString::from( - commit - .sha - .chars() - .take(MAX_SHORT_SHA_LEN) - .collect::(), - ) + commit + .sha + .chars() + .take(MAX_SHORT_SHA_LEN) + .collect::() }) }) - .unwrap_or_else(|| SharedString::from(" (no branch)")); + .unwrap_or_else(|| " (no branch)".to_owned()); let show_separator = self.branch.is_some() || self.head_commit.is_some(); let active_repo_name = self.active_repository.clone(); @@ -4542,7 +4545,7 @@ impl Component for PanelRepoFooter { fn branch(upstream: Option) -> Branch { Branch { is_head: true, - name: "some-branch".into(), + ref_name: "some-branch".into(), upstream: upstream.map(|tracking| Upstream { ref_name: "origin/some-branch".into(), tracking, @@ -4559,7 +4562,7 @@ impl Component for PanelRepoFooter { fn custom(branch_name: &str, upstream: Option) -> Branch { Branch { is_head: true, - name: branch_name.to_string().into(), + ref_name: branch_name.to_string().into(), upstream: upstream.map(|tracking| Upstream { ref_name: format!("zed/{}", branch_name).into(), tracking, diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index e88af5c47a..61f7097409 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/crates/git_ui/src/project_diff.rs @@ -1099,7 +1099,7 @@ impl RenderOnce for ProjectDiffEmptyState { v_flex() .child(Headline::new(ahead_string).size(HeadlineSize::Small)) .child( - Label::new(format!("Push your changes to {}", branch.name)) + Label::new(format!("Push your changes to {}", branch.name())) .color(Color::Muted), ), ) @@ -1113,7 +1113,7 @@ impl RenderOnce for ProjectDiffEmptyState { v_flex() .child(Headline::new("Publish Branch").size(HeadlineSize::Small)) .child( - Label::new(format!("Create {} on remote", branch.name)) + Label::new(format!("Create {} on remote", branch.name())) .color(Color::Muted), ), ) @@ -1183,7 +1183,7 @@ mod preview { fn branch(upstream: Option) -> Branch { Branch { is_head: true, - name: "some-branch".into(), + ref_name: "some-branch".into(), upstream: upstream.map(|tracking| Upstream { ref_name: "origin/some-branch".into(), tracking, diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index debb1c643b..d00f0a41fd 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -3790,13 +3790,9 @@ impl Repository { pub fn branches(&mut self) -> oneshot::Receiver>> { let id = self.id; - self.send_job(None, move |repo, cx| async move { + self.send_job(None, move |repo, _| async move { match repo { - RepositoryState::Local { backend, .. } => { - let backend = backend.clone(); - cx.background_spawn(async move { backend.branches().await }) - .await - } + RepositoryState::Local { backend, .. } => backend.branches().await, RepositoryState::Remote { project_id, client } => { let response = client .request(proto::GitGetBranches { @@ -4460,7 +4456,7 @@ fn deserialize_blame_buffer_response( fn branch_to_proto(branch: &git::repository::Branch) -> proto::Branch { proto::Branch { is_head: branch.is_head, - name: branch.name.to_string(), + ref_name: branch.ref_name.to_string(), unix_timestamp: branch .most_recent_commit .as_ref() @@ -4489,7 +4485,7 @@ fn branch_to_proto(branch: &git::repository::Branch) -> proto::Branch { fn proto_to_branch(proto: &proto::Branch) -> git::repository::Branch { git::repository::Branch { is_head: proto.is_head, - name: proto.name.clone().into(), + ref_name: proto.ref_name.clone().into(), upstream: proto .upstream .as_ref() diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index b2437d9c89..9f6ebf4ba7 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -75,7 +75,7 @@ message GetPermalinkToLineResponse { message Branch { bool is_head = 1; - string name = 2; + string ref_name = 2; optional uint64 unix_timestamp = 3; optional GitUpstream upstream = 4; optional CommitSummary most_recent_commit = 5; diff --git a/crates/remote_server/src/remote_editing_tests.rs b/crates/remote_server/src/remote_editing_tests.rs index d23d74537f..96863044cc 100644 --- a/crates/remote_server/src/remote_editing_tests.rs +++ b/crates/remote_server/src/remote_editing_tests.rs @@ -1472,7 +1472,7 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA let remote_branches = remote_branches .into_iter() - .map(|branch| branch.name.to_string()) + .map(|branch| branch.name().to_string()) .collect::>(); assert_eq!(&remote_branches, &branches_set); @@ -1505,7 +1505,7 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA }) }); - assert_eq!(server_branch.name, branches[2]); + assert_eq!(server_branch.name(), branches[2]); // Also try creating a new branch cx.update(|cx| { @@ -1545,7 +1545,7 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA }) }); - assert_eq!(server_branch.name, "totally-new-branch"); + assert_eq!(server_branch.name(), "totally-new-branch"); } pub async fn init_test( diff --git a/crates/title_bar/src/title_bar.rs b/crates/title_bar/src/title_bar.rs index 5819a0cab9..2001db9b8b 100644 --- a/crates/title_bar/src/title_bar.rs +++ b/crates/title_bar/src/title_bar.rs @@ -518,7 +518,7 @@ impl TitleBar { let repo = repository.read(cx); repo.branch .as_ref() - .map(|branch| branch.name.clone()) + .map(|branch| branch.name()) .map(|name| util::truncate_and_trailoff(&name, MAX_BRANCH_NAME_LENGTH)) .or_else(|| { repo.head_commit.as_ref().map(|commit| {