Fix missing RefsChangedEvent when packed refs are used

With atomic ref updates using packed refs, JGit did not fire a
RefsChangedEvent. This resulted in a user-visible regression in
EGit: the UI would not update after a "Fetch from upstream...".
Presumably it would also make Gerrit miss out on ref changes?

Strengthen the BatchRefUpdateTest by also asserting the expected
number of RefsChangedEvents, and ensure modCnt is incremented in
RefDirectory.commitPackedRefs() when refs really changed (as opposed
to some internal housekeeping operation, such as packing loose refs).

Bug: 521296
Change-Id: Ia985bda1d99f45a5f89c8020ca4845e7a66e743e
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java

@@ -73,6 +73,8 @@ import java.util.Map;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Predicate;
 
+import org.eclipse.jgit.events.ListenerHandle;
+import org.eclipse.jgit.events.RefsChangedListener;
 import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase;
 import org.eclipse.jgit.junit.StrictWorkMonitor;
 import org.eclipse.jgit.junit.TestRepository;
@@ -94,6 +96,7 @@ import org.eclipse.jgit.lib.StoredConfig;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -118,6 +121,21 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 	private RevCommit A;
 	private RevCommit B;
 
+	/**
+	 * When asserting the number of RefsChangedEvents you must account for one
+	 * additional event due to the initial ref setup via a number of calls to
+	 * {@link #writeLooseRef(String, AnyObjectId)} (will be fired in execute()
+	 * when it is detected that the on-disk loose refs have changed), or for one
+	 * additional event per {@link #writeRef(String, AnyObjectId)}.
+	 */
+	private int refsChangedEvents;
+
+	private ListenerHandle handle;
+
+	private RefsChangedListener refsChangedListener = event -> {
+		refsChangedEvents++;
+	};
+
 	@Override
 	@Before
 	public void setUp() throws Exception {
@@ -136,6 +154,15 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		repo = new TestRepository<>(diskRepo);
 		A = repo.commit().create();
 		B = repo.commit(repo.getRevWalk().parseCommit(A));
+		refsChangedEvents = 0;
+		handle = diskRepo.getListenerList()
+				.addRefsChangedListener(refsChangedListener);
+	}
+
+	@After
+	public void removeListener() {
+		handle.remove();
+		refsChangedEvents = 0;
 	}
 
 	@Test
@@ -153,11 +180,13 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 			assertRefs(
 					"refs/heads/master", A,
 					"refs/heads/masters", B);
+			assertEquals(1, refsChangedEvents);
 		} else {
 			assertResults(cmds, OK, REJECTED_NONFASTFORWARD);
 			assertRefs(
 					"refs/heads/master", B,
 					"refs/heads/masters", B);
+			assertEquals(2, refsChangedEvents);
 		}
 	}
 
@@ -175,6 +204,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		assertRefs(
 				"refs/heads/master", B,
 				"refs/heads/masters", A);
+		assertEquals(atomic ? 2 : 3, refsChangedEvents);
 	}
 
 	@Test
@@ -196,6 +226,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 
 		assertResults(cmds, OK);
 		assertRefs("refs/heads/master", A);
+		assertEquals(2, refsChangedEvents);
 	}
 
 	@Test
@@ -217,6 +248,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 			assertRefs(
 					"refs/heads/master", A,
 					"refs/heads/masters", B);
+			assertEquals(1, refsChangedEvents);
 		} else {
 			// Non-atomic updates are applied in order: master succeeds, then master/x
 			// fails due to conflict.
@@ -224,6 +256,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 			assertRefs(
 					"refs/heads/master", B,
 					"refs/heads/masters", B);
+			assertEquals(2, refsChangedEvents);
 		}
 	}
 
@@ -242,6 +275,15 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		assertRefs(
 				"refs/heads/master", B,
 				"refs/heads/masters/x", A);
+		if (atomic) {
+			assertEquals(2, refsChangedEvents);
+		} else {
+			// The non-atomic case actually produces 5 events, but that's an
+			// implementation detail. We expect at least 4 events, one for the
+			// initial read due to writeLooseRef(), and then one for each
+			// successful ref update.
+			assertTrue(refsChangedEvents >= 4);
+		}
 	}
 
 	@Test
@@ -258,11 +300,13 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		if (atomic) {
 			assertResults(cmds, REJECTED_MISSING_OBJECT, TRANSACTION_ABORTED);
 			assertRefs("refs/heads/master", A);
+			assertEquals(1, refsChangedEvents);
 		} else {
 			assertResults(cmds, REJECTED_MISSING_OBJECT, OK);
 			assertRefs(
 					"refs/heads/master", A,
 					"refs/heads/foo2", B);
+			assertEquals(2, refsChangedEvents);
 		}
 	}
 
