HTTP Smart: set correct HTTP status on error

Previous behavior was that status code was automatically set to 200
regardless of reported status and according to HTTP Smart protocol[1]:

  If there is no repository at $GIT_URL, or the resource pointed to by
  a location matching $GIT_URL does not exist, the server MUST NOT
  respond with 200 OK response. A server SHOULD respond with
  404 Not Found, 410 Gone, or any other suitable HTTP status code which
  does not imply the resource exists as requested.

Since the jgit HTTP client isn't able to handle reading content from a
response reporting an error (calling HttpURLConnection#getInputStream
on a "failed" connection throws an exception and the internal interface
HttpConnection does not expose HttpURLConnection#getErrorStream) the
SmartClientSmartServerTest needed to be rewritten to expect the generic
response messages.

[1] https://git-scm.com/docs/http-protocol#_general_request_processing

Bug: 579676
Change-Id: Ibb942d02124a0bc279df09600b091354019ce064

2 files changed, 24 insertions, 9 deletions

diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java
index df093c185b..3438c52c8d 100644
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java
@@ -23,6 +23,7 @@ import java.io.File;
 import java.io.OutputStream;
 import java.net.URI;
 import java.net.URL;
+import java.text.MessageFormat;
 import java.util.List;
 
 import javax.servlet.http.HttpServletRequest;
@@ -308,7 +309,10 @@ public class HttpClientTests extends AllFactoriesHttpTestCase {
 				fail("connection opened even though service disabled");
 			} catch (TransportException err) {
 				String exp = smartAuthNoneURI + ": "
-						+ JGitText.get().serviceNotEnabledNoName;
+						+ MessageFormat.format(
+								JGitText.get().serviceNotPermitted,
+								smartAuthNoneURI.toString() + "/",
+								"git-upload-pack");
 				assertEquals(exp, err.getMessage());
 			}
 		}
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 887e970a0c..8f3888e4d5 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
@@ -57,7 +57,7 @@ import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.TransportConfigCallback;
-import org.eclipse.jgit.errors.RemoteRepositoryException;
+import org.eclipse.jgit.errors.NoRemoteRepositoryException;
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.errors.UnsupportedCredentialItem;
 import org.eclipse.jgit.http.server.GitServlet;
@@ -496,8 +496,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 			try {
 				t.openFetch();
 				fail("fetch connection opened");
-			} catch (RemoteRepositoryException notFound) {
-				assertEquals(uri + ": Git repository not found",
+			} catch (NoRemoteRepositoryException notFound) {
+				assertEquals(uri + ": " + uri
+						+ "/info/refs?service=git-upload-pack not found: Not Found",
 						notFound.getMessage());
 			}
 		}
@@ -510,7 +511,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 		assertEquals(join(uri, "info/refs"), info.getPath());
 		assertEquals(1, info.getParameters().size());
 		assertEquals("git-upload-pack", info.getParameter("service"));
-		assertEquals(200, info.getStatus());
+		assertEquals(404, info.getStatus());
 		assertEquals("application/x-git-upload-pack-advertisement",
 				info.getResponseHeader(HDR_CONTENT_TYPE));
 	}
@@ -536,8 +537,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 							Collections.singletonList(
 									new RefSpec(unreachableCommit.name()))));
 			assertTrue(e.getMessage().contains(
-					"want " + unreachableCommit.name() + " not valid"));
+					"Bad Request"));
 		}
+		assertLastRequestStatusCode(400);
 	}
 
 	@Test
@@ -558,8 +560,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 					() -> t.fetch(NullProgressMonitor.INSTANCE,
 							Collections.singletonList(new RefSpec(A.name()))));
 			assertTrue(
-					e.getMessage().contains("want " + A.name() + " not valid"));
+					e.getMessage().contains("Bad Request"));
 		}
+		assertLastRequestStatusCode(400);
 	}
 
 	@Test
@@ -916,6 +919,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 		} catch (TransportException e) {
 			assertTrue(e.getMessage().contains("301"));
 		}
+		assertLastRequestStatusCode(301);
 	}
 
 	@Test
@@ -934,6 +938,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 			assertTrue(
 					e.getMessage().contains("http.followRedirects is false"));
 		}
+		assertLastRequestStatusCode(301);
 	}
 
 	private void assertFetchRequests(List<AccessEvent> requests, int index) {
@@ -1605,8 +1610,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 			fail("Server accepted want " + id.name());
 		} catch (TransportException err) {
 			assertTrue(err.getMessage()
-					.contains("want " + id.name() + " not valid"));
+					.contains("Bad Request"));
 		}
+		assertLastRequestStatusCode(400);
 	}
 
 	@Test
@@ -1650,7 +1656,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 				fail("Successfully served ref with value " + c.getRef(master));
 			} catch (TransportException err) {
 				assertTrue("Unexpected exception message " + err.getMessage(),
-						err.getMessage().contains("Internal server error"));
+						err.getMessage().contains("Server Error"));
 			}
 		} finally {
 			noRefServer.tearDown();
@@ -1821,6 +1827,11 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 				.getResponseHeader(HDR_CONTENT_TYPE));
 	}
 
+	private void assertLastRequestStatusCode(int statusCode) {
+		List<AccessEvent> requests = getRequests();
+		assertEquals(statusCode, requests.get(requests.size() - 1).getStatus());
+	}
+
 	private void enableReceivePack() throws IOException {
 		final StoredConfig cfg = remoteRepository.getConfig();
 		cfg.setBoolean("http", null, "receivepack", true);