From 9110037e3e9461ff4dac22fee84ef3694ed57648 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Wed, 3 Apr 2019 10:52:48 -0700 Subject: [PATCH] UploadPack: move writing of "packfile" header In a subsequent patch, in some cases, PackWriter#writePack will be responsible for both the "packfile-uris" and "packfile" sections, meaning that (in these cases) it must write the "packfile" section header itself. In preparation for that patch, move the writing of the "packfile" section header closer to the invocation of PackWriter#writePack when the entire fetch response is configured to use the sideband. This means that "packfile" is written *after* objects are counted (and progress messages sent to the client in sideband 2) when the "sideband-all" feature is used (whether "packfile-uris" is used or not), and written *before* objects are counted otherwise. Having code to write "packfile" in two places is unfortunate but necessary. When "sideband-all" is not used, object counting has to happen after "packfile" is written, because "packfile" activates the sideband that allows counting progress to be transmitted. When "packfile-uris" is used, object counting has to happen before "packfile" is written, because object counting determines whether to send "packfile-uris" or "packfile". When "sideband-all" is used but "packfile-uris" is not used, either way works; this commit uses "packfile-uris" behavior in this case. Also make the naming of the sideband-activating methods in PacketLineOut more consistent. Change-Id: Ifbfd26cc26af10c41b77758168833702d6983df1 Signed-off-by: Jonathan Tan --- .../jgit/transport/UploadPackTest.java | 9 ++++++- .../eclipse/jgit/transport/PacketLineOut.java | 26 ++++++++++++++----- .../eclipse/jgit/transport/UploadPack.java | 11 ++++++-- 3 files changed, 36 insertions(+), 10 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 65df6e3c4..22c67c101 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 @@ -2138,7 +2138,14 @@ public class UploadPackTest { PacketLineIn.END); PacketLineIn pckIn = new PacketLineIn(recvStream); - assertThat(pckIn.readString(), is("\001packfile")); + String s; + // When sideband-all is used, object counting happens before + // "packfile" is written, and object counting outputs progress + // in sideband 2. Skip all these lines. + for (s = pckIn.readString(); s.startsWith("\002"); s = pckIn.readString()) { + // do nothing + } + assertThat(s, is("\001packfile")); parsePack(recvStream); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java index b07b6f69d..7f837bb84 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java @@ -74,7 +74,7 @@ public class PacketLineOut { private boolean flushOnEnd; - private boolean useSideband; + private boolean usingSideband; /** * Create a new packet line writer. @@ -100,13 +100,25 @@ public class PacketLineOut { } /** - * When writing packet lines, use the first byte of each non-flush and - * non-delim packet as a sideband designator. - * + * @return whether to add a sideband designator to each non-flush and + * non-delim packet + * @see #setUsingSideband + * @since 5.5 + */ + public boolean isUsingSideband() { + return usingSideband; + } + + /** + * @param value If true, when writing packet lines, add, as the first + * byte, a sideband designator to each non-flush and non-delim + * packet. See pack-protocol.txt and protocol-v2.txt from the Git + * project for more information, specifically the "side-band" and + * "sideband-all" sections. * @since 5.5 */ - public void useSidebandFormat() { - this.useSideband = true; + public void setUsingSideband(boolean value) { + this.usingSideband = value; } /** @@ -151,7 +163,7 @@ public class PacketLineOut { * @since 4.5 */ public void writePacket(byte[] buf, int pos, int len) throws IOException { - if (useSideband) { + if (usingSideband) { formatLength(len + 5); out.write(lenbuffer, 0, 4); out.write(1); 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 c580ed008..2194f2f30 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -1136,7 +1136,7 @@ public class UploadPack { protocolV2Hook.onFetch(req); if (req.getSidebandAll()) { - pckOut.useSidebandFormat(); + pckOut.setUsingSideband(true); } // TODO(ifrade): Refactor to pass around the Request object, instead of @@ -1224,7 +1224,11 @@ public class UploadPack { if (sectionSent) pckOut.writeDelim(); - pckOut.writeString("packfile\n"); //$NON-NLS-1$ + if (!pckOut.isUsingSideband()) { + // sendPack will write "packfile\n" for us if sideband-all is used. + // But sideband-all is not used, so we have to write it ourselves. + pckOut.writeString("packfile\n"); //$NON-NLS-1$ + } sendPack(new PackStatistics.Accumulator(), req, req.getClientCapabilities().contains(OPTION_INCLUDE_TAG) @@ -2293,6 +2297,9 @@ public class UploadPack { } } + if (pckOut.isUsingSideband()) { + pckOut.writeString("packfile\n"); //$NON-NLS-1$ + } pw.writePack(pm, NullProgressMonitor.INSTANCE, packOut); if (msgOut != NullOutputStream.INSTANCE) {