Skip to content

Remove an optimization on PathSelector producing false negatives - #12623

Merged
gnodet merged 2 commits into
apache:maven-4.0.xfrom
Geomatys:remove-include-directory-filter
Jul 30, 2026
Merged

Remove an optimization on PathSelector producing false negatives#12623
gnodet merged 2 commits into
apache:maven-4.0.xfrom
Geomatys:remove-include-directory-filter

Conversation

@desruisseaux

Copy link
Copy Markdown
Contributor

Remove an optimization on PathSelector which was sometime producing false negatives. It is safe to use an exclude filter for directories during FileVisitor transversal, but it is unsafe to use an include filter because of the need to traverse parents first.

… false negative.

It is safe to use an exclude filter for directory during `FileVisitor` transversal,
but it is unsafe to use an include filter because of the need to traverse parents first.
@desruisseaux desruisseaux self-assigned this Jul 29, 2026
@desruisseaux
desruisseaux requested a review from gnodet July 29, 2026 22:12
@desruisseaux desruisseaux removed their assignment Jul 29, 2026
@desruisseaux desruisseaux added the bug Something isn't working label Jul 29, 2026

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: APPROVE

Well-reasoned fix that removes the unsafe include-based directory pre-filtering optimization from DirectoryPrefiltering. The rationale is sound: excluding directories is safe (if a directory matches an exclude, all files within are excluded), but include-based filtering is inherently unsafe because FileVisitor must enter parent directories before discovering matching children. For example, with include pattern **/*foo*/**, the old code would reject directory org/ because it doesn't contain "foo", preventing discovery of org/foo/ and files beneath it.

The fix correctly:

  • Removes the ignoreIncludes field and all include-based logic
  • Simplifies matches() to accept any directory not explicitly excluded
  • Updates simplify() to return INCLUDES_ALL when no excludes exist
  • Flips the appropriate test assertions with clear explanatory comments

The trade-off (slightly more directory traversal for correctness) is the right call, especially since file-level includes/excludes remain unchanged. The exclude-based filtering (retained) handles the most common performance case (skipping .git, target, etc.).

Minor nit: A couple of small language issues in the test comments — "Before to match" → "In order to match" / "Before matching", and "Same reason than above" → "Same reason as above". These are cosmetic only.


Reviewed via automated review loop · Feedback? Tag @gnodet

@gnodet
gnodet merged commit aecabed into apache:maven-4.0.x Jul 30, 2026
22 checks passed
@github-actions github-actions Bot added this to the 4.0.0-rc-6 milestone Jul 30, 2026
gnodet added a commit that referenced this pull request Jul 30, 2026
Forward-port of #12623 from maven-4.0.x to master.

Removes the unsafe include-based directory pre-filtering optimization
from PathSelector.DirectoryPrefiltering. Using exclude filters for
directory traversal is safe, but using include filters causes false
negatives because parent directories need to be traversed first.

Author: Martin Desruisseaux (@desruisseaux)
Closes #12624
@desruisseaux
desruisseaux deleted the remove-include-directory-filter branch July 30, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants