From e46520cf039bfd44bd54e2e7beafcb762532f8ef Mon Sep 17 00:00:00 2001 From: Julia Beliaeva Date: Fri, 12 Oct 2018 02:08:12 +0300 Subject: [PATCH] [file-history] fix file history issues that arise from incorrect rename detection in trivial merge commits For some trivial merge commits, renames are not detected when file was heavily changed in the other branch. For other trivial merge commits renames are detected for small files that were added or deleted in the other branch. This missing or extra renames can make refine algorithm to track file name through the graph incorrectly. In order to deal with this problem, trivial merges should be eliminated before refine. Since before refine it is possible to have several files changed in the commit, some merge commits could be trivial for one file, but not trivial for the other. It seems that such cases should be rare, it is perhaps enough to remove as much trivial merges as possible before refine, then repeat the process afterwards if necessary. --- .../intellij/vcs/log/history/FileHistory.kt | 58 ++++++++++++++----- .../vcs/log/history/FileHistoryTest.kt | 4 -- 2 files changed, 45 insertions(+), 17 deletions(-) diff --git a/platform/vcs-log/impl/src/com/intellij/vcs/log/history/FileHistory.kt b/platform/vcs-log/impl/src/com/intellij/vcs/log/history/FileHistory.kt index f2d53669385c..db6f0a6c8a79 100644 --- a/platform/vcs-log/impl/src/com/intellij/vcs/log/history/FileHistory.kt +++ b/platform/vcs-log/impl/src/com/intellij/vcs/log/history/FileHistory.kt @@ -23,10 +23,7 @@ import com.intellij.vcs.log.graph.utils.BfsUtil.getCorrespondingParent import com.intellij.vcs.log.graph.utils.DfsUtil import com.intellij.vcs.log.graph.utils.LinearGraphUtils import com.intellij.vcs.log.graph.utils.impl.BitSetFlags -import gnu.trove.THashMap -import gnu.trove.THashSet -import gnu.trove.TIntObjectHashMap -import gnu.trove.TObjectHashingStrategy +import gnu.trove.* import java.util.* import java.util.function.BiConsumer @@ -36,22 +33,43 @@ internal class FileHistoryBuilder(private val startCommit: Int?, val pathsMap = mutableMapOf() override fun accept(controller: LinearGraphController, permanentGraphInfo: PermanentGraphInfo) { + val needToRepeat = removeTrivialMerges(controller, permanentGraphInfo) + pathsMap.putAll(refine(controller, startCommit, permanentGraphInfo)) - val trivialCandidates = mutableSetOf() - pathsMap.forEach { c, p -> - if (fileNamesData.isTrivialMerge(c, p.filePath)) { - trivialCandidates.add(c) + if (needToRepeat) { + LOG.info("Some merge commits were not excluded from file history for ${startPath.path}") + removeTrivialMerges(controller, permanentGraphInfo) + } + } + + private fun removeTrivialMerges(controller: LinearGraphController, permanentGraphInfo: PermanentGraphInfo): Boolean { + val trivialCandidates = TIntHashSet() + val nonTrivialMerges = TIntHashSet() + fileNamesData.forEach { _, commit, changes -> + if (changes.size() > 1) { + if (changes.containsValue(ChangeKind.NOT_CHANGED)) { + trivialCandidates.add(commit) + } + else { + nonTrivialMerges.add(commit) + } } } + // since this code can be executed before refine, there can be commits with several files changed in them + // if several files are changed in the merge commit, it can be trivial for one file, but not trivial for the other + // in this case we may need to repeat the process after refine + val needToRepeat = trivialCandidates.removeAll(nonTrivialMerges) modifyGraph(controller) { collapsedGraph -> val trivialMerges = hideTrivialMerges(collapsedGraph) { nodeId: Int -> trivialCandidates.contains(permanentGraphInfo.permanentCommitsInfo.getCommitId(nodeId)) } if (trivialMerges.isNotEmpty()) LOG.debug("Excluding ${trivialMerges.size} trivial merges from history for ${startPath.path}") - trivialMerges.forEach { pathsMap.remove(permanentGraphInfo.permanentCommitsInfo.getCommitId(it)) } + fileNamesData.removeAll(trivialMerges.map { permanentGraphInfo.permanentCommitsInfo.getCommitId(it) }) } + + return needToRepeat } private fun refine(controller: LinearGraphController, @@ -305,10 +323,12 @@ abstract class FileNamesData(filePath: FilePath) { return result } - fun isTrivialMerge(commit: Int, filePath: FilePath): Boolean { - return affectedCommits[filePath]?.get(commit)?.let { - it.size() > 1 && it.containsValue(ChangeKind.NOT_CHANGED) - } ?: false + fun getChanges(filePath: FilePath, commit: Int) = affectedCommits[filePath]?.get(commit) + + fun forEach(action: (FilePath, Int, TIntObjectHashMap) -> Unit) = affectedCommits.forEach(action) + + fun removeAll(commits: List) { + affectedCommits.forEach { _, commitsMap -> commitsMap.removeAll(commits) } } abstract fun findRename(parent: Int, child: Int, accept: (Couple) -> Boolean): Couple? @@ -425,6 +445,18 @@ internal fun Map.removeAll(keys: List) { + keys.forEach { this.remove(it) } +} + +internal fun TIntHashSet.removeAll(elements: TIntHashSet): Boolean { + var result = false + for (i in elements) { + result = this.remove(i) or result + } + return result +} + private fun Collection.firstNotNull(mapping: (E) -> R): R? { for (e in this) { val value = mapping(e) diff --git a/platform/vcs-log/impl/test/com/intellij/vcs/log/history/FileHistoryTest.kt b/platform/vcs-log/impl/test/com/intellij/vcs/log/history/FileHistoryTest.kt index 8b340f60b280..b8ff5ac584c3 100644 --- a/platform/vcs-log/impl/test/com/intellij/vcs/log/history/FileHistoryTest.kt +++ b/platform/vcs-log/impl/test/com/intellij/vcs/log/history/FileHistoryTest.kt @@ -19,7 +19,6 @@ import gnu.trove.THashMap import gnu.trove.TIntObjectHashMap import org.junit.Assert import org.junit.Assume.assumeFalse -import org.junit.Ignore import org.junit.Test class FileHistoryTest { @@ -161,10 +160,7 @@ class FileHistoryTest { /** * Rename happens in one branch, while the other branch only consists of couple of trivial merge commits. - * Refiner walks to the trivial branch first instead of meaningful branch and because of this misses the rename completely. - * Solution would be to drop trivial merges before refining and always walk to the NOT_CHANGED branch. */ - @Ignore @Test fun historyWithUndetectedRename() { val after = LocalFilePath("after.txt", false)