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
