[Git] IDEA-59587 again. Moved New files are marked as not changed.
authorKirill Likhodedov <kirill.likhodedov@jetbrains.com>
Fri, 13 May 2011 14:14:55 +0000 (18:14 +0400)
committerKirill Likhodedov <kirill.likhodedov@jetbrains.com>
Fri, 13 May 2011 14:25:58 +0000 (18:25 +0400)
Root cause: comparing VirtualFiles when identifying who is the ancestor, while VirtualFile has changed due to the file move.
1. Don't work with FilePaths - instead compare paths as Strings sorted lexicographically. This might leave extra paths, but it's faster.
2. Add a test for the bug.

[Reviewed by Max]

plugins/git4idea/src/git4idea/changes/ChangeCollector.java
plugins/git4idea/tests/git4idea/tests/GitChangeProviderTest.java

index 57e76e96ea1dd58499a8c133c72f7bf018f75745..f2a8740e78bd726e1a5197e461bf29f414b095f2 100644 (file)
@@ -18,15 +18,14 @@ package git4idea.changes;
 import com.intellij.openapi.project.Project;
 import com.intellij.openapi.util.io.FileUtil;
 import com.intellij.openapi.vcs.FilePath;
+import com.intellij.openapi.vcs.FilePathImpl;
 import com.intellij.openapi.vcs.FileStatus;
 import com.intellij.openapi.vcs.VcsException;
 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.VcsDirtyScope;
-import com.intellij.openapi.vfs.VfsUtil;
 import com.intellij.openapi.vfs.VirtualFile;
-import com.intellij.vcsUtil.VcsUtil;
 import git4idea.GitContentRevision;
 import git4idea.GitRevisionNumber;
 import git4idea.GitUtil;
@@ -34,7 +33,7 @@ import git4idea.commands.GitCommand;
 import git4idea.commands.GitSimpleHandler;
 import git4idea.commands.StringScanner;
 
