Repository: scala-steward-org/scala-steward (scala)

Issue:
Feature idea: don't close superseded PRs if they were manually updated
E.g. in https://github.com/pitgull/pitgull/pull/102 I manually pushed a change to fix the compilation after the update. The superseding PR obviously didn't have that, so I'll need to repeat that work.

Pull request:
This changes the logic for closing obsolete PRs such that only PRs which
were not manually updated are closed and the corresponding update branch
is deleted. If an obsolete update branch contains commits by authors
other than Scala Steward, only the "Superseded by" comment is posted to the
old PR.

Closes: #1871

Patch:
diff --git a/modules/core/src/main/scala/org/scalasteward/core/git/Branch.scala b/modules/core/src/main/scala/org/scalasteward/core/git/Branch.scala
index fc3e23d543..5812e9fb2c 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/git/Branch.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/git/Branch.scala
@@ -18,7 +18,9 @@ package org.scalasteward.core.git
 
 import io.circe.{Decoder, Encoder}
 
-final case class Branch(name: String)
+final case class Branch(name: String) {
+  def withPrefix(prefix: String): Branch = Branch(prefix + name)
+}
 
 object Branch {
   val head: Branch = Branch("HEAD")
diff --git a/modules/core/src/main/scala/org/scalasteward/core/git/FileGitAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/git/FileGitAlg.scala
index 7b1f00dec9..0082b57d45 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/git/FileGitAlg.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/git/FileGitAlg.scala
@@ -32,7 +32,10 @@ final class FileGitAlg[F[_]](config: GitCfg)(implicit
     F: MonadCancelThrow[F]
 ) extends GenGitAlg[F, File] {
   override def branchAuthors(repo: File, branch: Branch, base: Branch): F[List[String]] =
-    git("log", "--pretty=format:'%an'", dotdot(base, branch))(repo)
+    git("log", "--pretty=format:'%an'", dotdot(base, branch))(repo).map(_.distinct)
+
+  override def branchExists(repo: File, branch: Branch): F[Boolean] =
+    git("branch", "--list", "--no-color", "--all", branch.name)(repo).map(_.mkString.trim.nonEmpty)
 
   override def checkoutBranch(repo: File, branch: Branch): F[Unit] =
     git("checkout", branch.name)(repo).void
@@ -58,13 +61,13 @@ final class FileGitAlg[F[_]](config: GitCfg)(implicit
   override def createBranch(repo: File, branch: Branch): F[Unit] =
     git("checkout", "-b", branch.name)(repo).void
 
-  override def removeBranch(repo: File, branch: Branch): F[Unit] =
-    git("push", "origin", "--delete", branch.name)(repo).void
-
   override def currentBranch(repo: File): F[Branch] =
     git("rev-parse", "--abbrev-ref", Branch.head.name)(repo)
       .map(lines => Branch(lines.mkString.trim))
 
+  override def deleteRemoteBranch(repo: File, branch: Branch): F[Unit] =
+    git("push", "origin", "--delete", branch.name)(repo).void
+
   override def discardChanges(repo: File): F[Unit] =
     git("checkout", "--", ".")(repo).void
 
diff --git a/modules/core/src/main/scala/org/scalasteward/core/git/GenGitAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/git/GenGitAlg.scala
index 19d0b6a5d7..1766dd4b33 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/git/GenGitAlg.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/git/GenGitAlg.scala
@@ -26,6 +26,8 @@ import org.scalasteward.core.io.{FileAlg, ProcessAlg, WorkspaceAlg}
 trait GenGitAlg[F[_], Repo] {
   def branchAuthors(repo: Repo, branch: Branch, base: Branch): F[List[String]]
 
+  def branchExists(repo: Repo, branch: Branch): F[Boolean]
+
   def checkoutBranch(repo: Repo, branch: Branch): F[Unit]
 
   def clone(repo: Repo, url: Uri): F[Unit]
@@ -38,10 +40,10 @@ trait GenGitAlg[F[_], Repo] {
 
   def createBranch(repo: Repo, branch: Branch): F[Unit]
 
-  def removeBranch(repo: Repo, branch: Branch): F[Unit]
-
   def currentBranch(repo: Repo): F[Branch]
 
+  def deleteRemoteBranch(repo: Repo, branch: Branch): F[Unit]
+
   /** Discards unstaged changes. */
   def discardChanges(repo: Repo): F[Unit]
 
@@ -83,6 +85,9 @@ trait GenGitAlg[F[_], Repo] {
       override def branchAuthors(repo: A, branch: Branch, base: Branch): F[List[String]] =
         f(repo).flatMap(self.branchAuthors(_, branch, base))
 
+      override def branchExists(repo: A, branch: Branch): F[Boolean] =
+        f(repo).flatMap(self.branchExists(_, branch))
+
       override def checkoutBranch(repo: A, branch: Branch): F[Unit] =
         f(repo).flatMap(self.checkoutBranch(_, branch))
 
@@ -101,12 +106,12 @@ trait GenGitAlg[F[_], Repo] {
       override def createBranch(repo: A, branch: Branch): F[Unit] =
         f(repo).flatMap(self.createBranch(_, branch))
 
-      override def removeBranch(repo: A, branch: Branch): F[Unit] =
-        f(repo).flatMap(self.removeBranch(_, branch))
-
       override def currentBranch(repo: A): F[Branch] =
         f(repo).flatMap(self.currentBranch)
 
+      override def deleteRemoteBranch(repo: A, branch: Branch): F[Unit] =
+        f(repo).flatMap(self.deleteRemoteBranch(_, branch))
+
       override def discardChanges(repo: A): F[Unit] =
         f(repo).flatMap(self.discardChanges)
 
diff --git a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala
index b7408d4154..2fa92f36bb 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala
@@ -76,11 +76,7 @@ final class NurtureAlg[F[_]](config: Config)(implicit
         update => {
           val updateData =
             UpdateData(data, fork, update, baseBranch, baseSha1, git.branchFor(update))
-          processUpdate(updateData).flatMap {
-            case result @ Created(newPrNumber) =>
-              closeObsoletePullRequests(updateData, newPrNumber).as[ProcessResult](result)
-            case result @ _ => F.pure(result)
-          }
+          processUpdate(updateData)
         },
         data.config.updates.limit
       )
@@ -94,11 +90,14 @@ final class NurtureAlg[F[_]](config: Config)(implicit
       result <- pullRequests.headOption match {
         case Some(pr) if pr.isClosed =>
           logger.info(s"PR ${pr.html_url} is closed") >>
-            removeRemoteBranch(data.repo, data.updateBranch).as(Ignored)
+            deleteRemoteBranch(data.repo, data.updateBranch).as(Ignored)
         case Some(pr) =>
           logger.info(s"Found PR ${pr.html_url}") >> updatePullRequest(data)
         case None =>
-          applyNewUpdate(data)
+          applyNewUpdate(data).flatTap {
+            case Created(newPrNumber) => closeObsoletePullRequests(data, newPrNumber)
+            case _                    => F.unit
+          }
       }
       _ <- pullRequests.headOption.traverse_ { pr =>
         pullRequestRepository.createOrUpdate(
@@ -115,12 +114,12 @@ final class NurtureAlg[F[_]](config: Config)(implicit
   def closeObsoletePullRequests(data: UpdateData, newNumber: PullRequestNumber): F[Unit] =
     pullRequestRepository.getObsoleteOpenPullRequests(data.repo, data.update).flatMap {
       _.traverse_ { case (oldNumber, oldUrl, oldUpdate) =>
-        closeObsoletePullRequest(data.repo, oldUpdate, oldUrl, oldNumber, newNumber)
+        closeObsoletePullRequest(data, oldUpdate, oldUrl, oldNumber, newNumber)
       }
     }
 
   private def closeObsoletePullRequest(
-      repo: Repo,
+      data: UpdateData,
       oldUpdate: Update,
       oldUrl: Uri,
       oldNumber: PullRequestNumber,
@@ -130,17 +129,28 @@ final class NurtureAlg[F[_]](config: Config)(implicit
       s"Closing obsolete PR ${oldUrl.renderString} for ${oldUpdate.show}"
     ) {
       for {
-        _ <- pullRequestRepository.changeState(repo, oldUrl, PullRequestState.Closed)
+        _ <- pullRequestRepository.changeState(data.repo, oldUrl, PullRequestState.Closed)
         comment = s"Superseded by ${vcsApiAlg.referencePullRequest(newNumber)}."
-        _ <- vcsApiAlg.commentPullRequest(repo, oldNumber, comment)
-        _ <- vcsApiAlg.closePullRequest(repo, oldNumber)
-        _ <- removeRemoteBranch(repo, git.branchFor(oldUpdate))
+        _ <- vcsApiAlg.commentPullRequest(data.repo, oldNumber, comment)
+        oldBranch = git.branchFor(oldUpdate)
+        oldRemoteBranch = oldBranch.withPrefix("origin/")
+        oldBranchExists <- gitAlg.branchExists(data.repo, oldRemoteBranch)
+        authors <-
+          if (oldBranchExists) gitAlg.branchAuthors(data.repo, oldRemoteBranch, data.baseBranch)
+          else List.empty.pure[F]
+        _ <-
+          if (authors.size <= 1) for {
+            _ <- vcsApiAlg.closePullRequest(data.repo, oldNumber)
+            _ <- deleteRemoteBranch(data.repo, oldBranch)
+          } yield ()
+          else F.unit
       } yield ()
     }
 
-  private def removeRemoteBranch(repo: Repo, branch: Branch): F[Unit] =
-    logger.attemptWarn.log_(s"Removing remote branch ${branch.name} failed") {
-      gitAlg.removeBranch(repo, branch)
+  private def deleteRemoteBranch(repo: Repo, branch: Branch): F[Unit] =
+    logger.attemptWarn.log_(s"Deleting remote branch ${branch.name} failed") {
+      val remoteBranch = branch.withPrefix("origin/")
+      gitAlg.branchExists(repo, remoteBranch).ifM(gitAlg.deleteRemoteBranch(repo, branch), F.unit)
     }
 
   def applyNewUpdate(data: UpdateData): F[ProcessResult] =
@@ -215,9 +225,8 @@ final class NurtureAlg[F[_]](config: Config)(implicit
       case true => (false, "PR has been merged").pure[F]
       case false =>
         gitAlg.branchAuthors(data.repo, data.updateBranch, data.baseBranch).flatMap { authors =>
-          val distinctAuthors = authors.distinct
-          if (distinctAuthors.length >= 2)
-            (false, s"PR has commits by ${distinctAuthors.mkString(", ")}").pure[F]
+          if (authors.length >= 2)
+            (false, s"PR has commits by ${authors.mkString(", ")}").pure[F]
           else if (data.repoConfig.updatePullRequestsOrDefault === PullRequestUpdateStrategy.Always)
             (true, "PR update strategy is set to always").pure[F]
           else


Test patch:
diff --git a/modules/core/src/test/scala/org/scalasteward/core/git/FileGitAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/git/FileGitAlgTest.scala
index e811db2ab9..c00fa40536 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/git/FileGitAlgTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/git/FileGitAlgTest.scala
@@ -31,6 +31,18 @@ class FileGitAlgTest extends FunSuite {
     assertEquals(p.unsafeRunSync(), List("'Bot Doe'"))
   }
 
+  test("branchExists") {
+    val repo = rootDir / "branchExists"
+    val (foo, bar) = (Branch("foo"), Branch("bar"))
+    val p = for {
+      _ <- supplement.createRepo(repo)
+      _ <- ioGitAlg.createBranch(repo, foo)
+      b1 <- ioGitAlg.branchExists(repo, foo)
+      b2 <- ioGitAlg.branchExists(repo, bar)
+    } yield (b1, b2)
+    assertEquals(p.unsafeRunSync(), (true, false))
+  }
+
   test("cloneExists") {
     val repo = rootDir / "cloneExists"
     val p = for {
@@ -63,7 +75,6 @@ class FileGitAlgTest extends FunSuite {
     val repo = rootDir / "currentBranch"
     val p = for {
       _ <- supplement.createRepo(repo)
-      _ <- supplement.git("commit", "--allow-empty", "-m", "Initial commit")(repo)
       branch <- ioGitAlg.currentBranch(repo)
       _ <- ioGitAlg.latestSha1(repo, branch)
     } yield branch
@@ -164,6 +175,7 @@ object FileGitAlgTest {
         _ <- fileAlg.ensureExists(repo)
         _ <- git("init", ".")(repo)
         _ <- gitAlg.setAuthor(repo, config.gitCfg.gitAuthor)
+        _ <- git("commit", "--allow-empty", "-m", "Initial commit")(repo)
       } yield ()
 
     def createConflict(repo: File): F[Unit] =