BatchRefUpdate: repro racy atomic update, and fix it

PackedBatchRefUpdate was creating a new packed-refs list that was
potentially unsorted. This would be papered over when the list was
read back from disk in parsePackedRef, which detects unsorted ref
lists on reading, and sorts them. However, the BatchRefUpdate also
installed the new (unsorted) list in-memory in
RefDirectory#packedRefs.

With the timestamp granularity code committed to stable-5.1, we can
more often accurately decide that the packed-refs file is clean, and
will return the erroneous unsorted data more often. Unluckily timed
delays also cause the file to be clean, hence this problem was
exacerbated under load.

The symptom is that refs added by a BatchRefUpdate would stop being
visible directly after they were added. In particular, the Gerrit
integration tests uses BatchRefUpdate in its setup for creating the
Admin group, and then tries to read it out directly afterward.

The tests recreates one failure case. A better approach would be to
revise RefList.Builder, so it detects out-of-order lists and
automatically sorts them.

Fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=548716 and
https://bugs.chromium.org/p/gerrit/issues/detail?id=11373.

Bug: 548716
Change-Id: I613c8059964513ce2370543620725b540b3cb6d1
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

1 files changed, 29 insertions, 0 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java
index 3c4b8cf4bc..2ac4a846ea 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java
@@ -43,6 +43,7 @@
 
 package org.eclipse.jgit.internal.storage.file;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.concurrent.TimeUnit.NANOSECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.eclipse.jgit.internal.storage.file.BatchRefUpdateTest.Result.LOCK_FAILURE;
@@ -64,6 +65,7 @@ import static org.junit.Assume.assumeTrue;
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Files;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -162,6 +164,33 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 	}
 
 	@Test
+	public void packedRefsFileIsSorted() throws IOException {
+		assumeTrue(atomic);
+
+		for (int i = 0; i < 2; i++) {
+			BatchRefUpdate bu = diskRepo.getRefDatabase().newBatchUpdate();
+			String b1  = String.format("refs/heads/a%d",i);
+			String b2  = String.format("refs/heads/b%d",i);
+			bu.setAtomic(atomic);
+			ReceiveCommand c1 = new ReceiveCommand(ObjectId.zeroId(), A, b1);
+			ReceiveCommand c2 = new ReceiveCommand(ObjectId.zeroId(), B, b2);
+			bu.addCommand(c1, c2);
+			try (RevWalk rw = new RevWalk(diskRepo)) {
+				bu.execute(rw, NullProgressMonitor.INSTANCE);
+			}
+			assertEquals(c1.getResult(), ReceiveCommand.Result.OK);
+			assertEquals(c2.getResult(), ReceiveCommand.Result.OK);
+		}
+
+		File packed = new File(diskRepo.getDirectory(), "packed-refs");
+		String packedStr = new String(Files.readAllBytes(packed.toPath()), UTF_8);
+
+		int a2 = packedStr.indexOf("refs/heads/a1");
+		int b1 = packedStr.indexOf("refs/heads/b0");
+		assertTrue(a2 <  b1);
+	}
+
+	@Test
 	public void simpleNoForce() throws IOException {
 		writeLooseRef("refs/heads/master", A);
 		writeLooseRef("refs/heads/masters", B);