From 237abe6ab5908cc26211d5877bdd1b60d5158f0e Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Mon, 1 Oct 2018 15:50:47 -0700 Subject: [PATCH] UploadPack: Implement deepen-not for protocol v2 This allows clients to use the --shallow-exclude parameter (producing a "deepen-not " 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 --- .../jgit/transport/UploadPackTest.java | 106 ++++++++++++++++++ .../eclipse/jgit/revwalk/DepthGenerator.java | 53 ++++++++- .../org/eclipse/jgit/revwalk/DepthWalk.java | 67 +++++++++++ .../eclipse/jgit/transport/UploadPack.java | 55 ++++++--- 4 files changed, 262 insertions(+), 19 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 ad3eb2e32..4b3fe28cf 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 @@ -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); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java index ef2575858..c42223408 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java +++ b/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 " 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 - // so that we can produce it later - if (newDepth <= depth && !failsDeepenSince) { + // If the parent is not too deep and was not excluded, add + // it to the queue so that we can produce it later + 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; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java index 3499572d0..66e8497a1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java +++ b/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 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 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 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 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 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 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; + } } } 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 a2174f641..f7af98088 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/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 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 shallowFunc, - IOConsumer unshallowFunc) + IOConsumer unshallowFunc, + List deepenNots) throws IOException { - if (req.getClientCapabilities().contains(OPTION_DEEPEN_RELATIVE) - || !req.getDeepenNotRefs().isEmpty()) { + if (req.getClientCapabilities().contains(OPTION_DEEPEN_RELATIVE)) { // 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 allTags, - List unshallowCommits) throws IOException { + List unshallowCommits, + List deepenNots) throws IOException { Set 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 allTags, - List unshallowCommits) throws IOException { + List unshallowCommits, + List 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()); - rw.assumeShallow(req.getClientShallowCommits()); + + DepthWalk.RevWalk dw = new DepthWalk.RevWalk( + walk.getObjectReader(), walkDepth); + dw.setDeepenSince(req.getDeepenSince()); + dw.setDeepenNots(deepenNots); + dw.assumeShallow(req.getClientShallowCommits()); + rw = dw; } if (wantAll.isEmpty()) {