From e5f7d5a136a9948aa16ca074540efc94dc8b1815 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Tue, 22 Jan 2019 13:17:12 -0800 Subject: [PATCH] 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 --- .../jgit/transport/UploadPackTest.java | 113 ++++++++++-------- 1 file changed, 62 insertions(+), 51 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 0ef29d486..a80990d6d 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 void testV2Capabilities() throws Exception { 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 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")); + } + + 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")); + + ArrayList 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 testV2CapabilitiesAllowFilter() throws Exception { + checkAdvertisedIfAllowed("uploadpack", "allowfilter", "filter"); + checkUnadvertisedIfUnallowed("filter"); } @Test public void testV2CapabilitiesRefInWant() throws Exception { - server.getConfig().setBoolean("uploadpack", null, "allowrefinwant", 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=ref-in-want shallow", - "server-option")); - assertTrue(PacketLineIn.isEnd(pckIn.readString())); + checkAdvertisedIfAllowed("uploadpack", "allowrefinwant", "ref-in-want"); } @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); - - 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())); + checkUnadvertisedIfUnallowed("ref-in-want"); } @Test @@ -1833,34 +1846,32 @@ public void testWantFilteredObject() throws Exception { .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 testV2FetchFilterWhenNotAllowed() throws Exception { + checkV2FetchWhenNotAllowed( + "filter blob:limit=5\n", + "unexpected filter blob:limit=5"); } @Test public void testV2FetchWantRefIfNotAllowed() throws Exception { - RevCommit one = remote.commit().message("1").create(); - remote.update("one", one); - - 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")); + checkV2FetchWhenNotAllowed( + "want-ref refs/heads/one\n", + "unexpected want-ref refs/heads/one"); } @Test