From fbb16e6c800073ca7248c91ea1bb6e7a6c9a2b7a Mon Sep 17 00:00:00 2001 From: Aleksey Pivovarov Date: Mon, 22 Jul 2019 15:09:00 +0300 Subject: [PATCH] git: support files that are renamed in working tree EA-209061 - GFE: GitChangesCollector.throwGFE GitOrigin-RevId: 037360774183da0426e4507511ff998bc776b938 --- .../git4idea/status/GitChangesCollector.java | 31 ++++-- .../tests/git4idea/test/GitPlatformTest.kt | 8 ++ .../tests/git4idea/test/GitTestUtil.kt | 2 +- .../tests/GitChangeProviderConflictTest.kt | 14 +-- .../git4idea/tests/GitChangeProviderTest.kt | 51 +++++----- .../tests/GitChangeProviderVersionedTest.kt | 95 +++++++++++++++++-- .../tests/git4idea/tests/GitCommitTest.kt | 9 -- .../git4idea/tests/GitPartialCommitTest.kt | 14 +-- 8 files changed, 152 insertions(+), 72 deletions(-) diff --git a/plugins/git4idea/src/git4idea/status/GitChangesCollector.java b/plugins/git4idea/src/git4idea/status/GitChangesCollector.java index 36846154a719..aa0cbb7b6f9e 100644 --- a/plugins/git4idea/src/git4idea/status/GitChangesCollector.java +++ b/plugins/git4idea/src/git4idea/status/GitChangesCollector.java @@ -231,6 +231,18 @@ class GitChangesCollector { final FilePath filepath = GitContentRevision.createPath(myVcsRoot, path); + FilePath oldFilepath; + if (xStatus == 'R' || xStatus == 'C' || + yStatus == 'R' || yStatus == 'C') { + // We treat "Copy" as "Added", but we still have to read the old path not to break the format parsing. + //noinspection AssignmentToForLoopParameter + pos += 1; // read the "from" filepath which is separated also by NUL character. + oldFilepath = GitContentRevision.createPath(myVcsRoot, split[pos]); + } + else { + oldFilepath = null; + } + switch (xStatus) { case ' ': if (yStatus == 'M') { @@ -239,7 +251,7 @@ class GitChangesCollector { else if (yStatus == 'D') { reportDeleted(filepath, head); } - else if (yStatus == 'A') { + else if (yStatus == 'A' || yStatus == 'C') { reportAdded(filepath); } else if (yStatus == 'T') { @@ -248,6 +260,9 @@ class GitChangesCollector { else if (yStatus == 'U') { reportConflict(filepath, head, Status.MODIFIED, Status.MODIFIED); } + else if (yStatus == 'R') { + reportRename(filepath, oldFilepath, head); + } else { throwYStatus(output, handler, line, xStatus, yStatus); } @@ -266,10 +281,6 @@ class GitChangesCollector { break; case 'C': - //noinspection AssignmentToForLoopParameter - pos += 1; // read the "from" filepath which is separated also by NUL character. - // NB: no "break" here! - // we treat "Copy" as "Added", but we still have to read the old path not to break the format parsing. case 'A': if (yStatus == 'M' || yStatus == ' ' || yStatus == 'T') { reportAdded(filepath); @@ -298,6 +309,12 @@ class GitChangesCollector { else if (yStatus == 'D') { // DD - unmerged, both deleted reportConflict(filepath, head, Status.DELETED, Status.DELETED); } + else if (yStatus == 'C') { + reportModified(filepath, head); + } + else if (yStatus == 'R') { + reportRename(filepath, oldFilepath, head); + } else { throwYStatus(output, handler, line, xStatus, yStatus); } @@ -319,10 +336,6 @@ class GitChangesCollector { break; case 'R': - //noinspection AssignmentToForLoopParameter - pos += 1; // read the "from" filepath which is separated also by NUL character. - FilePath oldFilepath = GitContentRevision.createPath(myVcsRoot, split[pos]); - if (yStatus == 'D') { reportDeleted(oldFilepath, head); } diff --git a/plugins/git4idea/tests/git4idea/test/GitPlatformTest.kt b/plugins/git4idea/tests/git4idea/test/GitPlatformTest.kt index 3e5c43881623..6b4d8909b688 100644 --- a/plugins/git4idea/tests/git4idea/test/GitPlatformTest.kt +++ b/plugins/git4idea/tests/git4idea/test/GitPlatformTest.kt @@ -228,6 +228,14 @@ abstract class GitPlatformTest : VcsPlatformTest() { assertTrue("Commit dialog was not shown", vcsHelper.commitDialogWasShown()) } + protected fun assertNoChanges() { + changeListManager.assertNoChanges() + } + + protected fun assertChanges(changes: ChangesBuilder.() -> Unit): List { + return changeListManager.assertChanges(changes) + } + protected data class ReposTrinity(val projectRepo: GitRepository, val parent: File, val bro: File) diff --git a/plugins/git4idea/tests/git4idea/test/GitTestUtil.kt b/plugins/git4idea/tests/git4idea/test/GitTestUtil.kt index 8fd7a4859e7d..cc8b0261fecb 100644 --- a/plugins/git4idea/tests/git4idea/test/GitTestUtil.kt +++ b/plugins/git4idea/tests/git4idea/test/GitTestUtil.kt @@ -55,7 +55,7 @@ fun createFileStructure(rootDir: VirtualFile, vararg paths: String) { mkdir(path) } else { - touch(path, "initial_content_" + Math.random()) + touch(path, "initial_content_in_{$path}") } } } diff --git a/plugins/git4idea/tests/git4idea/tests/GitChangeProviderConflictTest.kt b/plugins/git4idea/tests/git4idea/tests/GitChangeProviderConflictTest.kt index 2a6006fcf59d..f1458b28aaf0 100644 --- a/plugins/git4idea/tests/git4idea/tests/GitChangeProviderConflictTest.kt +++ b/plugins/git4idea/tests/git4idea/tests/GitChangeProviderConflictTest.kt @@ -27,7 +27,7 @@ class GitChangeProviderConflictTest : GitChangeProviderTest() { */ fun testConflictMM() { modifyFileInBranches("a.txt", FileAction.MODIFY, FileAction.MODIFY) - assertChanges(atxt, FileStatus.MERGED_WITH_CONFLICTS) + assertProviderChanges(atxt, FileStatus.MERGED_WITH_CONFLICTS) assertManagerConflicts(Conflict("a.txt", Status.MODIFIED, Status.MODIFIED)) } @@ -36,7 +36,7 @@ class GitChangeProviderConflictTest : GitChangeProviderTest() { */ fun testConflictMD() { modifyFileInBranches("a.txt", FileAction.MODIFY, FileAction.DELETE) - assertChanges(atxt, FileStatus.MERGED_WITH_CONFLICTS) + assertProviderChanges(atxt, FileStatus.MERGED_WITH_CONFLICTS) assertManagerConflicts(Conflict("a.txt", Status.MODIFIED, Status.DELETED)) } @@ -45,7 +45,7 @@ class GitChangeProviderConflictTest : GitChangeProviderTest() { */ fun testConflictDM() { modifyFileInBranches("a.txt", FileAction.DELETE, FileAction.MODIFY) - assertChanges(atxt, FileStatus.MERGED_WITH_CONFLICTS) + assertProviderChanges(atxt, FileStatus.MERGED_WITH_CONFLICTS) assertManagerConflicts(Conflict("a.txt", Status.DELETED, Status.MODIFIED)) } @@ -55,21 +55,21 @@ class GitChangeProviderConflictTest : GitChangeProviderTest() { fun testConflictCC() { modifyFileInBranches("z.txt", FileAction.CREATE, FileAction.CREATE) val zfile = projectRoot.findChild("z.txt") - assertChanges(zfile!!, FileStatus.MERGED_WITH_CONFLICTS) + assertProviderChanges(zfile!!, FileStatus.MERGED_WITH_CONFLICTS) assertManagerConflicts(Conflict("z.txt", Status.ADDED, Status.ADDED)) } fun testConflictRD() { modifyFileInBranches("a.txt", FileAction.RENAME, FileAction.DELETE) val newfile = projectRoot.findChild("a.txt_master_new") // renamed in master - assertChanges(newfile!!, FileStatus.MERGED_WITH_CONFLICTS) + assertProviderChanges(newfile!!, FileStatus.MERGED_WITH_CONFLICTS) assertManagerConflicts(Conflict("a.txt_master_new", Status.ADDED, Status.MODIFIED)) } fun testConflictDR() { modifyFileInBranches("a.txt", FileAction.DELETE, FileAction.RENAME) val newFile = projectRoot.findChild("a.txt_feature_new") // deleted in master, renamed in feature - assertChanges(newFile!!, FileStatus.MERGED_WITH_CONFLICTS) + assertProviderChanges(newFile!!, FileStatus.MERGED_WITH_CONFLICTS) assertManagerConflicts(Conflict("a.txt_feature_new", Status.MODIFIED, Status.ADDED)) } @@ -77,7 +77,7 @@ class GitChangeProviderConflictTest : GitChangeProviderTest() { modifyFileInBranches("a.txt", FileAction.RENAME, FileAction.RENAME) val newMasterFile = projectRoot.findChild("a.txt_master_new")!! val newFeatureFile = projectRoot.findChild("a.txt_feature_new")!! - assertChanges(listOf(newMasterFile, newFeatureFile), listOf(FileStatus.MERGED_WITH_CONFLICTS, FileStatus.MERGED_WITH_CONFLICTS)) + assertProviderChanges(listOf(newMasterFile, newFeatureFile), listOf(FileStatus.MERGED_WITH_CONFLICTS, FileStatus.MERGED_WITH_CONFLICTS)) assertManagerConflicts(Conflict("a.txt_master_new", Status.ADDED, Status.MODIFIED), Conflict("a.txt_feature_new", Status.MODIFIED, Status.ADDED), Conflict("a.txt", Status.DELETED, Status.DELETED, false)) diff --git a/plugins/git4idea/tests/git4idea/tests/GitChangeProviderTest.kt b/plugins/git4idea/tests/git4idea/tests/GitChangeProviderTest.kt index da31a5c73b37..7164185ca57a 100644 --- a/plugins/git4idea/tests/git4idea/tests/GitChangeProviderTest.kt +++ b/plugins/git4idea/tests/git4idea/tests/GitChangeProviderTest.kt @@ -7,20 +7,19 @@ import com.intellij.openapi.vcs.Executor.cd import com.intellij.openapi.vcs.FilePath import com.intellij.openapi.vcs.FileStatus import com.intellij.openapi.vcs.VcsTestUtil.* -import com.intellij.openapi.vcs.changes.Change -import com.intellij.openapi.vcs.changes.ChangeListManager -import com.intellij.openapi.vcs.changes.ContentRevision -import com.intellij.openapi.vcs.changes.VcsModifiableDirtyScope +import com.intellij.openapi.vcs.changes.* import com.intellij.openapi.vfs.VfsUtil import com.intellij.openapi.vfs.VirtualFile import com.intellij.testFramework.vcs.MockChangeListManagerGate import com.intellij.testFramework.vcs.MockChangelistBuilder import com.intellij.testFramework.vcs.MockDirtyScope import com.intellij.vcsUtil.VcsUtil +import git4idea.config.GitVersion import git4idea.status.GitChangeProvider import git4idea.test.GitSingleRepoTest import git4idea.test.addCommit import git4idea.test.createFileStructure +import org.junit.Assume import java.io.File import java.util.* @@ -61,16 +60,21 @@ abstract class GitChangeProviderTest : GitSingleRepoTest() { override fun makeInitialCommit() = false - private fun getVirtualFile(relativePath: String) = VfsUtil.findFileByIoFile(File(projectPath, relativePath), true)!! + protected fun getVirtualFile(relativePath: String) = VfsUtil.findFileByIoFile(File(projectPath, relativePath), true)!! /** * Checks that the given files have respective statuses in the change list retrieved from myChangesProvider. * Pass null in the fileStatuses array to indicate that proper file has not changed. */ - protected fun assertChanges(virtualFiles: List, fileStatuses: List) { - val result = getChanges(virtualFiles) - for (i in virtualFiles.indices) { - val fp = VcsUtil.getFilePath(virtualFiles[i]) + + protected fun assertProviderChanges(virtualFiles: List, fileStatuses: List) { + assertProviderChangesInPaths(virtualFiles.map { VcsUtil.getFilePath(it) }, fileStatuses) + } + + protected fun assertProviderChangesInPaths(paths: List, fileStatuses: List) { + val result = getProviderChanges(paths) + for (i in paths.indices) { + val fp = paths[i] val status = fileStatuses[i] if (status == null) { assertFalse("File [" + tos(fp) + " shouldn't be in the changelist, but it was.", result.containsKey(fp)) @@ -81,17 +85,20 @@ abstract class GitChangeProviderTest : GitSingleRepoTest() { } } - protected fun assertChanges(virtualFile: VirtualFile, fileStatus: FileStatus) { - assertChanges(listOf(virtualFile), listOf(fileStatus)) + protected fun assertProviderChanges(virtualFile: VirtualFile, fileStatus: FileStatus) { + assertProviderChanges(listOf(virtualFile), listOf(fileStatus)) + } + + protected fun assumeWorktreeRenamesSupported() { + Assume.assumeTrue("Worktree renames are not supported by git: ${vcs.version}", + vcs.version.isLaterOrEqual(GitVersion(2, 17, 0, 0))) } /** * Marks the given files dirty in myDirtyScope, gets changes from myChangeProvider and groups the changes in the map. * Assumes that only one change for a file has happened. */ - private fun getChanges(changedFiles: List): Map { - val changedPaths = changedFiles.map { VcsUtil.getFilePath(it) } - + private fun getProviderChanges(changedPaths: List): Map { // get changes val builder = MockChangelistBuilder() changeProvider.getChanges(dirtyScope, builder, EmptyProgressIndicator(), @@ -101,21 +108,7 @@ abstract class GitChangeProviderTest : GitSingleRepoTest() { // get changes for files val result = HashMap() for (change in changes) { - val file = change.virtualFile - var filePath: FilePath? = null - if (file == null) { // if a file was deleted, just find the reference in the original list of files and use it. - val path = change.beforeRevision!!.file.path - for (fp in changedPaths) { - if (FileUtil.pathsEqual(fp.path, path)) { - filePath = fp - break - } - } - } - else { - filePath = VcsUtil.getFilePath(file) - } - result.put(filePath!!, change) + result.put(ChangesUtil.getFilePath(change), change) } return result } diff --git a/plugins/git4idea/tests/git4idea/tests/GitChangeProviderVersionedTest.kt b/plugins/git4idea/tests/git4idea/tests/GitChangeProviderVersionedTest.kt index e3a3d8e992dc..a1d9975612bf 100644 --- a/plugins/git4idea/tests/git4idea/tests/GitChangeProviderVersionedTest.kt +++ b/plugins/git4idea/tests/git4idea/tests/GitChangeProviderVersionedTest.kt @@ -3,20 +3,29 @@ package git4idea.tests import com.intellij.openapi.application.ApplicationManager import com.intellij.openapi.util.io.FileUtil +import com.intellij.openapi.vcs.Executor.rm +import com.intellij.openapi.vcs.Executor.touch import com.intellij.openapi.vcs.FileStatus.* +import com.intellij.openapi.vfs.VfsUtil import com.intellij.openapi.vfs.VfsUtilCore import com.intellij.testFramework.VfsTestUtil.createDir import com.intellij.testFramework.runInEdtAndGet import com.intellij.ui.GuiUtils import com.intellij.vcsUtil.VcsUtil import git4idea.test.add +import git4idea.test.addCommit +import git4idea.test.git class GitChangeProviderVersionedTest : GitChangeProviderTest() { fun testCreateFile() { val file = create(projectRoot, "new.txt") repo.add(file.path) - assertChanges(file, ADDED) + assertProviderChanges(file, ADDED) + + assertChanges { + added("new.txt") + } } fun testCreateFileInDir() { @@ -24,17 +33,30 @@ class GitChangeProviderVersionedTest : GitChangeProviderTest() { dirty(dir) val bfile = create(dir, "new.txt") repo.add(bfile.path) - assertChanges(listOf(bfile, dir), listOf(ADDED, null)) + assertProviderChanges(listOf(bfile, dir), + listOf(ADDED, null)) + + assertChanges { + added("newdir/new.txt") + } } fun testEditFile() { edit(atxt, "new content") - assertChanges(atxt, MODIFIED) + assertProviderChanges(atxt, MODIFIED) + + assertChanges { + modified("a.txt") + } } fun testDeleteFile() { deleteFile(atxt) - assertChanges(atxt, DELETED) + assertProviderChanges(atxt, DELETED) + + assertChanges { + deleted("a.txt") + } } fun testDeleteDirRecursively() { @@ -45,8 +67,13 @@ class GitChangeProviderVersionedTest : GitChangeProviderTest() { FileUtil.delete(VfsUtilCore.virtualToIoFile(dir)) } } - assertChanges(listOf(dir_ctxt, subdir_dtxt), - listOf(DELETED, DELETED)) + assertProviderChanges(listOf(dir_ctxt, subdir_dtxt), + listOf(DELETED, DELETED)) + + assertChanges { + deleted("dir/c.txt") + deleted("dir/subdir/d.txt") + } } fun testSimultaneousOperationsOnMultipleFiles() { @@ -56,7 +83,61 @@ class GitChangeProviderVersionedTest : GitChangeProviderTest() { val newfile = create(projectRoot, "newfile.txt") repo.add() - assertChanges(listOf(atxt, dir_ctxt, subdir_dtxt, newfile), listOf(MODIFIED, MODIFIED, DELETED, ADDED)) + assertProviderChanges(listOf(atxt, dir_ctxt, subdir_dtxt, newfile), + listOf(MODIFIED, MODIFIED, DELETED, ADDED)) + + assertChanges { + modified("a.txt") + modified("dir/c.txt") + deleted("dir/subdir/d.txt") + added("newfile.txt") + } } + fun testRenamedInWorktree() { + assumeWorktreeRenamesSupported() + + touch("rename.txt", "rename_file_content") + addCommit("init rename") + + // do not trigger move via VcsVFSListener + rm("rename.txt") + touch("unstaged.txt", "rename_file_content") + repo.git("add -N unstaged.txt") + + VfsUtil.markDirtyAndRefresh(false, true, true, projectRoot) + dirty(projectRoot) + + assertProviderChangesInPaths(listOf("rename.txt", "staged.txt", "unstaged.txt").map { VcsUtil.getFilePath(projectRoot, it) }, + listOf(null, null, MODIFIED)) + + assertChanges { + rename("rename.txt", "unstaged.txt") + } + } + + fun testTwiceRenamed() { + assumeWorktreeRenamesSupported() + + touch("rename.txt", "rename_file_content") + addCommit("init rename") + + repo.git("mv rename.txt staged.txt") + + // do not trigger move via VcsVFSListener + rm("staged.txt") + touch("unstaged.txt", "rename_file_content") + repo.git("add -N unstaged.txt") + + VfsUtil.markDirtyAndRefresh(false, true, true, projectRoot) + dirty(projectRoot) + + assertProviderChangesInPaths(listOf("rename.txt", "staged.txt", "unstaged.txt").map { VcsUtil.getFilePath(projectRoot, it) }, + listOf(null, MODIFIED, MODIFIED)) + + assertChanges { + rename("rename.txt", "staged.txt") + rename("staged.txt", "unstaged.txt") + } + } } diff --git a/plugins/git4idea/tests/git4idea/tests/GitCommitTest.kt b/plugins/git4idea/tests/git4idea/tests/GitCommitTest.kt index 38b9fdfcc7cf..e554d571d82b 100644 --- a/plugins/git4idea/tests/git4idea/tests/GitCommitTest.kt +++ b/plugins/git4idea/tests/git4idea/tests/GitCommitTest.kt @@ -8,7 +8,6 @@ import com.intellij.openapi.util.io.FileUtil import com.intellij.openapi.util.registry.Registry import com.intellij.openapi.vcs.Executor.* import com.intellij.openapi.vcs.FilePath -import com.intellij.openapi.vcs.changes.Change import git4idea.checkin.GitCheckinEnvironment import git4idea.checkin.GitCheckinExplicitMovementProvider import git4idea.checkin.isCommitRenamesSeparately @@ -917,14 +916,6 @@ abstract class GitCommitTest(private val useStagingArea: Boolean) : GitSingleRep git("mv -f $from $to") } - private fun assertNoChanges() { - changeListManager.assertNoChanges() - } - - private fun assertChanges(changes: ChangesBuilder.() -> Unit): List { - return changeListManager.assertChanges(changes) - } - private class MyExplicitMovementProvider : GitCheckinExplicitMovementProvider() { override fun isEnabled(project: Project): Boolean = true diff --git a/plugins/git4idea/tests/git4idea/tests/GitPartialCommitTest.kt b/plugins/git4idea/tests/git4idea/tests/GitPartialCommitTest.kt index 380243728c91..68c5184fb1fb 100644 --- a/plugins/git4idea/tests/git4idea/tests/GitPartialCommitTest.kt +++ b/plugins/git4idea/tests/git4idea/tests/GitPartialCommitTest.kt @@ -7,14 +7,16 @@ import com.intellij.openapi.application.runWriteAction import com.intellij.openapi.editor.Document import com.intellij.openapi.fileEditor.FileDocumentManager import com.intellij.openapi.vcs.Executor.child -import com.intellij.openapi.vcs.changes.Change import com.intellij.openapi.vcs.changes.ChangesUtil import com.intellij.openapi.vcs.changes.LocalChangeList import com.intellij.openapi.vcs.ex.PartialLocalLineStatusTracker import com.intellij.openapi.vcs.impl.LineStatusTrackerManager import com.intellij.openapi.vfs.LocalFileSystem import com.intellij.util.ui.UIUtil -import git4idea.test.* +import git4idea.test.GitSingleRepoTest +import git4idea.test.assertCommitted +import git4idea.test.gitAsBytes +import git4idea.test.tac class GitPartialCommitTest : GitSingleRepoTest() { fun `test partial commit with changelists`() { @@ -190,14 +192,6 @@ class GitPartialCommitTest : GitSingleRepoTest() { } - private fun assertNoChanges() { - changeListManager.assertNoChanges() - } - - private fun assertChanges(changes: ChangesBuilder.() -> Unit): List { - return changeListManager.assertChanges(changes) - } - private fun assertCommittedContent(fileName: String, expectedContent: String, useFilters: Boolean = false) { val actualContent = repo.gitAsBytes("cat-file" + (if (useFilters) " --filters" else " -p") +