ReceivePack: clear advertised .haves if application changes refs

An application can choose to invoke setAdvertisedRefs multiple times,
for example several AdvertiseRefsHook installed in a chain. Each of
these invocations populates the advertisedHaves collection with the
unique set of ObjectIds.

This can lead to a server over-advertising with ".have" lines if the
first hook pushes in a lot of references, and the second hook filters
this to a subset.  ReceivePack will advertise the unique objects from
the first hook using ".have" lines, which may lead to a huge
advertisement sent to the client.

This can also contribute to a very slow connectivity check after the
pack is parsed as ReceivePack calls markUninteresting on every commit
in advertisedHaves.  This may require expanding a lot of subtrees to
mark all trees as uninteresting as well.  On a very big repository
this can lead to a many-second stall.

Clear the advertisedHaves collection any time the refs are updated.
Add a test to verify the correct set of objects was sent.

Change-Id: I97f6998d0597251444a2e846a3ea1f461bae96f9

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java
index 3411122888ed2bd6b6bf85433c256a0ceb0bfbe5..8ef87cb3c116b3e2a5c51854bb149d28e4855371 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackAdvertiseRefsHookTest.java
@@ -58,6 +58,8 @@ import java.security.MessageDigest;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.zip.Deflater;
 
 import org.eclipse.jgit.errors.MissingObjectException;
@@ -158,6 +160,45 @@ public class ReceivePackAdvertiseRefsHookTest extends LocalDiskRepositoryTestCas
                assertEquals(B, master.getObjectId());
        }
 
+       @Test
+       public void resetsHaves() throws Exception {
+               AtomicReference<Set<ObjectId>> haves = new AtomicReference<>();
+               try (TransportLocal t = new TransportLocal(src, uriOf(dst),
+                               dst.getDirectory()) {
+                       @Override
+                       ReceivePack createReceivePack(Repository db) {
+                               dst.incrementOpen();
+
+                               ReceivePack rp = super.createReceivePack(dst);
+                               rp.setAdvertiseRefsHook(new AdvertiseRefsHook() {
+                                       @Override
+                                       public void advertiseRefs(BaseReceivePack rp2)
+                                                       throws ServiceMayNotContinueException {
+                                               rp.setAdvertisedRefs(rp.getRepository().getAllRefs(),
+                                                               null);
+                                               new HidePrivateHook().advertiseRefs(rp);
+                                               haves.set(rp.getAdvertisedObjects());
+                                       }
+
+                                       @Override
+                                       public void advertiseRefs(UploadPack uploadPack)
+                                                       throws ServiceMayNotContinueException {
+                                               throw new UnsupportedOperationException();
+                                       }
+                               });
+                               return rp;
+                       }
+               }) {
+                       try (PushConnection c = t.openPush()) {
+                               // Just has to open/close for advertisement.
+                       }
+               }
+
+               assertEquals(1, haves.get().size());
+               assertTrue(haves.get().contains(B));
+               assertFalse(haves.get().contains(P));
+       }
+
        @Test
        public void testSuccess() throws Exception {
                // Manually force a delta of an object so we reuse it later.

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
index 6420015bd0aaf261e5d77a2fb3b4dcfd13e17a19..44abcd598bd0cccefca9ce09cc8b537c1d312207 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
@@ -447,6 +447,7 @@ public abstract class BaseReceivePack {
        public void setAdvertisedRefs(Map<String, Ref> allRefs, Set<ObjectId> additionalHaves) {
                refs = allRefs != null ? allRefs : db.getAllRefs();
                refs = refFilter.filter(refs);
+               advertisedHaves.clear();
 
                Ref head = refs.get(Constants.HEAD);
                if (head != null && head.isSymbolic())