Report commit failure caused by hook (#2984)

2026-01-05 07:09:56 +01:00 · 2022-01-30 21:38:15 +09:00
parent a49ffb07be
commit 70d584d56d
5 changed files with 166 additions and 122 deletions
--- a/src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala
+++ b/src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala
@@ -329,50 +329,12 @@ trait RepositoryViewerControllerBase extends ControllerBase {
  })

  post("/:owner/:repository/upload", uploadForm)(writableUsersOnly { (form, repository) =>
-    context.withLoginAccount {
-      loginAccount =>
-        val files = form.uploadFiles
-          .split("\n")
-          .map { line =>
-            val i = line.indexOf(':')
-            CommitFile(line.substring(0, i).trim, line.substring(i + 1).trim)
-          }
-          .toSeq
-
-        val newFiles = files.map { file =>
-          file.copy(name = if (form.path.length == 0) file.name else s"${form.path}/${file.name}")
-        }
-
-        if (form.newBranch) {
-          val newBranchName = createNewBranchForPullRequest(repository, form.branch, loginAccount)
-          val objectId = _commit(newBranchName, newFiles, loginAccount)
-          val issueId =
-            createIssueAndPullRequest(
-              repository,
-              form.branch,
-              newBranchName,
-              form.commit,
-              objectId.name,
-              form.message,
-              loginAccount
-            )
-          redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
-        } else {
-          _commit(form.branch, newFiles, loginAccount)
-          if (form.path.length == 0) {
-            redirect(s"/${repository.owner}/${repository.name}/tree/${form.branch}")
-          } else {
-            redirect(s"/${repository.owner}/${repository.name}/tree/${form.branch}/${form.path}")
-          }
-        }
-    }
-
    def _commit(
      branchName: String,
      //files: Seq[CommitFile],
      newFiles: Seq[CommitFile],
      loginAccount: Account
-    ): ObjectId = {
+    ): Either[String, ObjectId] = {
      commitFiles(
        repository = repository,
        branch = branchName,
@@ -399,6 +361,52 @@ trait RepositoryViewerControllerBase extends ControllerBase {
          }
      }
    }
+
+    context.withLoginAccount {
+      loginAccount =>
+        val files = form.uploadFiles
+          .split("\n")
+          .map { line =>
+            val i = line.indexOf(':')
+            CommitFile(line.substring(0, i).trim, line.substring(i + 1).trim)
+          }
+          .toSeq
+
+        val newFiles = files.map { file =>
+          file.copy(name = if (form.path.length == 0) file.name else s"${form.path}/${file.name}")
+        }
+
+        if (form.newBranch) {
+          val newBranchName = createNewBranchForPullRequest(repository, form.branch, loginAccount)
+          _commit(newBranchName, newFiles, loginAccount) match {
+            case Right(objectId) =>
+              val issueId =
+                createIssueAndPullRequest(
+                  repository,
+                  form.branch,
+                  newBranchName,
+                  form.commit,
+                  objectId.name,
+                  form.message,
+                  loginAccount
+                )
+              redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
+        } else {
+          _commit(form.branch, newFiles, loginAccount) match {
+            case Right(_) =>
+              if (form.path.length == 0) {
+                redirect(s"/${repository.owner}/${repository.name}/tree/${encodeRefName(form.branch)}")
+              } else {
+                redirect(
+                  s"/${repository.owner}/${repository.name}/tree/${encodeRefName(form.branch)}/${encodeRefName(form.path)}"
+                )
+              }
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
+        }
+    }
  })

  get("/:owner/:repository/edit/*")(writableUsersOnly { repository =>
@@ -456,32 +464,7 @@ trait RepositoryViewerControllerBase extends ControllerBase {
  })

  post("/:owner/:repository/create", editorForm)(writableUsersOnly { (form, repository) =>
-    context.withLoginAccount {
-      loginAccount =>
-        if (form.newBranch) {
-          val newBranchName = createNewBranchForPullRequest(repository, form.branch, loginAccount)
-          val objectId = _commit(newBranchName, loginAccount)
-          val issueId =
-            createIssueAndPullRequest(
-              repository,
-              form.branch,
-              newBranchName,
-              form.commit,
-              objectId.name,
-              form.message,
-              loginAccount
-            )
-          redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
-        } else {
-          _commit(form.branch, loginAccount)
-          redirect(
-            s"/${repository.owner}/${repository.name}/blob/${form.branch}/${if (form.path.length == 0) urlEncode(form.newFileName)
-            else s"${form.path}/${urlEncode(form.newFileName)}"}"
-          )
-        }
-    }
-
-    def _commit(branchName: String, loginAccount: Account): ObjectId = {
+    def _commit(branchName: String, loginAccount: Account): Either[String, ObjectId] = {
      commitFile(
        repository = repository,
        branch = branchName,
@@ -494,37 +477,48 @@ trait RepositoryViewerControllerBase extends ControllerBase {
        commit = form.commit,
        loginAccount = loginAccount,
        settings = context.settings
-      )._1
+      ).map(_._1)
    }
-  })

-  post("/:owner/:repository/update", editorForm)(writableUsersOnly { (form, repository) =>
    context.withLoginAccount {
      loginAccount =>
        if (form.newBranch) {
          val newBranchName = createNewBranchForPullRequest(repository, form.branch, loginAccount)
-          val objectId = _commit(newBranchName, loginAccount)
-          val issueId =
-            createIssueAndPullRequest(
-              repository,
-              form.branch,
-              newBranchName,
-              form.commit,
-              objectId.name,
-              form.message,
-              loginAccount
-            )
-          redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
+          _commit(newBranchName, loginAccount) match {
+            case Right(objectId) =>
+              val issueId =
+                createIssueAndPullRequest(
+                  repository,
+                  form.branch,
+                  newBranchName,
+                  form.commit,
+                  objectId.name,
+                  form.message,
+                  loginAccount
+                )
+              redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
        } else {
-          _commit(form.branch, loginAccount)
-          redirect(
-            s"/${repository.owner}/${repository.name}/blob/${urlEncode(form.branch)}/${if (form.path.length == 0) urlEncode(form.newFileName)
-            else s"${form.path}/${urlEncode(form.newFileName)}"}"
-          )
+          _commit(form.branch, loginAccount) match {
+            case Right(_) =>
+              if (form.path.length == 0) {
+                redirect(
+                  s"/${repository.owner}/${repository.name}/blob/${encodeRefName(form.branch)}/${urlEncode(form.newFileName)}"
+                )
+              } else {
+                redirect(
+                  s"/${repository.owner}/${repository.name}/blob/${encodeRefName(form.branch)}/${encodeRefName(form.path)}/${urlEncode(form.newFileName)}"
+                )
+              }
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
        }
    }
+  })

-    def _commit(branchName: String, loginAccount: Account): ObjectId = {
+  post("/:owner/:repository/update", editorForm)(writableUsersOnly { (form, repository) =>
+    def _commit(branchName: String, loginAccount: Account): Either[String, ObjectId] = {
      commitFile(
        repository = repository,
        branch = branchName,
@@ -541,37 +535,48 @@ trait RepositoryViewerControllerBase extends ControllerBase {
        commit = form.commit,
        loginAccount = loginAccount,
        settings = context.settings
-      )._1
+      ).map(_._1)
    }
-  })

-  post("/:owner/:repository/remove", deleteForm)(writableUsersOnly { (form, repository) =>
    context.withLoginAccount {
      loginAccount =>
        if (form.newBranch) {
          val newBranchName = createNewBranchForPullRequest(repository, form.branch, loginAccount)
-          val objectId = _commit(newBranchName, loginAccount)
-          val issueId =
-            createIssueAndPullRequest(
-              repository,
-              form.branch,
-              newBranchName,
-              form.commit,
-              objectId.name,
-              form.message,
-              loginAccount
-            )
-          redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
+          _commit(newBranchName, loginAccount) match {
+            case Right(objectId) =>
+              val issueId =
+                createIssueAndPullRequest(
+                  repository,
+                  form.branch,
+                  newBranchName,
+                  form.commit,
+                  objectId.name,
+                  form.message,
+                  loginAccount
+                )
+              redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
        } else {
-          _commit(form.branch, loginAccount)
-          redirect(
-            s"/${repository.owner}/${repository.name}/tree/${form.branch}${if (form.path.length == 0) ""
-            else "/" + form.path}"
-          )
+          _commit(form.branch, loginAccount) match {
+            case Right(_) =>
+              if (form.path.length == 0) {
+                redirect(
+                  s"/${repository.owner}/${repository.name}/blob/${encodeRefName(form.branch)}/${urlEncode(form.newFileName)}"
+                )
+              } else {
+                redirect(
+                  s"/${repository.owner}/${repository.name}/blob/${encodeRefName(form.branch)}/${encodeRefName(form.path)}/${urlEncode(form.newFileName)}"
+                )
+              }
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
        }
    }
+  })

-    def _commit(branchName: String, loginAccount: Account): ObjectId = {
+  post("/:owner/:repository/remove", deleteForm)(writableUsersOnly { (form, repository) =>
+    def _commit(branchName: String, loginAccount: Account): Either[String, ObjectId] = {
      commitFile(
        repository = repository,
        branch = branchName,
@@ -584,7 +589,41 @@ trait RepositoryViewerControllerBase extends ControllerBase {
        commit = form.commit,
        loginAccount = loginAccount,
        settings = context.settings
-      )._1
+      ).map(_._1)
+    }
+
+    context.withLoginAccount {
+      loginAccount =>
+        if (form.newBranch) {
+          val newBranchName = createNewBranchForPullRequest(repository, form.branch, loginAccount)
+          _commit(newBranchName, loginAccount) match {
+            case Right(objectId) =>
+              val issueId =
+                createIssueAndPullRequest(
+                  repository,
+                  form.branch,
+                  newBranchName,
+                  form.commit,
+                  objectId.name,
+                  form.message,
+                  loginAccount
+                )
+              redirect(s"/${repository.owner}/${repository.name}/pull/${issueId}")
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
+        } else {
+          _commit(form.branch, loginAccount) match {
+            case Right(_) =>
+              if (form.path.length == 0) {
+                redirect(s"/${repository.owner}/${repository.name}/tree/${encodeRefName(form.branch)}")
+              } else {
+                redirect(
+                  s"/${repository.owner}/${repository.name}/tree/${encodeRefName(form.branch)}/${encodeRefName(form.path)}"
+                )
+              }
+            case Left(error) => Forbidden(gitbucket.core.html.error(error))
+          }
+        }
    }
  })

--- a/src/main/scala/gitbucket/core/controller/api/ApiRepositoryContentsControllerBase.scala
+++ b/src/main/scala/gitbucket/core/controller/api/ApiRepositoryContentsControllerBase.scala
@@ -157,7 +157,7 @@ trait ApiRepositoryContentsControllerBase extends ControllerBase {
                    Some("https://docs.github.com/en/rest/reference/repos#create-or-update-file-contents")
                  )
                case _ =>
-                  val (commitId, blobId) = commitFile(
+                  commitFile(
                    repository,
                    branch,
                    path,
@@ -170,12 +170,12 @@ trait ApiRepositoryContentsControllerBase extends ControllerBase {
                    data.committer.map(_.name).getOrElse(loginAccount.fullName),
                    data.committer.map(_.email).getOrElse(loginAccount.mailAddress),
                    context.settings
-                  )
-
-                  blobId match {
-                    case None =>
+                  ) match {
+                    case Left(error) =>
+                      ApiError(s"Failed to commit a file: ${error}", None)
+                    case Right((_, None)) =>
                      ApiError("Failed to commit a file.", None)
-                    case Some(blobId) =>
+                    case Right((commitId, Some(blobId))) =>
                      Map(
                        "content" -> ApiContents(
                          "file",
--- a/src/main/scala/gitbucket/core/service/RepositoryCommitFileService.scala
+++ b/src/main/scala/gitbucket/core/service/RepositoryCommitFileService.scala
@@ -36,10 +36,10 @@ trait RepositoryCommitFileService {
    settings: SystemSettings
  )(
    f: (Git, ObjectId, DirCacheBuilder, ObjectInserter) => Unit
-  )(implicit s: Session, c: JsonFormat.Context): ObjectId = {
+  )(implicit s: Session, c: JsonFormat.Context): Either[String, ObjectId] = {
    _createFiles(repository, branch, message, loginAccount, loginAccount.fullName, loginAccount.mailAddress, settings)(
      f
-    )._1
+    ).map(_._1)
  }

  /**
@@ -58,7 +58,7 @@ trait RepositoryCommitFileService {
    commit: String,
    loginAccount: Account,
    settings: SystemSettings
-  )(implicit s: Session, c: JsonFormat.Context): (ObjectId, Option[ObjectId]) = {
+  )(implicit s: Session, c: JsonFormat.Context): Either[String, (ObjectId, Option[ObjectId])] = {
    commitFile(
      repository,
      branch,
@@ -92,7 +92,7 @@ trait RepositoryCommitFileService {
    committerName: String,
    committerMailAddress: String,
    settings: SystemSettings
-  )(implicit s: Session, c: JsonFormat.Context): (ObjectId, Option[ObjectId]) = {
+  )(implicit s: Session, c: JsonFormat.Context): Either[String, (ObjectId, Option[ObjectId])] = {

    val newPath = newFileName.map { newFileName =>
      if (path.length == 0) newFileName else s"${path}/${newFileName}"
@@ -141,7 +141,7 @@ trait RepositoryCommitFileService {
    settings: SystemSettings
  )(
    f: (Git, ObjectId, DirCacheBuilder, ObjectInserter) => R
-  )(implicit s: Session, c: JsonFormat.Context): (ObjectId, R) = {
+  )(implicit s: Session, c: JsonFormat.Context): Either[String, (ObjectId, R)] = {

    LockUtil.lock(s"${repository.owner}/${repository.name}") {
      Using.resource(Git.open(getRepositoryDir(repository.owner, repository.name))) { git =>
@@ -177,11 +177,11 @@ trait RepositoryCommitFileService {
        error match {
          case Some(error) =>
            // commit is rejected
-            // TODO Notify commit failure to edited user
            val refUpdate = git.getRepository.updateRef(headName)
            refUpdate.setNewObjectId(headTip)
            refUpdate.setForceUpdate(true)
            refUpdate.update()
+            Left(error)

          case None =>
            // update refs
@@ -242,8 +242,8 @@ trait RepositoryCommitFileService {
                )
              }
            }
+            Right((commitId, result))
        }
-        (commitId, result)
      }
    }
  }
--- a/src/main/scala/gitbucket/core/util/StringUtil.scala
+++ b/src/main/scala/gitbucket/core/util/StringUtil.scala
@@ -74,6 +74,11 @@ object StringUtil {

  def urlDecode(value: String): String = URLDecoder.decode(value, "UTF-8")

+  /**
+   * URL encode except '/'.
+   */
+  def encodeRefName(value: String): String = urlEncode(value).replace("%2F", "/")
+
  def splitWords(value: String): Array[String] = value.split("[ \\t　]+")

  def isInteger(value: String): Boolean = allCatch opt { value.toInt } map (_ => true) getOrElse (false)
--- a/src/main/scala/gitbucket/core/view/helpers.scala
+++ b/src/main/scala/gitbucket/core/view/helpers.scala
@@ -276,7 +276,7 @@ object helpers extends AvatarImageProvider with LinkConverter with RequestCache
  /**
   * URL encode except '/'.
   */
-  def encodeRefName(value: String): String = StringUtil.urlEncode(value).replace("%2F", "/")
+  def encodeRefName(value: String): String = StringUtil.encodeRefName(value)

  def urlEncode(value: String): String = StringUtil.urlEncode(value)