From e426aa8b908b528ab16107d92287050fd26ed047 Mon Sep 17 00:00:00 2001 From: Terry Parker Date: Thu, 4 Aug 2016 11:14:33 -0700 Subject: [PATCH] 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 --- .../internal/storage/file/PackWriterTest.java | 56 ++++++++++++++++--- .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../internal/storage/pack/PackWriter.java | 5 +- .../eclipse/jgit/transport/UploadPack.java | 17 ++++-- 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 50d831031..3cb4c39c7 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 @@ private static void assertContent(PackIndex pi, List expected) { } @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 void testShallowIsMinimal() throws Exception { } @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 @@ private static PackIndex writeShallowPack(FileRepository repo, int depth, Set 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 681b6ac9f..57b5b8d83 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 127596fe3..3a636a153 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 static JGitText get() { /***/ 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 106308f4a..16f85166d 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 void setTagTargets(Set objects) { * 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 void preparePack(ProgressMonitor countingMonitor, @NonNull Set 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 e1770f282..fdadb61d1 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 @@ private static Set refIdSet(Collection refs) { } 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 @@ private void processShallow() throws IOException { // 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 @@ private void recvWants() throws IOException { 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 @@ private void recvWants() throws IOException { } /** - * 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