Merge pull request #1373 from gitbucket/keep_pull_request_comment

(refs #1348)Keep pull request comment if new commits are pushed

build.sbt

@@ -15,6 +15,7 @@ scalaVersion := "2.11.8"
 // dependency settings
 resolvers ++= Seq(
   Classpaths.typesafeReleases,
+  Resolver.jcenterRepo,
   "amateras" at "http://amateras.sourceforge.jp/mvn/",
   "sonatype-snapshot" at "https://oss.sonatype.org/content/repositories/snapshots/",
   "amateras-snapshot" at "http://amateras.sourceforge.jp/mvn-snapshot/"
@@ -45,6 +46,7 @@ libraryDependencies ++= Seq(
   "com.typesafe"              % "config"                       % "1.3.0",
   "com.typesafe.akka"        %% "akka-actor"                   % "2.3.15",
   "fr.brouillard.oss.security.xhub" % "xhub4j-core"            % "1.0.0",
+  "com.github.bkromhout"      % "java-diff-utils"              % "2.1.1",
   "com.enragedginger"        %% "akka-quartz-scheduler"        % "1.4.0-akka-2.3.x" exclude("c3p0","c3p0"),
   "org.eclipse.jetty"         % "jetty-webapp"                 % JettyVersion     % "provided",
   "javax.servlet"             % "javax.servlet-api"            % "3.1.0"          % "provided",

src/main/scala/gitbucket/core/controller/ApiController.scala

@@ -22,6 +22,7 @@ class ApiController extends ApiControllerBase
   with IssuesService
   with LabelsService
   with PullRequestService
+  with CommitsService
   with CommitStatusService
   with RepositoryCreationService
   with HandleCommentService

src/main/scala/gitbucket/core/controller/DashboardController.scala

@@ -1,13 +1,13 @@
 package gitbucket.core.controller
 
 import gitbucket.core.dashboard.html
-import gitbucket.core.service.{RepositoryService, PullRequestService, AccountService, IssuesService}
-import gitbucket.core.util.{StringUtil, Keys, UsersAuthenticator}
+import gitbucket.core.service._
+import gitbucket.core.util.{Keys, UsersAuthenticator}
 import gitbucket.core.util.Implicits._
 import gitbucket.core.service.IssuesService._
 
 class DashboardController extends DashboardControllerBase
-  with IssuesService with PullRequestService with RepositoryService with AccountService
+  with IssuesService with PullRequestService with RepositoryService with AccountService with CommitsService
   with UsersAuthenticator
 
 trait DashboardControllerBase extends ControllerBase {

src/main/scala/gitbucket/core/controller/IssuesController.scala

@@ -14,12 +14,33 @@ import org.scalatra.Ok
 
 
 class IssuesController extends IssuesControllerBase
-  with IssuesService with RepositoryService with AccountService with LabelsService with MilestonesService with ActivityService with HandleCommentService
-  with ReadableUsersAuthenticator with ReferrerAuthenticator with WritableUsersAuthenticator with PullRequestService with WebHookIssueCommentService
+  with IssuesService
+  with RepositoryService
+  with AccountService
+  with LabelsService
+  with MilestonesService
+  with ActivityService
+  with HandleCommentService
+  with ReadableUsersAuthenticator
+  with ReferrerAuthenticator
+  with WritableUsersAuthenticator
+  with PullRequestService
+  with WebHookIssueCommentService
+  with CommitsService
 
 trait IssuesControllerBase extends ControllerBase {
-  self: IssuesService with RepositoryService with AccountService with LabelsService with MilestonesService with ActivityService with HandleCommentService
-    with ReadableUsersAuthenticator with ReferrerAuthenticator with WritableUsersAuthenticator with PullRequestService with WebHookIssueCommentService =>
+  self: IssuesService
+    with RepositoryService
+    with AccountService
+    with LabelsService
+    with MilestonesService
+    with ActivityService
+    with HandleCommentService
+    with ReadableUsersAuthenticator
+    with ReferrerAuthenticator
+    with WritableUsersAuthenticator
+    with PullRequestService
+    with WebHookIssueCommentService =>
 
   case class IssueCreateForm(title: String, content: Option[String],
     assignedUserName: Option[String], milestoneId: Option[Int], labelNames: Option[String])

src/main/scala/gitbucket/core/controller/PullRequestsController.scala

@@ -11,10 +11,7 @@ import gitbucket.core.service._
 import gitbucket.core.util.ControlUtil._
 import gitbucket.core.util.Directory._
 import gitbucket.core.util.Implicits._
-import gitbucket.core.util.JGitUtil._
 import gitbucket.core.util._
-import gitbucket.core.view
-import gitbucket.core.view.helpers
 import io.github.gitbucket.scalatra.forms._
 import org.eclipse.jgit.api.Git
 import org.eclipse.jgit.lib.PersonIdent
@@ -498,26 +495,6 @@ trait PullRequestsControllerBase extends ControllerBase {
       (defaultOwner, value)
     }
 
-  private def getRequestCompareInfo(userName: String, repositoryName: String, branch: String,
-      requestUserName: String, requestRepositoryName: String, requestCommitId: String): (Seq[Seq[CommitInfo]], Seq[DiffInfo]) =
-    using(
-      Git.open(getRepositoryDir(userName, repositoryName)),
-      Git.open(getRepositoryDir(requestUserName, requestRepositoryName))
-    ){ (oldGit, newGit) =>
-      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)
-      }.toList.splitWith { (commit1, commit2) =>
-        helpers.date(commit1.commitTime) == view.helpers.date(commit2.commitTime)
-      }
-
-      val diffs = JGitUtil.getDiffs(newGit, oldId.getName, newId.getName, true)
-
-      (commits, diffs)
-    }
-
   private def searchPullRequests(userName: Option[String], repository: RepositoryService.RepositoryInfo) =
     defining(repository.owner, repository.name){ case (owner, repoName) =>
       val page = IssueSearchCondition.page(request)

src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala

@@ -328,7 +328,7 @@ trait RepositoryViewerControllerBase extends ControllerBase {
               html.commit(id, new JGitUtil.CommitInfo(revCommit),
                 JGitUtil.getBranchesOfCommit(git, revCommit.getName),
                 JGitUtil.getTagsOfCommit(git, revCommit.getName),
-                getCommitComments(repository.owner, repository.name, id, false),
+                getCommitComments(repository.owner, repository.name, id, true),
                 repository, diffs, oldCommitId, hasDeveloperRole(repository.owner, repository.name, context.loginAccount))
           }
         }

src/main/scala/gitbucket/core/service/CommitsService.scala

@@ -42,6 +42,12 @@ trait CommitsService {
       updatedDate       = currentDate,
       issueId           = issueId)
 
+  def updateCommitCommentPosition(commentId: Int, commitId: String, oldLine: Option[Int], newLine: Option[Int])(implicit s: Session): Unit =
+    CommitComments.filter(_.byPrimaryKey(commentId))
+      .map { t =>
+        (t.commitId, t.oldLine, t.newLine)
+      }.update(commitId, oldLine, newLine)
+
   def updateCommitComment(commentId: Int, content: String)(implicit s: Session) =
     CommitComments
       .filter (_.byPrimaryKey(commentId))

src/main/scala/gitbucket/core/service/PullRequestService.scala

@@ -1,12 +1,22 @@
 package gitbucket.core.service
 
-import gitbucket.core.model.{Account, Issue, PullRequest, WebHook, CommitStatus, CommitState}
+import difflib.{Delta, DiffUtils}
+import gitbucket.core.model.{Session => _, _}
 import gitbucket.core.model.Profile._
+import gitbucket.core.util.ControlUtil._
+import gitbucket.core.util.Directory._
+import gitbucket.core.util.Implicits._
 import gitbucket.core.util.JGitUtil
+import gitbucket.core.util.JGitUtil.{CommitInfo, DiffInfo}
+import gitbucket.core.view
+import gitbucket.core.view.helpers
+import org.eclipse.jgit.api.Git
 import profile.simple._
 
+import scala.collection.JavaConverters._
 
-trait PullRequestService { self: IssuesService =>
+
+trait PullRequestService { self: IssuesService with CommitsService =>
   import PullRequestService._
 
   def getPullRequest(owner: String, repository: String, issueId: Int)
@@ -111,9 +121,26 @@ trait PullRequestService { self: IssuesService =>
   def updatePullRequests(owner: String, repository: String, branch: String)(implicit s: Session): Unit =
     getPullRequestsByRequest(owner, repository, branch, false).foreach { pullreq =>
       if(Repositories.filter(_.byRepository(pullreq.userName, pullreq.repositoryName)).exists.run){
+        // Update the git repository
         val (commitIdTo, commitIdFrom) = JGitUtil.updatePullRequest(
           pullreq.userName, pullreq.repositoryName, pullreq.branch, pullreq.issueId,
           pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch)
+
+        // Collect comment positions
+        val positions = getCommitComments(pullreq.userName, pullreq.repositoryName, pullreq.commitIdTo, true)
+          .collect {
+            case CommitComment(_, _, _, commentId, _, _, Some(file), None, Some(newLine), _, _, _) => (file, commentId, Right(newLine))
+            case CommitComment(_, _, _, commentId, _, _, Some(file), Some(oldLine), None, _, _, _) => (file, commentId, Left(oldLine))
+          }
+          .groupBy { case (file, _, _) => file }
+          .map { case (file, comments) => file ->
+            comments.map { case (_, commentId, lineNumber) => (commentId, lineNumber) }
+          }
+
+        // Update comments position
+        updatePullRequestCommentPositions(positions, pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.commitIdTo, commitIdTo)
+
+        // Update commit id in the PULL_REQUEST table
         updateCommitId(pullreq.userName, pullreq.repositoryName, pullreq.issueId, commitIdTo, commitIdFrom)
       }
     }
@@ -137,6 +164,78 @@ trait PullRequestService { self: IssuesService =>
         .firstOption
     }
   }
+
+  private def updatePullRequestCommentPositions(positions: Map[String, Seq[(Int, Either[Int, Int])]], userName: String, repositoryName: String,
+                                                oldCommitId: String, newCommitId: String)(implicit s: Session): Unit = {
+
+    val (_, diffs) = getRequestCompareInfo(userName, repositoryName, oldCommitId, userName, repositoryName, newCommitId)
+
+    val patchs = positions.map { case (file, _) =>
+      diffs.find(x => x.oldPath == file).map { diff =>
+        (diff.oldContent, diff.newContent) match {
+          case (Some(oldContent), Some(newContent)) => {
+            val oldLines = oldContent.replace("\r\n", "\n").split("\n")
+            val newLines = newContent.replace("\r\n", "\n").split("\n")
+            file -> Option(DiffUtils.diff(oldLines.toList.asJava, newLines.toList.asJava))
+          }
+          case _ =>
+            file -> None
+        }
+      }.getOrElse {
+        file -> None
+      }
+    }
+
+    positions.foreach { case (file, comments) =>
+      patchs(file) match {
+        case Some(patch) => file -> comments.foreach { case (commentId, lineNumber) => lineNumber match {
+          case Left(oldLine)  => updateCommitCommentPosition(commentId, newCommitId, Some(oldLine), None)
+          case Right(newLine) =>
+            var counter = newLine
+            patch.getDeltas.asScala.filter(_.getOriginal.getPosition < newLine).foreach { delta =>
+              delta.getType match {
+                case Delta.TYPE.CHANGE =>
+                  if(delta.getOriginal.getPosition <= newLine - 1 && newLine <= delta.getOriginal.getPosition + delta.getRevised.getLines.size){
+                    counter = -1
+                  } else {
+                    counter = counter + (delta.getRevised.getLines.size - delta.getOriginal.getLines.size)
+                  }
+                case Delta.TYPE.INSERT => counter = counter + delta.getRevised.getLines.size
+                case Delta.TYPE.DELETE => counter = counter - delta.getOriginal.getLines.size
+              }
+            }
+            if(counter >= 0){
+              updateCommitCommentPosition(commentId, newCommitId, None, Some(counter))
+            }
+        }}
+        case _ => comments.foreach { case (commentId, lineNumber) => lineNumber match {
+          case Right(oldLine) => updateCommitCommentPosition(commentId, newCommitId, Some(oldLine), None)
+          case Left(newLine)  => updateCommitCommentPosition(commentId, newCommitId, None, Some(newLine))
+        }}
+      }
+    }
+  }
+
+  def getRequestCompareInfo(userName: String, repositoryName: String, branch: String,
+                            requestUserName: String, requestRepositoryName: String, requestCommitId: String): (Seq[Seq[CommitInfo]], Seq[DiffInfo]) =
+    using(
+      Git.open(getRepositoryDir(userName, repositoryName)),
+      Git.open(getRepositoryDir(requestUserName, requestRepositoryName))
+    ){ (oldGit, newGit) =>
+      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)
+      }.toList.splitWith { (commit1, commit2) =>
+        helpers.date(commit1.commitTime) == view.helpers.date(commit2.commitTime)
+      }
+
+      val diffs = JGitUtil.getDiffs(newGit, oldId.getName, newId.getName, true)
+
+      (commits, diffs)
+    }
+
 }
 
 object PullRequestService {

src/main/scala/gitbucket/core/servlet/GitRepositoryServlet.scala

@@ -110,7 +110,7 @@ import scala.collection.JavaConverters._
 class CommitLogHook(owner: String, repository: String, pusher: String, baseUrl: String)(implicit session: Session)
   extends PostReceiveHook with PreReceiveHook
   with RepositoryService with AccountService with IssuesService with ActivityService with PullRequestService with WebHookService
-  with WebHookPullRequestService {
+  with WebHookPullRequestService with CommitsService {
   
   private val logger = LoggerFactory.getLogger(classOf[CommitLogHook])
   private var existIds: Seq[String] = Nil

src/main/twirl/gitbucket/core/helper/commitcomment.scala.html

@@ -13,15 +13,14 @@
       @helpers.avatar(comment.commentedUserName, 20)
       @helpers.user(comment.commentedUserName, styleClass="username strong")
         <span class="muted">
-          commented
+          commented on
           @if(comment.issueId.isDefined){
-            on this Pull request
-          } else {
-            @if(comment.fileName.isDefined){
-              on @comment.fileName.get
+            <a href="@helpers.url(repository)/pull/@comment.issueId">#@comment.issueId</a>
           }
-            in <a href="@context.path/@repository.owner/@repository.name/commit/@comment.commitId">@comment.commitId.substring(0, 7)</a>
+          @comment.fileName.map { fileName =>
+            @fileName in
           }
+          <a href="@context.path/@repository.owner/@repository.name/commit/@comment.commitId">@comment.commitId.substring(0, 7)</a>
           @gitbucket.core.helper.html.datetimeago(comment.registeredDate)
         </span>
       <span class="pull-right">

src/main/twirl/gitbucket/core/helper/diff.scala.html

@@ -240,7 +240,6 @@ $(function(){
     var oldline  = $v.attr('oldline');
     var newline  = $v.attr('newline');
     var tmp;
-    var diff;
     if (typeof oldline !== 'undefined') {
       if (typeof newline !== 'undefined') {
         tmp = getInlineContainer();
@@ -248,13 +247,11 @@ $(function(){
         tmp = getInlineContainer('old');
       }
       tmp.children('td:first').html($v.clone().show());
-      diff.find('table.diff').find('.oldline[line-number=' + oldline  + ']')
-      .parent().nextAll(':not(.not-diff):first').before(tmp);
+      diff.find('table.diff').find('.oldline[line-number=' + oldline  + ']').parent().nextAll(':not(.not-diff):first').before(tmp);
     } else {
       tmp = getInlineContainer('new');
       tmp.children('td:last').html($v.clone().show());
-      diff.find('table.diff').find('.newline[line-number=' + newline  + ']')
-      .parent().nextAll(':not(.not-diff):first').before(tmp);
+      diff.find('table.diff').find('.newline[line-number=' + newline  + ']').parent().nextAll(':not(.not-diff):first').before(tmp);
     }
     if (!diff.find('.toggle-notes').prop('checked')) {
       tmp.hide();
@@ -263,10 +260,10 @@ $(function(){
   function renderStatBar(add, del){
     if(add + del > 5){
       if(add){
-        if(add<del){
-          add = Math.floor(1 + (add * 4 / (add+del)));
+        if(add < del){
+          add = Math.floor(1 + (add * 4 / (add + del)));
         } else {
-          add = Math.ceil(1 + (add * 4 / (add+del)));
+          add = Math.ceil(1 + (add * 4 / (add + del)));
         }
       }
       del = 5 - add;
@@ -290,10 +287,12 @@ $(function(){
     var i = table.data("diff-id");
     var ignoreWhiteSpace = table.find('.ignore-whitespace').prop('checked');
     diffUsingJS('oldText-'+i, 'newText-'+i, diffText.attr('id'), viewType, ignoreWhiteSpace);
-    var add = diffText.find("table").attr("add")*1;
-    var del = diffText.find("table").attr("del")*1;
+    var add = diffText.find("table").attr("add") * 1;
+    var del = diffText.find("table").attr("del") * 1;
     table.find(".diffstat").text(add+del+" ").append(renderStatBar(add,del)).attr("title",add+" additions & "+del+" deletions").tooltip();
-    $('span.diffstat[data-diff-id="'+i+'"]').html('<span class="text-diff-added">+'+add+'</span><span class="text-diff-deleted">-'+del+'</span>').append(renderStatBar(add,del).attr('title',(add+del)+" lines changed").tooltip());
+    $('span.diffstat[data-diff-id="'+i+'"]')
+        .html('<span class="text-diff-added">+' + add + '</span><span class="text-diff-deleted">-' + del + '</span>')
+        .append(renderStatBar(add, del).attr('title', (add + del) + " lines changed").tooltip());
 
     @if(hasWritePermission) {
       diffText.find('.body').each(function(){ $('<b class="add-comment">+</b>').prependTo(this); });
@@ -301,7 +300,7 @@ $(function(){
     @if(showLineNotes){
       var fileName = table.attr('filename');
       $('.inline-comment').each(function(i, v) {
-        if($(this).attr('filename')==fileName){
+        if($(this).attr('filename') == fileName){
           renderOneCommitCommentIntoDiff($(this), table);
         }
       });

src/test/scala/gitbucket/core/service/PullRequestServiceSpec.scala

@@ -4,7 +4,7 @@ import gitbucket.core.model._
 import org.scalatest.FunSpec
 
 class PullRequestServiceSpec extends FunSpec with ServiceSpecBase
-  with PullRequestService with IssuesService with AccountService with RepositoryService {
+  with PullRequestService with IssuesService with AccountService with RepositoryService with CommitsService {
 
   def swap(r: (Issue, PullRequest)) = (r._2 -> r._1)
 

src/test/scala/gitbucket/core/service/ServiceSpecBase.scala

@@ -44,7 +44,7 @@ trait ServiceSpecBase {
   def user(name:String)(implicit s:Session):Account = AccountService.getAccountByUserName(name).get
 
   lazy val dummyService = new RepositoryService with AccountService with IssuesService with PullRequestService
-    with CommitStatusService with LabelsService (){}
+    with CommitsService with CommitStatusService with LabelsService (){}
 
   def generateNewUserWithDBRepository(userName:String, repositoryName:String)(implicit s:Session):Account = {
     val ac = AccountService.getAccountByUserName(userName).getOrElse(generateNewAccount(userName))

src/test/scala/gitbucket/core/service/WebHookServiceSpec.scala

@@ -6,7 +6,7 @@ import gitbucket.core.model.WebHookContentType
 
 
 class WebHookServiceSpec extends FunSuite with ServiceSpecBase {
-  lazy val service = new WebHookPullRequestService with AccountService with RepositoryService with PullRequestService with IssuesService
+  lazy val service = new WebHookPullRequestService with AccountService with RepositoryService with PullRequestService with IssuesService with CommitsService
 
   test("WebHookPullRequestService.getPullRequestsByRequestForWebhook") { withTestDB { implicit session =>
     val user1 = generateNewUserWithDBRepository("user1","repo1")