GitOps: do not mask exceptions, propagate instead

If we try to format files based on git output, and git fails, scalafmt
will silently do nothing because a git failure is returned as an empty
list of files, as if no files are present or have been modified.

scalafmt-sysops/shared/src/main/scala/org/scalafmt/sysops/GitOps.scala

@@ -2,7 +2,7 @@ package org.scalafmt.sysops
 
 import scala.sys.process.ProcessLogger
 import scala.util.{Failure, Success, Try}
-import java.nio.file.Path
+import java.nio.file.{InvalidPathException, Path}
 
 object GitOps {
 
@@ -39,83 +39,79 @@ trait GitOps {
 
 private class GitOpsImpl(val workingDirectory: AbsoluteFile) extends GitOps {
 
-  private[scalafmt] def exec(cmd: Seq[String]): Try[Seq[String]] = {
+  private[scalafmt] def exec(cmd: Seq[String]): Seq[String] = {
+    execImpl(cmd).linesIterator.toSeq
+  }
+
+  private def execImpl(cmd: Seq[String]): String = {
     val errors = Seq.newBuilder[String]
     Try {
       val swallowStderr = ProcessLogger(_ => (), errors += _)
       sys.process.Process(cmd, workingDirectory.jfile).!!(swallowStderr)
-    }.fold(
-      e => {
+    } match {
+      case Failure(e) =>
         val err = errors.result().mkString("\n> ", "\n> ", "\n")
         val msg = s"Failed to run command ${cmd.mkString(" ")}. Error:$err"
-        Failure(new IllegalStateException(msg, e))
-      },
-      x => Success(x.trim.linesIterator.toSeq)
-    )
+        throw new IllegalStateException(msg, e)
+      case Success(x) => x.trim
+    }
   }
 
   override def lsTree(dir: AbsoluteFile*): Seq[AbsoluteFile] = {
     val cmd = Seq("git", "ls-files", "--full-name") ++ dir.map(_.toString())
-    rootDir.fold(Seq.empty[AbsoluteFile]) { rtDir =>
-      exec(cmd)
-        .map(_.map(rtDir.join).filter(_.isRegularFile))
-        .getOrElse(Seq.empty)
-    }
+    withRoot(exec(cmd)).filter(_.isRegularFile)
   }
 
-  override def rootDir: Option[AbsoluteFile] = {
+  override def rootDir: Option[AbsoluteFile] = tryRoot.toOption
+
+  private lazy val tryRoot: Try[AbsoluteFile] = {
     val cmd = Seq("git", "rev-parse", "--show-toplevel")
-    for {
-      Seq(rootPath) <- exec(cmd).toOption
-      file <- AbsoluteFile.fromPathIfAbsolute(rootPath)
-      if file.isDirectory
-    } yield file
+    Try(execImpl(cmd)).flatMap { x =>
+      val file = AbsoluteFile(FileOps.getFile(x))
+      if (file.isDirectory) Success(file)
+      else Failure(new InvalidPathException(x, "not a directory"))
+    }
   }
 
   override def diff(branch: String, dir: AbsoluteFile*): Seq[AbsoluteFile] = {
     val cmd = Seq("git", "diff", "--name-only", "--diff-filter=d", branch) ++
       (if (dir.isEmpty) Seq.empty else "--" +: dir.map(_.toString()))
-    for {
-      root <- rootDir.toSeq
-      path <- exec(cmd).getOrElse(Seq.empty)
-    } yield root.join(path)
+    withRoot(exec(cmd))
   }
 
   override def status(dir: AbsoluteFile*): Seq[AbsoluteFile] = {
     val cmd = Seq("git", "status", "--porcelain") ++
       (if (dir.isEmpty) Seq.empty else "--" +: dir.map(_.toString()))
-    for {
-      root <- rootDir.toSeq
-      statusLine <- exec(cmd).getOrElse(Seq.empty)
-      file <- getFileFromGitStatusLine(statusLine).toOption.toSeq
-    } yield root.join(file)
-  }.filter(_.exists)
+    withRoot(exec(cmd).map(getFileFromGitStatusLine)).filter(_.exists)
+  }
 
   private final val renameStatusCode = "R"
   private final val renameStatusArrowDelimiter = "-> "
 
+  private def withRoot(files: => Seq[String]): Seq[AbsoluteFile] = {
+    val root = tryRoot.get
+    files.map(root.join)
+  }
+
   /*
     Method extracts path to changed file from the singular line of the `git status --porcelain` output.
    (see https://git-scm.com/docs/git-status#_short_format)
    */
-  private def extractPathPart(s: String): Try[String] =
-    Try(
-      Option(s)
-        // Checks if the line status states the file was renamed (E.g: `R  ORIG_PATH -> PATH`)
-        .filter(_.substring(0, 2).contains(renameStatusCode))
-        // takes the part of the string after the `-> ` character sequence
-        .map(_.split(renameStatusArrowDelimiter).last)
-        // fallback for the regular status line (E.g.: `XY PATH`)
-        // Drops the status codes by splitting on white spaces then taking the tail of the result
-        // Restores spaces in the path by merging the tail back with white space separator
-        .getOrElse(s.trim.split(' ').tail.mkString(" "))
-        .trim
-    )
+  private def extractPathPart(s: String): String =
+    Option(s)
+      // Checks if the line status states the file was renamed (E.g: `R  ORIG_PATH -> PATH`)
+      .filter(_.substring(0, 2).contains(renameStatusCode))
+      // takes the part of the string after the `-> ` character sequence
+      .map(_.split(renameStatusArrowDelimiter).last)
+      // fallback for the regular status line (E.g.: `XY PATH`)
+      // Drops the status codes by splitting on white spaces then taking the tail of the result
+      // Restores spaces in the path by merging the tail back with white space separator
+      .getOrElse(s.trim.split(' ').tail.mkString(" "))
+      .trim
 
   private def trimQuotes(s: String): String =
     s.replaceAll("^\"|\"$", "")
 
-  private def getFileFromGitStatusLine(s: String): Try[Path] =
-    extractPathPart(s)
-      .map(pathRaw => FileOps.getFile(trimQuotes(pathRaw)))
+  private def getFileFromGitStatusLine(s: String): String =
+    trimQuotes(extractPathPart(s))
 }

scalafmt-tests/src/test/scala/org/scalafmt/sysops/GitOpsTest.scala

@@ -347,7 +347,7 @@ private object GitOpsTest {
       ops: GitOpsImpl,
       loc: Location
   ): Seq[String] =
-    ops.exec("git" +: cmd +: args) match {
+    Try(ops.exec("git" +: cmd +: args)) match {
       case Failure(f) => Assertions.fail(s"Failed git command. Got: $f")
       case Success(s) => s
     }