motiejus
/
jgit
1
Fork 0
Code Releases Activity

Use bitmaps for non-commit reachability checks 

Currently, unless RequestPolicy#ANY is used, UploadPack rejects all
non-commit "want" lines unless they were advertized. This is fine,
except when "uploadpack.allowreachablesha1inwant" is true
(corresponding to RequestPolicy#REACHABLE_COMMIT), in which case one
would expect that "want"-ing anything reachable would work.

(There is no restriction that "want" lines must only contain commits -
it is allowed for refs to directly point to trees and blobs, and
requesting for them using "want" lines works.)

This commit has been written to avoid performance regressions as much
as possible. In the usual (and currently working) case where the only
unadvertized things requested are commits, we do a standard RevWalk in
order to avoid incurring the cost of loading bitmaps. However, if
unadvertized non-commits are requested, bitmaps are used instead, and
if there are no bitmaps, a WantNotValidException is thrown (as is
currently done).

Change-Id: I68ed4abd0e477ff415c696c7544ccaa234df7f99
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
This commit is contained in:
Jonathan Tan 2017-11-01 17:36:18 -07:00
parent ccf25f9541
commit d3021788d2
2 changed files with 120 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

87
org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
View File

@ -4,24 +4,33 @@
import static org.junit.Assert.assertTrue;
import java.util.Collections;
import org.eclipse.jgit.errors.TransportException;
import org.eclipse.jgit.internal.storage.dfs.DfsGarbageCollector;
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.RevBlob;
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.hamcrest.Matchers;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
/**
* Tests for server upload-pack utilities.
*/
public class UploadPackTest {
@Rule
public ExpectedException thrown = ExpectedException.none();
private URIish uri;
private TestProtocol<Object> testProtocol;
@ -51,6 +60,25 @@ private static InMemoryRepository newRepo(String name) {
return new InMemoryRepository(new DfsRepositoryDescription(name));
}
private void generateBitmaps(InMemoryRepository repo) throws Exception {
new DfsGarbageCollector(repo).pack(null);
repo.scanForRepoChanges();
}
private static TestProtocol<Object> generateReachableCommitUploadPackProtocol() {
return new TestProtocol<>(
new UploadPackFactory<Object>() {
@Override
public UploadPack create(Object req, Repository db)
throws ServiceNotEnabledException,
ServiceNotAuthorizedException {
UploadPack up = new UploadPack(db);
up.setRequestPolicy(RequestPolicy.REACHABLE_COMMIT);
return up;
}
}, null);
}
@Test
public void testFetchParentOfShallowCommit() throws Exception {
RevCommit commit0 = remote.commit().message("0").create();
@ -83,4 +111,63 @@ public UploadPack create(Object req, Repository db)
assertTrue(client.hasObject(commit0.toObjectId()));
}
}
@Test
public void testFetchUnreachableBlobWithBitmap() throws Exception {
RevBlob blob = remote.blob("foo");
remote.commit(remote.tree(remote.file("foo", blob)));
generateBitmaps(server);
testProtocol = generateReachableCommitUploadPackProtocol();
uri = testProtocol.register(ctx, server);
assertFalse(client.hasObject(blob.toObjectId()));
try (Transport tn = testProtocol.open(uri, client, "server")) {
thrown.expect(TransportException.class);
thrown.expectMessage(Matchers.containsString(
"want " + blob.name() + " not valid"));
tn.fetch(NullProgressMonitor.INSTANCE,
Collections.singletonList(new RefSpec(blob.name())));
}
}
@Test
public void testFetchReachableBlobWithBitmap() throws Exception {
RevBlob blob = remote.blob("foo");
RevCommit commit = remote.commit(remote.tree(remote.file("foo", blob)));
remote.update("master", commit);
generateBitmaps(server);
testProtocol = generateReachableCommitUploadPackProtocol();
uri = testProtocol.register(ctx, server);
assertFalse(client.hasObject(blob.toObjectId()));
try (Transport tn = testProtocol.open(uri, client, "server")) {
tn.fetch(NullProgressMonitor.INSTANCE,
Collections.singletonList(new RefSpec(blob.name())));
assertTrue(client.hasObject(blob.toObjectId()));
}
}
@Test
public void testFetchReachableBlobWithoutBitmap() throws Exception {
RevBlob blob = remote.blob("foo");
RevCommit commit = remote.commit(remote.tree(remote.file("foo", blob)));
remote.update("master", commit);
testProtocol = generateReachableCommitUploadPackProtocol();
uri = testProtocol.register(ctx, server);
assertFalse(client.hasObject(blob.toObjectId()));
try (Transport tn = testProtocol.open(uri, client, "server")) {
thrown.expect(TransportException.class);
thrown.expectMessage(Matchers.containsString(
"want " + blob.name() + " not valid"));
tn.fetch(NullProgressMonitor.INSTANCE,
Collections.singletonList(new RefSpec(blob.name())));
}
}
}

