Protocol V2: don't log spurious ACKs in UploadPack

UploadPack may log ACKs in protocol V2 that it doesn't send (if it
got a "done" from the client), or may log ACKs twice. That makes
packet log analysis difficult.

Add a new constructor to PacketLineOut to omit all logging from an
instance, and use it in UploadPack.

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

2 files changed, 29 insertions, 6 deletions

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 9fe3f0defc..77f0a7a516 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineOut.java
@@ -33,12 +33,15 @@ import org.slf4j.LoggerFactory;
  * against the underlying OutputStream.
  */
 public class PacketLineOut {
+
 	private static final Logger log = LoggerFactory.getLogger(PacketLineOut.class);
 
 	private final OutputStream out;
 
 	private final byte[] lenbuffer;
 
+	private final boolean logEnabled;
+
 	private boolean flushOnEnd;
 
 	private boolean usingSideband;
@@ -50,9 +53,24 @@ public class PacketLineOut {
 	 *            stream.
 	 */
 	public PacketLineOut(OutputStream outputStream) {
+		this(outputStream, true);
+	}
+
+	/**
+	 * Create a new packet line writer that potentially doesn't log.
+	 *
+	 * @param outputStream
+	 *            stream.
+	 * @param enableLogging
+	 *            {@code false} to suppress all logging; {@code true} to log
+	 *            normally
+	 * @since 5.11
+	 */
+	public PacketLineOut(OutputStream outputStream, boolean enableLogging) {
 		out = outputStream;
 		lenbuffer = new byte[5];
 		flushOnEnd = true;
+		logEnabled = enableLogging;
 	}
 
 	/**
@@ -139,7 +157,7 @@ public class PacketLineOut {
 			out.write(lenbuffer, 0, 4);
 		}
 		out.write(buf, pos, len);
-		if (log.isDebugEnabled()) {
+		if (logEnabled && log.isDebugEnabled()) {
 			// Escape a trailing \n to avoid empty lines in the log.
 			if (len > 0 && buf[pos + len - 1] == '\n') {
 				log.debug(
@@ -162,7 +180,9 @@ public class PacketLineOut {
 	public void writeDelim() throws IOException {
 		formatLength(1);
 		out.write(lenbuffer, 0, 4);
-		log.debug("git> 0001"); //$NON-NLS-1$
+		if (logEnabled && log.isDebugEnabled()) {
+			log.debug("git> 0001"); //$NON-NLS-1$
+		}
 	}
 
 	/**
@@ -181,9 +201,12 @@ public class PacketLineOut {
 	public void end() throws IOException {
 		formatLength(0);
 		out.write(lenbuffer, 0, 4);
-		log.debug("git> 0000"); //$NON-NLS-1$
-		if (flushOnEnd)
+		if (logEnabled && log.isDebugEnabled()) {
+			log.debug("git> 0000"); //$NON-NLS-1$
+		}
+		if (flushOnEnd) {
 			flush();
+		}
 	}
 
 	/**
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 5205ca9a9a..e0b86f5c11 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -1193,7 +1193,7 @@ public class UploadPack {
 
 		if (req.wasDoneReceived()) {
 			processHaveLines(req.getPeerHas(), ObjectId.zeroId(),
-					new PacketLineOut(NullOutputStream.INSTANCE),
+					new PacketLineOut(NullOutputStream.INSTANCE, false),
 					accumulator);
 		} else {
 			pckOut.writeString(
@@ -1204,7 +1204,7 @@ public class UploadPack {
 				}
 			}
 			processHaveLines(req.getPeerHas(), ObjectId.zeroId(),
-					new PacketLineOut(NullOutputStream.INSTANCE),
+					new PacketLineOut(NullOutputStream.INSTANCE, false),
 					accumulator);
 			if (okToGiveUp()) {
 				pckOut.writeString("ready\n"); //$NON-NLS-1$