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

@@ -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<SearchResult>, 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"), "");

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 {

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<dyn Fs>,
-        snapshot: &worktree::Snapshot,
-        path: &Path,
-        query: &SearchQuery,
-        filter_tx: &Sender<MatchingEntry>,
-        output_tx: &Sender<oneshot::Receiver<ProjectPath>>,
-    ) -> 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<dyn Fs>,
+        snapshot: &'a worktree::Snapshot,
+        path: &'a Path,
+        query: &'a SearchQuery,
+        include_root: bool,
+        filter_tx: &'a Sender<MatchingEntry>,
+        output_tx: &'a Sender<oneshot::Receiver<ProjectPath>>,
+    ) -> 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())?),
-                        },
-                    })
-                    .await?;
+                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;