Repository: scala-steward-org/scala-steward
Pull request #2254 (2021-09-28 13:44:49)

Do not close old PRs that were manually updated
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

codecov[bot]: # [Codecov](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org) Report
> Merging [#2254](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org) (abbe59e) into [master](https://codecov.io/gh/scala-steward-org/scala-steward/commit/06f6c27713838478ac211caebd12b55c48f29e7c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org) (06f6c27) will **decrease** coverage by `0.42%`.
> The diff coverage is `9.37%`.

[![Impacted file tree graph](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254/graphs/tree.svg?width=650&height=150&src=pr&token=udzMsc1Dne&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org)](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org)

```diff
@@            Coverage Diff             @@
##           master    #2254      +/-   ##
==========================================
- Coverage   78.29%   77.86%   -0.43%     
==========================================
  Files         135      135              
  Lines        2271     2286      +15     
  Branches       55       55              
==========================================
+ Hits         1778     1780       +2     
- Misses        493      506      +13     
```


| [Impacted Files](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org) | Coverage Δ | |
|---|---|---|
| [...org/scalasteward/core/application/StewardAlg.scala](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org#diff-bW9kdWxlcy9jb3JlL3NyYy9tYWluL3NjYWxhL29yZy9zY2FsYXN0ZXdhcmQvY29yZS9hcHBsaWNhdGlvbi9TdGV3YXJkQWxnLnNjYWxh) | `44.11% <0.00%> (ø)` | |
| [.../main/scala/org/scalasteward/core/git/Branch.scala](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org#diff-bW9kdWxlcy9jb3JlL3NyYy9tYWluL3NjYWxhL29yZy9zY2FsYXN0ZXdhcmQvY29yZS9naXQvQnJhbmNoLnNjYWxh) | `75.00% <0.00%> (-25.00%)` | :arrow_down: |
| [...ala/org/scalasteward/core/nurture/NurtureAlg.scala](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org#diff-bW9kdWxlcy9jb3JlL3NyYy9tYWluL3NjYWxhL29yZy9zY2FsYXN0ZXdhcmQvY29yZS9udXJ0dXJlL051cnR1cmVBbGcuc2NhbGE=) | `0.00% <0.00%> (ø)` | |
| [...main/scala/org/scalasteward/core/util/logger.scala](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org#diff-bW9kdWxlcy9jb3JlL3NyYy9tYWluL3NjYWxhL29yZy9zY2FsYXN0ZXdhcmQvY29yZS91dGlsL2xvZ2dlci5zY2FsYQ==) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
| [...in/scala/org/scalasteward/core/git/GenGitAlg.scala](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org#diff-bW9kdWxlcy9jb3JlL3NyYy9tYWluL3NjYWxhL29yZy9zY2FsYXN0ZXdhcmQvY29yZS9naXQvR2VuR2l0QWxnLnNjYWxh) | `92.30% <50.00%> (+0.30%)` | :arrow_up: |
| [...n/scala/org/scalasteward/core/git/FileGitAlg.scala](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org#diff-bW9kdWxlcy9jb3JlL3NyYy9tYWluL3NjYWxhL29yZy9zY2FsYXN0ZXdhcmQvY29yZS9naXQvRmlsZUdpdEFsZy5zY2FsYQ==) | `98.27% <66.66%> (+0.03%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org). Last update [06f6c27...abbe59e](https://codecov.io/gh/scala-steward-org/scala-steward/pull/2254?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scala-steward-org).


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 fc3e23d5..5812e9fb 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 7b1f00de..0082b57d 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 19d0b6a5..1766dd4b 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 b7408d41..2fa92f36 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 e811db2a..c00fa405 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] =