From e68a9b3ed8dbed4708f90d97ab2747c97aa0e123 Mon Sep 17 00:00:00 2001
From: Shawn Pearce <spearce@spearce.org>
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 341112288..8ef87cb3c 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<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 6420015bd..44abcd598 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<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())