Merge branch 'stable-5.2'

* stable-5.2:
  Prepare 5.2.2-SNAPSHOT builds
  JGit v5.2.1.201812262042-r
  Prepare 5.1.6-SNAPSHOT builds
  JGit v5.1.5.201812261915-r
  UploadPack: Filter refs used for deepen-not resolution
  UploadPack: Avoid calling AdvertiseRefsHook twice
  Prepare 5.1.5-SNAPSHOT builds
  JGit v5.1.4.201812251853-r
  UploadPack: Filter refs used for want-ref resolution
  UploadPack: Defer want-ref resolution to after parsing
  Call AdvertiseRefsHook for protocol v2
  Prepare 4.11.7-SNAPSHOT builds
  JGit v4.11.6.201812241910-r
  Prepare 4.9.9-SNAPSHOT builds
  JGit v4.9.8.201812241815-r
  UploadPack: Test filtering by AdvertiseRefsHook in stateless transports
  Prepare 4.7.8-SNAPSHOT builds
  JGit v4.7.7.201812240805-r
  Fix feature versions imported by feature org.eclipse.jgit.pgm
  Prepare 4.5.6-SNAPSHOT builds
  JGit v4.5.5.201812240535-r
  Call AdvertiseRefsHook before validating wants

Change-Id: Ia56348e54d62630d7c50a4747df89516fc5afad9
Signed-off-by: Jonathan Nieder <jrn@google.com>

