diff --git a/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF b/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF index ac66b7918..348c305d9 100644 --- a/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF +++ b/org.eclipse.jgit.http.test/META-INF/MANIFEST.MF @@ -50,5 +50,7 @@ Import-Package: javax.servlet;version="[2.5.0,3.2.0)", org.hamcrest;version="[1.1.0,2.0.0)", org.hamcrest.core;version="[1.1.0,2.0.0)", org.junit;version="[4.12,5.0.0)", + org.junit.rules;version="[4.12,5.0.0)", org.junit.runner;version="[4.12,5.0.0)", org.junit.runners;version="[4.12,5.0.0)" +Require-Bundle: org.hamcrest.library;bundle-version="[1.1.0,2.0.0)" diff --git a/org.eclipse.jgit.http.test/pom.xml b/org.eclipse.jgit.http.test/pom.xml index 23a08aa37..072756db3 100644 --- a/org.eclipse.jgit.http.test/pom.xml +++ b/org.eclipse.jgit.http.test/pom.xml @@ -70,6 +70,13 @@ test + + org.hamcrest + hamcrest-library + test + [1.1.0,2.0.0) + + org.eclipse.jgit org.eclipse.jgit @@ -83,13 +90,6 @@ test - - org.hamcrest - hamcrest-library - test - [1.1.0,2.0.0) - - org.eclipse.jgit org.eclipse.jgit.junit.http diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java index 51aa5f2ba..2b4a2511e 100644 --- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java @@ -85,6 +85,7 @@ import org.eclipse.jgit.errors.RemoteRepositoryException; import org.eclipse.jgit.errors.TransportException; import org.eclipse.jgit.errors.UnsupportedCredentialItem; import org.eclipse.jgit.http.server.GitServlet; +import org.eclipse.jgit.http.server.resolver.DefaultUploadPackFactory; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; import org.eclipse.jgit.junit.TestRepository; @@ -105,24 +106,35 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.revwalk.RevBlob; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.transport.AbstractAdvertiseRefsHook; +import org.eclipse.jgit.transport.AdvertiseRefsHook; import org.eclipse.jgit.transport.CredentialItem; import org.eclipse.jgit.transport.CredentialsProvider; import org.eclipse.jgit.transport.FetchConnection; import org.eclipse.jgit.transport.HttpTransport; +import org.eclipse.jgit.transport.RefSpec; import org.eclipse.jgit.transport.RemoteRefUpdate; import org.eclipse.jgit.transport.Transport; import org.eclipse.jgit.transport.TransportHttp; import org.eclipse.jgit.transport.URIish; +import org.eclipse.jgit.transport.UploadPack; import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; import org.eclipse.jgit.transport.http.HttpConnectionFactory; import org.eclipse.jgit.transport.http.JDKHttpConnectionFactory; import org.eclipse.jgit.transport.http.apache.HttpClientConnectionFactory; +import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException; +import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; +import org.eclipse.jgit.transport.resolver.UploadPackFactory; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.HttpSupport; import org.eclipse.jgit.util.SystemReader; +import org.hamcrest.Matchers; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; @@ -131,6 +143,11 @@ import org.junit.runners.Parameterized.Parameters; public class SmartClientSmartServerTest extends HttpTestCase { private static final String HDR_TRANSFER_ENCODING = "Transfer-Encoding"; + @Rule + public ExpectedException thrown = ExpectedException.none(); + + private AdvertiseRefsHook advertiseRefsHook; + private Repository remoteRepository; private CredentialsProvider testCredentials = new UsernamePasswordCredentialsProvider( @@ -148,7 +165,7 @@ public class SmartClientSmartServerTest extends HttpTestCase { private RevBlob A_txt; - private RevCommit A, B; + private RevCommit A, B, unreachableCommit; @Parameters public static Collection data() { @@ -175,6 +192,19 @@ public class SmartClientSmartServerTest extends HttpTestCase { ConfigConstants.CONFIG_KEY_LOGALLREFUPDATES, true); GitServlet gs = new GitServlet(); + gs.setUploadPackFactory(new UploadPackFactory() { + @Override + public UploadPack create(HttpServletRequest req, Repository db) + throws ServiceNotEnabledException, + ServiceNotAuthorizedException { + DefaultUploadPackFactory f = new DefaultUploadPackFactory(); + UploadPack up = f.create(req, db); + if (advertiseRefsHook != null) { + up.setAdvertiseRefsHook(advertiseRefsHook); + } + return up; + } + }); ServletContextHandler app = addNormalContext(gs, src, srcName); @@ -200,6 +230,8 @@ public class SmartClientSmartServerTest extends HttpTestCase { B = src.commit().parent(A).add("A_txt", "C").add("B", "B").create(); src.update(master, B); + unreachableCommit = src.commit().add("A_txt", A_txt).create(); + src.update("refs/garbage/a/very/long/ref/name/to/compress", B); } @@ -452,6 +484,56 @@ public class SmartClientSmartServerTest extends HttpTestCase { info.getResponseHeader(HDR_CONTENT_TYPE)); } + @Test + public void testFetchBySHA1() throws Exception { + Repository dst = createBareRepository(); + assertFalse(dst.hasObject(A_txt)); + + try (Transport t = Transport.open(dst, remoteURI)) { + t.fetch(NullProgressMonitor.INSTANCE, + Collections.singletonList(new RefSpec(B.name()))); + } + + assertTrue(dst.hasObject(A_txt)); + } + + @Test + public void testFetchBySHA1Unreachable() throws Exception { + Repository dst = createBareRepository(); + assertFalse(dst.hasObject(A_txt)); + + try (Transport t = Transport.open(dst, remoteURI)) { + thrown.expect(TransportException.class); + thrown.expectMessage(Matchers.containsString( + "want " + unreachableCommit.name() + " not valid")); + t.fetch(NullProgressMonitor.INSTANCE, Collections + .singletonList(new RefSpec(unreachableCommit.name()))); + } + } + + @Test + public void testFetchBySHA1UnreachableByAdvertiseRefsHook() + throws Exception { + Repository dst = createBareRepository(); + assertFalse(dst.hasObject(A_txt)); + + advertiseRefsHook = new AbstractAdvertiseRefsHook() { + @Override + protected Map getAdvertisedRefs(Repository repository, + RevWalk revWalk) { + return Collections.emptyMap(); + } + }; + + try (Transport t = Transport.open(dst, remoteURI)) { + thrown.expect(TransportException.class); + thrown.expectMessage(Matchers.containsString( + "want " + A.name() + " not valid")); + t.fetch(NullProgressMonitor.INSTANCE, Collections + .singletonList(new RefSpec(A.name()))); + } + } + @Test public void testInitialClone_Small() throws Exception { Repository dst = createBareRepository(); 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 cd7290edc..c753bcdc7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -43,8 +43,9 @@ package org.eclipse.jgit.transport; +import static java.util.function.Function.identity; +import static java.util.stream.Collectors.toMap; import static org.eclipse.jgit.lib.Constants.R_TAGS; -import static org.eclipse.jgit.lib.RefDatabase.ALL; import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REF_IN_WANT; import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_FETCH; import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_LS_REFS; @@ -262,7 +263,10 @@ public class UploadPack { private OutputStream msgOut = NullOutputStream.INSTANCE; - /** The refs we advertised as existing at the start of the connection. */ + /** + * Refs eligible for advertising to the client, set using + * {@link #setAdvertisedRefs}. + */ private Map refs; /** Hook used while processing Git protocol v2 requests. */ @@ -271,6 +275,9 @@ public class UploadPack { /** Hook used while advertising the refs to the client. */ private AdvertiseRefsHook advertiseRefsHook = AdvertiseRefsHook.DEFAULT; + /** Whether the {@link #advertiseRefsHook} has been invoked. */ + private boolean advertiseRefsHookCalled; + /** Filter used while advertising the refs to the client. */ private RefFilter refFilter = RefFilter.DEFAULT; @@ -793,11 +800,52 @@ public class UploadPack { } private Map getAdvertisedOrDefaultRefs() throws IOException { - if (refs == null) - setAdvertisedRefs(db.getRefDatabase().getRefs(ALL)); + if (refs != null) { + return refs; + } + + advertiseRefsHook.advertiseRefs(this); + advertiseRefsHookCalled = true; + if (refs == null) { + // Fall back to all refs. + setAdvertisedRefs( + db.getRefDatabase().getRefs().stream() + .collect(toMap(Ref::getName, identity()))); + } return refs; } + private Map getFilteredRefs(Collection refPrefixes) + throws IOException { + if (refPrefixes.isEmpty()) { + return getAdvertisedOrDefaultRefs(); + } + if (refs == null && !advertiseRefsHookCalled) { + advertiseRefsHook.advertiseRefs(this); + advertiseRefsHookCalled = true; + } + if (refs == null) { + // Fast path: the advertised refs hook did not set advertised refs. + Map rs = new HashMap<>(); + for (String p : refPrefixes) { + for (Ref r : db.getRefDatabase().getRefsByPrefix(p)) { + rs.put(r.getName(), r); + } + } + if (refFilter != RefFilter.DEFAULT) { + return refFilter.filter(rs); + } + return transferConfig.getRefFilter().filter(rs); + } + + // Slow path: filter the refs provided by the advertised refs hook. + // refFilter has already been applied to refs. + return refs.values().stream() + .filter(ref -> refPrefixes.stream() + .anyMatch(ref.getName()::startsWith)) + .collect(toMap(Ref::getName, identity())); + } + private void service() throws IOException { boolean sendPack = false; // If it's a non-bidi request, we need to read the entire request before @@ -923,17 +971,7 @@ public class UploadPack { if (req.getPeel()) { adv.setDerefTags(true); } - Map refsToSend; - if (req.getRefPrefixes().isEmpty()) { - refsToSend = getAdvertisedOrDefaultRefs(); - } else { - refsToSend = new HashMap<>(); - for (String refPrefix : req.getRefPrefixes()) { - for (Ref ref : db.getRefDatabase().getRefsByPrefix(refPrefix)) { - refsToSend.put(ref.getName(), ref); - } - } - } + Map refsToSend = getFilteredRefs(req.getRefPrefixes()); if (req.getSymrefs()) { findSymrefs(adv, refsToSend); } @@ -1281,15 +1319,7 @@ public class UploadPack { return; } - try { - advertiseRefsHook.advertiseRefs(this); - } catch (ServiceMayNotContinueException fail) { - if (fail.getMessage() != null) { - adv.writeOne("ERR " + fail.getMessage()); //$NON-NLS-1$ - fail.setOutput(); - } - throw fail; - } + Map advertisedOrDefaultRefs = getAdvertisedOrDefaultRefs(); if (serviceName != null) { adv.writeOne("# service=" + serviceName + '\n'); //$NON-NLS-1$ @@ -1321,7 +1351,6 @@ public class UploadPack { adv.advertiseCapability(OPTION_FILTER); } adv.setDerefTags(true); - Map advertisedOrDefaultRefs = getAdvertisedOrDefaultRefs(); findSymrefs(adv, advertisedOrDefaultRefs); advertised = adv.send(advertisedOrDefaultRefs); if (adv.isEmpty())