Merge "UploadPack: avoid conflating shallow commit lists in protocol v2"

2 files changed, 50 insertions, 7 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
index 79d3d87e2d..4c6076a20b 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
@@ -1417,6 +1417,44 @@ public class UploadPackTest {
 		assertFalse(client.hasObject(parent.toObjectId()));
 	}
 
+	@Test
+	public void testV2FetchMissingShallow() throws Exception {
+		RevCommit one = remote.commit().message("1").create();
+		RevCommit two = remote.commit().message("2").parent(one).create();
+		RevCommit three = remote.commit().message("3").parent(two).create();
+		remote.update("three", three);
+
+		server.getConfig().setBoolean("uploadpack", null, "allowrefinwant",
+				true);
+
+		ByteArrayInputStream recvStream = uploadPackV2("command=fetch\n",
+				PacketLineIn.DELIM,
+				"want-ref refs/heads/three\n",
+				"deepen 3",
+				"shallow 0123012301230123012301230123012301230123",
+				"shallow " + two.getName() + '\n',
+				"done\n",
+				PacketLineIn.END);
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+
+		assertThat(pckIn.readString(), is("shallow-info"));
+		assertThat(pckIn.readString(),
+				is("shallow " + one.toObjectId().getName()));
+		assertThat(pckIn.readString(),
+				is("unshallow " + two.toObjectId().getName()));
+		assertThat(pckIn.readString(), theInstance(PacketLineIn.DELIM));
+		assertThat(pckIn.readString(), is("wanted-refs"));
+		assertThat(pckIn.readString(),
+				is(three.toObjectId().getName() + " refs/heads/three"));
+		assertThat(pckIn.readString(), theInstance(PacketLineIn.DELIM));
+		assertThat(pckIn.readString(), is("packfile"));
+		parsePack(recvStream);
+
+		assertTrue(client.hasObject(one.toObjectId()));
+		assertTrue(client.hasObject(two.toObjectId()));
+		assertTrue(client.hasObject(three.toObjectId()));
+	}
+
 	private static class RejectAllRefFilter implements RefFilter {
 		@Override
 		public Map<String, Ref> filter(Map<String, Ref> refs) {
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 60938facbd..a880570891 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -295,7 +295,7 @@ public class UploadPack {
 	private final Set<RevObject> commonBase = new HashSet<>();
 
 	/** Shallow commits the client already has. */
-	private final Set<ObjectId> clientShallowCommits = new HashSet<>();
+	private Set<ObjectId> clientShallowCommits = new HashSet<>();
 
 	/** Desired depth from the client on a shallow request. */
 	private int depth;
@@ -832,7 +832,7 @@ public class UploadPack {
 				multiAck = MultiAck.OFF;
 
 			if (!clientShallowCommits.isEmpty())
-				verifyClientShallow();
+				verifyClientShallow(clientShallowCommits);
 			if (depth != 0)
 				processShallow(null, unshallowCommits, true);
 			if (!clientShallowCommits.isEmpty())
@@ -1068,7 +1068,7 @@ public class UploadPack {
 		// copying data back to class fields
 		options = req.getOptions();
 		wantIds.addAll(req.getWantsIds());
-		clientShallowCommits.addAll(req.getClientShallowCommits());
+		clientShallowCommits = req.getClientShallowCommits();
 		depth = req.getDepth();
 		shallowSince = req.getShallowSince();
 		filterBlobLimit = req.getFilterBlobLimit();
@@ -1079,7 +1079,7 @@ public class UploadPack {
 		List<ObjectId> unshallowCommits = new ArrayList<>();
 
 		if (!req.getClientShallowCommits().isEmpty()) {
-			verifyClientShallow();
+			verifyClientShallow(req.getClientShallowCommits());
 		}
 		if (req.getDepth() != 0 || req.getShallowSince() != 0
 				|| !req.getShallowExcludeRefs().isEmpty()) {
@@ -1303,9 +1303,14 @@ public class UploadPack {
 		}
 	}
 
-	private void verifyClientShallow()
+	/*
+	 * Verify all shallow lines refer to commits
+	 *
+	 * It can mutate the input set (removing missing object ids from it)
+	 */
+	private void verifyClientShallow(Set<ObjectId> shallowCommits)
 			throws IOException, PackProtocolException {
-		AsyncRevObjectQueue q = walk.parseAny(clientShallowCommits, true);
+		AsyncRevObjectQueue q = walk.parseAny(shallowCommits, true);
 		try {
 			for (;;) {
 				try {
@@ -1323,7 +1328,7 @@ public class UploadPack {
 				} catch (MissingObjectException notCommit) {
 					// shallow objects not known at the server are ignored
 					// by git-core upload-pack, match that behavior.
-					clientShallowCommits.remove(notCommit.getObjectId());
+					shallowCommits.remove(notCommit.getObjectId());
 					continue;
 				}
 			}