From 63bd24cf356af4a354ef452b01411d1f2cbbec80 Mon Sep 17 00:00:00 2001
From: Masaya Suzuki <masayasuzuki@google.com>
Date: Tue, 23 Jul 2019 11:10:24 -0700
Subject: [PATCH] UploadPack: Introduce ErrorWriter

ErrorWriter writes an error message to the user. The implementation is
swapped once it detects that the client supports sideband. By default it
uses the protocol level ERR packet, which was introduced recently.

In total the error output is done in two different places;
UploadPack#upload and UploadPack#sendPack. These will be consolidated in
the next change.

Change-Id: Ia8d72e31170bbeafc8ffa8ddb92702196af8a587
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 .../eclipse/jgit/transport/UploadPack.java    | 52 +++++++++++++------
 1 file changed, 35 insertions(+), 17 deletions(-)

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 20b45882d..b664cfe7b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -44,6 +44,7 @@
 package org.eclipse.jgit.transport;
 
 import static java.util.Collections.unmodifiableMap;
+import static java.util.Objects.requireNonNull;
 import static org.eclipse.jgit.lib.Constants.R_TAGS;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REF_IN_WANT;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_SERVER_OPTION;
@@ -282,6 +283,8 @@ private static interface IOConsumer<R> {
 
 	private OutputStream msgOut = NullOutputStream.INSTANCE;
 
+	private ErrorWriter errOut = new PackProtocolErrorWriter();
+
 	/**
 	 * Refs eligible for advertising to the client, set using
 	 * {@link #setAdvertisedRefs}.
@@ -812,9 +815,9 @@ public void upload(InputStream input, OutputStream output,
 			// nothing.
 			throw err;
 		} catch (ServiceMayNotContinueException err) {
-			if (!err.isOutput() && err.getMessage() != null && pckOut != null) {
+			if (!err.isOutput() && err.getMessage() != null) {
 				try {
-					pckOut.writeString("ERR " + err.getMessage() + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
+					errOut.writeError(err.getMessage());
 				} catch (IOException e) {
 					err.addSuppressed(e);
 					throw err;
@@ -823,12 +826,12 @@ public void upload(InputStream input, OutputStream output,
 			}
 			throw err;
 		} catch (IOException | RuntimeException | Error err) {
-			if (pckOut != null) {
+			if (rawOut != null) {
 				String msg = err instanceof PackProtocolException
 						? err.getMessage()
 						: JGitText.get().internalServerError;
 				try {
-					pckOut.writeString("ERR " + msg + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
+					errOut.writeError(msg);
 				} catch (IOException e) {
 					err.addSuppressed(e);
 					throw err;
@@ -2115,6 +2118,8 @@ private void sendPack(PackStatistics.Accumulator accumulator,
 		boolean sideband = caps.contains(OPTION_SIDE_BAND)
 				|| caps.contains(OPTION_SIDE_BAND_64K);
 		if (sideband) {
+			errOut = new SideBandErrorWriter();
+
 			try {
 				sendPack(true, req, accumulator, allTags, unshallowCommits,
 						deepenNots, pckOut);
@@ -2124,7 +2129,7 @@ private void sendPack(PackStatistics.Accumulator accumulator,
 					message = JGitText.get().internalServerError;
 				}
 				try {
-					reportInternalServerErrorOverSideband(message);
+					errOut.writeError(message);
 				} catch (IOException e) {
 					err.addSuppressed(e);
 					throw err;
@@ -2132,8 +2137,7 @@ private void sendPack(PackStatistics.Accumulator accumulator,
 				throw new UploadPackInternalServerErrorException(err);
 			} catch (IOException | RuntimeException | Error err) {
 				try {
-					reportInternalServerErrorOverSideband(
-							JGitText.get().internalServerError);
+					errOut.writeError(JGitText.get().internalServerError);
 				} catch (IOException e) {
 					err.addSuppressed(e);
 					throw err;
@@ -2146,16 +2150,6 @@ private void sendPack(PackStatistics.Accumulator accumulator,
 		}
 	}
 
-	private void reportInternalServerErrorOverSideband(String message)
-			throws IOException {
-		@SuppressWarnings("resource" /* java 7 */)
-		SideBandOutputStream err = new SideBandOutputStream(
-				SideBandOutputStream.CH_ERROR, SideBandOutputStream.SMALL_BUF,
-				rawOut);
-		err.write(Constants.encode(message));
-		err.flush();
-	}
-
 	/**
 	 * Send the requested objects to the client.
 	 *
@@ -2411,4 +2405,28 @@ void stopBuffering() throws IOException {
 			}
 		}
 	}
+
+	private interface ErrorWriter {
+		void writeError(String message) throws IOException;
+	}
+
+	private class SideBandErrorWriter implements ErrorWriter {
+		@Override
+		public void writeError(String message) throws IOException {
+			@SuppressWarnings("resource" /* java 7 */)
+			SideBandOutputStream err = new SideBandOutputStream(
+					SideBandOutputStream.CH_ERROR,
+					SideBandOutputStream.SMALL_BUF, requireNonNull(rawOut));
+			err.write(Constants.encode(message));
+			err.flush();
+		}
+	}
+
+	private class PackProtocolErrorWriter implements ErrorWriter {
+		@Override
+		public void writeError(String message) throws IOException {
+			new PacketLineOut(requireNonNull(rawOut))
+					.writeString("ERR " + message + '\n'); //$NON-NLS-1$
+		}
+	}
 }