@@ -280,9 +324,11 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		if (atomic) {
 			assertResults(cmds, TRANSACTION_ABORTED, REJECTED_MISSING_OBJECT);
 			assertRefs("refs/heads/master", A);
+			assertEquals(1, refsChangedEvents);
 		} else {
 			assertResults(cmds, OK, REJECTED_MISSING_OBJECT);
 			assertRefs("refs/heads/master", B);
+			assertEquals(2, refsChangedEvents);
 		}
 	}
 
@@ -296,9 +342,11 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		if (atomic) {
 			assertResults(cmds, LOCK_FAILURE, TRANSACTION_ABORTED);
 			assertRefs();
+			assertEquals(0, refsChangedEvents);
 		} else {
 			assertResults(cmds, LOCK_FAILURE, OK);
 			assertRefs("refs/heads/foo2", B);
+			assertEquals(1, refsChangedEvents);
 		}
 	}
 
@@ -314,11 +362,13 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		if (atomic) {
 			assertResults(cmds, LOCK_FAILURE, TRANSACTION_ABORTED);
 			assertRefs("refs/heads/master", A);
+			assertEquals(1, refsChangedEvents);
 		} else {
 			assertResults(cmds, LOCK_FAILURE, OK);
 			assertRefs(
 					"refs/heads/master", A,
 					"refs/heads/foo2", B);
+			assertEquals(2, refsChangedEvents);
 		}
 	}
 
@@ -334,9 +384,11 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		if (atomic) {
 			assertResults(cmds, TRANSACTION_ABORTED, LOCK_FAILURE);
 			assertRefs("refs/heads/master", A);
+			assertEquals(1, refsChangedEvents);
 		} else {
 			assertResults(cmds, OK, LOCK_FAILURE);
 			assertRefs("refs/heads/master", B);
+			assertEquals(2, refsChangedEvents);
 		}
 	}
 
@@ -357,6 +409,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		assertRefs(
 				"refs/heads/master", B,
 				"refs/heads/branch", B);
+		assertEquals(atomic ? 2 : 3, refsChangedEvents);
 		assertReflogUnchanged(oldLogs, "refs/heads/master");
 		assertReflogUnchanged(oldLogs, "refs/heads/branch");
 	}
@@ -381,6 +434,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 				"refs/heads/master", B,
 				"refs/heads/branch1", B,
 				"refs/heads/branch2", A);
+		assertEquals(atomic ? 3 : 4, refsChangedEvents);
 		assertReflogEquals(
 				reflog(A, B, new PersonIdent(diskRepo), "a reflog"),
 				getLastReflog("refs/heads/master"));
@@ -409,6 +463,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 				"refs/heads/master", B,
 				"refs/heads/branch1", A,
 				"refs/heads/branch2", A);
+		assertEquals(atomic ? 3 : 5, refsChangedEvents);
 		assertReflogEquals(
 				// Always forced; setAllowNonFastForwards(true) bypasses the check.
 				reflog(A, B, new PersonIdent(diskRepo), "forced-update"),
@@ -431,6 +486,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 
 		assertResults(cmds, OK);
 		assertRefs("refs/heads/master", B);
+		assertEquals(2, refsChangedEvents);
 		assertReflogEquals(
 				reflog(A, B, new PersonIdent(diskRepo), "fast-forward"),
 				getLastReflog("refs/heads/master"));
@@ -449,6 +505,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		assertRefs(
 				"refs/heads/master", B,
 				"refs/heads/branch", A);
+		assertEquals(atomic ? 2 : 3, refsChangedEvents);
 		assertReflogEquals(
 				reflog(A, B, new PersonIdent(diskRepo), "a reflog: fast-forward"),
 				getLastReflog("refs/heads/master"));
@@ -471,6 +528,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 						.setRefLogIdent(ident));
 
 		assertResults(cmds, OK, OK);
+		assertEquals(atomic ? 2 : 3, refsChangedEvents);
 		assertRefs(
 				"refs/heads/master", B,
 				"refs/heads/branch", B);
@@ -498,6 +556,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 
 		assertResults(cmds, OK, OK);
 		assertRefs("refs/heads/branch", B);
