oak-team/oak-gpui
3
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

git: Fix panic in git panel when sort_by_path is true (#39678)

Browse Source 
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
This commit is contained in:
Bennet Bo Fenner
2025-10-07 13:05:13 +00:00
committed by GitHub
parent 3bec885536
commit 85985fe960
1 changed files with 253 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/git_ui/src/git_panel.rs
+253 -1
View File
@@ -386,6 +386,7 @@ impl GitPanel {
cx.observe_global_in::<SettingsStore>(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())
);
}
}
}