From 65f9046547768187de89c3c8f1fdb243314675d8 Mon Sep 17 00:00:00 2001 From: Zhen Chen Date: Wed, 4 Oct 2017 14:02:24 -0700 Subject: [PATCH] Use a new RevWalk for validating not advertised wants Shadow commits in the RevWalk in the UploadPack object may cause the UNINTERESTING flag not being carried over to their parents commits since they were marked NO_PARENTS during the assumeShallow or initializeShallowCommits call. A new RevWalk needs to be created for this reason, but instead of creating a new RevWalk from Repository, we can reuse the ObjectReader in the RevWalk of UploadPack to load objects. Change-Id: Ic3fee0512d35b4f555c60e696a880f8b192e4439 Signed-off-by: Zhen Chen --- .../jgit/transport/UploadPackTest.java | 90 +++++++++++++++++++ .../eclipse/jgit/transport/UploadPack.java | 53 +++++------ 2 files changed, 117 insertions(+), 26 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java 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 new file mode 100644 index 0000000000..27c7674e9c --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java @@ -0,0 +1,90 @@ +package org.eclipse.jgit.transport; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.util.Collections; +import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; +import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.UploadPack.RequestPolicy; +import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException; +import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; +import org.eclipse.jgit.transport.resolver.UploadPackFactory; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * Tests for server upload-pack utilities. + */ +public class UploadPackTest { + private URIish uri; + + private TestProtocol testProtocol; + + private Object ctx = new Object(); + + private InMemoryRepository server; + + private InMemoryRepository client; + + private RevCommit commit0; + + private RevCommit commit1; + + private RevCommit tip; + + @Before + public void setUp() throws Exception { + server = newRepo("server"); + client = newRepo("client"); + + TestRepository remote = + new TestRepository<>(server); + commit0 = remote.commit().message("0").create(); + commit1 = remote.commit().message("1").parent(commit0).create(); + tip = remote.commit().message("2").parent(commit1).create(); + remote.update("master", tip); + } + + @After + public void tearDown() { + Transport.unregister(testProtocol); + } + + private static InMemoryRepository newRepo(String name) { + return new InMemoryRepository(new DfsRepositoryDescription(name)); + } + + @Test + public void testFetchParentOfShallowCommit() throws Exception { + testProtocol = new TestProtocol<>( + new UploadPackFactory() { + @Override + public UploadPack create(Object req, Repository db) + throws ServiceNotEnabledException, + ServiceNotAuthorizedException { + UploadPack up = new UploadPack(db); + up.setRequestPolicy(RequestPolicy.REACHABLE_COMMIT); + // assume client has a shallow commit + up.getRevWalk().assumeShallow( + Collections.singleton(commit1.getId())); + return up; + } + }, null); + uri = testProtocol.register(ctx, server); + + assertFalse(client.hasObject(commit0.toObjectId())); + + // Fetch of the parent of the shallow commit + try (Transport tn = testProtocol.open(uri, client, "server")) { + tn.fetch(NullProgressMonitor.INSTANCE, + Collections.singletonList(new RefSpec(commit0.name()))); + assertTrue(client.hasObject(commit0.toObjectId())); + } + } +} 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 c3f50a4d1b..cf070c6348 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -71,7 +71,6 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; - import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; @@ -1261,7 +1260,7 @@ public class UploadPack { @Override public void checkWants(UploadPack up, List wants) throws PackProtocolException, IOException { - checkNotAdvertisedWants(up.getRevWalk(), wants, + checkNotAdvertisedWants(up, wants, refIdSet(up.getAdvertisedRefs().values())); } } @@ -1298,7 +1297,7 @@ public class UploadPack { @Override public void checkWants(UploadPack up, List wants) throws PackProtocolException, IOException { - checkNotAdvertisedWants(up.getRevWalk(), wants, + checkNotAdvertisedWants(up, wants, refIdSet(up.getRepository().getRefDatabase().getRefs(ALL).values())); } } @@ -1316,7 +1315,7 @@ public class UploadPack { } } - private static void checkNotAdvertisedWants(RevWalk walk, + private static void checkNotAdvertisedWants(UploadPack up, List notAdvertisedWants, Set reachableFrom) throws MissingObjectException, IncorrectObjectTypeException, IOException { // Walk the requested commits back to the provided set of commits. If any @@ -1325,32 +1324,34 @@ public class UploadPack { // into an advertised branch it will be marked UNINTERESTING and no commits // return. - AsyncRevObjectQueue q = walk.parseAny(notAdvertisedWants, true); - try { - RevObject obj; - while ((obj = q.next()) != null) { - if (!(obj instanceof RevCommit)) - throw new WantNotValidException(obj); - walk.markStart((RevCommit) obj); - } - } catch (MissingObjectException notFound) { - throw new WantNotValidException(notFound.getObjectId(), notFound); - } finally { - q.release(); - } - for (ObjectId id : reachableFrom) { + try (RevWalk walk = new RevWalk(up.getRevWalk().getObjectReader())) { + AsyncRevObjectQueue q = walk.parseAny(notAdvertisedWants, true); try { - walk.markUninteresting(walk.parseCommit(id)); - } catch (IncorrectObjectTypeException notCommit) { - continue; + RevObject obj; + while ((obj = q.next()) != null) { + if (!(obj instanceof RevCommit)) + throw new WantNotValidException(obj); + walk.markStart((RevCommit) obj); + } + } catch (MissingObjectException notFound) { + throw new WantNotValidException(notFound.getObjectId(), + notFound); + } finally { + q.release(); + } + for (ObjectId id : reachableFrom) { + try { + walk.markUninteresting(walk.parseCommit(id)); + } catch (IncorrectObjectTypeException notCommit) { + continue; + } } - } - RevCommit bad = walk.next(); - if (bad != null) { - throw new WantNotValidException(bad); + RevCommit bad = walk.next(); + if (bad != null) { + throw new WantNotValidException(bad); + } } - walk.reset(); } private void addCommonBase(final RevObject o) { -- 2.39.5