Shallow fetch/clone: Make --depth mean the total history depth

cgit changed the --depth parameter to mean the total depth of history
rather than the depth of ancestors to be returned [1]. JGit still uses
the latter meaning, so update it to match cgit.

depth=0 still means a non-shallow clone. depth=1 now means only the
wants rather than the wants and their direct parents.

This is accomplished by changing the semantic meaning of "depth" in
UploadPack and PackWriter to mean the total depth of history desired,
while keeping "depth" in DepthWalk.{RevWalk,ObjectWalk} to mean
the depth of traversal. Thus UploadPack and PackWriter always
initialize their DepthWalks with "depth-1".

[1] upload-pack: fix off-by-one depth calculation in shallow clone
https://code.googlesource.com/git/+/682c7d2f1a2d1a5443777237450505738af2ff1a

Change-Id: I87ed3c0f56c37e3491e367a41f5e555c4207ff44
Signed-off-by: Terry Parker <tparker@google.com>

5 files changed, 67 insertions, 13 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java
index 50d8310314..3cb4c39c76 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java
@@ -566,17 +566,31 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 	}
 
 	@Test
-	public void testShallowIsMinimal() throws Exception {
+	public void testShallowIsMinimalDepth1() throws Exception {
 		FileRepository repo = setupRepoForShallowFetch();
 
 		PackIndex idx = writeShallowPack(repo, 1, wants(c2), NONE, NONE);
+		assertContent(idx, Arrays.asList(c2.getId(), c2.getTree().getId(),
+				contentA.getId(), contentB.getId()));
+
+		// Client already has blobs A and B, verify those are not packed.
+		idx = writeShallowPack(repo, 1, wants(c5), haves(c2), shallows(c2));
+		assertContent(idx, Arrays.asList(c5.getId(), c5.getTree().getId(),
+				contentC.getId(), contentD.getId(), contentE.getId()));
+	}
+
+	@Test
+	public void testShallowIsMinimalDepth2() throws Exception {
+		FileRepository repo = setupRepoForShallowFetch();
+
+		PackIndex idx = writeShallowPack(repo, 2, wants(c2), NONE, NONE);
 		assertContent(idx,
 				Arrays.asList(c1.getId(), c2.getId(), c1.getTree().getId(),
 						c2.getTree().getId(), contentA.getId(),
 						contentB.getId()));
 
 		// Client already has blobs A and B, verify those are not packed.
-		idx = writeShallowPack(repo, 1, wants(c5), haves(c1, c2), shallows(c1));
+		idx = writeShallowPack(repo, 2, wants(c5), haves(c1, c2), shallows(c1));
 		assertContent(idx,
 				Arrays.asList(c4.getId(), c5.getId(), c4.getTree().getId(),
 						c5.getTree().getId(), contentC.getId(),
@@ -584,33 +598,61 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 	}
 
 	@Test
-	public void testShallowFetchShallowParent() throws Exception {
+	public void testShallowFetchShallowParentDepth1() throws Exception {
 		FileRepository repo = setupRepoForShallowFetch();
 
 		PackIndex idx = writeShallowPack(repo, 1, wants(c5), NONE, NONE);
 		assertContent(idx,
+				Arrays.asList(c5.getId(), c5.getTree().getId(),
+						contentA.getId(), contentB.getId(), contentC.getId(),
+						contentD.getId(), contentE.getId()));
+
+		idx = writeShallowPack(repo, 1, wants(c4), haves(c5), shallows(c5));
+		assertContent(idx, Arrays.asList(c4.getId(), c4.getTree().getId()));
+	}
+
+	@Test
+	public void testShallowFetchShallowParentDepth2() throws Exception {
+		FileRepository repo = setupRepoForShallowFetch();
+
+		PackIndex idx = writeShallowPack(repo, 2, wants(c5), NONE, NONE);
+		assertContent(idx,
 				Arrays.asList(c4.getId(), c5.getId(), c4.getTree().getId(),
 						c5.getTree().getId(), contentA.getId(),
 						contentB.getId(), contentC.getId(), contentD.getId(),
 						contentE.getId()));
 
-		idx = writeShallowPack(repo, 1, wants(c3), haves(c4, c5), shallows(c4));
+		idx = writeShallowPack(repo, 2, wants(c3), haves(c4, c5), shallows(c4));
 		assertContent(idx, Arrays.asList(c2.getId(), c3.getId(),
 				c2.getTree().getId(), c3.getTree().getId()));
 	}
 
 	@Test
-	public void testShallowFetchShallowAncestor() throws Exception {
+	public void testShallowFetchShallowAncestorDepth1() throws Exception {
 		FileRepository repo = setupRepoForShallowFetch();
 
 		PackIndex idx = writeShallowPack(repo, 1, wants(c5), NONE, NONE);
 		assertContent(idx,
+				Arrays.asList(c5.getId(), c5.getTree().getId(),
+						contentA.getId(), contentB.getId(), contentC.getId(),
+						contentD.getId(), contentE.getId()));
+
+		idx = writeShallowPack(repo, 1, wants(c3), haves(c5), shallows(c5));
+		assertContent(idx, Arrays.asList(c3.getId(), c3.getTree().getId()));
+	}
+
+	@Test
+	public void testShallowFetchShallowAncestorDepth2() throws Exception {
+		FileRepository repo = setupRepoForShallowFetch();
+
+		PackIndex idx = writeShallowPack(repo, 2, wants(c5), NONE, NONE);
+		assertContent(idx,
 				Arrays.asList(c4.getId(), c5.getId(), c4.getTree().getId(),
 						c5.getTree().getId(), contentA.getId(),
 						contentB.getId(), contentC.getId(), contentD.getId(),
 						contentE.getId()));
 
-		idx = writeShallowPack(repo, 1, wants(c2), haves(c4, c5), shallows(c4));
+		idx = writeShallowPack(repo, 2, wants(c2), haves(c4, c5), shallows(c4));
 		assertContent(idx, Arrays.asList(c1.getId(), c2.getId(),
 				c1.getTree().getId(), c2.getTree().getId()));
 	}
@@ -645,7 +687,7 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 			Set<? extends ObjectId> shallow) throws IOException {
 		// During negotiation, UploadPack would have set up a DepthWalk and
 		// marked the client's "shallow" commits. Emulate that here.
-		DepthWalk.RevWalk walk = new DepthWalk.RevWalk(repo, depth);
+		DepthWalk.RevWalk walk = new DepthWalk.RevWalk(repo, depth - 1);
 		walk.assumeShallow(shallow);
 		return writePack(repo, walk, depth, want, have, EMPTY_ID_SET);
 	}
diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
index 681b6ac9f0..57b5b8d833 100644
--- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
+++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
@@ -332,6 +332,7 @@ invalidBooleanValue=Invalid boolean value: {0}.{1}={2}
 invalidChannel=Invalid channel {0}
 invalidCharacterInBase64Data=Invalid character in Base64 data.
 invalidCommitParentNumber=Invalid commit parent number
+invalidDepth=Invalid depth: {0}
 invalidEncryption=Invalid encryption
 invalidExpandWildcard=ExpandFromSource on a refspec that can have mismatched wildcards does not make sense.
 invalidGitdirRef = Invalid .git reference in file ''{0}''
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
index 127596fe3d..3a636a1538 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
@@ -391,6 +391,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String invalidChannel;
 	/***/ public String invalidCharacterInBase64Data;
 	/***/ public String invalidCommitParentNumber;
+	/***/ public String invalidDepth;
 	/***/ public String invalidEncryption;
 	/***/ public String invalidExpandWildcard;
 	/***/ public String invalidGitdirRef;
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
index 106308f4ad..16f85166dc 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
@@ -564,7 +564,8 @@ public class PackWriter implements AutoCloseable {
 	 * Configure this pack for a shallow clone.
 	 *
 	 * @param depth
-	 *            maximum depth to traverse the commit graph
+	 *            maximum depth of history to return. 1 means return only the
+	 *            "wants".
 	 * @param unshallow
 	 *            objects which used to be shallow on the client, but are being
 	 *            extended as part of this fetch
@@ -711,7 +712,7 @@ public class PackWriter implements AutoCloseable {
 			@NonNull Set<? extends ObjectId> have) throws IOException {
 		ObjectWalk ow;
 		if (shallowPack)
-			ow = new DepthWalk.ObjectWalk(reader, depth);
+			ow = new DepthWalk.ObjectWalk(reader, depth - 1);
 		else
 			ow = new ObjectWalk(reader);
 		preparePack(countingMonitor, ow, want, have);
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 e1770f282b..fdadb61d15 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -790,8 +790,9 @@ public class UploadPack {
 	}
 
 	private void processShallow() throws IOException {
+		int walkDepth = depth - 1;
 		try (DepthWalk.RevWalk depthWalk = new DepthWalk.RevWalk(
-				walk.getObjectReader(), depth)) {
+				walk.getObjectReader(), walkDepth)) {
 
 			// Find all the commits which will be shallow
 			for (ObjectId o : wantIds) {
@@ -808,12 +809,14 @@ public class UploadPack {
 
 				// Commits at the boundary which aren't already shallow in
 				// the client need to be marked as such
-				if (c.getDepth() == depth && !clientShallowCommits.contains(c))
+				if (c.getDepth() == walkDepth
+						&& !clientShallowCommits.contains(c))
 					pckOut.writeString("shallow " + o.name()); //$NON-NLS-1$
 
 				// Commits not on the boundary which are shallow in the client
 				// need to become unshallowed
-				if (c.getDepth() < depth && clientShallowCommits.remove(c)) {
+				if (c.getDepth() < walkDepth
+						&& clientShallowCommits.remove(c)) {
 					unshallowCommits.add(c.copy());
 					pckOut.writeString("unshallow " + c.name()); //$NON-NLS-1$
 				}
@@ -948,6 +951,11 @@ public class UploadPack {
 
 			if (line.startsWith("deepen ")) { //$NON-NLS-1$
 				depth = Integer.parseInt(line.substring(7));
+				if (depth <= 0) {
+					throw new PackProtocolException(
+							MessageFormat.format(JGitText.get().invalidDepth,
+									Integer.valueOf(depth)));
+				}
 				continue;
 			}
 
@@ -974,7 +982,8 @@ public class UploadPack {
 	}
 
 	/**
-	 * Returns the clone/fetch depth. Valid only after calling recvWants().
+	 * Returns the clone/fetch depth. Valid only after calling recvWants(). A
+	 * depth of 1 means return only the wants.
 	 *
 	 * @return the depth requested by the client, or 0 if unbounded.
 	 * @since 4.0