PackedBatchRefUpdate: Ensure updates are applied on latest packed refs

In the window between refs being packed (via refDb.pack) and obtaining
updates (via applyUpdates), packed-refs may have been updated by another
actor and relying on the previously read contents may lead to losing the
updates done by the other actor. To help avoid this, read packed-refs
from disk to ensure we have the latest copy after it is locked and
before committing updates to it.

Bug: 581641
Change-Id: Iae71cb30830b307d0df929c9131911ee476c711c
Signed-off-by: Kaushik Lingarkar <quic_kaushikl@quicinc.com>

2 files changed, 29 insertions, 16 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java
index 8b0ea4fcd6..560b807565 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java
@@ -160,31 +160,35 @@ class PackedBatchRefUpdate extends BatchRefUpdate {
 		Map<String, LockFile> locks = null;
 		refdb.inProcessPackedRefsLock.lock();
 		try {
-			PackedRefList oldPackedList;
+			// During clone locking isn't needed since no refs exist yet.
+			// This also helps to avoid problems with refs only differing in
+			// case on a case insensitive filesystem (bug 528497)
 			if (!refdb.isInClone() && shouldLockLooseRefs) {
 				locks = lockLooseRefs(pending);
 				if (locks == null) {
 					return;
 				}
-				oldPackedList = refdb.pack(locks);
-			} else {
-				// During clone locking isn't needed since no refs exist yet.
-				// This also helps to avoid problems with refs only differing in
-				// case on a case insensitive filesystem (bug 528497)
-				oldPackedList = refdb.getPackedRefs();
-			}
-			RefList<Ref> newRefs = applyUpdates(walk, oldPackedList, pending);
-			if (newRefs == null) {
-				return;
+				refdb.pack(locks);
 			}
+
 			LockFile packedRefsLock = refdb.lockPackedRefs();
 			if (packedRefsLock == null) {
 				lockFailure(pending.get(0), pending);
 				return;
 			}
-			// commitPackedRefs removes lock file (by renaming over real file).
-			refdb.commitPackedRefs(packedRefsLock, newRefs, oldPackedList,
-					true);
+			try {
+				PackedRefList oldPackedList = refdb.refreshPackedRefs();
+				RefList<Ref> newRefs = applyUpdates(walk, oldPackedList, pending);
+				if (newRefs == null) {
+					return;
+				}
+				refdb.commitPackedRefs(packedRefsLock, newRefs, oldPackedList,
+						true);
+			} finally {
+				// This will be no-op if commitPackedRefs is successful as it
+				// will remove the lock file (by renaming over real file).
+				packedRefsLock.unlock();
+			}
 		} finally {
 			try {
 				unlockAll(locks);
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 065c20b616..72f1d5afa2 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
@@ -924,9 +924,18 @@ public class RefDirectory extends RefDatabase {
 			break;
 		}
 
+		return refreshPackedRefs(curList);
+	}
+
+	PackedRefList refreshPackedRefs() throws IOException {
+		return refreshPackedRefs(packedRefs.get());
+	}
+
+	private PackedRefList refreshPackedRefs(PackedRefList curList)
+			throws IOException {
 		final PackedRefList newList = readPackedRefs();
-		if (packedRefs.compareAndSet(curList, newList)
-				&& !curList.id.equals(newList.id)) {
+		if (packedRefs.compareAndSet(curList, newList) && !curList.id.equals(
+				newList.id)) {
 			modCnt.incrementAndGet();
 		}
 		return newList;