From 4738fe33f9869fa6246a1f799c3abd8656a6d200 Mon Sep 17 00:00:00 2001
From: Kirill Likhodedov <Kirill.Likhodedov@jetbrains.com>
Date: Wed, 2 May 2012 17:04:40 +0400
Subject: [PATCH] IDEA-85521 Sometimes file appear both as ADDED and
 UNVERSIONED

Root cause:
Race condition around verifyPossiblyUntrackedFiles():
  1) AWT: File created. Added to myPossiblyUntrackedFiles.
  2) T2: starts background thread T2 to check file for ignorance and add it to Git.
  3) T1: File is marked dirty, its status is queried by GitChangeProvider in T1.
  4) T1: verifyPossiblyUntrackedFiles asks the state of the file, it is untracked, and it is put into untrackedFiles temp variable.
  5) T2: File is added to Git.
  6) T2: File is removed from myDUF, it is not there, but nobody cares.
  7) T1: File is added to myDUF from verifyPossiblyUntrackedFiles.

Solution:
  Check untracked files under the lock.
  The LOCK is used in after() => not to lock the AWT, use a separate lock for myDefinitelyUntrackedFiles, leaving the LOCK for myReady and myPossiblyUntrackedFiles.
  Don't check for myReady in add()/remove() - it is not necessary.
---
 .../repo/GitUntrackedFilesHolder.java         | 49 +++++++++++--------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/plugins/git4idea/src/git4idea/repo/GitUntrackedFilesHolder.java b/plugins/git4idea/src/git4idea/repo/GitUntrackedFilesHolder.java
index cd91d5aa2c02..c748728f852e 100644
--- a/plugins/git4idea/src/git4idea/repo/GitUntrackedFilesHolder.java
+++ b/plugins/git4idea/src/git4idea/repo/GitUntrackedFilesHolder.java
@@ -68,10 +68,18 @@ import java.util.Set;
  *   In some cases (file creation/deletion) the file is not silently added/removed from the list - instead the file is marked as
  *   "possibly untracked" and Git is asked for the exact status of this file.
  *   It is needed, since the file may be created and added to the index independently, and events may race.
- *   <br/>
+ * </p>
+ * <p>
  *   Also, if .git/index changes, then a full refresh is initiated. The reason is not only untracked files tracking, but also handling
  *   committing outside IDEA, etc.
  * </p>
+ * <p>
+ *   Synchronization policy used in this class:<br/>
+ *   myDefinitelyUntrackedFiles is accessed under the myDefinitelyUntrackedFiles lock.<br/>
+ *   myPossiblyUntrackedFiles and myReady is accessed under the LOCK lock.<br/>
+ *   This is done so, because the latter two variables are accessed from the AWT in after() and we don't want to lock the AWT long,
+ *   while myDefinitelyUntrackedFiles is modified along with native request to Git.
+ * </p>
  *
  * @author Kirill Likhodedov
  */
@@ -84,8 +92,8 @@ public class GitUntrackedFilesHolder implements Disposable, BulkFileListener {
   private final GitRepositoryFiles myRepositoryFiles;
   private final Git myGit;
 
-  private Set<VirtualFile> myDefinitelyUntrackedFiles = new HashSet<VirtualFile>();
-  private Set<VirtualFile> myPossiblyUntrackedFiles = new HashSet<VirtualFile>();
+  private final Set<VirtualFile> myDefinitelyUntrackedFiles = new HashSet<VirtualFile>();
+  private final Set<VirtualFile> myPossiblyUntrackedFiles = new HashSet<VirtualFile>();
   private boolean myReady;   // if false, total refresh is needed
   private final Object LOCK = new Object();
   private final GitRepositoryManager myRepositoryManager;
@@ -111,8 +119,10 @@ public class GitUntrackedFilesHolder implements Disposable, BulkFileListener {
 
   @Override
   public void dispose() {
-    synchronized (LOCK) {
+    synchronized (myDefinitelyUntrackedFiles) {
       myDefinitelyUntrackedFiles.clear();
+    }
+    synchronized (LOCK) {
       myPossiblyUntrackedFiles.clear();
     }
   }
@@ -121,10 +131,8 @@ public class GitUntrackedFilesHolder implements Disposable, BulkFileListener {
    * Adds the file to the list of untracked.
    */
   public void add(@NotNull VirtualFile file) {
-    synchronized (LOCK) {
-      if (myReady) {
-        myDefinitelyUntrackedFiles.add(file);
-      }
+    synchronized (myDefinitelyUntrackedFiles) {
+      myDefinitelyUntrackedFiles.add(file);
     }
   }
 
@@ -132,10 +140,8 @@ public class GitUntrackedFilesHolder implements Disposable, BulkFileListener {
    * Removes several files from untracked.
    */
   public void remove(@NotNull Collection<VirtualFile> files) {
-    synchronized (LOCK) {
-      if (myReady) {
-        myDefinitelyUntrackedFiles.removeAll(files);
-      }
+    synchronized (myDefinitelyUntrackedFiles) {
+      myDefinitelyUntrackedFiles.removeAll(files);
     }
   }
 
@@ -152,7 +158,7 @@ public class GitUntrackedFilesHolder implements Disposable, BulkFileListener {
     } else {
       rescanAll();
     }
-    synchronized (LOCK) {
+    synchronized (myDefinitelyUntrackedFiles) {
       return myDefinitelyUntrackedFiles;
     }
   }
@@ -168,8 +174,11 @@ public class GitUntrackedFilesHolder implements Disposable, BulkFileListener {
    */
   public void rescanAll() throws VcsException {
     Set<VirtualFile> untrackedFiles = myGit.untrackedFiles(myProject, myRoot, null);
+    synchronized (myDefinitelyUntrackedFiles) {
+      myDefinitelyUntrackedFiles.clear();
+      myDefinitelyUntrackedFiles.addAll(untrackedFiles);
+    }
     synchronized (LOCK) {
-      myDefinitelyUntrackedFiles = untrackedFiles;
       myPossiblyUntrackedFiles.clear();
       myReady = true;
     }
@@ -191,15 +200,15 @@ public class GitUntrackedFilesHolder implements Disposable, BulkFileListener {
     Set<VirtualFile> suspiciousFiles = new HashSet<VirtualFile>();
     synchronized (LOCK) {
       suspiciousFiles.addAll(myPossiblyUntrackedFiles);
+      myPossiblyUntrackedFiles.clear();
     }
 
-    Set<VirtualFile> untrackedFiles = myGit.untrackedFiles(myProject, myRoot, suspiciousFiles);
-    suspiciousFiles.removeAll(untrackedFiles);
-    // files that were suspicious (and thus passed to 'git ls-files'), but are not untracked, are definitely tracked.
-    Set<VirtualFile> trackedFiles  = suspiciousFiles;
+    synchronized (myDefinitelyUntrackedFiles) {
+      Set<VirtualFile> untrackedFiles = myGit.untrackedFiles(myProject, myRoot, suspiciousFiles);
+      suspiciousFiles.removeAll(untrackedFiles);
+      // files that were suspicious (and thus passed to 'git ls-files'), but are not untracked, are definitely tracked.
+      Set<VirtualFile> trackedFiles  = suspiciousFiles;
 
-    synchronized (LOCK) {
-      myPossiblyUntrackedFiles.clear();
       myDefinitelyUntrackedFiles.addAll(untrackedFiles);
       myDefinitelyUntrackedFiles.removeAll(trackedFiles);
     }