From 738dacb7fbdd1bfec0bd4d387b37ac5e89d36c07 Mon Sep 17 00:00:00 2001
From: Ronald Bhuleskar <funronald@google.com>
Date: Wed, 22 Mar 2023 15:07:19 -0700
Subject: [PATCH] BasePackFetchConnection: support negotiationTip feature

By default, Git will report, to the server, commits reachable from all local refs to find common commits in an attempt to reduce the size of the to-be-received packfile. If specified with negotiation tip, Git will only report commits reachable from the given tips. This is useful to speed up fetches when the user knows which local ref is likely to have commits in common with the upstream ref being fetched.

When negotation-tip is on, use the wanted refs instead of all refs as source of the "have" list to send.

This is controlled by the `fetch.usenegotationtip` flag, false by default. This works only for programmatic fetches and there is no support for it yet in the CLI.

Change-Id: I19f8fe48889bfe0ece7cdf78019b678ede5c6a32
---
 Documentation/config-options.md               |   6 +
 .../jgit/transport/UploadPackTest.java        | 232 ++++++++++++++++++
 .../transport/BasePackFetchConnection.java    |  38 ++-
 3 files changed, 274 insertions(+), 2 deletions(-)

diff --git a/Documentation/config-options.md b/Documentation/config-options.md
index cbcb36a250..3495813e76 100644
--- a/Documentation/config-options.md
+++ b/Documentation/config-options.md
@@ -50,6 +50,12 @@ For details on native git options see also the official [git config documentatio
 | `core.trustPackedRefsStat` | `unset` | &#x20DE; | Whether to trust the file attributes (Java equivalent of stat command on *nix) of the packed-refs file. If `never` JGit will ignore the file attributes of the packed-refs file and always read it. If `always` JGit will trust the file attributes of the packed-refs file and will only read it if a file attribute has changed. `after_open` behaves the same as `always`, except that the packed-refs file is opened and closed before its file attributes are considered. An open/close of the packed-refs file is known to refresh its file attributes, at least on some NFS clients. If `unset`, JGit will use the behavior described in `trustFolderStat`. |
 | `core.worktree` | Root directory of the working tree if it is not the parent directory of the `.git` directory | &#x2705; | The path to the root of the working tree. |
 
+## __fetch__ options
+
+|  option | default | git option | description |
+|---------|---------|------------|-------------|
+| `fetch.useNegotiationTip` | `false` | &#x2705; | When enabled it restricts the client negotiation on unrelated branches i.e. only send haves for the refs that the client is interested in fetching. |
+
 ## __gc__ options
 
 |  option | default | git option | description |
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
index 0e97d7b155..901b9de86d 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
@@ -22,9 +22,12 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Consumer;
 
 import org.eclipse.jgit.dircache.DirCache;
@@ -53,6 +56,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.storage.pack.PackStatistics;
+import org.eclipse.jgit.transport.BasePackFetchConnection.FetchConfig;
 import org.eclipse.jgit.transport.UploadPack.RequestPolicy;
 import org.eclipse.jgit.util.io.NullOutputStream;
 import org.junit.After;
@@ -63,6 +67,8 @@ import org.junit.Test;
  * Tests for server upload-pack utilities.
  */
 public class UploadPackTest {
+	private static final int MAX_HAVES = 64;
+
 	private URIish uri;
 
 	private TestProtocol<Object> testProtocol;
@@ -87,6 +93,7 @@ public class UploadPackTest {
 
 	@After
 	public void tearDown() {
+		TestProtocol.setFetchConfig(null);
 		Transport.unregister(testProtocol);
 	}
 
@@ -2158,6 +2165,231 @@ public class UploadPackTest {
 		assertFalse(client.getObjectDatabase().has(three.toObjectId()));
 	}
 
+	/**
+	 * <pre>
+	 * remote:
+	 *    foo <- foofoo <-- branchFoo
+	 *    bar <- barbar <-- branchBar
+	 *
+	 * client:
+	 *    foo <-- branchFoo
+	 *    bar <-- branchBar
+	 *
+	 * fetch(branchFoo) should send exactly 1 have (i.e. foo) from branchFoo
+	 * </pre>
+	 */
+	@Test
+	public void testNegotiationTip() throws Exception {
+		RevCommit fooParent = remote.commit().message("foo").create();
+		RevCommit fooChild = remote.commit().message("foofoo").parent(fooParent)
+				.create();
+		RevCommit barParent = remote.commit().message("bar").create();
+		RevCommit barChild = remote.commit().message("barbar").parent(barParent)
+				.create();
+
+		// Remote has branchFoo at fooChild and branchBar at barChild
+		remote.update("branchFoo", fooChild);
+		remote.update("branchBar", barChild);
+
+		AtomicReference<UploadPack> uploadPack = new AtomicReference<>();
+		CountHavesPreUploadHook countHavesHook = new CountHavesPreUploadHook();
+		RevCommit localFooParent = null;
+
+		// Client has lagging branchFoo at fooParent and branchBar at barParent
+		try (TestRepository<InMemoryRepository> clientRepo = new TestRepository<>(
+				client)) {
+			localFooParent = clientRepo.commit().message("foo")
+					.create();
+			RevCommit localBarParent = clientRepo.commit().message("bar")
+					.create();
+
+			clientRepo.update("branchFoo", localFooParent);
+			clientRepo.update("branchBar", localBarParent);
+
+			testProtocol = new TestProtocol<>((Object req, Repository db) -> {
+				UploadPack up = new UploadPack(db);
+				up.setPreUploadHook(countHavesHook);
+				uploadPack.set(up);
+				return up;
+			}, null);
+
+			uri = testProtocol.register(ctx, server);
+
+			TestProtocol.setFetchConfig(new FetchConfig(true, MAX_HAVES, true));
+			try (Transport tn = testProtocol.open(uri,
+					clientRepo.getRepository(), "server")) {
+
+				tn.fetch(NullProgressMonitor.INSTANCE, Collections
+						.singletonList(new RefSpec("refs/heads/branchFoo")),
+						"branchFoo");
+			}
+		}
+
+		assertTrue(client.getObjectDatabase().has(fooParent.toObjectId()));
+		assertTrue(client.getObjectDatabase().has(fooChild.toObjectId()));
+		assertFalse(client.getObjectDatabase().has(barChild.toObjectId()));
+
+		assertEquals(1, uploadPack.get().getStatistics().getHaves());
+		assertTrue(countHavesHook.havesSentDuringNegotiation
+				.contains(localFooParent.toObjectId()));
+	}
+
+	/**
+	 * Remote has 2 branches branchFoo and branchBar, each of them have 128 (2 x
+	 * MAX_HAVES) commits each. Local has both the branches with lagging
+	 * commits. Only 64 (1 x MAX_HAVES) from each branch and lagging 64.
+	 * fetch(branchFoo) should send all 64 (MAX_HAVES) commits on branchFoo as
+	 * haves and none from branchBar.
+	 *
+	 * Visual representation of the same:
+	 *
+	 * <pre>
+	 * remote:
+	 *             parent
+	 *             /    \
+	 *  branchFoo-0     branchBar-0
+	 *  branchFoo-1     branchBar-1
+	 *  ...			  ...
+	 *  ... 		  ...
+	 *  branchFoo-128   branchBar-128
+	 *    ^-- branchFoo		^--branchBar
+	 *
+	 *  local:
+	 *             parent
+	 *             /    \
+	 *  branchFoo-0     branchBar-0
+	 *  branchFoo-1     branchBar-1
+	 *  ...			  ...
+	 *  ... 		  ...
+	 *  branchFoo-64     branchBar-64
+	 *      ^-- branchFoo	^--branchBar
+	 *
+	 * fetch(branchFoo) should send all 64 (MAX_HAVES) commits on branchFoo as haves
+	 * </pre>
+	 */
+	@Test
+	public void testNegotiationTipWithLongerHistoryThanMaxHaves()
+			throws Exception {
+		Set<RevCommit> remoteFooCommits = new HashSet<>();
+		Set<RevCommit> remoteBarCommits = new HashSet<>();
+
+		RevCommit parent = remote.commit().message("branchFoo-0").create();
+		RevCommit parentFoo = parent;
+		remoteFooCommits.add(parentFoo);
+		for (int i = 1; i < 2 * MAX_HAVES; i++) {
+			RevCommit child = remote.commit()
+					.message("branchFoo-" + Integer.toString(i))
+					.parent(parentFoo)
+					.create();
+			parentFoo = child;
+			remoteFooCommits.add(parentFoo);
+
+		}
+		remote.update("branchFoo", parentFoo);
+
+		RevCommit parentBar = parent;
+		remoteBarCommits.add(parentBar);
+		for (int i = 1; i < 2 * MAX_HAVES; i++) {
+			RevCommit child = remote.commit()
+					.message("branchBar-" + Integer.toString(i))
+					.parent(parentBar)
+					.create();
+			parentBar = child;
+			remoteBarCommits.add(parentBar);
+		}
+		remote.update("branchBar", parentBar);
+
+		AtomicReference<UploadPack> uploadPack = new AtomicReference<>();
+		CountHavesPreUploadHook countHavesHook = new CountHavesPreUploadHook();
+		Set<ObjectId> localFooCommits = new HashSet<>();
+
+		try (TestRepository<InMemoryRepository> clientRepo = new TestRepository<>(
+				client)) {
+			RevCommit localParent = clientRepo.commit().message("branchBar-0")
+					.create();
+			RevCommit localParentFoo = localParent;
+			localFooCommits.add(localParentFoo);
+			for (int i = 1; i < 1 * MAX_HAVES; i++) {
+				RevCommit child = clientRepo.commit()
+						.message("branchFoo-" + Integer.toString(i))
+						.parent(localParentFoo).create();
+				localParentFoo = child;
+				localFooCommits.add(localParentFoo);
+			}
+			clientRepo.update("branchFoo", localParentFoo);
+
+			RevCommit localParentBar = localParent;
+			for (int i = 1; i < 1 * MAX_HAVES; i++) {
+				RevCommit child = clientRepo.commit()
+						.message("branchBar-" + Integer.toString(i))
+						.parent(localParentBar).create();
+				localParentBar = child;
+			}
+			clientRepo.update("branchBar", localParentBar);
+
+			testProtocol = new TestProtocol<>((Object req, Repository db) -> {
+				UploadPack up = new UploadPack(db);
+				up.setPreUploadHook(countHavesHook);
+				uploadPack.set(up);
+				return up;
+			}, null);
+
+			uri = testProtocol.register(ctx, server);
+			TestProtocol.setFetchConfig(new FetchConfig(true, MAX_HAVES, true));
+			try (Transport tn = testProtocol.open(uri,
+					clientRepo.getRepository(), "server")) {
+
+				tn.fetch(NullProgressMonitor.INSTANCE, Collections
+						.singletonList(new RefSpec("refs/heads/branchFoo")),
+						"branchFoo");
+			}
+		}
+
+		for (RevCommit c : remoteFooCommits) {
+			assertTrue(c.toObjectId() + "",
+					client.getObjectDatabase().has(c.toObjectId()));
+		}
+		remoteBarCommits.remove(parent);
+		for (RevCommit c : remoteBarCommits) {
+			assertFalse(client.getObjectDatabase().has(c.toObjectId()));
+		}
+
+		assertEquals(MAX_HAVES, uploadPack.get().getStatistics().getHaves());
+		// Verify that all the haves that were sent during negotiation are local
+		// commits from branchFoo
+		for (Object id : countHavesHook.havesSentDuringNegotiation) {
+			assertTrue(localFooCommits.contains(id));
+		}
+	}
+
+	private static class CountHavesPreUploadHook implements PreUploadHook {
+		Set<ObjectId> havesSentDuringNegotiation = new HashSet<>();
+
+		@Override
+		public void onSendPack(UploadPack unusedUploadPack,
+				Collection<? extends ObjectId> unusedWants,
+				Collection<? extends ObjectId> haves)
+				throws ServiceMayNotContinueException {
+			// record haves
+			havesSentDuringNegotiation.addAll(haves);
+		}
+
+		@Override
+		public void onEndNegotiateRound(UploadPack unusedUploadPack,
+				Collection<? extends ObjectId> unusedWants, int unusedCntCommon,
+				int unusedCntNotFound, boolean unusedReady)
+				throws ServiceMayNotContinueException {
+			// Do nothing.
+		}
+
+		@Override
+		public void onBeginNegotiateRound(UploadPack unusedUploadPack,
+				Collection<? extends ObjectId> unusedWants,
+				int unusedCntOffered) throws ServiceMayNotContinueException {
+			// Do nothing.
+		}
+	}
+
 	@Test
 	public void testV2FetchBadWantRef() throws Exception {
 		RevCommit one = remote.commit().message("1").create();
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
index be36d2b834..8909380176 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
@@ -37,6 +37,7 @@ import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.errors.RemoteRepositoryException;
@@ -215,6 +216,8 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
 	private boolean allowOfsDelta;
 
+	private boolean useNegotiationTip;
+
 	private boolean noDone;
 
 	private boolean noProgress;
@@ -259,9 +262,11 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 			final FetchConfig cfg = getFetchConfig();
 			allowOfsDelta = cfg.allowOfsDelta;
 			maxHaves = cfg.maxHaves;
+			useNegotiationTip = cfg.useNegotiationTip;
 		} else {
 			allowOfsDelta = true;
 			maxHaves = Integer.MAX_VALUE;
+			useNegotiationTip = false;
 		}
 
 		includeTags = transport.getTagOpt() != TagOpt.NO_TAGS;
@@ -297,14 +302,35 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
 		final int maxHaves;
 
+		final boolean useNegotiationTip;
+
 		FetchConfig(Config c) {
 			allowOfsDelta = c.getBoolean("repack", "usedeltabaseoffset", true); //$NON-NLS-1$ //$NON-NLS-2$
 			maxHaves = c.getInt("fetch", "maxhaves", Integer.MAX_VALUE); //$NON-NLS-1$ //$NON-NLS-2$
+			useNegotiationTip = c.getBoolean("fetch", "usenegotiationtip", //$NON-NLS-1$ //$NON-NLS-2$
+					false);
 		}
 
 		FetchConfig(boolean allowOfsDelta, int maxHaves) {
+			this(allowOfsDelta, maxHaves, false);
+		}
+
+		/**
+		 * @param allowOfsDelta
+		 *            when true optimizes the pack size by deltafying base
+		 *            object
+		 * @param maxHaves
+		 *            max haves to be sent per negotiation
+		 * @param useNegotiationTip
+		 *            if true uses the wanted refs instead of all refs as source
+		 *            of the "have" list to send.
+		 * @since 6.6
+		 */
+		FetchConfig(boolean allowOfsDelta, int maxHaves,
+				boolean useNegotiationTip) {
 			this.allowOfsDelta = allowOfsDelta;
 			this.maxHaves = maxHaves;
+			this.useNegotiationTip = useNegotiationTip;
 		}
 	}
 
@@ -384,7 +410,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 			noProgress = monitor == NullProgressMonitor.INSTANCE;
 
 			markRefsAdvertised();
-			markReachable(have, maxTimeWanted(want));
+			markReachable(want, have, maxTimeWanted(want));
 
 			if (TransferConfig.ProtocolVersion.V2
 					.equals(getProtocolVersion())) {
@@ -662,9 +688,17 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		return maxTime;
 	}
 
-	private void markReachable(Set<ObjectId> have, int maxTime)
+	private void markReachable(Collection<Ref> want, Set<ObjectId> have,
+			int maxTime)
 			throws IOException {
+		Set<String> wantRefs = want.stream().map(Ref::getName)
+				.collect(Collectors.toSet());
+
 		for (Ref r : local.getRefDatabase().getRefs()) {
+			if (useNegotiationTip && !wantRefs.contains(r.getName())) {
+				continue;
+			}
+
 			ObjectId id = r.getPeeledObjectId();
 			if (id == null)
 				id = r.getObjectId();
-- 
2.39.5