From 64e8167fcbc1aea06b08e4b736f1a6a394ff1dbb Mon Sep 17 00:00:00 2001 From: ziggystar Date: Thu, 5 Dec 2024 00:51:03 +0100 Subject: [PATCH] fix: calculate diffs between commits based on common ancestor; works for force push (#3647) fixes #3476 Co-authored-by: Thomas Geier --- .../core/controller/ReleasesController.scala | 7 +-- .../scala/gitbucket/core/util/JGitUtil.scala | 58 +++++++++++++++++-- .../gitbucket/core/util/JGitUtilSpec.scala | 56 ++++++++++++++++++ 3 files changed, 111 insertions(+), 10 deletions(-) diff --git a/src/main/scala/gitbucket/core/controller/ReleasesController.scala b/src/main/scala/gitbucket/core/controller/ReleasesController.scala index f572bc2bc..8d75e96e6 100644 --- a/src/main/scala/gitbucket/core/controller/ReleasesController.scala +++ b/src/main/scala/gitbucket/core/controller/ReleasesController.scala @@ -130,11 +130,10 @@ trait ReleaseControllerBase extends ControllerBase { }) get("/:owner/:repository/changelog/*...*")(writableUsersOnly { repository => - val Seq(previousTag, currentTag) = multiParams("splat") - val previousTagId = repository.tags.collectFirst { case x if x.name == previousTag => x.commitId }.getOrElse("") - val commitLog = Using.resource(Git.open(getRepositoryDir(repository.owner, repository.name))) { git => - val commits = JGitUtil.getCommitLog(git, previousTagId, currentTag).reverse + val Seq(previousTag, currentTag) = multiParams("splat") + + val commits = JGitUtil.getCommitLog(git, previousTag, currentTag).reverse commits .map { commit => s"- ${commit.shortMessage} ${commit.id}" diff --git a/src/main/scala/gitbucket/core/util/JGitUtil.scala b/src/main/scala/gitbucket/core/util/JGitUtil.scala index 4d080f9b6..067f8dbd0 100644 --- a/src/main/scala/gitbucket/core/util/JGitUtil.scala +++ b/src/main/scala/gitbucket/core/util/JGitUtil.scala @@ -28,6 +28,7 @@ import org.eclipse.jgit.util.io.DisabledOutputStream import org.slf4j.LoggerFactory import scala.util.Using.Releasable +import scala.util.{Try, Using} /** * Provides complex JGit operations. @@ -659,17 +660,62 @@ object JGitUtil { } } + /** + * Returns the commit list between two revisions. + * `to` and `from` must be valid revision strings. + * + * @see [[org.eclipse.jgit.lib.Repository#resolve]] + * @param git the Git object + * @param from Must refer to a valid commit object. + * @param to Must refer to a valid commit object. + * @return The commits before 'to', that are not already present in the tree of 'from'. + */ + def getCommitLog(git: Git, from: String, to: String): List[CommitInfo] = { + def resolveString(name: String): ObjectId = { + val objectId = git.getRepository.resolve(name) + git.getRepository.open(objectId).getType match { + case Constants.OBJ_COMMIT => objectId + case Constants.OBJ_TAG => + val ref = git.getRepository.getRefDatabase.findRef(name) + git.getRepository.getRefDatabase.peel(ref).getPeeledObjectId + case _ => ObjectId.zeroId() + } + } + + getCommitLog(git, resolveString(from), resolveString(to)) + } + /** * Returns the commit list between two revisions. * * @param git the Git object - * @param from the from revision - * @param to the to revision - * @return the commit list + * @param from Must refer to a valid commit object. + * @param to Must refer to a valid commit object. + * @return The commits before 'to', that are not already present in the tree of 'from'. */ - // TODO swap parameters 'from' and 'to'!? - def getCommitLog(git: Git, from: String, to: String): List[CommitInfo] = - getCommitLogs(git, to)(_.getName == from) + def getCommitLog(git: Git, from: ObjectId, to: ObjectId): List[CommitInfo] = + Option(from) + .filter(f => f != ObjectId.zeroId) + // find the common ancestor of the two commits + .flatMap(f => + git + .log() + .add(f) + .add(to) + .setRevFilter(RevFilter.MERGE_BASE) + .call() + .asScala + .headOption + ) + .fold( + git.log() // no stop condition when merge base with 'from' is not found + )(f => git.log().not(f)) // we have a stop condition (start commit) + .add(to) + .call() + .asScala + .map(new CommitInfo(_)) + .toList + .reverse /** * Returns the latest RevCommit of the specified path. diff --git a/src/test/scala/gitbucket/core/util/JGitUtilSpec.scala b/src/test/scala/gitbucket/core/util/JGitUtilSpec.scala index cfeb0946b..95b6a51be 100644 --- a/src/test/scala/gitbucket/core/util/JGitUtilSpec.scala +++ b/src/test/scala/gitbucket/core/util/JGitUtilSpec.scala @@ -119,6 +119,62 @@ class JGitUtilSpec extends AnyFunSuite { } } + test("getCommitLog") { + withTestRepository { git => + /** repo looks like this + * commit1 -> commit2 -> commit3 [main] + * \-> commit4 [branch1] + * */ + val root = git.getRepository.resolve("main") + + createFile(git, Constants.HEAD, "README.md", "body1", message = "commit1") + val commit1 = git.getRepository.resolve("main") + + createFile(git, Constants.HEAD, "LICENSE", "Apache License", message = "commit2") + val commit2 = git.getRepository.resolve("main") + // also make a tag + JGitUtil.createTag(git, "t1", None, commit2.getName) + + createFile(git, Constants.HEAD, "README.md", "body1\nbody2", message = "commit3") + val commit3 = git.getRepository.resolve("main") + + // create branch + JGitUtil.createBranch(git, "main", "branch1") + createFile(git, "branch1", "README.md", "body2", message = "commit4") + val commit4 = git.getRepository.resolve("branch1") + + // compare results for empty → commit3 + assert( + JGitUtil.getCommitLogs(git, commit3.getName, includesLastCommit = true)(_ => false) == JGitUtil.getCommitLog( + git, + root, + commit3 + ) + ) + // compare results for commit1 → commit3 + assert( + JGitUtil.getCommitLogs(git, commit3.getName, includesLastCommit = true)( + _.getName != commit3.getName + ) == JGitUtil.getCommitLog(git, commit1, commit3) + ) + + // compare results for empty → commit4 + assert( + JGitUtil.getCommitLogs(git, commit4.getName, includesLastCommit = true)(_ => false) == JGitUtil.getCommitLog( + git, + root, + commit4 + ) + ) + + // check with names + assert(JGitUtil.getCommitLog(git, "main", "branch1").size == 1) + + // tag names must work, too + assertResult(JGitUtil.getCommitLog(git, "t1", "main").length)(1) + } + } + test("createBranch, branchesOfCommit and getBranches") { withTestRepository { git => createFile(git, Constants.HEAD, "README.md", "body1", message = "commit1")