35
org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
View File

@ -77,14 +77,18 @@
import org.eclipse.jgit.errors.PackProtocolException;
import org.eclipse.jgit.internal.JGitText;
import org.eclipse.jgit.internal.storage.pack.PackWriter;
import org.eclipse.jgit.lib.BitmapIndex;
import org.eclipse.jgit.lib.BitmapIndex.BitmapBuilder;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.NullProgressMonitor;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.ProgressMonitor;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.AsyncRevObjectQueue;
import org.eclipse.jgit.revwalk.BitmapWalker;
import org.eclipse.jgit.revwalk.DepthWalk;
import org.eclipse.jgit.revwalk.ObjectWalk;
import org.eclipse.jgit.revwalk.RevCommit;
@ -1315,6 +1319,18 @@ public void checkWants(UploadPack up, List<ObjectId> wants)
}
}
private static void checkNotAdvertisedWantsUsingBitmap(ObjectReader reader,
BitmapIndex bitmapIndex, List<ObjectId> notAdvertisedWants,
Set<ObjectId> reachableFrom) throws IOException {
BitmapWalker bitmapWalker = new BitmapWalker(new ObjectWalk(reader), bitmapIndex, null);
BitmapBuilder reachables = bitmapWalker.findObjects(reachableFrom, null, false);
for (ObjectId oid : notAdvertisedWants) {
if (!reachables.contains(oid)) {
throw new WantNotValidException(oid);
}
}
}
private static void checkNotAdvertisedWants(UploadPack up,
List<ObjectId> notAdvertisedWants, Set<ObjectId> reachableFrom)
throws MissingObjectException, IncorrectObjectTypeException, IOException {
@ -1324,13 +1340,28 @@ private static void checkNotAdvertisedWants(UploadPack up,
// into an advertised branch it will be marked UNINTERESTING and no commits
// return.
try (RevWalk walk = new RevWalk(up.getRevWalk().getObjectReader())) {
ObjectReader reader = up.getRevWalk().getObjectReader();
try (RevWalk walk = new RevWalk(reader)) {
AsyncRevObjectQueue q = walk.parseAny(notAdvertisedWants, true);
try {
RevObject obj;
while ((obj = q.next()) != null) {
if (!(obj instanceof RevCommit))
if (!(obj instanceof RevCommit)) {
// If unadvertized non-commits are requested, use
// bitmaps. If there are no bitmaps, instead of
// incurring the expense of a manual walk, reject
// the request.
BitmapIndex bitmapIndex = reader.getBitmapIndex();
if (bitmapIndex != null) {
checkNotAdvertisedWantsUsingBitmap(
reader,
bitmapIndex,
notAdvertisedWants,
reachableFrom);
return;
}
throw new WantNotValidException(obj);
}
walk.markStart((RevCommit) obj);
}
} catch (MissingObjectException notFound) {