diff --git a/src/main/resources/update/gitbucket-core_4.0.xml b/src/main/resources/update/gitbucket-core_4.0.xml index 1603e59b5..0c95711a5 100644 --- a/src/main/resources/update/gitbucket-core_4.0.xml +++ b/src/main/resources/update/gitbucket-core_4.0.xml @@ -237,7 +237,7 @@ - + diff --git a/src/main/resources/update/gitbucket-core_4.46.xml b/src/main/resources/update/gitbucket-core_4.46.xml new file mode 100644 index 000000000..5537eac5c --- /dev/null +++ b/src/main/resources/update/gitbucket-core_4.46.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/src/main/scala/gitbucket/core/GitBucketCoreModule.scala b/src/main/scala/gitbucket/core/GitBucketCoreModule.scala index 8e3fc8127..4b1638c22 100644 --- a/src/main/scala/gitbucket/core/GitBucketCoreModule.scala +++ b/src/main/scala/gitbucket/core/GitBucketCoreModule.scala @@ -122,7 +122,8 @@ object GitBucketCoreModule new Version("4.42.1"), new Version("4.43.0"), new Version("4.44.0", new LiquibaseMigration("update/gitbucket-core_4.44.xml")), - new Version("4.45.0") + new Version("4.45.0"), + new Version("4.46.0", new LiquibaseMigration("update/gitbucket-core_4.46.xml")) ) { java.util.logging.Logger.getLogger("liquibase").setLevel(Level.SEVERE) } diff --git a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala index f745adcc6..bb6ec2640 100644 --- a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala +++ b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala @@ -247,41 +247,43 @@ trait PullRequestsControllerBase extends ControllerBase { }) get("/:owner/:repository/pull/:id/delete_branch")(readableUsersOnly { baseRepository => - (for { - issueId <- params("id").toIntOpt - loginAccount <- context.loginAccount - case (issue, pullreq) <- getPullRequest(baseRepository.owner, baseRepository.name, issueId) - owner = pullreq.requestUserName - name = pullreq.requestRepositoryName - if hasDeveloperRole(owner, name, context.loginAccount) - } yield { - val repository = getRepository(owner, name).get - val branchProtection = getProtectedBranchInfo(owner, name, pullreq.requestBranch) - if (branchProtection.enabled) { - flash.update("error", s"branch ${pullreq.requestBranch} is protected.") - } else { - if (repository.repository.defaultBranch != pullreq.requestBranch) { - val userName = context.loginAccount.get.userName - Using.resource(Git.open(getRepositoryDir(repository.owner, repository.name))) { git => - git.branchDelete().setForce(true).setBranchNames(pullreq.requestBranch).call() - val deleteBranchInfo = DeleteBranchInfo(repository.owner, repository.name, userName, pullreq.requestBranch) - recordActivity(deleteBranchInfo) - } - createComment( - baseRepository.owner, - baseRepository.name, - userName, - issueId, - pullreq.requestBranch, - "delete_branch" - ) + context.withLoginAccount { _ => + (for { + issueId <- params("id").toIntOpt + case (issue, pullreq) <- getPullRequest(baseRepository.owner, baseRepository.name, issueId) + owner = pullreq.requestUserName + name = pullreq.requestRepositoryName + if hasDeveloperRole(owner, name, context.loginAccount) + } yield { + val repository = getRepository(owner, name).get + val branchProtection = getProtectedBranchInfo(owner, name, pullreq.requestBranch) + if (branchProtection.enabled) { + flash.update("error", s"branch ${pullreq.requestBranch} is protected.") } else { - flash.update("error", s"""Can't delete the default branch "${pullreq.requestBranch}".""") + if (repository.repository.defaultBranch != pullreq.requestBranch) { + val userName = context.loginAccount.get.userName + Using.resource(Git.open(getRepositoryDir(repository.owner, repository.name))) { git => + git.branchDelete().setForce(true).setBranchNames(pullreq.requestBranch).call() + val deleteBranchInfo = + DeleteBranchInfo(repository.owner, repository.name, userName, pullreq.requestBranch) + recordActivity(deleteBranchInfo) + } + createComment( + baseRepository.owner, + baseRepository.name, + userName, + issueId, + pullreq.requestBranch, + "delete_branch" + ) + } else { + flash.update("error", s"""Can't delete the default branch "${pullreq.requestBranch}".""") + } } - } - redirect(s"/${baseRepository.owner}/${baseRepository.name}/pull/${issueId}") - }) getOrElse NotFound() + redirect(s"/${baseRepository.owner}/${baseRepository.name}/pull/${issueId}") + }) getOrElse NotFound() + } }) post("/:owner/:repository/pull/:id/update_branch")(readableUsersOnly { baseRepository => @@ -361,8 +363,11 @@ trait PullRequestsControllerBase extends ControllerBase { form.isDraft, context.settings ) match { - case Right(objectId) => redirect(s"/${repository.owner}/${repository.name}/pull/$issueId") - case Left(message) => Some(BadRequest(message)) + case Right(result) => + updateMergedCommitIds(repository.owner, repository.name, issueId, result.mergedCommitId) + redirect(s"/${repository.owner}/${repository.name}/pull/$issueId") + case Left(message) => + Some(BadRequest(message)) } } getOrElse NotFound() } @@ -722,15 +727,84 @@ trait PullRequestsControllerBase extends ControllerBase { ) } + post("/:owner/:repository/pull/:id/revert")(writableUsersOnly { repository => + context.withLoginAccount { loginAccount => + (for { + issueId <- params.get("id").map(_.toInt) + (issue, pullreq) <- getPullRequest(repository.owner, repository.name, issueId) if issue.closed + } yield { + val baseBranch = pullreq.branch + val revertBranch = s"revert-pr-$issueId-${System.currentTimeMillis()}" + + Using.resource(Git.open(getRepositoryDir(repository.owner, repository.name))) { git => + try { + // Create a new branch from base + JGitUtil.createBranch(git, baseBranch, revertBranch) + + val revertCommitId = pullreq.mergedCommitIds match { + case Some(mergedCommitIds) => + createRevertCommit( + git, + revertBranch, + mergedCommitIds.split(",").toSeq, + loginAccount.fullName, + loginAccount.mailAddress, + s"Revert #$issueId" + ) + case None => + Left("No merged commit IDs found for this pull request") + } + + revertCommitId match { + case Right(revertCommitObjectId) => + val newIssueId = insertIssue( + owner = repository.owner, + repository = repository.name, + loginUser = loginAccount.userName, + title = s"Revert #${issueId}", + content = Some(s"Revert #${issueId}"), + milestoneId = None, + priorityId = None, + isPullRequest = true + ) + createPullRequest( + originRepository = repository, + issueId = newIssueId, + originBranch = baseBranch, + requestUserName = repository.owner, + requestRepositoryName = repository.name, + requestBranch = revertBranch, + commitIdFrom = git.getRepository.resolve(s"refs/heads/$baseBranch").getName, + commitIdTo = revertCommitObjectId.name(), + isDraft = false, + loginAccount = loginAccount, + settings = context.settings + ) + redirect(s"/${repository.owner}/${repository.name}/pull/$newIssueId") + + case Left(errorMessage) => + // Clean up the branch we created + git.branchDelete().setForce(true).setBranchNames(revertBranch).call() + BadRequest(s"Failed to create revert commit: $errorMessage") + } + } catch { + case ex: Exception => + BadRequest(s"Revert failed: ${ex.getMessage}") + } + } + }) getOrElse NotFound() + } + }) + /** - * Tests whether an logged-in user can manage pull requests. + * Tests whether the logged-in user can manage pull requests. */ private def isManageable(repository: RepositoryInfo)(implicit context: Context): Boolean = { hasDeveloperRole(repository.owner, repository.name, context.loginAccount) } /** - * Tests whether an logged-in user can post pull requests. + * Tests whether the logged-in user can post pull requests. */ private def isEditable(repository: RepositoryInfo)(implicit context: Context): Boolean = { repository.repository.options.issuesOption match { @@ -740,5 +814,4 @@ trait PullRequestsControllerBase extends ControllerBase { case "DISABLE" => false } } - } diff --git a/src/main/scala/gitbucket/core/model/PullRequest.scala b/src/main/scala/gitbucket/core/model/PullRequest.scala index 570f5a7b1..2a2d60138 100644 --- a/src/main/scala/gitbucket/core/model/PullRequest.scala +++ b/src/main/scala/gitbucket/core/model/PullRequest.scala @@ -13,6 +13,7 @@ trait PullRequestComponent extends TemplateComponent { self: Profile => val commitIdFrom = column[String]("COMMIT_ID_FROM") val commitIdTo = column[String]("COMMIT_ID_TO") val isDraft = column[Boolean]("IS_DRAFT") + val mergedCommitIds = column[String]("MERGED_COMMIT_IDS") def * = ( userName, @@ -24,12 +25,13 @@ trait PullRequestComponent extends TemplateComponent { self: Profile => requestBranch, commitIdFrom, commitIdTo, - isDraft + isDraft, + mergedCommitIds.? ).mapTo[PullRequest] - def byPrimaryKey(userName: String, repositoryName: String, issueId: Int) = + def byPrimaryKey(userName: String, repositoryName: String, issueId: Int): Rep[Boolean] = byIssue(userName, repositoryName, issueId) - def byPrimaryKey(userName: Rep[String], repositoryName: Rep[String], issueId: Rep[Int]) = + def byPrimaryKey(userName: Rep[String], repositoryName: Rep[String], issueId: Rep[Int]): Rep[Boolean] = byIssue(userName, repositoryName, issueId) } } @@ -44,5 +46,6 @@ case class PullRequest( requestBranch: String, commitIdFrom: String, commitIdTo: String, - isDraft: Boolean + isDraft: Boolean, + mergedCommitIds: Option[String] ) diff --git a/src/main/scala/gitbucket/core/service/MergeService.scala b/src/main/scala/gitbucket/core/service/MergeService.scala index 1e7637940..fc721af48 100644 --- a/src/main/scala/gitbucket/core/service/MergeService.scala +++ b/src/main/scala/gitbucket/core/service/MergeService.scala @@ -7,7 +7,7 @@ import gitbucket.core.plugin.{PluginRegistry, ReceiveHook} import gitbucket.core.service.RepositoryService.RepositoryInfo import gitbucket.core.util.Directory._ import gitbucket.core.util.{JGitUtil, LockUtil} -import gitbucket.core.model.Profile.profile.blockingApi._ +import gitbucket.core.model.Profile.profile.blockingApi.* import gitbucket.core.model.activity.{CloseIssueInfo, MergeInfo, PushInfo} import gitbucket.core.service.SystemSettingsService.SystemSettings import gitbucket.core.service.WebHookService.WebHookPushPayload @@ -19,14 +19,14 @@ import org.eclipse.jgit.errors.NoMergeBaseException import org.eclipse.jgit.lib.{CommitBuilder, ObjectId, PersonIdent, Repository} import org.eclipse.jgit.revwalk.{RevCommit, RevWalk} -import scala.jdk.CollectionConverters._ +import scala.jdk.CollectionConverters.* import scala.util.Using trait MergeService { self: AccountService & ActivityService & IssuesService & RepositoryService & PullRequestService & WebHookPullRequestService & WebHookService => - import MergeService._ + import MergeService.* /** * Checks whether conflict will be caused in merging within pull request. @@ -61,15 +61,16 @@ trait MergeService { repository: RepositoryInfo, branch: String, issueId: Int, + commits: Seq[RevCommit], message: String, loginAccount: Account, settings: SystemSettings - )(implicit s: Session, c: JsonFormat.Context): ObjectId = { + )(implicit s: Session, c: JsonFormat.Context): MergeResult = { val beforeCommitId = git.getRepository.resolve(s"refs/heads/${branch}") - val afterCommitId = new MergeCacheInfo(git, repository.owner, repository.name, branch, issueId, getReceiveHooks()) - .merge(message, new PersonIdent(loginAccount.fullName, loginAccount.mailAddress), loginAccount.userName) - callWebHook(git, repository, branch, beforeCommitId, afterCommitId, loginAccount, settings) - afterCommitId + val mergeResult = new MergeCacheInfo(git, repository.owner, repository.name, branch, issueId, getReceiveHooks()) + .merge(message, new PersonIdent(loginAccount.fullName, loginAccount.mailAddress), loginAccount.userName, commits) + callWebHook(git, repository, branch, beforeCommitId, mergeResult.newCommitId, loginAccount, settings) + mergeResult } /** rebase to the head of the pull request branch */ @@ -81,13 +82,13 @@ trait MergeService { commits: Seq[RevCommit], loginAccount: Account, settings: SystemSettings - )(implicit s: Session, c: JsonFormat.Context): ObjectId = { + )(implicit s: Session, c: JsonFormat.Context): MergeResult = { val beforeCommitId = git.getRepository.resolve(s"refs/heads/${branch}") - val afterCommitId = + val mergeResult = new MergeCacheInfo(git, repository.owner, repository.name, branch, issueId, getReceiveHooks()) .rebase(new PersonIdent(loginAccount.fullName, loginAccount.mailAddress), loginAccount.userName, commits) - callWebHook(git, repository, branch, beforeCommitId, afterCommitId, loginAccount, settings) - afterCommitId + callWebHook(git, repository, branch, beforeCommitId, mergeResult.newCommitId, loginAccount, settings) + mergeResult } /** squash commits in the pull request and append it */ @@ -99,13 +100,13 @@ trait MergeService { message: String, loginAccount: Account, settings: SystemSettings - )(implicit s: Session, c: JsonFormat.Context): ObjectId = { + )(implicit s: Session, c: JsonFormat.Context): MergeResult = { val beforeCommitId = git.getRepository.resolve(s"refs/heads/${branch}") - val afterCommitId = + val mergeResult = new MergeCacheInfo(git, repository.owner, repository.name, branch, issueId, getReceiveHooks()) .squash(message, new PersonIdent(loginAccount.fullName, loginAccount.mailAddress), loginAccount.userName) - callWebHook(git, repository, branch, beforeCommitId, afterCommitId, loginAccount, settings) - afterCommitId + callWebHook(git, repository, branch, beforeCommitId, mergeResult.newCommitId, loginAccount, settings) + mergeResult } private def callWebHook( @@ -337,7 +338,7 @@ trait MergeService { strategy: String, isDraft: Boolean, settings: SystemSettings - )(implicit s: Session, c: JsonFormat.Context, context: Context): Either[String, ObjectId] = { + )(implicit s: Session, c: JsonFormat.Context, context: Context): Either[String, MergeResult] = { if (!isDraft) { if (repository.repository.options.mergeOptions.split(",").contains(strategy)) { LockUtil.lock(s"${repository.owner}/${repository.name}") { @@ -493,7 +494,7 @@ trait MergeService { commits: Seq[Seq[CommitInfo]], receiveHooks: Seq[ReceiveHook], settings: SystemSettings - )(implicit s: Session, c: JsonFormat.Context): Option[ObjectId] = { + )(implicit s: Session, c: JsonFormat.Context): Option[MergeResult] = { val revCommits = Using .resource(new RevWalk(git.getRepository)) { revWalk => commits.flatten.map { commit => @@ -510,6 +511,7 @@ trait MergeService { repository, pullRequest.branch, issue.issueId, + revCommits, s"Merge pull request #${issue.issueId} from ${pullRequest.requestUserName}/${pullRequest.requestBranch}\n\n" + message, loginAccount, settings @@ -600,13 +602,13 @@ object MergeService { private val mergedBranchName = s"refs/pull/${issueId}/merge" private val conflictedBranchName = s"refs/pull/${issueId}/conflict" - lazy val mergeBaseTip = git.getRepository.resolve(s"refs/heads/${branch}") - lazy val mergeTip = git.getRepository.resolve(s"refs/pull/${issueId}/head") + lazy val mergeBaseTip: ObjectId = git.getRepository.resolve(s"refs/heads/${branch}") + lazy val mergeTip: ObjectId = git.getRepository.resolve(s"refs/pull/${issueId}/head") def checkConflictCache(): Option[Option[String]] = { Option(git.getRepository.resolve(mergedBranchName)) .flatMap { merged => - if (parseCommit(merged).getParents().toSet == Set(mergeBaseTip, mergeTip)) { + if (parseCommit(merged).getParents.toSet == Set(mergeBaseTip, mergeTip)) { // merged branch exists Some(None) } else { @@ -615,7 +617,7 @@ object MergeService { } .orElse(Option(git.getRepository.resolve(conflictedBranchName)).flatMap { conflicted => val commit = parseCommit(conflicted) - if (commit.getParents().toSet == Set(mergeBaseTip, mergeTip)) { + if (commit.getParents.toSet == Set(mergeBaseTip, mergeTip)) { // conflict branch exists Some(Some(commit.getFullMessage)) } else { @@ -651,14 +653,16 @@ object MergeService { None } else { val message = createConflictMessage(mergeTip, mergeBaseTip, merger) - _updateBranch(mergeTipCommit.getTree().getId(), message, conflictedBranchName) + _updateBranch(mergeTipCommit.getTree.getId, message, conflictedBranchName) git.branchDelete().setForce(true).setBranchNames(mergedBranchName).call() Some(message) } } // update branch from cache - def merge(message: String, committer: PersonIdent, pusher: String)(implicit s: Session): ObjectId = { + def merge(message: String, committer: PersonIdent, pusher: String, commits: Seq[RevCommit])(implicit + s: Session + ): MergeResult = { if (checkConflict().isDefined) { throw new RuntimeException("This pull request can't merge automatically.") } @@ -666,7 +670,7 @@ object MergeService { throw new RuntimeException(s"Not found branch ${mergedBranchName}") }) // creates merge commit - val mergeCommitId = createMergeCommit(mergeResultCommit.getTree().getId(), committer, message) + val mergeCommitId = createMergeCommit(mergeResultCommit.getTree.getId, committer, message) val refName = s"refs/heads/${branch}" val currentObjectId = git.getRepository.resolve(refName) @@ -690,10 +694,10 @@ object MergeService { hook.postReceive(userName, repositoryName, receivePack, receiveCommand, committer.getName, true) } - objectId + MergeResult(objectId, commits.map(_.name())) } - def rebase(committer: PersonIdent, pusher: String, commits: Seq[RevCommit])(implicit s: Session): ObjectId = { + def rebase(committer: PersonIdent, pusher: String, commits: Seq[RevCommit])(implicit s: Session): MergeResult = { if (checkConflict().isDefined) { throw new RuntimeException("This pull request can't merge automatically.") } @@ -713,11 +717,13 @@ object MergeService { val mergeBaseTipCommit = Using.resource(new RevWalk(git.getRepository))(_.parseCommit(mergeBaseTip)) var previousId = mergeBaseTipCommit.getId + val mergedCommitIds = Seq.newBuilder[String] Using.resource(git.getRepository.newObjectInserter) { inserter => commits.foreach { commit => val nextCommit = _cloneCommit(commit, previousId, mergeBaseTipCommit.getId) previousId = inserter.insert(nextCommit) + mergedCommitIds += previousId.name() } inserter.flush() } @@ -745,10 +751,10 @@ object MergeService { hook.postReceive(userName, repositoryName, receivePack, receiveCommand, committer.getName, true) } - objectId + MergeResult(objectId, mergedCommitIds.result()) } - def squash(message: String, committer: PersonIdent, pusher: String)(implicit s: Session): ObjectId = { + def squash(message: String, committer: PersonIdent, pusher: String)(implicit s: Session): MergeResult = { if (checkConflict().isDefined) { throw new RuntimeException("This pull request can't merge automatically.") } @@ -804,7 +810,7 @@ object MergeService { hook.postReceive(userName, repositoryName, receivePack, receiveCommand, committer.getName, true) } - objectId + MergeResult(objectId, Seq(newCommitId.name())) } // return treeId @@ -823,4 +829,5 @@ object MergeService { mergeResults.asScala.map { case (key, _) => "- `" + key + "`\n" }.mkString } + case class MergeResult(newCommitId: ObjectId, mergedCommitId: Seq[String]) } diff --git a/src/main/scala/gitbucket/core/service/PullRequestService.scala b/src/main/scala/gitbucket/core/service/PullRequestService.scala index 08348cedf..a93bb9d94 100644 --- a/src/main/scala/gitbucket/core/service/PullRequestService.scala +++ b/src/main/scala/gitbucket/core/service/PullRequestService.scala @@ -19,7 +19,10 @@ import gitbucket.core.util.StringUtil.* import gitbucket.core.view import gitbucket.core.view.helpers import org.eclipse.jgit.api.Git -import org.eclipse.jgit.lib.ObjectId +import org.eclipse.jgit.dircache.{DirCache, DirCacheEntry} +import org.eclipse.jgit.lib.{CommitBuilder, FileMode, ObjectId, ObjectInserter, PersonIdent, Repository} +import org.eclipse.jgit.revwalk.{RevTree, RevWalk} +import org.eclipse.jgit.treewalk.{EmptyTreeIterator, TreeWalk} import scala.jdk.CollectionConverters.* import scala.util.Using @@ -63,6 +66,15 @@ trait PullRequestService { .update((baseBranch, commitIdTo)) } + def updateMergedCommitIds(owner: String, repository: String, issueId: Int, mergedCommitIds: Seq[String])(implicit + s: Session + ): Unit = { + PullRequests + .filter(_.byPrimaryKey(owner, repository, issueId)) + .map(pr => pr.mergedCommitIds) + .update(mergedCommitIds.mkString(",")) + } + def getPullRequestCountGroupByUser(closed: Boolean, owner: Option[String], repository: Option[String])(implicit s: Session ): List[PullRequestCount] = @@ -126,7 +138,8 @@ trait PullRequestService { requestBranch, commitIdFrom, commitIdTo, - isDraft + isDraft, + None ) // fetch requested branch @@ -408,11 +421,10 @@ trait PullRequestService { .find(x => x.oldPath == file) .map { diff => (diff.oldContent, diff.newContent) match { - case (Some(oldContent), Some(newContent)) => { + case (Some(oldContent), Some(newContent)) => val oldLines = convertLineSeparator(oldContent, "LF").split("\n") val newLines = convertLineSeparator(newContent, "LF").split("\n") file -> Option(DiffUtils.diff(oldLines.toList.asJava, newLines.toList.asJava)) - } case _ => file -> None } @@ -524,7 +536,6 @@ trait PullRequestService { helpers.date(commit1.commitTime) == view.helpers.date(commit2.commitTime) } - // TODO Isolate to an another method? val diffs = JGitUtil.getDiffs( git = newGit, from = Some(oldId.getName), @@ -634,6 +645,157 @@ trait PullRequestService { } } } + + /** + * Creates a revert commit directly on the bare repository without cloning. + * This works by creating a reverse diff of the merged commits and applying it to the base branch. + */ + def createRevertCommit( + git: Git, + targetBranch: String, + mergedCommitIds: Seq[String], + committerName: String, + committerEmail: String, + commitMessage: String + ): Either[String, ObjectId] = { + try { + val repository = git.getRepository + val inserter = repository.newObjectInserter() + + Using.resource(new RevWalk(repository)) { revWalk => + // Get the target branch head + val targetHeadId = repository.resolve(s"refs/heads/$targetBranch") + if (targetHeadId == null) { + return Left(s"Branch $targetBranch not found") + } + val targetHead = revWalk.parseCommit(targetHeadId) + + // Parse the commits to revert (in reverse order for proper reverting) + val commitsToRevert = mergedCommitIds.reverse.map { commitId => + val objectId = repository.resolve(commitId) + if (objectId == null) { + throw new IllegalArgumentException(s"Commit $commitId not found") + } + revWalk.parseCommit(objectId) + } + + // Start with the current tree of the target branch + var currentTreeId = targetHead.getTree.getId + + // Apply reverse changes for each commit + for (commit <- commitsToRevert) { + val parentCommit = if (commit.getParentCount > 0) { + revWalk.parseCommit(commit.getParent(0)) + } else { + // This is an initial commit, revert by creating empty tree + null + } + + // Create new tree by applying reverse diff + currentTreeId = createTreeWithReverseDiff( + repository, + inserter, + currentTreeId, + if (parentCommit != null) parentCommit.getTree else null, + commit.getTree + ) + } + + // Create the revert commit + val commitBuilder = new CommitBuilder() + commitBuilder.setTreeId(currentTreeId) + commitBuilder.setParentId(targetHeadId) + commitBuilder.setAuthor(new PersonIdent(committerName, committerEmail)) + commitBuilder.setCommitter(new PersonIdent(committerName, committerEmail)) + commitBuilder.setMessage(commitMessage) + + val revertCommitId = inserter.insert(commitBuilder) + inserter.flush() + + // Update the branch to point to the new commit + val refUpdate = repository.updateRef(s"refs/heads/$targetBranch") + refUpdate.setNewObjectId(revertCommitId) + refUpdate.update() + + Right(revertCommitId) + } + } catch { + case ex: Exception => + Left(ex.getMessage) + } + } + + /** + * Creates a new tree by applying the reverse of changes between fromTree and toTree to baseTree. + */ + private def createTreeWithReverseDiff( + repository: Repository, + inserter: ObjectInserter, + baseTreeId: ObjectId, + fromTree: RevTree, + toTree: RevTree + ): ObjectId = { + val dirCache = DirCache.newInCore() + val builder = dirCache.builder() + + val entries = scala.collection.mutable.Map[String, DirCacheEntry]() + + // Start with all files from the base tree + if (baseTreeId != null) { + Using.resource(new TreeWalk(repository)) { walk => + walk.addTree(baseTreeId) + walk.setRecursive(true) + + while (walk.next()) { + val entry = new DirCacheEntry(walk.getPathString) + entry.setFileMode(walk.getFileMode(0)) + entry.setObjectId(walk.getObjectId(0)) + entries(walk.getPathString) = entry + } + } + } + + // Apply reverse changes: if a file was added in the original change, remove it + // if a file was deleted, restore it; if modified, restore original content + Using.resource(new TreeWalk(repository)) { walk => + if (fromTree != null) walk.addTree(fromTree) else walk.addTree(new EmptyTreeIterator()) + walk.addTree(toTree) + walk.setRecursive(true) + + while (walk.next()) { + val path = walk.getPathString + val fromMode = if (walk.getTreeCount > 1) walk.getFileMode(0) else FileMode.MISSING + val toMode = walk.getFileMode(walk.getTreeCount - 1) + + if (fromMode == FileMode.MISSING && toMode != FileMode.MISSING) { + // File was added in the original change, so remove it in the revert + entries.remove(path) + } else if (fromMode != FileMode.MISSING && toMode == FileMode.MISSING) { + // File was deleted in the original change, so restore it in the revert + val entry = new DirCacheEntry(path) + entry.setFileMode(fromMode) + entry.setObjectId(walk.getObjectId(0)) + entries(path) = entry + } else if (fromMode != FileMode.MISSING && toMode != FileMode.MISSING) { + val fromObjectId = walk.getObjectId(0) + val toObjectId = walk.getObjectId(walk.getTreeCount - 1) + + if (!fromObjectId.equals(toObjectId)) { + // File was modified in the original change, restore original content + val entry = new DirCacheEntry(path) + entry.setFileMode(fromMode) + entry.setObjectId(fromObjectId) + entries(path) = entry + } + } + } + } + + // Build the final tree + entries.values.toSeq.sortBy(_.getPathString).foreach(builder.add) + builder.finish() + dirCache.writeTree(inserter) + } } object PullRequestService { diff --git a/src/main/twirl/gitbucket/core/pulls/conversation.scala.html b/src/main/twirl/gitbucket/core/pulls/conversation.scala.html index 93878744f..f1c4e7e37 100644 --- a/src/main/twirl/gitbucket/core/pulls/conversation.scala.html +++ b/src/main/twirl/gitbucket/core/pulls/conversation.scala.html @@ -36,17 +36,37 @@ } - @if(isManageableForkedRepository && issue.closed && merged && - forkedRepository.exists(r => r.branchList.contains(pullreq.requestBranch) && r.repository.defaultBranch != pullreq.requestBranch)){ -
-
- Delete branch -
- Pull request successfully merged and closed + @defining( + isManageableForkedRepository && issue.closed && merged && + forkedRepository.exists(r => r.branchList.contains(pullreq.requestBranch) && r.repository.defaultBranch != pullreq.requestBranch), + issue.closed && pullreq.mergedCommitIds.isDefined + ) { case (isBranchDeletable, isRevertible) => + @if(isBranchDeletable || isRevertible) { +
+
+ @if(isRevertible) { +
+ +
+ } + @if(isBranchDeletable) { + Delete branch + } +
+ Pull request successfully merged and closed +
+ + @if(isBranchDeletable) { + The @pullreq.requestBranch branch can now be safely deleted. + } else { + You can create another pull request to revert this merge. + } +
- You're all set. The @pullreq.requestBranch branch can now be safely deleted.
-
+ } + @if(isBranchDeletable) { + } } @gitbucket.core.issues.html.commentform(issue, !merged, isEditable, isManageable, repository) } diff --git a/src/test/scala/gitbucket/core/api/ApiSpecModels.scala b/src/test/scala/gitbucket/core/api/ApiSpecModels.scala index f2d466755..94cb9a2df 100644 --- a/src/test/scala/gitbucket/core/api/ApiSpecModels.scala +++ b/src/test/scala/gitbucket/core/api/ApiSpecModels.scala @@ -154,7 +154,8 @@ object ApiSpecModels { requestBranch = "new-topic", commitIdFrom = sha1, commitIdTo = sha1, - isDraft = true + isDraft = true, + mergedCommitIds = None ) val commitComment: CommitComment = CommitComment( diff --git a/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala b/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala index 099998cb4..d560b8796 100644 --- a/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala +++ b/src/test/scala/gitbucket/core/service/MergeServiceSpec.scala @@ -171,6 +171,7 @@ class MergeServiceSpec extends AnyFunSpec with ServiceSpecBase { repository, branch, issueId, + Seq(git.getRepository.parseCommit(commitId)), "merged", context.loginAccount.get, context.settings diff --git a/src/test/scala/gitbucket/core/service/PullRequestServiceSpec.scala b/src/test/scala/gitbucket/core/service/PullRequestServiceSpec.scala index a7abde76c..14d584c8d 100644 --- a/src/test/scala/gitbucket/core/service/PullRequestServiceSpec.scala +++ b/src/test/scala/gitbucket/core/service/PullRequestServiceSpec.scala @@ -1,8 +1,16 @@ package gitbucket.core.service import gitbucket.core.model.* +import gitbucket.core.util.GitSpecUtil.* +import gitbucket.core.util.JGitUtil +import org.eclipse.jgit.api.Git +import org.eclipse.jgit.lib.Constants +import org.eclipse.jgit.treewalk.TreeWalk import org.scalatest.funspec.AnyFunSpec +import scala.collection.mutable +import scala.util.Using + class PullRequestServiceSpec extends AnyFunSpec with ServiceSpecBase @@ -21,7 +29,33 @@ class PullRequestServiceSpec with WebHookPullRequestReviewCommentService with RequestCache { - def swap(r: (Issue, PullRequest)) = (r._2 -> r._1) + private def swap(r: (Issue, PullRequest)) = (r._2 -> r._1) + + /** + * Returns a map of (path -> content) for all files in the tree of the given branch. + */ + private def getFiles(git: Git, branch: String): Map[String, String] = { + val repository = git.getRepository + val headId = repository.resolve(s"refs/heads/$branch") + if (headId == null) return Map.empty + Using.resource(new org.eclipse.jgit.revwalk.RevWalk(repository)) { revWalk => + val commit = revWalk.parseCommit(headId) + val tree = commit.getTree + val files = mutable.Map[String, String]() + Using.resource(new TreeWalk(repository)) { walk => + walk.addTree(tree) + walk.setRecursive(true) + while (walk.next()) { + val path = walk.getPathString + val objectId = walk.getObjectId(0) + val loader = repository.open(objectId) + val content = new String(loader.getBytes, "UTF-8") + files(path) = content + } + } + files.toMap + } + } describe("PullRequestService.getPullRequestFromBranch") { it("""should @@ -52,4 +86,277 @@ class PullRequestServiceSpec } } } + + describe("PullRequestService.createRevertCommit") { + it("should revert a single commit that modified a file") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "original content", message = "initial commit") + val commitToRevert = createFile(git, Constants.HEAD, "README.md", "modified content", message = "modify readme") + + assert(getFiles(git, "main")("README.md") == "modified content") + + val result = createRevertCommit( + git, + "main", + Seq(commitToRevert.getName), + "test-user", + "test@example.com", + "Revert modify readme" + ) + + assert(result.isRight) + assert(getFiles(git, "main")("README.md") == "original content") + } + } + + it("should revert a single commit that added a new file") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "readme content", message = "initial commit") + val commitToRevert = + createFile(git, Constants.HEAD, "NEW_FILE.txt", "new file content", message = "add new file") + + assert(getFiles(git, "main").contains("NEW_FILE.txt")) + + val result = createRevertCommit( + git, + "main", + Seq(commitToRevert.getName), + "test-user", + "test@example.com", + "Revert add new file" + ) + + assert(result.isRight) + val files = getFiles(git, "main") + assert(files.contains("README.md")) + assert(!files.contains("NEW_FILE.txt")) + } + } + + it("should revert a commit that deleted a file") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "readme content", message = "initial commit") + createFile(git, Constants.HEAD, "TO_DELETE.txt", "delete me", message = "add file to delete") + + // Delete file by creating a commit without it + val repository = git.getRepository + val inserter = repository.newObjectInserter() + val headId = repository.resolve("main^{commit}") + val builder = org.eclipse.jgit.dircache.DirCache.newInCore.builder() + JGitUtil.processTree(git, headId) { (path, tree) => + if (path != "TO_DELETE.txt") { + builder.add(JGitUtil.createDirCacheEntry(path, tree.getEntryFileMode, tree.getEntryObjectId)) + } + } + builder.finish() + val deleteCommitId = JGitUtil.createNewCommit( + git, + inserter, + headId, + builder.getDirCache.writeTree(inserter), + Constants.HEAD, + "dummy", + "dummy@example.com", + "delete file" + ) + inserter.flush() + inserter.close() + + assert(!getFiles(git, "main").contains("TO_DELETE.txt")) + + val result = createRevertCommit( + git, + "main", + Seq(deleteCommitId.getName), + "test-user", + "test@example.com", + "Revert delete file" + ) + + assert(result.isRight) + val files = getFiles(git, "main") + assert(files.contains("TO_DELETE.txt")) + assert(files("TO_DELETE.txt") == "delete me") + assert(files.contains("README.md")) + } + } + + it("should revert multiple commits in sequence") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "v1", message = "initial commit") + val commit1 = createFile(git, Constants.HEAD, "README.md", "v2", message = "update to v2") + val commit2 = createFile(git, Constants.HEAD, "README.md", "v3", message = "update to v3") + + assert(getFiles(git, "main")("README.md") == "v3") + + val result = createRevertCommit( + git, + "main", + Seq(commit1.getName, commit2.getName), + "test-user", + "test@example.com", + "Revert two commits" + ) + + assert(result.isRight) + assert(getFiles(git, "main")("README.md") == "v1") + } + } + + it("should preserve unrelated files") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "readme content", message = "initial commit") + createFile(git, Constants.HEAD, "OTHER.txt", "other content", message = "add other file") + val commitToRevert = + createFile(git, Constants.HEAD, "README.md", "changed readme", message = "modify readme only") + + val result = createRevertCommit( + git, + "main", + Seq(commitToRevert.getName), + "test-user", + "test@example.com", + "Revert readme change" + ) + + assert(result.isRight) + val files = getFiles(git, "main") + assert(files("README.md") == "readme content") + assert(files("OTHER.txt") == "other content") + } + } + + it("should return Left for non-existent branch") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "content", message = "initial commit") + val commitId = git.getRepository.resolve("main").getName + + val result = createRevertCommit( + git, + "nonexistent-branch", + Seq(commitId), + "test-user", + "test@example.com", + "Revert" + ) + + assert(result.isLeft) + } + } + + it("should revert commit with files in subdirectories") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "readme", message = "initial commit") + createFile(git, Constants.HEAD, "src/main/Hello.scala", "hello v1", message = "add hello") + val commitToRevert = + createFile(git, Constants.HEAD, "src/main/Hello.scala", "hello v2", message = "modify hello") + + assert(getFiles(git, "main")("src/main/Hello.scala") == "hello v2") + + val result = createRevertCommit( + git, + "main", + Seq(commitToRevert.getName), + "test-user", + "test@example.com", + "Revert hello change" + ) + + assert(result.isRight) + val files = getFiles(git, "main") + assert(files("src/main/Hello.scala") == "hello v1") + assert(files("README.md") == "readme") + } + } + + it("should create proper commit metadata") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "v1", message = "initial commit") + val commitToRevert = createFile(git, Constants.HEAD, "README.md", "v2", message = "modify") + + val result = createRevertCommit( + git, + "main", + Seq(commitToRevert.getName), + "revert-author", + "revert@example.com", + "Revert: modify" + ) + + assert(result.isRight) + + val headId = git.getRepository.resolve("refs/heads/main") + Using.resource(new org.eclipse.jgit.revwalk.RevWalk(git.getRepository)) { revWalk => + val revertCommit = revWalk.parseCommit(headId) + + assert(revertCommit.getFullMessage == "Revert: modify") + assert(revertCommit.getAuthorIdent.getName == "revert-author") + assert(revertCommit.getAuthorIdent.getEmailAddress == "revert@example.com") + assert(revertCommit.getParentCount == 1) + assert(revertCommit.getParent(0).getId.getName == commitToRevert.getName) + } + } + } + + it("should revert a commit that adds multiple files at once") { + withTestRepository { git => + createFile(git, Constants.HEAD, "README.md", "readme", message = "initial commit") + + // Create a commit that adds two files using the low-level API + val repository = git.getRepository + val inserter = repository.newObjectInserter() + val headId = repository.resolve("main^{commit}") + val builder = org.eclipse.jgit.dircache.DirCache.newInCore.builder() + JGitUtil.processTree(git, headId) { (path, tree) => + builder.add(JGitUtil.createDirCacheEntry(path, tree.getEntryFileMode, tree.getEntryObjectId)) + } + builder.add( + JGitUtil.createDirCacheEntry( + "file1.txt", + org.eclipse.jgit.lib.FileMode.REGULAR_FILE, + inserter.insert(Constants.OBJ_BLOB, "file1 content".getBytes("UTF-8")) + ) + ) + builder.add( + JGitUtil.createDirCacheEntry( + "file2.txt", + org.eclipse.jgit.lib.FileMode.REGULAR_FILE, + inserter.insert(Constants.OBJ_BLOB, "file2 content".getBytes("UTF-8")) + ) + ) + builder.finish() + val multiAddCommitId = JGitUtil.createNewCommit( + git, + inserter, + headId, + builder.getDirCache.writeTree(inserter), + Constants.HEAD, + "dummy", + "dummy@example.com", + "add two files" + ) + inserter.flush() + inserter.close() + + val filesBefore = getFiles(git, "main") + assert(filesBefore.contains("file1.txt")) + assert(filesBefore.contains("file2.txt")) + + val result = createRevertCommit( + git, + "main", + Seq(multiAddCommitId.getName), + "test-user", + "test@example.com", + "Revert multi add" + ) + + assert(result.isRight) + val filesAfter = getFiles(git, "main") + assert(filesAfter.contains("README.md")) + assert(!filesAfter.contains("file1.txt")) + assert(!filesAfter.contains("file2.txt")) + } + } + } }