Only increment mod count if packed-refs file changes

Previously if a packed-refs file was racily clean then there
was a 2.5 second window in which each call to getPackedRefs
would increment the mod count causing a RefsChangedEvent to be
fired since the FileSnapshot would report the file as modified.

If a RefsChangedListener called getRef/getRefs from the
onRefsChanged method then a StackOverflowError could occur
since the stack could be exhausted before the 2.5 second
window expired and the packed-refs file would no longer
report being modified.

Now a SHA-1 is computed of the packed-refs file and the
mod count is only incremented when the packed refs are
successfully set and the id of the new packed-refs file
does not match the id of the old packed-refs file.

Change-Id: I8cab6e5929479ed748812b8598c7628370e79697

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java
index 3ca4f589dbcba4c267e528207cbff6c668d15363..508b6905090b1841106b3cfd2f6b4c71eb7769d1 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java
@@ -60,6 +60,8 @@ import static org.junit.Assert.fail;
 import java.io.File;
 import java.io.IOException;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jgit.events.ListenerHandle;
 import org.eclipse.jgit.events.RefsChangedEvent;
@@ -1077,6 +1079,36 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase {
                assertSame(master_p2, refdir.peel(master_p2));
        }
 
+       @Test
+       public void testRefsChangedStackOverflow() throws Exception {
+               final FileRepository newRepo = createBareRepository();
+               final RefDatabase refDb = newRepo.getRefDatabase();
+               File packedRefs = new File(newRepo.getDirectory(), "packed-refs");
+               assertTrue(packedRefs.createNewFile());
+               final AtomicReference<StackOverflowError> error = new AtomicReference<StackOverflowError>();
+               final AtomicReference<IOException> exception = new AtomicReference<IOException>();
+               final AtomicInteger changeCount = new AtomicInteger();
+               newRepo.getListenerList().addRefsChangedListener(
+                               new RefsChangedListener() {
+
+                                       public void onRefsChanged(RefsChangedEvent event) {
+                                               try {
+                                                       refDb.getRefs("ref");
+                                                       changeCount.incrementAndGet();
+                                               } catch (StackOverflowError soe) {
+                                                       error.set(soe);
+                                               } catch (IOException ioe) {
+                                                       exception.set(ioe);
+                                               }
+                                       }
+                               });
+               refDb.getRefs("ref");
+               refDb.getRefs("ref");
+               assertNull(error.get());
+               assertNull(exception.get());
+               assertEquals(1, changeCount.get());
+       }
+
        private void writeLooseRef(String name, AnyObjectId id) throws IOException {
                writeLooseRef(name, id.name() + "\n");
        }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java
index a152e86fb616ca0bea9cb2063bdb0237b9913b04..fe13f2c3c773d7f85acce11c27254bcf1b6f8a4b 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java
@@ -63,6 +63,8 @@ import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.security.DigestInputStream;
+import java.security.MessageDigest;
 import java.text.MessageFormat;
 import java.util.Arrays;
 import java.util.LinkedList;
@@ -626,24 +628,27 @@ public class RefDirectory extends RefDatabase {
                        return curList;
 
                final PackedRefList newList = readPackedRefs();
-               if (packedRefs.compareAndSet(curList, newList))
+               if (packedRefs.compareAndSet(curList, newList)
+                               && !curList.id.equals(newList.id))
                        modCnt.incrementAndGet();
                return newList;
        }
 
-       private PackedRefList readPackedRefs()
-                       throws IOException {
+       private PackedRefList readPackedRefs() throws IOException {
                final FileSnapshot snapshot = FileSnapshot.save(packedRefsFile);
                final BufferedReader br;
+               final MessageDigest digest = Constants.newMessageDigest();
                try {
-                       br = new BufferedReader(new InputStreamReader(new FileInputStream(
-                                       packedRefsFile), CHARSET));
+                       br = new BufferedReader(new InputStreamReader(
+                                       new DigestInputStream(new FileInputStream(packedRefsFile),
+                                                       digest), CHARSET));
                } catch (FileNotFoundException noPackedRefs) {
                        // Ignore it and leave the new list empty.
                        return PackedRefList.NO_PACKED_REFS;
                }
                try {
-                       return new PackedRefList(parsePackedRefs(br), snapshot);
+                       return new PackedRefList(parsePackedRefs(br), snapshot,
+                                       ObjectId.fromRaw(digest.digest()));
                } finally {
                        br.close();
                }
@@ -724,8 +729,9 @@ public class RefDirectory extends RefDatabase {
                                if (!lck.commit())
                                        throw new ObjectWritingException(MessageFormat.format(JGitText.get().unableToWrite, name));
 
-                               packedRefs.compareAndSet(oldPackedList, new PackedRefList(
-                                               refs, lck.getCommitSnapshot()));
+                               byte[] digest = Constants.newMessageDigest().digest(content);
+                               packedRefs.compareAndSet(oldPackedList, new PackedRefList(refs,
+                                               lck.getCommitSnapshot(), ObjectId.fromRaw(digest)));
                        }
                }.writePackedRefs();
        }
@@ -899,13 +905,17 @@ public class RefDirectory extends RefDatabase {
 
        private static class PackedRefList extends RefList<Ref> {
                static final PackedRefList NO_PACKED_REFS = new PackedRefList(
-                               RefList.emptyList(), FileSnapshot.MISSING_FILE);
+                               RefList.emptyList(), FileSnapshot.MISSING_FILE,
+                               ObjectId.zeroId());
 
                final FileSnapshot snapshot;
 
-               PackedRefList(RefList<Ref> src, FileSnapshot s) {
+               final ObjectId id;
+
+               PackedRefList(RefList<Ref> src, FileSnapshot s, ObjectId i) {
                        super(src);
                        snapshot = s;
+                       id = i;
                }
        }