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
8 changes: 4 additions & 4 deletions bin/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ impl Check {
Ok(ReadOnlyVfs::singleton("<stdin>", src.as_bytes()))
} else {
let all_ignores = [self.ignore.as_slice(), extra_ignores].concat();
let ignore = dirs::build_ignore_set(&all_ignores, &self.target, self.unrestricted)?;
let files = dirs::walk_nix_files(ignore, &self.target)?;
dirs::check_path_exists(&self.target)?;
let files = dirs::walk_nix_files(&self.target, &all_ignores, self.unrestricted)?;
Ok(vfs(&files.collect::<Vec<_>>()))
}
}
Expand Down Expand Up @@ -129,8 +129,8 @@ impl Fix {
Ok(ReadOnlyVfs::singleton("<stdin>", src.as_bytes()))
} else {
let all_ignores = [self.ignore.as_slice(), extra_ignores].concat();
let ignore = dirs::build_ignore_set(&all_ignores, &self.target, self.unrestricted)?;
let files = dirs::walk_nix_files(ignore, &self.target)?;
dirs::check_path_exists(&self.target)?;
let files = dirs::walk_nix_files(&self.target, &all_ignores, self.unrestricted)?;
Ok(vfs(&files.collect::<Vec<_>>()))
}
}
Expand Down
151 changes: 57 additions & 94 deletions bin/src/dirs.rs
Original file line number Diff line number Diff line change
@@ -1,112 +1,75 @@
use std::{
fs,
io::{self, Error, ErrorKind},
path::{Path, PathBuf},
};

use crate::dirs;

use ignore::{
Error as IgnoreError, Match,
gitignore::{Gitignore, GitignoreBuilder},
};

#[derive(Debug)]
pub struct Walker {
dirs: Vec<PathBuf>,
files: Vec<PathBuf>,
ignore: Gitignore,
}
use std::path::{Path, PathBuf};

impl Walker {
pub fn new<P: AsRef<Path>>(target: P, ignore: Gitignore) -> io::Result<Self> {
let target = target.as_ref().to_path_buf();
if !target.exists() {
Err(Error::new(
ErrorKind::NotFound,
format!("file not found: {}", target.display()),
))
Comment thread
luuumine marked this conversation as resolved.
} else if target.is_dir() {
Ok(Self {
dirs: vec![target],
files: vec![],
ignore,
})
} else {
Ok(Self {
dirs: vec![],
files: vec![target],
ignore,
})
}
}
}
use ignore::{Error as IgnoreError, Match, WalkBuilder, gitignore::GitignoreBuilder};

