From 64d0aaa2b624e9c011390d817a6d1fef335bf7b6 Mon Sep 17 00:00:00 2001 From: Ronald Bhuleskar Date: Wed, 9 Jun 2021 15:41:09 -0700 Subject: [PATCH] Teach independent negotiation (no pack file) using an option "wait-for-done" From Git commit 9c1e657a8f: Currently, the packfile negotiation step within a Git fetch cannot be done independent of sending the packfile, even though there is at least one application wherein this is useful - push negotiation. Therefore, make it possible for this negotiation step to be done independently. This feature is for protocol v2 only. In the protocol, the main hindrance towards independent negotiation is that the server can unilaterally decide to send the packfile. This is solved by a "wait-for-done" argument: the server will then wait for the client to say "done". In practice, the client will never say it; instead it will cease requests once it is satisfied. Advertising the server capability option "wait-for-done" is behind the transport config: uploadpack.advertisewaitfordone, which by default is false. Change-Id: I5ebd3e99ad76b8943597216e23ced2ed38eb5224 --- .../jgit/transport/UploadPackTest.java | 102 ++++++++++++++++++ .../jgit/transport/FetchV2Request.java | 29 ++++- .../jgit/transport/GitProtocolConstants.java | 7 ++ .../jgit/transport/ProtocolV2Parser.java | 3 + .../jgit/transport/TransferConfig.java | 13 +++ .../eclipse/jgit/transport/UploadPack.java | 52 ++++++--- 6 files changed, 189 insertions(+), 17 deletions(-) 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 5045e9464..b0b5f68ef 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 @@ -998,6 +998,108 @@ public void testV2FetchClientStopsNegotiation() throws Exception { assertTrue(client.getObjectDatabase().has(barChild.toObjectId())); } + @Test + public void testV2FetchWithoutWaitForDoneReceivesPackfile() + throws Exception { + String commonInBlob = "abcdefghijklmnopqrstuvwxyz"; + + RevBlob parentBlob = remote.blob(commonInBlob + "a"); + RevCommit parent = remote + .commit(remote.tree(remote.file("foo", parentBlob))); + remote.update("branch1", parent); + + RevCommit localParent = null; + RevCommit localChild = null; + try (TestRepository local = new TestRepository<>( + client)) { + RevBlob localParentBlob = local.blob(commonInBlob + "a"); + localParent = local + .commit(local.tree(local.file("foo", localParentBlob))); + RevBlob localChildBlob = local.blob(commonInBlob + "b"); + localChild = local.commit( + local.tree(local.file("foo", localChildBlob)), localParent); + local.update("branch1", localChild); + } + + ByteArrayInputStream recvStream = uploadPackV2("command=fetch\n", + PacketLineIn.delimiter(), + "have " + localParent.toObjectId().getName() + "\n", + "have " + localChild.toObjectId().getName() + "\n", + PacketLineIn.end()); + PacketLineIn pckIn = new PacketLineIn(recvStream); + assertThat(pckIn.readString(), is("acknowledgments")); + assertThat(Arrays.asList(pckIn.readString()), + hasItems("ACK " + parent.toObjectId().getName())); + assertThat(pckIn.readString(), is("ready")); + assertTrue(PacketLineIn.isDelimiter(pckIn.readString())); + assertThat(pckIn.readString(), is("packfile")); + parsePack(recvStream); + } + + @Test + public void testV2FetchWithWaitForDoneOnlyDoesNegotiation() + throws Exception { + String commonInBlob = "abcdefghijklmnopqrstuvwxyz"; + + RevBlob parentBlob = remote.blob(commonInBlob + "a"); + RevCommit parent = remote + .commit(remote.tree(remote.file("foo", parentBlob))); + remote.update("branch1", parent); + + RevCommit localParent = null; + RevCommit localChild = null; + try (TestRepository local = new TestRepository<>( + client)) { + RevBlob localParentBlob = local.blob(commonInBlob + "a"); + localParent = local + .commit(local.tree(local.file("foo", localParentBlob))); + RevBlob localChildBlob = local.blob(commonInBlob + "b"); + localChild = local.commit( + local.tree(local.file("foo", localChildBlob)), localParent); + local.update("branch1", localChild); + } + + ByteArrayInputStream recvStream = uploadPackV2("command=fetch\n", + PacketLineIn.delimiter(), "wait-for-done\n", + "have " + localParent.toObjectId().getName() + "\n", + "have " + localChild.toObjectId().getName() + "\n", + PacketLineIn.end()); + PacketLineIn pckIn = new PacketLineIn(recvStream); + assertThat(pckIn.readString(), is("acknowledgments")); + assertThat(Arrays.asList(pckIn.readString()), + hasItems("ACK " + parent.toObjectId().getName())); + assertTrue(PacketLineIn.isEnd(pckIn.readString())); + } + + @Test + public void testV2FetchWithWaitForDoneOnlyDoesNegotiationAndNothingToAck() + throws Exception { + String commonInBlob = "abcdefghijklmnopqrstuvwxyz"; + + RevCommit localParent = null; + RevCommit localChild = null; + try (TestRepository local = new TestRepository<>( + client)) { + RevBlob localParentBlob = local.blob(commonInBlob + "a"); + localParent = local + .commit(local.tree(local.file("foo", localParentBlob))); + RevBlob localChildBlob = local.blob(commonInBlob + "b"); + localChild = local.commit( + local.tree(local.file("foo", localChildBlob)), localParent); + local.update("branch1", localChild); + } + + ByteArrayInputStream recvStream = uploadPackV2("command=fetch\n", + PacketLineIn.delimiter(), "wait-for-done\n", + "have " + localParent.toObjectId().getName() + "\n", + "have " + localChild.toObjectId().getName() + "\n", + PacketLineIn.end()); + PacketLineIn pckIn = new PacketLineIn(recvStream); + assertThat(pckIn.readString(), is("acknowledgments")); + assertThat(pckIn.readString(), is("NAK")); + assertTrue(PacketLineIn.isEnd(pckIn.readString())); + } + @Test public void testV2FetchThinPack() throws Exception { String commonInBlob = "abcdefghijklmnopqrstuvwxyz"; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java index ea639332e..50fb9d226 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java @@ -36,6 +36,8 @@ public final class FetchV2Request extends FetchRequest { private final boolean doneReceived; + private final boolean waitForDone; + @NonNull private final List serverOptions; @@ -50,7 +52,8 @@ public final class FetchV2Request extends FetchRequest { @NonNull Set clientShallowCommits, int deepenSince, @NonNull List deepenNotRefs, int depth, @NonNull FilterSpec filterSpec, - boolean doneReceived, @NonNull Set clientCapabilities, + boolean doneReceived, boolean waitForDone, + @NonNull Set clientCapabilities, @Nullable String agent, @NonNull List serverOptions, boolean sidebandAll, @NonNull List packfileUriProtocols) { super(wantIds, depth, clientShallowCommits, filterSpec, @@ -59,6 +62,7 @@ public final class FetchV2Request extends FetchRequest { this.peerHas = requireNonNull(peerHas); this.wantedRefs = requireNonNull(wantedRefs); this.doneReceived = doneReceived; + this.waitForDone = waitForDone; this.serverOptions = requireNonNull(serverOptions); this.sidebandAll = sidebandAll; this.packfileUriProtocols = packfileUriProtocols; @@ -90,7 +94,14 @@ boolean wasDoneReceived() { } /** - * Options received in server-option lines. The caller can choose to act on + * @return true if the request had a "wait-for-done" line + */ + boolean wasWaitForDoneReceived() { + return waitForDone; + } + + /** + * Options received in server-option lines. The caller can choose to act on * these in an application-specific way * * @return Immutable list of server options received in the request @@ -141,6 +152,8 @@ static final class Builder { boolean doneReceived; + boolean waitForDone; + @Nullable String agent; @@ -279,6 +292,16 @@ Builder setDoneReceived() { return this; } + /** + * Mark that the "wait-for-done" line has been received. + * + * @return this builder + */ + Builder setWaitForDone() { + waitForDone = true; + return this; + } + /** * Value of an agent line received after the command and before the * arguments. E.g. "agent=a.b.c/1.0" should set "a.b.c/1.0". @@ -328,7 +351,7 @@ Builder addPackfileUriProtocol(@NonNull String value) { FetchV2Request build() { return new FetchV2Request(peerHas, wantedRefs, wantIds, clientShallowCommits, deepenSince, deepenNotRefs, - depth, filterSpec, doneReceived, clientCapabilities, + depth, filterSpec, doneReceived, waitForDone, clientCapabilities, agent, Collections.unmodifiableList(serverOptions), sidebandAll, Collections.unmodifiableList(packfileUriProtocols)); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java index 36fce7a3f..048097163 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java @@ -148,6 +148,13 @@ public final class GitProtocolConstants { */ public static final String OPTION_SIDEBAND_ALL = "sideband-all"; //$NON-NLS-1$ + /** + * The server waits for client to send "done" before sending any packs back. + * + * @since 5.12 + */ + public static final String OPTION_WAIT_FOR_DONE = "wait-for-done"; //$NON-NLS-1$ + /** * The client supports atomic pushes. If this option is used, the server * will update all refs within one atomic transaction. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java index faccc2518..92f0133f5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java @@ -19,6 +19,7 @@ import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDEBAND_ALL; import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDE_BAND_64K; import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_THIN_PACK; +import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_WAIT_FOR_DONE; import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_WANT_REF; import java.io.IOException; @@ -123,6 +124,8 @@ FetchV2Request parseFetchRequest(PacketLineIn pckIn) reqBuilder.addPeerHas(ObjectId.fromString(line2.substring(5))); } else if (line2.equals("done")) { //$NON-NLS-1$ reqBuilder.setDoneReceived(); + } else if (line2.equals(OPTION_WAIT_FOR_DONE)) { + reqBuilder.setWaitForDone(); } else if (line2.equals(OPTION_THIN_PACK)) { reqBuilder.addClientCapability(OPTION_THIN_PACK); } else if (line2.equals(OPTION_NO_PROGRESS)) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java index 83ffd4123..c1d630c5c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java @@ -120,7 +120,10 @@ static ProtocolVersion parse(@Nullable String name) { private final boolean allowReachableSha1InWant; private final boolean allowFilter; private final boolean allowSidebandAll; + private final boolean advertiseSidebandAll; + private final boolean advertiseWaitForDone; + final @Nullable ProtocolVersion protocolVersion; final String[] hideRefs; @@ -206,6 +209,8 @@ public TransferConfig(Config rc) { "uploadpack", "allowsidebandall", false); advertiseSidebandAll = rc.getBoolean("uploadpack", "advertisesidebandall", false); + advertiseWaitForDone = rc.getBoolean("uploadpack", + "advertisewaitfordone", false); } /** @@ -304,6 +309,14 @@ public boolean isAdvertiseSidebandAll() { return advertiseSidebandAll && allowSidebandAll; } + /** + * @return true to advertise wait-for-done all to the clients + * @since 5.12 + */ + public boolean isAdvertiseWaitForDone() { + return advertiseWaitForDone; + } + /** * Get {@link org.eclipse.jgit.transport.RefFilter} respecting configured * hidden refs. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 7f1ddaab2..ecf175193 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -33,6 +33,7 @@ import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDE_BAND; import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDE_BAND_64K; import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_THIN_PACK; +import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_WAIT_FOR_DONE; import static org.eclipse.jgit.transport.GitProtocolConstants.VERSION_2_REQUEST; import static org.eclipse.jgit.util.RefMap.toRefMap; @@ -1192,9 +1193,10 @@ private void fetchV2(PacketLineOut pckOut) throws IOException { walk.assumeShallow(req.getClientShallowCommits()); if (req.wasDoneReceived()) { - processHaveLines(req.getPeerHas(), ObjectId.zeroId(), + processHaveLines( + req.getPeerHas(), ObjectId.zeroId(), new PacketLineOut(NullOutputStream.INSTANCE, false), - accumulator); + accumulator, req.wasWaitForDoneReceived() ? Option.WAIT_FOR_DONE : Option.NONE); } else { pckOut.writeString( GitProtocolConstants.SECTION_ACKNOWLEDGMENTS + '\n'); @@ -1205,8 +1207,8 @@ private void fetchV2(PacketLineOut pckOut) throws IOException { } processHaveLines(req.getPeerHas(), ObjectId.zeroId(), new PacketLineOut(NullOutputStream.INSTANCE, false), - accumulator); - if (okToGiveUp()) { + accumulator, Option.NONE); + if (!req.wasWaitForDoneReceived() && okToGiveUp()) { pckOut.writeString("ready\n"); //$NON-NLS-1$ } else if (commonBase.isEmpty()) { pckOut.writeString("NAK\n"); //$NON-NLS-1$ @@ -1214,7 +1216,7 @@ private void fetchV2(PacketLineOut pckOut) throws IOException { sectionSent = true; } - if (req.wasDoneReceived() || okToGiveUp()) { + if (req.wasDoneReceived() || (!req.wasWaitForDoneReceived() && okToGiveUp())) { if (mayHaveShallow) { if (sectionSent) pckOut.writeDelim(); @@ -1312,6 +1314,9 @@ private List getV2CapabilityAdvertisement() { ? OPTION_SIDEBAND_ALL + ' ' : "") + (cachedPackUriProvider != null ? "packfile-uris " : "") + + (transferConfig.isAdvertiseWaitForDone() + ? OPTION_WAIT_FOR_DONE + ' ' + : "") + OPTION_SHALLOW); caps.add(CAPABILITY_SERVER_OPTION); return caps; @@ -1656,7 +1661,7 @@ private boolean negotiate(FetchRequest req, } if (PacketLineIn.isEnd(line)) { - last = processHaveLines(peerHas, last, pckOut, accumulator); + last = processHaveLines(peerHas, last, pckOut, accumulator, Option.NONE); if (commonBase.isEmpty() || multiAck != MultiAck.OFF) pckOut.writeString("NAK\n"); //$NON-NLS-1$ if (noDone && sentReady) { @@ -1671,7 +1676,7 @@ private boolean negotiate(FetchRequest req, peerHas.add(ObjectId.fromString(line.substring(5))); accumulator.haves++; } else if (line.equals("done")) { //$NON-NLS-1$ - last = processHaveLines(peerHas, last, pckOut, accumulator); + last = processHaveLines(peerHas, last, pckOut, accumulator, Option.NONE); if (commonBase.isEmpty()) pckOut.writeString("NAK\n"); //$NON-NLS-1$ @@ -1687,8 +1692,14 @@ else if (multiAck != MultiAck.OFF) } } + private enum Option { + WAIT_FOR_DONE, + NONE; + } + private ObjectId processHaveLines(List peerHas, ObjectId last, - PacketLineOut out, PackStatistics.Accumulator accumulator) + PacketLineOut out, PackStatistics.Accumulator accumulator, + Option option) throws IOException { preUploadHook.onBeginNegotiateRound(this, wantIds, peerHas.size()); if (wantAll.isEmpty() && !wantIds.isEmpty()) @@ -1754,6 +1765,18 @@ private ObjectId processHaveLines(List peerHas, ObjectId last, // create a pack at this point, let the client know so it stops // telling us about its history. // + if (option != Option.WAIT_FOR_DONE) { + sentReady = shouldGiveUp(peerHas, out, missCnt); + } + + preUploadHook.onEndNegotiateRound(this, wantAll, haveCnt, missCnt, sentReady); + peerHas.clear(); + return last; + } + + private boolean shouldGiveUp(List peerHas, PacketLineOut out, int missCnt) + throws IOException { + boolean sentReady = false; boolean didOkToGiveUp = false; if (0 < missCnt) { for (int i = peerHas.size() - 1; i >= 0; i--) { @@ -1765,10 +1788,12 @@ private ObjectId processHaveLines(List peerHas, ObjectId last, case OFF: break; case CONTINUE: - out.writeString("ACK " + id.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$ + out.writeString( + "ACK " + id.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$ break; case DETAILED: - out.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$ + out.writeString( + "ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$ sentReady = true; break; } @@ -1778,15 +1803,14 @@ private ObjectId processHaveLines(List peerHas, ObjectId last, } } - if (multiAck == MultiAck.DETAILED && !didOkToGiveUp && okToGiveUp()) { + if (multiAck == MultiAck.DETAILED && !didOkToGiveUp + && okToGiveUp()) { ObjectId id = peerHas.get(peerHas.size() - 1); out.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$ sentReady = true; } - preUploadHook.onEndNegotiateRound(this, wantAll, haveCnt, missCnt, sentReady); - peerHas.clear(); - return last; + return sentReady; } private void parseWants(PackStatistics.Accumulator accumulator) throws IOException {