From 4281972f1a3232d1c2eb76e1197bbb02a142993a Mon Sep 17 00:00:00 2001
From: Ethan Atkins <ethan.atkins@gmail.com>
Date: Mon, 19 Nov 2018 07:55:14 -0800
Subject: [PATCH 1/4] Refactor multi parser

Prior to this commit, there was no unit testing of the parser for
multiple commands. I wanted to make some improvements to the parser, so
I reworked the implementation to be testable. This change also allows
the multiParserImpl method to be shared with Watched.watch, which I will
also update in a subsequent commit.

There also were no explicit scripted tests for multiple commands, so I
added one that I will augment in later commits.
---
 .../src/main/scala/sbt/BasicCommands.scala    |  7 ++--
 .../src/test/scala/sbt/MultiParserSpec.scala  | 36 +++++++++++++++++++
 .../sbt-test/actions/multi-command/build.sbt  | 14 ++++++++
 .../actions/multi-command/project/Build.scala | 18 ++++++++++
 sbt/src/sbt-test/actions/multi-command/test   |  9 +++++
 5 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 main-command/src/test/scala/sbt/MultiParserSpec.scala
 create mode 100644 sbt/src/sbt-test/actions/multi-command/build.sbt
 create mode 100644 sbt/src/sbt-test/actions/multi-command/project/Build.scala
 create mode 100644 sbt/src/sbt-test/actions/multi-command/test

diff --git a/main-command/src/main/scala/sbt/BasicCommands.scala b/main-command/src/main/scala/sbt/BasicCommands.scala
index 0e8c51a41..8d4b9c34d 100644
--- a/main-command/src/main/scala/sbt/BasicCommands.scala
+++ b/main-command/src/main/scala/sbt/BasicCommands.scala
@@ -154,15 +154,18 @@ object BasicCommands {
     state
   }
 
