From 37b3d7b06a5d23ab9ea5e112b8b9d57cd6461e0e Mon Sep 17 00:00:00 2001 From: Julia Beliaeva Date: Wed, 12 Dec 2018 18:46:41 +0300 Subject: [PATCH] [file-history] track paths more precisely during history calculation File history worked incorrectly when a file was renamed and later on a new file with the same name appeared. Commits before the first rename appeared in history for a new file, while they should not have, since they are a part of different history. In order to fix the problem, paths tracking was rewritten. It now takes deleted flag into the account, and if file is deleted, tries to find a conflicting rename. --- .../intellij/vcs/log/history/FileHistory.kt | 66 ++++++++++++++----- .../vcs/log/history/FileHistoryTest.kt | 66 +++++++++++++++++++ 2 files changed, 115 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 4aaf3df4f1d5..87f4d5a2a27c 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 @@ -173,7 +173,7 @@ internal class FileHistoryRefiner(private val visibleLinearGraph: LinearGraph, LinearGraphUtils.asLiteLinearGraph(visibleLinearGraph).walk(row, this) pathsForCommits.forEach { commit, path -> - if (path != null && !namesData.affects(commit, path)) { + if (path != null && !namesData.affects(commit, path, true)) { excluded.add(commit) } } @@ -195,14 +195,14 @@ internal class FileHistoryRefiner(private val visibleLinearGraph: LinearGraph, currentPath = if (down) { val pathGetter = { parentIndex: Int -> - namesData.getPathInParentRevision(previousCommit, permanentCommitsInfo.getCommitId(parentIndex), previousPath.filePath) + namesData.getPathInParentRevision(previousCommit, permanentCommitsInfo.getCommitId(parentIndex), previousPath) } val path = findPathWithoutConflict(previousNodeId, pathGetter) path ?: pathGetter(permanentLinearGraph.getCorrespondingParent(previousNodeId, currentNodeId, visibilityBuffer)) } else { val pathGetter = { parentIndex: Int -> - namesData.getPathInChildRevision(currentCommit, permanentCommitsInfo.getCommitId(parentIndex), previousPath.filePath) + namesData.getPathInChildRevision(currentCommit, permanentCommitsInfo.getCommitId(parentIndex), previousPath) } val path = findPathWithoutConflict(currentNodeId, pathGetter) // since in reality there is no edge between the nodes, but the whole path, we need to know, which parent is affected by this path @@ -281,27 +281,59 @@ abstract class FileNamesData(filePath: FilePath) { return null } - fun getPathInParentRevision(commit: Int, parent: Int, childPath: FilePath): MaybeDeletedFilePath { - val commits = UnorderedPair(commit, parent) - val otherPath = commitToRename.get(commits).firstNotNull { rename -> rename.getOtherPath(commit, childPath) } - if (otherPath != null) return MaybeDeletedFilePath(otherPath) + fun getPathInParentRevision(commit: Int, parent: Int, childPath: MaybeDeletedFilePath): MaybeDeletedFilePath { + val childFilePath = childPath.filePath + val changeKind = affectedCommits[childFilePath]?.get(commit)?.get(parent) ?: return childPath + if (changeKind == ChangeKind.NOT_CHANGED) return childPath - val changes = affectedCommits[childPath]?.get(commit) ?: return MaybeDeletedFilePath(childPath) - return MaybeDeletedFilePath(childPath, changes[parent] == ChangeKind.ADDED) + val renames = commitToRename.get(UnorderedPair(commit, parent)) + if (!childPath.deleted) { + val otherPath = renames.firstNotNull { rename -> rename.getOtherPath(commit, childFilePath) } + if (otherPath != null) return MaybeDeletedFilePath(otherPath) + return MaybeDeletedFilePath(childFilePath, changeKind == ChangeKind.ADDED) + } + + if (changeKind == ChangeKind.REMOVED) { + // checking if this is actually an unrelated rename + if (renames.firstNotNull { rename -> rename.getOtherPath(parent, childFilePath) } != null) return childPath + } + return MaybeDeletedFilePath(childFilePath, changeKind != ChangeKind.REMOVED) } - fun getPathInChildRevision(commit: Int, parent: Int, parentPath: FilePath): MaybeDeletedFilePath { - val commits = UnorderedPair(commit, parent) - val otherPath = commitToRename.get(commits).firstNotNull { rename -> rename.getOtherPath(parent, parentPath) } - if (otherPath != null) return MaybeDeletedFilePath(otherPath) + fun getPathInChildRevision(commit: Int, parent: Int, parentPath: MaybeDeletedFilePath): MaybeDeletedFilePath { + val parentFilePath = parentPath.filePath + val changeKind = affectedCommits[parentFilePath]?.get(commit)?.get(parent) ?: return parentPath + if (changeKind == ChangeKind.NOT_CHANGED) return parentPath - val changes = affectedCommits[parentPath]?.get(commit) ?: return MaybeDeletedFilePath(parentPath) - return MaybeDeletedFilePath(parentPath, changes[parent] == ChangeKind.REMOVED) + val renames = commitToRename.get(UnorderedPair(commit, parent)) + if (!parentPath.deleted) { + val otherPath = renames.firstNotNull { rename -> rename.getOtherPath(parent, parentFilePath) } + if (otherPath != null) return MaybeDeletedFilePath(otherPath) + return MaybeDeletedFilePath(parentFilePath, changeKind == ChangeKind.REMOVED) + } + + if (changeKind == ChangeKind.ADDED) { + // checking if this is actually an unrelated rename + if (renames.firstNotNull { rename -> rename.getOtherPath(commit, parentFilePath) } != null) return parentPath + } + return MaybeDeletedFilePath(parentFilePath, changeKind != ChangeKind.ADDED) } - fun affects(commit: Int, path: MaybeDeletedFilePath): Boolean { + fun affects(commit: Int, path: MaybeDeletedFilePath, verify: Boolean = false): Boolean { val changes = affectedCommits[path.filePath]?.get(commit) ?: return false - if (path.deleted) return changes.containsValue(ChangeKind.REMOVED) + if (path.deleted) { + if (!changes.containsValue(ChangeKind.REMOVED)) return false + if (!verify) return true + for (parent in changes.keys()) { + if (commitToRename.get(UnorderedPair(commit, parent)).firstNotNull { rename -> + rename.getOtherPath(parent, path.filePath) + } != null) { + // this is a rename from path to something else, we should not match this commit + return false + } + } + return true + } return !changes.containsValue(ChangeKind.REMOVED) } 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 b8ff5ac584c3..4b979cae39ec 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 @@ -277,6 +277,72 @@ class FileHistoryTest { 7() } } + + /* + * Two file histories: `create initialFile.txt, rename to file.txt, rename to otherFile.txt` and some time later `create file.txt` + */ + @Test + fun twoFileByTheSameName() { + val file = LocalFilePath("file.txt", false) + val otherFile = LocalFilePath("otherFile.txt", false) + val initialFile = LocalFilePath("initialFile.txt", false) + val fileNamesData = FileNamesDataBuilder(file) + .addChange(file, 0, listOf(MODIFIED), listOf(1)) + .addChange(otherFile, 1, listOf(MODIFIED), listOf(2)) + .addChange(file, 2, listOf(ADDED), listOf(3)) + .addChange(otherFile, 3, listOf(ADDED), listOf(4)) + .addChange(file, 3, listOf(REMOVED), listOf(4)) + .addRename(4, 3, file, otherFile) + .addChange(file, 5, listOf(ADDED), listOf(6)) + .addChange(initialFile, 5, listOf(REMOVED), listOf(6)) + .addRename(6, 5, initialFile, file) + .addChange(initialFile, 6, listOf(ADDED), listOf(6)) + .build() + + graph { + 0(1) + 1(2) + 2(3) + 3(4) + 4(5) + 5(6) + 6() + }.assert(0, file, fileNamesData) { + 0(2.dot) + 2() + } + } + + @Test + fun revertedDeletion() { + val file = LocalFilePath("file.txt", false) + val renamedFile = LocalFilePath("renamedFile.txt", false) + val fileNamesData = FileNamesDataBuilder(file) + .addChange(renamedFile, 0, listOf(ADDED), listOf(1)) + .addChange(file, 0, listOf(REMOVED), listOf(1)) + .addRename(1, 0, file, renamedFile) + .addChange(file, 1, listOf(ADDED), listOf(2)) + .addChange(file, 3, listOf(REMOVED), listOf(4)) + .addChange(file, 4, listOf(MODIFIED), listOf(5)) + .addChange(file, 5, listOf(ADDED), listOf(6)) + .build() + + graph { + 0(1) + 1(2) + 2(3) + 3(4) + 4(5) + 5(6) + 6() + }.assert(0, renamedFile, fileNamesData) { + 0(1) + 1(3.dot) + 3(4) + 4(5) + 5() + } + } } private class FileNamesDataBuilder(private val path: FilePath) {