RefDirectory: Do not unlock until after deleting loose ref

Fix a potential race condition where we would remove our loose ref lock
file before deleting the loose ref itself. This race could result in the
current thread deleting a loose ref newly written by another thread.

Other callers seem to be following the correct pattern, but improve the
method naming to try to help future callers.

Change-Id: I80cebe4788edd855e15381336d980c41498fca80
Signed-off-by: Nasser Grainawi <quic_nasserg@quicinc.com>

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
index 9f31e688c3a21d213031b95fae715010b0997605..695721ec42aa48ae3ec4249f649819c5b74136ce 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
@@ -706,8 +706,7 @@ public class RefDirectory extends RefDatabase {
                int levels = levelsIn(name) - 2;
                delete(logFor(name), levels);
                if (dst.getStorage().isLoose()) {
-                       update.unlock();
-                       delete(fileFor(name), levels);
+                       deleteAndUnlock(fileFor(name), levels, update);
                }
 
                modCnt.incrementAndGet();
@@ -825,7 +824,7 @@ public class RefDirectory extends RefDatabase {
                                                                newLoose = curLoose.remove(idx);
                                                        } while (!looseRefs.compareAndSet(curLoose, newLoose));
                                                        int levels = levelsIn(refName) - 2;
-                                                       delete(refFile, levels, rLck);
+                                                       deleteAndUnlock(refFile, levels, rLck);
                                                }
                                        } finally {
                                                if (shouldUnlock) {
@@ -1302,19 +1301,37 @@ public class RefDirectory extends RefDatabase {
        }
 
        static void delete(File file, int depth) throws IOException {
-               delete(file, depth, null);
+               delete(file);
+               deleteEmptyParentDirs(file, depth);
        }
 
-       private static void delete(File file, int depth, LockFile rLck)
-                       throws IOException {
+       private static void delete(File file) throws IOException {
                if (!file.delete() && file.isFile()) {
-                       throw new IOException(MessageFormat.format(
-                                       JGitText.get().fileCannotBeDeleted, file));
+                       throw new IOException(
+                                       MessageFormat.format(JGitText.get().fileCannotBeDeleted,
+                                                       file));
                }
+       }
 
+       private static void deleteAndUnlock(File file, int depth,
+                       RefDirectoryUpdate refUpdate) throws IOException {
+               delete(file);
+               if (refUpdate != null) {
+                       refUpdate.unlock(); // otherwise cannot delete parent directories emptied by the update
+               }
+               deleteEmptyParentDirs(file, depth);
+       }
+
+       private static void deleteAndUnlock(File file, int depth, LockFile rLck)
+                       throws IOException {
+               delete(file);
                if (rLck != null) {
-                       rLck.unlock(); // otherwise cannot delete dir below
+                       rLck.unlock(); // otherwise cannot delete parent directories of the lock file
                }
+               deleteEmptyParentDirs(file, depth);
+       }
+
+       private static void deleteEmptyParentDirs(File file, int depth) {
                File dir = file.getParentFile();
                for (int i = 0; i < depth; ++i) {
                        try {