4 files changed, 161 insertions, 70 deletions

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 8e36a109e9..ac6361cdeb 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java
@@ -48,9 +48,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 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.annotations.Nullable;
@@ -67,7 +65,7 @@ import org.eclipse.jgit.lib.ObjectId;
 public final class FetchV2Request extends FetchRequest {
 	private final List<ObjectId> peerHas;
 
-	private final TreeMap<String, ObjectId> wantedRefs;
+	private final List<String> wantedRefs;
 
 	private final boolean doneReceived;
 
@@ -75,7 +73,7 @@ public final class FetchV2Request extends FetchRequest {
 	private final List<String> serverOptions;
 
 	FetchV2Request(@NonNull List<ObjectId> peerHas,
-			@NonNull TreeMap<String, ObjectId> wantedRefs,
+			@NonNull List<String> wantedRefs,
 			@NonNull Set<ObjectId> wantIds,
 			@NonNull Set<ObjectId> clientShallowCommits, int deepenSince,
 			@NonNull List<String> deepenNotRefs, int depth,
@@ -102,7 +100,7 @@ public final class FetchV2Request extends FetchRequest {
 	 * @return list of references received in "want-ref" lines
 	 */
 	@NonNull
-	Map<String, ObjectId> getWantedRefs() {
+	List<String> getWantedRefs() {
 		return wantedRefs;
 	}
 
@@ -135,7 +133,7 @@ public final class FetchV2Request extends FetchRequest {
 	static final class Builder {
 		final List<ObjectId> peerHas = new ArrayList<>();
 
-		final TreeMap<String, ObjectId> wantedRefs = new TreeMap<>();
+		final List<String> wantedRefs = new ArrayList<>();
 
 		final Set<ObjectId> wantIds = new HashSet<>();
 
@@ -176,12 +174,10 @@ public final class FetchV2Request extends FetchRequest {
 		 *
 		 * @param refName
 		 *            reference name
-		 * @param oid
-		 *            object id the reference is pointing at
 		 * @return this builder
 		 */
-		Builder addWantedRef(String refName, ObjectId oid) {
-			wantedRefs.put(refName, oid);
+		Builder addWantedRef(String refName) {
+			wantedRefs.add(refName);
 			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 a03f02146a..8f4b86ee0a 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java
@@ -62,8 +62,6 @@ import java.util.function.Consumer;
 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
@@ -113,24 +111,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();
 
@@ -158,22 +149,7 @@ final class ProtocolV2Parser {
 				reqBuilder.addWantId(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.addWantId(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/TransferConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java
index d0db9f0e95..a3e655cd92 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransferConfig.java
@@ -327,6 +327,16 @@ public class TransferConfig {
 		};
 	}
 
+	/**
+	 * Like {@code getRefFilter() == RefFilter.DEFAULT}, but faster.
+	 *
+	 * @return {@code true} if no ref filtering is needed because there
+	 *         are no configured hidden refs.
+	 */
+	boolean hasDefaultRefFilter() {
+		return hideRefs.length == 0;
+	}
+
 	static class FsckKeyNameHolder {
 		private static final Map<String, ObjectChecker.ErrorType> errors;
 
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 7700eea9f9..62c8dc9244 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -43,8 +43,9 @@
 
 package org.eclipse.jgit.transport;
 
+import static java.util.function.Function.identity;
+import static java.util.stream.Collectors.toMap;
 import static org.eclipse.jgit.lib.Constants.R_TAGS;
-import static org.eclipse.jgit.lib.RefDatabase.ALL;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REF_IN_WANT;
 import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_FETCH;
 import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_LS_REFS;
@@ -75,11 +76,11 @@ import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 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;
@@ -97,6 +98,7 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.AsyncRevObjectQueue;
 import org.eclipse.jgit.revwalk.BitmapWalker;
@@ -274,7 +276,10 @@ public class UploadPack {
 
 	private OutputStream msgOut = NullOutputStream.INSTANCE;
 
-	/** The refs we advertised as existing at the start of the connection. */
+	/**
+	 * Refs eligible for advertising to the client, set using
+	 * {@link #setAdvertisedRefs}.
+	 */
 	private Map<String, Ref> refs;
 
 	/** Hook used while processing Git protocol v2 requests. */
@@ -283,6 +288,9 @@ public class UploadPack {
 	/** Hook used while advertising the refs to the client. */
 	private AdvertiseRefsHook advertiseRefsHook = AdvertiseRefsHook.DEFAULT;
 
+	/** Whether the {@link #advertiseRefsHook} has been invoked. */
+	private boolean advertiseRefsHookCalled;
+
 	/** Filter used while advertising the refs to the client. */
 	private RefFilter refFilter = RefFilter.DEFAULT;
 
@@ -794,11 +802,115 @@ public class UploadPack {
 	}
 
 	private Map<String, Ref> getAdvertisedOrDefaultRefs() throws IOException {
-		if (refs == null)
-			setAdvertisedRefs(db.getRefDatabase().getRefs(ALL));
+		if (refs != null) {
+			return refs;
+		}
+
+		if (!advertiseRefsHookCalled) {
+			advertiseRefsHook.advertiseRefs(this);
+			advertiseRefsHookCalled = true;
+		}
+		if (refs == null) {
+			// Fall back to all refs.
+			setAdvertisedRefs(
+					db.getRefDatabase().getRefs().stream()
+						.collect(toMap(Ref::getName, identity())));
+		}
 		return refs;
 	}
 
+	private Map<String, Ref> getFilteredRefs(Collection<String> refPrefixes)
+					throws IOException {
+		if (refPrefixes.isEmpty()) {
+			return getAdvertisedOrDefaultRefs();
+		}
+		if (refs == null && !advertiseRefsHookCalled) {
+			advertiseRefsHook.advertiseRefs(this);
+			advertiseRefsHookCalled = true;
+		}
+		if (refs == null) {
+			// Fast path: the advertised refs hook did not set advertised refs.
+			String[] prefixes = refPrefixes.toArray(new String[0]);
+			Map<String, Ref> rs =
+					db.getRefDatabase().getRefsByPrefix(prefixes).stream()
+						.collect(toMap(Ref::getName, identity(), (a, b) -> b));
+			if (refFilter != RefFilter.DEFAULT) {
+				return refFilter.filter(rs);
+			}
+			return transferConfig.getRefFilter().filter(rs);
+		}
+
+		// Slow path: filter the refs provided by the advertised refs hook.
+		// refFilter has already been applied to refs.
+		return refs.values().stream()
+				.filter(ref -> refPrefixes.stream()
+						.anyMatch(ref.getName()::startsWith))
+				.collect(toMap(Ref::getName, identity()));
+	}
+
+	/**
+	 * Read a ref on behalf of the client.
+	 * <p>
+	 * This checks that the ref is present in the ref advertisement since
+	 * otherwise the client might not be supposed to be able to read it.
+	 *
+	 * @param name
+	 *            the unabbreviated name of the reference.
+	 * @return the requested Ref, or {@code null} if it is not visible or
+	 *         does not exist.
+	 * @throws java.io.IOException
+	 *            on failure to read the ref or check it for visibility.
+	 */
+	@Nullable
+	private Ref getRef(String name) throws IOException {
+		if (refs != null) {
+			return refs.get(name);
+		}
+		if (!advertiseRefsHookCalled) {
+			advertiseRefsHook.advertiseRefs(this);
+			advertiseRefsHookCalled = true;
+		}
+		if (refs == null &&
+				refFilter == RefFilter.DEFAULT &&
+				transferConfig.hasDefaultRefFilter()) {
+			// Fast path: no ref filtering is needed.
+			return db.getRefDatabase().exactRef(name);
+		}
+		return getAdvertisedOrDefaultRefs().get(name);
+	}
+
+	/**
+	 * Find a ref in the usual search path on behalf of the client.
+	 * <p>
+	 * This checks that the ref is present in the ref advertisement since
+	 * otherwise the client might not be supposed to be able to read it.
+	 *
+	 * @param name
+	 *            short name of the ref to find, e.g. "master" to find
+	 *            "refs/heads/master".
+	 * @return the requested Ref, or {@code null} if it is not visible or
+	 *         does not exist.
+	 * @throws java.io.IOException
+	 *            on failure to read the ref or check it for visibility.
+	 */
+	@Nullable
+	private Ref findRef(String name) throws IOException {
+		if (refs != null) {
+			return RefDatabase.findRef(refs, name);
+		}
+		if (!advertiseRefsHookCalled) {
+			advertiseRefsHook.advertiseRefs(this);
+			advertiseRefsHookCalled = true;
+		}
+		if (refs == null &&
+				refFilter == RefFilter.DEFAULT &&
+				transferConfig.hasDefaultRefFilter()) {
+			// Fast path: no ref filtering is needed.
+			return db.getRefDatabase().findRef(name);
+		}
+		return RefDatabase.findRef(getAdvertisedOrDefaultRefs(), name);
+	}
+
 	private void service() throws IOException {
 		boolean sendPack = false;
 		// If it's a non-bidi request, we need to read the entire request before
@@ -921,16 +1033,7 @@ public class UploadPack {
 		if (req.getPeel()) {
 			adv.setDerefTags(true);
 		}
-		Map<String, Ref> refsToSend;
-		if (req.getRefPrefixes().isEmpty()) {
-			refsToSend = getAdvertisedOrDefaultRefs();
-		} else {
-			refsToSend = new HashMap<>();
-			String[] prefixes = req.getRefPrefixes().toArray(new String[0]);
-			for (Ref ref : db.getRefDatabase().getRefsByPrefix(prefixes)) {
-				refsToSend.put(ref.getName(), ref);
-			}
-		}
+		Map<String, Ref> refsToSend = getFilteredRefs(req.getRefPrefixes());
 		if (req.getSymrefs()) {
 			findSymrefs(adv, refsToSend);
 		}
@@ -953,8 +1056,7 @@ public class UploadPack {
 		}
 
 		ProtocolV2Parser parser = new ProtocolV2Parser(transferConfig);
-		FetchV2Request req = parser.parseFetchRequest(pckIn,
-				db.getRefDatabase());
+		FetchV2Request req = parser.parseFetchRequest(pckIn);
 		currentRequest = req;
 		rawOut.stopBuffering();
 
@@ -962,11 +1064,9 @@ public class UploadPack {
 
 		// TODO(ifrade): Refactor to pass around the Request object, instead of
 		// copying data back to class fields
-		wantIds = req.getWantIds();
-
 		List<ObjectId> deepenNots = new ArrayList<>();
 		for (String s : req.getDeepenNotRefs()) {
-			Ref ref = db.getRefDatabase().findRef(s);
+			Ref ref = findRef(s);
 			if (ref == null) {
 				throw new PackProtocolException(MessageFormat
 						.format(JGitText.get().invalidRefName, s));
@@ -974,6 +1074,24 @@ public class UploadPack {
 			deepenNots.add(ref.getObjectId());
 		}
 
+		Map<String, ObjectId> wantedRefs = new TreeMap<>();
+		for (String refName : req.getWantedRefs()) {
+			Ref ref = getRef(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));
+			}
+			// TODO(ifrade): Avoid mutating the parsed request.
+			req.getWantIds().add(oid);
+			wantedRefs.put(refName, oid);
+		}
+		wantIds = req.getWantIds();
+
 		boolean sectionSent = false;
 		boolean mayHaveShallow = req.getDepth() != 0
 				|| req.getDeepenSince() != 0
@@ -1027,13 +1145,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<String, ObjectId> entry : req.getWantedRefs()
-						.entrySet()) {
+				for (Map.Entry<String, ObjectId> entry :
+						wantedRefs.entrySet()) {
 					pckOut.writeString(entry.getValue().getName() + ' ' +
 							entry.getKey() + '\n');
 				}
@@ -1286,15 +1404,7 @@ public class UploadPack {
 			return;
 		}
 
-		try {
-			advertiseRefsHook.advertiseRefs(this);
-		} catch (ServiceMayNotContinueException fail) {
-			if (fail.getMessage() != null) {
-				adv.writeOne("ERR " + fail.getMessage()); //$NON-NLS-1$
-				fail.setOutput();
-			}
-			throw fail;
-		}
+		Map<String, Ref> advertisedOrDefaultRefs = getAdvertisedOrDefaultRefs();
 
 		if (serviceName != null) {
 			adv.writeOne("# service=" + serviceName + '\n'); //$NON-NLS-1$
@@ -1326,7 +1436,6 @@ public class UploadPack {
 			adv.advertiseCapability(OPTION_FILTER);
 		}
 		adv.setDerefTags(true);
-		Map<String, Ref> advertisedOrDefaultRefs = getAdvertisedOrDefaultRefs();
 		findSymrefs(adv, advertisedOrDefaultRefs);
 		advertised = adv.send(advertisedOrDefaultRefs);
 		if (adv.isEmpty())