Show conflicting files

2025-11-10 15:35:59 +01:00 · 2017-12-12 03:20:50 +09:00
parent 847f96d537
commit 1033122fec
4 changed files with 51 additions and 32 deletions
--- a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala
+++ b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala
@@ -115,13 +115,13 @@ trait PullRequestsControllerBase extends ControllerBase {
      val owner = repository.owner
      val name  = repository.name
      getPullRequest(owner, name, issueId) map { case(issue, pullreq) =>
-        val hasConflict = LockUtil.lock(s"${owner}/${name}"){
+        val conflictMessage = LockUtil.lock(s"${owner}/${name}"){
          checkConflict(owner, name, pullreq.branch, issueId)
        }
        val hasMergePermission = hasDeveloperRole(owner, name, context.loginAccount)
        val branchProtection = getProtectedBranchInfo(owner, name, pullreq.branch)
        val mergeStatus = PullRequestService.MergeStatus(
-           hasConflict         = hasConflict,
+          conflictMessage      = conflictMessage,
           commitStatues       = getCommitStatues(owner, name, pullreq.commitIdTo),
           branchProtection    = branchProtection,
           branchIsOutOfDate   = JGitUtil.getShaByRef(owner, name, pullreq.branch) != Some(pullreq.commitIdFrom),
@@ -437,7 +437,7 @@ trait PullRequestsControllerBase extends ControllerBase {
          checkConflict(originRepository.owner, originRepository.name, originBranch,
                        forkedRepository.owner, forkedRepository.name, forkedBranch)
        }
-        html.mergecheck(conflict)
+        html.mergecheck(conflict.isDefined)
      }
    }) getOrElse NotFound()
  })
--- a/src/main/scala/gitbucket/core/service/MergeService.scala
+++ b/src/main/scala/gitbucket/core/service/MergeService.scala
@@ -3,21 +3,22 @@ package gitbucket.core.service
 import gitbucket.core.model.Account
 import gitbucket.core.util.Directory._
 import gitbucket.core.util.SyntaxSugars._
-
-import org.eclipse.jgit.merge.MergeStrategy
-import org.eclipse.jgit.api.Git
+import org.eclipse.jgit.merge.{MergeStrategy, Merger, RecursiveMerger}
+import org.eclipse.jgit.api.{Git, MergeResult}
 import org.eclipse.jgit.transport.RefSpec
 import org.eclipse.jgit.errors.NoMergeBaseException
-import org.eclipse.jgit.lib.{ObjectId, CommitBuilder, PersonIdent, Repository}
+import org.eclipse.jgit.lib.{CommitBuilder, ObjectId, PersonIdent, Repository}
 import org.eclipse.jgit.revwalk.RevWalk

