Move exception handling code to the caller

By doing this, exceptions thrown by sendPack are also covered by the
same code.

Change-Id: I3509f2d832af1410f307e931577e4d07e32b014e
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java

@@ -10,7 +10,6 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -28,7 +27,6 @@ import java.util.Map;
 import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.dircache.DirCacheBuilder;
 import org.eclipse.jgit.dircache.DirCacheEntry;
-import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.internal.storage.dfs.DfsGarbageCollector;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
@@ -734,11 +732,12 @@ public class UploadPackTest {
 
 	@Test
 	public void testV2LsRefsUnrecognizedArgument() throws Exception {
-		PackProtocolException e = assertThrows(PackProtocolException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2("command=ls-refs\n",
 						PacketLineIn.delimiter(), "invalid-argument\n",
 						PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("unexpected invalid-argument"));
 	}
 
@@ -791,12 +790,13 @@ public class UploadPackTest {
 				PacketLineIn.end());
 
 		// This doesn't
-		TransportException e = assertThrows(TransportException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2(RequestPolicy.ADVERTISED, null, null,
 						"command=fetch\n", PacketLineIn.delimiter(),
 						"want " + unadvertized.name() + "\n",
 						PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("want " + unadvertized.name() + " not valid"));
 	}
 
@@ -814,12 +814,13 @@ public class UploadPackTest {
 				"want " + reachable.name() + "\n", PacketLineIn.end());
 
 		// This doesn't
-		TransportException e = assertThrows(TransportException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2(RequestPolicy.REACHABLE_COMMIT, null, null,
 						"command=fetch\n", PacketLineIn.delimiter(),
 						"want " + unreachable.name() + "\n",
 						PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("want " + unreachable.name() + " not valid"));
 	}
 
@@ -836,12 +837,13 @@ public class UploadPackTest {
 				"want " + tip.name() + "\n", PacketLineIn.end());
 
 		// This doesn't
-		TransportException e = assertThrows(TransportException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2(RequestPolicy.TIP, new RejectAllRefFilter(),
 						null, "command=fetch\n", PacketLineIn.delimiter(),
 						"want " + parentOfTip.name() + "\n",
 						PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("want " + parentOfTip.name() + " not valid"));
 	}
 
@@ -860,13 +862,14 @@ public class UploadPackTest {
 				PacketLineIn.end());
 
 		// This doesn't
-		TransportException e = assertThrows(TransportException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2(RequestPolicy.REACHABLE_COMMIT_TIP,
 						new RejectAllRefFilter(), null, "command=fetch\n",
 						PacketLineIn.delimiter(),
 						"want " + unreachable.name() + "\n",
 						PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("want " + unreachable.name() + " not valid"));
 	}
 
@@ -1302,12 +1305,13 @@ public class UploadPackTest {
 
 		remote.update("branch1", tooOld);
 
-		PackProtocolException e = assertThrows(PackProtocolException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
 						"deepen-since 1510000\n",
 						"want " + tooOld.toObjectId().getName() + "\n",
 						"done\n", PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("No commits selected for shallow request"));
 	}
 
@@ -1377,12 +1381,13 @@ public class UploadPackTest {
 		remote.update("two", two);
 		remote.update("four", four);
 
-		PackProtocolException e = assertThrows(PackProtocolException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
 						"deepen-not four\n",
 						"want " + two.toObjectId().getName() + "\n", "done\n",
 						PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("No commits selected for shallow request"));
 	}
 
@@ -1461,10 +1466,11 @@ public class UploadPackTest {
 
 	@Test
 	public void testV2FetchUnrecognizedArgument() throws Exception {
-		PackProtocolException e = assertThrows(PackProtocolException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
 						"invalid-argument\n", PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("unexpected invalid-argument"));
 	}
 
@@ -1834,11 +1840,12 @@ public class UploadPackTest {
 
 		server.getConfig().setBoolean("uploadpack", null, "allowfilter", false);
 
-		PackProtocolException e = assertThrows(PackProtocolException.class,
+		UploadPackInternalServerErrorException e = assertThrows(
+				UploadPackInternalServerErrorException.class,
 				() -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
 						"want " + commit.toObjectId().getName() + "\n",
 						"filter blob:limit=5\n", "done\n", PacketLineIn.end()));
-		assertThat(e.getMessage(),
+		assertThat(e.getCause().getMessage(),
 				containsString("unexpected filter blob:limit=5"));
 	}
 
@@ -1847,20 +1854,13 @@ public class UploadPackTest {
 		RevCommit one = remote.commit().message("1").create();
 		remote.update("one", one);
 
-		try {
+		UploadPackInternalServerErrorException e = assertThrows(
-			uploadPackV2(
+				UploadPackInternalServerErrorException.class,
-				"command=fetch\n",
+				() -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
-				PacketLineIn.delimiter(),
+						"want-ref refs/heads/one\n", "done\n",
-				"want-ref refs/heads/one\n",
+						PacketLineIn.end()));
-				"done\n",
+		assertThat(e.getCause().getMessage(),
-					PacketLineIn.end());
-		} catch (PackProtocolException e) {
-			assertThat(
-				e.getMessage(),
 				containsString("unexpected want-ref refs/heads/one"));
-			return;
-		}
-		fail("expected PackProtocolException");
 	}
 
 	@Test
@@ -1902,23 +1902,17 @@ public class UploadPackTest {
 		RevCommit one = remote.commit().message("1").create();
 		remote.update("one", one);
 
-		server.getConfig().setBoolean("uploadpack", null, "allowrefinwant", true);
+		server.getConfig().setBoolean("uploadpack", null, "allowrefinwant",
+				true);
 
-		try {
+		UploadPackInternalServerErrorException e = assertThrows(
-			uploadPackV2(
+				UploadPackInternalServerErrorException.class,
-				"command=fetch\n",
+				() -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
-				PacketLineIn.delimiter(),
+						"want-ref refs/heads/one\n",
-				"want-ref refs/heads/one\n",
+						"want-ref refs/heads/nonExistentRef\n", "done\n",
-				"want-ref refs/heads/nonExistentRef\n",
+						PacketLineIn.end()));
-				"done\n",
+		assertThat(e.getCause().getMessage(),
-					PacketLineIn.end());
-		} catch (PackProtocolException e) {
-			assertThat(
-				e.getMessage(),
 				containsString("Invalid ref name: refs/heads/nonExistentRef"));
-			return;
-		}
-		fail("expected PackProtocolException");
 	}
 
 	@Test

org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java

@@ -751,8 +751,8 @@ public class UploadPack {
 	 *            other network connections this should be null.
 	 * @throws java.io.IOException
 	 */
-	public void upload(final InputStream input, OutputStream output,
+	public void upload(InputStream input, OutputStream output,
-			final OutputStream messages) throws IOException {
+			@Nullable OutputStream messages) throws IOException {
 		try {
 			rawIn = input;
 			if (messages != null)
@@ -782,6 +782,36 @@ public class UploadPack {
 			} else {
 				service();
 			}
+		} catch (UploadPackInternalServerErrorException err) {
+			// UploadPackInternalServerErrorException is a special exception
+			// that indicates an error is already written to the client. Do
+			// nothing.
+			throw err;
+		} catch (ServiceMayNotContinueException err) {
+			if (!err.isOutput() && err.getMessage() != null && pckOut != null) {
+				try {
+					pckOut.writeString("ERR " + err.getMessage() + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
+				} catch (IOException e) {
+					err.addSuppressed(e);
+					throw err;
+				}
+				err.setOutput();
+			}
+			throw err;
+		} catch (IOException | RuntimeException | Error err) {
+			if (pckOut != null) {
+				String msg = err instanceof PackProtocolException
+						? err.getMessage()
+						: JGitText.get().internalServerError;
+				try {
+					pckOut.writeString("ERR " + msg + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
+				} catch (IOException e) {
+					err.addSuppressed(e);
+					throw err;
+				}
+				throw new UploadPackInternalServerErrorException(err);
+			}
+			throw err;
 		} finally {
 			msgOut = NullOutputStream.INSTANCE;
 			walk.close();
@@ -1013,27 +1043,6 @@ public class UploadPack {
 							"\\x" + Integer.toHexString(eof))); //$NON-NLS-1$
 				}
 			}
-		} catch (ServiceMayNotContinueException err) {
-			if (!err.isOutput() && err.getMessage() != null) {
-				try {
-					pckOut.writeString("ERR " + err.getMessage() + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
-				} catch (IOException e) {
-					err.addSuppressed(e);
-					throw err;
-				}
-				err.setOutput();
-			}
-			throw err;
-		} catch (IOException | RuntimeException | Error err) {
-			String msg = err instanceof PackProtocolException ? err.getMessage()
-					: JGitText.get().internalServerError;
-			try {
-				pckOut.writeString("ERR " + msg + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
-			} catch (IOException e) {
-				err.addSuppressed(e);
-				throw err;
-			}
-			throw new UploadPackInternalServerErrorException(err);
 		} finally {
 			if (!sendPack && !biDirectionalPipe) {
 				while (0 < rawIn.skip(2048) || 0 <= rawIn.read()) {