From 0149de4b54c55df8ec9ebc6a1da5b43c68e407e9 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 10 Nov 2025 10:27:51 +0100 Subject: [PATCH] git: Fix panic in `git2` due to empty repo paths (#42304) Fixes ZED-1VR Release Notes: - Fixed sporadic panic in git features --- crates/agent_servers/src/acp.rs | 2 +- crates/editor/src/test/editor_test_context.rs | 6 +- crates/fs/src/fake_git_repo.rs | 4 +- crates/fs/src/fs.rs | 3 +- crates/git/src/repository.rs | 72 +++++++++---------- crates/git/src/status.rs | 4 +- crates/git_ui/src/commit_view.rs | 2 +- crates/git_ui/src/git_panel.rs | 8 +-- crates/git_ui/src/project_diff.rs | 4 +- crates/project/src/git_store.rs | 31 ++++---- crates/project/src/git_store/conflict_set.rs | 4 +- crates/project/src/git_store/pending_op.rs | 4 +- crates/project/src/project_tests.rs | 26 +++---- 13 files changed, 88 insertions(+), 82 deletions(-) diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index 4e41b247599e511b6345882701a34a6b66f3c418..15f56bf2ed4ee100fd22dc0d7df73f2e8a3274ea 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -136,7 +136,7 @@ impl AcpConnection { while let Ok(n) = stderr.read_line(&mut line).await && n > 0 { - log::warn!("agent stderr: {}", &line); + log::warn!("agent stderr: {}", line.trim()); line.clear(); } Ok(()) diff --git a/crates/editor/src/test/editor_test_context.rs b/crates/editor/src/test/editor_test_context.rs index c6779d1e564deb57233dd9e4719ca87f8d6a2da1..7f5bb227fb98d1ebe5df51d59bdae22825bc4fef 100644 --- a/crates/editor/src/test/editor_test_context.rs +++ b/crates/editor/src/test/editor_test_context.rs @@ -6,6 +6,7 @@ use buffer_diff::DiffHunkStatusKind; use collections::BTreeMap; use futures::Future; +use git::repository::RepoPath; use gpui::{ AnyWindowHandle, App, Context, Entity, Focusable as _, Keystroke, Pixels, Point, VisualTestContext, Window, WindowHandle, prelude::*, @@ -334,7 +335,10 @@ impl EditorTestContext { let path = self.update_buffer(|buffer, _| buffer.file().unwrap().path().clone()); let mut found = None; fs.with_git_state(&Self::root_path().join(".git"), false, |git_state| { - found = git_state.index_contents.get(&path.into()).cloned(); + found = git_state + .index_contents + .get(&RepoPath::from_rel_path(&path)) + .cloned(); }) .unwrap(); assert_eq!(expected, found.as_deref()); diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index e3d58f3001c407f8d4deea115b460000bc666574..97cd13d185817453c369356bdc60cbc1517bf1e1 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -272,7 +272,7 @@ impl GitRepository for FakeGitRepository { .ok() .map(|content| String::from_utf8(content).unwrap())?; let repo_path = RelPath::new(repo_path, PathStyle::local()).ok()?; - Some((repo_path.into(), (content, is_ignored))) + Some((RepoPath::from_rel_path(&repo_path), (content, is_ignored))) }) .collect(); @@ -436,7 +436,7 @@ impl GitRepository for FakeGitRepository { state .blames .get(&path) - .with_context(|| format!("failed to get blame for {:?}", path.0)) + .with_context(|| format!("failed to get blame for {:?}", path)) .cloned() }) } diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 0202b2134f4fd0d3f983b2c67e97414a44457143..53af6ba6afc50cb0e568a01e25d1af22c02d9e36 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -1792,7 +1792,8 @@ impl FakeFs { for (path, content) in workdir_contents { use util::{paths::PathStyle, rel_path::RelPath}; - let repo_path: RepoPath = RelPath::new(path.strip_prefix(&workdir_path).unwrap(), PathStyle::local()).unwrap().into(); + let repo_path = RelPath::new(path.strip_prefix(&workdir_path).unwrap(), PathStyle::local()).unwrap(); + let repo_path = RepoPath::from_rel_path(&repo_path); let status = statuses .iter() .find_map(|(p, status)| (*p == repo_path.as_unix_str()).then_some(status)); diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 1ad21d993607c75777302ddb6f4ec1964a916ad0..2a1cd9478d3079716eda8234c02c8122b9381b38 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -14,7 +14,6 @@ use rope::Rope; use schemars::JsonSchema; use serde::Deserialize; use smol::io::{AsyncBufReadExt, AsyncReadExt, BufReader}; -use std::borrow::Cow; use std::ffi::{OsStr, OsString}; use std::process::{ExitStatus, Stdio}; use std::{ @@ -848,7 +847,7 @@ impl GitRepository for RealGitRepository { } files.push(CommitFile { - path: rel_path.into(), + path: RepoPath(Arc::from(rel_path)), old_text, new_text, }) @@ -2049,6 +2048,11 @@ fn git_status_args(path_prefixes: &[RepoPath]) -> Vec { OsString::from("--no-renames"), OsString::from("-z"), ]; + args.extend( + path_prefixes + .iter() + .map(|path_prefix| path_prefix.as_std_path().into()), + ); args.extend(path_prefixes.iter().map(|path_prefix| { if path_prefix.is_empty() { Path::new(".").into() @@ -2304,52 +2308,54 @@ async fn run_askpass_command( } } -#[derive(Clone, Debug, Ord, Hash, PartialOrd, Eq, PartialEq)] -pub struct RepoPath(pub Arc); +#[derive(Clone, Ord, Hash, PartialOrd, Eq, PartialEq)] +pub struct RepoPath(Arc); + +impl std::fmt::Debug for RepoPath { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} impl RepoPath { pub fn new + ?Sized>(s: &S) -> Result { let rel_path = RelPath::unix(s.as_ref())?; - Ok(rel_path.into()) - } - - pub fn from_proto(proto: &str) -> Result { - let rel_path = RelPath::from_proto(proto)?; - Ok(rel_path.into()) + Ok(Self::from_rel_path(rel_path)) } pub fn from_std_path(path: &Path, path_style: PathStyle) -> Result { let rel_path = RelPath::new(path, path_style)?; - Ok(Self(rel_path.as_ref().into())) + Ok(Self::from_rel_path(&rel_path)) } -} -#[cfg(any(test, feature = "test-support"))] -pub fn repo_path + ?Sized>(s: &S) -> RepoPath { - RepoPath(RelPath::unix(s.as_ref()).unwrap().into()) -} + pub fn from_proto(proto: &str) -> Result { + let rel_path = RelPath::from_proto(proto)?; + Ok(Self(rel_path)) + } -impl From<&RelPath> for RepoPath { - fn from(value: &RelPath) -> Self { - RepoPath(value.into()) + pub fn from_rel_path(path: &RelPath) -> RepoPath { + Self(Arc::from(path)) } -} -impl<'a> From> for RepoPath { - fn from(value: Cow<'a, RelPath>) -> Self { - value.as_ref().into() + pub fn as_std_path(&self) -> &Path { + // git2 does not like empty paths and our RelPath infra turns `.` into `` + // so undo that here + if self.is_empty() { + Path::new(".") + } else { + self.0.as_std_path() + } } } -impl From> for RepoPath { - fn from(value: Arc) -> Self { - RepoPath(value) - } +#[cfg(any(test, feature = "test-support"))] +pub fn repo_path + ?Sized>(s: &S) -> RepoPath { + RepoPath(RelPath::unix(s.as_ref()).unwrap().into()) } -impl Default for RepoPath { - fn default() -> Self { - RepoPath(RelPath::empty().into()) +impl AsRef> for RepoPath { + fn as_ref(&self) -> &Arc { + &self.0 } } @@ -2361,12 +2367,6 @@ impl std::ops::Deref for RepoPath { } } -// impl AsRef for RepoPath { -// fn as_ref(&self) -> &Path { -// RelPath::as_ref(&self.0) -// } -// } - #[derive(Debug)] pub struct RepoPathDescendants<'a>(pub &'a RepoPath); diff --git a/crates/git/src/status.rs b/crates/git/src/status.rs index a36e24dd3bf0a8c67ee1c70566f4564ba8362616..2cf7cc7c1810620f1cf1aaea831fb337810c83d8 100644 --- a/crates/git/src/status.rs +++ b/crates/git/src/status.rs @@ -454,7 +454,7 @@ impl FromStr for GitStatus { let status = entry.as_bytes()[0..2].try_into().unwrap(); let status = FileStatus::from_bytes(status).log_err()?; // git-status outputs `/`-delimited repo paths, even on Windows. - let path = RepoPath(RelPath::unix(path).log_err()?.into()); + let path = RepoPath::from_rel_path(RelPath::unix(path).log_err()?); Some((path, status)) }) .collect::>(); @@ -539,7 +539,7 @@ impl FromStr for TreeDiff { let mut fields = s.split('\0'); let mut parsed = HashMap::default(); while let Some((status, path)) = fields.next().zip(fields.next()) { - let path = RepoPath(RelPath::unix(path)?.into()); + let path = RepoPath::from_rel_path(RelPath::unix(path)?); let mut fields = status.split(" ").skip(2); let old_sha = fields diff --git a/crates/git_ui/src/commit_view.rs b/crates/git_ui/src/commit_view.rs index 4608397c0a6f2462bcc4bc50e06de99d1749af30..765e1f84a4a3a5b7e257e51df9a9542d0abff067 100644 --- a/crates/git_ui/src/commit_view.rs +++ b/crates/git_ui/src/commit_view.rs @@ -266,7 +266,7 @@ impl language::File for GitBlob { } fn path(&self) -> &Arc { - &self.path.0 + self.path.as_ref() } fn full_path(&self, _: &App) -> PathBuf { diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index aec36e1730b282e94e5dba6847eb55ab464beb00..85cfb3b499f5cc2baefdc23f8e0ffc91f09b620d 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -879,7 +879,7 @@ impl GitPanel { let active_repository = self.active_repository.as_ref()?.downgrade(); cx.spawn(async move |_, cx| { - let file_path_str = repo_path.0.display(PathStyle::Posix); + let file_path_str = repo_path.as_ref().display(PathStyle::Posix); let repo_root = active_repository.read_with(cx, |repository, _| { repository.snapshot().work_directory_abs_path @@ -1074,7 +1074,7 @@ impl GitPanel { } let mut details = entries .iter() - .filter_map(|entry| entry.repo_path.0.file_name()) + .filter_map(|entry| entry.repo_path.as_ref().file_name()) .map(|filename| filename.to_string()) .take(5) .join("\n"); @@ -1129,7 +1129,7 @@ impl GitPanel { .map(|entry| { entry .repo_path - .0 + .as_ref() .file_name() .map(|f| f.to_string()) .unwrap_or_default() @@ -5647,7 +5647,7 @@ mod tests { assert_eq!( entry.status_entry().map(|status| status .repo_path - .0 + .as_ref() .as_std_path() .to_string_lossy() .to_string()), diff --git a/crates/git_ui/src/project_diff.rs b/crates/git_ui/src/project_diff.rs index 305e30d9d7a49d30aad09863e04dc11641f3017e..6f8195c8b718640de4fed421253d5f1bd2f8f14e 100644 --- a/crates/git_ui/src/project_diff.rs +++ b/crates/git_ui/src/project_diff.rs @@ -336,7 +336,7 @@ impl ProjectDiff { }; let repo = git_repo.read(cx); 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); + let path_key = PathKey::with_sort_prefix(sort_prefix, entry.repo_path.as_ref().clone()); self.move_to_path(path_key, window, cx) } @@ -566,7 +566,7 @@ impl ProjectDiff { for entry in buffers_to_load.iter() { let sort_prefix = sort_prefix(&repo, &entry.repo_path, entry.file_status, cx); let path_key = - PathKey::with_sort_prefix(sort_prefix, entry.repo_path.0.clone()); + PathKey::with_sort_prefix(sort_prefix, entry.repo_path.as_ref().clone()); previous_paths.remove(&path_key); path_keys.push(path_key) } diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index b4b353faadcaa359f85d263e63d4e370aaec1e4a..5fcf28aff3554149ece954074f312e0fe37a9208 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -227,7 +227,7 @@ impl sum_tree::Item for StatusEntry { fn summary(&self, _: ::Context<'_>) -> Self::Summary { PathSummary { - max_path: self.repo_path.0.clone(), + max_path: self.repo_path.as_ref().clone(), item_summary: self.status.summary(), } } @@ -237,7 +237,7 @@ impl sum_tree::KeyedItem for StatusEntry { type Key = PathKey; fn key(&self) -> Self::Key { - PathKey(self.repo_path.0.clone()) + PathKey(self.repo_path.as_ref().clone()) } } @@ -990,7 +990,7 @@ impl GitStore { RepositoryState::Local { backend, .. } => backend .blame(repo_path.clone(), content) .await - .with_context(|| format!("Failed to blame {:?}", repo_path.0)) + .with_context(|| format!("Failed to blame {:?}", repo_path.as_ref())) .map(Some), RepositoryState::Remote { project_id, client } => { let response = client @@ -2376,7 +2376,7 @@ impl GitStore { .entries .into_iter() .map(|(path, status)| proto::TreeDiffStatus { - path: path.0.to_proto(), + path: path.as_ref().to_proto(), status: match status { TreeDiffStatus::Added {} => proto::tree_diff_status::Status::Added.into(), TreeDiffStatus::Modified { .. } => { @@ -3152,13 +3152,13 @@ impl RepositorySnapshot { pub fn status_for_path(&self, path: &RepoPath) -> Option { self.statuses_by_path - .get(&PathKey(path.0.clone()), ()) + .get(&PathKey(path.as_ref().clone()), ()) .cloned() } pub fn pending_ops_for_path(&self, path: &RepoPath) -> Option { self.pending_ops_by_path - .get(&PathKey(path.0.clone()), ()) + .get(&PathKey(path.as_ref().clone()), ()) .cloned() } @@ -4727,7 +4727,9 @@ impl Repository { } }; Some(( - RepoPath(RelPath::from_proto(&entry.path).log_err()?), + RepoPath::from_rel_path( + &RelPath::from_proto(&entry.path).log_err()?, + ), status, )) }) @@ -5289,7 +5291,8 @@ impl Repository { let mut cursor = prev_statuses.cursor::(()); for path in changed_paths.into_iter() { if cursor.seek_forward(&PathTarget::Path(&path), Bias::Left) { - changed_path_statuses.push(Edit::Remove(PathKey(path.0))); + changed_path_statuses + .push(Edit::Remove(PathKey(path.as_ref().clone()))); } } changed_path_statuses @@ -5435,10 +5438,8 @@ fn get_permalink_in_rust_registry_src( remote, BuildPermalinkParams::new( &cargo_vcs_info.git.sha1, - &RepoPath( - RelPath::new(&path, PathStyle::local()) - .context("invalid path")? - .into_arc(), + &RepoPath::from_rel_path( + &RelPath::new(&path, PathStyle::local()).context("invalid path")?, ), Some(selection), ), @@ -5640,7 +5641,11 @@ async fn compute_snapshot( let mut events = Vec::new(); let branches = backend.branches().await?; let branch = branches.into_iter().find(|branch| branch.is_head); - let statuses = backend.status(&[RelPath::empty().into()]).await?; + let statuses = backend + .status(&[RepoPath::from_rel_path( + &RelPath::new(".".as_ref(), PathStyle::local()).unwrap(), + )]) + .await?; let stash_entries = backend.stash_entries().await?; let statuses_by_path = SumTree::from_iter( statuses diff --git a/crates/project/src/git_store/conflict_set.rs b/crates/project/src/git_store/conflict_set.rs index 7d99571b5b88d7f5d37d56d47ff32a71fd5a29ff..bd80214c2c0b6d5b1a5da3ba497c5670cb26cb93 100644 --- a/crates/project/src/git_store/conflict_set.rs +++ b/crates/project/src/git_store/conflict_set.rs @@ -264,7 +264,7 @@ mod tests { use super::*; use fs::FakeFs; use git::{ - repository::repo_path, + repository::{RepoPath, repo_path}, status::{UnmergedStatus, UnmergedStatusCode}, }; use gpui::{BackgroundExecutor, TestAppContext}; @@ -617,7 +617,7 @@ mod tests { cx.run_until_parked(); fs.with_git_state(path!("/project/.git").as_ref(), true, |state| { state.unmerged_paths.insert( - rel_path("a.txt").into(), + RepoPath::from_rel_path(rel_path("a.txt")), UnmergedStatus { first_head: UnmergedStatusCode::Updated, second_head: UnmergedStatusCode::Updated, diff --git a/crates/project/src/git_store/pending_op.rs b/crates/project/src/git_store/pending_op.rs index fd1b35035a8e334acdd244d8e663212f39bc383e..1991eed407833d47fd35f6f573fbb46c692aed91 100644 --- a/crates/project/src/git_store/pending_op.rs +++ b/crates/project/src/git_store/pending_op.rs @@ -46,7 +46,7 @@ impl Item for PendingOps { fn summary(&self, _cx: ()) -> Self::Summary { PathSummary { - max_path: self.repo_path.0.clone(), + max_path: self.repo_path.as_ref().clone(), item_summary: PendingOpsSummary { staged_count: self.staged() as usize, staging_count: self.staging() as usize, @@ -73,7 +73,7 @@ impl KeyedItem for PendingOps { type Key = PathKey; fn key(&self) -> Self::Key { - PathKey(self.repo_path.0.clone()) + PathKey(self.repo_path.as_ref().clone()) } } diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index 1f76b905be2843605b32918e6d3bf1a037ced636..ad2c339d22fdd49d6565ff5be491749cfcac7830 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -7937,7 +7937,7 @@ async fn test_staging_random_hunks( log::info!( "index text:\n{}", - repo.load_index_text(rel_path("file.txt").into()) + repo.load_index_text(RepoPath::from_rel_path(rel_path("file.txt"))) .await .unwrap() ); @@ -8523,7 +8523,7 @@ async fn test_repository_pending_ops_staging( assert_eq!( pending_ops_all .lock() - .get(&worktree::PathKey(repo_path("a.txt").0), ()) + .get(&worktree::PathKey(repo_path("a.txt").as_ref().clone()), ()) .unwrap() .ops, vec![ @@ -8644,7 +8644,7 @@ async fn test_repository_pending_ops_long_running_staging( assert_eq!( pending_ops_all .lock() - .get(&worktree::PathKey(repo_path("a.txt").0), ()) + .get(&worktree::PathKey(repo_path("a.txt").as_ref().clone()), ()) .unwrap() .ops, vec![ @@ -8752,7 +8752,7 @@ async fn test_repository_pending_ops_stage_all( assert_eq!( pending_ops_all .lock() - .get(&worktree::PathKey(repo_path("a.txt").0), ()) + .get(&worktree::PathKey(repo_path("a.txt").as_ref().clone()), ()) .unwrap() .ops, vec![ @@ -8771,7 +8771,7 @@ async fn test_repository_pending_ops_stage_all( assert_eq!( pending_ops_all .lock() - .get(&worktree::PathKey(repo_path("b.txt").0), ()) + .get(&worktree::PathKey(repo_path("b.txt").as_ref().clone()), ()) .unwrap() .ops, vec![ @@ -9309,11 +9309,9 @@ async fn test_file_status(cx: &mut gpui::TestAppContext) { repository.read_with(cx, |repository, _cx| { assert_eq!( repository - .status_for_path( - &rel_path(renamed_dir_name) - .join(rel_path(RENAMED_FILE)) - .into() - ) + .status_for_path(&RepoPath::from_rel_path( + &rel_path(renamed_dir_name).join(rel_path(RENAMED_FILE)) + )) .unwrap() .status, FileStatus::Untracked, @@ -9337,11 +9335,9 @@ async fn test_file_status(cx: &mut gpui::TestAppContext) { repository.read_with(cx, |repository, _cx| { assert_eq!( repository - .status_for_path( - &rel_path(renamed_dir_name) - .join(rel_path(RENAMED_FILE)) - .into() - ) + .status_for_path(&RepoPath::from_rel_path( + &rel_path(renamed_dir_name).join(rel_path(RENAMED_FILE)) + )) .unwrap() .status, FileStatus::Untracked,