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>

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 34f6c714a485f95fb2bfeb379c0969be0eed5942..8097cd6ae178fe7b5363d120d337a93430d45842 100644 (file)
--- 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 b661ae78cf9ae19796967618c5406fc89f979443..4309089b725852b338fddf6d7375705a833d2047 100644 (file)
--- 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 ecf7ef942eca4bbfee62b7d4bace335e06458825..59f4e50f9158f249017037ce2054218e5bbd68b3 100644 (file)
--- 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<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();