motiejus
/
jgit
1
Fork 0
Code Releases Activity

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>
This commit is contained in:
Thomas Wolf 2021-01-03 23:07:18 +01:00
parent d9143287b7
commit 0d7d98620f
2 changed files with 74 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
View File

@ -22,10 +22,14 @@
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.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 void testFetch_TooManyLocalCommits() throws Exception {
.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();

12
org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
View File

@ -503,12 +503,16 @@ private boolean sendNextHaveBatch(FetchStateV2 fetchState,
}
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 @@ private boolean readAcknowledgments(FetchStateV2 fetchState,
// 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 @@ private static class FetchStateV2 {
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) {