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>
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 4892dab..0ef29d4 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
@@ -10,7 +10,6 @@
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 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 void testV2LsRefsRefPrefixNoSlash() throws Exception {
@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 void testV2FetchRequestPolicyAdvertised() throws Exception {
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 void testV2FetchRequestPolicyReachableCommit() throws Exception {
"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 void testV2FetchRequestPolicyTip() throws Exception {
"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 void testV2FetchRequestPolicyReachableCommitTip() throws Exception {
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 void testV2FetchShallowSince_noCommitsSelected() throws Exception {
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 void testV2FetchDeepenNot_excludeDescendantOfWant()
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 void testV2FetchDeepenNot_excludedParentWithMultipleChildren() throws Exc
@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 void testV2FetchFilterWhenNotAllowed() throws Exception {
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 void testV2FetchWantRefIfNotAllowed() throws Exception {
RevCommit one = remote.commit().message("1").create();
remote.update("one", one);
- try {
- uploadPackV2(
- "command=fetch\n",
- PacketLineIn.delimiter(),
- "want-ref refs/heads/one\n",
- "done\n",
- PacketLineIn.end());
- } catch (PackProtocolException e) {
- assertThat(
- e.getMessage(),
+ UploadPackInternalServerErrorException e = assertThrows(
+ UploadPackInternalServerErrorException.class,
+ () -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
+ "want-ref refs/heads/one\n", "done\n",
+ PacketLineIn.end()));
+ assertThat(e.getCause().getMessage(),
containsString("unexpected want-ref refs/heads/one"));
- return;
- }
- fail("expected PackProtocolException");
}
@Test
@@ -1902,23 +1902,17 @@ public void testV2FetchBadWantRef() throws Exception {
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 {
- uploadPackV2(
- "command=fetch\n",
- PacketLineIn.delimiter(),
- "want-ref refs/heads/one\n",
- "want-ref refs/heads/nonExistentRef\n",
- "done\n",
- PacketLineIn.end());
- } catch (PackProtocolException e) {
- assertThat(
- e.getMessage(),
+ UploadPackInternalServerErrorException e = assertThrows(
+ UploadPackInternalServerErrorException.class,
+ () -> uploadPackV2("command=fetch\n", PacketLineIn.delimiter(),
+ "want-ref refs/heads/one\n",
+ "want-ref refs/heads/nonExistentRef\n", "done\n",
+ PacketLineIn.end()));
+ assertThat(e.getCause().getMessage(),
containsString("Invalid ref name: refs/heads/nonExistentRef"));
- return;
- }
- fail("expected PackProtocolException");
}
@Test
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 926c35a..52d2ed1 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -751,8 +751,8 @@ private boolean useProtocolV2() {
* other network connections this should be null.
* @throws java.io.IOException
*/
- public void upload(final InputStream input, OutputStream output,
- final OutputStream messages) throws IOException {
+ public void upload(InputStream input, OutputStream output,
+ @Nullable OutputStream messages) throws IOException {
try {
rawIn = input;
if (messages != null)
@@ -782,6 +782,36 @@ public void upload(final InputStream input, OutputStream output,
} 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 @@ else if (requestValidator instanceof AnyRequestValidator)
"\\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()) {
