From 8643b11f57bef595fbe935ea3e76cce4b6ffeee3 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Tue, 27 Aug 2024 13:46:36 -0600 Subject: [PATCH] Fix search sorting (#16970) Ensures we sort paths in search the same as we do in panels. Ideally we'd store things like this in the worktree, but the sort order depends on file vs directory, and callers generally don't know which they're asking for. Release Notes: - N/A --- crates/project/src/project_tests.rs | 56 ++++++++++- crates/project/src/search.rs | 27 +++--- crates/project/src/worktree_store.rs | 137 +++++++++++++++++---------- 3 files changed, 151 insertions(+), 69 deletions(-) diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index 783bac388518955dbd71ebb6510f4a9fcd11b4aa..5280234beb0d7a363b6269de8391fecd9b23c84b 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -4495,7 +4495,7 @@ async fn test_search_in_gitignored_dirs(cx: &mut gpui::TestAppContext) { "Unrestricted search with ignored directories should find every file with the query" ); - let files_to_include = PathMatcher::new(&["/dir/node_modules/prettier/**".to_owned()]).unwrap(); + let files_to_include = PathMatcher::new(&["node_modules/prettier/**".to_owned()]).unwrap(); let files_to_exclude = PathMatcher::new(&["*.ts".to_owned()]).unwrap(); let project = Project::test(fs.clone(), ["/dir".as_ref()], cx).await; assert_eq!( @@ -4522,6 +4522,60 @@ async fn test_search_in_gitignored_dirs(cx: &mut gpui::TestAppContext) { ); } +#[gpui::test] +async fn test_search_ordering(cx: &mut gpui::TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + "/dir", + json!({ + ".git": {}, + ".gitignore": "**/target\n/node_modules\n", + "aaa.txt": "key:value", + "bbb": { + "index.txt": "index_key:index_value" + }, + "node_modules": { + "10 eleven": "key", + "1 two": "key" + }, + }), + ) + .await; + let project = Project::test(fs.clone(), ["/dir".as_ref()], cx).await; + + let mut search = project.update(cx, |project, cx| { + project.search( + SearchQuery::text( + "key", + false, + false, + true, + Default::default(), + Default::default(), + ) + .unwrap(), + cx, + ) + }); + + fn file_name(search_result: Option, cx: &mut gpui::TestAppContext) -> String { + match search_result.unwrap() { + SearchResult::Buffer { buffer, .. } => buffer.read_with(cx, |buffer, _| { + buffer.file().unwrap().path().to_string_lossy().to_string() + }), + _ => panic!("Expected buffer"), + } + } + + assert_eq!(file_name(search.next().await, cx), "bbb/index.txt"); + assert_eq!(file_name(search.next().await, cx), "node_modules/1 two"); + assert_eq!(file_name(search.next().await, cx), "node_modules/10 eleven"); + assert_eq!(file_name(search.next().await, cx), "aaa.txt"); + assert!(search.next().await.is_none()) +} + #[test] fn test_glob_literal_prefix() { assert_eq!(glob_literal_prefix("**/*.js"), ""); diff --git a/crates/project/src/search.rs b/crates/project/src/search.rs index e28781fdb57abf48686cc4ae7da460b9374f31aa..d545d8d5747cb1a265ae9273a761a0a5c3d46985 100644 --- a/crates/project/src/search.rs +++ b/crates/project/src/search.rs @@ -425,23 +425,18 @@ impl SearchQuery { && self.files_to_include().sources().is_empty()) } - pub fn file_matches(&self, file_path: Option<&Path>) -> bool { - match file_path { - Some(file_path) => { - let mut path = file_path.to_path_buf(); - loop { - if self.files_to_exclude().is_match(&path) { - return false; - } else if self.files_to_include().sources().is_empty() - || self.files_to_include().is_match(&path) - { - return true; - } else if !path.pop() { - return false; - } - } + pub fn file_matches(&self, file_path: &Path) -> bool { + let mut path = file_path.to_path_buf(); + loop { + if self.files_to_exclude().is_match(&path) { + return false; + } else if self.files_to_include().sources().is_empty() + || self.files_to_include().is_match(&path) + { + return true; + } else if !path.pop() { + return false; } - None => self.files_to_include().sources().is_empty(), } } pub fn as_inner(&self) -> &SearchInputs { diff --git a/crates/project/src/worktree_store.rs b/crates/project/src/worktree_store.rs index 73ead64455707ecaaa2dac7deb939dab1dcc33fa..56a054cd975317601766f7d373761987418b0712 100644 --- a/crates/project/src/worktree_store.rs +++ b/crates/project/src/worktree_store.rs @@ -1,5 +1,4 @@ use std::{ - collections::VecDeque, path::{Path, PathBuf}, sync::Arc, }; @@ -7,7 +6,7 @@ use std::{ use anyhow::{anyhow, Context as _, Result}; use collections::{HashMap, HashSet}; use fs::Fs; -use futures::SinkExt; +use futures::{future::BoxFuture, SinkExt}; use gpui::{AppContext, AsyncAppContext, EntityId, EventEmitter, Model, ModelContext, WeakModel}; use postage::oneshot; use rpc::{ @@ -16,10 +15,11 @@ use rpc::{ }; use smol::{ channel::{Receiver, Sender}, + future::FutureExt, stream::StreamExt, }; use text::ReplicaId; -use util::ResultExt; +use util::{paths::compare_paths, ResultExt}; use worktree::{Entry, ProjectEntryId, Worktree, WorktreeId, WorktreeSettings}; use crate::{search::SearchQuery, ProjectPath}; @@ -351,61 +351,91 @@ impl WorktreeStore { return matching_paths_rx; } - async fn scan_ignored_dir( - fs: &Arc, - snapshot: &worktree::Snapshot, - path: &Path, - query: &SearchQuery, - filter_tx: &Sender, - output_tx: &Sender>, - ) -> Result<()> { - let mut ignored_paths_to_process = VecDeque::from([snapshot.abs_path().join(&path)]); - - while let Some(ignored_abs_path) = ignored_paths_to_process.pop_front() { - let metadata = fs - .metadata(&ignored_abs_path) + fn scan_ignored_dir<'a>( + fs: &'a Arc, + snapshot: &'a worktree::Snapshot, + path: &'a Path, + query: &'a SearchQuery, + include_root: bool, + filter_tx: &'a Sender, + output_tx: &'a Sender>, + ) -> BoxFuture<'a, Result<()>> { + async move { + let abs_path = snapshot.abs_path().join(&path); + let Some(mut files) = fs + .read_dir(&abs_path) .await - .with_context(|| format!("fetching fs metadata for {ignored_abs_path:?}")) + .with_context(|| format!("listing ignored path {abs_path:?}")) .log_err() - .flatten(); - - let Some(fs_metadata) = metadata else { - continue; + else { + return Ok(()); }; - if fs_metadata.is_dir { - let files = fs - .read_dir(&ignored_abs_path) - .await - .with_context(|| format!("listing ignored path {ignored_abs_path:?}")) - .log_err(); - if let Some(mut subfiles) = files { - while let Some(subfile) = subfiles.next().await { - if let Some(subfile) = subfile.log_err() { - ignored_paths_to_process.push_back(subfile); - } - } - } - } else if !fs_metadata.is_symlink { - if !query.file_matches(Some(&ignored_abs_path)) { + let mut results = Vec::new(); + + while let Some(Ok(file)) = files.next().await { + let Some(metadata) = fs + .metadata(&file) + .await + .with_context(|| format!("fetching fs metadata for {abs_path:?}")) + .log_err() + .flatten() + else { + continue; + }; + if metadata.is_symlink || metadata.is_fifo { continue; } - - let (tx, rx) = oneshot::channel(); - output_tx.send(rx).await?; - filter_tx - .send(MatchingEntry { - respond: tx, - worktree_path: snapshot.abs_path().clone(), - path: ProjectPath { - worktree_id: snapshot.id(), - path: Arc::from(ignored_abs_path.strip_prefix(snapshot.abs_path())?), - }, - }) + results.push(( + file.strip_prefix(snapshot.abs_path())?.to_path_buf(), + !metadata.is_dir, + )) + } + results.sort_by(|(a_path, a_is_file), (b_path, b_is_file)| { + compare_paths((a_path, *a_is_file), (b_path, *b_is_file)) + }); + for (path, is_file) in results { + if is_file { + if query.filters_path() { + let matched_path = if include_root { + let mut full_path = PathBuf::from(snapshot.root_name()); + full_path.push(&path); + query.file_matches(&full_path) + } else { + query.file_matches(&path) + }; + if !matched_path { + continue; + } + } + let (tx, rx) = oneshot::channel(); + output_tx.send(rx).await?; + filter_tx + .send(MatchingEntry { + respond: tx, + worktree_path: snapshot.abs_path().clone(), + path: ProjectPath { + worktree_id: snapshot.id(), + path: Arc::from(path), + }, + }) + .await?; + } else { + Self::scan_ignored_dir( + fs, + snapshot, + &path, + query, + include_root, + filter_tx, + output_tx, + ) .await?; + } } + Ok(()) } - Ok(()) + .boxed() } async fn find_candidate_paths( @@ -418,7 +448,9 @@ impl WorktreeStore { ) -> Result<()> { let include_root = snapshots.len() > 1; for (snapshot, settings) in snapshots { - for entry in snapshot.entries(query.include_ignored(), 0) { + let mut entries: Vec<_> = snapshot.entries(query.include_ignored(), 0).collect(); + entries.sort_by(|a, b| compare_paths((&a.path, a.is_file()), (&b.path, b.is_file()))); + for entry in entries { if entry.is_dir() && entry.is_ignored { if !settings.is_path_excluded(&entry.path) { Self::scan_ignored_dir( @@ -426,6 +458,7 @@ impl WorktreeStore { &snapshot, &entry.path, &query, + include_root, &filter_tx, &output_tx, ) @@ -453,9 +486,9 @@ impl WorktreeStore { let matched_path = if include_root { let mut full_path = PathBuf::from(snapshot.root_name()); full_path.push(&entry.path); - query.file_matches(Some(&full_path)) + query.file_matches(&full_path) } else { - query.file_matches(Some(&entry.path)) + query.file_matches(&entry.path) }; if !matched_path { continue;