diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java index 4d1150844..bf67d46d5 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java @@ -42,7 +42,6 @@ */ package org.eclipse.jgit.transport; -import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItems; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; @@ -160,8 +159,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertTrue(request.getOptions() .contains(GitProtocolConstants.OPTION_THIN_PACK)); assertTrue(request.getOptions() @@ -191,8 +189,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertThat(objIdsAsStrings(request.getClientShallowCommits()), hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0", "145e683b229dcab9d0e2ccb01b386f9ecc17d29d")); @@ -211,8 +208,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertThat(objIdsAsStrings(request.getClientShallowCommits()), hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0", "145e683b229dcab9d0e2ccb01b386f9ecc17d29d")); @@ -229,8 +225,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertThat(objIdsAsStrings(request.getClientShallowCommits()), hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0", "145e683b229dcab9d0e2ccb01b386f9ecc17d29d")); @@ -244,8 +239,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowFilter().done()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(0, request.getFilterBlobLimit()); } @@ -256,8 +250,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowFilter().done()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(15, request.getFilterBlobLimit()); } @@ -270,8 +263,7 @@ public class ProtocolV2ParserTest { PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowFilter().done()); - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(0, request.getFilterBlobLimit()); } @@ -282,8 +274,7 @@ public class ProtocolV2ParserTest { "filter blob:limit=12", PacketLineIn.END); ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.getDefault()); - parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + parser.parseFetchRequest(pckIn); } @Test @@ -300,23 +291,16 @@ public class ProtocolV2ParserTest { ProtocolV2Parser parser = new ProtocolV2Parser( ConfigBuilder.start().allowRefInWant().done()); - - FetchV2Request request = parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); assertEquals(1, request.getWantedRefs().size()); - assertThat(request.getWantedRefs().keySet(), - hasItems("refs/heads/branchA")); - assertEquals(2, request.getWantsIds().size()); + assertThat(request.getWantedRefs(), hasItems("refs/heads/branchA")); + assertEquals(1, request.getWantsIds().size()); assertThat(objIdsAsStrings(request.getWantsIds()), - hasItems("e4980cdc48cfa1301493ca94eb70523f6788b819", - one.getName())); + hasItems("e4980cdc48cfa1301493ca94eb70523f6788b819")); } @Test public void testFetchWithRefInWantUnknownRef() throws Exception { - thrown.expect(PackProtocolException.class); - thrown.expectMessage(containsString("refs/heads/branchC")); - PacketLineIn pckIn = formatAsPacketLine(PacketLineIn.DELIM, "want e4980cdc48cfa1301493ca94eb70523f6788b819", "want-ref refs/heads/branchC", @@ -329,8 +313,9 @@ public class ProtocolV2ParserTest { testRepo.update("branchA", one); testRepo.update("branchB", two); - parser.parseFetchRequest(pckIn, - testRepo.getRepository().getRefDatabase()); + FetchV2Request request = parser.parseFetchRequest(pckIn); + assertEquals(1, request.getWantedRefs().size()); + assertThat(request.getWantedRefs(), hasItems("refs/heads/branchC")); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java index 853d96905..34f348495 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java @@ -45,9 +45,7 @@ package org.eclipse.jgit.transport; import java.util.ArrayList; import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Set; -import java.util.TreeMap; import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.lib.ObjectId; @@ -63,7 +61,7 @@ import org.eclipse.jgit.lib.ObjectId; public final class FetchV2Request { private final List peerHas; - private final TreeMap wantedRefs; + private final List wantedRefs; private final Set wantsIds; @@ -82,7 +80,7 @@ public final class FetchV2Request { private final boolean doneReceived; private FetchV2Request(List peerHas, - TreeMap wantedRefs, Set wantsIds, + List wantedRefs, Set wantsIds, Set clientShallowCommits, int deepenSince, List deepenNotRefs, int depth, long filterBlobLimit, boolean doneReceived, Set options) { @@ -110,12 +108,12 @@ public final class FetchV2Request { * @return list of references in the "want-ref" lines of the request */ @NonNull - Map getWantedRefs() { - return this.wantedRefs; + List getWantedRefs() { + return wantedRefs; } /** - * @return object ids in the "want" (and "want-ref") lines of the request + * @return object ids in the "want" (but not "want-ref") lines of the request */ @NonNull Set getWantsIds() { @@ -198,7 +196,7 @@ public final class FetchV2Request { static final class Builder { List peerHas = new ArrayList<>(); - TreeMap wantedRefs = new TreeMap<>(); + List wantedRefs = new ArrayList<>(); Set wantsIds = new HashSet<>(); @@ -234,12 +232,10 @@ public final class FetchV2Request { * * @param refName * reference name - * @param oid - * object id * @return the builder */ - Builder addWantedRef(String refName, ObjectId oid) { - wantedRefs.put(refName, oid); + Builder addWantedRef(String refName) { + wantedRefs.add(refName); return this; } @@ -258,7 +254,7 @@ public final class FetchV2Request { * from a "want" line in a fetch request * @return the builder */ - Builder addWantsIds(ObjectId objectId) { + Builder addWantsId(ObjectId objectId) { wantsIds.add(objectId); return this; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java index eae2c6edb..2cc50a7f3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java @@ -57,8 +57,6 @@ import java.text.MessageFormat; import org.eclipse.jgit.errors.PackProtocolException; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.RefDatabase; /** * Parse the incoming git protocol lines from the wire and translate them into a @@ -79,24 +77,17 @@ final class ProtocolV2Parser { * Parse the incoming fetch request arguments from the wire. The caller must * be sure that what is comings is a fetch request before coming here. * - * This operation requires the reference database to validate incoming - * references. - * * @param pckIn * incoming lines - * @param refdb - * reference database (to validate that received references exist - * and point to valid objects) * @return A FetchV2Request populated with information received from the * wire. * @throws PackProtocolException * incompatible options, wrong type of arguments or other issues * where the request breaks the protocol. * @throws IOException - * an IO error prevented reading the incoming message or - * accessing the ref database. + * an IO error prevented reading the incoming message. */ - FetchV2Request parseFetchRequest(PacketLineIn pckIn, RefDatabase refdb) + FetchV2Request parseFetchRequest(PacketLineIn pckIn) throws PackProtocolException, IOException { FetchV2Request.Builder reqBuilder = FetchV2Request.builder(); @@ -116,25 +107,10 @@ final class ProtocolV2Parser { boolean filterReceived = false; while ((line = pckIn.readString()) != PacketLineIn.END) { if (line.startsWith("want ")) { //$NON-NLS-1$ - reqBuilder.addWantsIds(ObjectId.fromString(line.substring(5))); + reqBuilder.addWantsId(ObjectId.fromString(line.substring(5))); } else if (transferConfig.isAllowRefInWant() && line.startsWith(OPTION_WANT_REF + " ")) { //$NON-NLS-1$ - String refName = line.substring(OPTION_WANT_REF.length() + 1); - // TODO(ifrade): This validation should be done after the - // protocol parsing. It is not a protocol problem asking for an - // unexisting ref and we wouldn't need the ref database here - Ref ref = refdb.exactRef(refName); - if (ref == null) { - throw new PackProtocolException(MessageFormat - .format(JGitText.get().invalidRefName, refName)); - } - ObjectId oid = ref.getObjectId(); - if (oid == null) { - throw new PackProtocolException(MessageFormat - .format(JGitText.get().invalidRefName, refName)); - } - reqBuilder.addWantedRef(refName, oid); - reqBuilder.addWantsIds(oid); + reqBuilder.addWantedRef(line.substring(OPTION_WANT_REF.length() + 1)); } else if (line.startsWith("have ")) { //$NON-NLS-1$ reqBuilder.addPeerHas(ObjectId.fromString(line.substring(5))); } else if (line.equals("done")) { //$NON-NLS-1$ 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 c753bcdc7..7e29c9b7a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -79,6 +79,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeMap; import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.CorruptObjectException; @@ -994,8 +995,7 @@ public class UploadPack { } ProtocolV2Parser parser = new ProtocolV2Parser(transferConfig); - FetchV2Request req = parser.parseFetchRequest(pckIn, - db.getRefDatabase()); + FetchV2Request req = parser.parseFetchRequest(pckIn); rawOut.stopBuffering(); protocolV2Hook.onFetch(req); @@ -1003,13 +1003,29 @@ public class UploadPack { // TODO(ifrade): Refactor to pass around the Request object, instead of // copying data back to class fields options = req.getOptions(); - wantIds.addAll(req.getWantsIds()); clientShallowCommits = req.getClientShallowCommits(); depth = req.getDepth(); shallowSince = req.getDeepenSince(); filterBlobLimit = req.getFilterBlobLimit(); deepenNotRefs = req.getDeepenNotRefs(); + wantIds.addAll(req.getWantsIds()); + Map wantedRefs = new TreeMap<>(); + for (String refName : req.getWantedRefs()) { + Ref ref = db.getRefDatabase().exactRef(refName); + if (ref == null) { + throw new PackProtocolException(MessageFormat + .format(JGitText.get().invalidRefName, refName)); + } + ObjectId oid = ref.getObjectId(); + if (oid == null) { + throw new PackProtocolException(MessageFormat + .format(JGitText.get().invalidRefName, refName)); + } + wantIds.add(oid); + wantedRefs.put(refName, oid); + } + boolean sectionSent = false; @Nullable List shallowCommits = null; List unshallowCommits = new ArrayList<>(); @@ -1059,13 +1075,13 @@ public class UploadPack { sectionSent = true; } - if (!req.getWantedRefs().isEmpty()) { + if (!wantedRefs.isEmpty()) { if (sectionSent) { pckOut.writeDelim(); } pckOut.writeString("wanted-refs\n"); //$NON-NLS-1$ - for (Map.Entry entry : req.getWantedRefs() - .entrySet()) { + for (Map.Entry entry : + wantedRefs.entrySet()) { pckOut.writeString(entry.getValue().getName() + ' ' + entry.getKey() + '\n'); }