-  def multiParser(s: State): Parser[List[String]] = {
+  private[sbt] def multiParserImpl(state: Option[State]): Parser[List[String]] = {
     val nonSemi = token(charClass(_ != ';', "not ';'").+, hide = const(true))
     val semi = token(';' ~> OptSpace)
+    def commandParser = state.map(s => (s.combinedParser & nonSemi) | nonSemi).getOrElse(nonSemi)
     val part = semi flatMap (
-        _ => matched((s.combinedParser & nonSemi) | nonSemi) <~ token(OptSpace)
+        _ => matched(commandParser) <~ token(OptSpace)
     )
     (part map (_.trim)).+ map (_.toList)
   }
 
+  def multiParser(s: State): Parser[List[String]] = multiParserImpl(Some(s))
+
   def multiApplied(s: State): Parser[() => State] =
     Command.applyEffect(multiParser(s))(_ ::: s)
 
diff --git a/main-command/src/test/scala/sbt/MultiParserSpec.scala b/main-command/src/test/scala/sbt/MultiParserSpec.scala
new file mode 100644
index 000000000..e29eca571
--- /dev/null
+++ b/main-command/src/test/scala/sbt/MultiParserSpec.scala
@@ -0,0 +1,36 @@
+/*
+ * sbt
+ * Copyright 2011 - 2018, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+
+import org.scalatest.{ FlatSpec, Matchers }
+import sbt.internal.util.complete.Parser
+
+object MultiParserSpec {
+  val parser: Parser[Seq[String]] = BasicCommands.multiParserImpl(None)
+  implicit class StringOps(val s: String) {
+    def parse: Seq[String] = Parser.parse(s, parser).right.get
+  }
+}
+import MultiParserSpec._
+class MultiParserSpec extends FlatSpec with Matchers {
+  "parsing" should "parse single commands" in {
+    ";foo".parse shouldBe Seq("foo")
+    ";   foo".parse shouldBe Seq("foo")
+  }
+  it should "parse multiple commands" in {
+    ";foo;bar".parse shouldBe Seq("foo", "bar")
+  }
+  it should "parse single command with leading spaces" in {
+    ";     foo".parse shouldBe Seq("foo")
+  }
+  it should "parse multiple commands with leading spaces" in {
+    ";     foo;bar".parse shouldBe Seq("foo", "bar")
+    ";     foo;    bar".parse shouldBe Seq("foo", "bar")
+    ";foo; bar".parse shouldBe Seq("foo", "bar")
+  }
+}
diff --git a/sbt/src/sbt-test/actions/multi-command/build.sbt b/sbt/src/sbt-test/actions/multi-command/build.sbt
new file mode 100644
index 000000000..0aff2d364
--- /dev/null
+++ b/sbt/src/sbt-test/actions/multi-command/build.sbt
@@ -0,0 +1,14 @@
+import Build._
+
+organization := "sbt"
+
+name := "scripted-multi-command-parser"
+
+setStringValue := setStringValueImpl.evaluated
+
+checkStringValue := checkStringValueImpl.evaluated
+
+taskThatFails := {
+  throw new IllegalArgumentException("")
+  ()
+}
diff --git a/sbt/src/sbt-test/actions/multi-command/project/Build.scala b/sbt/src/sbt-test/actions/multi-command/project/Build.scala
new file mode 100644
index 000000000..5190dc191
--- /dev/null
+++ b/sbt/src/sbt-test/actions/multi-command/project/Build.scala
@@ -0,0 +1,18 @@
+import sbt._
+
+object Build {
+  private[this] var string: String = ""
+  private[this] val stringFile = file("string.txt")
+  val setStringValue = inputKey[Unit]("set a global string to a value")
+  val checkStringValue = inputKey[Unit]("check the value of a global")
+  val taskThatFails = taskKey[Unit]("this should fail")
+  def setStringValueImpl: Def.Initialize[InputTask[Unit]] = Def.inputTask {
+    string = Def.spaceDelimited().parsed.mkString(" ").trim
+    IO.write(stringFile, string)
+  }
+  def checkStringValueImpl: Def.Initialize[InputTask[Unit]] = Def.inputTask {
+    val actual = Def.spaceDelimited().parsed.mkString(" ").trim
+    assert(string == actual)
+    assert(IO.read(stringFile) == string)
+  }
+}
diff --git a/sbt/src/sbt-test/actions/multi-command/test b/sbt/src/sbt-test/actions/multi-command/test
new file mode 100644
index 000000000..a98fad58c
--- /dev/null
+++ b/sbt/src/sbt-test/actions/multi-command/test
@@ -0,0 +1,9 @@
+> ; setStringValue baz
+
+> ; checkStringValue baz
+
+> ; setStringValue foo; setStringValue bar
+
+> checkStringValue bar
+
+> ; setStringValue foo; setStringValue bar; setStringValue baz; checkStringValue baz

From 05e3a8609b18dcad45af6d885310b13ca5afa440 Mon Sep 17 00:00:00 2001
From: Ethan Atkins <ethan.atkins@gmail.com>
Date: Sun, 18 Nov 2018 09:23:02 -0800
Subject: [PATCH 2/4] Fix watch command parser

I discovered that when I ran multi-commands with '~' that if there was a
space between the ';' and the command, then the parsing of the command
would fail and the watch would abort. To fix this, I refactor
Watched.watch to use the multi command parser and, if that parser fails,
we fallback on a single command.
---
 main-command/src/main/scala/sbt/Watched.scala  |  8 ++++----
 sbt/src/sbt-test/watch/watch-parser/build.sbt  | 13 +++++++++++++
 .../watch/watch-parser/project/Build.scala     | 16 ++++++++++++++++
 sbt/src/sbt-test/watch/watch-parser/test       | 18 ++++++++++++++++++
 4 files changed, 51 insertions(+), 4 deletions(-)
 create mode 100644 sbt/src/sbt-test/watch/watch-parser/build.sbt
 create mode 100644 sbt/src/sbt-test/watch/watch-parser/project/Build.scala
 create mode 100644 sbt/src/sbt-test/watch/watch-parser/test

diff --git a/main-command/src/main/scala/sbt/Watched.scala b/main-command/src/main/scala/sbt/Watched.scala
index e6b851d14..082f4374d 100644
--- a/main-command/src/main/scala/sbt/Watched.scala
+++ b/main-command/src/main/scala/sbt/Watched.scala
@@ -21,7 +21,7 @@ import sbt.internal.LegacyWatched
 import sbt.internal.inc.Stamper
 import sbt.internal.io.{ EventMonitor, Source, WatchState }
 import sbt.internal.util.Types.const
-import sbt.internal.util.complete.DefaultParsers
+import sbt.internal.util.complete.{ DefaultParsers, Parser }
 import sbt.internal.util.{ AttributeKey, JLine }
 import sbt.io.FileEventMonitor.{ Creation, Deletion, Event, Update }
 import sbt.io._
@@ -279,9 +279,9 @@ object Watched {
       onFailure = Some(Exec(failureCommandName, None)),
       definedCommands = s0.definedCommands :+ onFail
     )
-    val commands = command.split(";") match {
-      case Array("", rest @ _*) => rest
-      case Array(cmd)           => Seq(cmd)
+    val commands = Parser.parse(command, BasicCommands.multiParserImpl(Some(s))) match {
+      case Left(_)  => command :: Nil
+      case Right(c) => c
     }
     val parser = Command.combine(s.definedCommands)(s)
     val tasks = commands.foldLeft(Nil: Seq[Either[String, () => Either[Exception, Boolean]]]) {
diff --git a/sbt/src/sbt-test/watch/watch-parser/build.sbt b/sbt/src/sbt-test/watch/watch-parser/build.sbt
new file mode 100644
index 000000000..c29f61af0
--- /dev/null
+++ b/sbt/src/sbt-test/watch/watch-parser/build.sbt
@@ -0,0 +1,13 @@
+import Build._
+
+organization := "sbt"
+
+name := "scripted-watch-parser"
+
+setStringValue := setStringValueImpl.evaluated
+
+checkStringValue := checkStringValueImpl.evaluated
+
+watchSources += file("string.txt")
+
+watchOnEvent := { _ => Watched.CancelWatch }
diff --git a/sbt/src/sbt-test/watch/watch-parser/project/Build.scala b/sbt/src/sbt-test/watch/watch-parser/project/Build.scala
new file mode 100644
index 000000000..19380e885
--- /dev/null
+++ b/sbt/src/sbt-test/watch/watch-parser/project/Build.scala
@@ -0,0 +1,16 @@
+import sbt._
+
+object Build {
+  private[this] var string: String = ""
+  private[this] val stringFile = file("string.txt")
+  val setStringValue = inputKey[Unit]("set a global string to a value")
+  val checkStringValue = inputKey[Unit]("check the value of a global")
+  def setStringValueImpl: Def.Initialize[InputTask[Unit]] = Def.inputTask {
+    string = Def.spaceDelimited().parsed.mkString(" ").trim
+    IO.write(stringFile, string)
+  }
+  def checkStringValueImpl: Def.Initialize[InputTask[Unit]] = Def.inputTask {
+    assert(string == Def.spaceDelimited().parsed.mkString(" ").trim)
+    assert(IO.read(stringFile) == string)
+  }
+}
\ No newline at end of file
diff --git a/sbt/src/sbt-test/watch/watch-parser/test b/sbt/src/sbt-test/watch/watch-parser/test
new file mode 100644
index 000000000..f14bef5e1
--- /dev/null
+++ b/sbt/src/sbt-test/watch/watch-parser/test
@@ -0,0 +1,18 @@
+> ~; setStringValue foo; setStringValue bar
+
+> checkStringValue bar
+
+> ~;setStringValue foo;setStringValue bar; checkStringValue bar
+
+> ~;   setStringValue foo;setStringValue bar; checkStringValue bar
+
+> ~; setStringValue foo; setStringValue bar; checkStringValue bar
+
+> ~ setStringValue foo
+
+> checkStringValue foo
+
+# All of the other tests have involved input tasks, so include commands with regular tasks as well.
+> ~; compile; setStringValue baz; checkStringValue baz
+# No trailing semicolons are allowed
+-> ~; compile; setStringValue baz; checkStringValue baz;

From 51d986d751866491acf408ca7882b48abeecd06d Mon Sep 17 00:00:00 2001
From: Ethan Atkins <ethan.atkins@gmail.com>
Date: Mon, 19 Nov 2018 08:36:48 -0800
Subject: [PATCH 3/4] Make multi command parser work with string literals

Presently the multi command parser doesn't work correctly if one of the
commands includes a string literal. For example, suppose that there is
an input task defined name "bash" that shells out and runs the input.
Then the following does not work with the current multi command parser:
; bash "rm target/classes/Foo.class; touch src/main/scala/Foo.scala"; comple
Note that this is a real use case that has caused me issues in the past.

The problem is that the semicolon inside of the quote gets interpreted
as a command separator token. To fix this, I rework the parser so that
it consumes string literals and doesn't modify them. By using
StringEscapable, I allow the string to contain quotation marks itself.

I couldn't write a scripted test for this because in a command like
`; foo "bar"; baz`, the quotes around bar seem to get stripped. This
could be fixed by adding an alternative to StringEscapable that matches
an escaped string, but that is more work than I'm willing to do right
now.
---
 main-command/src/main/scala/sbt/BasicCommands.scala | 13 ++++++++-----
 .../src/test/scala/sbt/MultiParserSpec.scala        | 12 ++++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/main-command/src/main/scala/sbt/BasicCommands.scala b/main-command/src/main/scala/sbt/BasicCommands.scala
index 8d4b9c34d..f647c9478 100644
--- a/main-command/src/main/scala/sbt/BasicCommands.scala
+++ b/main-command/src/main/scala/sbt/BasicCommands.scala
@@ -155,13 +155,16 @@ object BasicCommands {
   }
 
   private[sbt] def multiParserImpl(state: Option[State]): Parser[List[String]] = {
-    val nonSemi = token(charClass(_ != ';', "not ';'").+, hide = const(true))
+    val nonSemi = charClass(_ != ';', "not ';'")
     val semi = token(';' ~> OptSpace)
-    def commandParser = state.map(s => (s.combinedParser & nonSemi) | nonSemi).getOrElse(nonSemi)
-    val part = semi flatMap (
-        _ => matched(commandParser) <~ token(OptSpace)
+    val nonQuote = charClass(_ != '"', label = "not '\"'")
+    val cmdPart = token(
+      ((nonSemi & nonQuote).map(_.toString) | StringEscapable.map(c => s""""$c"""")).+,
+      hide = const(true)
     )
-    (part map (_.trim)).+ map (_.toList)
+    def commandParser = state.map(s => (s.combinedParser & cmdPart) | cmdPart).getOrElse(cmdPart)
+    val part = semi.flatMap(_ => matched(commandParser) <~ token(OptSpace)).map(_.trim)
+    part.+ map (_.toList)
   }
 
   def multiParser(s: State): Parser[List[String]] = multiParserImpl(Some(s))
diff --git a/main-command/src/test/scala/sbt/MultiParserSpec.scala b/main-command/src/test/scala/sbt/MultiParserSpec.scala
index e29eca571..06a30a179 100644
--- a/main-command/src/test/scala/sbt/MultiParserSpec.scala
+++ b/main-command/src/test/scala/sbt/MultiParserSpec.scala
@@ -33,4 +33,16 @@ class MultiParserSpec extends FlatSpec with Matchers {
     ";     foo;    bar".parse shouldBe Seq("foo", "bar")
     ";foo; bar".parse shouldBe Seq("foo", "bar")
   }
+  it should "parse command with string literal" in {
+    "; foo \"barbaz\"".parse shouldBe Seq("foo \"barbaz\"")
+    "; foo \"bar;baz\"".parse shouldBe Seq("foo \"bar;baz\"")
+    "; foo \"barbaz\"; bar".parse shouldBe Seq("foo \"barbaz\"", "bar")
+    "; foo \"barbaz\"; bar \"blah\"".parse shouldBe Seq("foo \"barbaz\"", "bar \"blah\"")
+    "; foo \"bar;baz\"; bar".parse shouldBe Seq("foo \"bar;baz\"", "bar")
+    "; foo \"bar;baz\"; bar \"buzz\"".parse shouldBe Seq("foo \"bar;baz\"", "bar \"buzz\"")
+    "; foo \"bar;baz\"; bar \"buzz;two\"".parse shouldBe Seq("foo \"bar;baz\"", "bar \"buzz;two\"")
+    """; foo "bar;\"baz\""; bar""".parse shouldBe Seq("""foo "bar;\"baz\""""", "bar")
+    """; setStringValue "foo;bar"; checkStringValue "foo;bar"""".parse shouldBe
+      Seq("""setStringValue "foo;bar"""", """checkStringValue "foo;bar"""")
+  }
 }

From c00cc3795399d945ce2bc3a54da80a1403ca60d0 Mon Sep 17 00:00:00 2001
From: Ethan Atkins <ethan.atkins@gmail.com>
Date: Mon, 19 Nov 2018 08:36:58 -0800
Subject: [PATCH 4/4] Do not require leading semicolon for multi command

It has long been a frustration of mine that it is necessary to prepend
multiple commands with a ';'. In this commit, I relax that restriction.
I had to reorder the command definitions so that multi comes before act.
This was because if the multi command did not have a leading semicolon,
then it would be handled by the action parser before the multi command
parser had a shot at it. Sadness ensued.
---
 .../src/main/scala/sbt/BasicCommands.scala      |  6 ++++--
 .../src/test/scala/sbt/MultiParserSpec.scala    | 17 +++++++++++++++++
 main/src/main/scala/sbt/Main.scala              |  1 +
 sbt/src/sbt-test/actions/multi-command/test     | 12 ++++++++++++
 sbt/src/sbt-test/watch/watch-parser/test        |  3 +++
 5 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/main-command/src/main/scala/sbt/BasicCommands.scala b/main-command/src/main/scala/sbt/BasicCommands.scala
index f647c9478..f5432333e 100644
--- a/main-command/src/main/scala/sbt/BasicCommands.scala
+++ b/main-command/src/main/scala/sbt/BasicCommands.scala
@@ -39,7 +39,6 @@ object BasicCommands {
     ignore,
     help,
     completionsCommand,
-    multi,
     ifLast,
     append,
     setOnFailure,
@@ -164,7 +163,10 @@ object BasicCommands {
     )
     def commandParser = state.map(s => (s.combinedParser & cmdPart) | cmdPart).getOrElse(cmdPart)
     val part = semi.flatMap(_ => matched(commandParser) <~ token(OptSpace)).map(_.trim)
-    part.+ map (_.toList)
+    (cmdPart.? ~ part.+).map {
+      case (Some(h), t) => h.mkString.trim +: t.toList
+      case (_, t)       => t.toList
+    }
   }
 
   def multiParser(s: State): Parser[List[String]] = multiParserImpl(Some(s))
diff --git a/main-command/src/test/scala/sbt/MultiParserSpec.scala b/main-command/src/test/scala/sbt/MultiParserSpec.scala
index 06a30a179..ec18b1c58 100644
--- a/main-command/src/test/scala/sbt/MultiParserSpec.scala
+++ b/main-command/src/test/scala/sbt/MultiParserSpec.scala
@@ -14,6 +14,7 @@ object MultiParserSpec {
   val parser: Parser[Seq[String]] = BasicCommands.multiParserImpl(None)
   implicit class StringOps(val s: String) {
     def parse: Seq[String] = Parser.parse(s, parser).right.get
+    def parseEither: Either[String, Seq[String]] = Parser.parse(s, parser)
   }
 }
 import MultiParserSpec._
@@ -45,4 +46,20 @@ class MultiParserSpec extends FlatSpec with Matchers {
     """; setStringValue "foo;bar"; checkStringValue "foo;bar"""".parse shouldBe
       Seq("""setStringValue "foo;bar"""", """checkStringValue "foo;bar"""")
   }
+  it should "parse commands without leading ';'" in {
+    "setStringValue foo; setStringValue bar".parse shouldBe Seq(
+      "setStringValue foo",
+      "setStringValue bar"
+    )
+    "foo; bar".parse shouldBe Seq("foo", "bar")
+    "foo bar ;bar".parse shouldBe Seq("foo bar", "bar")
+    "foo \"a;b\"; bar".parse shouldBe Seq("foo \"a;b\"", "bar")
+    " foo ; bar \"b;c\"".parse shouldBe Seq("foo", "bar \"b;c\"")
+  }
+  it should "not parse single commands without leading ';'" in {
+    "foo".parseEither shouldBe Left("Expected ';'\nfoo\n   ^")
+    "foo bar baz".parseEither shouldBe Left("Expected ';'\nfoo bar baz\n           ^")
+    "foo bar baz;".parseEither shouldBe
+      Left("Expected not ';'\nExpected '\"'\nfoo bar baz;\n            ^")
+  }
 }
diff --git a/main/src/main/scala/sbt/Main.scala b/main/src/main/scala/sbt/Main.scala
index 03dd44844..23c4b232e 100644
--- a/main/src/main/scala/sbt/Main.scala
+++ b/main/src/main/scala/sbt/Main.scala
@@ -241,6 +241,7 @@ object BuiltinCommands {
       export,
       boot,
       initialize,
+      BasicCommands.multi,
       act,
       continuous,
       flushFileTreeRepository
diff --git a/sbt/src/sbt-test/actions/multi-command/test b/sbt/src/sbt-test/actions/multi-command/test
index a98fad58c..8bc828c03 100644
--- a/sbt/src/sbt-test/actions/multi-command/test
+++ b/sbt/src/sbt-test/actions/multi-command/test
@@ -7,3 +7,15 @@
 > checkStringValue bar
 
 > ; setStringValue foo; setStringValue bar; setStringValue baz; checkStringValue baz
+
+> setStringValue foo; setStringValue bar
+
+> checkStringValue bar
+
+> setStringValue foo; setStringValue bar; setStringValue baz
+
+> checkStringValue baz
+
+-> setStringValue foo; taskThatFails; setStringValue bar
+
+> checkStringValue foo
diff --git a/sbt/src/sbt-test/watch/watch-parser/test b/sbt/src/sbt-test/watch/watch-parser/test
index f14bef5e1..63a58eb8a 100644
--- a/sbt/src/sbt-test/watch/watch-parser/test
+++ b/sbt/src/sbt-test/watch/watch-parser/test
@@ -8,6 +8,9 @@
 
 > ~; setStringValue foo; setStringValue bar; checkStringValue bar
 
+# no leading semicolon
+> ~ setStringValue foo; setStringValue bar; checkStringValue bar
+
 > ~ setStringValue foo
 
 > checkStringValue foo