-import java.io.IOException;
+import java.io.File;
 import java.util.*;
 
 /**
@@ -113,14 +112,15 @@ class ChangeCollector {
    * @return the set of dirty paths to check, the paths are automatically collapsed if the summary length more than limit
    */
   private Collection<FilePath> dirtyPaths(boolean includeChanges) {
-    // TODO collapse paths with common prefix
-    ArrayList<FilePath> paths = new ArrayList<FilePath>();
-    FilePath rootPath = VcsUtil.getFilePath(myVcsRoot.getPath(), true);
+    final List<String> allPaths = new ArrayList<String>();
+
     for (FilePath p : myDirtyScope.getRecursivelyDirtyDirectories()) {
-      addToPaths(rootPath, paths, p);
+      addToPaths(p, allPaths);
+    }
+    for (FilePath p : myDirtyScope.getDirtyFilesNoExpand()) {
+      addToPaths(p, allPaths);
     }
-    ArrayList<FilePath> candidatePaths = new ArrayList<FilePath>();
-    candidatePaths.addAll(myDirtyScope.getDirtyFilesNoExpand());
+
     if (includeChanges) {
       try {
         for (Change c : myChangeListManager.getChangesIn(myVcsRoot)) {
@@ -129,10 +129,10 @@ class ChangeCollector {
             case DELETED:
             case MOVED:
               if (c.getAfterRevision() != null) {
-                addToPaths(rootPath, paths, c.getAfterRevision().getFile());
+                addToPaths(c.getAfterRevision().getFile(), allPaths);
               }
               if (c.getBeforeRevision() != null) {
-                addToPaths(rootPath, paths, c.getBeforeRevision().getFile());
+                addToPaths(c.getBeforeRevision().getFile(), allPaths);
               }
             case MODIFICATION:
             default:
@@ -144,61 +144,36 @@ class ChangeCollector {
         // ignore exceptions
       }
     }
-    for (FilePath p : candidatePaths) {
-      addToPaths(rootPath, paths, p);
+
+    removeCommonParents(allPaths);
+
+    final List<FilePath> paths = new ArrayList<FilePath>(allPaths.size());
+    for (String p : allPaths) {
+      final File file = new File(p);
+      paths.add(new FilePathImpl(file, file.isDirectory()));
     }
     return paths;
   }
 
-  /**
-   * Add path to the collection of the paths to check for this vcs root
-   *
-   * @param root  the root path
-   * @param paths the existing paths
-   * @param toAdd the path to add
-   */
-  void addToPaths(FilePath root, Collection<FilePath> paths, FilePath toAdd) {
-    if (GitUtil.getGitRootOrNull(toAdd) != myVcsRoot) {
-      return;
-    }
-    if (root.isUnder(toAdd, true)) {
-      toAdd = root;
-    }
-    for (Iterator<FilePath> i = paths.iterator(); i.hasNext();) {
-      FilePath p = i.next();
-      if (isAncestor(toAdd, p, true)) { // toAdd is an ancestor of p => adding toAdd instead of p.
-        i.remove();
-      }
-      if (isAncestor(p, toAdd, false)) { // p is an ancestor of toAdd => no need to add toAdd.
-        return;
-      }
+  private void addToPaths(FilePath pathToAdd, List<String> paths) {
+    if (myVcsRoot.equals(GitUtil.getGitRootOrNull(pathToAdd))) {
+      paths.add(pathToAdd.getPath());
     }
-    paths.add(toAdd);
   }
 
-  /**
-   * Returns true if childCandidate file is located under parentCandidate.
-   * This is an alternative to {@link com.intellij.openapi.vcs.FilePathImpl#isUnder(com.intellij.openapi.vcs.FilePath, boolean)}:
-   * it doesn't check VirtualFile associated with this FilePath.
-   * When we move a file we get a VcsDirtyScope with old and new FilePaths, but unfortunately the virtual file in the FilePath is
-   * refreshed ({@link com.intellij.openapi.vcs.changes.VirtualFileHolder#cleanAndAdjustScope(com.intellij.openapi.vcs.changes.VcsModifiableDirtyScope)}
-   * and thus points to the new position which makes FilePathImpl#isUnder useless.
-   *
-   * @param parentCandidate FilePath which we check to be the parent of childCandidate.
-   * @param childCandidate  FilePath which we check to be a child of parentCandidate.
-   * @param strict          if false, the method also returns true if files are equal
-   * @return true if childCandidate is a child of parentCandidate.
-   */
-  private static boolean isAncestor(FilePath parentCandidate, FilePath childCandidate, boolean strict) {
-    try {
-      if (childCandidate.getPath().length() < parentCandidate.getPath().length()) return false;
-      if (childCandidate.getVirtualFile() != null && parentCandidate.getVirtualFile() != null) {
-        return VfsUtil.isAncestor(parentCandidate.getVirtualFile(), childCandidate.getVirtualFile(), strict);
+  private static void removeCommonParents(List<String> allPaths) {
+    Collections.sort(allPaths);
+
+    String prevPath = null;
+    Iterator<String> it = allPaths.iterator();
+    while (it.hasNext()) {
+      String path = it.next();
+      if (prevPath != null && FileUtil.startsWith(path, prevPath)) {      // the file is under previous file, so enough to check the parent
+        it.remove();
+      }
+      else {
+        prevPath = path;
       }
-      return FileUtil.isAncestor(parentCandidate.getIOFile(), childCandidate.getIOFile(), strict);
-    }
-    catch (IOException e) {
-      return false;
     }
   }
 
index e89f1b3ba74649777ba5142f82d8f5cb1e577a09..2aa33f4adc3510023bd3c4eb2c2805d18d1be83c 100644 (file)
@@ -105,6 +105,13 @@ public class GitChangeProviderTest extends GitSingleUserTest {
     assertChanges(new VirtualFile[] { myFiles.get("dir/c.txt"), myFiles.get("dir/subdir/d.txt") }, new FileStatus[] { DELETED, DELETED });
   }
 
+  @Test
+  public void testMoveNewFile() throws Exception {
+    VirtualFile file = myRepo.createFile("new.txt");
+    moveFileInCommand(file, myRepo.getDir().findChild("dir"));
+    assertChanges(file, ADDED);
+  }
+
   @Test
   public void testSimultaneousOperationsOnMultipleFiles() throws Exception {
     VirtualFile dfile = myFiles.get("dir/subdir/d.txt");