diff --git a/Documentation/config-options.md b/Documentation/config-options.md index cbcb36a25..3495813e7 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` | ⃞ | 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 | ✅ | The path to the root of the working tree. | +## __fetch__ options + +| option | default | git option | description | +|---------|---------|------------|-------------| +| `fetch.useNegotiationTip` | `false` | ✅ | 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 0e97d7b15..901b9de86 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.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.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 @@ * Tests for server upload-pack utilities. */ public class UploadPackTest { + private static final int MAX_HAVES = 64; + private URIish uri; private TestProtocol testProtocol; @@ -87,6 +93,7 @@ public void setUp() throws Exception { @After public void tearDown() { + TestProtocol.setFetchConfig(null); Transport.unregister(testProtocol); } @@ -2158,6 +2165,231 @@ public void testV2FetchWantRef() throws Exception { assertFalse(client.getObjectDatabase().has(three.toObjectId())); } + /** + *
+	 * remote:
+	 *    foo <- foofoo <-- branchFoo
+	 *    bar <- barbar <-- branchBar
+	 *
+	 * client:
+	 *    foo <-- branchFoo
+	 *    bar <-- branchBar
+	 *
+	 * fetch(branchFoo) should send exactly 1 have (i.e. foo) from branchFoo
+	 * 
+ */ + @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 = new AtomicReference<>(); + CountHavesPreUploadHook countHavesHook = new CountHavesPreUploadHook(); + RevCommit localFooParent = null; + + // Client has lagging branchFoo at fooParent and branchBar at barParent + try (TestRepository 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: + * + *
+	 * 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
+	 * 
+ */ + @Test + public void testNegotiationTipWithLongerHistoryThanMaxHaves() + throws Exception { + Set remoteFooCommits = new HashSet<>(); + Set 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 = new AtomicReference<>(); + CountHavesPreUploadHook countHavesHook = new CountHavesPreUploadHook(); + Set localFooCommits = new HashSet<>(); + + try (TestRepository 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 havesSentDuringNegotiation = new HashSet<>(); + + @Override + public void onSendPack(UploadPack unusedUploadPack, + Collection unusedWants, + Collection haves) + throws ServiceMayNotContinueException { + // record haves + havesSentDuringNegotiation.addAll(haves); + } + + @Override + public void onEndNegotiateRound(UploadPack unusedUploadPack, + Collection unusedWants, int unusedCntCommon, + int unusedCntNotFound, boolean unusedReady) + throws ServiceMayNotContinueException { + // Do nothing. + } + + @Override + public void onBeginNegotiateRound(UploadPack unusedUploadPack, + Collection 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 be36d2b83..890938017 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.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 BasePackFetchConnection(PackTransport packTransport) { 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 @@ static class FetchConfig { 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 @@ protected void doFetch(final ProgressMonitor monitor, noProgress = monitor == NullProgressMonitor.INSTANCE; markRefsAdvertised(); - markReachable(have, maxTimeWanted(want)); + markReachable(want, have, maxTimeWanted(want)); if (TransferConfig.ProtocolVersion.V2 .equals(getProtocolVersion())) { @@ -662,9 +688,17 @@ private int maxTimeWanted(Collection wants) { return maxTime; } - private void markReachable(Set have, int maxTime) + private void markReachable(Collection want, Set have, + int maxTime) throws IOException { + Set 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();