(refs #2)Fix pull request. Basic pattern had been tested but it's still unstable.

2025-11-06 05:25:50 +01:00 · 2013-07-24 22:05:36 +09:00
parent 205119cc01
commit 88caff38f0
7 changed files with 120 additions and 82 deletions
--- a/src/main/resources/update/1_5.sql
+++ b/src/main/resources/update/1_5.sql
@@ -0,0 +1,19 @@
+ALTER TABLE REPOSITORY ADD COLUMN ORIGIN_USER_NAME VARCHAR(100);
+ALTER TABLE REPOSITORY ADD COLUMN ORIGIN_REPOSITORY_NAME VARCHAR(100);
+
+
+CREATE TABLE PULL_REQUEST(
+		USER_NAME VARCHAR(100) NOT NULL,
+		REPOSITORY_NAME VARCHAR(100) NOT NULL,
+		ISSUE_ID INT NOT NULL,
+		BRANCH VARCHAR(100) NOT NULL,
+		REQUEST_USER_NAME VARCHAR(100) NOT NULL,
+		REQUEST_REPOSITORY_NAME VARCHAR(100) NOT NULL,
+		REQUEST_BRANCH VARCHAR(100) NOT NULL,
+		COMMIT_ID_FROM VARCHAR(40) NOT NULL,
+		COMMIT_ID_TO VARCHAR(40) NOT NULL
+);
+
+ALTER TABLE PULL_REQUEST ADD CONSTRAINT IDX_PULL_REQUEST_PK PRIMARY KEY (USER_NAME, REPOSITORY_NAME, ISSUE_ID);
+ALTER TABLE PULL_REQUEST ADD CONSTRAINT IDX_PULL_REQUEST_FK0 FOREIGN KEY (USER_NAME, REPOSITORY_NAME, ISSUE_ID) REFERENCES ISSUE (USER_NAME, REPOSITORY_NAME, ISSUE_ID);
+ALTER TABLE PULL_REQUEST ADD CONSTRAINT IDX_PULL_REQUEST_FK1 FOREIGN KEY (REQUEST_USER_NAME, REQUEST_REPOSITORY_NAME) REFERENCES REPOSITORY (USER_NAME, REPOSITORY_NAME);
--- a/src/main/scala/app/PullRequestsController.scala
+++ b/src/main/scala/app/PullRequestsController.scala
@@ -1,6 +1,6 @@
 package app

-import util.{CollaboratorsAuthenticator, FileUtil, JGitUtil, ReferrerAuthenticator}
+import util.{CollaboratorsAuthenticator, JGitUtil, ReferrerAuthenticator}
 import util.Directory._
 import util.Implicits._
 import util.JGitUtil.{DiffInfo, CommitInfo}
@@ -9,7 +9,6 @@ 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 org.eclipse.jgit.lib.PersonIdent
 import scala.collection.JavaConverters._

 class PullRequestsController extends PullRequestsControllerBase
@@ -25,15 +24,23 @@ trait PullRequestsControllerBase extends ControllerBase {
    "content"         -> trim(label("Content", optional(text()))),
    "branch"          -> trim(text(required, maxlength(100))),
    "requestUserName" -> trim(text(required, maxlength(100))),
-    "requestBranch"   -> trim(text(required, maxlength(100)))
+    "requestCommitId" -> trim(text(required, maxlength(100))),
+    "commitIdFrom"    -> trim(text(required, maxlength(40))),
+    "commitIdTo"      -> trim(text(required, maxlength(40)))
  )(PullRequestForm.apply)

  val mergeForm = mapping(
    "message" -> trim(label("Message", text(required)))
  )(MergeForm.apply)

-  case class PullRequestForm(title: String, content: Option[String], branch: String,
-                             requestUserName: String, requestBranch: String)
+  case class PullRequestForm(
+    title: String,
+    content: Option[String],
+    branch: String,
+    requestUserName: String,
+    requestBranch: String,
+    commitIdFrom: String,
+    commitIdTo: String)

  case class MergeForm(message: String)

@@ -50,11 +57,8 @@ trait PullRequestsControllerBase extends ControllerBase {
      JGitUtil.withGit(getRepositoryDir(owner, name)){ git =>
        val requestCommitId = git.getRepository.resolve(pullreq.requestBranch)

-        val (commits, diffs) = if(pullreq.mergeStartId.isDefined){
-          getMergedCompareInfo(owner, name, pullreq.mergeStartId.get, pullreq.mergeEndId.get)
-        } else {
-          getRequestCompareInfo(owner, name, pullreq.branch, pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch)
-        }
+        val (commits, diffs) =
+          getRequestCompareInfo(owner, name, pullreq.commitIdFrom, owner, name, pullreq.commitIdTo)

        pulls.html.pullreq(
          issue, pullreq,
@@ -64,10 +68,10 @@ trait PullRequestsControllerBase extends ControllerBase {
          commits,
          diffs,
          requestCommitId.getName,
-          if(pullreq.mergeStartId.isDefined){
+          if(issue.closed){
            false
          } else {
-            checkConflict(owner, name, pullreq.branch, pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch)
+            checkConflict(owner, name, pullreq.branch, owner, name, pullreq.requestBranch)
          },
          hasWritePermission(owner, name, context.loginAccount),
          repository,
@@ -95,26 +99,25 @@ trait PullRequestsControllerBase extends ControllerBase {
        git.checkout.setName(pullreq.branch).call

        git.fetch
-          .setRemote(getRepositoryDir(pullreq.requestUserName, pullreq.requestRepositoryName).toURI.toString)
-          .setRefSpecs(new RefSpec(s"refs/heads/${pullreq.branch}:refs/heads/${pullreq.requestBranch}")).call
+          .setRemote(getRepositoryDir(repository.owner, repository.name).toURI.toString)
+          .setRefSpecs(new RefSpec(s"refs/pull/${issueId}/head:refs/heads/${pullreq.branch}")).call
+
+        val result = git.merge
+          .include(git.getRepository.resolve("FETCH_HEAD"))
+          .setCommit(false).call

-        val result = git.merge.include(git.getRepository.resolve("FETCH_HEAD")).setCommit(false).call
        if(result.getConflicts != null){
          throw new RuntimeException("This pull request can't merge automatically.")
        }

-        git.commit
-          .setCommitter(new PersonIdent(loginAccount.userName, loginAccount.mailAddress))
-          .setMessage(s"Merge pull request #${issueId} from ${pullreq.requestUserName}/${pullreq.requestRepositoryName}\n"
-                     + form.message).call
+//        git.commit
+//          .setCommitter(new PersonIdent(loginAccount.userName, loginAccount.mailAddress))
+//          .setMessage(s"Merge pull request #${issueId} from ${pullreq.requestUserName}/${pullreq.requestRepositoryName}\n"
+//                     + form.message).call
        git.push.call

-        val (commits, _) = getRequestCompareInfo(repository.owner, repository.name, pullreq.branch,
-          pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch)
-
-        mergePullRequest(repository.owner, repository.name, issueId,
-          git.getRepository.resolve(pullreq.requestBranch).getName,
-          commits.head.head.id)
+        val (commits, _) = getRequestCompareInfo(repository.owner, repository.name, pullreq.commitIdFrom,
+          pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.commitIdTo)

        commits.flatten.foreach { commit =>
          insertCommitId(repository.owner, repository.name, commit.id)
@@ -180,17 +183,25 @@ trait PullRequestsControllerBase extends ControllerBase {
      val originRepositoryName = repository.repository.originRepositoryName.get

      getRepository(originUserName, originRepositoryName, baseUrl).map{ originRepository =>
-        val Seq(origin, originId, forkedId) = multiParams("splat")
+        val Seq(compareUserName, compareFrom, compareTo) = multiParams("splat")

-        JGitUtil.withGit(getRepositoryDir(repository.owner, repository.name)){ git =>
-          val newId = git.getRepository.resolve(forkedId)
+        withGit(
+          getRepositoryDir(originUserName, originRepositoryName),
+          getRepositoryDir(repository.owner, repository.name)
+        ){ case (oldGit, newGit) =>
+
+          val forkedId = getForkedCommitId(oldGit, newGit, originUserName, originRepositoryName, compareFrom,
+            repository.owner, repository.name, compareTo)
+
+          val oldId = oldGit.getRepository.resolve(forkedId)
+          val newId = newGit.getRepository.resolve(compareTo)

          val (commits, diffs) = getRequestCompareInfo(
-            origin, repository.repository.originRepositoryName.get, originId,
-            repository.owner, repository.name, forkedId)
+            compareUserName, repository.repository.originRepositoryName.get, forkedId,
+            repository.owner, repository.name, compareTo)

-          pulls.html.compare(commits, diffs, origin, originId, forkedId, newId.getName,
-            checkConflict(originUserName, originRepositoryName, originId, repository.owner, repository.name, forkedId),
+          pulls.html.compare(commits, diffs, compareUserName, compareFrom, compareTo, oldId.getName, newId.getName,
+            checkConflict(originUserName, originRepositoryName, compareFrom, repository.owner, repository.name, compareTo),
            repository, originRepository)
        }
      } getOrElse NotFound
@@ -215,7 +226,17 @@ trait PullRequestsControllerBase extends ControllerBase {
      form.branch,
      form.requestUserName,
      repository.name,
-      form.requestBranch)
+      form.requestBranch,
+      form.commitIdFrom,
+      form.commitIdTo)
+
+    // fetch requested branch
+    JGitUtil.withGit(getRepositoryDir(repository.owner, repository.name)){ git =>
+      git.fetch
+        .setRemote(getRepositoryDir(form.requestUserName, repository.name).toURI.toString)
+        .setRefSpecs(new RefSpec(s"refs/heads/${form.requestBranch}:refs/pull/${issueId}/head"))
+        .call
+    }

    recordPullRequestActivity(repository.owner, repository.name, loginUserName, issueId, form.title)

@@ -233,17 +254,22 @@ trait PullRequestsControllerBase extends ControllerBase {
    }
  }

+  private def getForkedCommitId(oldGit: Git, newGit: Git, userName: String, repositoryName: String, branch: String,
+      requestUserName: String, requestRepositoryName: String, requestBranch: String): String =
+    JGitUtil.getCommitLogs(newGit, requestBranch, true){ commit =>
+      existsCommitId(userName, repositoryName, commit.getName) &&
+        JGitUtil.getBranchesOfCommit(oldGit, commit.getName).contains(branch)
+    }.head.id
+
  private def getRequestCompareInfo(userName: String, repositoryName: String, branch: String,
-      requestUserName: String, requestRepositoryName: String, requestBranch: String): (Seq[Seq[CommitInfo]], Seq[DiffInfo]) = {
+      requestUserName: String, requestRepositoryName: String, requestCommitId: String): (Seq[Seq[CommitInfo]], Seq[DiffInfo]) = {

    withGit(
      getRepositoryDir(userName, repositoryName),
      getRepositoryDir(requestUserName, requestRepositoryName)
    ){ (oldGit, newGit) =>
-      val newId = newGit.getRepository.resolve(requestBranch)
-      val oldId = newGit.getRepository.resolve(JGitUtil.getCommitLogFrom(newGit, newId.getName, true){ revCommit =>
-        existsCommitId(userName, repositoryName, revCommit.getName)
-      }.head.id)
+      val oldId = oldGit.getRepository.resolve(branch)
+      val newId = newGit.getRepository.resolve(requestCommitId)

      val commits = newGit.log.addRange(oldId, newId).call.iterator.asScala.map { revCommit =>
        new CommitInfo(revCommit)
@@ -257,24 +283,4 @@ trait PullRequestsControllerBase extends ControllerBase {
    }
  }

-  private def getMergedCompareInfo(userName: String, repositoryName: String,
-                                   startId: String, endId: String): (Seq[Seq[CommitInfo]], Seq[DiffInfo]) = {
-
-    JGitUtil.withGit(getRepositoryDir(userName, repositoryName)){ git =>
-      val oldId = git.getRepository.resolve(startId)
-      val newId = git.getRepository.resolve(endId)
-
-      val commits = git.log.addRange(newId, oldId).call.iterator.asScala.map { revCommit =>
-        new CommitInfo(revCommit)
-      }.toList.splitWith{ (commit1, commit2) =>
-        view.helpers.date(commit1.time) == view.helpers.date(commit2.time)
-      }
-
-      val diffs = JGitUtil.getDiffs(git, oldId.getName, newId.getName, true)
-
-      (commits, diffs)
-    }
-  }
-
-
 }
--- a/src/main/scala/model/PullRequest.scala
+++ b/src/main/scala/model/PullRequest.scala
@@ -7,9 +7,9 @@ object PullRequests extends Table[PullRequest]("PULL_REQUEST") with IssueTemplat
  def requestUserName = column[String]("REQUEST_USER_NAME")
  def requestRepositoryName = column[String]("REQUEST_REPOSITORY_NAME")
  def requestBranch = column[String]("REQUEST_BRANCH")
-  def mergeStartId = column[String]("MERGE_START_ID")
-  def mergeEndId = column[String]("MERGE_END_ID")
-  def * = userName ~ repositoryName ~ issueId ~ branch ~ requestUserName ~ requestRepositoryName ~ requestBranch ~ mergeStartId.? ~ mergeEndId.? <> (PullRequest, PullRequest.unapply _)
+  def commitIdFrom = column[String]("COMMIT_ID_FROM")
+  def commitIdTo = column[String]("COMMIT_ID_TO")
+  def * = userName ~ repositoryName ~ issueId ~ branch ~ requestUserName ~ requestRepositoryName ~ requestBranch ~ commitIdFrom ~ commitIdTo <> (PullRequest, PullRequest.unapply _)

  def byPrimaryKey(userName: String, repositoryName: String, issueId: Int) = byIssue(userName, repositoryName, issueId)
  def byPrimaryKey(userName: Column[String], repositoryName: Column[String], issueId: Column[Int]) = byIssue(userName, repositoryName, issueId)
@@ -23,5 +23,6 @@ case class PullRequest(
  requestUserName: String,
  requestRepositoryName: String,
  requestBranch: String,
-  mergeStartId: Option[String],
-  mergeEndId: Option[String])
+  commitIdFrom: String,
+  commitIdTo: String
+)
--- a/src/main/scala/service/PullRequestService.scala
+++ b/src/main/scala/service/PullRequestService.scala
@@ -18,7 +18,8 @@ trait PullRequestService { self: IssuesService =>
  }

  def createPullRequest(originUserName: String, originRepositoryName: String, issueId: Int,
-        originBranch: String, requestUserName: String, requestRepositoryName: String, requestBranch: String): Unit =
+        originBranch: String, requestUserName: String, requestRepositoryName: String, requestBranch: String,
+        commitIdFrom: String, commitIdTo: String): Unit =
    PullRequests insert (PullRequest(
      originUserName,
      originRepositoryName,
@@ -27,15 +28,15 @@ trait PullRequestService { self: IssuesService =>
      requestUserName,
      requestRepositoryName,
      requestBranch,
-      None,
-      None))
+      commitIdFrom,
+      commitIdTo))

-  def mergePullRequest(originUserName: String, originRepositoryName: String, issueId: Int,
-        mergeStartId: String, mergeEndId: String): Unit = {
-    Query(PullRequests)
-      .filter(_.byPrimaryKey(originUserName, originRepositoryName, issueId))
-      .map(t => t.mergeStartId ~ t.mergeEndId)
-      .update(mergeStartId, mergeEndId)
-  }
+//  def mergePullRequest(originUserName: String, originRepositoryName: String, issueId: Int,
+//        mergeStartId: String, mergeEndId: String): Unit = {
+//    Query(PullRequests)
+//      .filter(_.byPrimaryKey(originUserName, originRepositoryName, issueId))
+//      .map(t => t.mergeStartId ~ t.mergeEndId)
+//      .update(mergeStartId, mergeEndId)
+//  }

 }
--- a/src/main/scala/servlet/AutoUpdateListener.scala
+++ b/src/main/scala/servlet/AutoUpdateListener.scala
@@ -49,6 +49,7 @@ object AutoUpdate {
   * The history of versions. A head of this sequence is the current BitBucket version.
   */
  val versions = Seq(
+    Version(1, 5),
    Version(1, 4),
    new Version(1, 3){
      override def update(conn: Connection): Unit = {
--- a/src/main/scala/util/JGitUtil.scala
+++ b/src/main/scala/util/JGitUtil.scala
@@ -291,14 +291,15 @@ object JGitUtil {
    }
  }

-  def getCommitLogFrom(git: Git, to: String, containsLast: Boolean = false)(from: RevCommit => Boolean): List[CommitInfo] = {
+  def getCommitLogs(git: Git, begin: String, includesLastCommit: Boolean = false)
+                   (endCondition: RevCommit => Boolean): List[CommitInfo] = {
    @scala.annotation.tailrec
    def getCommitLog(i: java.util.Iterator[RevCommit], logs: List[CommitInfo]): List[CommitInfo] =
      i.hasNext match {
        case true  => {
          val revCommit = i.next
-          if(from(revCommit)){
-            if(containsLast) logs :+ new CommitInfo(revCommit) else logs
+          if(endCondition(revCommit)){
+            if(includesLastCommit) logs :+ new CommitInfo(revCommit) else logs
          } else {
            getCommitLog(i, logs :+ new CommitInfo(revCommit))
          }
@@ -307,7 +308,7 @@ object JGitUtil {
      }

    val revWalk = new RevWalk(git.getRepository)
-    revWalk.markStart(revWalk.parseCommit(git.getRepository.resolve(to)))
+    revWalk.markStart(revWalk.parseCommit(git.getRepository.resolve(begin)))

    val commits = getCommitLog(revWalk.iterator, Nil)
    revWalk.release
@@ -324,8 +325,9 @@ object JGitUtil {
   * @param to the to revision
   * @return the commit list
   */
+  // TODO swap parameters 'from' and 'to'!?
  def getCommitLog(git: Git, from: String, to: String): List[CommitInfo] =
-    getCommitLogFrom(git, to)(_.getName == from)
+    getCommitLogs(git, to)(_.getName == from)
  
  /**
   * Returns the latest RevCommit of the specified path.
--- a/src/main/twirl/pulls/compare.scala.html
+++ b/src/main/twirl/pulls/compare.scala.html
@@ -1,5 +1,11 @@
-@(commits: Seq[Seq[util.JGitUtil.CommitInfo]], diffs: Seq[util.JGitUtil.DiffInfo],
-  origin: String, originId: String, forkedId: String, commitId: String, hasConflict: Boolean,
+@(commits: Seq[Seq[util.JGitUtil.CommitInfo]],
+  diffs: Seq[util.JGitUtil.DiffInfo],
+  origin: String,
+  originId: String,
+  forkedId: String,
+  sourceId: String,
+  commitId: String,
+  hasConflict: Boolean,
  repository: service.RepositoryService.RepositoryInfo,
  originRepository: service.RepositoryService.RepositoryInfo)(implicit context: app.Context)
@import context._
@@ -12,7 +18,7 @@
      <a href="#" id="edit-compare-condition" class="btn btn-mini pull-right">Edit</a>
      <span class="label label-info monospace">@origin:@originId</span> ... <span class="label label-info monospace">@repository.owner:@forkedId</span>
    </div>
-    <div id="compare-edit" style="display: none; width: 620px;">
+    <div id="compare-edit" style="display: none;">
      <a href="#" id="refresh-compare" class="pull-right"><i class="icon-remove-circle"></i></a>
      <span class="label label-info monospace">@origin/@repository.name:</span>
      @helper.html.dropdown(originId) {
@@ -52,7 +58,9 @@
              @helper.html.preview(repository, "", false, true, "width: 600px; height: 200px;")
              <input type="hidden" name="branch" value="@originId"/>
              <input type="hidden" name="requestUserName" value="@repository.owner"/>
-              <input type="hidden" name="requestBranch" value="@forkedId"/>
+              <input type="hidden" name="requestCommitId" value="@forkedId"/>
+              <input type="hidden" name="commitIdFrom" value="@sourceId"/>
+              <input type="hidden" name="commitIdTo" value="@commitId"/>
            </div>
          </form>
        </div>