From 85985fe9608ea59907b673b53f563019da1917b9 Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Tue, 7 Oct 2025 15:05:13 +0200 Subject: [PATCH] git: Fix panic in git panel when `sort_by_path` is `true` (#39678) Fixes ZED-1NX This panic could occur when an `bulk_staging` was set to `Some(...)` and `sort_by_path` was set to `true`. When setting `sort_by_path: true`, we call `update_visible_entries(...)` which then checks if `bulk_staging ` is `Some(...)` and calls `entry_by_path`. That function accesses `entries`, which still consists of both headers and entries. But the code (`entry.status_entry().unwrap()`) assumes that there are no headers in the entry list if `sort_by_path: true`. ```rust if GitPanelSettings::get_global(cx).sort_by_path { return self .entries .binary_search_by(|entry| entry.status_entry().unwrap().repo_path.cmp(path)) //This unwrap() would panic .ok(); } ``` This has now been fixed by clearing all the entries when `sort_by_path` changes, as this is the only case where our assumptions are invalid. I also added a test which 1) actually tests the sort_by_path logic 2) ensures that we do not re-introduce this panic in the future. Release Notes: - Fixed a panic that could occur when using `sort_by_path: true` in the git panel --- crates/git_ui/src/git_panel.rs | 254 ++++++++++++++++++++++++++++++++- 1 file changed, 253 insertions(+), 1 deletion(-) diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index 7914e488a96f730766cd744faa10bc95b403c251..4c76030f5f596eb2d4d13178e4210c4bcd399bb0 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -386,6 +386,7 @@ impl GitPanel { cx.observe_global_in::(window, move |this, window, cx| { let is_sort_by_path = GitPanelSettings::get_global(cx).sort_by_path; if is_sort_by_path != was_sort_by_path { + this.entries.clear(); this.update_visible_entries(window, cx); } was_sort_by_path = is_sort_by_path @@ -4887,7 +4888,7 @@ mod tests { repository::repo_path, status::{StatusCode, UnmergedStatus, UnmergedStatusCode}, }; - use gpui::{TestAppContext, VisualTestContext}; + use gpui::{TestAppContext, UpdateGlobal, VisualTestContext}; use project::{FakeFs, WorktreeSettings}; use serde_json::json; use settings::SettingsStore; @@ -5210,6 +5211,242 @@ mod tests { ); } + #[gpui::test] + async fn test_bulk_staging_with_sort_by_paths(cx: &mut TestAppContext) { + use GitListEntry::*; + + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + "/root", + json!({ + "project": { + ".git": {}, + "src": { + "main.rs": "fn main() {}", + "lib.rs": "pub fn hello() {}", + "utils.rs": "pub fn util() {}" + }, + "tests": { + "test.rs": "fn test() {}" + }, + "new_file.txt": "new content", + "another_new.rs": "// new file", + "conflict.txt": "conflicted content" + } + }), + ) + .await; + + fs.set_status_for_repo( + Path::new(path!("/root/project/.git")), + &[ + ("src/main.rs", StatusCode::Modified.worktree()), + ("src/lib.rs", StatusCode::Modified.worktree()), + ("tests/test.rs", StatusCode::Modified.worktree()), + ("new_file.txt", FileStatus::Untracked), + ("another_new.rs", FileStatus::Untracked), + ("src/utils.rs", FileStatus::Untracked), + ( + "conflict.txt", + UnmergedStatus { + first_head: UnmergedStatusCode::Updated, + second_head: UnmergedStatusCode::Updated, + } + .into(), + ), + ], + ); + + let project = Project::test(fs.clone(), [Path::new(path!("/root/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); + + cx.read(|cx| { + project + .read(cx) + .worktrees(cx) + .next() + .unwrap() + .read(cx) + .as_local() + .unwrap() + .scan_complete() + }) + .await; + + cx.executor().run_until_parked(); + + let panel = workspace.update(cx, GitPanel::new).unwrap(); + + let handle = cx.update_window_entity(&panel, |panel, _, _| { + std::mem::replace(&mut panel.update_visible_entries_task, Task::ready(())) + }); + cx.executor().advance_clock(2 * UPDATE_DEBOUNCE); + handle.await; + + let entries = panel.read_with(cx, |panel, _| panel.entries.clone()); + #[rustfmt::skip] + pretty_assertions::assert_matches!( + entries.as_slice(), + &[ + Header(GitHeaderEntry { header: Section::Conflict }), + Status(GitStatusEntry { staging: StageStatus::Unstaged, .. }), + Header(GitHeaderEntry { header: Section::Tracked }), + Status(GitStatusEntry { staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { staging: StageStatus::Unstaged, .. }), + Header(GitHeaderEntry { header: Section::New }), + Status(GitStatusEntry { staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { staging: StageStatus::Unstaged, .. }), + ], + ); + + assert_entry_paths( + &entries, + &[ + None, + Some("conflict.txt"), + None, + Some("src/lib.rs"), + Some("src/main.rs"), + Some("tests/test.rs"), + None, + Some("another_new.rs"), + Some("new_file.txt"), + Some("src/utils.rs"), + ], + ); + + let second_status_entry = entries[3].clone(); + panel.update_in(cx, |panel, window, cx| { + panel.toggle_staged_for_entry(&second_status_entry, window, cx); + }); + + 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); + }) + }); + }); + + panel.update_in(cx, |panel, window, cx| { + panel.selected_entry = Some(7); + panel.stage_range(&git::StageRange, window, cx); + }); + + cx.read(|cx| { + project + .read(cx) + .worktrees(cx) + .next() + .unwrap() + .read(cx) + .as_local() + .unwrap() + .scan_complete() + }) + .await; + + cx.executor().run_until_parked(); + + let handle = cx.update_window_entity(&panel, |panel, _, _| { + std::mem::replace(&mut panel.update_visible_entries_task, Task::ready(())) + }); + cx.executor().advance_clock(2 * UPDATE_DEBOUNCE); + handle.await; + + let entries = panel.read_with(cx, |panel, _| panel.entries.clone()); + #[rustfmt::skip] + pretty_assertions::assert_matches!( + entries.as_slice(), + &[ + Status(GitStatusEntry { status: FileStatus::Untracked, staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Unmerged(..), staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Untracked, staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Tracked(..), staging: StageStatus::Staged, .. }), + Status(GitStatusEntry { status: FileStatus::Tracked(..), staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Untracked, staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Tracked(..), staging: StageStatus::Unstaged, .. }), + ], + ); + + assert_entry_paths( + &entries, + &[ + Some("another_new.rs"), + Some("conflict.txt"), + Some("new_file.txt"), + Some("src/lib.rs"), + Some("src/main.rs"), + Some("src/utils.rs"), + Some("tests/test.rs"), + ], + ); + + let third_status_entry = entries[4].clone(); + panel.update_in(cx, |panel, window, cx| { + panel.toggle_staged_for_entry(&third_status_entry, window, cx); + }); + + panel.update_in(cx, |panel, window, cx| { + panel.selected_entry = Some(9); + panel.stage_range(&git::StageRange, window, cx); + }); + + cx.read(|cx| { + project + .read(cx) + .worktrees(cx) + .next() + .unwrap() + .read(cx) + .as_local() + .unwrap() + .scan_complete() + }) + .await; + + cx.executor().run_until_parked(); + + let handle = cx.update_window_entity(&panel, |panel, _, _| { + std::mem::replace(&mut panel.update_visible_entries_task, Task::ready(())) + }); + cx.executor().advance_clock(2 * UPDATE_DEBOUNCE); + handle.await; + + let entries = panel.read_with(cx, |panel, _| panel.entries.clone()); + #[rustfmt::skip] + pretty_assertions::assert_matches!( + entries.as_slice(), + &[ + Status(GitStatusEntry { status: FileStatus::Untracked, staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Unmerged(..), staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Untracked, staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Tracked(..), staging: StageStatus::Staged, .. }), + Status(GitStatusEntry { status: FileStatus::Tracked(..), staging: StageStatus::Staged, .. }), + Status(GitStatusEntry { status: FileStatus::Untracked, staging: StageStatus::Unstaged, .. }), + Status(GitStatusEntry { status: FileStatus::Tracked(..), staging: StageStatus::Unstaged, .. }), + ], + ); + + assert_entry_paths( + &entries, + &[ + Some("another_new.rs"), + Some("conflict.txt"), + Some("new_file.txt"), + Some("src/lib.rs"), + Some("src/main.rs"), + Some("src/utils.rs"), + Some("tests/test.rs"), + ], + ); + } + #[gpui::test] async fn test_amend_commit_message_handling(cx: &mut TestAppContext) { init_test(cx); @@ -5278,4 +5515,19 @@ mod tests { assert_eq!(current_message, ""); }); } + + 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) { + assert_eq!( + entry.status_entry().map(|status| status + .repo_path + .0 + .as_std_path() + .to_string_lossy() + .to_string()), + expected_path.map(|s| s.to_string()) + ); + } + } }