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.

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

ignore case while matching excluded files #4697

Merged

Conversation

Ferada
Copy link
Contributor

@Ferada Ferada commented Jun 27, 2024

Otherwise specifying one variant might not match the same file on e.g. Windows systems, when in fact it should match. This PR replaces the path comparisons with case-insensitive ones.

Test cases are happy, but I don't have a Windows one to test this. Ideally the comparisons should be purely on the paths, but instead there's an isDirectory call in there - it might be better (faster) to require callers to specify the ending slash / for directories instead and get rid of that case.

c.f. https://shiftleft.zendesk.com/agent/tickets/102700 and https://shiftleftsecurity.slack.com/archives/CE822HDC1/p1719386396027259

https://shiftleftinc.atlassian.net/browse/SEN-2828

@Ferada Ferada requested a review from ml86 June 27, 2024 09:56
Otherwise specifying one variant might not match the same file on e.g.
Windows systems, when in fact it should match.
@Ferada Ferada force-pushed the olof/fix/ignore-case-while-matching-excluded-files branch from da9c42e to 294f771 Compare June 27, 2024 09:59
Copy link
Contributor

@max-leuthaeuser max-leuthaeuser left a comment

Choose a reason for hiding this comment

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

We will have to do this differently.
Path insensitive comparison should only happen for Windows.

@max-leuthaeuser
Copy link
Contributor

Side info: MacOS also has a case insensitive filesystem by default.

@max-leuthaeuser max-leuthaeuser merged commit b76cdda into master Jul 2, 2024
5 checks passed
@max-leuthaeuser max-leuthaeuser deleted the olof/fix/ignore-case-while-matching-excluded-files branch July 2, 2024 14:24
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.

3 participants