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 34f6c714a..8097cd6ae 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 @@ -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); 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 b661ae78c..4309089b7 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 @@ -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); 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 ecf7ef942..59f4e50f9 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 @@ -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 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 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();