UploadPack: Implement deepen-not for protocol v2

This allows clients to use the --shallow-exclude parameter (producing a
"deepen-not <ref>" line when communicating with the server) in their fetch
commands when fetching against a JGit server using protocol v2.

Note that the implementation in this commit is somewhat inefficient, as
described in the TODO comment in DepthGenerator.

Change-Id: I9fad3ed9276b624d8f668356ffd99a067dc67ef7
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java

@@ -1299,6 +1299,112 @@ public class UploadPackTest {
 			PacketLineIn.END);
 	}
 
+	@Test
+	public void testV2FetchDeepenNot() throws Exception {
+		RevCommit one = remote.commit().message("one").create();
+		RevCommit two = remote.commit().message("two").parent(one).create();
+		RevCommit three = remote.commit().message("three").parent(two).create();
+		RevCommit side = remote.commit().message("side").parent(one).create();
+		RevCommit merge = remote.commit().message("merge")
+			.parent(three).parent(side).create();
+
+		remote.update("branch1", merge);
+		remote.update("side", side);
+
+		// The client is a shallow clone that only has "three", and
+		// wants "merge" while excluding "side".
+		ByteArrayInputStream recvStream = uploadPackV2(
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"shallow " + three.toObjectId().getName() + "\n",
+			"deepen-not side\n",
+			"want " + merge.toObjectId().getName() + "\n",
+			"have " + three.toObjectId().getName() + "\n",
+			"done\n",
+			PacketLineIn.END);
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+		assertThat(pckIn.readString(), is("shallow-info"));
+
+		// "merge" is shallow because "side" is excluded by deepen-not.
+		// "two" is shallow because "one" (as parent of "side") is excluded by deepen-not.
+		assertThat(
+			Arrays.asList(pckIn.readString(), pckIn.readString()),
+			hasItems(
+				"shallow " + merge.toObjectId().getName(),
+				"shallow " + two.toObjectId().getName()));
+
+		// "three" is unshallow because its parent "two" is now available.
+		assertThat(pckIn.readString(), is("unshallow " + three.toObjectId().getName()));
+
+		assertThat(pckIn.readString(), theInstance(PacketLineIn.DELIM));
+		assertThat(pckIn.readString(), is("packfile"));
+		parsePack(recvStream);
+
+		// The server does not send these because they are excluded by
+		// deepen-not.
+		assertFalse(client.hasObject(side.toObjectId()));
+		assertFalse(client.hasObject(one.toObjectId()));
+
+		// The server does not send this because the client claims to
+		// have it.
+		assertFalse(client.hasObject(three.toObjectId()));
+
+		// The server sends both these commits.
+		assertTrue(client.hasObject(merge.toObjectId()));
+		assertTrue(client.hasObject(two.toObjectId()));
+	}
+
+	@Test
+	public void testV2FetchDeepenNot_excludeDescendantOfWant() throws Exception {
+		RevCommit one = remote.commit().message("one").create();
+		RevCommit two = remote.commit().message("two").parent(one).create();
+		RevCommit three = remote.commit().message("three").parent(two).create();
+		RevCommit four = remote.commit().message("four").parent(three).create();
+
+		remote.update("two", two);
+		remote.update("four", four);
+
+		thrown.expect(PackProtocolException.class);
+		thrown.expectMessage("No commits selected for shallow request");
+		uploadPackV2(
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"deepen-not four\n",
+			"want " + two.toObjectId().getName() + "\n",
+			"done\n",
+			PacketLineIn.END);
+	}
+
+	@Test
+	public void testV2FetchDeepenNot_supportAnnotatedTags() throws Exception {
+		RevCommit one = remote.commit().message("one").create();
+		RevCommit two = remote.commit().message("two").parent(one).create();
+		RevCommit three = remote.commit().message("three").parent(two).create();
+		RevCommit four = remote.commit().message("four").parent(three).create();
+		RevTag twoTag = remote.tag("twotag", two);
+
+		remote.update("refs/tags/twotag", twoTag);
+		remote.update("four", four);
+
+		ByteArrayInputStream recvStream = uploadPackV2(
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"deepen-not twotag\n",
+			"want " + four.toObjectId().getName() + "\n",
+			"done\n",
+			PacketLineIn.END);
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+		assertThat(pckIn.readString(), is("shallow-info"));
+		assertThat(pckIn.readString(), is("shallow " + three.toObjectId().getName()));
+		assertThat(pckIn.readString(), theInstance(PacketLineIn.DELIM));
+		assertThat(pckIn.readString(), is("packfile"));
+		parsePack(recvStream);
+		assertFalse(client.hasObject(one.toObjectId()));
+		assertFalse(client.hasObject(two.toObjectId()));
+		assertTrue(client.hasObject(three.toObjectId()));
+		assertTrue(client.hasObject(four.toObjectId()));
+	}
+
 	@Test
 	public void testV2FetchUnrecognizedArgument() throws Exception {
 		thrown.expect(PackProtocolException.class);

org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java

@@ -48,6 +48,7 @@ import java.io.IOException;
 
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.lib.ObjectId;
 
 /**
  * Only produce commits which are below a specified depth.
@@ -80,6 +81,11 @@ class DepthGenerator extends Generator {
 	 */
 	private final RevFlag REINTERESTING;
 
+	/**
+	 * Commits reachable from commits that the client specified using --shallow-exclude.
+	 */
+	private final RevFlag DEEPEN_NOT;
+
 	/**
 	 * @param w
 	 * @param s Parent generator
@@ -96,6 +102,7 @@ class DepthGenerator extends Generator {
 		this.deepenSince = w.getDeepenSince();
 		this.UNSHALLOW = w.getUnshallowFlag();
 		this.REINTERESTING = w.getReinterestingFlag();
+		this.DEEPEN_NOT = w.getDeepenNotFlag();
 
 		s.shareFreeList(pending);
 
@@ -108,6 +115,37 @@ class DepthGenerator extends Generator {
 			if (((DepthWalk.Commit) c).getDepth() == 0)
 				pending.add(c);
 		}
+
+		// Mark DEEPEN_NOT on all deepen-not commits and their ancestors.
+		// TODO(jonathantanmy): This implementation is somewhat
+		// inefficient in that any "deepen-not <ref>" in the request
+		// results in all commits reachable from that ref being parsed
+		// and marked, even if the commit topology is such that it is
+		// not necessary.
+		for (ObjectId oid : w.getDeepenNots()) {
+			RevCommit c;
+			try {
+				c = walk.parseCommit(oid);
+			} catch (IncorrectObjectTypeException notCommit) {
+				// The C Git implementation silently tolerates
+				// non-commits, so do the same here.
+				continue;
+			}
+
+			FIFORevQueue queue = new FIFORevQueue();
+			queue.add(c);
+			while ((c = queue.next()) != null) {
+				if (c.has(DEEPEN_NOT)) {
+					continue;
+				}
+
+				walk.parseHeaders(c);
+				c.add(DEEPEN_NOT);
+				for (RevCommit p : c.getParents()) {
+					queue.add(p);
+				}
+			}
+		}
 	}
 
 	@Override
@@ -139,6 +177,10 @@ class DepthGenerator extends Generator {
 				continue;
 			}
 
+			if (c.has(DEEPEN_NOT)) {
+				continue;
+			}
+
 			int newDepth = c.depth + 1;
 
 			for (RevCommit p : c.parents) {
@@ -160,9 +202,10 @@ class DepthGenerator extends Generator {
 
 					dp.depth = newDepth;
 
-					// If the parent is not too deep, add it to the queue
+					// If the parent is not too deep and was not excluded, add
-					// so that we can produce it later
+					// it to the queue so that we can produce it later
-					if (newDepth <= depth && !failsDeepenSince) {
+					if (newDepth <= depth && !failsDeepenSince &&
+							!p.has(DEEPEN_NOT)) {
 						pending.add(p);
 					} else {
 						c.isBoundary = true;
@@ -187,6 +230,10 @@ class DepthGenerator extends Generator {
 			if ((c.flags & RevWalk.UNINTERESTING) != 0 && !c.has(UNSHALLOW))
 				produce = false;
 
+			if (c.getCommitTime() < deepenSince) {
+				produce = false;
+			}
+
 			if (produce)
 				return c;
 		}

org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java

@@ -45,10 +45,14 @@
 package org.eclipse.jgit.revwalk;
 
 import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
 
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.AnyObjectId;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.Repository;
 
@@ -72,6 +76,14 @@ public interface DepthWalk {
 		return 0;
 	}
 
+	/**
+	 * @return the objects specified by the client using --shallow-exclude
+	 * @since 5.2
+	 */
+	public default List<ObjectId> getDeepenNots() {
+		return Collections.emptyList();
+	}
+
 	/** @return flag marking commits that should become unshallow. */
 	/**
 	 * Get flag marking commits that should become unshallow.
@@ -87,6 +99,12 @@ public interface DepthWalk {
 	 */
 	public RevFlag getReinterestingFlag();
 
+	/**
+	 * @return flag marking commits that are to be excluded because of --shallow-exclude
+	 * @since 5.2
+	 */
+	public RevFlag getDeepenNotFlag();
+
 	/** RevCommit with a depth (in commits) from a root. */
 	public static class Commit extends RevCommit {
 		/** Depth of this commit in the graph, via shortest path. */
@@ -126,10 +144,14 @@ public interface DepthWalk {
 
 		private int deepenSince;
 
+		private List<ObjectId> deepenNots;
+
 		private final RevFlag UNSHALLOW;
 
 		private final RevFlag REINTERESTING;
 
+		private final RevFlag DEEPEN_NOT;
+
 		/**
 		 * @param repo Repository to walk
 		 * @param depth Maximum depth to return
@@ -138,8 +160,10 @@ public interface DepthWalk {
 			super(repo);
 
 			this.depth = depth;
+			this.deepenNots = Collections.emptyList();
 			this.UNSHALLOW = newFlag("UNSHALLOW"); //$NON-NLS-1$
 			this.REINTERESTING = newFlag("REINTERESTING"); //$NON-NLS-1$
+			this.DEEPEN_NOT = newFlag("DEEPEN_NOT"); //$NON-NLS-1$
 		}
 
 		/**
@@ -150,8 +174,10 @@ public interface DepthWalk {
 			super(or);
 
 			this.depth = depth;
+			this.deepenNots = Collections.emptyList();
 			this.UNSHALLOW = newFlag("UNSHALLOW"); //$NON-NLS-1$
 			this.REINTERESTING = newFlag("REINTERESTING"); //$NON-NLS-1$
+			this.DEEPEN_NOT = newFlag("DEEPEN_NOT"); //$NON-NLS-1$
 		}
 
 		/**
@@ -196,6 +222,23 @@ public interface DepthWalk {
 			deepenSince = limit;
 		}
 
+		@Override
+		public List<ObjectId> getDeepenNots() {
+			return deepenNots;
+		}
+
+		/**
+		 * Mark objects that the client specified using
+		 * --shallow-exclude. Objects that are not commits have no
+		 * effect.
+		 *
+		 * @param deepenNots specified objects
+		 * @since 5.2
+		 */
+		public void setDeepenNots(List<ObjectId> deepenNots) {
+			this.deepenNots = Objects.requireNonNull(deepenNots);
+		}
+
 		@Override
 		public RevFlag getUnshallowFlag() {
 			return UNSHALLOW;
@@ -206,6 +249,11 @@ public interface DepthWalk {
 			return REINTERESTING;
 		}
 
+		@Override
+		public RevFlag getDeepenNotFlag() {
+			return DEEPEN_NOT;
+		}
+
 		/**
 		 * @since 4.5
 		 */
@@ -213,6 +261,7 @@ public interface DepthWalk {
 		public ObjectWalk toObjectWalkWithSameObjects() {
 			ObjectWalk ow = new ObjectWalk(reader, depth);
 			ow.deepenSince = deepenSince;
+			ow.deepenNots = deepenNots;
 			ow.objects = objects;
 			ow.freeFlags = freeFlags;
 			return ow;
@@ -225,10 +274,14 @@ public interface DepthWalk {
 
 		private int deepenSince;
 
+		private List<ObjectId> deepenNots;
+
 		private final RevFlag UNSHALLOW;
 
 		private final RevFlag REINTERESTING;
 
+		private final RevFlag DEEPEN_NOT;
+
 		/**
 		 * @param repo Repository to walk
 		 * @param depth Maximum depth to return
@@ -237,8 +290,10 @@ public interface DepthWalk {
 			super(repo);
 
 			this.depth = depth;
+			this.deepenNots = Collections.emptyList();
 			this.UNSHALLOW = newFlag("UNSHALLOW"); //$NON-NLS-1$
 			this.REINTERESTING = newFlag("REINTERESTING"); //$NON-NLS-1$
+			this.DEEPEN_NOT = newFlag("DEEPEN_NOT"); //$NON-NLS-1$
 		}
 
 		/**
@@ -249,8 +304,10 @@ public interface DepthWalk {
 			super(or);
 
 			this.depth = depth;
+			this.deepenNots = Collections.emptyList();
 			this.UNSHALLOW = newFlag("UNSHALLOW"); //$NON-NLS-1$
 			this.REINTERESTING = newFlag("REINTERESTING"); //$NON-NLS-1$
+			this.DEEPEN_NOT = newFlag("DEEPEN_NOT"); //$NON-NLS-1$
 		}
 
 		/**
@@ -308,6 +365,11 @@ public interface DepthWalk {
 			return deepenSince;
 		}
 
+		@Override
+		public List<ObjectId> getDeepenNots() {
+			return deepenNots;
+		}
+
 		@Override
 		public RevFlag getUnshallowFlag() {
 			return UNSHALLOW;
@@ -317,5 +379,10 @@ public interface DepthWalk {
 		public RevFlag getReinterestingFlag() {
 			return REINTERESTING;
 		}
+
+		@Override
+		public RevFlag getDeepenNotFlag() {
+			return DEEPEN_NOT;
+		}
 	}
 }

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

@@ -849,7 +849,7 @@ public class UploadPack {
 				}, unshallow -> {
 					pckOut.writeString("unshallow " + unshallow.name() + '\n'); //$NON-NLS-1$
 					unshallowCommits.add(unshallow);
-				});
+				}, Collections.emptyList());
 				pckOut.end();
 			}
 
@@ -906,7 +906,7 @@ public class UploadPack {
 
 		if (sendPack) {
 			sendPack(accumulator, req, refs == null ? null : refs.values(),
-					unshallowCommits);
+					unshallowCommits, Collections.emptyList());
 		}
 	}
 
@@ -964,6 +964,16 @@ public class UploadPack {
 		// copying data back to class fields
 		wantIds = req.getWantIds();
 
+		List<ObjectId> deepenNots = new ArrayList<>();
+		for (String s : req.getDeepenNotRefs()) {
+			Ref ref = db.getRefDatabase().getRef(s);
+			if (ref == null) {
+				throw new PackProtocolException(MessageFormat
+						.format(JGitText.get().invalidRefName, s));
+			}
+			deepenNots.add(ref.getObjectId());
+		}
+
 		boolean sectionSent = false;
 		boolean mayHaveShallow = req.getDepth() != 0
 				|| req.getDeepenSince() != 0
@@ -977,7 +987,8 @@ public class UploadPack {
 		if (mayHaveShallow) {
 			computeShallowsAndUnshallows(req,
 					shallowCommit -> shallowCommits.add(shallowCommit),
-					unshallowCommit -> unshallowCommits.add(unshallowCommit));
+					unshallowCommit -> unshallowCommits.add(unshallowCommit),
+					deepenNots);
 		}
 		if (!req.getClientShallowCommits().isEmpty())
 			walk.assumeShallow(req.getClientShallowCommits());
@@ -1037,7 +1048,7 @@ public class UploadPack {
 					req.getClientCapabilities().contains(OPTION_INCLUDE_TAG)
 						? db.getRefDatabase().getRefsByPrefix(R_TAGS)
 						: null,
-					unshallowCommits);
+					unshallowCommits, deepenNots);
 			// sendPack invokes pckOut.end() for us, so we do not
 			// need to invoke it here.
 		} else {
@@ -1142,12 +1153,11 @@ public class UploadPack {
 	 */
 	private void computeShallowsAndUnshallows(FetchRequest req,
 			IOConsumer<ObjectId> shallowFunc,
-			IOConsumer<ObjectId> unshallowFunc)
+			IOConsumer<ObjectId> unshallowFunc,
+			List<ObjectId> deepenNots)
 			throws IOException {
-		if (req.getClientCapabilities().contains(OPTION_DEEPEN_RELATIVE)
+		if (req.getClientCapabilities().contains(OPTION_DEEPEN_RELATIVE)) {
-				|| !req.getDeepenNotRefs().isEmpty()) {
 			// TODO(jonathantanmy): Implement deepen-relative
-			// and deepen-not.
 			throw new UnsupportedOperationException();
 		}
 
@@ -1167,6 +1177,8 @@ public class UploadPack {
 				}
 			}
 
+			depthWalk.setDeepenNots(deepenNots);
+
 			RevCommit o;
 			boolean atLeastOne = false;
 			while ((o = depthWalk.next()) != null) {
@@ -1793,19 +1805,23 @@ public class UploadPack {
 	 *            the {@link #OPTION_INCLUDE_TAG} capability was requested.
 	 * @param unshallowCommits
 	 *            shallow commits on the client that are now becoming unshallow
+	 * @param deepenNots
+	 *            objects that the client specified using --shallow-exclude
 	 * @throws IOException
 	 *             if an error occured while generating or writing the pack.
 	 */
 	private void sendPack(PackStatistics.Accumulator accumulator,
 			FetchRequest req,
 			@Nullable Collection<Ref> allTags,
-			List<ObjectId> unshallowCommits) throws IOException {
+			List<ObjectId> unshallowCommits,
+			List<ObjectId> deepenNots) throws IOException {
 		Set<String> caps = req.getClientCapabilities();
 		boolean sideband = caps.contains(OPTION_SIDE_BAND)
 				|| caps.contains(OPTION_SIDE_BAND_64K);
 		if (sideband) {
 			try {
-				sendPack(true, req, accumulator, allTags, unshallowCommits);
+				sendPack(true, req, accumulator, allTags, unshallowCommits,
+						deepenNots);
 			} catch (ServiceMayNotContinueException noPack) {
 				// This was already reported on (below).
 				throw noPack;
@@ -1826,7 +1842,7 @@ public class UploadPack {
 					throw err;
 			}
 		} else {
-			sendPack(false, req, accumulator, allTags, unshallowCommits);
+			sendPack(false, req, accumulator, allTags, unshallowCommits, deepenNots);
 		}
 	}
 
@@ -1861,6 +1877,8 @@ public class UploadPack {
 	 *            the {@link #OPTION_INCLUDE_TAG} capability was requested.
 	 * @param unshallowCommits
 	 *            shallow commits on the client that are now becoming unshallow
+	 * @param deepenNots
+	 *            objects that the client specified using --shallow-exclude
 	 * @throws IOException
 	 *             if an error occured while generating or writing the pack.
 	 */
@@ -1868,7 +1886,8 @@ public class UploadPack {
 			FetchRequest req,
 			PackStatistics.Accumulator accumulator,
 			@Nullable Collection<Ref> allTags,
-			List<ObjectId> unshallowCommits) throws IOException {
+			List<ObjectId> unshallowCommits,
+			List<ObjectId> deepenNots) throws IOException {
 		ProgressMonitor pm = NullProgressMonitor.INSTANCE;
 		OutputStream packOut = rawOut;
 
@@ -1947,13 +1966,17 @@ public class UploadPack {
 			}
 
 			RevWalk rw = walk;
-			if (req.getDepth() > 0 || req.getDeepenSince() != 0) {
+			if (req.getDepth() > 0 || req.getDeepenSince() != 0 || !deepenNots.isEmpty()) {
 				int walkDepth = req.getDepth() == 0 ? Integer.MAX_VALUE
 						: req.getDepth() - 1;
 				pw.setShallowPack(req.getDepth(), unshallowCommits);
-				rw = new DepthWalk.RevWalk(walk.getObjectReader(), walkDepth);
+
-				((DepthWalk.RevWalk) rw).setDeepenSince(req.getDeepenSince());
+				DepthWalk.RevWalk dw = new DepthWalk.RevWalk(
-				rw.assumeShallow(req.getClientShallowCommits());
+						walk.getObjectReader(), walkDepth);
+				dw.setDeepenSince(req.getDeepenSince());
+				dw.setDeepenNots(deepenNots);
+				dw.assumeShallow(req.getClientShallowCommits());
+				rw = dw;
 			}
 
 			if (wantAll.isEmpty()) {