From c58931ac04a2e0fa66653ca367fff449ab5b696d Mon Sep 17 00:00:00 2001 From: Dino Date: Thu, 9 Oct 2025 15:34:52 +0100 Subject: [PATCH] git_ui: Fix open diff for untracked files when sorting by path enabled (#39862) Fixes the `Open Diff` action for untracked files when the `sort_by_path` setting is enabled. The `ProjectDiff` wasn't correctly moving the multibuffer's cursor to the untracked file because, when that setting is enabled, it's sort prefix is changed to the tracked files sort prefix, and that wasn't accounted for in `move_to_entry`. Before these changes, the `sort_prefix` field for `PathKey` was called `namespace`, it was renamed to be clearer what its purpose is. Closes #39529 Release Notes: - Fixed 'Open Diff' action for untracked files when `sort_by_path` is enabled --------- Co-authored-by: David Kleingeld --- crates/editor/src/editor_tests.rs | 6 +- crates/git_ui/src/commit_view.rs | 8 +-- crates/git_ui/src/git_panel.rs | 63 +++++++++++++++++++ crates/git_ui/src/project_diff.rs | 50 +++++++-------- crates/multi_buffer/src/multi_buffer.rs | 11 ++-- crates/multi_buffer/src/multi_buffer_tests.rs | 8 +-- 6 files changed, 103 insertions(+), 43 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 4dab8ae474..8f7dac889d 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -16518,7 +16518,7 @@ async fn test_following_with_multiple_excerpts(cx: &mut TestAppContext) { leader.update(cx, |leader, cx| { leader.buffer.update(cx, |multibuffer, cx| { multibuffer.set_excerpts_for_path( - PathKey::namespaced(1, rel_path("b.txt").into_arc()), + PathKey::with_sort_prefix(1, rel_path("b.txt").into_arc()), buffer_1.clone(), vec![ Point::row_range(0..3), @@ -16529,7 +16529,7 @@ async fn test_following_with_multiple_excerpts(cx: &mut TestAppContext) { cx, ); multibuffer.set_excerpts_for_path( - PathKey::namespaced(1, rel_path("a.txt").into_arc()), + PathKey::with_sort_prefix(1, rel_path("a.txt").into_arc()), buffer_2.clone(), vec![Point::row_range(0..6), Point::row_range(8..12)], 0, @@ -21032,7 +21032,7 @@ async fn test_display_diff_hunks(cx: &mut TestAppContext) { for buffer in &buffers { let snapshot = buffer.read(cx).snapshot(); multibuffer.set_excerpts_for_path( - PathKey::namespaced(0, buffer.read(cx).file().unwrap().path().clone()), + PathKey::with_sort_prefix(0, buffer.read(cx).file().unwrap().path().clone()), buffer.clone(), vec![text::Anchor::MIN.to_point(&snapshot)..text::Anchor::MAX.to_point(&snapshot)], 2, diff --git a/crates/git_ui/src/commit_view.rs b/crates/git_ui/src/commit_view.rs index a87db56c96..201a699e2f 100644 --- a/crates/git_ui/src/commit_view.rs +++ b/crates/git_ui/src/commit_view.rs @@ -43,8 +43,8 @@ struct CommitMetadataFile { worktree_id: WorktreeId, } -const COMMIT_METADATA_NAMESPACE: u64 = 0; -const FILE_NAMESPACE: u64 = 1; +const COMMIT_METADATA_SORT_PREFIX: u64 = 0; +const FILE_NAMESPACE_SORT_PREFIX: u64 = 1; impl CommitView { pub fn open( @@ -145,7 +145,7 @@ impl CommitView { }); multibuffer.update(cx, |multibuffer, cx| { multibuffer.set_excerpts_for_path( - PathKey::namespaced(COMMIT_METADATA_NAMESPACE, file.title.clone()), + PathKey::with_sort_prefix(COMMIT_METADATA_SORT_PREFIX, file.title.clone()), buffer.clone(), vec![Point::zero()..buffer.read(cx).max_point()], 0, @@ -193,7 +193,7 @@ impl CommitView { .collect::>(); let path = snapshot.file().unwrap().path().clone(); let _is_newly_added = multibuffer.set_excerpts_for_path( - PathKey::namespaced(FILE_NAMESPACE, path), + PathKey::with_sort_prefix(FILE_NAMESPACE_SORT_PREFIX, path), buffer, diff_hunk_ranges, multibuffer_context_lines(cx), diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index d8e15247c1..ce6ddf43f6 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -4973,6 +4973,7 @@ mod tests { use settings::SettingsStore; use theme::LoadThemes; use util::path; + use util::rel_path::rel_path; use super::*; @@ -5595,6 +5596,68 @@ mod tests { }); } + #[gpui::test] + async fn test_open_diff(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "tracked": "tracked\n", + "untracked": "\n", + }), + ) + .await; + + fs.set_head_and_index_for_repo( + path!("/project/.git").as_ref(), + &[("tracked", "old tracked\n".into())], + ); + + let project = Project::test(fs.clone(), [Path::new(path!("/project"))], cx).await; + let workspace = + cx.add_window(|window, cx| Workspace::test_new(project.clone(), window, cx)); + let cx = &mut VisualTestContext::from_window(*workspace, cx); + let panel = workspace.update(cx, GitPanel::new).unwrap(); + + // Enable the `sort_by_path` setting and wait for entries to be updated, + // as there should no longer be separators between Tracked and Untracked + // files. + cx.update(|_window, cx| { + SettingsStore::update_global(cx, |store, cx| { + store.update_user_settings(cx, |settings| { + settings.git_panel.get_or_insert_default().sort_by_path = Some(true); + }) + }); + }); + + cx.update_window_entity(&panel, |panel, _, _| { + std::mem::replace(&mut panel.update_visible_entries_task, Task::ready(())) + }) + .await; + + // Confirm that `Open Diff` still works for the untracked file, updating + // the Project Diff's active path. + panel.update_in(cx, |panel, window, cx| { + panel.selected_entry = Some(1); + panel.open_diff(&Confirm, window, cx); + }); + cx.run_until_parked(); + + let _ = workspace.update(cx, |workspace, _window, cx| { + let active_path = workspace + .item_of_type::(cx) + .expect("ProjectDiff should exist") + .read(cx) + .active_path(cx) + .expect("active_path should exist"); + + assert_eq!(active_path.path, rel_path("untracked").into_arc()); + }); + } + fn assert_entry_paths(entries: &[GitListEntry], expected_paths: &[Option<&str>]) { assert_eq!(entries.len(), expected_paths.len()); for (entry, expected_path) in entries.iter().zip(expected_paths) { diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index 0e59d25222..6b70f1975e 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/crates/git_ui/src/project_diff.rs @@ -16,7 +16,7 @@ use editor::{ use futures::StreamExt; use git::{ Commit, StageAll, StageAndNext, ToggleStaged, UnstageAll, UnstageAndNext, - repository::{Branch, Upstream, UpstreamTracking, UpstreamTrackingStatus}, + repository::{Branch, RepoPath, Upstream, UpstreamTracking, UpstreamTrackingStatus}, status::FileStatus, }; use gpui::{ @@ -27,7 +27,7 @@ use language::{Anchor, Buffer, Capability, OffsetRangeExt}; use multi_buffer::{MultiBuffer, PathKey}; use project::{ Project, ProjectPath, - git_store::{GitStore, GitStoreEvent}, + git_store::{GitStore, GitStoreEvent, Repository}, }; use settings::{Settings, SettingsStore}; use std::any::{Any, TypeId}; @@ -73,9 +73,9 @@ struct DiffBuffer { file_status: FileStatus, } -const CONFLICT_NAMESPACE: u64 = 1; -const TRACKED_NAMESPACE: u64 = 2; -const NEW_NAMESPACE: u64 = 3; +const CONFLICT_SORT_PREFIX: u64 = 1; +const TRACKED_SORT_PREFIX: u64 = 2; +const NEW_SORT_PREFIX: u64 = 3; impl ProjectDiff { pub(crate) fn register(workspace: &mut Workspace, cx: &mut Context) { @@ -234,16 +234,8 @@ impl ProjectDiff { return; }; let repo = git_repo.read(cx); - - let namespace = if repo.had_conflict_on_last_merge_head_change(&entry.repo_path) { - CONFLICT_NAMESPACE - } else if entry.status.is_created() { - NEW_NAMESPACE - } else { - TRACKED_NAMESPACE - }; - - let path_key = PathKey::namespaced(namespace, entry.repo_path.0); + let sort_prefix = sort_prefix(repo, &entry.repo_path, entry.status, cx); + let path_key = PathKey::with_sort_prefix(sort_prefix, entry.repo_path.0); self.move_to_path(path_key, window, cx) } @@ -388,16 +380,8 @@ impl ProjectDiff { else { continue; }; - let namespace = if GitPanelSettings::get_global(cx).sort_by_path { - TRACKED_NAMESPACE - } else if repo.had_conflict_on_last_merge_head_change(&entry.repo_path) { - CONFLICT_NAMESPACE - } else if entry.status.is_created() { - NEW_NAMESPACE - } else { - TRACKED_NAMESPACE - }; - let path_key = PathKey::namespaced(namespace, entry.repo_path.0.clone()); + let sort_prefix = sort_prefix(repo, &entry.repo_path, entry.status, cx); + let path_key = PathKey::with_sort_prefix(sort_prefix, entry.repo_path.0.clone()); previous_paths.remove(&path_key); let load_buffer = self @@ -541,6 +525,18 @@ impl ProjectDiff { } } +fn sort_prefix(repo: &Repository, repo_path: &RepoPath, status: FileStatus, cx: &App) -> u64 { + if GitPanelSettings::get_global(cx).sort_by_path { + TRACKED_SORT_PREFIX + } else if repo.had_conflict_on_last_merge_head_change(repo_path) { + CONFLICT_SORT_PREFIX + } else if status.is_created() { + NEW_SORT_PREFIX + } else { + TRACKED_SORT_PREFIX + } +} + impl EventEmitter for ProjectDiff {} impl Focusable for ProjectDiff { @@ -1463,7 +1459,7 @@ mod tests { let editor = cx.update_window_entity(&diff, |diff, window, cx| { diff.move_to_path( - PathKey::namespaced(TRACKED_NAMESPACE, rel_path("foo").into_arc()), + PathKey::with_sort_prefix(TRACKED_SORT_PREFIX, rel_path("foo").into_arc()), window, cx, ); @@ -1484,7 +1480,7 @@ mod tests { let editor = cx.update_window_entity(&diff, |diff, window, cx| { diff.move_to_path( - PathKey::namespaced(TRACKED_NAMESPACE, rel_path("bar").into_arc()), + PathKey::with_sort_prefix(TRACKED_SORT_PREFIX, rel_path("bar").into_arc()), window, cx, ); diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 31ffde7bb9..be01c4b6a1 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -161,24 +161,25 @@ impl MultiBufferDiffHunk { #[derive(PartialEq, Eq, Ord, PartialOrd, Clone, Hash, Debug)] pub struct PathKey { - namespace: Option, + // Used by the derived PartialOrd & Ord + sort_prefix: Option, path: Arc, } impl PathKey { - pub fn namespaced(namespace: u64, path: Arc) -> Self { + pub fn with_sort_prefix(sort_prefix: u64, path: Arc) -> Self { Self { - namespace: Some(namespace), + sort_prefix: Some(sort_prefix), path, } } pub fn for_buffer(buffer: &Entity, cx: &App) -> Self { if let Some(file) = buffer.read(cx).file() { - Self::namespaced(file.worktree_id(cx).to_proto(), file.path().clone()) + Self::with_sort_prefix(file.worktree_id(cx).to_proto(), file.path().clone()) } else { Self { - namespace: None, + sort_prefix: None, path: RelPath::unix(&buffer.entity_id().to_string()) .unwrap() .into_arc(), diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index e82c44d644..bad99d5412 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -1525,7 +1525,7 @@ fn test_set_excerpts_for_buffer_ordering(cx: &mut TestAppContext) { cx, ) }); - let path1: PathKey = PathKey::namespaced(0, rel_path("root").into_arc()); + let path1: PathKey = PathKey::with_sort_prefix(0, rel_path("root").into_arc()); let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite)); multibuffer.update(cx, |multibuffer, cx| { @@ -1620,7 +1620,7 @@ fn test_set_excerpts_for_buffer(cx: &mut TestAppContext) { cx, ) }); - let path1: PathKey = PathKey::namespaced(0, rel_path("root").into_arc()); + let path1: PathKey = PathKey::with_sort_prefix(0, rel_path("root").into_arc()); let buf2 = cx.new(|cx| { Buffer::local( indoc! { @@ -1639,7 +1639,7 @@ fn test_set_excerpts_for_buffer(cx: &mut TestAppContext) { cx, ) }); - let path2 = PathKey::namespaced(1, rel_path("root").into_arc()); + let path2 = PathKey::with_sort_prefix(1, rel_path("root").into_arc()); let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite)); multibuffer.update(cx, |multibuffer, cx| { @@ -1816,7 +1816,7 @@ fn test_set_excerpts_for_buffer_rename(cx: &mut TestAppContext) { cx, ) }); - let path: PathKey = PathKey::namespaced(0, rel_path("root").into_arc()); + let path: PathKey = PathKey::with_sort_prefix(0, rel_path("root").into_arc()); let buf2 = cx.new(|cx| { Buffer::local( indoc! {