Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions rust/rubydex-sys/src/graph_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ pub unsafe extern "C" fn rdx_graph_resolve_constant(
#[unsafe(no_mangle)]
pub unsafe extern "C" fn rdx_graph_exclude_paths(pointer: GraphPointer, paths: *const *const c_char, count: usize) {
let paths: Vec<String> = unsafe { utils::convert_double_pointer_to_vec(paths, count).unwrap() };
let path_bufs: Vec<PathBuf> = paths.into_iter().map(PathBuf::from).collect();
with_mut_graph(pointer, |graph| graph.exclude_paths(path_bufs));
let entries: Vec<Box<str>> = paths.into_iter().map(String::into_boxed_str).collect();
with_mut_graph(pointer, |graph| graph.exclude_paths(entries));
}

/// Returns the currently excluded paths as an array of C strings. Writes the count to `out_count`. Returns NULL if no
Expand Down Expand Up @@ -202,7 +202,7 @@ pub unsafe extern "C" fn rdx_graph_excluded_paths(
// on Windows if a configuration file is using forward slashes. For example:
//
// C:\project/vendor/bundle
let normalized = path.to_string_lossy().replace(std::path::MAIN_SEPARATOR, "/");
let normalized = path.replace(std::path::MAIN_SEPARATOR, "/");

CString::new(normalized)
.ok()
Expand Down
45 changes: 28 additions & 17 deletions rust/rubydex/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub const DEFAULT_EXCLUDED_DIRECTORIES: &[&str] = &[
struct ConfigFile {
/// Paths to exclude from file discovery during indexing.
#[serde(default)]
exclude: Vec<PathBuf>,
exclude: Vec<Box<str>>,
}

/// Project configuration
Expand All @@ -32,7 +32,7 @@ pub struct Config {
/// Path to the workspace being analyzed
workspace_path: Box<Path>,
/// Paths to exclude from file discovery during indexing.
excluded_paths: HashSet<PathBuf>,
excluded_paths: HashSet<Box<str>>,
}
assert_mem_size!(Config, 64);

Expand All @@ -50,7 +50,7 @@ impl Config {
workspace_path: std::env::current_dir()
.unwrap_or_else(|_| PathBuf::from("."))
.into_boxed_path(),
excluded_paths: DEFAULT_EXCLUDED_DIRECTORIES.iter().map(PathBuf::from).collect(),
excluded_paths: DEFAULT_EXCLUDED_DIRECTORIES.iter().map(|&dir| Box::from(dir)).collect(),
}
}

Expand All @@ -67,16 +67,22 @@ impl Config {

/// Adds paths to exclude from file discovery during indexing. Excluded directories will be skipped entirely during
/// directory traversal.
pub fn exclude_paths(&mut self, paths: impl IntoIterator<Item = PathBuf>) {
pub fn exclude_paths(&mut self, paths: impl IntoIterator<Item = Box<str>>) {
self.excluded_paths.extend(paths);
}

/// Returns the set of paths excluded from file discovery
#[must_use]
pub fn excluded_paths(&self) -> HashSet<PathBuf> {
pub fn excluded_paths(&self) -> HashSet<Box<str>> {
self.excluded_paths
.iter()
.map(|path| self.workspace_path.join(path))
.map(|entry| {
self.workspace_path
.join(&**entry)
.to_string_lossy()
.replace(std::path::MAIN_SEPARATOR, "/")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we substituting the separator here? Note that this PR changes excluded_paths into excluded_glob_patterns. For file paths, backslashes are valid (on Windows), but not for glob patterns (which always use forward slashes).

.into_boxed_str()
})
.collect()
}

Expand Down Expand Up @@ -143,15 +149,15 @@ mod tests {
fn excluded_paths_are_resolved_against_the_workspace_path() {
let mut config = Config::new();
config.set_workspace_path(PathBuf::from("/workspace"));
config.exclude_paths([PathBuf::from("vendor"), PathBuf::from("/absolute/path")]);
config.exclude_paths([Box::from("vendor"), Box::from("/absolute/path")]);

let excluded = config.excluded_paths();

// Relative entries (including the defaults) are joined with the workspace path.
assert!(excluded.contains(Path::new("/workspace/vendor")));
assert!(excluded.contains(Path::new("/workspace/.git")));
assert!(excluded.contains("/workspace/vendor"));
assert!(excluded.contains("/workspace/.git"));
// Absolute entries pass through unchanged.
assert!(excluded.contains(Path::new("/absolute/path")));
assert!(excluded.contains("/absolute/path"));
}

#[test]
Expand All @@ -160,7 +166,7 @@ mod tests {

for default in DEFAULT_EXCLUDED_DIRECTORIES {
assert!(
config.excluded_paths.contains(Path::new(default)),
config.excluded_paths.contains(*default),
"expected `{default}` to be excluded by default"
);
}
Expand All @@ -181,10 +187,10 @@ mod tests {

let excluded = config.excluded_paths();
// Entries from the file are merged in and resolved against the workspace path.
assert!(excluded.contains(Path::new("/workspace/vendor")));
assert!(excluded.contains(Path::new("/workspace/generated")));
assert!(excluded.contains("/workspace/vendor"));
assert!(excluded.contains("/workspace/generated"));
// Defaults seeded at construction survive the merge.
assert!(excluded.contains(Path::new("/workspace/node_modules")));
assert!(excluded.contains("/workspace/node_modules"));
// A config file cannot override the programmatically-set workspace path.
assert_eq!(config.workspace_path(), Path::new("/workspace"));
}
Expand All @@ -205,8 +211,8 @@ mod tests {
.expect("expected the second file to load");

let excluded = config.excluded_paths();
assert!(excluded.contains(Path::new("/workspace/vendor")));
assert!(excluded.contains(Path::new("/workspace/generated")));
assert!(excluded.contains("/workspace/vendor"));
assert!(excluded.contains("/workspace/generated"));
}

#[test]
Expand Down Expand Up @@ -244,7 +250,12 @@ mod tests {
config.set_workspace_path(dir.path().to_path_buf());
config.load_default().expect("expected rubydex.toml to load");

assert!(config.excluded_paths().contains(&dir.path().join("vendor")));
let expected = dir
.path()
.join("vendor")
.to_string_lossy()
.replace(std::path::MAIN_SEPARATOR, "/");
assert!(config.excluded_paths().contains(expected.as_str()));
}

#[test]
Expand Down
69 changes: 53 additions & 16 deletions rust/rubydex/src/listing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::{
job_queue::{Job, JobQueue},
};
use crossbeam_channel::{Sender, unbounded};
use glob::Pattern;
use std::{
collections::HashSet,
fs,
Expand All @@ -16,7 +17,7 @@ pub struct FileDiscoveryJob {
queue: Arc<JobQueue>,
paths_tx: Sender<PathBuf>,
errors_tx: Sender<Errors>,
excluded_paths: Arc<HashSet<PathBuf>>,
excluded_patterns: Arc<Vec<Pattern>>,
}

impl FileDiscoveryJob {
Expand All @@ -26,14 +27,14 @@ impl FileDiscoveryJob {
queue: Arc<JobQueue>,
paths_tx: Sender<PathBuf>,
errors_tx: Sender<Errors>,
excluded_paths: Arc<HashSet<PathBuf>>,
excluded_patterns: Arc<Vec<Pattern>>,
) -> Self {
Self {
path,
queue,
paths_tx,
errors_tx,
excluded_paths,
excluded_patterns,
}
}
}
Expand All @@ -43,6 +44,10 @@ fn is_indexable_file(path: &Path) -> bool {
.is_some_and(|ext| ext == "rb" || ext == "rake" || ext == "rbs" || ext == "ru")
}

fn is_excluded(excluded_patterns: &[Pattern], path: &Path) -> bool {
excluded_patterns.iter().any(|pattern| pattern.matches_path(path))
}

impl FileDiscoveryJob {
fn handle_file(&self, path: &Path) {
if is_indexable_file(path) {
Expand All @@ -62,7 +67,7 @@ impl FileDiscoveryJob {
return;
};

if self.excluded_paths.contains(&canonicalized) {
if is_excluded(&self.excluded_patterns, &canonicalized) {
return;
}

Expand All @@ -71,7 +76,7 @@ impl FileDiscoveryJob {
Arc::clone(&self.queue),
self.paths_tx.clone(),
self.errors_tx.clone(),
Arc::clone(&self.excluded_paths),
Arc::clone(&self.excluded_patterns),
)));
}

Expand Down Expand Up @@ -107,7 +112,7 @@ impl Job for FileDiscoveryJob {
let kind = entry.file_type().unwrap();

if kind.is_dir() {
if self.excluded_paths.contains(&entry.path()) {
if is_excluded(&self.excluded_patterns, &entry.path()) {
continue;
}

Expand All @@ -116,7 +121,7 @@ impl Job for FileDiscoveryJob {
Arc::clone(&self.queue),
self.paths_tx.clone(),
self.errors_tx.clone(),
Arc::clone(&self.excluded_paths),
Arc::clone(&self.excluded_patterns),
)));
} else if kind.is_file() {
self.handle_file(&entry.path());
Expand Down Expand Up @@ -154,14 +159,29 @@ impl Job for FileDiscoveryJob {
#[must_use]
pub fn collect_file_paths<S: BuildHasher>(
paths: Vec<String>,
excluded: &HashSet<PathBuf, S>,
excluded: &HashSet<Box<str>, S>,
) -> (Vec<PathBuf>, Vec<Errors>) {
let queue = Arc::new(JobQueue::new());
let (files_tx, files_rx) = unbounded();
let (errors_tx, errors_rx) = unbounded();

// Canonicalize the excluded paths since they may be symlinks
let excluded: Arc<HashSet<PathBuf>> = Arc::new(excluded.iter().filter_map(|p| fs::canonicalize(p).ok()).collect());
// Canonicalize concrete excluded paths (they may be symlinks) and escape them so they match exactly, not
// as globs. Globs are kept as written.
let excluded_patterns: Arc<Vec<Pattern>> = Arc::new(
excluded
.iter()
.filter_map(|entry| {
let entry: &str = entry;

if entry.contains(['*', '?', '[']) {
Pattern::new(entry).ok()
} else {
let canonical = fs::canonicalize(entry).ok()?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you find that treating all entries as glob patterns was too slow? Do we need the two separate paths?

Also, I think there's potential for this to be slightly confusing. Consider this example:

project_path/
  my_symlink -> some_other_path

Trying to exclude the same directory using a pattern vs a path results in different behaviour:

# rubydex.toml

# This config path gets canonicalized to `some_other_path`, which then
# gets excluded since we also canonicalize the paths being compared
exclude = ["my_symlink"]

# This pattern does not get canonicalized since you cannot do it for a glob
# pattern, which means we compare if `some_other_path` matches `**/my_symlink`.
#
# It does not and so we fail to exclude
exclude = ["**/my_symlink/"]

I would vote for us to do the following:

  1. Stop following symlinks (both in the config and in the listing). If someone created a symlink in their project, I would assume they'd prefer dealing with the symlink they created rather than the destination path
  2. Treat everything as glob patterns

Pattern::new(&Pattern::escape(&canonical.to_string_lossy())).ok()
}
})
.collect(),
);

for path in paths {
let Ok(canonicalized) = fs::canonicalize(&path) else {
Expand All @@ -172,7 +192,7 @@ pub fn collect_file_paths<S: BuildHasher>(
continue;
};

if excluded.contains(&canonicalized) {
if is_excluded(&excluded_patterns, &canonicalized) {
continue;
}

Expand All @@ -181,7 +201,7 @@ pub fn collect_file_paths<S: BuildHasher>(
Arc::clone(&queue),
files_tx.clone(),
errors_tx.clone(),
Arc::clone(&excluded),
Arc::clone(&excluded_patterns),
)));
}

Expand All @@ -205,7 +225,7 @@ mod tests {
fn collect_document_paths_with_exclusions(
context: &Context,
paths: &[&str],
excluded: &HashSet<PathBuf>,
excluded: &HashSet<Box<str>>,
) -> (Vec<String>, Vec<Errors>) {
let (files, errors) = collect_file_paths(
paths
Expand Down Expand Up @@ -333,7 +353,7 @@ mod tests {
context.touch(&excluded_file);

let mut excluded = HashSet::new();
excluded.insert(context.absolute_path_to("excluded"));
excluded.insert(context.absolute_path_to("excluded").to_string_lossy().into());

let (files, errors) = collect_document_paths_with_exclusions(&context, &["included", "excluded"], &excluded);

Expand All @@ -350,7 +370,7 @@ mod tests {
context.touch(&nested);

let mut excluded = HashSet::new();
excluded.insert(context.absolute_path_to("root/skip"));
excluded.insert(context.absolute_path_to("root/skip").to_string_lossy().into());

let (files, errors) = collect_document_paths_with_exclusions(&context, &["root"], &excluded);

Expand All @@ -372,11 +392,28 @@ mod tests {

// Excluding the real directory while requesting to index the symlink should properly exclude the link
let mut excluded = HashSet::new();
excluded.insert(context.absolute_path_to("real_dir"));
excluded.insert(context.absolute_path_to("real_dir").to_string_lossy().into());

let (files, errors) = collect_document_paths_with_exclusions(&context, &["included", "link"], &excluded);

assert!(errors.is_empty());
assert_eq!(files, [included.to_str().unwrap().to_string()]);
}

#[test]
fn collect_files_excludes_glob_patterns() {
let context = Context::new();
let kept = PathBuf::from("lib").join("foo.rb");
let nested = PathBuf::from("lib").join("test/fixtures").join("bar.rb");
context.touch(&kept);
context.touch(&nested);

let mut excluded = HashSet::new();
excluded.insert("**/fixtures".into());

let (files, errors) = collect_document_paths_with_exclusions(&context, &["lib"], &excluded);

assert!(errors.is_empty());
assert_eq!(files, [kept.to_str().unwrap().to_string()]);
}
}
4 changes: 2 additions & 2 deletions rust/rubydex/src/model/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ impl Graph {

/// Adds paths to exclude from file discovery during indexing. Excluded directories will be skipped entirely during
/// directory traversal.
pub fn exclude_paths(&mut self, paths: Vec<PathBuf>) {
pub fn exclude_paths(&mut self, paths: Vec<Box<str>>) {
self.config.exclude_paths(paths);
}

/// Returns the set of paths excluded from file discovery.
#[must_use]
pub fn excluded_paths(&self) -> HashSet<PathBuf> {
pub fn excluded_paths(&self) -> HashSet<Box<str>> {
self.config.excluded_paths()
}

Expand Down
Loading