Use walker from ignore crate by luuumine · Pull Request #2756 · molybdenumsoftware/statix

luuumine · 2026-08-29T13:57:03Z

As discussed in #2751, this PR rewrites the walk_nix_files function to use WalkBuilder from the ignore crate directly.

This also adds support for nested gitignores and a new test for those.

mightyiam

Not a comprehensive review.

mightyiam · 2026-08-30T05:01:18Z

+        builder.standard_filters(false);
+    } else {
+        builder.require_git(false);
+        builder.add_custom_ignore_filename(".gitignore");


Is this really necessary? Wouldn't this be one of the standard filters?

true, just removed this and behavior stayed as expected with all tests passing

🤔 but it's still here? Or is GitHub showing this on a line of code that is not the original of this comment?

current code is rust if unrestricted { builder.standard_filters(false); } else { builder.add_custom_ignore_filename(".gitignore"); }
we cant remove any of those lines if we want the tests to pass

Hrm... I'm reading the library docs and it seems to me that the behavior would be the same without this else. Am I missing something?

mightyiam · 2026-08-30T05:03:28Z

+        // Disables .gitignore parsing, global ignores, and .git/ exclusion natively.
+        builder.standard_filters(false);
+    } else {
+        builder.require_git(false);


I find this actually interesting. I think we should have a test for current behavior in default branch prior to this PR.

i just checked this against the code present in master

in unrestricted: the walker does traverse .git. this seems expected to me. any user could probably just add .git to their ignore list manually

in "normal" mode, .git is not traversed

i can confirm this behavior is what we have in the new branch, and added 2 new tests to check both cases

What about this part from the require_git docs

In particular, if this is false then .gitignore files will be read from parent directories above the git root directory containing .git, which is different from the git behavior.

Do you think we have a test for this in master?

mightyiam · 2026-08-30T09:41:15Z

Would it be more prudent that tests that pass in the default branch be added in a prior PR?

luuumine · 2026-08-30T10:12:01Z

I can add them in another PR if that's something you'd prefer

mightyiam · 2026-08-30T13:06:18Z

Well, I think achieving the coverage that will give us (me as reviewer?) confidence with regards to suggested changes, as an independent step, with no implementation changes, on the default branch, would mean I don't have to take your word on it, nor do I have to test it myself.

luuumine · 2026-08-30T14:10:20Z

fair enough! do you want me to add all the tests in a separate PR first (even the non-passing one with the nested gitignores) or only the ones that pass (hidden files, missing path, and the tests regarding .git and the unrestricted mode) ?

mightyiam · 2026-08-30T15:47:12Z

Thank you for asking. I'd be most convenienced with a precursor PR that has all the passing tests.

mightyiam · 2026-08-30T15:47:59Z

Oh, and that precursor PR would have no implementation changes.

luuumine · 2026-08-30T16:09:58Z

Thanks for your reply, i just created the new PR

mightyiam · 2026-08-30T20:25:53Z

Brilliant, thank you! Would you mind rebasing?

luuumine · 2026-08-30T21:09:46Z

that would be it. please tell me if anything else needs fixing. otherwise, i believe that you can squash merge that

mightyiam

🤔 are we missing precursor test in the default branch for current behavior that has to do with the behavior that is described in WalkBuilder::require_git?

mightyiam · 2026-08-30T22:36:24Z

+        builder.standard_filters(false);
+    } else {
+        builder.require_git(false);
+        builder.add_custom_ignore_filename(".gitignore");


🤔 but it's still here? Or is GitHub showing this on a line of code that is not the original of this comment?

mightyiam · 2026-08-30T22:42:39Z

+
+    Ok(builder
+        .build()
+        .filter_map(Result::ok)


Are we currently ignoring walk errors?

what do you mean buy walk errors? as talked in the previous PR and added in the new test, it exist with 0 (no error) on the missing_path error. this is current behavior and stays the case after this PR.

This filter_map seems to ignore walking errors.

For example, if the directory is removed during the walk 🤷, we'd probably get an error here and this would ignore that error. I don't think we should be ignoring such errors.

But regardless of what we should be doing, it is more important that we don't change existing behavior. Perhaps a test that triggers a walk error. I think such a test can be achieved by removing the read permission bit of a file inside the target directory. It should be a precursor, existing behavior test, if I get to decide.

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 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

mightyiam

Getting closer

mightyiam · 2026-08-30T23:26:21Z

+        builder.standard_filters(false);
+    } else {
+        builder.require_git(false);
+        builder.add_custom_ignore_filename(".gitignore");


Hrm... I'm reading the library docs and it seems to me that the behavior would be the same without this else. Am I missing something?

mightyiam · 2026-08-30T23:29:40Z

+    });
+
+    Ok(builder.build().filter_map(|entry| {
+        let entry = entry.ok()?;


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?

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.

Made the PR for those 2 new tests in #2759

luuumine · 2026-08-31T14:47:27Z

@mightyiam Continuing the conversation here because for some reason GitHub doesnt let me reply in the thread about the else block.
We either need a require_git(false) or a add_custom_ignore_filename(".gitignore") block. The ignore crate only reads gitignore files if we are in git repositories. During the tests, this is not the case as we are in a tempdir..
I changed it to keep the require_git(false) block instead of the add_custom_ignore_filename one because this makes its purpose clearer.

luuumine · 2026-09-01T18:32:46Z

Just rebased the branch to includes tests from #2759
You can merge that

mightyiam

Sorry for not finding all my concerns in one go 😅

mightyiam · 2026-09-01T22:00:54Z

+        // Disables .gitignore parsing, global ignores, and .git/ exclusion natively.
+        builder.standard_filters(false);
+    } else {
+        builder.require_git(false);


What about this part from the require_git docs

In particular, if this is false then .gitignore files will be read from parent directories above the git root directory containing .git, which is different from the git behavior.

Do you think we have a test for this in master?

mightyiam · 2026-09-01T22:04:04Z

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


🤔 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.

luuumine mentioned this pull request Aug 29, 2026

add support for nested gitignores #2751

Closed

mightyiam reviewed Aug 30, 2026

View reviewed changes

luuumine requested a review from mightyiam August 30, 2026 09:28

luuumine mentioned this pull request Aug 30, 2026

Add tests #2757

Merged

luuumine force-pushed the rewrite branch from 863aa7a to ad11530 Compare August 30, 2026 21:08

mightyiam reviewed Aug 30, 2026

View reviewed changes

luuumine mentioned this pull request Aug 31, 2026

test: add tests for walker errors #2759

Merged

luuumine added 2 commits September 1, 2026 20:26

test: multiple gitignores

5009c8e

feat: use default walker from ignore crate

63c3fce

luuumine force-pushed the rewrite branch from e3573bf to 63c3fce Compare September 1, 2026 18:28

mightyiam reviewed Sep 1, 2026

View reviewed changes

Uh oh!

Conversation

luuumine commented Aug 29, 2026

Uh oh!

mightyiam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mightyiam commented Aug 30, 2026

Uh oh!

luuumine commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mightyiam commented Aug 30, 2026

Uh oh!

luuumine commented Aug 30, 2026

Uh oh!

mightyiam commented Aug 30, 2026

Uh oh!

mightyiam commented Aug 30, 2026

Uh oh!

luuumine commented Aug 30, 2026

Uh oh!

mightyiam commented Aug 30, 2026

Uh oh!

luuumine commented Aug 30, 2026

Uh oh!

mightyiam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mightyiam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mightyiam left a comment •

edited

Loading

luuumine commented Aug 30, 2026 •

edited

Loading