(refs #179) Merge branch 'improve-pullreq-performance'

src/main/scala/app/PullRequestsController.scala

@@ -4,20 +4,20 @@ import util.{LockUtil, CollaboratorsAuthenticator, JGitUtil, ReferrerAuthenticat
 import util.Directory._
 import util.Implicits._
 import util.ControlUtil._
-import util.FileUtil._
 import service._
 import org.eclipse.jgit.api.Git
 import jp.sf.amateras.scalatra.forms._
 import org.eclipse.jgit.transport.RefSpec
-import org.apache.commons.io.FileUtils
 import scala.collection.JavaConverters._
-import org.eclipse.jgit.lib.PersonIdent
+import org.eclipse.jgit.lib.{ObjectId, CommitBuilder, PersonIdent}
-import org.eclipse.jgit.api.MergeCommand.FastForwardMode
 import service.IssuesService._
 import service.PullRequestService._
 import util.JGitUtil.DiffInfo
 import service.RepositoryService.RepositoryTreeNode
 import util.JGitUtil.CommitInfo
+import org.slf4j.LoggerFactory
+import org.eclipse.jgit.merge.MergeStrategy
+import org.eclipse.jgit.errors.NoMergeBaseException
 
 class PullRequestsController extends PullRequestsControllerBase
   with RepositoryService with AccountService with IssuesService with PullRequestService with MilestonesService with ActivityService
@@ -27,6 +27,8 @@ trait PullRequestsControllerBase extends ControllerBase {
   self: RepositoryService with AccountService with IssuesService with MilestonesService with ActivityService with PullRequestService
     with ReferrerAuthenticator with CollaboratorsAuthenticator =>
 
+  private val logger = LoggerFactory.getLogger(classOf[PullRequestsControllerBase])
+
   val pullRequestForm = mapping(
     "title"           -> trim(label("Title"  , text(required, maxlength(100)))),
     "content"         -> trim(label("Content", optional(text()))),
@@ -91,7 +93,7 @@ trait PullRequestsControllerBase extends ControllerBase {
       val name = repository.name
       getPullRequest(owner, name, issueId) map { case(issue, pullreq) =>
         pulls.html.mergeguide(
-          checkConflict(owner, name, pullreq.branch, owner, name, pullreq.requestBranch),
+          checkConflictInPullRequest(owner, name, pullreq.branch, pullreq.requestUserName, name, pullreq.requestBranch, issueId),
           pullreq,
           s"${baseUrl}${context.path}/git/${pullreq.requestUserName}/${pullreq.requestRepositoryName}.git")
       }
@@ -104,67 +106,72 @@ trait PullRequestsControllerBase extends ControllerBase {
       val name = repository.name
       LockUtil.lock(s"${owner}/${name}/merge"){
         getPullRequest(owner, name, issueId).map { case (issue, pullreq) =>
-          val remote = getRepositoryDir(owner, name)
+          using(Git.open(getRepositoryDir(owner, name))) { git =>
-          withTmpDir(new java.io.File(getTemporaryDir(owner, name), s"merge-${issueId}")){ tmpdir =>
+            // mark issue as merged and close.
-            using(Git.cloneRepository.setDirectory(tmpdir).setURI(remote.toURI.toString).setBranch(pullreq.branch).call){ git =>
+            val loginAccount = context.loginAccount.get
+            createComment(owner, name, loginAccount.userName, issueId, form.message, "merge")
+            createComment(owner, name, loginAccount.userName, issueId, "Close", "close")
+            updateClosed(owner, name, issueId, true)
 
-              // mark issue as merged and close.
+            // record activity
-              val loginAccount = context.loginAccount.get
+            recordMergeActivity(owner, name, loginAccount.userName, issueId, form.message)
-              createComment(owner, name, loginAccount.userName, issueId, form.message, "merge")
-              createComment(owner, name, loginAccount.userName, issueId, "Close", "close")
-              updateClosed(owner, name, issueId, true)
 
-              // record activity
+            // prepare head branch
-              recordMergeActivity(owner, name, loginAccount.userName, issueId, form.message)
+            fetchPullRequest(git, issueId, pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch)
 
-              // fetch pull request to temporary working repository
+            // merge
-              val pullRequestBranchName = s"gitbucket-pullrequest-${issueId}"
+            val mergeBaseRefName = s"refs/heads/${pullreq.branch}"
-
+            val merger = MergeStrategy.RECURSIVE.newMerger(git.getRepository, true)
-              git.fetch
+            val mergeBaseTip = git.getRepository.resolve(mergeBaseRefName)
-                .setRemote(getRepositoryDir(owner, name).toURI.toString)
+            val mergeTip = git.getRepository.resolve(s"refs/pull/${issueId}/head")
-                .setRefSpecs(new RefSpec(s"refs/pull/${issueId}/head:refs/heads/${pullRequestBranchName}")).call
+            val conflicted = try {
-
+              !merger.merge(mergeBaseTip, mergeTip)
-              // merge pull request
+            } catch {
-              git.checkout.setName(pullreq.branch).call
+              case e: NoMergeBaseException => true
-
-              val result = git.merge
-                .include(git.getRepository.resolve(pullRequestBranchName))
-                .setFastForward(FastForwardMode.NO_FF)
-                .setCommit(false)
-                .call
-
-              if(result.getConflicts != null){
-                throw new RuntimeException("This pull request can't merge automatically.")
-              }
-
-              // merge commit
-              git.getRepository.writeMergeCommitMsg(
-                s"Merge pull request #${issueId} from ${pullreq.requestUserName}/${pullreq.requestRepositoryName}\n"
-                + form.message)
-
-              git.commit
-                .setCommitter(new PersonIdent(loginAccount.fullName, loginAccount.mailAddress))
-                .call
-
-              // push
-              git.push.call
-
-              val (commits, _) = getRequestCompareInfo(owner, name, pullreq.commitIdFrom,
-                pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.commitIdTo)
-
-              commits.flatten.foreach { commit =>
-                if(!existsCommitId(owner, name, commit.id)){
-                  insertCommitId(owner, name, commit.id)
-                }
-              }
-
-              // notifications
-              Notifier().toNotify(repository, issueId, "merge"){
-                Notifier.msgStatus(s"${baseUrl}/${owner}/${name}/pull/${issueId}")
-              }
-
-              redirect(s"/${owner}/${name}/pull/${issueId}")
             }
+            if (conflicted) {
+              throw new RuntimeException("This pull request can't merge automatically.")
+            }
+
+            // creates merge commit
+            val mergeCommit = new CommitBuilder()
+            mergeCommit.setTreeId(merger.getResultTreeId)
+            mergeCommit.setParentIds(Array[ObjectId](mergeBaseTip, mergeTip): _*)
+            val personIdent = new PersonIdent(loginAccount.fullName, loginAccount.mailAddress)
+            mergeCommit.setAuthor(personIdent)
+            mergeCommit.setCommitter(personIdent)
+            mergeCommit.setMessage(s"Merge pull request #${issueId} from ${pullreq.requestUserName}/${pullreq.requestRepositoryName}\n\n" +
+                                   form.message)
+
+            // insertObject and got mergeCommit Object Id
+            val inserter = git.getRepository.newObjectInserter
+            val mergeCommitId = inserter.insert(mergeCommit)
+            inserter.flush()
+            inserter.release()
+
+            // update refs
+            val refUpdate = git.getRepository.updateRef(mergeBaseRefName)
+            refUpdate.setNewObjectId(mergeCommitId)
+            refUpdate.setForceUpdate(false)
+            refUpdate.setRefLogIdent(personIdent)
+            refUpdate.setRefLogMessage("merged", true)
+            refUpdate.update()
+
+            val (commits, _) = getRequestCompareInfo(owner, name, pullreq.commitIdFrom,
+              pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.commitIdTo)
+
+            commits.flatten.foreach { commit =>
+              if(!existsCommitId(owner, name, commit.id)){
+                insertCommitId(owner, name, commit.id)
+              }
+            }
+
+            // notifications
+            Notifier().toNotify(repository, issueId, "merge"){
+              Notifier.msgStatus(s"${baseUrl}/${owner}/${name}/pull/${issueId}")
+            }
+
+            redirect(s"/${owner}/${name}/pull/${issueId}")
           }
         }
       }
@@ -315,23 +322,51 @@ trait PullRequestsControllerBase extends ControllerBase {
    */
   private def checkConflict(userName: String, repositoryName: String, branch: String,
                             requestUserName: String, requestRepositoryName: String, requestBranch: String): Boolean = {
-    // TODO Are there more quick way?
     LockUtil.lock(s"${userName}/${repositoryName}/merge-check"){
-      val remote = getRepositoryDir(userName, repositoryName)
+      using(Git.open(getRepositoryDir(requestUserName, requestRepositoryName))) { git =>
-      withTmpDir(new java.io.File(getTemporaryDir(userName, repositoryName), "merge-check")){ tmpdir =>
+        val remoteRefName = s"refs/heads/${branch}"
-        using(Git.cloneRepository.setDirectory(tmpdir).setURI(remote.toURI.toString).setBranch(branch).call){ git =>
+        val tmpRefName = s"refs/merge-check/${userName}/${branch}"
-
-          git.checkout.setName(branch).call
 
+        withTmpRefSpec(new RefSpec(s"${remoteRefName}:${tmpRefName}").setForceUpdate(true), git) { ref =>
+          // fetch objects from origin repository branch
           git.fetch
-            .setRemote(getRepositoryDir(requestUserName, requestRepositoryName).toURI.toString)
+             .setRemote(getRepositoryDir(userName, repositoryName).toURI.toString)
-            .setRefSpecs(new RefSpec(s"refs/heads/${branch}:refs/heads/${requestBranch}")).call
+             .setRefSpecs(ref)
+             .call
 
-          val result = git.merge
+          // merge conflict check
-            .include(git.getRepository.resolve("FETCH_HEAD"))
+          val merger = MergeStrategy.RECURSIVE.newMerger(git.getRepository, true)
-            .setCommit(false).call
+          val mergeBaseTip = git.getRepository.resolve(s"refs/heads/${requestBranch}")
+          val mergeTip = git.getRepository.resolve(tmpRefName)
+          try {
+            !merger.merge(mergeBaseTip, mergeTip)
+          } catch {
+            case e: NoMergeBaseException =>  true
+          }
+        }
+      }
+    }
+  }
 
-          result.getConflicts != null
+  /**
+   * Checks whether conflict will be caused in merging within pull request. Returns true if conflict will be caused.
+   */
+  private def checkConflictInPullRequest(userName: String, repositoryName: String, branch: String,
+                                         requestUserName: String, requestRepositoryName: String, requestBranch: String,
+                                         issueId: Int): Boolean = {
+    LockUtil.lock(s"${userName}/${repositoryName}/merge") {
+      using(Git.open(getRepositoryDir(userName, repositoryName))) { git =>
+        // fetch pull request contents
+        fetchPullRequest(git, issueId, requestUserName, requestRepositoryName, requestBranch)
+
+        // merge
+        val merger = MergeStrategy.RECURSIVE.newMerger(git.getRepository, true)
+        val mergeBaseTip = git.getRepository.resolve(s"refs/heads/${branch}")
+        val mergeTip = git.getRepository.resolve(s"refs/pull/${issueId}/head")
+        try {
+          !merger.merge(mergeBaseTip, mergeTip)
+        } catch {
+          case e: NoMergeBaseException => true
         }
       }
     }
@@ -414,4 +449,14 @@ trait PullRequestsControllerBase extends ControllerBase {
         hasWritePermission(owner, repoName, context.loginAccount))
     }
 
+  /**
+   * Fetch pull request contents into refs/pull/${issueId}/head
+   */
+  private def fetchPullRequest(git: Git, issueId: Int, requestUserName: String, requestRepositoryName: String, requestBranch: String): Unit = {
+    git.fetch
+      .setRemote(getRepositoryDir(requestUserName, requestRepositoryName).toURI.toString)
+      .setRefSpecs(new RefSpec(s"refs/heads/${requestBranch}:refs/pull/${issueId}/head").setForceUpdate(true))
+      .call
+  }
+
 }

src/main/scala/util/ControlUtil.scala

@@ -3,6 +3,7 @@ package util
 import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.revwalk.RevWalk
 import org.eclipse.jgit.treewalk.TreeWalk
+import org.eclipse.jgit.transport.RefSpec
 
 /**
  * Provides control facilities.
@@ -37,6 +38,17 @@ object ControlUtil {
   def using[T](treeWalk: TreeWalk)(f: TreeWalk => T): T =
     try f(treeWalk) finally treeWalk.release()
 
+
+  def withTmpRefSpec[T](ref: RefSpec, git: Git)(f: RefSpec => T): T = {
+    try {
+      f(ref)
+    } finally {
+      val refUpdate = git.getRepository.updateRef(ref.getDestination)
+      refUpdate.setForceUpdate(true)
+      refUpdate.delete()
+    }
+  }
+
   def executeIf(condition: => Boolean)(action: => Unit): Boolean =
     if(condition){
       action