Fix more bugs in files (#16241)

Fixes:
- [x] an issue where directories would only match by prefix, causing
both a directory and a file to be matched if in the same directory
- [x] An issue where you could not continue a file completion when
selecting a directory, as `tab` on a file would always run the command.
This effectively disabled directory sub queries.
- [x] Inconsistent rendering of files and directories in the slash
command

Release Notes:

- N/A

---------

Co-authored-by: max <max@zed.dev>

Cargo.lock

@@ -367,6 +367,7 @@ dependencies = [
  "fs",
  "futures 0.3.30",
  "fuzzy",
+ "globset",
  "gpui",
  "handlebars",
  "heed",

crates/assistant/Cargo.toml

@@ -39,6 +39,7 @@ feature_flags.workspace = true
 fs.workspace = true
 futures.workspace = true
 fuzzy.workspace = true
+globset.workspace = true
 gpui.workspace = true
 handlebars.workspace = true
 heed.workspace = true

crates/assistant/src/slash_command.rs

@@ -1,5 +1,6 @@
 use crate::assistant_panel::ContextEditor;
 use anyhow::Result;
+use assistant_slash_command::AfterCompletion;
 pub use assistant_slash_command::{SlashCommand, SlashCommandOutput, SlashCommandRegistry};
 use editor::{CompletionProvider, Editor};
 use fuzzy::{match_strings, StringMatchCandidate};
@@ -197,7 +198,9 @@ impl SlashCommandCompletionProvider {
                                         let command_range = command_range.clone();
                                         let command_name = command_name.clone();
                                         move |intent: CompletionIntent, cx: &mut WindowContext| {
-                                            if new_argument.run_command || intent.is_complete() {
+                                            if new_argument.after_completion.run()
+                                                || intent.is_complete()
+                                            {
                                                 editor
                                                     .update(cx, |editor, cx| {
                                                         editor.run_command(
@@ -212,14 +215,14 @@ impl SlashCommandCompletionProvider {
                                                     .ok();
                                                 false
                                             } else {
-                                                !new_argument.run_command
+                                                !new_argument.after_completion.run()
                                             }
                                         }
                                     }) as Arc<_>
                                 });
 
                         let mut new_text = new_argument.new_text.clone();
-                        if !new_argument.run_command {
+                        if new_argument.after_completion == AfterCompletion::Continue {
                             new_text.push(' ');
                         }
 

crates/assistant/src/slash_command/diagnostics_command.rs

@@ -153,7 +153,7 @@ impl SlashCommand for DiagnosticsSlashCommand {
                 .map(|completion| ArgumentCompletion {
                     label: completion.clone().into(),
                     new_text: completion,
-                    run_command: true,
+                    after_completion: assistant_slash_command::AfterCompletion::Run,
                     replace_previous_arguments: false,
                 })
                 .collect())

crates/assistant/src/slash_command/docs_command.rs

@@ -181,7 +181,7 @@ impl SlashCommand for DocsSlashCommand {
                     .map(|item| ArgumentCompletion {
                         label: item.clone().into(),
                         new_text: item.to_string(),
-                        run_command: true,
+                        after_completion: assistant_slash_command::AfterCompletion::Run,
                         replace_previous_arguments: false,
                     })
                     .collect()
@@ -194,7 +194,7 @@ impl SlashCommand for DocsSlashCommand {
                         return Ok(vec![ArgumentCompletion {
                             label: "No available docs providers.".into(),
                             new_text: String::new(),
-                            run_command: false,
+                            after_completion: false.into(),
                             replace_previous_arguments: false,
                         }]);
                     }
@@ -204,7 +204,7 @@ impl SlashCommand for DocsSlashCommand {
                         .map(|provider| ArgumentCompletion {
                             label: provider.to_string().into(),
                             new_text: provider.to_string(),
-                            run_command: false,
+                            after_completion: false.into(),
                             replace_previous_arguments: false,
                         })
                         .collect())
@@ -236,7 +236,7 @@ impl SlashCommand for DocsSlashCommand {
                         .map(|package_name| ArgumentCompletion {
                             label: format!("{package_name} (unindexed)").into(),
                             new_text: format!("{package_name}"),
-                            run_command: true,
+                            after_completion: true.into(),
                             replace_previous_arguments: false,
                         })
                         .collect::<Vec<_>>();
@@ -250,7 +250,7 @@ impl SlashCommand for DocsSlashCommand {
                             )
                             .into(),
                             new_text: provider.to_string(),
-                            run_command: false,
+                            after_completion: false.into(),
                             replace_previous_arguments: false,
                         }]);
                     }

crates/assistant/src/slash_command/file_command.rs

@@ -1,6 +1,6 @@
 use super::{diagnostics_command::write_single_file_diagnostics, SlashCommand, SlashCommandOutput};
 use anyhow::{anyhow, Context as _, Result};
-use assistant_slash_command::{ArgumentCompletion, SlashCommandOutputSection};
+use assistant_slash_command::{AfterCompletion, ArgumentCompletion, SlashCommandOutputSection};
 use fuzzy::PathMatch;
 use gpui::{AppContext, Model, Task, View, WeakView};
 use language::{BufferSnapshot, CodeLabel, HighlightId, LineEnding, LspAdapterDelegate};
@@ -12,7 +12,7 @@ use std::{
     sync::{atomic::AtomicBool, Arc},
 };
 use ui::prelude::*;
-use util::{paths::PathMatcher, ResultExt};
+use util::ResultExt;
 use workspace::Workspace;
 
 pub(crate) struct FileSlashCommand;
@@ -164,7 +164,11 @@ impl SlashCommand for FileSlashCommand {
                     Some(ArgumentCompletion {
                         label,
                         new_text: text,
-                        run_command: true,
+                        after_completion: if path_match.is_dir {
+                            AfterCompletion::Compose
+                        } else {
+                            AfterCompletion::Run
+                        },
                         replace_previous_arguments: false,
                     })
                 })
@@ -190,16 +194,17 @@ impl SlashCommand for FileSlashCommand {
         let task = collect_files(workspace.read(cx).project().clone(), arguments, cx);
 
         cx.foreground_executor().spawn(async move {
-            let (text, ranges) = task.await?;
+            let output = task.await?;
             Ok(SlashCommandOutput {
-                text,
+                text: output.completion_text,
-                sections: ranges
+                sections: output
+                    .files
                     .into_iter()
-                    .map(|(range, path, entry_type)| {
+                    .map(|file| {
                         build_entry_output_section(
-                            range,
+                            file.range_in_text,
-                            Some(&path),
+                            Some(&file.path),
-                            entry_type == EntryType::Directory,
+                            file.entry_type == EntryType::Directory,
                             None,
                         )
                     })
@@ -210,24 +215,37 @@ impl SlashCommand for FileSlashCommand {
     }
 }
 
-#[derive(Clone, Copy, PartialEq)]
+#[derive(Clone, Copy, PartialEq, Debug)]
 enum EntryType {
     File,
     Directory,
 }
 
+#[derive(Clone, PartialEq, Debug)]
+struct FileCommandOutput {
+    completion_text: String,
+    files: Vec<OutputFile>,
+}
+
+#[derive(Clone, PartialEq, Debug)]
+struct OutputFile {
+    range_in_text: Range<usize>,
+    path: PathBuf,
+    entry_type: EntryType,
+}
+
 fn collect_files(
     project: Model<Project>,
     glob_inputs: &[String],
     cx: &mut AppContext,
-) -> Task<Result<(String, Vec<(Range<usize>, PathBuf, EntryType)>)>> {
+) -> Task<Result<FileCommandOutput>> {
     let Ok(matchers) = glob_inputs
         .into_iter()
         .map(|glob_input| {
-            PathMatcher::new(&[glob_input.to_owned()])
+            custom_path_matcher::PathMatcher::new(&[glob_input.to_owned()])
                 .with_context(|| format!("invalid path {glob_input}"))
         })
-        .collect::<anyhow::Result<Vec<PathMatcher>>>()
+        .collect::<anyhow::Result<Vec<custom_path_matcher::PathMatcher>>>()
     else {
         return Task::ready(Err(anyhow!("invalid path")));
     };
@@ -238,6 +256,7 @@ fn collect_files(
         .worktrees(cx)
         .map(|worktree| worktree.read(cx).snapshot())
         .collect::<Vec<_>>();
+
     cx.spawn(|mut cx| async move {
         let mut text = String::new();
         let mut ranges = Vec::new();
@@ -246,10 +265,12 @@ fn collect_files(
             let mut directory_stack: Vec<(Arc<Path>, String, usize)> = Vec::new();
             let mut folded_directory_names_stack = Vec::new();
             let mut is_top_level_directory = true;
+
             for entry in snapshot.entries(false, 0) {
                 let mut path_including_worktree_name = PathBuf::new();
                 path_including_worktree_name.push(snapshot.root_name());
                 path_including_worktree_name.push(&entry.path);
+
                 if !matchers
                     .iter()
                     .any(|matcher| matcher.is_match(&path_including_worktree_name))
@@ -262,11 +283,11 @@ fn collect_files(
                         break;
                     }
                     let (_, entry_name, start) = directory_stack.pop().unwrap();
-                    ranges.push((
+                    ranges.push(OutputFile {
-                        start..text.len().saturating_sub(1),
+                        range_in_text: start..text.len().saturating_sub(1),
-                        PathBuf::from(entry_name),
+                        path: PathBuf::from(entry_name),
-                        EntryType::Directory,
+                        entry_type: EntryType::Directory,
-                    ));
+                    });
                 }
 
                 let filename = entry
@@ -339,24 +360,39 @@ fn collect_files(
                         ) {
                             text.pop();
                         }
-                        ranges.push((
+                        ranges.push(OutputFile {
-                            prev_len..text.len(),
+                            range_in_text: prev_len..text.len(),
-                            path_including_worktree_name,
+                            path: path_including_worktree_name,
-                            EntryType::File,
+                            entry_type: EntryType::File,
-                        ));
+                        });
                         text.push('\n');
                     }
                 }
             }
 
-            while let Some((dir, _, start)) = directory_stack.pop() {
+            while let Some((dir, entry, start)) = directory_stack.pop() {
-                let mut root_path = PathBuf::new();
+                if directory_stack.is_empty() {
-                root_path.push(snapshot.root_name());
+                    let mut root_path = PathBuf::new();
-                root_path.push(&dir);
+                    root_path.push(snapshot.root_name());
-                ranges.push((start..text.len(), root_path, EntryType::Directory));
+                    root_path.push(&dir);
+                    ranges.push(OutputFile {
+                        range_in_text: start..text.len(),
+                        path: root_path,
+                        entry_type: EntryType::Directory,
+                    });
+                } else {
+                    ranges.push(OutputFile {
+                        range_in_text: start..text.len(),
+                        path: PathBuf::from(entry.as_str()),
+                        entry_type: EntryType::Directory,
+                    });
+                }
             }
         }
-        Ok((text, ranges))
+        Ok(FileCommandOutput {
+            completion_text: text,
+            files: ranges,
+        })
     })
 }
 
@@ -424,3 +460,300 @@ pub fn build_entry_output_section(
         label: label.into(),
     }
 }
+
+/// This contains a small fork of the util::paths::PathMatcher, that is stricter about the prefix
+/// check. Only subpaths pass the prefix check, rather than any prefix.
+mod custom_path_matcher {
+    use std::{fmt::Debug as _, path::Path};
+
+    use globset::{Glob, GlobSet, GlobSetBuilder};
+
+    #[derive(Clone, Debug, Default)]
+    pub struct PathMatcher {
+        sources: Vec<String>,
+        sources_with_trailing_slash: Vec<String>,
+        glob: GlobSet,
+    }
+
+    impl std::fmt::Display for PathMatcher {
+        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            self.sources.fmt(f)
+        }
+    }
+
+    impl PartialEq for PathMatcher {
+        fn eq(&self, other: &Self) -> bool {
+            self.sources.eq(&other.sources)
+        }
+    }
+
+    impl Eq for PathMatcher {}
+
+    impl PathMatcher {
+        pub fn new(globs: &[String]) -> Result<Self, globset::Error> {
+            let globs = globs
+                .into_iter()
+                .map(|glob| Glob::new(&glob))
+                .collect::<Result<Vec<_>, _>>()?;
+            let sources = globs.iter().map(|glob| glob.glob().to_owned()).collect();
+            let sources_with_trailing_slash = globs
+                .iter()
+                .map(|glob| glob.glob().to_string() + std::path::MAIN_SEPARATOR_STR)
+                .collect();
+            let mut glob_builder = GlobSetBuilder::new();
+            for single_glob in globs {
+                glob_builder.add(single_glob);
+            }
+            let glob = glob_builder.build()?;
+            Ok(PathMatcher {
+                glob,
+                sources,
+                sources_with_trailing_slash,
+            })
+        }
+
+        pub fn sources(&self) -> &[String] {
+            &self.sources
+        }
+
+        pub fn is_match<P: AsRef<Path>>(&self, other: P) -> bool {
+            let other_path = other.as_ref();
+            self.sources
+                .iter()
+                .zip(self.sources_with_trailing_slash.iter())
+                .any(|(source, with_slash)| {
+                    let as_bytes = other_path.as_os_str().as_encoded_bytes();
+                    let with_slash = if source.ends_with("/") {
+                        source.as_bytes()
+                    } else {
+                        with_slash.as_bytes()
+                    };
+
+                    as_bytes.starts_with(with_slash) || as_bytes.ends_with(source.as_bytes())
+                })
+                || self.glob.is_match(other_path)
+                || self.check_with_end_separator(other_path)
+        }
+
+        fn check_with_end_separator(&self, path: &Path) -> bool {
+            let path_str = path.to_string_lossy();
+            let separator = std::path::MAIN_SEPARATOR_STR;
+            if path_str.ends_with(separator) {
+                return false;
+            } else {
+                self.glob.is_match(path_str.to_string() + separator)
+            }
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use fs::FakeFs;
+    use gpui::TestAppContext;
+    use project::Project;
+    use serde_json::json;
+    use settings::SettingsStore;
+
+    use crate::slash_command::file_command::collect_files;
+
+    pub fn init_test(cx: &mut gpui::TestAppContext) {
+        if std::env::var("RUST_LOG").is_ok() {
+            env_logger::try_init().ok();
+        }
+
+        cx.update(|cx| {
+            let settings_store = SettingsStore::test(cx);
+            cx.set_global(settings_store);
+            // release_channel::init(SemanticVersion::default(), cx);
+            language::init(cx);
+            Project::init_settings(cx);
+        });
+    }
+
+    #[gpui::test]
+    async fn test_file_exact_matching(cx: &mut TestAppContext) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+
+        fs.insert_tree(
+            "/root",
+            json!({
+                "dir": {
+                    "subdir": {
+                       "file_0": "0"
+                    },
+                    "file_1": "1",
+                    "file_2": "2",
+                    "file_3": "3",
+                },
+                "dir.rs": "4"
+            }),
+        )
+        .await;
+
+        let project = Project::test(fs, ["/root".as_ref()], cx).await;
+
+        let result_1 = cx
+            .update(|cx| collect_files(project.clone(), &["root/dir".to_string()], cx))
+            .await
+            .unwrap();
+
+        assert!(result_1.completion_text.starts_with("root/dir"));
+        // 4 files + 2 directories
+        assert_eq!(6, result_1.files.len());
+
+        let result_2 = cx
+            .update(|cx| collect_files(project.clone(), &["root/dir/".to_string()], cx))
+            .await
+            .unwrap();
+
+        assert_eq!(result_1, result_2);
+
+        let result = cx
+            .update(|cx| collect_files(project.clone(), &["root/dir*".to_string()], cx))
+            .await
+            .unwrap();
+
+        assert!(result.completion_text.starts_with("root/dir"));
+        // 5 files + 2 directories
+        assert_eq!(7, result.files.len());
+
+        // Ensure that the project lasts until after the last await
+        drop(project);
+    }
+
+    #[gpui::test]
+    async fn test_file_sub_directory_rendering(cx: &mut TestAppContext) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+
+        fs.insert_tree(
+            "/zed",
+            json!({
+                "assets": {
+                    "dir1": {
+                        ".gitkeep": ""
+                    },
+                    "dir2": {
+                        ".gitkeep": ""
+                    },
+                    "themes": {
+                        "ayu": {
+                            "LICENSE": "1",
+                        },
+                        "andromeda": {
+                            "LICENSE": "2",
+                        },
+                        "summercamp": {
+                            "LICENSE": "3",
+                        },
+                    },
+                },
+            }),
+        )
+        .await;
+
+        let project = Project::test(fs, ["/zed".as_ref()], cx).await;
+
+        let result = cx
+            .update(|cx| collect_files(project.clone(), &["zed/assets/themes".to_string()], cx))
+            .await
+            .unwrap();
+
+        // Sanity check
+        assert!(result.completion_text.starts_with("zed/assets/themes\n"));
+        assert_eq!(7, result.files.len());
+
+        // Ensure that full file paths are included in the real output
+        assert!(result
+            .completion_text
+            .contains("zed/assets/themes/andromeda/LICENSE"));
+        assert!(result
+            .completion_text
+            .contains("zed/assets/themes/ayu/LICENSE"));
+        assert!(result
+            .completion_text
+            .contains("zed/assets/themes/summercamp/LICENSE"));
+
+        assert_eq!("summercamp", result.files[5].path.to_string_lossy());
+
+        // Ensure that things are in descending order, with properly relativized paths
+        assert_eq!(
+            "zed/assets/themes/andromeda/LICENSE",
+            result.files[0].path.to_string_lossy()
+        );
+        assert_eq!("andromeda", result.files[1].path.to_string_lossy());
+        assert_eq!(
+            "zed/assets/themes/ayu/LICENSE",
+            result.files[2].path.to_string_lossy()
+        );
+        assert_eq!("ayu", result.files[3].path.to_string_lossy());
+        assert_eq!(
+            "zed/assets/themes/summercamp/LICENSE",
+            result.files[4].path.to_string_lossy()
+        );
+
+        // Ensure that the project lasts until after the last await
+        drop(project);
+    }
+
+    #[gpui::test]
+    async fn test_file_deep_sub_directory_rendering(cx: &mut TestAppContext) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+
+        fs.insert_tree(
+            "/zed",
+            json!({
+                "assets": {
+                    "themes": {
+                        "LICENSE": "1",
+                        "summercamp": {
+                            "LICENSE": "1",
+                            "subdir": {
+                                "LICENSE": "1",
+                                "subsubdir": {
+                                    "LICENSE": "3",
+                                }
+                            }
+                        },
+                    },
+                },
+            }),
+        )
+        .await;
+
+        let project = Project::test(fs, ["/zed".as_ref()], cx).await;
+
+        let result = cx
+            .update(|cx| collect_files(project.clone(), &["zed/assets/themes".to_string()], cx))
+            .await
+            .unwrap();
+
+        assert!(result.completion_text.starts_with("zed/assets/themes\n"));
+        assert_eq!(
+            "zed/assets/themes/LICENSE",
+            result.files[0].path.to_string_lossy()
+        );
+        assert_eq!(
+            "zed/assets/themes/summercamp/LICENSE",
+            result.files[1].path.to_string_lossy()
+        );
+        assert_eq!(
+            "zed/assets/themes/summercamp/subdir/LICENSE",
+            result.files[2].path.to_string_lossy()
+        );
+        assert_eq!(
+            "zed/assets/themes/summercamp/subdir/subsubdir/LICENSE",
+            result.files[3].path.to_string_lossy()
+        );
+        assert_eq!("subsubdir", result.files[4].path.to_string_lossy());
+        assert_eq!("subdir", result.files[5].path.to_string_lossy());
+        assert_eq!("summercamp", result.files[6].path.to_string_lossy());
+        assert_eq!("zed/assets/themes", result.files[7].path.to_string_lossy());
+
+        // Ensure that the project lasts until after the last await
+        drop(project);
+    }
+}

crates/assistant/src/slash_command/prompt_command.rs

@@ -45,7 +45,7 @@ impl SlashCommand for PromptSlashCommand {
                     Some(ArgumentCompletion {
                         label: prompt_title.clone().into(),
                         new_text: prompt_title,
-                        run_command: true,
+                        after_completion: true.into(),
                         replace_previous_arguments: true,
                     })
                 })

crates/assistant/src/slash_command/tab_command.rs

@@ -94,7 +94,7 @@ impl SlashCommand for TabSlashCommand {
                     label: path_string.clone().into(),
                     new_text: path_string,
                     replace_previous_arguments: false,
-                    run_command,
+                    after_completion: run_command.into(),
                 })
             });
 
