From e665e3fcd4fdb5097332c0e048c80ccc0dd05caf Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Wed, 29 Aug 2018 16:36:42 -0700 Subject: [PATCH] UploadPack: avoid conflating shallow commit lists in protocol v2 At the moment there are two copies of the client shallow commit list: one in the request and another in the clientShallowCommits member of the class. The verifyShallowCommit function was removing missing object ids from the member but not the request list, and code afterwards was using the request's version. In practice, this didn't cause trouble because these shallow commits are used as endpoint for a walk, and missing ids are just never reached. Change-Id: I70a8f1fd46de135da09f16e5d954693c8438ffcb Signed-off-by: Ivan Frade --- .../jgit/transport/UploadPackTest.java | 38 +++++++++++++++++++ .../eclipse/jgit/transport/UploadPack.java | 19 ++++++---- 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 79d3d87e2..4c6076a20 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 void testV2FetchWantRefAndDeepen() throws Exception { 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 filter(Map 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 60938facb..a88057089 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 Set getOptions() { private final Set commonBase = new HashSet<>(); /** Shallow commits the client already has. */ - private final Set clientShallowCommits = new HashSet<>(); + private Set clientShallowCommits = new HashSet<>(); /** Desired depth from the client on a shallow request. */ private int depth; @@ -832,7 +832,7 @@ else if (requestValidator instanceof AnyRequestValidator) multiAck = MultiAck.OFF; if (!clientShallowCommits.isEmpty()) - verifyClientShallow(); + verifyClientShallow(clientShallowCommits); if (depth != 0) processShallow(null, unshallowCommits, true); if (!clientShallowCommits.isEmpty()) @@ -1068,7 +1068,7 @@ private void fetchV2() throws IOException { // 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 @@ private void fetchV2() throws IOException { List 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 @@ private void processShallow(@Nullable List shallowCommits, } } - 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 shallowCommits) throws IOException, PackProtocolException { - AsyncRevObjectQueue q = walk.parseAny(clientShallowCommits, true); + AsyncRevObjectQueue q = walk.parseAny(shallowCommits, true); try { for (;;) { try { @@ -1323,7 +1328,7 @@ private void verifyClientShallow() } 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; } }