From 4d031e957562ebe47bf504ec2d72ee5ee07f2982 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Fri, 30 Aug 2019 10:18:03 -0700 Subject: [PATCH] Fix and comprehensively test glob ordering The Ordering[Matcher] implementation violated the general ordering contract. https://github.com/sbt/sbt/issues/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. --- io/src/main/scala/sbt/nio/file/Glob.scala | 42 +++++++-------- .../test/scala/sbt/nio/GlobOrderingSpec.scala | 52 +++++++++++++++++-- 2 files changed, 67 insertions(+), 27 deletions(-) diff --git a/io/src/main/scala/sbt/nio/file/Glob.scala b/io/src/main/scala/sbt/nio/file/Glob.scala index aa59843c..6060d3e9 100644 --- a/io/src/main/scala/sbt/nio/file/Glob.scala +++ b/io/src/main/scala/sbt/nio/file/Glob.scala @@ -262,13 +262,16 @@ object Glob { case Root(leftRoot) => right match { case Root(rightRoot) => leftRoot.compareTo(rightRoot) - case _: FullFileGlob => -1 - case _ => -compare(right, left) + case FullFileGlob(leftBase, _, _) => + leftRoot.compareTo(leftBase) match { + case 0 => -1 + case i => i + } + case _ => -compare(right, left) } case FullFileGlob(leftBase, _, _) => right match { case FullFileGlob(rightBase, _, _) => leftBase.compareTo(rightBase) - case _: Root => 1 case _ => -compare(right, left) } } @@ -777,35 +780,28 @@ object RelativeGlob { case spm: SingleComponentMatcher => y match { case that: SingleComponentMatcher => spm.glob.compareTo(that.glob) - case _ => 1 + case _ => -1 } case _: FunctionNameFilter => y match { - case _: FunctionNameFilter => 0 - case _ => 1 + case _: FunctionNameFilter => 0 + case _: NotMatcher | _: AndMatcher | _: OrMatcher => -1 + case _ => 1 } - case nm: NotMatcher => + case _: NotMatcher => y match { - case that: NotMatcher => compare(nm.matcher, that.matcher) - case _ => 1 + case _: NotMatcher | _: AndMatcher | _: OrMatcher => 0 + case _ => 1 } - case am: AndMatcher => + case _: AndMatcher => y match { - case that: AndMatcher => - compare(am.left, that.left) match { - case 0 => compare(am.right, that.right) - case _ => 1 - } - case _ => 1 + case _: NotMatcher | _: AndMatcher | _: OrMatcher => 0 + case _ => 1 } - case om: OrMatcher => + case _: OrMatcher => y match { - case that: OrMatcher => - compare(om.left, that.left) match { - case 0 => compare(om.right, that.right) - case _ => 1 - } - case _ => 1 + case _: NotMatcher | _: AndMatcher | _: OrMatcher => 0 + case _ => 1 } } } diff --git a/io/src/test/scala/sbt/nio/GlobOrderingSpec.scala b/io/src/test/scala/sbt/nio/GlobOrderingSpec.scala index 12133860..06a82ee4 100644 --- a/io/src/test/scala/sbt/nio/GlobOrderingSpec.scala +++ b/io/src/test/scala/sbt/nio/GlobOrderingSpec.scala @@ -13,8 +13,12 @@ package sbt.nio import java.io.File import org.scalatest.FlatSpec -import sbt.io.IO -import sbt.nio.file.{ Glob, RecursiveGlob } +import sbt.io.{ IO, SimpleFilter } +import sbt.nio.file.RelativeGlob.Matcher +import sbt.nio.file._ +import sbt.nio.file.syntax._ + +import scala.collection.JavaConverters._ class GlobOrderingSpec extends FlatSpec { "Globs" should "be ordered" in IO.withTemporaryDirectory { dir => @@ -22,13 +26,53 @@ class GlobOrderingSpec extends FlatSpec { assert(Seq(Glob(subdir), Glob(dir)).sorted == Seq(Glob(dir), Glob(subdir))) } they should "fall back on depth" in IO.withTemporaryDirectory { dir => - val recursive = Glob(dir, RecursiveGlob) + val recursive = Glob(dir, **) val nonRecursive = Glob(dir) assert(Seq(nonRecursive, recursive).sorted == Seq(recursive, nonRecursive)) } they should "not stack overflow" in IO.withTemporaryDirectory { dir => val exact = Glob(dir.toPath.resolve("foo")) - val fullFile = sbt.internal.nio.Globs(dir.toPath, true, sbt.io.HiddenFileFilter) + val fullFile = + sbt.internal.nio.Globs(dir.toPath / "foo", recursive = true, sbt.io.HiddenFileFilter) assert(Seq(exact, fullFile).sorted == Seq(exact, fullFile)) } + they should "not violate sorting contract" in IO.withTemporaryDirectory { dir => + val globs = Seq( + **, + ** / "foo", + ** / * / "bar", + ** / "foo" / "bar", + ** / Matcher.or(Matcher("foo"), Matcher("bar")), + ** / Matcher.and(Matcher("foo"), Matcher("bar")), + ** / Matcher.not(Matcher("foo")), + ** / Matcher.not(Matcher.and(Matcher("foo"), Matcher("bar"))), + ** / Matcher(_.contains("foo")), + ** / "foo" / Matcher(_.contains("bar")), + ** / "foo" / Matcher.not(Matcher(_.contains("bar"))), + ** / "foo" / "*.scala", + ** / **, + ** / *, + (** / "foo") / * / "*.scala", + (** / "foo") / * / "*.scala*", + (** / "foo") / ** / "*.scala*", + Glob(dir.toPath.resolve("foo")), + Glob(dir.toPath.resolve("bar")), + Glob(dir.toPath.resolve("bar").resolve("baz")), + sbt.internal.nio.Globs(dir.toPath, recursive = false, sbt.io.AllPassFilter), + sbt.internal.nio.Globs(dir.toPath, recursive = false, new SimpleFilter(_.contains("bar"))), + sbt.internal.nio.Globs(dir.toPath, recursive = true, new SimpleFilter(_.contains("baz"))), + sbt.internal.nio.Globs(dir.toPath, recursive = true, sbt.io.HiddenFileFilter), + sbt.internal.nio.Globs(dir.toPath / "foo", recursive = true, sbt.io.HiddenFileFilter), + sbt.internal.nio.Globs(dir.toPath, recursive = true, sbt.io.NothingFilter), + sbt.internal.nio.Globs(dir.toPath, recursive = true, new SimpleFilter(_.contains("foo"))), + Glob(dir.toPath / "scala", ** / "*.scala"), + Glob(dir.toPath / "java", ** / "*.java"), + Glob(dir.toPath / "scala", ** / "*.java"), + ) + val javaGlobs = new java.util.ArrayList((globs ++ globs ++ globs).asJava) + 1 to 1000 foreach { _ => + java.util.Collections.shuffle(javaGlobs) + javaGlobs.asScala.sorted + } + } }