@@ -106,7 +106,7 @@ impl SlashCommand for TabSlashCommand {
                     label: path_string.clone().into(),
                     new_text: path_string,
                     replace_previous_arguments: false,
-                    run_command,
+                    after_completion: run_command.into(),
                 });
 
             Ok(active_item_completion
@@ -115,7 +115,7 @@ impl SlashCommand for TabSlashCommand {
                     label: ALL_TABS_COMPLETION_ITEM.into(),
                     new_text: ALL_TABS_COMPLETION_ITEM.to_owned(),
                     replace_previous_arguments: false,
-                    run_command: true,
+                    after_completion: true.into(),
                 }))
                 .chain(tab_completion_items)
                 .collect())

crates/assistant_slash_command/src/assistant_slash_command.rs

@@ -15,6 +15,35 @@ pub fn init(cx: &mut AppContext) {
     SlashCommandRegistry::default_global(cx);
 }
 
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum AfterCompletion {
+    /// Run the command
+    Run,
+    /// Continue composing the current argument, doesn't add a space
+    Compose,
+    /// Continue the command composition, adds a space
+    Continue,
+}
+
+impl From<bool> for AfterCompletion {
+    fn from(value: bool) -> Self {
+        if value {
+            AfterCompletion::Run
+        } else {
+            AfterCompletion::Continue
+        }
+    }
+}
+
+impl AfterCompletion {
+    pub fn run(&self) -> bool {
+        match self {
+            AfterCompletion::Run => true,
+            AfterCompletion::Compose | AfterCompletion::Continue => false,
+        }
+    }
+}
+
 #[derive(Debug)]
 pub struct ArgumentCompletion {
     /// The label to display for this completion.
@@ -22,7 +51,7 @@ pub struct ArgumentCompletion {
     /// The new text that should be inserted into the command when this completion is accepted.
     pub new_text: String,
     /// Whether the command should be run when accepting this completion.
-    pub run_command: bool,
+    pub after_completion: AfterCompletion,
     /// Whether to replace the all arguments, or whether to treat this as an independent argument.
     pub replace_previous_arguments: bool,
 }

crates/extension/src/extension_slash_command.rs

@@ -67,7 +67,7 @@ impl SlashCommand for ExtensionSlashCommand {
                                         label: completion.label.into(),
                                         new_text: completion.new_text,
                                         replace_previous_arguments: false,
-                                        run_command: completion.run_command,
+                                        after_completion: completion.run_command.into(),
                                     })
                                     .collect(),
                             )

crates/project/src/project.rs

@@ -12,6 +12,7 @@ pub mod worktree_store;
 
 #[cfg(test)]
 mod project_tests;
+
 pub mod search_history;
 mod yarn;
 

crates/project/src/project_tests.rs

@@ -5204,7 +5204,7 @@ async fn search(
         .collect())
 }
 
-fn init_test(cx: &mut gpui::TestAppContext) {
+pub fn init_test(cx: &mut gpui::TestAppContext) {
     if std::env::var("RUST_LOG").is_ok() {
         env_logger::try_init().ok();
     }

crates/util/src/paths.rs

@@ -263,7 +263,7 @@ impl PathMatcher {
         let path_str = path.to_string_lossy();
         let separator = std::path::MAIN_SEPARATOR_STR;
         if path_str.ends_with(separator) {
-            self.glob.is_match(path)
+            return false;
         } else {
             self.glob.is_match(path_str.to_string() + separator)
         }