file_finder: Respect .gitignore and file_scan_inclusions with ** in glob (#40654)

Coenen Benjamin and Julia Ryan created 3 weeks ago

Closes #39037 

Previously, the code split the `**/.env` glob in `file_scan_inclusions`
into two sources for the `PathMatcher`: `["**", "**/.env"]`. This
approach works for directories, but including `**` will match all
directories and their files. To address this, I now select the
appropriate `PathMatcher` using only `**/.env` when specifically
targeting a file to determine whether to include it in the file finder.

Release Notes:

- Fixed: respect `.gitignore` and `file_scan_inclusions` settings with
`**` in glob for file finder

---------

Signed-off-by: Benjamin <[e-mail]>
Co-authored-by: Julia Ryan <[e-mail]>

Change summary

crates/file_finder/src/file_finder_tests.rs | 142 +++++++++++++++++++++++
crates/util/src/paths.rs                    |  15 ++
crates/worktree/src/worktree.rs             |  17 +
crates/worktree/src/worktree_settings.rs    |  19 ++
crates/worktree/src/worktree_tests.rs       |   1 
5 files changed, 183 insertions(+), 11 deletions(-)

Detailed changes

crates/file_finder/src/file_finder_tests.rs

@@ -7,6 +7,7 @@ use menu::{Confirm, SelectNext, SelectPrevious};
 use pretty_assertions::{assert_eq, assert_matches};
 use project::{FS_WATCH_LATENCY, RemoveOptions};
 use serde_json::json;
+use settings::SettingsStore;
 use util::{path, rel_path::rel_path};
 use workspace::{AppState, CloseActiveItem, OpenOptions, ToggleFileFinder, Workspace, open_paths};
 
@@ -658,6 +659,147 @@ async fn test_matching_cancellation(cx: &mut TestAppContext) {
     });
 }
 
