Merge pull request #5361 from eatkins/no-escape

Cleanup scripted quotation mark parsing

internal/util-scripted/src/main/scala/sbt/internal/scripted/ScriptedTests.scala

@@ -58,8 +58,11 @@ object ScriptedRunnerImpl {
 final class ScriptedTests(
     resourceBaseDirectory: File,
     bufferLog: Boolean,
-    handlersProvider: HandlersProvider
+    handlersProvider: HandlersProvider,
+    stripQuotes: Boolean
 ) {
+  def this(resourceBaseDirectory: File, bufferLog: Boolean, handlersProvider: HandlersProvider) =
+    this(resourceBaseDirectory, bufferLog, handlersProvider, true)
   private val testResources = new Resources(resourceBaseDirectory)
   private val consoleAppender: ConsoleAppender = ConsoleAppender()
 
@@ -127,10 +130,10 @@ final class ScriptedTests(
     }
     val pendingString = if (pending) " [PENDING]" else ""
 
-    def runTest() = {
+    def runTest(): Unit = {
       val run = new ScriptRunner
       val parser = createParser()
-      run(parser.parse(file))
+      run(parser.parse(file, stripQuotes))
     }
     def testFailed(): Unit = {
       if (pending) buffered.clearBuffer() else buffered.stopBuffer()

internal/util-scripted/src/main/scala/sbt/internal/scripted/TestScriptParser.scala

@@ -50,11 +50,22 @@ class TestScriptParser(handlers: Map[Char, StatementHandler]) extends RegexParse
   if (handlers.keys.exists(key => key == '+' || key == '-'))
     sys.error("Start characters cannot be '+' or '-'")
 
+  @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0")
   def parse(scriptFile: File): List[(StatementHandler, Statement)] =
-    parse(read(scriptFile), Some(scriptFile.getAbsolutePath))
+    parse(scriptFile, stripQuotes = true)
-  def parse(script: String): List[(StatementHandler, Statement)] = parse(script, None)
+  def parse(scriptFile: File, stripQuotes: Boolean): List[(StatementHandler, Statement)] =
-  private def parse(script: String, label: Option[String]): List[(StatementHandler, Statement)] = {
+    parse(read(scriptFile), Some(scriptFile.getAbsolutePath), stripQuotes)
-    parseAll(statements, script) match {
+  @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0")
+  def parse(script: String): List[(StatementHandler, Statement)] =
+    parse(script, None, stripQuotes = true)
+  def parse(script: String, stripQuotes: Boolean): List[(StatementHandler, Statement)] =
+    parse(script, None, stripQuotes)
+  private def parse(
+      script: String,
+      label: Option[String],
+      stripQuotes: Boolean
+  ): List[(StatementHandler, Statement)] = {
+    parseAll(statements(stripQuotes), script) match {
       case Success(result, next) => result
       case err: NoSuccess => {
         val labelString = label.map("'" + _ + "' ").getOrElse("")
@@ -63,15 +74,21 @@ class TestScriptParser(handlers: Map[Char, StatementHandler]) extends RegexParse
     }
   }
 
+  @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0")
   lazy val statements = rep1(space ~> statement <~ newline)
+  def statements(stripQuotes: Boolean): Parser[List[(StatementHandler, Statement)]] =
+    rep1(space ~> statement(stripQuotes) <~ newline)
 
-  def statement: Parser[(StatementHandler, Statement)] = {
+  @deprecated("Use variant that specifies whether to strip quotes or not", "1.4.0")
+  def statement: Parser[(StatementHandler, Statement)] = statement(stripQuotes = true)
+  def statement(stripQuotes: Boolean): Parser[(StatementHandler, Statement)] = {
     trait PositionalStatement extends Positional {
       def tuple: (StatementHandler, Statement)
     }
     positioned {
-      val command = (word | err("expected command"))
+      val w = if (stripQuotes) word else rawWord
-      val arguments = rep(space ~> (word | failure("expected argument")))
+      val command = w | err("expected command")
+      val arguments = rep(space ~> w | failure("expected argument"))
       (successParser ~ (space ~> startCharacterParser <~ space) ~! command ~! arguments) ^^ {
         case successExpected ~ start ~ command ~ arguments =>
           new PositionalStatement {
@@ -86,7 +103,9 @@ class TestScriptParser(handlers: Map[Char, StatementHandler]) extends RegexParse
   def space: Parser[String] = """[ \t]*""".r
 
   lazy val word: Parser[String] =
-    ("\'" ~> "[^'\n\r]*".r <~ "\'") | ("\"" ~> "[^\"\n\r]*".r <~ "\"") | WordRegex
+    ("\'" ~> "[^'\n\r]*".r <~ "\'") | "\"" ~> "[^\"\n\r]*".r <~ "\'" | WordRegex
+  private lazy val rawWord: Parser[String] =
+    ("\'" ~> "[^'\n\r]*".r <~ "\'") | "\"[^\"\n\r]*\"".r | WordRegex
 
   def startCharacterParser: Parser[Char] =
     elem("start character", handlers.contains _) |

sbt/src/sbt-test/actions/doc/test

@@ -1,33 +1,33 @@
 -> doc
 
-> 'set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filterNot(_.getName contains "B") }'
+> set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filterNot(_.getName contains "B") }
 
 # hybrid project, only scaladoc run
 > doc
-$ exists "target/api/index.js"
+$ exists target/api/index.js
-$ absent "target/api/scala"
+$ absent target/api/scala
-$ absent "target/api/java"
+$ absent target/api/java
 
-> 'set sources in (Compile, doc) := { val src = (sources in (Compile, doc)).value; src.filterNot(_.getName endsWith ".java") }'
+> set sources in (Compile, doc) := { val src = (sources in (Compile, doc)).value; src.filterNot(_.getName endsWith ".java") }
 
 # compile task is superfluous. Since doc task preceded by compile task has been problematic due to scala
 # compiler's way of handling empty classpath. We have it here to test that our workaround works.
-> ; clean ; compile ; doc
+> clean ; compile ; doc
 
 # pure scala project, only scaladoc at top level
-$ exists "target/api/index.js"
+$ exists target/api/index.js
-$ absent "target/api/package-list"
+$ absent target/api/package-list
-$ absent "target/api/scala"
+$ absent target/api/scala
-$ absent "target/api/java"
+$ absent target/api/java
 
-> 'set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filter(_.getName endsWith ".java") }'
+> set sources in (Compile, doc) := { val src = (sources in Compile).value; src.filter(_.getName endsWith ".java") }
 
-> ; clean ; doc
+> clean ; doc
 
 # pure java project, only javadoc at top level
-$ exists "target/api/index.html"
+$ exists target/api/index.html
-$ absent "target/api/index.js"
+$ absent target/api/index.js
 
 # pending
-# $ absent "target/api/scala"
+# $ absent target/api/scala
-# $ absent "target/api/java"
+# $ absent target/api/java

sbt/src/sbt-test/package/lazy-name/test

@@ -4,19 +4,19 @@
 # much purpose other than checking that the 'set' command
 # re-evaluates the project data.
 
-> 'set name := "lazy-package-name"'
+> set name := "lazy-package-name"
 > set crossPaths := false
 
-> 'set version := "0.1.1"'
+> set version := "0.1.1"
 > package
-$ exists "target/lazy-package-name-0.1.1.jar"
+$ exists target/lazy-package-name-0.1.1.jar
 > clean
 
-> 'set version := "0.1.2"'
+> set version := "0.1.2"
 > package
-$ exists "target/lazy-package-name-0.1.2.jar"
+$ exists target/lazy-package-name-0.1.2.jar
 > clean
 
-> 'set version := "0.1.3"'
+> set version := "0.1.3"
 > package
-$ exists "target/lazy-package-name-0.1.3.jar"
+$ exists target/lazy-package-name-0.1.3.jar

sbt/src/sbt-test/package/manifest/test

@@ -1,4 +1,4 @@
 > package
-$ exists "./target/jar-manifest-test-0.2.jar"
+$ exists ./target/jar-manifest-test-0.2.jar
-$ exec java -jar "./target/jar-manifest-test-0.2.jar"
+$ exec java -jar ./target/jar-manifest-test-0.2.jar
 > run

sbt/src/sbt-test/package/resources/test

@@ -10,7 +10,7 @@
 
 # This should fail because sbt should include the resource in the jar but it won't have the right
 # directory structure
--$ exec java -jar "./target/main-resources-test-0.1.jar"
+-$ exec java -jar ./target/main-resources-test-0.1.jar
 
 # Give the resource the right directory structure
 $ mkdir src/main/resources/jartest
@@ -27,4 +27,4 @@ $ delete src/main/resources/main_resource_test
 > package
 
 # This should succeed because sbt should include the resource in the jar with the right directory structure
-$ exec java -jar "./target/main-resources-test-0.1.jar"
+$ exec java -jar ./target/main-resources-test-0.1.jar

sbt/src/sbt-test/run/fork/test

@@ -1,18 +1,18 @@
-> run "fork"
+> run fork
 $ exists flag
 $ delete flag
 
-$ mkdir "forked"
+$ mkdir forked
 > set fork := true
-> 'set baseDirectory in run := baseDirectory(_ / "forked").value'
+> set baseDirectory in run := baseDirectory(_ / "forked").value
 
-> run "forked"
+> run forked
 $ exists forked/flag
 $ absent flag
 $ delete forked/flag
 
-> 'set envVars += ("flag.name" -> "env.flag")'
+> set envVars += ("flag.name" -> "env.flag")
-> run "forked"
+> run forked
 $ exists forked/env.flag
 $ absent flag
 $ absent forked/flag

sbt/src/sbt-test/tests/arguments/build.sbt

@@ -2,8 +2,16 @@ val scalatest = "org.scalatest" %% "scalatest" % "3.0.5"
 
 ThisBuild / scalaVersion := "2.12.10"
 
+val foo = settingKey[Seq[String]]("foo")
+val checkFoo = inputKey[Unit]("check contents of foo")
+
 lazy val root = (project in file("."))
   .settings(
+    foo := Nil,
+    checkFoo := {
+      val arguments = Def.spaceDelimited("").parsed.mkString(" ")
+      assert(foo.value.contains(arguments))
+    },
     libraryDependencies += scalatest % Test,
     // testOptions in Test += Tests.Argument(TestFrameworks.ScalaTest, "-f", "result.txt", "-eNDXEHLO")
     testOptions in Configurations.Test ++= {

sbt/src/sbt-test/tests/arguments/test

@@ -24,3 +24,9 @@ $ delete success3
 $ touch failure3
 -> test:test
 $ delete failure3
+
+> set Compile / scalacOptions += "-Xfatal-warnings"
+
+> set foo += "an argument with spaces"
+
+> checkFoo an argument with spaces

scripted-sbt-redux/src/main/scala/sbt/scriptedtest/SbtHandler.scala

@@ -27,7 +27,7 @@ final class SbtHandler(remoteSbtCreator: RemoteSbtCreator) extends StatementHand
 
   def apply(command: String, arguments: List[String], i: Option[SbtInstance]): Option[SbtInstance] =
     onSbtInstance(i) { (_, server) =>
-      send((command :: arguments.map(escape)).mkString(" "), server)
+      send((command :: arguments).mkString(" "), server)
       receive(s"$command failed", server)
     }
 
@@ -80,12 +80,4 @@ final class SbtHandler(remoteSbtCreator: RemoteSbtCreator) extends StatementHand
     catch { case _: SocketException => throw new TestFailed("Remote sbt initialization failed") }
     p
   }
-
-  // if the argument contains spaces, enclose it in quotes, quoting backslashes and quotes
-  def escape(argument: String) = {
-    import java.util.regex.Pattern.{ quote => q }
-    if (argument.contains(" "))
-      "\"" + argument.replaceAll(q("""\"""), """\\""").replaceAll(q("\""), "\\\"") + "\""
-    else argument
-  }
 }

scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala

@@ -443,7 +443,7 @@ final class ScriptedTests(
     catching(classOf[TestException]).withApply(testFailed).andFinally(log.clear).apply {
       preScriptedHook(testDirectory)
       val parser = new TestScriptParser(handlers)
-      val handlersAndStatements = parser.parse(file)
+      val handlersAndStatements = parser.parse(file, stripQuotes = false)
       runner.apply(handlersAndStatements, states)
 
       // Handle successful tests