From 126d708fa1dc493e2d0fafb477bd814d40df0238 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Fri, 5 Dec 2025 07:59:13 -0500 Subject: [PATCH] git: Fix branch picker creating new branches with refs/head/ prefixed on the branch name (#44206) The bug was introduced in this recent PR: https://github.com/zed-industries/zed/pull/42819. Since it's still in nightly, there is no need for release notes. I also polished the feature a bit by: - Ensuring branch names are always a single line so the branch picker's uniform list uses the correct element height. - Adding tooltip text for the filter remotes button. - Fixing the create branch from default icon showing up for non-new branch entries. Release Notes: - N/A --- assets/keymaps/default-linux.json | 3 +- assets/keymaps/default-macos.json | 3 +- assets/keymaps/default-windows.json | 3 +- crates/fs/src/fake_git_repo.rs | 17 ++++-- crates/git_ui/src/branch_picker.rs | 89 +++++++++++++++++------------ 5 files changed, 69 insertions(+), 46 deletions(-) diff --git a/assets/keymaps/default-linux.json b/assets/keymaps/default-linux.json index 0b001f31790c7f8211a6b44d227c15a6ff605ca4..41415bf2047e1faadd86dd5be159f526d6c57678 100644 --- a/assets/keymaps/default-linux.json +++ b/assets/keymaps/default-linux.json @@ -1332,7 +1332,8 @@ "context": "GitBranchSelector || (GitBranchSelector > Picker > Editor)", "use_key_equivalents": true, "bindings": { - "ctrl-shift-backspace": "branch_picker::DeleteBranch" + "ctrl-shift-backspace": "branch_picker::DeleteBranch", + "ctrl-shift-i": "branch_picker::FilterRemotes" } } ] diff --git a/assets/keymaps/default-macos.json b/assets/keymaps/default-macos.json index e4595242d570628e2e70c43b66d14a0f9820512b..fa8edbe5c23b008eb2c267850e440a851c54087d 100644 --- a/assets/keymaps/default-macos.json +++ b/assets/keymaps/default-macos.json @@ -1437,7 +1437,8 @@ "context": "GitBranchSelector || (GitBranchSelector > Picker > Editor)", "use_key_equivalents": true, "bindings": { - "cmd-shift-backspace": "branch_picker::DeleteBranch" + "cmd-shift-backspace": "branch_picker::DeleteBranch", + "cmd-shift-i": "branch_picker::FilterRemotes" } } ] diff --git a/assets/keymaps/default-windows.json b/assets/keymaps/default-windows.json index b625e7c7018c0f4c8277fcf3f739a8f06361c4df..45f37fbd41af3fcc3108f0ffe150a80ff25332e1 100644 --- a/assets/keymaps/default-windows.json +++ b/assets/keymaps/default-windows.json @@ -1351,7 +1351,8 @@ "context": "GitBranchSelector || (GitBranchSelector > Picker > Editor)", "use_key_equivalents": true, "bindings": { - "ctrl-shift-backspace": "branch_picker::DeleteBranch" + "ctrl-shift-backspace": "branch_picker::DeleteBranch", + "ctrl-shift-i": "branch_picker::FilterRemotes" } } ] diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 3bc411ff2d9b917fd409c29cca03d2191ee80978..6ca8b5a58f9f8f75023aa73e7a80e8547eb156f3 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -381,11 +381,18 @@ impl GitRepository for FakeGitRepository { Ok(state .branches .iter() - .map(|branch_name| Branch { - is_head: Some(branch_name) == current_branch.as_ref(), - ref_name: branch_name.into(), - most_recent_commit: None, - upstream: None, + .map(|branch_name| { + let ref_name = if branch_name.starts_with("refs/") { + branch_name.into() + } else { + format!("refs/heads/{branch_name}").into() + }; + Branch { + is_head: Some(branch_name) == current_branch.as_ref(), + ref_name, + most_recent_commit: None, + upstream: None, + } }) .collect()) }) diff --git a/crates/git_ui/src/branch_picker.rs b/crates/git_ui/src/branch_picker.rs index 42e043cada2813126af3489c9769aca9c675999f..33b852c1de9b1bd1a8abcc36dff964d14cbe1807 100644 --- a/crates/git_ui/src/branch_picker.rs +++ b/crates/git_ui/src/branch_picker.rs @@ -770,7 +770,7 @@ impl PickerDelegate for BranchListDelegate { } else { None }; - self.create_branch(from_branch, format!("refs/heads/{name}").into(), window, cx); + self.create_branch(from_branch, name.into(), window, cx); } } @@ -812,28 +812,21 @@ impl PickerDelegate for BranchListDelegate { }) .unwrap_or_else(|| (None, None, None)); - let icon = if let Some(default_branch) = self.default_branch.clone() { - let icon = match &entry { - Entry::Branch { .. } => Some(( - IconName::GitBranchAlt, - format!("Create branch based off default: {default_branch}"), - )), - Entry::NewUrl { url } => { - Some((IconName::Screen, format!("Create remote based off {url}"))) - } - Entry::NewBranch { .. } => None, - }; + let icon = if let Some(default_branch) = self.default_branch.clone() + && matches!(entry, Entry::NewBranch { .. }) + { + let tooltip_text = format!("Create branch based off default: {default_branch}"); - icon.map(|(icon, tooltip_text)| { - IconButton::new("branch-from-default", icon) + Some( + IconButton::new("branch-from-default", IconName::GitBranchAlt) .on_click(cx.listener(move |this, _, window, cx| { this.delegate.set_selected_index(ix, window, cx); this.delegate.confirm(true, window, cx); })) .tooltip(move |_window, cx| { Tooltip::for_action(tooltip_text.clone(), &menu::SecondaryConfirm, cx) - }) - }) + }), + ) } else { None }; @@ -875,7 +868,9 @@ impl PickerDelegate for BranchListDelegate { .max_w_48() .child(h_flex().mr_1().child(icon_element)) .child( - HighlightedLabel::new(branch.name().to_string(), positions.clone()).truncate(), + HighlightedLabel::new(branch.name().to_string(), positions.clone()) + .single_line() + .truncate(), ) .into_any_element(), }; @@ -962,18 +957,13 @@ impl PickerDelegate for BranchListDelegate { _window: &mut Window, cx: &mut Context>, ) -> Option { - if matches!( - self.state, - PickerState::CreateRemote(_) | PickerState::NewRemote | PickerState::NewBranch - ) { - return None; - } - let label = if self.display_remotes { - "Remote" - } else { - "Local" - }; - Some( + matches!(self.state, PickerState::List).then(|| { + let label = if self.display_remotes { + "Remote" + } else { + "Local" + }; + h_flex() .w_full() .p_1p5() @@ -981,8 +971,8 @@ impl PickerDelegate for BranchListDelegate { .border_t_1() .border_color(cx.theme().colors().border_variant) .child(Label::new(label).size(LabelSize::Small).color(Color::Muted)) - .into_any(), - ) + .into_any() + }) } fn render_footer(&self, _: &mut Window, cx: &mut Context>) -> Option { @@ -1010,7 +1000,8 @@ impl PickerDelegate for BranchListDelegate { .border_t_1() .border_color(cx.theme().colors().border_variant) .justify_between() - .child( + .child({ + let focus_handle = focus_handle.clone(); Button::new("filter-remotes", "Filter remotes") .key_binding( KeyBinding::for_action_in( @@ -1028,8 +1019,26 @@ impl PickerDelegate for BranchListDelegate { }) .disabled(self.loading) .style(ButtonStyle::Subtle) - .toggle_state(self.display_remotes), - ) + .toggle_state(self.display_remotes) + .tooltip({ + let state = self.display_remotes; + + move |_window, cx| { + let tooltip_text = if state { + "Show local branches" + } else { + "Show remote branches" + }; + + Tooltip::for_action_in( + tooltip_text, + &branch_picker::FilterRemotes, + &focus_handle, + cx, + ) + } + }) + }) .child( Button::new("delete-branch", "Delete") .key_binding( @@ -1527,10 +1536,14 @@ mod tests { .unwrap() .unwrap(); - assert!( - branches - .into_iter() - .any(|branch| branch.name() == "new-feature-branch") + let new_branch = branches + .into_iter() + .find(|branch| branch.name() == "new-feature-branch") + .expect("new-feature-branch should exist"); + assert_eq!( + new_branch.ref_name.as_ref(), + "refs/heads/new-feature-branch", + "branch ref_name should not have duplicate refs/heads/ prefix" ); }