+#[gpui::test]
+async fn test_ignored_root_with_file_inclusions(cx: &mut TestAppContext) {
+    let app_state = init_test(cx);
+    cx.update(|cx| {
+        cx.update_global::<SettingsStore, _>(|store, cx| {
+            store.update_user_settings(cx, |settings| {
+                settings.project.worktree.file_scan_inclusions = Some(vec![
+                    "height_demo/**/hi_bonjour".to_string(),
+                    "**/height_1".to_string(),
+                ]);
+            });
+        })
+    });
+    app_state
+        .fs
+        .as_fake()
+        .insert_tree(
+            "/ancestor",
+            json!({
+                ".gitignore": "ignored-root",
+                "ignored-root": {
+                    "happiness": "",
+                    "height": "",
+                    "hi": "",
+                    "hiccup": "",
+                },
+                "tracked-root": {
+                    ".gitignore": "height*",
+                    "happiness": "",
+                    "height": "",
+                    "heights": {
+                        "height_1": "",
+                        "height_2": "",
+                    },
+                    "height_demo": {
+                        "test_1": {
+                            "hi_bonjour": "hi_bonjour",
+                            "hi": "hello",
+                        },
+                        "hihi": "bye",
+                        "test_2": {
+                            "hoi": "nl"
+                        }
+                    },
+                    "height_include": {
+                        "height_1_include": "",
+                        "height_2_include": "",
+                    },
+                    "hi": "",
+                    "hiccup": "",
+                },
+            }),
+        )
+        .await;
+
+    let project = Project::test(
+        app_state.fs.clone(),
+        [
+            Path::new(path!("/ancestor/tracked-root")),
+            Path::new(path!("/ancestor/ignored-root")),
+        ],
+        cx,
+    )
+    .await;
+    let (picker, _workspace, cx) = build_find_picker(project, cx);
+
+    picker
+        .update_in(cx, |picker, window, cx| {
+            picker
+                .delegate
+                .spawn_search(test_path_position("hi"), window, cx)
+        })
+        .await;
+    picker.update(cx, |picker, _| {
+        let matches = collect_search_matches(picker);
+        assert_eq!(matches.history.len(), 0);
+        assert_eq!(
+            matches.search,
+            vec![
+                rel_path("ignored-root/hi").into(),
+                rel_path("tracked-root/hi").into(),
+                rel_path("ignored-root/hiccup").into(),
+                rel_path("tracked-root/hiccup").into(),
+                rel_path("tracked-root/height_demo/test_1/hi_bonjour").into(),
+                rel_path("ignored-root/height").into(),
+                rel_path("tracked-root/heights/height_1").into(),
+                rel_path("ignored-root/happiness").into(),
+                rel_path("tracked-root/happiness").into(),
+            ],
+            "All ignored files that were indexed are found for default ignored mode"
+        );
+    });
+}
+
+#[gpui::test]
+async fn test_ignored_root_with_file_inclusions_repro(cx: &mut TestAppContext) {
+    let app_state = init_test(cx);
+    cx.update(|cx| {
+        cx.update_global::<SettingsStore, _>(|store, cx| {
+            store.update_user_settings(cx, |settings| {
+                settings.project.worktree.file_scan_inclusions = Some(vec!["**/.env".to_string()]);
+            });
+        })
+    });
+    app_state
+        .fs
+        .as_fake()
+        .insert_tree(
+            "/src",
+            json!({
+                ".gitignore": "node_modules",
+                "node_modules": {
+                    "package.json": "// package.json",
+                    ".env": "BAR=FOO"
+                },
+                ".env": "FOO=BAR"
+            }),
+        )
+        .await;
+
+    let project = Project::test(app_state.fs.clone(), [Path::new(path!("/src"))], cx).await;
+    let (picker, _workspace, cx) = build_find_picker(project, cx);
+
+    picker
+        .update_in(cx, |picker, window, cx| {
+            picker
+                .delegate
+                .spawn_search(test_path_position("json"), window, cx)
+        })
+        .await;
+    picker.update(cx, |picker, _| {
+        let matches = collect_search_matches(picker);
+        assert_eq!(matches.history.len(), 0);
+        assert_eq!(
+            matches.search,
+            vec![],
+            "All ignored files that were indexed are found for default ignored mode"
+        );
+    });
+}
+
 #[gpui::test]
 async fn test_ignored_root(cx: &mut TestAppContext) {
     let app_state = init_test(cx);

crates/util/src/paths.rs

@@ -1573,6 +1573,21 @@ mod tests {
         );
     }
 
