diff --git a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala index 70ef74e..18fef96 100644 --- a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala +++ b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala @@ -117,13 +117,13 @@ 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), @@ -455,7 +455,7 @@ checkConflict(originRepository.owner, originRepository.name, originBranch, forkedRepository.owner, forkedRepository.name, forkedBranch) } - html.mergecheck(conflict) + html.mergecheck(conflict.isDefined) } }) getOrElse NotFound() }) diff --git a/src/main/scala/gitbucket/core/service/MergeService.scala b/src/main/scala/gitbucket/core/service/MergeService.scala index 2f609bd..061b2a6 100644 --- a/src/main/scala/gitbucket/core/service/MergeService.scala +++ b/src/main/scala/gitbucket/core/service/MergeService.scala @@ -3,13 +3,15 @@ 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.{CommitBuilder, ObjectId, PersonIdent, Repository} import org.eclipse.jgit.revwalk.{RevCommit, RevWalk} +import scala.collection.JavaConverters._ + trait MergeService { import MergeService._ @@ -17,7 +19,7 @@ * 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 => new MergeCacheInfo(git, branch, issueId).checkConflict() } @@ -29,7 +31,7 @@ * 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 => new MergeCacheInfo(git, branch, issueId).checkConflictCache() } @@ -64,7 +66,7 @@ * 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}" @@ -81,12 +83,12 @@ 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) @@ -97,24 +99,23 @@ } /** - * 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) => - 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 + 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 } } @@ -151,35 +152,37 @@ class MergeCacheInfo(git: Git, branch: String, issueId: Int){ private val repository = git.getRepository - private val mergedBranchName = s"refs/pull/${issueId}/merge" + + private val mergedBranchName = s"refs/pull/${issueId}/merge" private 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") + lazy val mergeTip = repository.resolve(s"refs/pull/${issueId}/head") - def checkConflictCache(): Option[Boolean] = { + def checkConflictCache(): Option[Option[String]] = { Option(repository.resolve(mergedBranchName)).flatMap { merged => - if(parseCommit(merged).getParents().toSet == Set( mergeBaseTip, mergeTip )){ - // merged branch exists - Some(false) - } else { - None - } - }.orElse(Option(repository.resolve(conflictedBranchName)).flatMap { conflicted => - if(parseCommit(conflicted).getParents().toSet == Set( mergeBaseTip, mergeTip )){ + if(parseCommit(merged).getParents().toSet == Set( mergeBaseTip, mergeTip )){ + // merged branch exists + Some(None) + } else { + None + } + }.orElse(Option(repository.resolve(conflictedBranchName)).flatMap{ conflicted => + 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) @@ -198,19 +201,22 @@ 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 } - def merge(message: String, committer: PersonIdent): Unit = { - if(checkConflict()){ + // update branch from cache + def merge(message:String, committer:PersonIdent) = { + 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}") + throw new RuntimeException(s"Not found branch ${mergedBranchName}") }) // creates merge commit val mergeCommitId = createMergeCommit(mergeResultCommit.getTree().getId(), committer, message) @@ -219,7 +225,7 @@ } def rebase(committer: PersonIdent, commits: Seq[RevCommit]): Unit = { - if(checkConflict()){ + if(checkConflict().isDefined){ throw new RuntimeException("This pull request can't merge automatically.") } @@ -250,7 +256,7 @@ } def squash(message: String, committer: PersonIdent): Unit = { - if(checkConflict()){ + if(checkConflict().isDefined){ throw new RuntimeException("This pull request can't merge automatically.") } @@ -285,4 +291,13 @@ 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 + } + } diff --git a/src/main/scala/gitbucket/core/service/PullRequestService.scala b/src/main/scala/gitbucket/core/service/PullRequestService.scala index 0ecf20f..b6c009a 100644 --- a/src/main/scala/gitbucket/core/service/PullRequestService.scala +++ b/src/main/scala/gitbucket/core/service/PullRequestService.scala @@ -244,8 +244,8 @@ 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 @@ 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) diff --git a/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html b/src/main/twirl/gitbucket/core/pulls/mergeguide.scala.html index 0dcf106..3332c1f 100644 --- 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. } +
+
+ @status.conflictMessage.map { message => @helpers.markdown(message, originRepository, false, true, false) } +
} else { @if(status.branchIsOutOfDate){ @if(status.hasUpdatePermission){ diff --git a/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala b/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala index be90da4..18d9e52 100644 --- a/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala +++ b/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala @@ -32,8 +32,8 @@ val repo1Dir = initRepository("user1","repo1") assert(service.checkConflictCache("user1", "repo1", branch, issueId) == None) val conflicted = service.checkConflict("user1", "repo1", branch, issueId) - assert(service.checkConflictCache("user1", "repo1", branch, issueId) == Some(false)) - assert(conflicted == false) + assert(service.checkConflictCache("user1", "repo1", branch, issueId) == Some(None)) + assert(conflicted.isEmpty) } it("checkConflict true if not conflicted, and create cache") { val repo2Dir = initRepository("user1","repo2") @@ -42,15 +42,18 @@ } assert(service.checkConflictCache("user1", "repo2", branch, issueId) == None) val conflicted = service.checkConflict("user1", "repo2", branch, issueId) - assert(conflicted == true) - assert(service.checkConflictCache("user1", "repo2", branch, issueId) == Some(true)) + assert(conflicted.isDefined) + assert(service.checkConflictCache("user1", "repo2", branch, issueId) match { + case Some(Some(_: String)) => true + case _ => false + }) } } describe("checkConflictCache") { it("merged cache invalid if origin branch moved") { val repo3Dir = initRepository("user1","repo3") - assert(service.checkConflict("user1", "repo3", branch, issueId) == false) - assert(service.checkConflictCache("user1", "repo3", branch, issueId) == Some(false)) + assert(service.checkConflict("user1", "repo3", branch, issueId).isEmpty) + assert(service.checkConflictCache("user1", "repo3", branch, issueId) == Some(None)) using(Git.open(repo3Dir)){ git => createFile(git, s"refs/heads/${branch}", "test.txt", "hoge2" ) } @@ -58,8 +61,8 @@ } it("merged cache invalid if request branch moved") { val repo4Dir = initRepository("user1","repo4") - assert(service.checkConflict("user1", "repo4", branch, issueId) == false) - assert(service.checkConflictCache("user1", "repo4", branch, issueId) == Some(false)) + assert(service.checkConflict("user1", "repo4", branch, issueId).isEmpty) + assert(service.checkConflictCache("user1", "repo4", branch, issueId) == Some(None)) using(Git.open(repo4Dir)){ git => createFile(git, s"refs/pull/${issueId}/head", "test.txt", "hoge4" ) } @@ -67,8 +70,8 @@ } it("should merged cache invalid if origin branch moved") { val repo5Dir = initRepository("user1","repo5") - assert(service.checkConflict("user1", "repo5", branch, issueId) == false) - assert(service.checkConflictCache("user1", "repo5", branch, issueId) == Some(false)) + assert(service.checkConflict("user1", "repo5", branch, issueId).isEmpty) + assert(service.checkConflictCache("user1", "repo5", branch, issueId) == Some(None)) using(Git.open(repo5Dir)){ git => createFile(git, s"refs/heads/${branch}", "test.txt", "hoge2" ) } @@ -79,8 +82,11 @@ using(Git.open(repo6Dir)){ git => createConfrict(git) } - assert(service.checkConflict("user1", "repo6", branch, issueId) == true) - assert(service.checkConflictCache("user1", "repo6", branch, issueId) == Some(true)) + assert(service.checkConflict("user1", "repo6", branch, issueId).isDefined) + assert(service.checkConflictCache("user1", "repo6", branch, issueId) match { + case Some(Some(_: String)) => true + case _ => false + }) using(Git.open(repo6Dir)){ git => createFile(git, s"refs/pull/${issueId}/head", "test.txt", "hoge4" ) } @@ -91,8 +97,11 @@ using(Git.open(repo7Dir)){ git => createConfrict(git) } - assert(service.checkConflict("user1", "repo7", branch, issueId) == true) - assert(service.checkConflictCache("user1", "repo7", branch, issueId) == Some(true)) + assert(service.checkConflict("user1", "repo7", branch, issueId).isDefined) + assert(service.checkConflictCache("user1", "repo7", branch, issueId) match { + case Some(Some(_)) => true + case _ => false + }) using(Git.open(repo7Dir)){ git => createFile(git, s"refs/heads/${branch}", "test.txt", "hoge4" ) }