From 455850505f7b3a3f091cf8c26f251d8fa8471dcf Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Fri, 16 Aug 2024 10:09:38 -0700 Subject: [PATCH] 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 --- Cargo.lock | 1 + crates/assistant/Cargo.toml | 1 + crates/assistant/src/slash_command.rs | 9 +- .../src/slash_command/diagnostics_command.rs | 2 +- .../src/slash_command/docs_command.rs | 10 +- .../src/slash_command/file_command.rs | 393 ++++++++++++++++-- .../src/slash_command/prompt_command.rs | 2 +- .../src/slash_command/tab_command.rs | 6 +- .../src/assistant_slash_command.rs | 31 +- .../extension/src/extension_slash_command.rs | 2 +- crates/project/src/project.rs | 1 + crates/project/src/project_tests.rs | 2 +- crates/util/src/paths.rs | 2 +- 13 files changed, 415 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fb50dfb37e..402953d95f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -367,6 +367,7 @@ dependencies = [ "fs", "futures 0.3.30", "fuzzy", + "globset", "gpui", "handlebars", "heed", diff --git a/crates/assistant/Cargo.toml b/crates/assistant/Cargo.toml index 9f084bbefb..98dfd40c74 100644 --- a/crates/assistant/Cargo.toml +++ b/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 diff --git a/crates/assistant/src/slash_command.rs b/crates/assistant/src/slash_command.rs index d938225dab..667b11c742 100644 --- a/crates/assistant/src/slash_command.rs +++ b/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(' '); } diff --git a/crates/assistant/src/slash_command/diagnostics_command.rs b/crates/assistant/src/slash_command/diagnostics_command.rs index 46342fc945..6c821bd7b4 100644 --- a/crates/assistant/src/slash_command/diagnostics_command.rs +++ b/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()) diff --git a/crates/assistant/src/slash_command/docs_command.rs b/crates/assistant/src/slash_command/docs_command.rs index 370805b090..e114cfeab7 100644 --- a/crates/assistant/src/slash_command/docs_command.rs +++ b/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::>(); @@ -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, }]); } diff --git a/crates/assistant/src/slash_command/file_command.rs b/crates/assistant/src/slash_command/file_command.rs index e73fb158df..244dee2107 100644 --- a/crates/assistant/src/slash_command/file_command.rs +++ b/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, - sections: ranges + text: output.completion_text, + sections: output + .files .into_iter() - .map(|(range, path, entry_type)| { + .map(|file| { build_entry_output_section( - range, - Some(&path), - entry_type == EntryType::Directory, + file.range_in_text, + Some(&file.path), + 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, +} + +#[derive(Clone, PartialEq, Debug)] +struct OutputFile { + range_in_text: Range, + path: PathBuf, + entry_type: EntryType, +} + fn collect_files( project: Model, glob_inputs: &[String], cx: &mut AppContext, -) -> Task, PathBuf, EntryType)>)>> { +) -> Task> { 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::>>() + .collect::>>() else { return Task::ready(Err(anyhow!("invalid path"))); }; @@ -238,6 +256,7 @@ fn collect_files( .worktrees(cx) .map(|worktree| worktree.read(cx).snapshot()) .collect::>(); + 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, 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(( - start..text.len().saturating_sub(1), - PathBuf::from(entry_name), - EntryType::Directory, - )); + ranges.push(OutputFile { + range_in_text: start..text.len().saturating_sub(1), + path: PathBuf::from(entry_name), + entry_type: EntryType::Directory, + }); } let filename = entry @@ -339,24 +360,39 @@ fn collect_files( ) { text.pop(); } - ranges.push(( - prev_len..text.len(), - path_including_worktree_name, - EntryType::File, - )); + ranges.push(OutputFile { + range_in_text: prev_len..text.len(), + path: path_including_worktree_name, + entry_type: EntryType::File, + }); text.push('\n'); } } } - while let Some((dir, _, start)) = directory_stack.pop() { - let mut root_path = PathBuf::new(); - root_path.push(snapshot.root_name()); - root_path.push(&dir); - ranges.push((start..text.len(), root_path, EntryType::Directory)); + while let Some((dir, entry, start)) = directory_stack.pop() { + if directory_stack.is_empty() { + let mut root_path = PathBuf::new(); + root_path.push(snapshot.root_name()); + 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, + sources_with_trailing_slash: Vec, + 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 { + let globs = globs + .into_iter() + .map(|glob| Glob::new(&glob)) + .collect::, _>>()?; + 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>(&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); + } +} diff --git a/crates/assistant/src/slash_command/prompt_command.rs b/crates/assistant/src/slash_command/prompt_command.rs index 783d120dad..4d64bba2ed 100644 --- a/crates/assistant/src/slash_command/prompt_command.rs +++ b/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, }) }) diff --git a/crates/assistant/src/slash_command/tab_command.rs b/crates/assistant/src/slash_command/tab_command.rs index de590ef837..0d8559896b 100644 --- a/crates/assistant/src/slash_command/tab_command.rs +++ b/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()) diff --git a/crates/assistant_slash_command/src/assistant_slash_command.rs b/crates/assistant_slash_command/src/assistant_slash_command.rs index 1a9cad5611..c5dece11ca 100644 --- a/crates/assistant_slash_command/src/assistant_slash_command.rs +++ b/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 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, } diff --git a/crates/extension/src/extension_slash_command.rs b/crates/extension/src/extension_slash_command.rs index d0ec466767..60b027ef9d 100644 --- a/crates/extension/src/extension_slash_command.rs +++ b/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(), ) diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index af5cc97c4f..04ecad7fc5 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -12,6 +12,7 @@ pub mod worktree_store; #[cfg(test)] mod project_tests; + pub mod search_history; mod yarn; diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index d9df60c099..783bac3885 100644 --- a/crates/project/src/project_tests.rs +++ b/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(); } diff --git a/crates/util/src/paths.rs b/crates/util/src/paths.rs index 43937ff328..42d215b322 100644 --- a/crates/util/src/paths.rs +++ b/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) }