+    #[perf]
+    fn file_in_dirs() {
+        let path = Path::new("/work/.env");
+        let path_matcher = PathMatcher::new(&["**/.env".to_owned()], PathStyle::Posix).unwrap();
+        assert!(
+            path_matcher.is_match(path),
+            "Path matcher should match {path:?}"
+        );
+        let path = Path::new("/work/package.json");
+        assert!(
+            !path_matcher.is_match(path),
+            "Path matcher should not match {path:?}"
+        );
+    }
+
     #[perf]
     fn project_search() {
         let path = Path::new("/Users/someonetoignore/work/zed/zed.dev/node_modules");

crates/worktree/src/worktree.rs

@@ -505,7 +505,7 @@ impl Worktree {
                 project_id,
                 replica_id,
                 snapshot,
-                file_scan_inclusions: settings.file_scan_inclusions.clone(),
+                file_scan_inclusions: settings.parent_dir_scan_inclusions.clone(),
                 background_snapshot: background_snapshot.clone(),
                 updates_tx: Some(background_updates_tx),
                 update_observer: None,
@@ -520,8 +520,10 @@ impl Worktree {
                 while let Some(update) = background_updates_rx.next().await {
                     {
                         let mut lock = background_snapshot.lock();
-                        lock.0
-                            .apply_remote_update(update.clone(), &settings.file_scan_inclusions);
+                        lock.0.apply_remote_update(
+                            update.clone(),
+                            &settings.parent_dir_scan_inclusions,
+                        );
                         lock.1.push(update);
                     }
                     snapshot_updated_tx.send(()).await.ok();
@@ -4290,7 +4292,8 @@ impl BackgroundScanner {
 
             if child_entry.is_dir() {
                 child_entry.is_ignored = ignore_stack.is_abs_path_ignored(&child_abs_path, true);
-                child_entry.is_always_included = self.settings.is_path_always_included(&child_path);
+                child_entry.is_always_included =
+                    self.settings.is_path_always_included(&child_path, true);
 
                 // Avoid recursing until crash in the case of a recursive symlink
                 if job.ancestor_inodes.contains(&child_entry.inode) {
@@ -4315,7 +4318,8 @@ impl BackgroundScanner {
                 }
             } else {
                 child_entry.is_ignored = ignore_stack.is_abs_path_ignored(&child_abs_path, false);
-                child_entry.is_always_included = self.settings.is_path_always_included(&child_path);
+                child_entry.is_always_included =
+                    self.settings.is_path_always_included(&child_path, false);
             }
 
             {
@@ -4450,7 +4454,8 @@ impl BackgroundScanner {
                     fs_entry.is_ignored = ignore_stack.is_abs_path_ignored(&abs_path, is_dir);
                     fs_entry.is_external = is_external;
                     fs_entry.is_private = self.is_path_private(path);
-                    fs_entry.is_always_included = self.settings.is_path_always_included(path);
+                    fs_entry.is_always_included =
+                        self.settings.is_path_always_included(path, is_dir);
 
                     let parent_is_hidden = path
                         .parent()

crates/worktree/src/worktree_settings.rs

@@ -13,8 +13,11 @@ pub struct WorktreeSettings {
     pub project_name: Option<String>,
     /// Whether to prevent this project from being shared in public channels.
     pub prevent_sharing_in_public_channels: bool,
-    pub file_scan_inclusions: PathMatcher,
     pub file_scan_exclusions: PathMatcher,
+    pub file_scan_inclusions: PathMatcher,
+    /// This field contains all ancestors of the `file_scan_inclusions`. It's used to
+    /// determine whether to terminate worktree scanning for a given dir.
+    pub parent_dir_scan_inclusions: PathMatcher,
     pub private_files: PathMatcher,
 }
 
@@ -29,9 +32,12 @@ impl WorktreeSettings {
             .any(|ancestor| self.file_scan_exclusions.is_match(ancestor.as_std_path()))
     }
 
-    pub fn is_path_always_included(&self, path: &RelPath) -> bool {
-        path.ancestors()
-            .any(|ancestor| self.file_scan_inclusions.is_match(ancestor.as_std_path()))
+    pub fn is_path_always_included(&self, path: &RelPath, is_dir: bool) -> bool {
+        if is_dir {
+            self.parent_dir_scan_inclusions.is_match(path.as_std_path())
+        } else {
+            self.file_scan_inclusions.is_match(path.as_std_path())
+        }
     }
 }
 
@@ -46,6 +52,7 @@ impl Settings for WorktreeSettings {
             .flat_map(|glob| {
                 Path::new(glob)
                     .ancestors()
+                    .skip(1)
                     .map(|a| a.to_string_lossy().into())
             })
             .filter(|p: &String| !p.is_empty())
@@ -57,11 +64,13 @@ impl Settings for WorktreeSettings {
             file_scan_exclusions: path_matchers(file_scan_exclusions, "file_scan_exclusions")
                 .log_err()
                 .unwrap_or_default(),
-            file_scan_inclusions: path_matchers(
+            parent_dir_scan_inclusions: path_matchers(
                 parsed_file_scan_inclusions,
                 "file_scan_inclusions",
             )
             .unwrap(),
+            file_scan_inclusions: path_matchers(file_scan_inclusions, "file_scan_inclusions")
+                .unwrap(),
             private_files: path_matchers(private_files, "private_files")
                 .log_err()
                 .unwrap_or_default(),

crates/worktree/src/worktree_tests.rs

@@ -760,6 +760,7 @@ async fn test_file_scan_inclusions(cx: &mut TestAppContext) {
             "prettier": {
                 "package.json": "{}",
             },
+            "package.json": "//package.json"
         },
         "src": {
             ".DS_Store": "",