ReceivePack: Moves connectivity check to separate class

Move all connectivity check to separate classes. Set default to be
FullConnectivityChecker i.e. checker which will check connectivity
from all advertised refs. Add ability to set other connectivity
checker which can use different approach.

Signed-off-by: Demetr Starshov <dstarshov@google.com>
Change-Id: Ibb107dbfbdde8ad109be25b5ecf42be7660ef736

org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java

@@ -74,7 +74,6 @@ import java.util.concurrent.TimeUnit;
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.InvalidObjectIdException;
 import org.eclipse.jgit.errors.LargeObjectException;
-import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.errors.TooLargePackException;
 import org.eclipse.jgit.errors.UnpackException;
@@ -93,24 +92,21 @@ import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectChecker;
 import org.eclipse.jgit.lib.ObjectDatabase;
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectIdSubclassMap;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.ObjectLoader;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.ObjectWalk;
-import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevObject;
-import org.eclipse.jgit.revwalk.RevSort;
-import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.PacketLineIn.InputOverLimitIOException;
 import org.eclipse.jgit.transport.ReceiveCommand.Result;
 import org.eclipse.jgit.transport.RefAdvertiser.PacketLineOutRefAdvertiser;
+import org.eclipse.jgit.transport.internal.ConnectivityChecker.ConnectivityCheckInfo;
+import org.eclipse.jgit.transport.internal.ConnectivityChecker;
+import org.eclipse.jgit.transport.internal.FullConnectivityChecker;
 import org.eclipse.jgit.util.io.InterruptTimer;
 import org.eclipse.jgit.util.io.LimitedInputStream;
 import org.eclipse.jgit.util.io.TimeoutInputStream;
