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 and comprehensively test glob ordering #267

Merged
merged 1 commit into from
Aug 30, 2019
Merged

Fix and comprehensively test glob ordering #267

merged 1 commit into from
Aug 30, 2019

Conversation

eatkins
Copy link
Contributor

@eatkins eatkins commented Aug 30, 2019

The Ordering[Matcher] implementation violated the general ordering
contract. sbt/sbt#4970 was still present
with sbt 1.3.0-RC5. This commit fixes the contract of Ordering[Matcher]
so that it should be sound for all Matcher instances.

Using jacoco, I was able to identify which branches of
the matchers were not being hit. After this change, all of the branches
were hit. While I'm not sure what was specifically causing the issue in
sbt/4970, I suspect that somewhere was an AndFilter, OrFilter or
NotFilter. The conversion in Globs from AndFilter, NotFilter and OrFilter to
Matcher created AndMatcher, NotMatcher and OrMatcher. The contract
between these matchers was not sound in the Ordering[Matcher]
implementation. Instead of trying to sort these, I just throw up my
hands and say that they are uncomparable.

After this change, I have 100% coverage of all of the branches in the
ordering implementations. Note that the ordering is unspecified between
AndMatcher, NotMatcher and OrMatcher. This means that, for example,
{ **/!foo, **/(foo && bar) } and { **/(foo && bar), **/!foo }
are both possible orderings depending on the initial ordering of these
two globs. However,
{ **/bar/!foo, **/foo/!bar }
is the only possible ordering for those two globs. In other words,
ordering is preserved up until two components differ only by an
(And|Not|Or)Matcher.

Here are screenshots showing that all of the branches are covered in jacoco:
Screen Shot 2019-08-30 at 11 05 26 AM
Screen Shot 2019-08-30 at 11 04 52 AM
By comparison, here was the previous jacoco coverage:
Screen Shot 2019-08-30 at 11 09 37 AM
(whoops)

The Ordering[Matcher] implementation violated the general ordering
contract. sbt/sbt#4970 was still present
with sbt 1.3.0-RC5. This commit fixes the contract of Ordering[Matcher]
so that it should be sound for all Matcher instances.

Using jacoco, I was able to identify which branches of
the matchers were not being hit. After this change, all of the branches
were hit. While I'm not sure what was specifically causing the issue in
sbt/4970, I suspect that somewhere was an AndFilter, OrFilter or
NotFilter. The conversion in Globs from AndFilter, NotFilter and OrFilter to
Matcher created AndMatcher, NotMatcher and OrMatcher. The contract
between these matchers was not sound in the Ordering[Matcher]
implementation. Instead of trying to sort these, I just throw up my
hands and say that they are uncomparable.

After this change, I have 100% coverage of all of the branches in the
ordering implementations. Note that the ordering is unspecified between
AndMatcher, NotMatcher and OrMatcher. This means that, for example,
{ **/!foo, **/(foo && bar) } and { **/(foo && bar), **/!foo }
are both possible orderings depending on the initial ordering of these
two globs. However,
{ **/bar/!foo, **/foo/!bar }
is the only possible ordering for those two globs. In other words,
ordering is preserved up until two components differ only by an
(And|Not|Or)Matcher.
@eed3si9n eed3si9n merged commit 40d5993 into sbt:develop Aug 30, 2019
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.

2 participants