Split merge commits related stats from the ones about non-merge commits. Did some small code refactoring to better support the separation Post review changes, refactored some of the test library code to streamline postconditions using ensuring statement Jira-Id: GERICS-610 Change-Id: Iaeda01475dd4616cf64ec8e4c28c26e49f019be3
diff --git a/src/main/scala/com/googlesource/gerrit/plugins/analytics/Contributors.scala b/src/main/scala/com/googlesource/gerrit/plugins/analytics/Contributors.scala index 23ff03c..6e16763 100644 --- a/src/main/scala/com/googlesource/gerrit/plugins/analytics/Contributors.scala +++ b/src/main/scala/com/googlesource/gerrit/plugins/analytics/Contributors.scala
@@ -132,7 +132,8 @@ histogram.get(repo, new AggregatedHistogramFilterByDates(startDate, stopDate, aggregationStrategy)) .par - .map(UserActivitySummary.apply(stats)).toStream + .flatMap(UserActivitySummary.apply(stats)) + .toStream } } } @@ -150,27 +151,27 @@ addedLines: Integer, deletedLines: Integer, commits: Array[CommitInfo], - lastCommitDate: Long) + lastCommitDate: Long, + isMerge: Boolean + ) object UserActivitySummary { - def apply(statisticsHandler: Statistics)(uca: AggregatedUserCommitActivity): UserActivitySummary = { + def apply(statisticsHandler: Statistics)(uca: AggregatedUserCommitActivity): Iterable[UserActivitySummary] = { val INCLUDESEMPTY = -1 implicit def stringToIntOrNull(x: String): Integer = if (x.isEmpty) null else new Integer(x) uca.key.split("/", INCLUDESEMPTY) match { - case a@Array(email, year, month, day, hour) => - val commits = getCommits(uca.getIds, uca.getTimes, uca.getMerges) - val stats = statisticsHandler.find(uca.getIds.toSeq) - UserActivitySummary(year, month, day, hour, uca.getName, uca.getEmail, uca.getCount, stats.numFiles, - stats.addedLines, stats.deletedLines, commits, uca.getLatest) + case Array(email, year, month, day, hour) => + statisticsHandler.forCommits(uca.getIds: _*).map { stat => + UserActivitySummary( + year, month, day, hour, uca.getName, uca.getEmail, uca.getCount, + stat.numFiles, stat.addedLines, stat.deletedLines, stat.commits.toArray, uca.getLatest, stat.isForMergeCommits + ) + } + case _ => throw new Exception(s"invalid key format found ${uca.key}") } } - - private def getCommits(ids: Array[ObjectId], times: Array[Long], merges: Array[Boolean]): - Array[CommitInfo] = { - (ids, times, merges).zipped.map((id, time, merge) => CommitInfo(id.name, time, merge)) - } }
diff --git a/src/main/scala/com/googlesource/gerrit/plugins/analytics/common/CommitsStatistics.scala b/src/main/scala/com/googlesource/gerrit/plugins/analytics/common/CommitsStatistics.scala index efff4cb..7d434b1 100644 --- a/src/main/scala/com/googlesource/gerrit/plugins/analytics/common/CommitsStatistics.scala +++ b/src/main/scala/com/googlesource/gerrit/plugins/analytics/common/CommitsStatistics.scala
@@ -14,32 +14,59 @@ package com.googlesource.gerrit.plugins.analytics.common +import com.googlesource.gerrit.plugins.analytics.CommitInfo +import com.googlesource.gerrit.plugins.analytics.common.ManagedResource.use import org.eclipse.jgit.diff.{DiffFormatter, RawTextComparator} import org.eclipse.jgit.lib.{ObjectId, Repository} -import org.eclipse.jgit.revwalk.RevWalk +import org.eclipse.jgit.revwalk.{RevCommit, RevWalk} import org.eclipse.jgit.treewalk.{CanonicalTreeParser, EmptyTreeIterator} import org.eclipse.jgit.util.io.DisabledOutputStream -import ManagedResource.use import scala.collection.JavaConversions._ -case class CommitsStatistics(numFiles: Int, addedLines: Int, deletedLines: Int) +case class CommitsStatistics(numFiles: Int, addedLines: Int, deletedLines: Int, isForMergeCommits: Boolean, commits: List[CommitInfo]) { + require(commits.forall(_.merge == isForMergeCommits), s"Creating a stats object with isMergeCommit = $isForMergeCommits but containing commits of different type") + + def isEmpty = commits.isEmpty + + // Is not a proper monoid since we cannot sum a MergeCommit with a non merge one but it would overkill to define two classes + protected [common] def + (that: CommitsStatistics) = { + require(this.isForMergeCommits == that.isForMergeCommits, "Cannot sum a merge commit stats with a non merge commit stats") + this.copy( + numFiles = this.numFiles + that.numFiles, + addedLines = this.addedLines + that.addedLines, + deletedLines = this.deletedLines + that.deletedLines, + commits = this.commits ++ that.commits + ) + } +} object CommitsStatistics { - val Empty = CommitsStatistics(0, 0, 0) + val Empty = CommitsStatistics(0, 0, 0, false, List.empty) + val EmptyMerge = Empty.copy(isForMergeCommits = true) } class Statistics(repo: Repository) { - def find(objectIds: Seq[ObjectId]): CommitsStatistics = - objectIds.foldLeft(CommitsStatistics(0, 0, 0)) { - (acc, objectId) => { - val stats = find(objectId) - CommitsStatistics(acc.numFiles + stats.numFiles, acc.addedLines + stats.addedLines, acc.deletedLines + stats.deletedLines) - } - } + /** + * Returns up to two different CommitsStatistics object grouping the stats into merge and non-merge commits + * + * @param commits + * @return + */ + def forCommits(commits: ObjectId*): Iterable[CommitsStatistics] = { - def find(objectId: ObjectId): CommitsStatistics = { + val stats = commits.map(forSingleCommit) + + val nonMergeStats = stats.filterNot(_.isForMergeCommits).foldLeft(CommitsStatistics.Empty)(_ + _) + val mergeStats = stats.filter(_.isForMergeCommits).foldLeft(CommitsStatistics.EmptyMerge)(_ + _) + + List(nonMergeStats, mergeStats).filterNot(_.isEmpty) + } + + protected def forSingleCommit(objectId: ObjectId): CommitsStatistics = { + import RevisionBrowsingSupport._ + // I can imagine this kind of statistics is already being available in Gerrit but couldn't understand how to access it // which Injection can be useful for this task? use(new RevWalk(repo)) { rw => @@ -69,7 +96,10 @@ edit <- df.toFileHeader(diff).toEditList } yield Lines(edit.getEndA - edit.getBeginA, edit.getEndB - edit.getBeginB)).fold(Lines(0, 0))(_ + _) - CommitsStatistics(diffs.size, lines.added, lines.deleted) + val commitInfo = CommitInfo(objectId.getName, commit.getAuthorIdent.getWhen.getTime, commit.isMerge) + + CommitsStatistics(diffs.size, lines.added, lines.deleted, commitInfo.merge, List(commitInfo)) } } + }
diff --git a/src/main/scala/com/googlesource/gerrit/plugins/analytics/common/RevisionBrowsingSupport.scala b/src/main/scala/com/googlesource/gerrit/plugins/analytics/common/RevisionBrowsingSupport.scala new file mode 100644 index 0000000..d07143a --- /dev/null +++ b/src/main/scala/com/googlesource/gerrit/plugins/analytics/common/RevisionBrowsingSupport.scala
@@ -0,0 +1,11 @@ +package com.googlesource.gerrit.plugins.analytics.common + +import org.eclipse.jgit.revwalk.RevCommit + +object RevisionBrowsingSupport { + + implicit class PimpedRevCommit(val self: RevCommit) extends AnyVal { + def isMerge : Boolean = self.getParentCount > 1 + } + +}
diff --git a/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/CommitStatisticsSpec.scala b/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/CommitStatisticsSpec.scala index 315369f..6912e7b 100644 --- a/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/CommitStatisticsSpec.scala +++ b/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/CommitStatisticsSpec.scala
@@ -16,13 +16,14 @@ import java.util.Date +import com.googlesource.gerrit.plugins.analytics.CommitInfo import com.googlesource.gerrit.plugins.analytics.common.{CommitsStatistics, Statistics} +import org.eclipse.jgit.api.{Git, MergeResult} import org.eclipse.jgit.internal.storage.file.FileRepository +import org.eclipse.jgit.lib.ObjectId import org.eclipse.jgit.revwalk.RevCommit import org.scalatest.{FlatSpec, Inside, Matchers} -import scala.collection.JavaConverters._ - class CommitStatisticsSpec extends FlatSpec with GitTestCase with Matchers with Inside { @@ -31,16 +32,32 @@ val stats = new Statistics(repo) } - def commit(committer: String, fname: String, content: String): RevCommit = { + def commit(committer: String, fileName: String, content: String): RevCommit = { val date = new Date() val person = newPersonIdent(committer, committer, date) - add(testRepo, "afile.txt", content, author = person, committer = author) + add(testRepo, fileName, content, author = person, committer = author) + } + + def mergeCommit(committer: String, fname: String, content: String): MergeResult = { + val currentBranch = Git.open(testRepo).getRepository.getBranch + val tmpBranch = branch(testRepo, "tmp") + try { + val commitToMerge = commit(committer, fname, content) + checkout(currentBranch) + mergeBranch("tmp", true) + } finally { + deleteBranch(testRepo, tmpBranch.getName) + } + } + + def idWithInfo(revCommit: RevCommit): (ObjectId, CommitInfo) = { + revCommit.getId -> CommitInfo(revCommit.getId.getName, revCommit.getCommitterIdent.getWhen.getTime, revCommit.getParentCount > 1) } "CommitStatistics" should "stats a single file added" in new TestEnvironment { val change = commit("user", "file1.txt", "line1\nline2") - inside(stats.find(change)) { case s: CommitsStatistics => + inside(stats.forCommits(change)) { case List(s: CommitsStatistics) => s.numFiles should be(1) s.addedLines should be(2) s.deletedLines should be(0) @@ -50,19 +67,22 @@ it should "stats multiple files added" in new TestEnvironment { val initial = commit("user", "file1.txt", "line1\nline2\n") val second = add(testRepo, - List("file1.txt", "file2.txt").asJava, - List("line1\n", "line1\nline2\n").asJava, "second commit") - inside(stats.find(second)) { case s: CommitsStatistics => + List( + "file1.txt" -> "line1\n", + "file2.txt" -> "line1\nline2\n" + ), "second commit") + + inside(stats.forCommits(second)) { case List(s: CommitsStatistics) => s.numFiles should be(2) - s.addedLines should be(3) - s.deletedLines should be(0) + s.addedLines should be(2) + s.deletedLines should be(1) } } it should "stats lines eliminated" in new TestEnvironment { val initial = commit("user", "file1.txt", "line1\nline2\nline3") val second = commit("user", "file1.txt", "line1\n") - inside(stats.find(second)) { case s: CommitsStatistics => + inside(stats.forCommits(second)) { case List(s: CommitsStatistics) => s.numFiles should be(1) s.addedLines should be(0) s.deletedLines should be(2) @@ -71,22 +91,53 @@ it should "stats a Seq[RevCommit]" in new TestEnvironment { val initial = add(testRepo, - List("file1.txt", "file3.txt").asJava, - List("line1\n", "line1\nline2\n").asJava, "first commit") + List( + "file1.txt" -> "line1\n", + "file3.txt" -> "line1\nline2\n"), + "first commit") + val second = add(testRepo, - List("file1.txt", "file2.txt").asJava, - List("line1a\n", "line1\nline2\n").asJava, "second commit") - inside(stats.find(List(initial, second))) { case s: CommitsStatistics => - s.numFiles should be(4) - s.addedLines should be(6) - s.deletedLines should be(1) + List( + "file1.txt" -> "line1a\n", + "file2.txt" -> "line1\nline2\n"), + "second commit") + + inside(stats.forCommits(initial, second)) { case List(nonMergeStats: CommitsStatistics) => + nonMergeStats.numFiles should be(4) //this is wrong? shouldn't we consider just three files? + nonMergeStats.addedLines should be(6) + nonMergeStats.deletedLines should be(1) } } - it should "not return any stats if the commit does not include any file" in new TestEnvironment { - val emptyCommit = add(testRepo, - List.empty[String].asJava, - List.empty[String].asJava, "Empty commit") - stats.find(emptyCommit) shouldBe CommitsStatistics.Empty + it should "return zero value stats if the commit does not include any file" in new TestEnvironment { + val emptyCommit = add(testRepo, List.empty, "Empty commit") + inside(stats.forCommits(emptyCommit)) { case List(stats) => + stats.numFiles should be(0) + stats.addedLines should be(0) + stats.deletedLines should be(0) + } + } + + it should "split merge commits and non-merge commits" in new TestEnvironment { + val firstNonMerge = commit("user", "file1.txt", "line1\nline2\n") + val merge = mergeCommit("user", "file1.txt", "line1\nline2\nline3") + val nonMerge = add(testRepo, + List( + "file1.txt" -> "line1\n", + "file2.txt" -> "line1\nline2\n"), + "second commit") + + inside(stats.forCommits(firstNonMerge, merge.getNewHead, nonMerge)) { + case List(nonMergeStats, mergeStats) => + mergeStats.numFiles should be(1) + mergeStats.addedLines should be(1) + mergeStats.deletedLines should be(0) + + nonMergeStats.numFiles should be(3) + nonMergeStats.addedLines should be(4) + nonMergeStats.deletedLines should be(2) + + case wrongContent => fail(s"Expected two results instad got $wrongContent") + } } }
diff --git a/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/GitTestCase.scala b/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/GitTestCase.scala index 6375b7f..423f92e 100644 --- a/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/GitTestCase.scala +++ b/src/test/scala/com/googlesource/gerrit/plugins/analytics/test/GitTestCase.scala
@@ -41,7 +41,10 @@ import java.util.Date import java.util +import com.googlesource.gerrit.plugins.analytics.common.ManagedResource +import com.googlesource.gerrit.plugins.analytics.common.ManagedResource.use import org.eclipse.jgit.api.Git +import org.eclipse.jgit.api.MergeCommand.FastForwardMode import org.eclipse.jgit.api.MergeResult import org.eclipse.jgit.api.errors.GitAPIException import org.eclipse.jgit.lib.Constants @@ -116,6 +119,15 @@ protected def branch(name: String): Ref = branch(testRepo, name) /** + * Delete branch with name + * + * @param name + * @return branch ref + * + */ + protected def deleteBranch(name: String): String = deleteBranch(testRepo, name) + + /** * Create branch with name and checkout * * @param repo @@ -124,9 +136,24 @@ * */ protected def branch(repo: File, name: String): Ref = { - val git = Git.open(repo) - git.branchCreate.setName(name).call - checkout(repo, name) + use(Git.open(repo)) { git => + git.branchCreate.setName(name).call + checkout(repo, name) + } + } + + /** + * Delete branch with name + * + * @param repo + * @param name + * @return branch ref + * + */ + protected def deleteBranch(repo: File, name: String): String = { + use(Git.open(repo)) { git => + git.branchDelete().setBranchNames(name).call.get(0) + } } /** @@ -148,11 +175,10 @@ */ @throws[Exception] protected def checkout(repo: File, name: String): Ref = { - val git = Git.open(repo) - val ref = git.checkout.setName(name).call - assert(ref != null) - ref - } + use(Git.open(repo)) { git => + git.checkout.setName(name).call + } + } ensuring(_ != null, "Unable to checkout result") /** * Create tag with name @@ -172,12 +198,11 @@ * */ protected def tag(repo: File, name: String): Ref = { - val git = Git.open(repo) - git.tag.setName(name).setMessage(name).call - val tagRef = git.getRepository.getTags.get(name) - assert(tagRef != null) - tagRef - } + use(Git.open(repo)) { git => + git.tag.setName(name).setMessage(name).call + git.getRepository.getTags.get(name) + } + } ensuring(_ != null, s"Unable to tag file $name") /** * Add file to test repository @@ -218,16 +243,17 @@ val file = new File(repo.getParentFile, path) if (!file.getParentFile.exists) assert(file.getParentFile.mkdirs) if (!file.exists) assert(file.createNewFile) + val writer = new PrintWriter(file) try writer.print(Option(content).fold("")(identity)) finally writer.close() - val git = Git.open(repo) - git.add.addFilepattern(path).call - val commit = git.commit.setOnly(path).setMessage(message).setAuthor(author).setCommitter(committer).call - assert(null != commit) - commit - } + + use(Git.open(repo)) { git => + git.add.addFilepattern(path).call + git.commit.setOnly(path).setMessage(message).setAuthor(author).setCommitter(committer).call + } + } ensuring (_ != null, s"Unable to commit addition of path $path") /** * Move file in test repository @@ -263,63 +289,64 @@ protected def mv(repo: File, from: String, to: String, message: String): RevCommit = { val file = new File(repo.getParentFile, from) file.renameTo(new File(repo.getParentFile, to)) - val git = Git.open(repo) - git.rm.addFilepattern(from) - git.add.addFilepattern(to).call - val commit = git.commit.setAll(true).setMessage(message).setAuthor(author).setCommitter(committer).call - assert(null != commit) - commit - } + + use(Git.open(testRepo)) { git => + git.rm.addFilepattern(from) + git.add.addFilepattern(to).call + git.commit.setAll(true).setMessage(message).setAuthor(author).setCommitter(committer).call + } + } ensuring (_ != null, "Unable to commit MV operation") /** * Add files to test repository * - * @param paths - * @param contents + * @param contents iterable of file names and associated content * @return commit * */ - protected def add(paths: util.List[String], contents: util.List[String]): RevCommit = add(testRepo, paths, contents, "Committing multiple files") + protected def add(contents: Iterable[(String, String)]): RevCommit = add(testRepo, contents, "Committing multiple files") /** * Add files to test repository * * @param repo - * @param paths - * @param contents + * @param contents iterable of file names and associated content * @param message * @return commit * */ - protected def add(repo: File, paths: util.List[String], contents: util.List[String], message: String): RevCommit = { - val git = Git.open(repo) - var i = 0 - while ( { - i < paths.size - }) { - val path = paths.get(i) - var content = contents.get(i) - val file = new File(repo.getParentFile, path) - if (!file.getParentFile.exists) assert(file.getParentFile.mkdirs) - if (!file.exists) assert(file.createNewFile) - val writer = new PrintWriter(file) - if (content == null) content = "" - try - writer.print(content) - finally writer.close() - git.add.addFilepattern(path).call - - { - i += 1; - i - 1 + protected def add(repo: File, contents: Iterable[(String, String)], message: String): RevCommit = { + use(Git.open(testRepo)) { git => + var i = 0 + contents.foreach { case (path, content) => + val file = new File(repo.getParentFile, path) + if (!file.getParentFile.exists) require(file.getParentFile.mkdirs, s"Cannot create parent dir '${file.getParent}'") + if (!file.exists) require(file.createNewFile, s"Cannot create file '$file'") + val writer = new PrintWriter(file) + try + writer.print(content) + finally writer.close() + git.add.addFilepattern(path).call } + + git.commit.setMessage(message).setAuthor(author).setCommitter(committer).call } - val commit = git.commit.setMessage(message).setAuthor(author).setCommitter(committer).call - assert(null != commit) - commit - } + } ensuring (_ != null, "Unable to commit content addition") /** + * Merge given branch into current branch + * + * @param branch + * @return result + * + */ + protected def mergeBranch(branch: String, withCommit: Boolean): MergeResult = { + use(Git.open(testRepo)) { git => + git.merge.setStrategy(MergeStrategy.RESOLVE).include(CommitUtils.getRef(git.getRepository, branch)).setCommit(withCommit).setFastForward(FastForwardMode.NO_FF).setMessage(s"merging branch $branch").call + } + } + + /** * Merge ref into current branch * * @param ref @@ -327,8 +354,9 @@ * */ protected def merge(ref: String): MergeResult = { - val git = Git.open(testRepo) - git.merge.setStrategy(MergeStrategy.RESOLVE).include(CommitUtils.getCommit(git.getRepository, ref)).call + use(Git.open(testRepo)) { git => + git.merge.setStrategy(MergeStrategy.RESOLVE).include(CommitUtils.getCommit(git.getRepository, ref)).call + } } /** @@ -349,11 +377,10 @@ * */ protected def note(content: String, ref: String): Note = { - val git = Git.open(testRepo) - val note = git.notesAdd.setMessage(content).setNotesRef(Constants.R_NOTES + ref).setObjectId(CommitUtils.getHead(git.getRepository)).call - assert(null != note) - note - } + use(Git.open(testRepo)) { git => + git.notesAdd.setMessage(content).setNotesRef(Constants.R_NOTES + ref).setObjectId(CommitUtils.getHead(git.getRepository)).call + } + } ensuring (_ != null, "Unable to add note") /** * Delete and commit file at path @@ -364,10 +391,9 @@ */ protected def delete(path: String): RevCommit = { val message = MessageFormat.format("Committing {0} at {1}", path, new Date) - val git = Git.open(testRepo) - git.rm.addFilepattern(path).call - val commit = git.commit.setOnly(path).setMessage(message).setAuthor(author).setCommitter(committer).call - assert(null != commit) - commit - } + use(Git.open(testRepo)) { git => + git.rm.addFilepattern(path).call + git.commit.setOnly(path).setMessage(message).setAuthor(author).setCommitter(committer).call + } + } ensuring (_ != null, "Unable to commit delete operation") } \ No newline at end of file