From e7a6e85b95b2665b124f57181d8f9bc29100adce Mon Sep 17 00:00:00 2001
From: Shawn Pearce <spearce@spearce.org>
Date: Tue, 16 Feb 2016 15:09:05 -0800
Subject: [PATCH] smart HTTP server: Pass along "want X not valid" to client

If the client sends a SHA-1 that the server does not recognize echo
this back to the client with an explicit error message instead of
the generic "internal server error".

This was always the intent of the implementation but it was being
dropped on smart HTTP due to the UploadPackServlet catching the
PackProtocolException, discarding the buffered message UploadPack
meant to send, and sending along a generic message instead.

Change-Id: I8d96b064ec655aef64ac2ef3e01853625af32cd1
---
 .../http/test/SmartClientSmartServerTest.java | 22 ++++++++++++
 .../eclipse/jgit/transport/UploadPack.java    | 35 ++++++++-----------
 2 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
index 82861ed9b..0f3d3c6cf 100644
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
@@ -56,6 +56,7 @@ import static org.junit.Assert.fail;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
@@ -89,6 +90,8 @@ import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectIdRef;
+import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.ReflogEntry;
 import org.eclipse.jgit.lib.ReflogReader;
@@ -486,6 +489,25 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 				service.getResponseHeader(HDR_CONTENT_TYPE));
 	}
 
+	@Test
+	public void testInvalidWant() throws Exception {
+		@SuppressWarnings("resource")
+		ObjectId id = new ObjectInserter.Formatter().idFor(Constants.OBJ_BLOB,
+				"testInvalidWant".getBytes(StandardCharsets.UTF_8));
+
+		Repository dst = createBareRepository();
+		try (Transport t = Transport.open(dst, remoteURI);
+				FetchConnection c = t.openFetch()) {
+			Ref want = new ObjectIdRef.Unpeeled(Ref.Storage.NETWORK, id.name(),
+					id);
+			c.fetch(NullProgressMonitor.INSTANCE, Collections.singleton(want),
+					Collections.<ObjectId> emptySet());
+			fail("Server accepted want " + id.name());
+		} catch (TransportException err) {
+			assertEquals("want " + id.name() + " not valid", err.getMessage());
+		}
+	}
+
 	@Test
 	public void testPush_NotAuthorized() throws Exception {
 		final TestRepository src = createTestRepository();
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 101057fb4..8c1f99d22 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -742,10 +742,6 @@ public class UploadPack {
 			if (!clientShallowCommits.isEmpty())
 				walk.assumeShallow(clientShallowCommits);
 			sendPack = negotiate();
-		} catch (PackProtocolException err) {
-			reportErrorDuringNegotiate(err.getMessage());
-			throw err;
-
 		} catch (ServiceMayNotContinueException err) {
 			if (!err.isOutput() && err.getMessage() != null) {
 				try {
@@ -756,15 +752,20 @@ public class UploadPack {
 				}
 			}
 			throw err;
-
-		} catch (IOException err) {
-			reportErrorDuringNegotiate(JGitText.get().internalServerError);
-			throw err;
-		} catch (RuntimeException err) {
-			reportErrorDuringNegotiate(JGitText.get().internalServerError);
-			throw err;
-		} catch (Error err) {
-			reportErrorDuringNegotiate(JGitText.get().internalServerError);
+		} catch (IOException | RuntimeException | Error err) {
+			boolean output = false;
+			try {
+				String msg = err instanceof PackProtocolException
+						? err.getMessage()
+						: JGitText.get().internalServerError;
+				pckOut.writeString("ERR " + msg + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
+				output = true;
+			} catch (Throwable err2) {
+				// Ignore this secondary failure, leave output false.
+			}
+			if (output) {
+				throw new UploadPackInternalServerErrorException(err);
+			}
 			throw err;
 		}
 
@@ -781,14 +782,6 @@ public class UploadPack {
 		return ids;
 	}
 
-	private void reportErrorDuringNegotiate(String msg) {
-		try {
-			pckOut.writeString("ERR " + msg + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
-		} catch (Throwable err) {
-			// Ignore this secondary failure.
-		}
-	}
-
 	private void processShallow() throws IOException {
 		try (DepthWalk.RevWalk depthWalk = new DepthWalk.RevWalk(
 				walk.getObjectReader(), depth)) {