+		assertEquals(atomic ? 3 : 4, refsChangedEvents);
 		assertNull(getLastReflog("refs/heads/master"));
 		assertReflogEquals(
 				reflog(A, B, new PersonIdent(diskRepo), "a reflog"),
@@ -515,6 +574,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 
 		assertResults(cmds, OK, OK);
 		assertRefs("refs/heads/master/x", A);
+		assertEquals(atomic ? 2 : 3, refsChangedEvents);
 		assertNull(getLastReflog("refs/heads/master"));
 		assertReflogEquals(
 				reflog(zeroId(), A, new PersonIdent(diskRepo), "a reflog"),
@@ -535,10 +595,12 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 
 		if (atomic) {
 			assertResults(cmds, TRANSACTION_ABORTED, LOCK_FAILURE);
+			assertEquals(1, refsChangedEvents);
 			assertReflogUnchanged(oldLogs, "refs/heads/master");
 			assertReflogUnchanged(oldLogs, "refs/heads/branch");
 		} else {
 			assertResults(cmds, OK, LOCK_FAILURE);
+			assertEquals(2, refsChangedEvents);
 			assertReflogEquals(
 					reflog(A, B, new PersonIdent(diskRepo), "a reflog"),
 					getLastReflog("refs/heads/master"));
@@ -561,6 +623,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 						.setRefLogMessage("a reflog", true));
 
 		assertResults(cmds, OK, OK);
+		assertEquals(atomic ? 2 : 3, refsChangedEvents);
 		assertReflogEquals(
 				reflog(A, B, ident, "custom log"),
 				getLastReflog("refs/heads/master"),
@@ -585,6 +648,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		execute(newBatchUpdate(cmds).setRefLogMessage("a reflog", true));
 
 		assertResults(cmds, OK, OK);
+		assertEquals(atomic ? 2 : 3, refsChangedEvents);
 		assertReflogUnchanged(oldLogs, "refs/heads/master");
 		assertReflogEquals(
 				reflog(zeroId(), B, new PersonIdent(diskRepo), "a reflog: created"),
@@ -609,12 +673,14 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 			if (atomic) {
 				assertResults(cmds, LOCK_FAILURE, TRANSACTION_ABORTED);
 				assertRefs("refs/heads/master", A);
+				assertEquals(1, refsChangedEvents);
 			} else {
 				// Only operates on loose refs, doesn't care that packed-refs is locked.
 				assertResults(cmds, OK, OK);
 				assertRefs(
 						"refs/heads/master", B,
 						"refs/heads/branch", B);
+				assertEquals(3, refsChangedEvents);
 			}
 		} finally {
 			myLock.unlock();
@@ -640,11 +706,13 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 			if (atomic) {
 				assertResults(cmds, TRANSACTION_ABORTED, LOCK_FAILURE);
 				assertRefs("refs/heads/master", A);
+				assertEquals(1, refsChangedEvents);
 			} else {
 				assertResults(cmds, OK, LOCK_FAILURE);
 				assertRefs(
 						"refs/heads/branch", B,
 						"refs/heads/master", A);
+				assertEquals(2, refsChangedEvents);
 			}
 		} finally {
 			myLock.unlock();
@@ -664,6 +732,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 
 			assertFalse(getLockFile("refs/heads/master").exists());
 			assertResults(cmds, OK);
+			assertEquals(2, refsChangedEvents);
 			assertRefs("refs/heads/master", B);
 		} finally {
 			myLock.unlock();
@@ -716,6 +785,7 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase {
 		}
 
 		assertResults(cmds, OK, OK);
+		assertEquals(2, refsChangedEvents);
 		assertRefs(
 				"refs/heads/master", B,
 				"refs/heads/branch", B);

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java

@@ -194,7 +194,8 @@ class PackedBatchRefUpdate extends BatchRefUpdate {
 				return;
 			}
 			// commitPackedRefs removes lock file (by renaming over real file).
-			refdb.commitPackedRefs(packedRefsLock, newRefs, oldPackedList);
+			refdb.commitPackedRefs(packedRefsLock, newRefs, oldPackedList,
+					true);
 		} finally {
 			try {
 				unlockAll(locks);

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java

@@ -633,8 +633,9 @@ public class RefDirectory extends RefDatabase {
 				try {
 					PackedRefList cur = readPackedRefs();
 					int idx = cur.find(name);
-					if (0 <= idx)
-						commitPackedRefs(lck, cur.remove(idx), packed);
+					if (0 <= idx) {
+						commitPackedRefs(lck, cur.remove(idx), packed, true);
+					}
 				} finally {
 					lck.unlock();
 				}
@@ -733,7 +734,8 @@ public class RefDirectory extends RefDatabase {
 				}
 
 				// The new content for packed-refs is collected. Persist it.
-				PackedRefList result = commitPackedRefs(lck, cur, packed);
+				PackedRefList result = commitPackedRefs(lck, cur, packed,
+						false);
 
 				// Now delete the loose refs which are now packed
 				for (String refName : refs) {
@@ -988,7 +990,8 @@ public class RefDirectory extends RefDatabase {
 	}
 
 	PackedRefList commitPackedRefs(final LockFile lck, final RefList<Ref> refs,
-			final PackedRefList oldPackedList) throws IOException {
+			final PackedRefList oldPackedList, boolean changed)
+			throws IOException {
 		// Can't just return packedRefs.get() from this method; it might have been
 		// updated again after writePackedRefs() returns.
 		AtomicReference<PackedRefList> result = new AtomicReference<>();
@@ -1031,6 +1034,9 @@ public class RefDirectory extends RefDatabase {
 					throw new ObjectWritingException(
 							MessageFormat.format(JGitText.get().unableToWrite, name));
 				}
+				if (changed) {
+					modCnt.incrementAndGet();
+				}
 				result.set(newPackedList);
 			}
 		}.writePackedRefs();