+import scala.collection.JavaConverters._
+
 trait MergeService {
  import MergeService._
  /**
   * Checks whether conflict will be caused in merging within pull request.
   * Returns true if conflict will be caused.
   */
-  def checkConflict(userName: String, repositoryName: String, branch: String, issueId: Int): Boolean = {
+  def checkConflict(userName: String, repositoryName: String, branch: String, issueId: Int): Option[String] = {
    using(Git.open(getRepositoryDir(userName, repositoryName))) { git =>
      MergeCacheInfo(git, branch, issueId).checkConflict()
    }
@@ -28,7 +29,7 @@ trait MergeService {
   * Returns Some(true) if conflict will be caused.
   * Returns None if cache has not created yet.
   */
-  def checkConflictCache(userName: String, repositoryName: String, branch: String, issueId: Int): Option[Boolean] = {
+  def checkConflictCache(userName: String, repositoryName: String, branch: String, issueId: Int): Option[Option[String]] = {
    using(Git.open(getRepositoryDir(userName, repositoryName))) { git =>
      MergeCacheInfo(git, branch, issueId).checkConflictCache()
    }
@@ -50,7 +51,7 @@ trait MergeService {
   * Checks whether conflict will be caused in merging. Returns true if conflict will be caused.
   */
  def tryMergeRemote(localUserName: String, localRepositoryName: String, localBranch: String,
-                      remoteUserName: String, remoteRepositoryName: String, remoteBranch: String): Option[(ObjectId, ObjectId, ObjectId)] = {
+                      remoteUserName: String, remoteRepositoryName: String, remoteBranch: String): Either[String, (ObjectId, ObjectId, ObjectId)] = {
    using(Git.open(getRepositoryDir(localUserName, localRepositoryName))) { git =>
      val remoteRefName = s"refs/heads/${remoteBranch}"
      val tmpRefName = s"refs/remote-temp/${remoteUserName}/${remoteRepositoryName}/${remoteBranch}"
@@ -67,12 +68,12 @@ trait MergeService {
        val mergeTip = git.getRepository.resolve(tmpRefName)
        try {
          if(merger.merge(mergeBaseTip, mergeTip)){
-            Some((merger.getResultTreeId, mergeBaseTip, mergeTip))
+            Right((merger.getResultTreeId, mergeBaseTip, mergeTip))
          } else {
-            None
+            Left(createConflictMessage(mergeTip, mergeBaseTip, merger))
          }
        } catch {
-          case e: NoMergeBaseException =>  None
+          case e: NoMergeBaseException => Left(e.toString)
        }
      } finally {
        val refUpdate = git.getRepository.updateRef(refSpec.getDestination)
@@ -81,24 +82,25 @@ trait MergeService {
      }
    }
  }
+
  /**
-   * Checks whether conflict will be caused in merging. Returns true if conflict will be caused.
+   * Checks whether conflict will be caused in merging. Returns `Some(errorMessage)` if conflict will be caused.
   */
  def checkConflict(userName: String, repositoryName: String, branch: String,
-                            requestUserName: String, requestRepositoryName: String, requestBranch: String): Boolean =
-    tryMergeRemote(userName, repositoryName, branch, requestUserName, requestRepositoryName, requestBranch).isEmpty
+                            requestUserName: String, requestRepositoryName: String, requestBranch: String): Option[String] =
+    tryMergeRemote(userName, repositoryName, branch, requestUserName, requestRepositoryName, requestBranch).left.toOption

  def pullRemote(localUserName: String, localRepositoryName: String, localBranch: String,
                      remoteUserName: String, remoteRepositoryName: String, remoteBranch: String,
                      loginAccount: Account, message: String): Option[ObjectId] = {
-    tryMergeRemote(localUserName, localRepositoryName, localBranch, remoteUserName, remoteRepositoryName, remoteBranch).map{ case (newTreeId, oldBaseId, oldHeadId) =>
+    tryMergeRemote(localUserName, localRepositoryName, localBranch, remoteUserName, remoteRepositoryName, remoteBranch).map { case (newTreeId, oldBaseId, oldHeadId) =>
      using(Git.open(getRepositoryDir(localUserName, localRepositoryName))) { git =>
        val committer = new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
        val newCommit = Util.createMergeCommit(git.getRepository, newTreeId, committer, message, Seq(oldBaseId, oldHeadId))
        Util.updateRefs(git.getRepository, s"refs/heads/${localBranch}", newCommit, false, committer, Some("merge"))
      }
      oldBaseId
-    }
+    }.toOption
  }

 }
@@ -135,27 +137,28 @@ object MergeService{
    val conflictedBranchName = s"refs/pull/${issueId}/conflict"
    lazy val mergeBaseTip = repository.resolve(s"refs/heads/${branch}")
    lazy val mergeTip = repository.resolve(s"refs/pull/${issueId}/head")
-    def checkConflictCache(): Option[Boolean] = {
-      Option(repository.resolve(mergedBranchName)).flatMap{ merged =>
+    def checkConflictCache(): Option[Option[String]] = {
+      Option(repository.resolve(mergedBranchName)).flatMap { merged =>
          if(parseCommit(merged).getParents().toSet == Set( mergeBaseTip, mergeTip )){
            // merged branch exists
-            Some(false)
+            Some(None)
          } else {
            None
          }
      }.orElse(Option(repository.resolve(conflictedBranchName)).flatMap{ conflicted =>
-        if(parseCommit(conflicted).getParents().toSet == Set( mergeBaseTip, mergeTip )){
+        val commit = parseCommit(conflicted)
+        if(commit.getParents().toSet == Set( mergeBaseTip, mergeTip )){
          // conflict branch exists
-          Some(true)
+          Some(Some(commit.getFullMessage))
        } else {
          None
        }
      })
    }
-    def checkConflict():Boolean ={
+    def checkConflict():Option[String] ={
      checkConflictCache.getOrElse(checkConflictForce)
    }
-    def checkConflictForce():Boolean ={
+    def checkConflictForce():Option[String] ={
      val merger = MergeStrategy.RECURSIVE.newMerger(repository, true)
      val conflicted = try {
        !merger.merge(mergeBaseTip, mergeTip)
@@ -172,15 +175,17 @@ object MergeService{
      if(!conflicted){
        updateBranch(merger.getResultTreeId, s"Merge ${mergeTip.name} into ${mergeBaseTip.name}", mergedBranchName)
        git.branchDelete().setForce(true).setBranchNames(conflictedBranchName).call()
+        None
      } else {
-        updateBranch(mergeTipCommit.getTree().getId(), s"can't merge ${mergeTip.name} into ${mergeBaseTip.name}", conflictedBranchName)
+        val message = createConflictMessage(mergeTip, mergeBaseTip, merger)
+        updateBranch(mergeTipCommit.getTree().getId(), message, conflictedBranchName)
        git.branchDelete().setForce(true).setBranchNames(mergedBranchName).call()
+        Some(message)
      }
-      conflicted
    }
    // update branch from cache
    def merge(message:String, committer:PersonIdent) = {
-      if(checkConflict()){
+      if(checkConflict().isDefined){
        throw new RuntimeException("This pull request can't merge automatically.")
      }
      val mergeResultCommit = parseCommit( Option(repository.resolve(mergedBranchName)).getOrElse(throw new RuntimeException(s"not found branch ${mergedBranchName}")) )
@@ -195,4 +200,13 @@ object MergeService{

    private def parseCommit(id:ObjectId) = using(new RevWalk( repository ))(_.parseCommit(id))
  }
+
+  private def createConflictMessage(mergeTip: ObjectId, mergeBaseTip: ObjectId, merger: Merger): String = {
+    val mergeResults = merger.asInstanceOf[RecursiveMerger].getMergeResults
+
+    s"can't merge ${mergeTip.name} into ${mergeBaseTip.name}\n\n" +
+      "Conflicting files:\n" +
+      mergeResults.asScala.map { case (key, _) => "- " + key + "\n" }.mkString
+  }
+
 }
--- a/src/main/scala/gitbucket/core/service/PullRequestService.scala
+++ b/src/main/scala/gitbucket/core/service/PullRequestService.scala
@@ -244,8 +244,8 @@ object PullRequestService {
  case class PullRequestCount(userName: String, count: Int)

  case class MergeStatus(
-    hasConflict: Boolean,
-    commitStatues:List[CommitStatus],
+    conflictMessage: Option[String],
+    commitStatues: List[CommitStatus],
    branchProtection: ProtectedBranchService.ProtectedBranchInfo,
    branchIsOutOfDate: Boolean,
    hasUpdatePermission: Boolean,
@@ -253,12 +253,13 @@ object PullRequestService {
    hasMergePermission: Boolean,
    commitIdTo: String){

+    val hasConflict = conflictMessage.isDefined
    val statuses: List[CommitStatus] =
      commitStatues ++ (branchProtection.contexts.toSet -- commitStatues.map(_.context).toSet).map(CommitStatus.pending(branchProtection.owner, branchProtection.repository, _))
    val hasRequiredStatusProblem = needStatusCheck && branchProtection.contexts.exists(context => statuses.find(_.context == context).map(_.state) != Some(CommitState.SUCCESS))
    val hasProblem = hasRequiredStatusProblem || hasConflict || (statuses.nonEmpty && CommitState.combine(statuses.map(_.state).toSet) != CommitState.SUCCESS)
-    val canUpdate = branchIsOutOfDate && !hasConflict
-    val canMerge = hasMergePermission && !hasConflict && !hasRequiredStatusProblem
+    val canUpdate  = branchIsOutOfDate  && !hasConflict
+    val canMerge   = hasMergePermission && !hasConflict && !hasRequiredStatusProblem
    lazy val commitStateSummary:(CommitState, String) = {
      val stateMap = statuses.groupBy(_.state)
      val state = CommitState.combine(stateMap.keySet)
--- a/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html
+++ b/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html
@@ -41,6 +41,10 @@
              Only those with write access to this repository can merge pull requests.
            }
          </div>
+          <div>
+            <hr>
+            @status.conflictMessage.map { message => @helpers.markdown(message, originRepository, false, true, false) }
+          </div>
        } else {
          @if(status.branchIsOutOfDate){
            @if(status.hasUpdatePermission){