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 48ea13b..ea8dfa2 100644 --- 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 @@ 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; @@ -81,6 +84,17 @@ public void looseRefPacked() throws Exception { } @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"); tr.lightweightTag("t", 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 4bb2982..e5ca736 100644 --- 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 void pack(List<String> refs) throws IOException { 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 @@ static int levelsIn(final String name) { } 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(); } }