]> source.dussan.org Git - jgit.git/commitdiff
BasePackFetchConnection: support negotiationTip feature 86/200786/13
authorRonald Bhuleskar <funronald@google.com>
Wed, 22 Mar 2023 22:07:19 +0000 (15:07 -0700)
committerRonald Bhuleskar <funronald@google.com>
Tue, 28 Mar 2023 21:29:54 +0000 (14:29 -0700)
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
org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java

index cbcb36a25059df7ad4271ad4c5d8468ed1341084..3495813e76138c1d91721f826bb007c68741e224 100644 (file)
@@ -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 |
index 0e97d7b155f0c3d337d69ec190338a7106935443..901b9de86dd35afe4a14253f8f04c89abbf1d213 100644 (file)
@@ -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();
index be36d2b8347b0f62777ff01517393c4fb35f1fa7..8909380176e1edc6f9827391af6a5ca53a0f90ff 100644 (file)
@@ -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();