@@ -273,7 +269,7 @@ public class ReceivePack {
 	/** Lock around the received pack file, while updating refs. */
 	private PackLock packLock;
 
-	private boolean checkReferencedIsReachable;
+	private boolean checkReferencedAreReachable;
 
 	/** Git object size limit */
 	private long maxObjectSizeLimit;
@@ -292,6 +288,9 @@ public class ReceivePack {
 
 	private ReceivedPackStatistics stats;
 
+	/** Connectivity checker to use. */
+	protected ConnectivityChecker connectivityChecker = new FullConnectivityChecker();
+
 	/** Hook to validate the update commands before execution. */
 	private PreReceiveHook preReceive;
 
@@ -514,7 +513,7 @@ public class ReceivePack {
 	 *         reference.
 	 */
 	public boolean isCheckReferencedObjectsAreReachable() {
-		return checkReferencedIsReachable;
+		return checkReferencedAreReachable;
 	}
 
 	/**
@@ -539,7 +538,7 @@ public class ReceivePack {
 	 *            {@code true} to enable the additional check.
 	 */
 	public void setCheckReferencedObjectsAreReachable(boolean b) {
-		this.checkReferencedIsReachable = b;
+		this.checkReferencedAreReachable = b;
 	}
 
 	/**
@@ -1514,10 +1513,10 @@ public class ReceivePack {
 
 			parser = ins.newPackParser(packInputStream());
 			parser.setAllowThin(true);
-			parser.setNeedNewObjectIds(checkReferencedIsReachable);
+			parser.setNeedNewObjectIds(checkReferencedAreReachable);
-			parser.setNeedBaseObjectIds(checkReferencedIsReachable);
+			parser.setNeedBaseObjectIds(checkReferencedAreReachable);
-			parser.setCheckEofAfterPackFooter(
+			parser.setCheckEofAfterPackFooter(!biDirectionalPipe
-					!biDirectionalPipe && !isExpectDataAfterPackFooter());
+					&& !isExpectDataAfterPackFooter());
 			parser.setExpectDataAfterPackFooter(isExpectDataAfterPackFooter());
 			parser.setObjectChecker(objectChecker);
 			parser.setLockMessage(lockMsg);
@@ -1567,8 +1566,6 @@ public class ReceivePack {
 	}
 
 	private void checkConnectivity() throws IOException {
-		ObjectIdSubclassMap<ObjectId> baseObjects = null;
-		ObjectIdSubclassMap<ObjectId> providedObjects = null;
 		ProgressMonitor checking = NullProgressMonitor.INSTANCE;
 		if (sideBand && !quiet) {
 			SideBandProgressMonitor m = new SideBandProgressMonitor(msgOut);
@@ -1576,76 +1573,18 @@ public class ReceivePack {
 			checking = m;
 		}
 
-		if (checkReferencedIsReachable) {
+		connectivityChecker.checkConnectivity(createConnectivityCheckInfo(),
-			baseObjects = parser.getBaseObjectIds();
+				advertisedHaves, checking);
-			providedObjects = parser.getNewObjectIds();
+	}
-		}
-		parser = null;
-
-		try (ObjectWalk ow = new ObjectWalk(db)) {
-			if (baseObjects != null) {
-				ow.sort(RevSort.TOPO);
-				if (!baseObjects.isEmpty())
-					ow.sort(RevSort.BOUNDARY, true);
-			}
-
-			for (ReceiveCommand cmd : commands) {
-				if (cmd.getResult() != Result.NOT_ATTEMPTED)
-					continue;
-				if (cmd.getType() == ReceiveCommand.Type.DELETE)
-					continue;
-				ow.markStart(ow.parseAny(cmd.getNewId()));
-			}
-			for (ObjectId have : advertisedHaves) {
-				RevObject o = ow.parseAny(have);
-				ow.markUninteresting(o);
-
-				if (baseObjects != null && !baseObjects.isEmpty()) {
-					o = ow.peel(o);
-					if (o instanceof RevCommit)
-						o = ((RevCommit) o).getTree();
-					if (o instanceof RevTree)
-						ow.markUninteresting(o);
-				}
-			}
-
-			checking.beginTask(JGitText.get().countingObjects,
-					ProgressMonitor.UNKNOWN);
-			RevCommit c;
-			while ((c = ow.next()) != null) {
-				checking.update(1);
-				if (providedObjects != null //
-						&& !c.has(RevFlag.UNINTERESTING) //
-						&& !providedObjects.contains(c))
-					throw new MissingObjectException(c, Constants.TYPE_COMMIT);
-			}
-
-			RevObject o;
-			while ((o = ow.nextObject()) != null) {
-				checking.update(1);
-				if (o.has(RevFlag.UNINTERESTING))
-					continue;
-
-				if (providedObjects != null) {
-					if (providedObjects.contains(o)) {
-						continue;
-					}
-					throw new MissingObjectException(o, o.getType());
-				}
-
-				if (o instanceof RevBlob && !db.getObjectDatabase().has(o))
-					throw new MissingObjectException(o, Constants.TYPE_BLOB);
-			}
-			checking.endTask();
 
-			if (baseObjects != null) {
+	private ConnectivityCheckInfo createConnectivityCheckInfo() {
-				for (ObjectId id : baseObjects) {
+		ConnectivityCheckInfo info = new ConnectivityCheckInfo();
-					o = ow.parseAny(id);
+		info.setCheckObjects(checkReferencedAreReachable);
-					if (!o.has(RevFlag.UNINTERESTING))
+		info.setCommands(getAllCommands());
-						throw new MissingObjectException(o, o.getType());
+		info.setRepository(db);
-				}
+		info.setParser(parser);
-			}
+		info.setWalk(walk);
-		}
+		return info;
 	}
 
 	/**

org.eclipse.jgit/src/org/eclipse/jgit/transport/internal/ConnectivityChecker.java

@@ -0,0 +1,138 @@
+/*
+ * Copyright (c) 2019, Google LLC  and others
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * http://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+package org.eclipse.jgit.transport.internal;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Set;
+
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ProgressMonitor;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.PackParser;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+/**
+ * Checks that a received pack only depends on objects which are reachable from
+ * a defined set of references.
+ */
+public interface ConnectivityChecker {
+
+	/**
+	 * Checks connectivity of the commit graph after pack uploading.
+	 *
+	 * @param connectivityCheckInfo
+	 *            Input for the connectivity check.
+	 * @param haves
+	 *            Set of references known for client.
+	 * @param pm
+	 *            Monitor to publish progress to.
+	 * @throws IOException
+	 *             an error occurred during connectivity checking.
+	 *
+	 */
+	void checkConnectivity(ConnectivityCheckInfo connectivityCheckInfo,
+			Set<ObjectId> haves, ProgressMonitor pm)
+			throws IOException;
+
+	/**
+	 * POJO which is used to pass all information which is needed to perform
+	 * connectivity check.
+	 */
+	public static class ConnectivityCheckInfo {
+		private Repository repository;
+
+		private PackParser parser;
+
+		private boolean checkObjects;
+
+		private List<ReceiveCommand> commands;
+
+		private RevWalk walk;
+
+		/**
+		 * @return database we write the stored objects into.
+		 */
+		public Repository getRepository() {
+			return repository;
+		}
+
+		/**
+		 * @param repository
+		 *            set database we write the stored objects into.
+		 */
+		public void setRepository(Repository repository) {
+			this.repository = repository;
+		}
+
+		/**
+		 * @return the parser used to parse pack.
+		 */
+		public PackParser getParser() {
+			return parser;
+		}
+
+		/**
+		 * @param parser
+		 *            the parser to set
+		 */
+		public void setParser(PackParser parser) {
+			this.parser = parser;
+		}
+
+		/**
+		 * @return if checker should check objects.
+		 */
+		public boolean isCheckObjects() {
+			return checkObjects;
+		}
+
+		/**
+		 * @param checkObjects
+		 *            set if checker should check referenced objects outside of
+		 *            the received pack are reachable.
+		 */
+		public void setCheckObjects(boolean checkObjects) {
+			this.checkObjects = checkObjects;
+		}
+
+		/**
+		 * @return command received by the current request.
+		 */
+		public List<ReceiveCommand> getCommands() {
+			return commands;
+		}
+
+		/**
+		 * @param commands
+		 *            set command received by the current request.
+		 */
+		public void setCommands(List<ReceiveCommand> commands) {
+			this.commands = commands;
+		}
+
+		/**
+		 * @param walk
+		 *            the walk to parse commits
+		 */
+		public void setWalk(RevWalk walk) {
+			this.walk = walk;
+		}
+
+		/**
+		 * @return the walk to parse commits
+		 */
+		public RevWalk getWalk() {
+			return walk;
+		}
+	}
+}

org.eclipse.jgit/src/org/eclipse/jgit/transport/internal/FullConnectivityChecker.java

@@ -0,0 +1,200 @@
+/*
+ * Copyright (c) 2019, Google LLC  and others
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * http://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+package org.eclipse.jgit.transport.internal;
+
+import java.io.IOException;
+import java.util.Set;
+
+import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.internal.JGitText;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectIdSubclassMap;
+import org.eclipse.jgit.lib.ProgressMonitor;
+import org.eclipse.jgit.revwalk.ObjectWalk;
+import org.eclipse.jgit.revwalk.RevBlob;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevFlag;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevSort;
+import org.eclipse.jgit.revwalk.RevTree;
+import org.eclipse.jgit.transport.ReceiveCommand;
+import org.eclipse.jgit.transport.ReceiveCommand.Result;
+
+/**
+ * A connectivity checker that uses the entire reference database to perform
+ * reachability checks when checking the connectivity of objects. If
+ * info.isCheckObjects() is set it will also check that objects referenced by
+ * deltas are either provided or reachable as well.
+ */
+public final class FullConnectivityChecker implements ConnectivityChecker {
+	@Override
+	public void checkConnectivity(ConnectivityCheckInfo connectivityCheckInfo,
+			Set<ObjectId> haves, ProgressMonitor pm)
+			throws MissingObjectException, IOException {
+		pm.beginTask(JGitText.get().countingObjects,
+				ProgressMonitor.UNKNOWN);
+		try (ObjectWalk ow = new ObjectWalk(connectivityCheckInfo.getRepository())) {
+			if (!markStartAndKnownNodes(connectivityCheckInfo, ow, haves,
+					pm)) {
+				return;
+			}
+			checkCommitTree(connectivityCheckInfo, ow, pm);
+			checkObjects(connectivityCheckInfo, ow, pm);
+		} finally {
+			pm.endTask();
+		}
+	}
+
+	/**
+	 * @param connectivityCheckInfo
+	 *            Source for connectivity check.
+	 * @param ow
+	 *            Walk which can also check blobs.
+	 * @param haves
+	 *            Set of references known for client.
+	 * @param pm
+	 *            Monitor to publish progress to.
+	 * @return true if at least one new node was marked.
+	 * @throws IOException
+	 *             an error occurred during connectivity checking.
+	 */
+	private boolean markStartAndKnownNodes(
+			ConnectivityCheckInfo connectivityCheckInfo,
+			ObjectWalk ow,
+			Set<ObjectId> haves, ProgressMonitor pm)
+			throws IOException {
+		boolean markTrees = connectivityCheckInfo
+				.isCheckObjects()
+				&& !connectivityCheckInfo.getParser().getBaseObjectIds()
+						.isEmpty();
+		if (connectivityCheckInfo.isCheckObjects()) {
+			ow.sort(RevSort.TOPO);
+			if (!connectivityCheckInfo.getParser().getBaseObjectIds()
+					.isEmpty()) {
+				ow.sort(RevSort.BOUNDARY, true);
+			}
+		}
+		boolean hasInteresting = false;
+
+		for (ReceiveCommand cmd : connectivityCheckInfo.getCommands()) {
+			if (cmd.getResult() != Result.NOT_ATTEMPTED) {
+				continue;
+			}
+			if (cmd.getType() == ReceiveCommand.Type.DELETE) {
+				continue;
+			}
+			if (haves.contains(cmd.getNewId())) {
+				continue;
+			}
+			ow.markStart(ow.parseAny(cmd.getNewId()));
+			pm.update(1);
+			hasInteresting = true;
+		}
+		if (!hasInteresting) {
+			return false;
+		}
+		for (ObjectId have : haves) {
+			RevObject o = ow.parseAny(have);
+			ow.markUninteresting(o);
+			pm.update(1);
+
+			if (markTrees) {
+				o = ow.peel(o);
+				if (o instanceof RevCommit) {
+					o = ((RevCommit) o).getTree();
+				}
+				if (o instanceof RevTree) {
+					ow.markUninteresting(o);
+				}
+			}
+		}
+		return true;
+	}
+
+	/**
+	 * @param connectivityCheckInfo
+	 *            Source for connectivity check.
+	 * @param ow
+	 *            Walk which can also check blobs.
+	 * @param pm
+	 *            Monitor to publish progress to.
+	 * @throws IOException
+	 *             an error occurred during connectivity checking.
+	 */
+	private void checkCommitTree(ConnectivityCheckInfo connectivityCheckInfo,
+			ObjectWalk ow,
+			ProgressMonitor pm) throws IOException {
+		RevCommit c;
+		ObjectIdSubclassMap<ObjectId> newObjectIds = connectivityCheckInfo
+				.getParser()
+				.getNewObjectIds();
+		while ((c = ow.next()) != null) {
+			pm.update(1);
+			if (connectivityCheckInfo.isCheckObjects()
+					&& !c.has(RevFlag.UNINTERESTING)
+					&& !newObjectIds.contains(c)) {
+				throw new MissingObjectException(c, Constants.TYPE_COMMIT);
+			}
+		}
+	}
+
+	/**
+	 * @param connectivityCheckInfo
+	 *            Source for connectivity check.
+	 * @param ow
+	 *            Walk which can also check blobs.
+	 * @param pm
+	 *            Monitor to publish progress to.
+	 * @throws IOException
+	 *             an error occurred during connectivity checking.
+	 *
+	 */
+	private void checkObjects(ConnectivityCheckInfo connectivityCheckInfo,
+			ObjectWalk ow,
+			ProgressMonitor pm) throws IOException {
+		RevObject o;
+		ObjectIdSubclassMap<ObjectId> newObjectIds = connectivityCheckInfo
+				.getParser()
+				.getNewObjectIds();
+
+		while ((o = ow.nextObject()) != null) {
+			pm.update(1);
+			if (o.has(RevFlag.UNINTERESTING)) {
+				continue;
+			}
+
+			if (connectivityCheckInfo.isCheckObjects()) {
+				if (newObjectIds.contains(o)) {
+					continue;
+				}
+				throw new MissingObjectException(o, o.getType());
+
+			}
+
+			if (o instanceof RevBlob
+					&& !connectivityCheckInfo.getRepository().getObjectDatabase()
+							.has(o)) {
+				throw new MissingObjectException(o, Constants.TYPE_BLOB);
+			}
+		}
+
+		if (connectivityCheckInfo.isCheckObjects()) {
+			for (ObjectId id : connectivityCheckInfo.getParser()
+					.getBaseObjectIds()) {
+				o = ow.parseAny(id);
+				if (!o.has(RevFlag.UNINTERESTING)) {
+					throw new MissingObjectException(o, o.getType());
+				}
+			}
+		}
+	}
+}