From f4cbf31ae4350f5df0f54401ba10fb9b84ec31e2 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 17 Jun 2022 13:49:59 +0200 Subject: [PATCH 1/4] Add missing package import javax.management to org.eclipse.jgit Class org.eclipse.jgit.util.Monitoring uses JMX hence we need this import otherwise OSGi applications can face ClassNotFoundException. Bug: 577018 Change-Id: Ifd75337b87c7faec95d333b771bb0a2f3e46a418 --- org.eclipse.jgit/META-INF/MANIFEST.MF | 1 + 1 file changed, 1 insertion(+) diff --git a/org.eclipse.jgit/META-INF/MANIFEST.MF b/org.eclipse.jgit/META-INF/MANIFEST.MF index 7f1af12dd..9e275f985 100644 --- a/org.eclipse.jgit/META-INF/MANIFEST.MF +++ b/org.eclipse.jgit/META-INF/MANIFEST.MF @@ -220,6 +220,7 @@ Export-Package: org.eclipse.jgit.annotations;version="5.13.2", Bundle-RequiredExecutionEnvironment: JavaSE-1.8 Import-Package: com.googlecode.javaewah;version="[1.1.6,2.0.0)", javax.crypto, + javax.management, javax.net.ssl, org.slf4j;version="[1.7.0,2.0.0)", org.xml.sax, From 66ace4b9af137466a4dd6187a11e473351c306a8 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 19 May 2022 11:12:28 +0100 Subject: [PATCH 2/4] UploadPack: do not check reachability of visible SHA1s When JGit needs to serve a Git client requesting SHA1s during the want phase, it needs to make a full reachability check from the advertised refs to the ones requested to keep all objects in the correct scope of confidentiality allowed by the avertised refs. The check is also performed when the SHA1 corresponds to one of the tips of the advertised refs which is a waste of resources. Example: fetch> ref-prefix refs/heads/foo fetch< 900505eb8ce8ced2a1757906da1b25c357b9654e refs/heads/foo fetch< 0000 fetch> command=fetch fetch> 0001 fetch> thin-pack fetch> ofs-delta fetch> want 900505eb8ce8ced2a1757906da1b25c357b9654e The SHA1 in the want is the tip of refs/heads/foo and therefore the full reachability check can be shortened and resolved more quickly. Change-Id: I49bd9e2464e0bd3bca2abf14c6e9df550d07383b Signed-off-by: Luca Milanesio --- .../src/org/eclipse/jgit/transport/UploadPack.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 9080f51d7..c389ce44f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -1981,12 +1981,16 @@ private static void checkNotAdvertisedWants(UploadPack up, throws IOException { ObjectReader reader = up.getRevWalk().getObjectReader(); + Set directlyVisibleObjects = refIdSet(visibleRefs); + List nonTipWants = notAdvertisedWants.stream() + .filter(not(directlyVisibleObjects::contains)) + .collect(Collectors.toList()); try (RevWalk walk = new RevWalk(reader)) { walk.setRetainBody(false); // Missing "wants" throw exception here List wantsAsObjs = objectIdsToRevObjects(walk, - notAdvertisedWants); + nonTipWants); List wantsAsCommits = wantsAsObjs.stream() .filter(obj -> obj instanceof RevCommit) .map(obj -> (RevCommit) obj) @@ -2046,6 +2050,10 @@ private static void checkNotAdvertisedWants(UploadPack up, } } + private static Predicate not(Predicate t) { + return t.negate(); + } + static Stream importantRefsFirst( Collection visibleRefs) { Predicate startsWithRefsHeads = ref -> ref.getName() From 4bb46936332e9d66569810f0a77bb08bb46fc950 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 29 Apr 2022 16:45:03 +0100 Subject: [PATCH 3/4] Do not create reflog for remote tracking branches during clone When using JGit on a non-bare repository, the CloneCommand it previously created local reflogs for all branches including remote tracking ones, causing the generation of a potentially large number of files on the local filesystem. The creation of the remote-tracking branches (refs/remotes/*) during clone is not an issue for the local filesystem because all of them are stored in a single packed-refs file. However, the creation of a large number of ref logs on a local filesystem IS an issue because it may not be tuned or initialised in term of inodes to contain a very large number of files. When a user (or a CI system) performs the CloneCommand against a potentially large repository (e.g., millions of branches), it is interested in working or validating a single branch or tag and is unlikely to work with all the remote-tracking branches. The eager creation of a reflogs for all the remote-tracking branches is not just a performance issue but may also compromise the ability to use JGit for cloning a large repository. The behaviour implemented in this change is also consistent with the optimisation done in the C code-base [1]. We differentiate between clone and fetch commands using --branch option, that is only available in clone command, and is set as HEAD per default. [1] https://github.com/git/git/commit/58f233ce1ed67bbc31a429fde5c65d5050fdbd7d Bug: 579805 Change-Id: I58d0d36a8a4ce42e0f59b8bf063747c4b81bd859 Signed-off-by: Luca Milanesio --- .../eclipse/jgit/api/CloneCommandTest.java | 44 +++++++++++++++++++ .../eclipse/jgit/api/FetchCommandTest.java | 20 +++++++++ .../eclipse/jgit/transport/FetchProcess.java | 9 +++- 3 files changed, 71 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CloneCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CloneCommandTest.java index de25870bd..c928d2ad2 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CloneCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CloneCommandTest.java @@ -22,6 +22,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.stream.Stream; import org.eclipse.jgit.api.ListBranchCommand.ListMode; import org.eclipse.jgit.api.errors.GitAPIException; @@ -114,6 +115,49 @@ public void testCloneRepository() throws IOException, assertTagOption(git2.getRepository(), TagOpt.AUTO_FOLLOW); } + @Test + public void testCloneRepository_refLogForLocalRefs() + throws IOException, JGitInternalException, GitAPIException { + File directory = createTempDirectory("testCloneRepository"); + CloneCommand command = Git.cloneRepository(); + command.setDirectory(directory); + command.setURI(fileUri()); + Git git2 = command.call(); + Repository clonedRepo = git2.getRepository(); + addRepoToClose(clonedRepo); + + List clonedRefs = clonedRepo.getRefDatabase().getRefs(); + Stream remoteRefs = clonedRefs.stream() + .filter(CloneCommandTest::isRemote); + Stream localHeadsRefs = clonedRefs.stream() + .filter(CloneCommandTest::isLocalHead); + + remoteRefs.forEach(ref -> assertFalse( + "Ref " + ref.getName() + + " is remote and should not have a reflog", + hasRefLog(clonedRepo, ref))); + localHeadsRefs.forEach(ref -> assertTrue( + "Ref " + ref.getName() + + " is local head and should have a reflog", + hasRefLog(clonedRepo, ref))); + } + + private static boolean isRemote(Ref ref) { + return ref.getName().startsWith(Constants.R_REMOTES); + } + + private static boolean isLocalHead(Ref ref) { + return !isRemote(ref) && ref.getName().startsWith(Constants.R_HEADS); + } + + private static boolean hasRefLog(Repository repo, Ref ref) { + try { + return repo.getReflogReader(ref.getName()).getLastEntry() != null; + } catch (IOException ioe) { + throw new IllegalStateException(ioe); + } + } + @Test public void testCloneRepositoryExplicitGitDir() throws IOException, JGitInternalException, GitAPIException { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java index 6479d157e..b608afa5c 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java @@ -76,6 +76,26 @@ public void testFetch() throws Exception { db.resolve(tagRef.getObjectId().getName())); } + @Test + public void testFetchHasRefLogForRemoteRef() throws Exception { + // create an initial commit SHA1 for the default branch + ObjectId defaultBranchSha1 = remoteGit.commit() + .setMessage("initial commit").call().getId(); + + git.fetch().setRemote("test") + .setRefSpecs("refs/heads/*:refs/remotes/origin/*").call(); + + List allFetchedRefs = git.getRepository().getRefDatabase() + .getRefs(); + assertEquals(allFetchedRefs.size(), 1); + Ref remoteRef = allFetchedRefs.get(0); + + assertTrue(remoteRef.getName().startsWith(Constants.R_REMOTES)); + assertEquals(defaultBranchSha1, remoteRef.getObjectId()); + assertNotNull(git.getRepository().getReflogReader(remoteRef.getName()) + .getLastEntry()); + } + @Test public void testForcedFetch() throws Exception { remoteGit.commit().setMessage("commit").call(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java index 34bad6e02..2cedd4b07 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java @@ -205,8 +205,13 @@ else if (tagopt == TagOpt.FETCH_TAGS) BatchRefUpdate batch = transport.local.getRefDatabase() .newBatchUpdate() - .setAllowNonFastForwards(true) - .setRefLogMessage("fetch", true); //$NON-NLS-1$ + .setAllowNonFastForwards(true); + + // Generate reflog only when fetching updates and not at the first clone + if (initialBranch == null) { + batch.setRefLogMessage("fetch", true); //$NON-NLS-1$ + } + try (RevWalk walk = new RevWalk(transport.local)) { walk.setRetainBody(false); if (monitor instanceof BatchingProgressMonitor) { From 035e0e23f251fdb766a6630509bcf342efb8b3ad Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Wed, 29 Jun 2022 14:58:17 +0200 Subject: [PATCH 4/4] UploadPack: don't prematurely terminate timer in case of error In uploadWithExceptionPropagation don't prematurely terminate timer in case of error to enable reporting it to the client. Expose a close method so that callers can terminate it at the appropriate time. If the timer is already terminated when trying to report it to the client this failed with the error java.lang.IllegalStateException: "Timer already terminated". Bug: 579670 Change-Id: I95827442ccb0f9b1ede83630cf7c51cf619c399a --- .../jgit/http/server/UploadPackServlet.java | 2 ++ .../eclipse/jgit/transport/UploadPack.java | 27 +++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java index 2759b8a0f..c4f845e52 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java @@ -195,6 +195,8 @@ public void flush() throws IOException { log(up.getRepository(), e.getCause()); consumeRequestBody(req); out.close(); + } finally { + up.close(); } }; 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 c389ce44f..160e2e6e3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -39,6 +39,7 @@ import static org.eclipse.jgit.util.RefMap.toRefMap; import java.io.ByteArrayOutputStream; +import java.io.Closeable; import java.io.EOFException; import java.io.IOException; import java.io.InputStream; @@ -104,7 +105,7 @@ /** * Implements the server side of a fetch connection, transmitting objects. */ -public class UploadPack { +public class UploadPack implements Closeable { /** Policy the server uses to validate client requests */ public enum RequestPolicy { /** Client may only ask for objects the server advertised a reference for. */ @@ -730,6 +731,17 @@ private boolean useProtocolV2() { && clientRequestedV2; } + @Override + public void close() { + if (timer != null) { + try { + timer.terminate(); + } finally { + timer = null; + } + } + } + /** * Execute the upload task on the socket. * @@ -777,6 +789,8 @@ public void upload(InputStream input, OutputStream output, throw new UploadPackInternalServerErrorException(err); } throw err; + } finally { + close(); } } @@ -786,6 +800,10 @@ public void upload(InputStream input, OutputStream output, *

* If the client passed extra parameters (e.g., "version=2") through a side * channel, the caller must call setExtraParameters first to supply them. + * Callers of this method should call {@link #close()} to terminate the + * internal interrupt timer thread. If the caller fails to terminate the + * thread, it will (eventually) terminate itself when the InterruptTimer + * instance is garbage collected. * * @param input * raw input to read client commands from. Caller must ensure the @@ -842,13 +860,6 @@ public void uploadWithExceptionPropagation(InputStream input, } finally { msgOut = NullOutputStream.INSTANCE; walk.close(); - if (timer != null) { - try { - timer.terminate(); - } finally { - timer = null; - } - } } }