From e68a9b3ed8dbed4708f90d97ab2747c97aa0e123 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Wed, 6 Sep 2017 19:09:42 -0700 Subject: [PATCH] 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 --- .../ReceivePackAdvertiseRefsHookTest.java | 41 +++++++++++++++++++ .../jgit/transport/BaseReceivePack.java | 1 + 2 files changed, 42 insertions(+) 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 3411122888..8ef87cb3c1 100644 --- 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> 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 6420015bd0..44abcd598b 100644 --- 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 allRefs, Set additionalHaves) { refs = allRefs != null ? allRefs : db.getAllRefs(); refs = refFilter.filter(refs); + advertisedHaves.clear(); Ref head = refs.get(Constants.HEAD); if (head != null && head.isSymbolic()) -- 2.39.5