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

Fix "any" matching when not all globs match #75

Merged
merged 1 commit into from
Jun 19, 2020

Conversation

jalaziz
Copy link
Contributor

@jalaziz jalaziz commented Jun 10, 2020

The previous logic had a bug where the "any" pattern list could match
against changed files even when not all globs matched a single changed
file. The bug arose individual globs in the "any" list were tested
against all changed files individually. Therefore, as long as at least
one changed file matched an individual glob in the list, a successful
match would be found.

The correct behavior is to match all the globs in the list against each
individual file. This ensures that is possible to define exlcusions
correctly and matched the documented behavior.

@jalaziz
Copy link
Contributor Author

jalaziz commented Jun 10, 2020

@dakale Still testing this, but discovered a bug in the previous logic.

There was an edge case where "any" matching would be successful when separate files each individually matched a glob in the "any" list. However, this can already be accomplished by listing out each glob on its own line.

The correct behavior should be matching all the globs against each changed file and returning true when at least one match is found.

The "all" logic was updated to match the testing order, but it should be functionally equivalent.

@jalaziz
Copy link
Contributor Author

jalaziz commented Jun 10, 2020

Ran some tests against a test repo I have and I believe this logic works and fixed the issue.

Copy link

@siemelnaranhighfive siemelnaranhighfive left a comment

Choose a reason for hiding this comment

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

LGTM but the comment on old line 4220 "// equivalent to "Array.some()" but expanded for debugging and clarity" is gone.

The previous logic had a bug where the "any" pattern list could match
against changed files even when not all globs matched a single changed
file. The bug arose individual globs in the "any" list were tested
against all changed files individually. Therefore, as long as at least
one changed file matched an individual glob in the list, a successful
match would be found.

The correct behavior is to match all the globs in the list against each
individual file. This ensures that is possible to define exlcusions
correctly and matched the documented behavior.
@jalaziz
Copy link
Contributor Author

jalaziz commented Jun 19, 2020

@dakale Just wanted to ping on this since master is currently buggy.

Copy link
Contributor

@dakale dakale left a comment

Choose a reason for hiding this comment

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

Sorry for slow response, just saw this. LGTM

@dakale dakale merged commit efc1b29 into actions:master Jun 19, 2020
@jalaziz
Copy link
Contributor Author

jalaziz commented Jun 19, 2020

Thanks!

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.

None yet

3 participants