impl Iterator for Walker {
type Item = PathBuf;
fn next(&mut self) -> Option<Self::Item> {
self.files.pop().or_else(|| {
while let Some(dir) = self.dirs.pop() {
if dir.is_dir()
&& let Match::None | Match::Whitelist(_) = self.ignore.matched(&dir, true)
{
let mut found = false;
for entry in fs::read_dir(&dir).ok()? {
let entry = entry.ok()?;
let path = entry.path();
if path.is_dir() {
self.dirs.push(path);
} else if path.is_file()
&& let Match::None | Match::Whitelist(_) =
self.ignore.matched(&path, false)
{
found = true;
self.files.push(path);
}
}
if found {
break;
}
}
}
self.files.pop()
})
pub fn check_path_exists<P: AsRef<Path>>(target: P) -> std::io::Result<()> {
let target = target.as_ref();
if target.exists() {
Ok(())
} else {
Err(std::io::Error::new(
std::io::ErrorKind::NotFound,
format!("file not found: {}", target.display()),
))
}
}

pub fn build_ignore_set<P: AsRef<Path>>(
ignore: &[String],
pub fn walk_nix_files<P: AsRef<Path>>(
target: P,
extra_ignores: &[String],
unrestricted: bool,
) -> Result<Gitignore, IgnoreError> {
let gitignore_path = target.as_ref().join(".gitignore");
) -> Result<impl Iterator<Item = PathBuf>, IgnoreError> {
let target = target.as_ref();

// Looks like GitignoreBuilder::new does not source globs
// within gitignore_path by default, we have to enforce that
// using GitignoreBuilder::add. Probably a bug in the ignore
// crate?
let mut gitignore = GitignoreBuilder::new(&gitignore_path);
let mut builder = WalkBuilder::new(target);

// if we are to "restrict" aka "respect" .gitignore, then
// add globs from gitignore path as well
if !unrestricted {
gitignore.add(&gitignore_path);
// read as "do not ignore hidden files"
Comment thread
luuumine marked this conversation as resolved.
builder.hidden(false);

if unrestricted {
builder.standard_filters(false);
} else {
builder.require_git(false);
}

let mut gitignore = GitignoreBuilder::new(target);

if !unrestricted {
// ignore .git by default, nobody cares about .git, i'm sure
gitignore.add_line(None, ".git")?;
}

for i in ignore {
gitignore.add_line(None, i.as_str())?;
for ignore_rule in extra_ignores {
gitignore.add_line(None, ignore_rule)?;

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.

🤔 I'm also concerned about not having a test with regards to --ignore not ignoring files it should not. And its behavior with regards to files inside directories. And its behavior with arguments that are paths, not only filenames.

}
let custom_ignore = gitignore.build()?;

gitignore.build()
}
builder.filter_entry(move |entry| {
if entry.depth() == 0 {
return true;
}

pub fn walk_nix_files<P: AsRef<Path>>(
ignore: Gitignore,
target: P,
) -> Result<impl Iterator<Item = PathBuf>, io::Error> {
let walker = dirs::Walker::new(target, ignore)?;
Ok(walker.filter(|path: &PathBuf| matches!(path.extension(), Some(e) if e == "nix")))
let is_dir = entry.file_type().is_some_and(|ft| ft.is_dir());

!matches!(
custom_ignore.matched(entry.path(), is_dir),
Match::Ignore(_)
)
});

Ok(builder.build().filter_map(|entry| {
let entry = 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.

To continue conversation about error handling of walk errors: this seems to just ignore these errors. As I wrote elsewhere, would you be happy adding a test in a precursor PR that confirms whateber the current behavior is?

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.

Continuing from #2756 (comment)

i believe it would be a good idea (in a later PR probably?) to change the some error behaviors, especially the fact that, on a missing path, the programs exits with status code 0 (no error).

I don't have the confidence that that is the current behavior. That is why I am interested in having a test added in a precursor PR to confirm whatever the current bevahior is.

i don't really know how we could test the removal of a directory during traversal or equivalent though. adding more tests around error behaviors later could be a good idea though

I think this could be achieved by having a file with the read permission bit removed, within the target directory.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Made the PR for those 2 new tests in #2759

let file_type = entry.file_type()?;

if !file_type.is_file() {
return None;
}

let path = entry.into_path();
if path.extension()? != "nix" {
return None;
}

Some(path)
}))
}
19 changes: 19 additions & 0 deletions bin/tests/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,25 @@ mod gitignored_files {

assert_eq!(report.paths, ["./linted.nix"]);
}

#[test]
fn multiple_gitignores() {
let report = Fixture::with_files(&[
("file.nix", CODE_THAT_TRIGGERS_A_LINT),
("a/.gitignore", "file.nix\nbuild/\n"),
("a/file.nix", CODE_THAT_TRIGGERS_A_LINT),
("a/build/inside.nix", CODE_THAT_TRIGGERS_A_LINT),
("b/file.nix", CODE_THAT_TRIGGERS_A_LINT),
("b/build/inside.nix", CODE_THAT_TRIGGERS_A_LINT),
])
.run_with_args(&[])
.unwrap();

assert_eq!(
report.paths,
["./file.nix", "./b/file.nix", "./b/build/inside.nix"]
);
}
}

mod error {
Expand Down