From 1638a2fce8e2a71f4cdfdee278e0cb9699860add Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 18 Dec 2018 19:44:39 -0800 Subject: UploadPack: Defer want-ref resolution to after parsing ProtocolV2Parser explains: // 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. Do so. This way all ref database accesses are in one place, in the UploadPack class. No user-visible change intended --- this is just to make the code easier to manipulate. Change-Id: I68e87dff7b9a63ccc169bd0836e8e8baaf5d1048 Signed-off-by: Jonathan Nieder Signed-off-by: Matthias Sohn --- .../org/eclipse/jgit/transport/FetchV2Request.java | 22 ++++++--------- .../eclipse/jgit/transport/ProtocolV2Parser.java | 32 +++------------------- .../src/org/eclipse/jgit/transport/UploadPack.java | 28 +++++++++++++++---- 3 files changed, 35 insertions(+), 47 deletions(-) (limited to 'org.eclipse.jgit') 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 853d96905c..34f3484951 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 eae2c6edb9..2cc50a7f38 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 c753bcdc7a..7e29c9b7a2 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'); } -- cgit v1.2.3