Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort ignore_dirs before to build GlobSet #223

Merged
merged 3 commits into from
Oct 30, 2018
Merged

Sort ignore_dirs before to build GlobSet #223

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3eccc29
Sort ignore_dirs before to build GlobSet
calixteman Oct 30, 2018
080de65
Fix nits
calixteman Oct 30, 2018
dfb04f7
Fix windows test
calixteman Oct 30, 2018
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
97 changes: 56 additions & 41 deletions src/path_rewriting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,15 @@ pub fn rewrite_paths(
source_dir: Option<PathBuf>,
prefix_dir: Option<PathBuf>,
ignore_not_existing: bool,
to_ignore_dirs: Vec<String>,
mut to_ignore_dirs: Vec<String>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice not to require mutation, but I guess the only way would be to clone the Vec (or use an iterator, but it would either require a complex implementation or using a third-party library such as https://docs.rs/itertools/0.5.8/itertools/trait.Itertools.html#method.sorted).
So, let's keep the mut.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My first idea was to insert-sort the vec when building it in main.rs and so not to have this "mut" here.

filter_option: Option<bool>,
) -> CovResultIter {
let mut glob_builder = GlobSetBuilder::new();

// workaround for bug: https://github.com/BurntSushi/ripgrep/issues/1079
// Some filters foo/* are ignored when not sorted
to_ignore_dirs.sort_unstable();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: can you add a comment here, explaining why this is necessary (and pointing to the globset bug)

We should also file an issue so we remember to remove this when it's no longer necessary.


for to_ignore_dir in to_ignore_dirs {
glob_builder.add(Glob::new(&to_ignore_dir).unwrap());
}
Expand Down Expand Up @@ -533,53 +538,63 @@ mod tests {
#[cfg(unix)]
#[test]
fn test_rewrite_paths_ignore_multiple_directories() {
let mut result_map: CovResultMap = HashMap::new();
result_map.insert("main.cpp".to_string(), empty_result!());
result_map.insert("mydir/prova.h".to_string(), empty_result!());
result_map.insert("mydir2/prova.h".to_string(), empty_result!());
let results = rewrite_paths(
result_map,
None,
None,
None,
false,
vec!["mydir/*".to_string(), "mydir2/*".to_string()],
None,
);
let mut count = 0;
for (abs_path, rel_path, result) in results {
count += 1;
assert_eq!(abs_path, PathBuf::from("main.cpp"));
assert_eq!(rel_path, PathBuf::from("main.cpp"));
assert_eq!(result, empty_result!());
let mut ignore_dirs = vec!["mydir/*".to_string(), "mydir2/*".to_string()];
for _ in 0..2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: please add a comment here saying that you run the test twice, with different ordering of ignore_dirs.

// we run the test twice, one with ignore_dirs and the other with ignore_dirs.reverse()
let mut result_map: CovResultMap = HashMap::new();
result_map.insert("main.cpp".to_string(), empty_result!());
result_map.insert("mydir/prova.h".to_string(), empty_result!());
result_map.insert("mydir2/prova.h".to_string(), empty_result!());
let results = rewrite_paths(
result_map,
None,
None,
None,
false,
ignore_dirs.clone(),
None,
);
let mut count = 0;
for (abs_path, rel_path, result) in results {
count += 1;
assert_eq!(abs_path, PathBuf::from("main.cpp"));
assert_eq!(rel_path, PathBuf::from("main.cpp"));
assert_eq!(result, empty_result!());
}
assert_eq!(count, 1);
ignore_dirs.reverse();
}
assert_eq!(count, 1);
}

#[cfg(windows)]
#[test]
fn test_rewrite_paths_ignore_multiple_directories() {
let mut result_map: CovResultMap = HashMap::new();
result_map.insert("main.cpp".to_string(), empty_result!());
result_map.insert("mydir\\prova.h".to_string(), empty_result!());
result_map.insert("mydir2\\prova.h".to_string(), empty_result!());
let results = rewrite_paths(
result_map,
None,
None,
None,
false,
vec!["mydir/*".to_string(), "mydir2/*".to_string()],
None,
);
let mut count = 0;
for (abs_path, rel_path, result) in results {
count += 1;
assert_eq!(abs_path, PathBuf::from("main.cpp"));
assert_eq!(rel_path, PathBuf::from("main.cpp"));
assert_eq!(result, empty_result!());
let mut ignore_dirs = vec!["mydir/*".to_string(), "mydir2/*".to_string()];
for _ in 0..2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Same comment here

// we run the test twice, one with ignore_dirs and the other with ignore_dirs.reverse()
let mut result_map: CovResultMap = HashMap::new();
result_map.insert("main.cpp".to_string(), empty_result!());
result_map.insert("mydir\\prova.h".to_string(), empty_result!());
result_map.insert("mydir2\\prova.h".to_string(), empty_result!());
let results = rewrite_paths(
result_map,
None,
None,
None,
false,
ignore_dirs.clone(),
None,
);
let mut count = 0;
for (abs_path, rel_path, result) in results {
count += 1;
assert_eq!(abs_path, PathBuf::from("main.cpp"));
assert_eq!(rel_path, PathBuf::from("main.cpp"));
assert_eq!(result, empty_result!());
}
assert_eq!(count, 1);
ignore_dirs.reverse();
}
assert_eq!(count, 1);
}

#[test]
Expand Down