Protocol V2: respect MAX_HAVES only once we got at least one ACK

The negotiation in the git protocol contains a cutoff: if the client
has sent more than MAX_HAVES "have" lines without getting an ACK, it
gives up and sends a "done". MAX_HAVES is 256.

However, this cutoff must kick in only if at least one ACK has been
received. Otherwise the client may give up way too early, which makes
the server send all its history. See [1].

This was missed when protocol V2 was implemented for fetching in JGit
in commit 0853a241.

Compare also C git commit 0b07eecf6ed.[2] C git had the same bug.[3][4]

[1] https://github.com/git/git/blob/6c430a647cb9/Documentation/technical/pack-protocol.txt#L385
[2] https://github.com/git/git/commit/0b07eecf6ed
[3] https://lore.kernel.org/git/b7f5bfb9-61fb-2552-4399-b744428728e4@suse.cz/
[4] https://lore.kernel.org/git/20200422084254.GA27502@furthur.local/

Bug: 553083
Change-Id: I1f4e2cc16b5eed6971d981d472329185abb9e4a9
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
index f2c7bc63c41d6bb8543877f1d97d38a2f92ac729..def52ff6bb50e83538274a585dd51bd0aef8addf 100644 (file)
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
@@ -22,10 +22,14 @@ import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.OutputStreamWriter;
 import java.io.PrintWriter;
+import java.io.Writer;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
 import java.text.MessageFormat;
 import java.util.Collections;
 import java.util.EnumSet;
@@ -70,6 +74,7 @@ import org.eclipse.jgit.lib.ReflogEntry;
 import org.eclipse.jgit.lib.ReflogReader;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.StoredConfig;
+import org.eclipse.jgit.lib.TextProgressMonitor;
 import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -1265,6 +1270,64 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
                                .getResponseHeader(HDR_CONTENT_TYPE));
        }
 
+       @Test
+       public void testFetch_MaxHavesCutoffAfterAckOnly() throws Exception {
+               // Bootstrap by doing the clone.
+               //
+               TestRepository dst = createTestRepository();
+               try (Transport t = Transport.open(dst.getRepository(), remoteURI)) {
+                       t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+               }
+               assertEquals(B, dst.getRepository().exactRef(master).getObjectId());
+
+               // Force enough into the local client that enumeration will
+               // need more than MAX_HAVES (256) haves to be sent. The server
+               // doesn't know any of these, so it will never ACK. The client
+               // should keep going.
+               //
+               // If it does, client and server will find a common commit, and
+               // the pack file will contain exactly the one commit object Z
+               // we create on the remote, which we can test for via the progress
+               // monitor, which should have something like
+               // "Receiving objects: 100% (1/1)". If the client sends a "done"
+               // too early, the server will send more objects, and we'll have
+               // a line like "Receiving objects: 100% (8/8)".
+               TestRepository.BranchBuilder b = dst.branch(master);
+               // The client will send 32 + 64 + 128 + 256 + 512 haves. Only the
+               // last one will be a common commit. If the cutoff kicks in too
+               // early (after 480), we'll get too many objects in the fetch.
+               for (int i = 0; i < 992; i++)
+                       b.commit().tick(3600 /* 1 hour */).message("c" + i).create();
+
+               // Create a new commit on the remote.
+               //
+               RevCommit Z;
+               try (TestRepository<Repository> tr = new TestRepository<>(
+                               remoteRepository)) {
+                       b = tr.branch(master);
+                       Z = b.commit().message("Z").create();
+               }
+
+               // Now incrementally update.
+               //
+               ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+               Writer writer = new OutputStreamWriter(buffer, StandardCharsets.UTF_8);
+               TextProgressMonitor monitor = new TextProgressMonitor(writer);
+               try (Transport t = Transport.open(dst.getRepository(), remoteURI)) {
+                       t.fetch(monitor, mirror(master));
+               }
+               assertEquals(Z, dst.getRepository().exactRef(master).getObjectId());
+
+               String progressMessages = new String(buffer.toByteArray(),
+                               StandardCharsets.UTF_8);
+               Pattern expected = Pattern
+                               .compile("Receiving objects:\\s+100% \\(1/1\\)\n");
+               if (!expected.matcher(progressMessages).find()) {
+                       System.out.println(progressMessages);
+                       fail("Expected only one object to be sent");
+               }
+       }
+
        @Test
        public void testInitialClone_BrokenServer() throws Exception {
                try (Repository dst = createBareRepository();

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 fa0c0c6670cf6d9175de432fc425a446c46d788e..d344deac267ddc173352baff255d2047dba39472 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
@@ -503,12 +503,16 @@ public abstract class BasePackFetchConnection extends BasePackConnection
                }
                fetchState.havesTotal += n;
                if (n == 0
-                               || fetchState.havesWithoutAck > MAX_HAVES
+                               || (fetchState.hadAcks
+                                               && fetchState.havesWithoutAck > MAX_HAVES)
                                || fetchState.havesTotal > maxHaves) {
                        output.writeString("done\n"); //$NON-NLS-1$
                        output.end();
                        return true;
                }
+               // Increment only after the test above. Of course we have no ACKs yet
+               // for the newly added "have"s, so it makes no sense to count them
+               // against the MAX_HAVES limit.
                fetchState.havesWithoutAck += n;
                output.end();
                fetchState.incHavesToSend(statelessRPC);
@@ -555,6 +559,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection
                                        // markCommon appends the object to the "state"
                                        markCommon(walk.parseAny(returnedId), ack, true);
                                        fetchState.havesWithoutAck = 0;
+                                       fetchState.hadAcks = true;
                                } else if (ack == AckNackResult.ACK_READY) {
                                        gotReady = true;
                                }
@@ -1035,6 +1040,11 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
                long havesTotal;
 
+               // Set to true if we got at least one ACK in protocol V2.
+               boolean hadAcks;
+
+               // Counts haves without ACK. Use as cutoff for negotiation only once
+               // hadAcks == true.
                long havesWithoutAck;
 
                void incHavesToSend(boolean statelessRPC) {