Skip to content

fix: make FileFilter patterns case-insensitive - #859

Merged
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
Itachi7011:fix/844-case-insensitive-file-filter
Aug 13, 2026
Merged

fix: make FileFilter patterns case-insensitive#859
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
Itachi7011:fix/844-case-insensitive-file-filter

Conversation

@Itachi7011

Copy link
Copy Markdown
Contributor

Summary

Fixes #844.

FileFilter.IsUserIncluded and FileFilter.IsUserExcluded lowercased
the file path before matching, but did not lowercase the user-provided
pattern.

This caused patterns containing uppercase letters such as
README.md and **/CHANGELOG.md to fail to match.

Changes

  • Lowercase expanded exclude patterns before doublestar.Match
  • Lowercase expanded include patterns before doublestar.Match
  • Extended TestFileFilter_CaseInsensitive with uppercase-pattern
    regression coverage

Testing

go test ./internal/config/rules/...

@CLAassistant

CLAassistant commented Aug 12, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

@wu21-web wu21-web 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.

No issues found.

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

@Itachi7011 Nice first contribution — the fix is exactly right, and I appreciate that you kept it minimal instead of refactoring around it. I pulled the branch locally: tests pass, and I confirmed the new assertions genuinely fail once system_rules.go is reverted, so the regression coverage is real. I also grepped every doublestar.Match call site — these two were the only remaining asymmetric ones, so nothing was missed (allowlist/allowed_ext.go lowercases its patterns at init, and diff/git.go is case-sensitive on purpose since that's gitignore semantics).

Three small suggestions, none of them blocking:

1. Use mixed-case paths in the test. The new assertions pair an uppercase pattern with an all-lowercase path:

if !f.IsUserIncluded("docs/changelog.md") { ... }
if !f.IsUserExcluded("readme.md") { ... }

That does catch the regression, but it sidesteps the spelling people actually hit — pattern and path both containing uppercase, e.g. --exclude packages/backend/convex/seedVolume.ts against a file named seedVolume.ts. The repro in #844 is written that way. The real-world spelling still fails before the fix, and additionally pins the user-facing case:

if !f.IsUserIncluded("docs/CHANGELOG.md") { ... }
if !f.IsUserExcluded("README.md") { ... }

2. Add one brace-expansion case. The only interaction still untested is the ordering of ToLower and expandBraces. You went with expand-then-lowercase — correct, and it mirrors Resolve — but nothing currently locks that in. One extra pattern would:

Exclude: []string{"**/*.{Go,Java}"},  // assert "pkg/Main.JAVA" is excluded

3. State the contract in the doc comments. Neither IsUserExcluded nor IsUserIncluded mentions being case-insensitive, whereas allowlist.IsExcludedPath spells it out ("The check is case-insensitive."). Given this bug was precisely a half-implemented version of that unwritten contract, one line would help keep it from drifting back:

// IsUserExcluded reports whether the given path matches any user exclude pattern.
// The check is case-insensitive: both path and pattern are lowercased.

Thanks again — clean, well-scoped patch.

@Itachi7011

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review! I appreciate the suggestions. I'll keep these in mind. Glad to hear the fix and regression coverage look good. 🙏

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Itachi7011 I'd like to see your updates regarding these three small suggestions.

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 8ce5297 into alibaba:main Aug 13, 2026
10 checks passed
@Itachi7011

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've addressed all three suggestions:

  • Updated the regression cases to use mixed-case paths.
  • Added brace-expansion coverage with an uppercase pattern.
  • Documented the case-insensitive behavior in both FileFilter methods.

I also ran go test ./... locally, and all tests pass.

@Fanzzzd

Fanzzzd commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Congrats on landing your first contribution, @Itachi7011 — clean fix, regression test included, merged within a day. Exactly how it should go. Welcome aboard 🎉

@Itachi7011
Itachi7011 deleted the fix/844-case-insensitive-file-filter branch August 14, 2026 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include/exclude patterns containing an uppercase letter never match: FileFilter lowercases the path but not the pattern

5 participants