RefDirectory: remove ref lock file for following ref dir removal

Before this fix, ref directory removal did not work. That was because
the ref lock file was still in the leaf directory at deletion time.
Hence no deep ref directories were ever deleted, which negatively
impacted performance under large directory structure circumstances.

This fix removes the ref lock file before attempting to delete the ref
directory (which includes it). The other deep parent directories are
therefore now successfully deleted in turn, since leaf's content
(lock file) gets removed first.

So, given a structure such as refs/any/directory[/**], this fix now
deletes all empty directories up to -and including- 'directory'. The
'any' directory (e.g.) does not get deleted even if empty, as before.

The ref lock file is still also removed in the calling block's finally
clause, just in case, as before. Such double-unlock brought by this
fix is harmless (a no-op).

A new (private) RefDirectory#delete method is introduced to support
this #pack-specific case; other RefDirectory#delete callers remain
untouched.

Change-Id: I47ba1eeb9bcf0cb93d2ed105d84fea2dac756a5a
Signed-off-by: Marco Miller <marco.miller@ericsson.com>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java
index 48ea13b98a1df71703ec9b16a014b430b285a753..ea8dfa2939d6886d86d19250bc0087c0be8c6583 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java
@@ -45,11 +45,14 @@ package org.eclipse.jgit.internal.storage.file;
 
 import static java.lang.Integer.valueOf;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.concurrent.BrokenBarrierException;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CyclicBarrier;
@@ -80,6 +83,17 @@ public class GcPackRefsTest extends GcTestCase {
                assertSame(repo.exactRef("refs/tags/t").getStorage(), Storage.PACKED);
        }
 
+       @Test
+       public void emptyRefDirectoryDeleted() throws Exception {
+               String ref = "dir/ref";
+               tr.branch(ref).commit().create();
+               String name = repo.findRef(ref).getName();
+               Path dir = repo.getDirectory().toPath().resolve(name).getParent();
+
+               gc.packRefs();
+               assertFalse(Files.exists(dir));
+       }
+
        @Test
        public void concurrentOnlyOneWritesPackedRefs() throws Exception {
                RevBlob a = tr.blob("a");

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 4bb2982b481810b7ee09b768afe3cf8a52069d9f..e5ca7368245f8439a2f39db53b854d6d76461945 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
@@ -688,7 +688,7 @@ public class RefDirectory extends RefDatabase {
                                                        newLoose = curLoose.remove(idx);
                                                } while (!looseRefs.compareAndSet(curLoose, newLoose));
                                                int levels = levelsIn(refName) - 2;
-                                               delete(fileFor(refName), levels);
+                                               delete(refFile, levels, rLck);
                                        }
                                } finally {
                                        rLck.unlock();
@@ -1062,13 +1062,24 @@ public class RefDirectory extends RefDatabase {
        }
 
        static void delete(final File file, final int depth) throws IOException {
-               if (!file.delete() && file.isFile())
-                       throw new IOException(MessageFormat.format(JGitText.get().fileCannotBeDeleted, file));
+               delete(file, depth, null);
+       }
 
+       private static void delete(final File file, final int depth, LockFile rLck)
+                       throws IOException {
+               if (!file.delete() && file.isFile()) {
+                       throw new IOException(MessageFormat.format(
+                                       JGitText.get().fileCannotBeDeleted, file));
+               }
+
+               if (rLck != null) {
+                       rLck.unlock(); // otherwise cannot delete dir below
+               }
                File dir = file.getParentFile();
                for (int i = 0; i < depth; ++i) {
-                       if (!dir.delete())
+                       if (!dir.delete()) {
                                break; // ignore problem here
+                       }
                        dir = dir.getParentFile();
                }
        }