UploadPackTest: refactor capability config test

UploadPackTest.java contains tests that check behavior when
"allowfilter" and "allowrefinwant" are not set, are set, and are not set
but the client insists on using them anyway. Because another capability
is to be included in a subsequent patch, refactor the common code in
these tests.

Remove setBoolean calls with "false", as they are no-ops.

Also take the opportunity to eliminate the overspecification of the
"fetch=" line returned by the capability advertisement.

Change-Id: I289bbd11c902a513cd8d53bc34767e61ebbd5f17
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

1 files changed, 59 insertions, 48 deletions

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 0ef29d4862..a80990d6d1 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
@@ -1,6 +1,7 @@
 package org.eclipse.jgit.transport;
 
 import static org.eclipse.jgit.lib.MoreAsserts.assertThrows;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.is;
@@ -531,54 +532,66 @@ public class UploadPackTest {
 		assertTrue(PacketLineIn.isEnd(pckIn.readString()));
 	}
 
-	@Test
-	public void testV2CapabilitiesAllowFilter() throws Exception {
-		server.getConfig().setBoolean("uploadpack", null, "allowfilter", true);
+	private void checkAdvertisedIfAllowed(String configSection, String configName,
+			String fetchCapability) throws Exception {
+		server.getConfig().setBoolean(configSection, null, configName, true);
 		ByteArrayInputStream recvStream =
 				uploadPackV2Setup(null, null, null, PacketLineIn.end());
 		PacketLineIn pckIn = new PacketLineIn(recvStream);
 
 		assertThat(pckIn.readString(), is("version 2"));
-		assertThat(
-				Arrays.asList(pckIn.readString(), pckIn.readString(),
-						pckIn.readString()),
-				// TODO(jonathantanmy) This check overspecifies the
-				// order of the capabilities of "fetch".
-				hasItems("ls-refs", "fetch=filter shallow", "server-option"));
-		assertTrue(PacketLineIn.isEnd(pckIn.readString()));
+
+		ArrayList<String> lines = new ArrayList<>();
+		String line;
+		while (!PacketLineIn.isEnd((line = pckIn.readString()))) {
+			if (line.startsWith("fetch=")) {
+				assertThat(
+					Arrays.asList(line.substring(6).split(" ")),
+					containsInAnyOrder(fetchCapability, "shallow"));
+				lines.add("fetch");
+			} else {
+				lines.add(line);
+			}
+		}
+		assertThat(lines, containsInAnyOrder("ls-refs", "fetch", "server-option"));
 	}
 
-	@Test
-	public void testV2CapabilitiesRefInWant() throws Exception {
-		server.getConfig().setBoolean("uploadpack", null, "allowrefinwant", true);
+	private void checkUnadvertisedIfUnallowed(String fetchCapability) throws Exception {
 		ByteArrayInputStream recvStream =
 				uploadPackV2Setup(null, null, null, PacketLineIn.end());
 		PacketLineIn pckIn = new PacketLineIn(recvStream);
 
 		assertThat(pckIn.readString(), is("version 2"));
-		assertThat(
-				Arrays.asList(pckIn.readString(), pckIn.readString(),
-						pckIn.readString()),
-				// TODO(jonathantanmy) This check overspecifies the
-				// order of the capabilities of "fetch".
-				hasItems("ls-refs", "fetch=ref-in-want shallow",
-						"server-option"));
-		assertTrue(PacketLineIn.isEnd(pckIn.readString()));
+
+		ArrayList<String> lines = new ArrayList<>();
+		String line;
+		while (!PacketLineIn.isEnd((line = pckIn.readString()))) {
+			if (line.startsWith("fetch=")) {
+				assertThat(
+					Arrays.asList(line.substring(6).split(" ")),
+					hasItems("shallow"));
+				lines.add("fetch");
+			} else {
+				lines.add(line);
+			}
+		}
+		assertThat(lines, hasItems("ls-refs", "fetch", "server-option"));
 	}
 
 	@Test
-	public void testV2CapabilitiesRefInWantNotAdvertisedIfUnallowed() throws Exception {
-		server.getConfig().setBoolean("uploadpack", null, "allowrefinwant", false);
-		ByteArrayInputStream recvStream =
-				uploadPackV2Setup(null, null, null, PacketLineIn.end());
-		PacketLineIn pckIn = new PacketLineIn(recvStream);
+	public void testV2CapabilitiesAllowFilter() throws Exception {
+		checkAdvertisedIfAllowed("uploadpack", "allowfilter", "filter");
+		checkUnadvertisedIfUnallowed("filter");
+	}
 
-		assertThat(pckIn.readString(), is("version 2"));
-		assertThat(
-				Arrays.asList(pckIn.readString(), pckIn.readString(),
-						pckIn.readString()),
-				hasItems("ls-refs", "fetch=shallow", "server-option"));
-		assertTrue(PacketLineIn.isEnd(pckIn.readString()));
+	@Test
+	public void testV2CapabilitiesRefInWant() throws Exception {
+		checkAdvertisedIfAllowed("uploadpack", "allowrefinwant", "ref-in-want");
+	}
+
+	@Test
+	public void testV2CapabilitiesRefInWantNotAdvertisedIfUnallowed() throws Exception {
+		checkUnadvertisedIfUnallowed("ref-in-want");
 	}
 
 	@Test
@@ -1833,34 +1846,32 @@ public class UploadPackTest {
 				.has(preparator.subtree3.toObjectId()));
 	}
 
-	@Test
-	public void testV2FetchFilterWhenNotAllowed() throws Exception {
+	private void checkV2FetchWhenNotAllowed(String fetchLine, String expectedMessage)
+			throws Exception {
 		RevCommit commit = remote.commit().message("0").create();
 		remote.update("master", commit);
 
-		server.getConfig().setBoolean("uploadpack", null, "allowfilter", false);
-
 		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()));
+						fetchLine, "done\n", PacketLineIn.end()));
 		assertThat(e.getCause().getMessage(),
-				containsString("unexpected filter blob:limit=5"));
+				containsString(expectedMessage));
 	}
 
 	@Test
-	public void testV2FetchWantRefIfNotAllowed() throws Exception {
-		RevCommit one = remote.commit().message("1").create();
-		remote.update("one", one);
+	public void testV2FetchFilterWhenNotAllowed() throws Exception {
+		checkV2FetchWhenNotAllowed(
+			"filter blob:limit=5\n",
+			"unexpected filter blob:limit=5");
+	}
 
-		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"));
+	@Test
+	public void testV2FetchWantRefIfNotAllowed() throws Exception {
+		checkV2FetchWhenNotAllowed(
+			"want-ref refs/heads/one\n",
+			"unexpected want-ref refs/heads/one");
 	}
 
 	@Test