Merge branch 'stable-6.8' into master

* stable-6.8:
  RefDirectory: Do not unlock until after deleting loose ref
  Add missing javadoc description for declared exception
  SnapshottingRefDirectory: Invalidate snapshot after locking ref for update
  SnapshottingRefDir: Replace lambas with method refs
  SnapshottingRefDir: Reduce casts with overrides
  [errorprone] Fix wrong comparison which always evaluated to false
  [errorprone] Remove unnecessary comparison

Change-Id: I1d65c41292779dd5f8f46bc0adefbfc9a62ba2ce

3 files changed, 80 insertions, 26 deletions

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 a41c7a70e6..e27029798d 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
@@ -703,8 +703,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();
@@ -823,7 +822,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) {
@@ -1304,19 +1303,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 {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java
index 1dc4e3011f..436957bb24 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java
@@ -51,6 +51,7 @@ class RefDirectoryUpdate extends RefUpdate {
 		String name = dst.getName();
 		lock = new LockFile(database.fileFor(name));
 		if (lock.lock()) {
+			doAfterLocking(name);
 			dst = database.findRef(name);
 			setOldObjectId(dst != null ? dst.getObjectId() : null);
 			return true;
@@ -134,4 +135,14 @@ class RefDirectoryUpdate extends RefUpdate {
 			return Result.NEW;
 		return Result.FORCED;
 	}
+
+	/**
+	 * Do any actions needed immediately after a lock on the ref is acquired
+	 *
+	 * @param name
+	 *            the name of the reference.
+	 */
+	protected void doAfterLocking(String name) {
+		// No actions by default
+	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java
index 67e8d795cf..46607f60d9 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/SnapshottingRefDirectory.java
@@ -13,7 +13,6 @@ package org.eclipse.jgit.internal.storage.file;
 
 import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.revwalk.RevWalk;
 
@@ -144,29 +143,29 @@ class SnapshottingRefDirectory extends RefDirectory {
 	}
 
 	private static <T> T invalidateSnapshotOnError(
-			SupplierThrowsException<T, IOException> f, RefDatabase refDb)
+			SupplierThrowsException<T, IOException> f, SnapshottingRefDirectory refDb)
 			throws IOException {
 		return invalidateSnapshotOnError(a -> f.call(), null, refDb);
 	}
 
 	private static <A, R> R invalidateSnapshotOnError(
 			FunctionThrowsException<A, R, IOException> f, A a,
-			RefDatabase refDb) throws IOException {
+			SnapshottingRefDirectory refDb) throws IOException {
 		try {
 			return f.apply(a);
 		} catch (IOException e) {
-			((SnapshottingRefDirectory) refDb).invalidateSnapshot();
+			refDb.invalidateSnapshot();
 			throw e;
 		}
 	}
 
 	private static <A1, A2, A3> void invalidateSnapshotOnError(
 			TriConsumerThrowsException<A1, A2, A3, IOException> f, A1 a1, A2 a2,
-			A3 a3, RefDatabase refDb) throws IOException {
+			A3 a3, SnapshottingRefDirectory refDb) throws IOException {
 		try {
 			f.accept(a1, a2, a3);
 		} catch (IOException e) {
-			((SnapshottingRefDirectory) refDb).invalidateSnapshot();
+			refDb.invalidateSnapshot();
 			throw e;
 		}
 	}
@@ -178,39 +177,57 @@ class SnapshottingRefDirectory extends RefDirectory {
 
 		@Override
 		public Result forceUpdate() throws IOException {
-			return invalidateSnapshotOnError(() -> super.forceUpdate(),
+			return invalidateSnapshotOnError(super::forceUpdate,
 					getRefDatabase());
 		}
 
 		@Override
 		public Result update() throws IOException {
-			return invalidateSnapshotOnError(() -> super.update(),
-					getRefDatabase());
+			return invalidateSnapshotOnError(super::update, getRefDatabase());
 		}
 
 		@Override
 		public Result update(RevWalk walk) throws IOException {
-			return invalidateSnapshotOnError(rw -> super.update(rw), walk,
+			return invalidateSnapshotOnError(super::update, walk,
 					getRefDatabase());
 		}
 
 		@Override
 		public Result delete() throws IOException {
-			return invalidateSnapshotOnError(() -> super.delete(),
-					getRefDatabase());
+			return invalidateSnapshotOnError(super::delete, getRefDatabase());
 		}
 
 		@Override
 		public Result delete(RevWalk walk) throws IOException {
-			return invalidateSnapshotOnError(rw -> super.delete(rw), walk,
+			return invalidateSnapshotOnError(super::delete, walk,
 					getRefDatabase());
 		}
 
 		@Override
 		public Result link(String target) throws IOException {
-			return invalidateSnapshotOnError(t -> super.link(t), target,
+			return invalidateSnapshotOnError(super::link, target,
 					getRefDatabase());
 		}
+
+		/**
+		 * Invalidate the SnapshottingRefDirectory snapshot after locking the
+		 * ref.
+		 * <p>
+		 * Doing this after locking the ref ensures that the upcoming write is
+		 * not based on a cached value.
+		 *
+		 * @param name
+		 *            the name of the reference.
+		 */
+		@Override
+		protected void doAfterLocking(String name) {
+			getRefDatabase().invalidateSnapshot();
+		}
+
+		@Override
+		public SnapshottingRefDirectory getRefDatabase() {
+			return (SnapshottingRefDirectory) super.getRefDatabase();
+		}
 	}
 
 	private static class SnapshotRefDirectoryRename extends RefDirectoryRename {
@@ -221,8 +238,12 @@ class SnapshottingRefDirectory extends RefDirectory {
 
 		@Override
 		public RefUpdate.Result rename() throws IOException {
-			return invalidateSnapshotOnError(() -> super.rename(),
-					getRefDirectory());
+			return invalidateSnapshotOnError(super::rename, getRefDirectory());
+		}
+
+		@Override
+		public SnapshottingRefDirectory getRefDirectory() {
+			return (SnapshottingRefDirectory) super.getRefDirectory();
 		}
 	}
 
@@ -240,7 +261,7 @@ class SnapshottingRefDirectory extends RefDirectory {
 		@Override
 		public void execute(RevWalk walk, ProgressMonitor monitor,
 				List<String> options) throws IOException {
-			invalidateSnapshotOnError((rw, m, o) -> super.execute(rw, m, o),
+			invalidateSnapshotOnError(super::execute,
 					walk, monitor, options, getRefDatabase());
 		}
 
@@ -250,5 +271,10 @@ class SnapshottingRefDirectory extends RefDirectory {
 			invalidateSnapshotOnError((rw, m, a3) -> super.execute(rw, m), walk,
 					monitor, null, getRefDatabase());
 		}
+
+		@Override
+		public SnapshottingRefDirectory getRefDatabase() {
+			return (SnapshottingRefDirectory) super.getRefDatabase();
+		}
 	}
 }