From d60c8331143788f1ae18955d6a93d184bd1a2a3f Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Wed, 10 Sep 2014 11:02:59 +0200 Subject: [PATCH 01/36] Allow plugins to use self-provided licenses for used Maven Jars maven_jar only allowed to use licenses that were defined in '//lib:'. But plugins may need further licenses that are not yet in core (E.g.: plugins/its-jira relies on a jar with CPL1.0 license). Since, plugins cannot add rules underneath '//lib:', we add the local_license parameter to maven_jar, which allows plugins to use licenses from ':' instead of '//lib:'. Change-Id: Icc40c413eed65a0b6e3003021050d91086ca4c34 --- lib/maven.defs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/maven.defs b/lib/maven.defs index 5f4006fe27..23708ca249 100644 --- a/lib/maven.defs +++ b/lib/maven.defs @@ -40,7 +40,8 @@ def maven_jar( sha1 = '', bin_sha1 = '', src_sha1 = '', repository = MAVEN_CENTRAL, attach_source = True, - visibility = ['PUBLIC']): + visibility = ['PUBLIC'], + local_license = False): from os import path parts = id.split(':') @@ -89,7 +90,10 @@ def maven_jar( cmd = ' '.join(cmd), out = binjar, ) - license = ['//lib:LICENSE-' + license] + license = ':LICENSE-' + license + if not local_license: + license = '//lib' + license + license = [license] if src_sha1 or attach_source: cmd = ['$(exe //tools:download_file)', '-o', '$OUT', '-u', srcurl] From a4e0250cbdf9344035bcf9168b59e8e3d52ddbb1 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Tue, 26 Aug 2014 15:16:51 -0700 Subject: [PATCH 02/36] Allow PatchListLoader to use recursive merger The automerge "base" created on merge commits should use the recursive merger, if it has not been disabled on this server. Switching to recursive allows handling changes that require the virtual common ancestor to be built when multiple merge bases are discovered for the two parents. Change-Id: Ica2daa6bd0a38f7fa1430bed28bd7f54d3bfb2c4 --- .../google/gerrit/server/git/MergeUtil.java | 7 +++ .../server/index/ChangeBatchIndexer.java | 36 ++++++++++------ .../gerrit/server/patch/PatchListLoader.java | 43 +++++++++++-------- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java index b35d7e478f..709a8ac9dd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java @@ -52,6 +52,7 @@ import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.Merger; +import org.eclipse.jgit.merge.ThreeWayMergeStrategy; import org.eclipse.jgit.merge.ThreeWayMerger; import org.eclipse.jgit.revwalk.FooterKey; import org.eclipse.jgit.revwalk.FooterLine; @@ -89,6 +90,12 @@ public class MergeUtil { return cfg.getBoolean("core", null, "useRecursiveMerge", true); } + public static ThreeWayMergeStrategy getMergeStrategy(Config cfg) { + return useRecursiveMerge(cfg) + ? MergeStrategy.RECURSIVE + : MergeStrategy.RESOLVE; + } + public static interface Factory { MergeUtil create(ProjectState project); MergeUtil create(ProjectState project, boolean useContentMerge); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java index 5ee240b8a6..b2ce8f3859 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java @@ -32,7 +32,9 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.change.MergeabilityChecker; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.MultiProgressMonitor; import com.google.gerrit.server.git.MultiProgressMonitor.Task; import com.google.gerrit.server.patch.PatchListLoader; @@ -43,6 +45,7 @@ import com.google.inject.Inject; import org.eclipse.jgit.diff.DiffEntry; import org.eclipse.jgit.diff.DiffFormatter; import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; @@ -50,6 +53,7 @@ import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.ThreeWayMergeStrategy; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTree; @@ -112,6 +116,7 @@ public class ChangeBatchIndexer { private final ListeningExecutorService executor; private final ChangeIndexer.Factory indexerFactory; private final MergeabilityChecker mergeabilityChecker; + private final ThreeWayMergeStrategy mergeStrategy; @Inject ChangeBatchIndexer(SchemaFactory schemaFactory, @@ -119,6 +124,7 @@ public class ChangeBatchIndexer { GitRepositoryManager repoManager, @IndexExecutor ListeningExecutorService executor, ChangeIndexer.Factory indexerFactory, + @GerritServerConfig Config config, @Nullable MergeabilityChecker mergeabilityChecker) { this.schemaFactory = schemaFactory; this.changeDataFactory = changeDataFactory; @@ -126,6 +132,7 @@ public class ChangeBatchIndexer { this.executor = executor; this.indexerFactory = indexerFactory; this.mergeabilityChecker = mergeabilityChecker; + this.mergeStrategy = MergeUtil.getMergeStrategy(config); } public Result indexAll(ChangeIndex index, Iterable projects, @@ -239,8 +246,13 @@ public class ChangeBatchIndexer { byId.put(r.getObjectId(), changeDataFactory.create(db, c)); } } - new ProjectIndexer(indexer, byId, repo, done, failed, verboseWriter) - .call(); + new ProjectIndexer(indexer, + mergeStrategy, + byId, + repo, + done, + failed, + verboseWriter).call(); } catch (RepositoryNotFoundException rnfe) { log.error(rnfe.getMessage()); } finally { @@ -261,6 +273,7 @@ public class ChangeBatchIndexer { public static class ProjectIndexer implements Callable { private final ChangeIndexer indexer; + private final ThreeWayMergeStrategy mergeStrategy; private final Multimap byId; private final ProgressMonitor done; private final ProgressMonitor failed; @@ -268,16 +281,15 @@ public class ChangeBatchIndexer { private final Repository repo; private RevWalk walk; - public ProjectIndexer(ChangeIndexer indexer, - Multimap changesByCommitId, Repository repo) { - this(indexer, changesByCommitId, repo, - NullProgressMonitor.INSTANCE, NullProgressMonitor.INSTANCE, null); - } - - ProjectIndexer(ChangeIndexer indexer, - Multimap changesByCommitId, Repository repo, - ProgressMonitor done, ProgressMonitor failed, PrintWriter verboseWriter) { + private ProjectIndexer(ChangeIndexer indexer, + ThreeWayMergeStrategy mergeStrategy, + Multimap changesByCommitId, + Repository repo, + ProgressMonitor done, + ProgressMonitor failed, + PrintWriter verboseWriter) { this.indexer = indexer; + this.mergeStrategy = mergeStrategy; this.byId = changesByCommitId; this.repo = repo; this.done = done; @@ -376,7 +388,7 @@ public class ChangeBatchIndexer { walk.parseBody(a); return walk.parseTree(a.getTree()); case 2: - return PatchListLoader.automerge(repo, walk, b); + return PatchListLoader.automerge(repo, walk, b, mergeStrategy); default: return null; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index 4eede003ba..de36020d85 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -21,7 +21,9 @@ import com.google.common.collect.FluentIterable; import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MergeUtil; import com.google.inject.Inject; import org.eclipse.jgit.diff.DiffEntry; @@ -35,6 +37,7 @@ import org.eclipse.jgit.diff.Sequence; import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.dircache.DirCacheBuilder; import org.eclipse.jgit.dircache.DirCacheEntry; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; @@ -45,8 +48,8 @@ import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.merge.MergeFormatter; import org.eclipse.jgit.merge.MergeResult; -import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ResolveMerger; +import org.eclipse.jgit.merge.ThreeWayMergeStrategy; import org.eclipse.jgit.patch.FileHeader; import org.eclipse.jgit.patch.FileHeader.PatchType; import org.eclipse.jgit.revwalk.RevCommit; @@ -72,11 +75,14 @@ public class PatchListLoader extends CacheLoader { private final GitRepositoryManager repoManager; private final PatchListCache patchListCache; + private final ThreeWayMergeStrategy mergeStrategy; @Inject - PatchListLoader(GitRepositoryManager mgr, PatchListCache plc) { + PatchListLoader(GitRepositoryManager mgr, PatchListCache plc, + @GerritServerConfig Config cfg) { repoManager = mgr; patchListCache = plc; + mergeStrategy = MergeUtil.getMergeStrategy(cfg); } @Override @@ -224,7 +230,7 @@ public class PatchListLoader extends CacheLoader { } } - private static RevObject aFor(final PatchListKey key, + private RevObject aFor(final PatchListKey key, final Repository repo, final RevWalk rw, final RevCommit b) throws IOException { if (key.getOldId() != null) { @@ -240,20 +246,20 @@ public class PatchListLoader extends CacheLoader { return r; } case 2: - return automerge(repo, rw, b); + return automerge(repo, rw, b, mergeStrategy); default: // TODO(sop) handle an octopus merge. return null; } } - public static RevTree automerge(Repository repo, RevWalk rw, RevCommit b) - throws IOException { - return automerge(repo, rw, b, true); + public static RevTree automerge(Repository repo, RevWalk rw, RevCommit b, + ThreeWayMergeStrategy mergeStrategy) throws IOException { + return automerge(repo, rw, b, mergeStrategy, true); } public static RevTree automerge(Repository repo, RevWalk rw, RevCommit b, - boolean save) throws IOException { + ThreeWayMergeStrategy mergeStrategy, boolean save) throws IOException { String hash = b.name(); String refName = RefNames.REFS_CACHE_AUTOMERGE + hash.substring(0, 2) @@ -264,8 +270,7 @@ public class PatchListLoader extends CacheLoader { return rw.parseTree(ref.getObjectId()); } - ObjectId treeId; - ResolveMerger m = (ResolveMerger) MergeStrategy.RESOLVE.newMerger(repo, true); + ResolveMerger m = (ResolveMerger) mergeStrategy.newMerger(repo, true); final ObjectInserter ins = repo.newObjectInserter(); try { DirCache dc = DirCache.newInCore(); @@ -297,6 +302,7 @@ public class PatchListLoader extends CacheLoader { return null; } + ObjectId treeId; if (couldMerge) { treeId = m.getResultTreeId(); @@ -381,17 +387,18 @@ public class PatchListLoader extends CacheLoader { treeId = dc.writeTree(ins); } ins.flush(); + + if (save) { + RefUpdate update = repo.updateRef(refName); + update.setNewObjectId(treeId); + update.disableRefLog(); + update.forceUpdate(); + } + + return rw.lookupTree(treeId); } finally { ins.release(); } - - if (save) { - RefUpdate update = repo.updateRef(refName); - update.setNewObjectId(treeId); - update.disableRefLog(); - update.forceUpdate(); - } - return rw.parseTree(treeId); } private static ObjectId emptyTree(final Repository repo) throws IOException { From 735f7fa45831679ff378ce53c84a53633ebf7ac2 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sun, 15 Mar 2015 08:42:10 +0100 Subject: [PATCH 03/36] OAuth: Allow to change username OAuth extension point was derived from GitHub OAuth implementation. Changing of user setting was disabled for GitHub, because it can be induced from the endpoints. Given that other OAuth providers don't expose username it makes sense to allow to change the attributes, as it is the case for OpenID authentication scheme. Inspired-by: Kelly Campbell Change-Id: Idc15271b619f11c17c9ea88e611259e5a113b0e5 --- .../java/com/google/gerrit/server/account/DefaultRealm.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java index 4b8f0b4b1e..938d940ac6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DefaultRealm.java @@ -40,8 +40,7 @@ public class DefaultRealm implements Realm { @Override public boolean allowsEdit(final Account.FieldName field) { - if (authConfig.getAuthType() == AuthType.HTTP - || authConfig.getAuthType() == AuthType.OAUTH) { + if (authConfig.getAuthType() == AuthType.HTTP) { switch (field) { case USER_NAME: return false; From 043c85728abbd67b8d5e7fca1b2f9b8f50178cc7 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sun, 15 Mar 2015 08:46:08 +0100 Subject: [PATCH 04/36] OAuth: Allow to link claimed identity to existing accounts One of use cases OAuth plugin based authentication scheme is aiming to support is switch from deprecated OpenID provider to OAuth scheme offered by the same povider. In this specific case the database is already pre-populated with OpenID accounts. After switching the auth scheme to OAuth all existing accounts must be linked to the new OAuth identity. To support linking new OAuth identity to existing accounts, user info extension point is extended with claimed identity attribute. When passed, the account for this identity is looked up and when found new OAuth identity is linked to it. Change-Id: Ia6489762dd370bfbbaa16a7418cd3106d2d1112a --- .../extensions/auth/oauth/OAuthUserInfo.java | 9 ++++- gerrit-oauth/BUCK | 2 + .../gerrit/httpd/auth/oauth/OAuthSession.java | 40 +++++++++++++++++-- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java index 23a7bec4b4..388ce361e5 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthUserInfo.java @@ -20,15 +20,18 @@ public class OAuthUserInfo { private final String userName; private final String emailAddress; private final String displayName; + private final String claimedIdentity; public OAuthUserInfo(String externalId, String userName, String emailAddress, - String displayName) { + String displayName, + String claimedIdentity) { this.externalId = externalId; this.userName = userName; this.emailAddress = emailAddress; this.displayName = displayName; + this.claimedIdentity = claimedIdentity; } public String getExternalId() { @@ -46,4 +49,8 @@ public class OAuthUserInfo { public String getDisplayName() { return displayName; } + + public String getClaimedIdentity() { + return claimedIdentity; + } } diff --git a/gerrit-oauth/BUCK b/gerrit-oauth/BUCK index 4641e8168e..fa5a8e2cb1 100644 --- a/gerrit-oauth/BUCK +++ b/gerrit-oauth/BUCK @@ -11,9 +11,11 @@ java_library( '//gerrit-common:annotations', '//gerrit-extension-api:api', '//gerrit-httpd:httpd', + '//gerrit-reviewdb:server', '//gerrit-server:server', '//lib:gson', '//lib:guava', + '//lib:gwtorm', '//lib/commons:codec', '//lib/guice:guice', '//lib/guice:guice-servlet', diff --git a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java index d625e02abd..27a1f57144 100644 --- a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java +++ b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java @@ -23,9 +23,11 @@ import com.google.gerrit.extensions.auth.oauth.OAuthUserInfo; import com.google.gerrit.extensions.auth.oauth.OAuthVerifier; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.httpd.WebSession; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.AccountException; import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.AuthResult; +import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.servlet.SessionScoped; @@ -113,11 +115,43 @@ class OAuthSession { throws IOException { com.google.gerrit.server.account.AuthRequest areq = new com.google.gerrit.server.account.AuthRequest(user.getExternalId()); - areq.setUserName(user.getUserName()); - areq.setEmailAddress(user.getEmailAddress()); - areq.setDisplayName(user.getDisplayName()); AuthResult arsp; try { + String claimedIdentifier = user.getClaimedIdentity(); + Account.Id actualId = accountManager.lookup(user.getExternalId()); + if (!Strings.isNullOrEmpty(claimedIdentifier)) { + Account.Id claimedId = accountManager.lookup(claimedIdentifier); + if (claimedId != null && actualId != null) { + if (claimedId.equals(actualId)) { + // Both link to the same account, that's what we expected. + } else { + // This is (for now) a fatal error. There are two records + // for what might be the same user. + // + log.error("OAuth accounts disagree over user identity:\n" + + " Claimed ID: " + claimedId + " is " + claimedIdentifier + + "\n" + " Delgate ID: " + actualId + " is " + + user.getExternalId()); + rsp.sendError(HttpServletResponse.SC_FORBIDDEN); + return; + } + } else if (claimedId != null && actualId == null) { + // Claimed account already exists: link to it. + // + try { + accountManager.link(claimedId, areq); + } catch (OrmException e) { + log.error("Cannot link: " + user.getExternalId() + + " to user identity:\n" + + " Claimed ID: " + claimedId + " is " + claimedIdentifier); + rsp.sendError(HttpServletResponse.SC_FORBIDDEN); + return; + } + } + } + areq.setUserName(user.getUserName()); + areq.setEmailAddress(user.getEmailAddress()); + areq.setDisplayName(user.getDisplayName()); arsp = accountManager.authenticate(areq); } catch (AccountException e) { log.error("Unable to authenticate user \"" + user + "\"", e); From 16133e6e84edec601a10b252dcbf0ebc28f3529f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 20 Mar 2015 11:06:54 +0100 Subject: [PATCH 05/36] Workaround a RecursiveMerger bug [1], avoid online reindexing failure When performing a mergeability check for two commits which don't have a common parent the RecursiveMerger will fail with a NPE. This prevented the OnlineReindexer from finishing Lucene index upgrade. This issue was also discussed in [2]. Workaround: consider any exception from the RecursiveMerger as an expected error and return false as the result of the mergeability check. Also, never fail the online reindexing when the mergeability check is not able to finish properly. [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=462671 [2] https://groups.google.com/d/topic/repo-discuss/REYuk4mgIw8/discussion Change-Id: Iea88440a9e25edf9385aba0663779d30e622e4f8 --- .../com/google/gerrit/server/index/ChangeBatchIndexer.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java index b2ce8f3859..be87ea0b68 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java @@ -157,9 +157,7 @@ public class ChangeBatchIndexer { final AtomicBoolean ok = new AtomicBoolean(true); for (final Project.NameKey project : projects) { - if (!updateMergeable(project)) { - ok.set(false); - } + updateMergeable(project); final ListenableFuture future = executor.submit(reindexProject( indexerFactory.create(index), project, doneTask, failedTask, verboseWriter)); @@ -219,7 +217,7 @@ public class ChangeBatchIndexer { if (mergeabilityChecker != null) { try { mergeabilityChecker.newCheck().addProject(project).run(); - } catch (IOException e) { + } catch (Exception e) { log.error("Error in mergeability checker", e); return false; } From 4cf494eced8f8fcc11fcad638f746900e8756d66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Fri, 20 Mar 2015 09:04:56 -0400 Subject: [PATCH 06/36] Update replication plugin * Fix replication_log with external log4j.configuration Change-Id: I3594fa888e408cf5dbfecaf9531941abc019dfc8 --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index 50e3b57294..ae055c09e1 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit 50e3b57294f445a51252c6d858aaf402ea3a24c7 +Subproject commit ae055c09e13b95278a795b4f5eb45dbe8b99a140 From cffa508a9161e8680b5272da79c43655ab53a2c1 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 18 Mar 2015 14:36:19 -0700 Subject: [PATCH 07/36] Update JGit to 3.7.0.201502260915-r.58-g65c379e Change-Id: Ib4b994c10d83ffc30e478428545dfb0d1be97dfe --- lib/jgit/BUCK | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/jgit/BUCK b/lib/jgit/BUCK index 2d0af1b174..a658fc36e7 100644 --- a/lib/jgit/BUCK +++ b/lib/jgit/BUCK @@ -1,13 +1,15 @@ include_defs('//lib/maven.defs') -VERS = '3.6.2.201501210735-r' +REPO = GERRIT # Leave here even if set to MAVEN_CENTRAL. +VERS = '3.7.0.201502260915-r.58-g65c379e' maven_jar( name = 'jgit', id = 'org.eclipse.jgit:org.eclipse.jgit:' + VERS, - bin_sha1 = '47d59dffb5f02470ccfb6c1a5a31b6040a1636e5', - src_sha1 = '52e133360f2c38046de262c827dfec8ec5b7c885', + bin_sha1 = '8fc9620ec499169facad3355f7417eb6a8aff511', + src_sha1 = '40bd9ae8af8e0b03eb4e43f44f5feda8b7325221', license = 'jgit', + repository = REPO, unsign = True, deps = [':ewah'], exclude = [ @@ -20,8 +22,9 @@ maven_jar( maven_jar( name = 'jgit-servlet', id = 'org.eclipse.jgit:org.eclipse.jgit.http.server:' + VERS, - sha1 = 'f7788bbd0c0414e856c84ddf353e6f4c62fe1d28', + sha1 = 'cecc2b9c0b94455348c3a0c63eb83f72cc595757', license = 'jgit', + repository = REPO, deps = [':jgit'], unsign = True, exclude = [ @@ -33,8 +36,9 @@ maven_jar( maven_jar( name = 'jgit-archive', id = 'org.eclipse.jgit:org.eclipse.jgit.archive:' + VERS, - sha1 = 'ca9da919275dad5ac2844529f4cdccdd919bab5f', + sha1 = '7ccc7c78bf47566045ea7a3c08508ba18e4684ca', license = 'jgit', + repository = REPO, deps = [':jgit', '//lib/commons:compress', '//lib:tukaani-xz', @@ -49,8 +53,9 @@ maven_jar( maven_jar( name = 'junit', id = 'org.eclipse.jgit:org.eclipse.jgit.junit:' + VERS, - sha1 = 'b7418e19efbeb9490b359c8a921cf32bfc57ebbd', + sha1 = '87d64d722447dc3971ace30d2a72593c72a4d05f', license = 'DO_NOT_DISTRIBUTE', + repository = REPO, unsign = True, deps = [':jgit'], ) From 3f2a1e1b2eaf9f30945794c32f0fa605fd9523f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Wed, 18 Mar 2015 15:48:41 -0400 Subject: [PATCH 08/36] Rework intra line diff to interrupt threads instead of killing them Now that MyersDiff is interruptible[1], interrupt threads instead of killing them when MyersDiff goes into infinite loop. Replace IntraLineWorkerPool that was created to allow killing threads by a standard CachedThreadPool. [1] https://git.eclipse.org/r/44041 Change-Id: I39ceef66f503fb9f0b6036fc32b671818772c258 --- Documentation/config-gerrit.txt | 9 - .../java/com/google/gerrit/pgm/Daemon.java | 4 +- .../gerrit/server/patch/DiffExecutor.java | 31 +++ .../server/patch/DiffExecutorModule.java | 39 ++++ .../gerrit/server/patch/IntraLineLoader.java | 50 +++-- .../server/patch/IntraLineWorkerPool.java | 182 ------------------ .../gerrit/testutil/InMemoryModule.java | 15 ++ .../gerrit/httpd/WebAppInitializer.java | 4 +- 8 files changed, 119 insertions(+), 215 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 4ad7d21da9..f174b28229 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -706,15 +706,6 @@ See also link:cmd-flush-caches.html[gerrit flush-caches]. ==== [[cache_options]]Cache Options -[[cache.diff_intraline.maxIdleWorkers]]cache.diff_intraline.maxIdleWorkers:: -+ -Number of idle worker threads to maintain for the intraline difference -computations. There is no upper bound on how many concurrent requests -can occur at once, if additional threads are started to handle a peak -load, only this many will remain idle afterwards. -+ -Default is 1.5x number of available CPUs. - [[cache.diff_intraline.timeout]]cache.diff_intraline.timeout:: + Maximum number of milliseconds to wait for intraline difference data diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java index e28b5ba9ad..1c667e3ad0 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Daemon.java @@ -63,7 +63,7 @@ import com.google.gerrit.server.index.IndexModule; import com.google.gerrit.server.index.IndexModule.IndexType; import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier; import com.google.gerrit.server.mail.SmtpEmailSender; -import com.google.gerrit.server.patch.IntraLineWorkerPool; +import com.google.gerrit.server.patch.DiffExecutorModule; import com.google.gerrit.server.plugins.PluginGuiceEnvironment; import com.google.gerrit.server.plugins.PluginRestApiModule; import com.google.gerrit.server.schema.DataSourceProvider; @@ -320,7 +320,7 @@ public class Daemon extends SiteProgram { modules.add(new ChangeHookRunner.Module()); modules.add(new ReceiveCommitsExecutorModule()); modules.add(new MergeabilityChecksExecutorModule()); - modules.add(new IntraLineWorkerPool.Module()); + modules.add(new DiffExecutorModule()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new InternalAccountDirectory.Module()); modules.add(new DefaultCacheFactory.Module()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java new file mode 100644 index 0000000000..23589e3e3a --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java @@ -0,0 +1,31 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.patch; + +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import com.google.inject.BindingAnnotation; + +import java.lang.annotation.Retention; +import java.util.concurrent.ExecutorService; + +/** + * Marker on {@link ExecutorService} used by + * {@link IntraLineLoader}. + */ +@Retention(RUNTIME) +@BindingAnnotation +public @interface DiffExecutor { +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java new file mode 100644 index 0000000000..9eaea3a62f --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutorModule.java @@ -0,0 +1,39 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.patch; + +import com.google.common.util.concurrent.ThreadFactoryBuilder; +import com.google.inject.AbstractModule; +import com.google.inject.Provides; +import com.google.inject.Singleton; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +/** Module providing the {@link DiffExecutor}. */ +public class DiffExecutorModule extends AbstractModule { + + @Override + protected void configure() { + } + + @Provides + @Singleton + @DiffExecutor + public ExecutorService createDiffExecutor() { + return Executors.newCachedThreadPool(new ThreadFactoryBuilder() + .setNameFormat("Diff-%d").setDaemon(true).build()); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java index 4961befe4d..009b4920a6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.patch; +import com.google.common.base.Throwables; import com.google.common.cache.CacheLoader; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; @@ -29,7 +30,12 @@ import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.regex.Pattern; class IntraLineLoader extends CacheLoader { @@ -41,12 +47,13 @@ class IntraLineLoader extends CacheLoader { private static final Pattern CONTROL_BLOCK_START_RE = Pattern .compile("[{:][ \\t]*$"); - private final IntraLineWorkerPool workerPool; + private final ExecutorService diffExecutor; private final long timeoutMillis; @Inject - IntraLineLoader(IntraLineWorkerPool pool, @GerritServerConfig Config cfg) { - workerPool = pool; + IntraLineLoader(@DiffExecutor ExecutorService diffExecutor, + @GerritServerConfig Config cfg) { + this.diffExecutor = diffExecutor; timeoutMillis = ConfigUtil.getTimeUnit(cfg, "cache", PatchListCacheImpl.INTRA_NAME, "timeout", TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS), @@ -54,27 +61,30 @@ class IntraLineLoader extends CacheLoader { } @Override - public IntraLineDiff load(IntraLineDiffKey key) throws Exception { - IntraLineWorkerPool.Worker w = workerPool.acquire(); - IntraLineWorkerPool.Worker.Result r = w.computeWithTimeout(key, timeoutMillis); - - if (r == IntraLineWorkerPool.Worker.Result.TIMEOUT) { - // Don't keep this thread. We have to murder it unsafely, which - // means its unable to be reused in the future. Return back a - // null result, indicating the cache cannot load this key. - // + public IntraLineDiff load(final IntraLineDiffKey key) throws Exception { + Future result = diffExecutor.submit(new Callable() { + @Override + public IntraLineDiff call() throws Exception { + return IntraLineLoader.compute(key); + } + }); + try { + return result.get(timeoutMillis, TimeUnit.MILLISECONDS); + } catch (InterruptedException | TimeoutException e) { + log.warn(timeoutMillis + " ms timeout reached for IntraLineDiff" + + " in project " + key.getProject().get() + + " on commit " + key.getCommit().name() + + " for path " + key.getPath() + + " comparing " + key.getBlobA().name() + + ".." + key.getBlobB().name()); + result.cancel(true); return new IntraLineDiff(IntraLineDiff.Status.TIMEOUT); - } - workerPool.release(w); - - if (r.error != null) { + } catch (ExecutionException e) { // If there was an error computing the result, carry it // up to the caller so the cache knows this key is invalid. - // - throw r.error; + Throwables.propagateIfInstanceOf(e.getCause(), Exception.class); + throw new Exception(e.getMessage(), e.getCause()); } - - return r.diff; } static IntraLineDiff compute(IntraLineDiffKey key) throws Exception { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java deleted file mode 100644 index 49ae9503ac..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineWorkerPool.java +++ /dev/null @@ -1,182 +0,0 @@ -// Copyright (C) 2009 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -package com.google.gerrit.server.patch; - -import static com.google.gerrit.server.patch.IntraLineLoader.log; - -import com.google.gerrit.server.config.GerritServerConfig; -import com.google.inject.AbstractModule; -import com.google.inject.Inject; -import com.google.inject.Singleton; - -import org.eclipse.jgit.lib.Config; - -import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.BlockingQueue; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; - -@Singleton -public class IntraLineWorkerPool { - public static class Module extends AbstractModule { - @Override - protected void configure() { - bind(IntraLineWorkerPool.class); - } - } - - private final BlockingQueue workerPool; - - @Inject - public IntraLineWorkerPool(@GerritServerConfig Config cfg) { - int workers = cfg.getInt( - "cache", PatchListCacheImpl.INTRA_NAME, "maxIdleWorkers", - Runtime.getRuntime().availableProcessors() * 3 / 2); - workerPool = new ArrayBlockingQueue<>(workers, true /* fair */); - } - - Worker acquire() { - Worker w = workerPool.poll(); - if (w == null) { - // If no worker is immediately available, start a new one. - // Maximum parallelism is controlled by the web server. - w = new Worker(); - w.start(); - } - return w; - } - - void release(Worker w) { - if (!workerPool.offer(w)) { - // If the idle worker pool is full, terminate the worker. - w.shutdownGracefully(); - } - } - - static class Worker extends Thread { - private static final AtomicInteger count = new AtomicInteger(1); - - private final ArrayBlockingQueue input; - private final ArrayBlockingQueue result; - - Worker() { - input = new ArrayBlockingQueue<>(1); - result = new ArrayBlockingQueue<>(1); - - setName("IntraLineDiff-" + count.getAndIncrement()); - setDaemon(true); - } - - Result computeWithTimeout(IntraLineDiffKey key, long timeoutMillis) - throws Exception { - if (!input.offer(new Input(key))) { - log.error("Cannot enqueue task to thread " + getName()); - return Result.TIMEOUT; - } - - Result r = result.poll(timeoutMillis, TimeUnit.MILLISECONDS); - if (r != null) { - return r; - } else { - log.warn(timeoutMillis + " ms timeout reached for IntraLineDiff" - + " in project " + key.getProject().get() - + " on commit " + key.getCommit().name() - + " for path " + key.getPath() - + " comparing " + key.getBlobA().name() - + ".." + key.getBlobB().name() - + ". Killing " + getName()); - forcefullyKillThreadInAnUglyWay(); - return Result.TIMEOUT; - } - } - - @SuppressWarnings("deprecation") - private void forcefullyKillThreadInAnUglyWay() { - try { - stop(); - } catch (Throwable error) { - // Ignore any reason the thread won't stop. - log.error("Cannot stop runaway thread " + getName(), error); - } - } - - private void shutdownGracefully() { - if (!input.offer(Input.END_THREAD)) { - log.error("Cannot gracefully stop thread " + getName()); - } - } - - @Override - public void run() { - try { - for (;;) { - Input in; - try { - in = input.take(); - } catch (InterruptedException e) { - log.error("Unexpected interrupt on " + getName()); - continue; - } - - if (in == Input.END_THREAD) { - return; - } - - Result r; - try { - r = new Result(IntraLineLoader.compute(in.key)); - } catch (Exception error) { - r = new Result(error); - } - - if (!result.offer(r)) { - log.error("Cannot return result from " + getName()); - } - } - } catch (ThreadDeath iHaveBeenShot) { - // Handle thread death by gracefully returning to the caller, - // allowing the thread to be destroyed. - } - } - - private static class Input { - static final Input END_THREAD = new Input(null); - - final IntraLineDiffKey key; - - Input(IntraLineDiffKey key) { - this.key = key; - } - } - - static class Result { - static final Result TIMEOUT = new Result((IntraLineDiff) null); - - final IntraLineDiff diff; - final Exception error; - - Result(IntraLineDiff diff) { - this.diff = diff; - this.error = null; - } - - Result(Exception error) { - this.diff = null; - this.error = error; - } - } - } -} diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java index 6b2f7c7556..d96e86126c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryModule.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.inject.Scopes.SINGLETON; import com.google.common.net.InetAddresses; +import com.google.common.util.concurrent.MoreExecutors; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.DisabledChangeHooks; import com.google.gerrit.reviewdb.client.AuthType; @@ -49,6 +50,7 @@ import com.google.gerrit.server.index.ChangeSchemas; import com.google.gerrit.server.index.IndexModule.IndexType; import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier; import com.google.gerrit.server.mail.SmtpEmailSender; +import com.google.gerrit.server.patch.DiffExecutor; import com.google.gerrit.server.schema.Current; import com.google.gerrit.server.schema.DataSourceType; import com.google.gerrit.server.schema.SchemaCreator; @@ -75,6 +77,7 @@ import java.lang.reflect.Constructor; import java.lang.reflect.InvocationTargetException; import java.net.InetSocketAddress; import java.net.SocketAddress; +import java.util.concurrent.ExecutorService; public class InMemoryModule extends FactoryModule { public static Config newDefaultConfig() { @@ -157,6 +160,18 @@ public class InMemoryModule extends FactoryModule { return CanonicalWebUrlProvider.class; } }); + //Replacement of DiffExecutorModule to not use thread pool in the tests + install(new AbstractModule() { + @Override + protected void configure() { + } + @Provides + @Singleton + @DiffExecutor + public ExecutorService createDiffExecutor() { + return MoreExecutors.sameThreadExecutor(); + } + }); install(new DefaultCacheFactory.Module()); install(new SmtpEmailSender.Module()); install(new SignedTokenEmailTokenVerifier.Module()); diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index e9bd296ea9..f59401c47f 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -46,7 +46,7 @@ import com.google.gerrit.server.git.WorkQueue; import com.google.gerrit.server.index.IndexModule; import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier; import com.google.gerrit.server.mail.SmtpEmailSender; -import com.google.gerrit.server.patch.IntraLineWorkerPool; +import com.google.gerrit.server.patch.DiffExecutorModule; import com.google.gerrit.server.plugins.PluginGuiceEnvironment; import com.google.gerrit.server.plugins.PluginRestApiModule; import com.google.gerrit.server.schema.DataSourceModule; @@ -280,7 +280,7 @@ public class WebAppInitializer extends GuiceServletContextListener modules.add(new ChangeHookRunner.Module()); modules.add(new ReceiveCommitsExecutorModule()); modules.add(new MergeabilityChecksExecutorModule()); - modules.add(new IntraLineWorkerPool.Module()); + modules.add(new DiffExecutorModule()); modules.add(cfgInjector.getInstance(GerritGlobalModule.class)); modules.add(new InternalAccountDirectory.Module()); modules.add(new DefaultCacheFactory.Module()); From c33a9108e82ca07a7e006ffa5117492e0866cc53 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 23 Mar 2015 19:50:47 +0000 Subject: [PATCH 09/36] Update replication plugin - Fix replication_log with external log4j.configuration Change-Id: I804b0eba6ff48d62be9a61f5153f756df8bdab9a --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index eded6451be..90c23d25bd 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit eded6451be8dc10945c110955d2bfcfe629eac32 +Subproject commit 90c23d25bdef78bd7f5639cedf9a4790e0437ebe From 9eb14549a03d83a300b779fa0dc2ed2c80605854 Mon Sep 17 00:00:00 2001 From: Dariusz Luksza Date: Fri, 20 Mar 2015 20:31:34 +0100 Subject: [PATCH 10/36] Bind SecureStore and SecureStoreClassName in WebAppInitializer Bug: Issue 3255 Change-Id: I71265720be581dc961d717096e44a84c6858ba55 --- .../google/gerrit/pgm/util/SiteProgram.java | 22 +-------- .../config/GerritServerConfigModule.java | 47 +++++++++++++++++++ .../gerrit/httpd/WebAppInitializer.java | 22 ++++++++- 3 files changed, 69 insertions(+), 22 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java index 02a8eac46e..7e0aa1e53b 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java @@ -14,6 +14,7 @@ package com.google.gerrit.pgm.util; +import static com.google.gerrit.server.config.GerritServerConfigModule.getSecureStoreClassName; import static com.google.inject.Scopes.SINGLETON; import static com.google.inject.Stage.PRODUCTION; @@ -188,26 +189,7 @@ public abstract class SiteProgram extends AbstractProgram { } protected final String getConfiguredSecureStoreClass() { - Module m = new AbstractModule() { - @Override - protected void configure() { - bind(File.class).annotatedWith(SitePath.class).toInstance(sitePath); - bind(SitePaths.class); - } - }; - Injector i = Guice.createInjector(m); - SitePaths site = i.getInstance(SitePaths.class); - FileBasedConfig cfg = new FileBasedConfig(site.gerrit_config, FS.DETECTED); - if (!cfg.getFile().exists()) { - return null; - } - - try { - cfg.load(); - return cfg.getString("gerrit", null, "secureStoreClass"); - } catch (IOException | ConfigInvalidException e) { - throw new ProvisionException(e.getMessage(), e); - } + return getSecureStoreClassName(sitePath); } private String getDbType(Provider dsProvider) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigModule.java index 2683ca1229..89331fd381 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritServerConfigModule.java @@ -16,14 +16,61 @@ package com.google.gerrit.server.config; import static com.google.inject.Scopes.SINGLETON; +import com.google.gerrit.server.securestore.DefaultSecureStore; import com.google.gerrit.server.securestore.SecureStore; import com.google.gerrit.server.securestore.SecureStoreProvider; import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.google.inject.ProvisionException; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.util.FS; + +import java.io.File; +import java.io.IOException; /** Creates {@link GerritServerConfig}. */ public class GerritServerConfigModule extends AbstractModule { + public static String getSecureStoreClassName(final File sitePath) { + if (sitePath != null) { + return getSecureStoreFromGerritConfig(sitePath); + } + + String secureStoreProperty = System.getProperty("gerrit.secure_store_class"); + return nullToDefault(secureStoreProperty); + } + + private static String getSecureStoreFromGerritConfig(final File sitePath) { + AbstractModule m = new AbstractModule() { + @Override + protected void configure() { + bind(File.class).annotatedWith(SitePath.class).toInstance(sitePath); + bind(SitePaths.class); + } + }; + Injector injector = Guice.createInjector(m); + SitePaths site = injector.getInstance(SitePaths.class); + FileBasedConfig cfg = new FileBasedConfig(site.gerrit_config, FS.DETECTED); + if (!cfg.getFile().exists()) { + return DefaultSecureStore.class.getName(); + } + + try { + cfg.load(); + String className = cfg.getString("gerrit", null, "secureStoreClass"); + return nullToDefault(className); + } catch (IOException | ConfigInvalidException e) { + throw new ProvisionException(e.getMessage(), e); + } + } + + private static String nullToDefault(String className) { + return className != null ? className : DefaultSecureStore.class.getName(); + } + @Override protected void configure() { bind(SitePaths.class); diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index a395a3b203..f0ad01dccd 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -56,6 +56,9 @@ import com.google.gerrit.server.schema.DataSourceType; import com.google.gerrit.server.schema.DatabaseModule; import com.google.gerrit.server.schema.SchemaModule; import com.google.gerrit.server.schema.SchemaVersionCheck; +import com.google.gerrit.server.securestore.SecureStore; +import com.google.gerrit.server.securestore.SecureStoreClassName; +import com.google.gerrit.server.securestore.SecureStoreProvider; import com.google.gerrit.server.ssh.NoSshModule; import com.google.gerrit.server.ssh.SshAddressesModule; import com.google.gerrit.solr.SolrIndexModule; @@ -74,6 +77,7 @@ import com.google.inject.name.Names; import com.google.inject.servlet.GuiceFilter; import com.google.inject.servlet.GuiceServletContextListener; import com.google.inject.spi.Message; +import com.google.inject.util.Providers; import org.eclipse.jgit.lib.Config; import org.slf4j.Logger; @@ -206,6 +210,7 @@ public class WebAppInitializer extends GuiceServletContextListener private Injector createDbInjector() { final List modules = new ArrayList<>(); + AbstractModule secureStore = createSecureStoreModule(); if (sitePath != null) { Module sitePathModule = new AbstractModule() { @Override @@ -218,13 +223,13 @@ public class WebAppInitializer extends GuiceServletContextListener Module configModule = new GerritServerConfigModule(); modules.add(configModule); - Injector cfgInjector = Guice.createInjector(sitePathModule, configModule); + Injector cfgInjector = Guice.createInjector(sitePathModule, configModule, secureStore); Config cfg = cfgInjector.getInstance(Key.get(Config.class, GerritServerConfig.class)); String dbType = cfg.getString("database", null, "type"); final DataSourceType dst = Guice.createInjector(new DataSourceModule(), - configModule, sitePathModule).getInstance( + configModule, sitePathModule, secureStore).getInstance( Key.get(DataSourceType.class, Names.named(dbType.toLowerCase()))); modules.add(new LifecycleModule() { @Override @@ -239,6 +244,7 @@ public class WebAppInitializer extends GuiceServletContextListener }); } else { + modules.add(secureStore); modules.add(new LifecycleModule() { @Override protected void configure() { @@ -375,4 +381,16 @@ public class WebAppInitializer extends GuiceServletContextListener manager = null; } } + + private AbstractModule createSecureStoreModule() { + return new AbstractModule() { + @Override + public void configure() { + String secureStoreClassName = + GerritServerConfigModule.getSecureStoreClassName(sitePath); + bind(String.class).annotatedWith(SecureStoreClassName.class).toProvider( + Providers.of(secureStoreClassName)); + } + }; + } } From 61074ca4fbe7881212c6a921fb843378ed4fb124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 9 Mar 2015 15:20:28 -0400 Subject: [PATCH 11/36] Work around MyersDiff infinite loop in PatchListLoader This infinite loop is happening for some files when the PatchListLoader is computing the differences between 2 commits. It first showed up[1] in the mergeability checks done in background and was easy to work around by killing the thread using Javamelody and abandoning the faulty commits. The issue showed up again, when upgrading from 2.9.x to 2.10: the online reindexer getting stuck because of that infinite loop and this time, no easy work around. Use a similar approach that was done in intraline diff to work around the MyersDiff infinite loop. Instead of returning a timeout error message when the infinite loop is detected, fallback to a diff algorithm that does not use MyersDiff. Returning a timeout error was not an option because the failing operation is not always triggered by a user. From the user perspective, the only difference when the infinite loop is detected is that the files in the commit will not be compared in-depth, which will result in bigger edit regions. [1]https://groups.google.com/d/msg/repo-discuss/ZtiCilM3wFA/LijfZ4YkLHsJ Change-Id: Ib00de070dd8df1722d4ade0a83c0ffa8eaa37f8e --- Documentation/config-gerrit.txt | 19 ++++++ .../java/com/google/gerrit/pgm/Reindex.java | 2 + .../gerrit/server/patch/DiffExecutor.java | 2 +- .../server/patch/PatchListCacheImpl.java | 2 +- .../gerrit/server/patch/PatchListLoader.java | 62 ++++++++++++++++++- 5 files changed, 82 insertions(+), 5 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index f174b28229..3368db9f58 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -706,6 +706,25 @@ See also link:cmd-flush-caches.html[gerrit flush-caches]. ==== [[cache_options]]Cache Options +[[cache.diff.timeout]]cache.diff.timeout:: ++ +Maximum number of milliseconds to wait for diff data before giving up and +falling back on a simpler diff algorithm that will not be able to break down +modified regions into smaller ones. This is a work around for an infinite loop +bug in the default difference algorithm implementation. ++ +Values should use common unit suffixes to express their setting: ++ +* ms, milliseconds +* s, sec, second, seconds +* m, min, minute, minutes +* h, hr, hour, hours + ++ +If a unit suffix is not specified, `milliseconds` is assumed. ++ +Default is 5 seconds. + [[cache.diff_intraline.timeout]]cache.diff_intraline.timeout:: + Maximum number of milliseconds to wait for intraline difference data diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java index c0cf8f9222..161efc1ed6 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/Reindex.java @@ -70,6 +70,7 @@ import com.google.gerrit.server.index.IndexModule; import com.google.gerrit.server.index.IndexModule.IndexType; import com.google.gerrit.server.mail.ReplacePatchSetSender; import com.google.gerrit.server.notedb.NoteDbModule; +import com.google.gerrit.server.patch.DiffExecutorModule; import com.google.gerrit.server.patch.PatchListCacheImpl; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.CommentLinkInfo; @@ -191,6 +192,7 @@ public class Reindex extends SiteProgram { private Injector createSysInjector() { List modules = Lists.newArrayList(); + modules.add(new DiffExecutorModule()); modules.add(PatchListCacheImpl.module()); AbstractModule changeIndexModule; switch (IndexModule.getIndexType(dbInjector)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java index 23589e3e3a..564ca58453 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffExecutor.java @@ -23,7 +23,7 @@ import java.util.concurrent.ExecutorService; /** * Marker on {@link ExecutorService} used by - * {@link IntraLineLoader}. + * {@link IntraLineLoader} and {@link PatchListLoader}. */ @Retention(RUNTIME) @BindingAnnotation diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index 7b7c73108a..6c769f742a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -35,7 +35,7 @@ import java.util.concurrent.ExecutionException; /** Provides a cached list of {@link PatchListEntry}. */ @Singleton public class PatchListCacheImpl implements PatchListCache { - private static final String FILE_NAME = "diff"; + static final String FILE_NAME = "diff"; static final String INTRA_NAME = "diff_intraline"; public static Module module() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index de36020d85..d9e796958c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -16,11 +16,13 @@ package com.google.gerrit.server.patch; import com.google.common.base.Function; +import com.google.common.base.Throwables; import com.google.common.cache.CacheLoader; import com.google.common.collect.FluentIterable; import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; @@ -69,6 +71,12 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; public class PatchListLoader extends CacheLoader { static final Logger log = LoggerFactory.getLogger(PatchListLoader.class); @@ -76,13 +84,23 @@ public class PatchListLoader extends CacheLoader { private final GitRepositoryManager repoManager; private final PatchListCache patchListCache; private final ThreeWayMergeStrategy mergeStrategy; + private final ExecutorService diffExecutor; + private final long timeoutMillis; + @Inject - PatchListLoader(GitRepositoryManager mgr, PatchListCache plc, - @GerritServerConfig Config cfg) { + PatchListLoader(GitRepositoryManager mgr, + PatchListCache plc, + @GerritServerConfig Config cfg, + @DiffExecutor ExecutorService de) { repoManager = mgr; patchListCache = plc; mergeStrategy = MergeUtil.getMergeStrategy(cfg); + diffExecutor = de; + timeoutMillis = + ConfigUtil.getTimeUnit(cfg, "cache", PatchListCacheImpl.FILE_NAME, + "timeout", TimeUnit.MILLISECONDS.convert(5, TimeUnit.SECONDS), + TimeUnit.MILLISECONDS); } @Override @@ -164,7 +182,7 @@ public class PatchListLoader extends CacheLoader { DiffEntry diffEntry = diffEntries.get(i); if (paths == null || paths.contains(diffEntry.getNewPath()) || paths.contains(diffEntry.getOldPath())) { - FileHeader fh = df.toFileHeader(diffEntry); + FileHeader fh = toFileHeader(key, df, diffEntry); entries.add(newEntry(aTree, fh)); } } @@ -175,6 +193,44 @@ public class PatchListLoader extends CacheLoader { } } + private FileHeader toFileHeader(PatchListKey key, + final DiffFormatter diffFormatter, final DiffEntry diffEntry) + throws IOException { + + Future result = diffExecutor.submit(new Callable() { + @Override + public FileHeader call() throws IOException { + return diffFormatter.toFileHeader(diffEntry); + } + }); + + try { + return result.get(timeoutMillis, TimeUnit.MILLISECONDS); + } catch (InterruptedException | TimeoutException e) { + log.warn(timeoutMillis + " ms timeout reached for Diff loader" + + " in project " + key.projectKey.get() + + " on commit " + key.getNewId().name() + + " on path " + diffEntry.getNewPath() + + " comparing " + diffEntry.getOldId().name() + + ".." + diffEntry.getNewId().name()); + result.cancel(true); + return toFileHeaderWithoutMyersDiff(diffFormatter, diffEntry); + } catch (ExecutionException e) { + // If there was an error computing the result, carry it + // up to the caller so the cache knows this key is invalid. + Throwables.propagateIfInstanceOf(e.getCause(), IOException.class); + throw new IOException(e.getMessage(), e.getCause()); + } + } + + private FileHeader toFileHeaderWithoutMyersDiff(DiffFormatter diffFormatter, + DiffEntry diffEntry) throws IOException { + HistogramDiff histogramDiff = new HistogramDiff(); + histogramDiff.setFallbackAlgorithm(null); + diffFormatter.setDiffAlgorithm(histogramDiff); + return diffFormatter.toFileHeader(diffEntry); + } + private PatchListEntry newCommitMessage(final RawTextComparator cmp, final Repository db, final ObjectReader reader, final RevCommit aCommit, final RevCommit bCommit) throws IOException { From c89b7599edcf32ae73806c20a22dbc0c55dde58f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Wed, 25 Mar 2015 13:50:42 +0100 Subject: [PATCH 12/36] OnlineReindexer: log the success/failure numbers on exit When the OnlineReindexer fails to reindex all changes it will not activate the new index schema version. However, it may happen that only there were only a few failures and that activating the new index schema makes sense. Help the admins make the decision whether to activate the new index schema version by logging the success/failure numbers in the error_log. Change-Id: I522e236ab1e9b60d5c3c7c215d8308972db45f70 --- .../main/java/com/google/gerrit/lucene/OnlineReindexer.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/OnlineReindexer.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/OnlineReindexer.java index d3dc963f80..41c83ebe7c 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/OnlineReindexer.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/OnlineReindexer.java @@ -79,7 +79,9 @@ public class OnlineReindexer { ChangeBatchIndexer.Result result = batchIndexer.indexAll( index, projectCache.all(), -1, -1, null, null); if (!result.success()) { - log.error("Online reindex of schema version {} failed", version(index)); + log.error("Online reindex of schema version {} failed. Successfully" + + " indexed {} changes, failed to index {} changes", + version(index), result.doneCount(), result.failedCount()); return; } From 3408d4deb866a54580f836ff109f3ef6ba940d45 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 25 Mar 2015 08:05:37 +0000 Subject: [PATCH 13/36] Remove unused OAuthToken in authorisation URL When the user needs to be redirected to the OAuth authentication URL for entering their credentials, the session is not active yet and there is no OAuthToken available. There is no value then in having a RequestToken parameter that will always be null anyway. Change-Id: I00fdbd32923a51e0c92e6bc0efff551936ec344f --- .../auth/oauth/OAuthServiceProvider.java | 15 +++------------ .../gerrit/httpd/auth/oauth/OAuthSession.java | 5 ++--- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthServiceProvider.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthServiceProvider.java index 8375e3179e..9be2630792 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthServiceProvider.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/auth/oauth/OAuthServiceProvider.java @@ -22,30 +22,21 @@ import java.io.IOException; @ExtensionPoint public interface OAuthServiceProvider { - /** - * Retrieve the request token. - * - * @return request token - */ - OAuthToken getRequestToken(); - /** * Returns the URL where you should redirect your users to authenticate * your application. * - * @param requestToken the request token you need to authorize - * @return the URL where you should redirect your users + * @return the OAuth service URL to redirect your users for authentication */ - String getAuthorizationUrl(OAuthToken requestToken); + String getAuthorizationUrl(); /** * Retrieve the access token * - * @param requestToken request token (obtained previously) * @param verifier verifier code * @return access token */ - OAuthToken getAccessToken(OAuthToken requestToken, OAuthVerifier verifier); + OAuthToken getAccessToken(OAuthVerifier verifier); /** * After establishing of secure communication channel, this method supossed to diff --git a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java index d625e02abd..3eba6d9492 100644 --- a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java +++ b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java @@ -87,8 +87,7 @@ class OAuthSession { } log.debug("Login-Retrieve-User " + this); - token = oauth.getAccessToken(null, - new OAuthVerifier(request.getParameter("code"))); + token = oauth.getAccessToken(new OAuthVerifier(request.getParameter("code"))); user = oauth.getUserInfo(token); @@ -103,7 +102,7 @@ class OAuthSession { } else { log.debug("Login-PHASE1 " + this); redirectUrl = request.getRequestURI(); - response.sendRedirect(oauth.getAuthorizationUrl(null) + + response.sendRedirect(oauth.getAuthorizationUrl() + "&state=" + state); return false; } From 7273df290a812f28c28e2fbd3dcaccb1e34b4d76 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 10 Mar 2015 16:10:59 -0700 Subject: [PATCH 14/36] Convert to new AutoCloseable instances coming in JGit 4.0 Repository, RevWalk, and friends have been converted to AutoCloseable. Use them in try-with-resources blocks in order to avoid warnings about unclosed resources or calls to deprecated release() methods. Where a larger rewrite might have been possible to reduce try/finally blocks, err on the side of keeping the same behavior. The proximate goal of this change was to eliminate all Eclipse warnings about deprecated method calls (namely release()) and leaked resources. Some of these warnings were in fact potential leaks; Eclipse finding such cases is a side benefit of having these classes implement AutoCloseable. However, this change certainly does not cover all cases where try-with-resources could have been used, where it was not causing a leak. Fixes all such warnings in plugin submodules as well. Change-Id: I5d151996ae012d0e8fdfa27cce8cf5e2bfc856a2 --- .../gerrit/acceptance/git/SubmitOnPushIT.java | 54 +-- .../rest/change/AbstractSubmit.java | 61 +-- .../rest/project/CreateProjectIT.java | 12 +- .../google/gerrit/httpd/raw/CatServlet.java | 41 +- .../com/google/gerrit/pgm/RebuildNotedb.java | 5 +- .../pgm/init/api/AllProjectsConfig.java | 56 ++- .../com/google/gerrit/server/ChangeUtil.java | 336 +++++++-------- .../server/change/ChangeKindCacheImpl.java | 4 +- .../server/change/CherryPickChange.java | 160 ++++--- .../server/change/ConsistencyChecker.java | 2 +- .../gerrit/server/change/CreateChange.java | 123 +++--- .../gerrit/server/change/FileContentUtil.java | 89 ++-- .../google/gerrit/server/change/Files.java | 160 ++++--- .../gerrit/server/change/GetArchive.java | 4 +- .../google/gerrit/server/change/GetPatch.java | 13 +- .../gerrit/server/change/GetRelated.java | 17 +- .../gerrit/server/change/IncludedIn.java | 32 +- .../server/change/MergeabilityCacheImpl.java | 5 +- .../server/changedetail/RebaseChange.java | 21 +- .../server/edit/ChangeEditModifier.java | 182 ++++---- .../gerrit/server/edit/ChangeEditUtil.java | 38 +- .../google/gerrit/server/git/BanCommit.java | 66 ++- .../com/google/gerrit/server/git/MergeOp.java | 4 +- .../gerrit/server/git/NotesBranchUtil.java | 4 +- .../google/gerrit/server/git/SubmoduleOp.java | 7 +- .../com/google/gerrit/server/git/TagSet.java | 9 +- .../gerrit/server/git/VersionedMetaData.java | 7 +- .../gerrit/server/index/SiteIndexer.java | 46 +- .../gerrit/server/mail/ChangeEmail.java | 40 +- .../mail/PatchSetNotificationSender.java | 10 +- .../gerrit/server/notedb/ChangeNotes.java | 8 +- .../gerrit/server/notedb/ChangeRebuilder.java | 5 +- .../server/notedb/DraftCommentNotes.java | 8 +- .../google/gerrit/server/patch/PatchFile.java | 7 +- .../gerrit/server/patch/PatchListLoader.java | 19 +- .../server/patch/PatchScriptBuilder.java | 9 +- .../server/patch/PatchSetInfoFactory.java | 17 +- .../com/google/gerrit/server/patch/Text.java | 61 +-- .../server/project/CommitsCollection.java | 29 +- .../gerrit/server/project/DeleteBranches.java | 5 +- .../google/gerrit/server/project/GetHead.java | 13 +- .../gerrit/server/project/ListDashboards.java | 68 ++- .../gerrit/server/project/ListTags.java | 29 +- .../server/project/PerformCreateProject.java | 5 +- .../server/query/change/ChangeData.java | 17 +- .../query/change/ConflictsPredicate.java | 47 +- .../server/query/change/RevWalkPredicate.java | 15 +- .../main/java/gerrit/PRED_commit_edits_2.java | 13 +- .../server/change/IncludedInResolverTest.java | 4 +- .../gerrit/server/git/SubmoduleOpTest.java | 400 +++++++++--------- .../server/notedb/ChangeNotesParserTest.java | 7 +- .../gerrit/server/notedb/ChangeNotesTest.java | 142 +++---- .../notedb/CommitMessageOutputTest.java | 5 +- .../server/tools/hooks/CommitMsgHookTest.java | 10 +- plugins/reviewnotes | 2 +- 55 files changed, 1107 insertions(+), 1446 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java index 790d3c58d4..c7958ccd9e 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java @@ -222,53 +222,37 @@ public class SubmitOnPushIT extends AbstractDaemonTest { } private void assertCommit(Project.NameKey project, String branch) throws IOException { - Repository r = repoManager.openRepository(project); - try { - RevWalk rw = new RevWalk(r); - try { - RevCommit c = rw.parseCommit(r.getRef(branch).getObjectId()); - assertThat(c.getShortMessage()).isEqualTo(PushOneCommit.SUBJECT); - assertThat(c.getAuthorIdent().getEmailAddress()).isEqualTo(admin.email); - assertThat(c.getCommitterIdent().getEmailAddress()).isEqualTo( - admin.email); - } finally { - rw.release(); - } - } finally { - r.close(); + try (Repository r = repoManager.openRepository(project); + RevWalk rw = new RevWalk(r)) { + RevCommit c = rw.parseCommit(r.getRef(branch).getObjectId()); + assertThat(c.getShortMessage()).isEqualTo(PushOneCommit.SUBJECT); + assertThat(c.getAuthorIdent().getEmailAddress()).isEqualTo(admin.email); + assertThat(c.getCommitterIdent().getEmailAddress()).isEqualTo( + admin.email); } } private void assertMergeCommit(String branch, String subject) throws IOException { - Repository r = repoManager.openRepository(project); - try { - RevWalk rw = new RevWalk(r); - try { - RevCommit c = rw.parseCommit(r.getRef(branch).getObjectId()); - assertThat(c.getParentCount()).is(2); - assertThat(c.getShortMessage()).isEqualTo("Merge \"" + subject + "\""); - assertThat(c.getAuthorIdent().getEmailAddress()).isEqualTo(admin.email); - assertThat(c.getCommitterIdent().getEmailAddress()).isEqualTo( - serverIdent.getEmailAddress()); - } finally { - rw.release(); - } - } finally { - r.close(); + try (Repository r = repoManager.openRepository(project); + RevWalk rw = new RevWalk(r)) { + RevCommit c = rw.parseCommit(r.getRef(branch).getObjectId()); + assertThat(c.getParentCount()).is(2); + assertThat(c.getShortMessage()).isEqualTo("Merge \"" + subject + "\""); + assertThat(c.getAuthorIdent().getEmailAddress()).isEqualTo(admin.email); + assertThat(c.getCommitterIdent().getEmailAddress()).isEqualTo( + serverIdent.getEmailAddress()); } } private void assertTag(Project.NameKey project, String branch, PushOneCommit.Tag tag) throws IOException { - Repository repo = repoManager.openRepository(project); - try { + try (Repository repo = repoManager.openRepository(project)) { Ref tagRef = repo.getRef(tag.name); assertThat(tagRef).isNotNull(); ObjectId taggedCommit = null; if (tag instanceof PushOneCommit.AnnotatedTag) { PushOneCommit.AnnotatedTag annotatedTag = (PushOneCommit.AnnotatedTag)tag; - RevWalk rw = new RevWalk(repo); - try { + try (RevWalk rw = new RevWalk(repo)) { RevObject object = rw.parseAny(tagRef.getObjectId()); assertThat(object).isInstanceOf(RevTag.class); RevTag tagObject = (RevTag) object; @@ -276,8 +260,6 @@ public class SubmitOnPushIT extends AbstractDaemonTest { .isEqualTo(annotatedTag.message); assertThat(tagObject.getTaggerIdent()).isEqualTo(annotatedTag.tagger); taggedCommit = tagObject.getObject(); - } finally { - rw.dispose(); } } else { taggedCommit = tagRef.getObjectId(); @@ -285,8 +267,6 @@ public class SubmitOnPushIT extends AbstractDaemonTest { ObjectId headCommit = repo.getRef(branch).getObjectId(); assertThat(taggedCommit).isNotNull(); assertThat(taggedCommit).isEqualTo(headCommit); - } finally { - repo.close(); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 66911b8710..dec3e6547d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -337,40 +337,23 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } protected RevCommit getRemoteHead() throws IOException { - Repository repo = repoManager.openRepository(project); - try { + try (Repository repo = repoManager.openRepository(project)) { return getHead(repo, "refs/heads/master"); - } finally { - repo.close(); } } protected List getRemoteLog() throws IOException { - Repository repo = repoManager.openRepository(project); - try { - RevWalk rw = new RevWalk(repo); - try { - rw.markStart(rw.parseCommit( - repo.getRef("refs/heads/master").getObjectId())); - return Lists.newArrayList(rw); - } finally { - rw.release(); - } - } finally { - repo.close(); + try (Repository repo = repoManager.openRepository(project); + RevWalk rw = new RevWalk(repo)) { + rw.markStart(rw.parseCommit( + repo.getRef("refs/heads/master").getObjectId())); + return Lists.newArrayList(rw); } } private RevCommit getHead(Repository repo, String name) throws IOException { - try { - RevWalk rw = new RevWalk(repo); - try { - return rw.parseCommit(repo.getRef(name).getObjectId()); - } finally { - rw.release(); - } - } finally { - repo.close(); + try (RevWalk rw = new RevWalk(repo)) { + return rw.parseCommit(repo.getRef(name).getObjectId()); } } @@ -381,28 +364,22 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } private String getLatestRemoteDiff() throws IOException { - Repository repo = repoManager.openRepository(project); - try { - RevWalk rw = new RevWalk(repo); - try { - ObjectId oldTreeId = repo.resolve("refs/heads/master~1^{tree}"); - ObjectId newTreeId = repo.resolve("refs/heads/master^{tree}"); - return getLatestDiff(repo, oldTreeId, newTreeId); - } finally { - rw.release(); - } - } finally { - repo.close(); + try (Repository repo = repoManager.openRepository(project); + RevWalk rw = new RevWalk(repo)) { + ObjectId oldTreeId = repo.resolve("refs/heads/master~1^{tree}"); + ObjectId newTreeId = repo.resolve("refs/heads/master^{tree}"); + return getLatestDiff(repo, oldTreeId, newTreeId); } } private String getLatestDiff(Repository repo, ObjectId oldTreeId, ObjectId newTreeId) throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); - DiffFormatter fmt = new DiffFormatter(out); - fmt.setRepository(repo); - fmt.format(oldTreeId, newTreeId); - fmt.flush(); - return out.toString(); + try (DiffFormatter fmt = new DiffFormatter(out)) { + fmt.setRepository(repo); + fmt.format(oldTreeId, newTreeId); + fmt.flush(); + return out.toString(); + } } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java index 998c5ea450..68c9a5e894 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateProjectIT.java @@ -238,11 +238,10 @@ public class CreateProjectIT extends AbstractDaemonTest { private void assertEmptyCommit(String projectName, String... refs) throws RepositoryNotFoundException, IOException { - Repository repo = - repoManager.openRepository(new Project.NameKey(projectName)); - RevWalk rw = new RevWalk(repo); - TreeWalk tw = new TreeWalk(repo); - try { + Project.NameKey projectKey = new Project.NameKey(projectName); + try (Repository repo = repoManager.openRepository(projectKey); + RevWalk rw = new RevWalk(repo); + TreeWalk tw = new TreeWalk(rw.getObjectReader())) { for (String ref : refs) { RevCommit commit = rw.lookupCommit(repo.getRef(ref).getObjectId()); rw.parseBody(commit); @@ -250,9 +249,6 @@ public class CreateProjectIT extends AbstractDaemonTest { assertThat(tw.next()).isFalse(); tw.reset(); } - } finally { - rw.release(); - repo.close(); } } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/CatServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/CatServlet.java index dddcd67353..72fc008d75 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/CatServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/CatServlet.java @@ -202,32 +202,29 @@ public class CatServlet extends HttpServlet { final RevCommit fromCommit; final String suffix; final String path = patchKey.getFileName(); - try { - final ObjectReader reader = repo.newObjectReader(); - try { - final RevWalk rw = new RevWalk(reader); - final RevCommit c; - final TreeWalk tw; + try (ObjectReader reader = repo.newObjectReader(); + RevWalk rw = new RevWalk(reader)) { + final RevCommit c; - c = rw.parseCommit(ObjectId.fromString(revision)); - if (side == 0) { - fromCommit = c; - suffix = "new"; - - } else if (1 <= side && side - 1 < c.getParentCount()) { - fromCommit = rw.parseCommit(c.getParent(side - 1)); - if (c.getParentCount() == 1) { - suffix = "old"; - } else { - suffix = "old" + side; - } + c = rw.parseCommit(ObjectId.fromString(revision)); + if (side == 0) { + fromCommit = c; + suffix = "new"; + } else if (1 <= side && side - 1 < c.getParentCount()) { + fromCommit = rw.parseCommit(c.getParent(side - 1)); + if (c.getParentCount() == 1) { + suffix = "old"; } else { - rsp.sendError(HttpServletResponse.SC_NOT_FOUND); - return; + suffix = "old" + side; } - tw = TreeWalk.forPath(reader, path, fromCommit.getTree()); + } else { + rsp.sendError(HttpServletResponse.SC_NOT_FOUND); + return; + } + + try (TreeWalk tw = TreeWalk.forPath(reader, path, fromCommit.getTree())) { if (tw == null) { rsp.sendError(HttpServletResponse.SC_NOT_FOUND); return; @@ -240,8 +237,6 @@ public class CatServlet extends HttpServlet { rsp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); return; } - } finally { - reader.release(); } } catch (IOException e) { getServletContext().log("Cannot read repository", e); diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java index 88115b6172..ab20f531f5 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNotedb.java @@ -185,11 +185,8 @@ public class RebuildNotedb extends SiteProgram { private static void execute(BatchRefUpdate bru, Repository repo) throws IOException { - RevWalk rw = new RevWalk(repo); - try { + try (RevWalk rw = new RevWalk(repo)) { bru.execute(rw, NullProgressMonitor.INSTANCE); - } finally { - rw.release(); } } diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java index dda536db6c..f45a970c67 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java @@ -136,49 +136,41 @@ public class AllProjectsConfig extends VersionedMetaData { throw new IOException("All-Projects does not exist."); } - Repository repo = new FileRepository(path); - try { + try (Repository repo = new FileRepository(path)) { inserter = repo.newObjectInserter(); reader = repo.newObjectReader(); - try { - RevWalk rw = new RevWalk(reader); - try { - RevTree srcTree = revision != null ? rw.parseTree(revision) : null; - newTree = readTree(srcTree); - saveConfig(ProjectConfig.PROJECT_CONFIG, cfg); - saveGroupList(); - ObjectId res = newTree.writeTree(inserter); - if (res.equals(srcTree)) { - // If there are no changes to the content, don't create the commit. - return; - } - - CommitBuilder commit = new CommitBuilder(); - commit.setAuthor(ident); - commit.setCommitter(ident); - commit.setMessage(msg); - commit.setTreeId(res); - if (revision != null) { - commit.addParentId(revision); - } - ObjectId newRevision = inserter.insert(commit); - updateRef(repo, ident, newRevision, "commit: " + msg); - revision = newRevision; - } finally { - rw.release(); + try (RevWalk rw = new RevWalk(reader)) { + RevTree srcTree = revision != null ? rw.parseTree(revision) : null; + newTree = readTree(srcTree); + saveConfig(ProjectConfig.PROJECT_CONFIG, cfg); + saveGroupList(); + ObjectId res = newTree.writeTree(inserter); + if (res.equals(srcTree)) { + // If there are no changes to the content, don't create the commit. + return; } + + CommitBuilder commit = new CommitBuilder(); + commit.setAuthor(ident); + commit.setCommitter(ident); + commit.setMessage(msg); + commit.setTreeId(res); + if (revision != null) { + commit.addParentId(revision); + } + ObjectId newRevision = inserter.insert(commit); + updateRef(repo, ident, newRevision, "commit: " + msg); + revision = newRevision; } finally { if (inserter != null) { - inserter.release(); + inserter.close(); inserter = null; } if (reader != null) { - reader.release(); + reader.close(); reader = null; } } - } finally { - repo.close(); } // we need to invalidate the JGit cache if the group list is invalidated in diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 93bbd7b4e7..9faa516995 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetAncestor; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.change.ChangeInserter; @@ -227,119 +228,107 @@ public class ChangeUtil { } Change changeToRevert = db.get().changes().get(changeId); - Repository git; - try { - git = gitManager.openRepository(ctl.getChange().getProject()); + Project.NameKey project = ctl.getChange().getProject(); + try (Repository git = gitManager.openRepository(project); + RevWalk revWalk = new RevWalk(git)) { + RevCommit commitToRevert = + revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get())); + + PersonIdent authorIdent = + user().newCommitterIdent(myIdent.getWhen(), myIdent.getTimeZone()); + + RevCommit parentToCommitToRevert = commitToRevert.getParent(0); + revWalk.parseHeaders(parentToCommitToRevert); + + CommitBuilder revertCommitBuilder = new CommitBuilder(); + revertCommitBuilder.addParentId(commitToRevert); + revertCommitBuilder.setTreeId(parentToCommitToRevert.getTree()); + revertCommitBuilder.setAuthor(authorIdent); + revertCommitBuilder.setCommitter(authorIdent); + + if (message == null) { + message = MessageFormat.format( + ChangeMessages.get().revertChangeDefaultMessage, + changeToRevert.getSubject(), patch.getRevision().get()); + } + + ObjectId computedChangeId = + ChangeIdUtil.computeChangeId(parentToCommitToRevert.getTree(), + commitToRevert, authorIdent, myIdent, message); + revertCommitBuilder.setMessage( + ChangeIdUtil.insertId(message, computedChangeId, true)); + + RevCommit revertCommit; + try (ObjectInserter oi = git.newObjectInserter()) { + ObjectId id = oi.insert(revertCommitBuilder); + oi.flush(); + revertCommit = revWalk.parseCommit(id); + } + + RefControl refControl = ctl.getRefControl(); + Change change = new Change( + new Change.Key("I" + computedChangeId.name()), + new Change.Id(db.get().nextChangeId()), + user().getAccountId(), + changeToRevert.getDest(), + TimeUtil.nowTs()); + change.setTopic(changeToRevert.getTopic()); + ChangeInserter ins = + changeInserterFactory.create(refControl.getProjectControl(), + change, revertCommit); + PatchSet ps = ins.getPatchSet(); + + String ref = refControl.getRefName(); + String cmdRef = MagicBranch.NEW_PUBLISH_CHANGE + + ref.substring(ref.lastIndexOf('/') + 1); + CommitReceivedEvent commitReceivedEvent = new CommitReceivedEvent( + new ReceiveCommand(ObjectId.zeroId(), revertCommit.getId(), cmdRef), + refControl.getProjectControl().getProject(), + refControl.getRefName(), revertCommit, user()); + + try { + commitValidatorsFactory.create(refControl, sshInfo, git) + .validateForGerritCommits(commitReceivedEvent); + } catch (CommitValidationException e) { + throw new InvalidChangeOperationException(e.getMessage()); + } + + RefUpdate ru = git.updateRef(ps.getRefName()); + ru.setExpectedOldObjectId(ObjectId.zeroId()); + ru.setNewObjectId(revertCommit); + ru.disableRefLog(); + if (ru.update(revWalk) != RefUpdate.Result.NEW) { + throw new IOException(String.format( + "Failed to create ref %s in %s: %s", ps.getRefName(), + change.getDest().getParentKey().get(), ru.getResult())); + } + + ChangeMessage cmsg = new ChangeMessage( + new ChangeMessage.Key(changeId, messageUUID(db.get())), + user().getAccountId(), TimeUtil.nowTs(), patchSetId); + StringBuilder msgBuf = new StringBuilder(); + msgBuf.append("Patch Set ").append(patchSetId.get()).append(": Reverted"); + msgBuf.append("\n\n"); + msgBuf.append("This patchset was reverted in change: ") + .append(change.getKey().get()); + cmsg.setMessage(msgBuf.toString()); + + ins.setMessage(cmsg).insert(); + + try { + RevertedSender cm = revertedSenderFactory.create(change); + cm.setFrom(user().getAccountId()); + cm.setChangeMessage(cmsg); + cm.send(); + } catch (Exception err) { + log.error("Cannot send email for revert change " + change.getId(), + err); + } + + return change.getId(); } catch (RepositoryNotFoundException e) { throw new NoSuchChangeException(changeId, e); } - try { - RevWalk revWalk = new RevWalk(git); - try { - RevCommit commitToRevert = - revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get())); - - PersonIdent authorIdent = - user().newCommitterIdent(myIdent.getWhen(), myIdent.getTimeZone()); - - RevCommit parentToCommitToRevert = commitToRevert.getParent(0); - revWalk.parseHeaders(parentToCommitToRevert); - - CommitBuilder revertCommitBuilder = new CommitBuilder(); - revertCommitBuilder.addParentId(commitToRevert); - revertCommitBuilder.setTreeId(parentToCommitToRevert.getTree()); - revertCommitBuilder.setAuthor(authorIdent); - revertCommitBuilder.setCommitter(authorIdent); - - if (message == null) { - message = MessageFormat.format( - ChangeMessages.get().revertChangeDefaultMessage, - changeToRevert.getSubject(), patch.getRevision().get()); - } - - ObjectId computedChangeId = - ChangeIdUtil.computeChangeId(parentToCommitToRevert.getTree(), - commitToRevert, authorIdent, myIdent, message); - revertCommitBuilder.setMessage( - ChangeIdUtil.insertId(message, computedChangeId, true)); - - RevCommit revertCommit; - ObjectInserter oi = git.newObjectInserter(); - try { - ObjectId id = oi.insert(revertCommitBuilder); - oi.flush(); - revertCommit = revWalk.parseCommit(id); - } finally { - oi.release(); - } - - RefControl refControl = ctl.getRefControl(); - Change change = new Change( - new Change.Key("I" + computedChangeId.name()), - new Change.Id(db.get().nextChangeId()), - user().getAccountId(), - changeToRevert.getDest(), - TimeUtil.nowTs()); - change.setTopic(changeToRevert.getTopic()); - ChangeInserter ins = - changeInserterFactory.create(refControl.getProjectControl(), - change, revertCommit); - PatchSet ps = ins.getPatchSet(); - - String ref = refControl.getRefName(); - String cmdRef = MagicBranch.NEW_PUBLISH_CHANGE - + ref.substring(ref.lastIndexOf('/') + 1); - CommitReceivedEvent commitReceivedEvent = new CommitReceivedEvent( - new ReceiveCommand(ObjectId.zeroId(), revertCommit.getId(), cmdRef), - refControl.getProjectControl().getProject(), - refControl.getRefName(), revertCommit, user()); - - try { - commitValidatorsFactory.create(refControl, sshInfo, git) - .validateForGerritCommits(commitReceivedEvent); - } catch (CommitValidationException e) { - throw new InvalidChangeOperationException(e.getMessage()); - } - - RefUpdate ru = git.updateRef(ps.getRefName()); - ru.setExpectedOldObjectId(ObjectId.zeroId()); - ru.setNewObjectId(revertCommit); - ru.disableRefLog(); - if (ru.update(revWalk) != RefUpdate.Result.NEW) { - throw new IOException(String.format( - "Failed to create ref %s in %s: %s", ps.getRefName(), - change.getDest().getParentKey().get(), ru.getResult())); - } - - ChangeMessage cmsg = new ChangeMessage( - new ChangeMessage.Key(changeId, messageUUID(db.get())), - user().getAccountId(), TimeUtil.nowTs(), patchSetId); - StringBuilder msgBuf = new StringBuilder(); - msgBuf.append("Patch Set ").append(patchSetId.get()).append(": Reverted"); - msgBuf.append("\n\n"); - msgBuf.append("This patchset was reverted in change: ") - .append(change.getKey().get()); - cmsg.setMessage(msgBuf.toString()); - - ins.setMessage(cmsg).insert(); - - try { - RevertedSender cm = revertedSenderFactory.create(change); - cm.setFrom(user().getAccountId()); - cm.setChangeMessage(cmsg); - cm.send(); - } catch (Exception err) { - log.error("Cannot send email for revert change " + change.getId(), - err); - } - - return change.getId(); - } finally { - revWalk.release(); - } - } finally { - git.close(); - } } public Change.Id editCommitMessage(ChangeControl ctl, PatchSet ps, @@ -354,68 +343,56 @@ public class ChangeUtil { "The commit message cannot be empty"); } - Repository git; - try { - git = gitManager.openRepository(ctl.getChange().getProject()); + Project.NameKey project = ctl.getChange().getProject(); + try (Repository git = gitManager.openRepository(project); + RevWalk revWalk = new RevWalk(git)) { + RevCommit commit = + revWalk.parseCommit(ObjectId.fromString(ps.getRevision() + .get())); + if (commit.getFullMessage().equals(message)) { + throw new InvalidChangeOperationException( + "New commit message cannot be same as existing commit message"); + } + + Date now = myIdent.getWhen(); + PersonIdent authorIdent = + user().newCommitterIdent(now, myIdent.getTimeZone()); + + CommitBuilder commitBuilder = new CommitBuilder(); + commitBuilder.setTreeId(commit.getTree()); + commitBuilder.setParentIds(commit.getParents()); + commitBuilder.setAuthor(commit.getAuthorIdent()); + commitBuilder.setCommitter(authorIdent); + commitBuilder.setMessage(message); + + RevCommit newCommit; + try (ObjectInserter oi = git.newObjectInserter()) { + ObjectId id = oi.insert(commitBuilder); + oi.flush(); + newCommit = revWalk.parseCommit(id); + } + + PatchSet.Id id = nextPatchSetId(git, change.currentPatchSetId()); + PatchSet newPatchSet = new PatchSet(id); + newPatchSet.setCreatedOn(new Timestamp(now.getTime())); + newPatchSet.setUploader(user().getAccountId()); + newPatchSet.setRevision(new RevId(newCommit.name())); + + String msg = "Patch Set " + newPatchSet.getPatchSetId() + + ": Commit message was updated"; + + change = patchSetInserterFactory + .create(git, revWalk, ctl, newCommit) + .setPatchSet(newPatchSet) + .setMessage(msg) + .setValidatePolicy(RECEIVE_COMMITS) + .setDraft(ps.isDraft()) + .insert(); + + return change.getId(); } catch (RepositoryNotFoundException e) { throw new NoSuchChangeException(changeId, e); } - try { - RevWalk revWalk = new RevWalk(git); - try { - RevCommit commit = - revWalk.parseCommit(ObjectId.fromString(ps.getRevision() - .get())); - if (commit.getFullMessage().equals(message)) { - throw new InvalidChangeOperationException( - "New commit message cannot be same as existing commit message"); - } - - Date now = myIdent.getWhen(); - PersonIdent authorIdent = - user().newCommitterIdent(now, myIdent.getTimeZone()); - - CommitBuilder commitBuilder = new CommitBuilder(); - commitBuilder.setTreeId(commit.getTree()); - commitBuilder.setParentIds(commit.getParents()); - commitBuilder.setAuthor(commit.getAuthorIdent()); - commitBuilder.setCommitter(authorIdent); - commitBuilder.setMessage(message); - - RevCommit newCommit; - ObjectInserter oi = git.newObjectInserter(); - try { - ObjectId id = oi.insert(commitBuilder); - oi.flush(); - newCommit = revWalk.parseCommit(id); - } finally { - oi.release(); - } - - PatchSet.Id id = nextPatchSetId(git, change.currentPatchSetId()); - PatchSet newPatchSet = new PatchSet(id); - newPatchSet.setCreatedOn(new Timestamp(now.getTime())); - newPatchSet.setUploader(user().getAccountId()); - newPatchSet.setRevision(new RevId(newCommit.name())); - - String msg = "Patch Set " + newPatchSet.getPatchSetId() - + ": Commit message was updated"; - - change = patchSetInserterFactory - .create(git, revWalk, ctl, newCommit) - .setPatchSet(newPatchSet) - .setMessage(msg) - .setValidatePolicy(RECEIVE_COMMITS) - .setDraft(ps.isDraft()) - .insert(); - - return change.getId(); - } finally { - revWalk.release(); - } - } finally { - git.close(); - } } public String getMessage(Change change) @@ -427,25 +404,14 @@ public class ChangeUtil { throw new NoSuchChangeException(changeId); } - Repository git; - try { - git = gitManager.openRepository(change.getProject()); + try (Repository git = gitManager.openRepository(change.getProject()); + RevWalk revWalk = new RevWalk(git)) { + RevCommit commit = revWalk.parseCommit( + ObjectId.fromString(ps.getRevision().get())); + return commit.getFullMessage(); } catch (RepositoryNotFoundException e) { throw new NoSuchChangeException(changeId, e); } - try { - RevWalk revWalk = new RevWalk(git); - try { - RevCommit commit = - revWalk.parseCommit(ObjectId.fromString(ps.getRevision() - .get())); - return commit.getFullMessage(); - } finally { - revWalk.release(); - } - } finally { - git.close(); - } } public void deleteDraftChange(Change change) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java index 76cf49cb72..23039aa486 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java @@ -189,8 +189,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache { return ChangeKind.NO_CODE_CHANGE; } - RevWalk walk = new RevWalk(key.repo); - try { + try (RevWalk walk = new RevWalk(key.repo)) { RevCommit prior = walk.parseCommit(key.prior); walk.parseBody(prior); RevCommit next = walk.parseCommit(key.next); @@ -227,7 +226,6 @@ public class ChangeKindCacheImpl implements ChangeKindCache { } } finally { key.repo = null; - walk.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java index 48944fa13d..b386894ece 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java @@ -121,96 +121,82 @@ public class CherryPickChange { Project.NameKey project = change.getProject(); IdentifiedUser identifiedUser = (IdentifiedUser) currentUser.get(); - final Repository git; - try { - git = gitManager.openRepository(project); + try (Repository git = gitManager.openRepository(project); + RevWalk revWalk = new RevWalk(git)) { + Ref destRef = git.getRef(destinationBranch); + if (destRef == null) { + throw new InvalidChangeOperationException("Branch " + + destinationBranch + " does not exist."); + } + + final RevCommit mergeTip = revWalk.parseCommit(destRef.getObjectId()); + + RevCommit commitToCherryPick = + revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get())); + + PersonIdent committerIdent = + identifiedUser.newCommitterIdent(TimeUtil.nowTs(), + serverTimeZone); + + final ObjectId computedChangeId = + ChangeIdUtil + .computeChangeId(commitToCherryPick.getTree(), mergeTip, + commitToCherryPick.getAuthorIdent(), committerIdent, message); + String commitMessage = + ChangeIdUtil.insertId(message, computedChangeId).trim() + '\n'; + + RevCommit cherryPickCommit; + try (ObjectInserter oi = git.newObjectInserter()) { + ProjectState projectState = refControl.getProjectControl().getProjectState(); + cherryPickCommit = + mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip, + commitToCherryPick, committerIdent, commitMessage, revWalk); + } catch (MergeIdenticalTreeException | MergeConflictException e) { + throw new MergeException("Cherry pick failed: " + e.getMessage()); + } + + Change.Key changeKey; + final List idList = cherryPickCommit.getFooterLines( + FooterConstants.CHANGE_ID); + if (!idList.isEmpty()) { + final String idStr = idList.get(idList.size() - 1).trim(); + changeKey = new Change.Key(idStr); + } else { + changeKey = new Change.Key("I" + computedChangeId.name()); + } + + Branch.NameKey newDest = + new Branch.NameKey(change.getProject(), destRef.getName()); + List destChanges = queryProvider.get() + .setLimit(2) + .byBranchKey(newDest, changeKey); + if (destChanges.size() > 1) { + throw new InvalidChangeOperationException("Several changes with key " + + changeKey + " reside on the same branch. " + + "Cannot create a new patch set."); + } else if (destChanges.size() == 1) { + // The change key exists on the destination branch. The cherry pick + // will be added as a new patch set. + return insertPatchSet(git, revWalk, destChanges.get(0).change(), + cherryPickCommit, refControl, identifiedUser); + } else { + // Change key not found on destination branch. We can create a new + // change. + Change newChange = createNewChange(git, revWalk, changeKey, project, + destRef, cherryPickCommit, refControl, + identifiedUser, change.getTopic()); + + addMessageToSourceChange(change, patch.getId(), destinationBranch, + cherryPickCommit, identifiedUser, refControl); + + addMessageToDestinationChange(newChange, change.getDest().getShortName(), + identifiedUser, refControl); + + return newChange.getId(); + } } catch (RepositoryNotFoundException e) { throw new NoSuchChangeException(change.getId(), e); } - - try { - RevWalk revWalk = new RevWalk(git); - try { - Ref destRef = git.getRef(destinationBranch); - if (destRef == null) { - throw new InvalidChangeOperationException("Branch " - + destinationBranch + " does not exist."); - } - - final RevCommit mergeTip = revWalk.parseCommit(destRef.getObjectId()); - - RevCommit commitToCherryPick = - revWalk.parseCommit(ObjectId.fromString(patch.getRevision().get())); - - PersonIdent committerIdent = - identifiedUser.newCommitterIdent(TimeUtil.nowTs(), - serverTimeZone); - - final ObjectId computedChangeId = - ChangeIdUtil - .computeChangeId(commitToCherryPick.getTree(), mergeTip, - commitToCherryPick.getAuthorIdent(), committerIdent, message); - String commitMessage = - ChangeIdUtil.insertId(message, computedChangeId).trim() + '\n'; - - RevCommit cherryPickCommit; - ObjectInserter oi = git.newObjectInserter(); - try { - ProjectState projectState = refControl.getProjectControl().getProjectState(); - cherryPickCommit = - mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip, - commitToCherryPick, committerIdent, commitMessage, revWalk); - } catch (MergeIdenticalTreeException | MergeConflictException e) { - throw new MergeException("Cherry pick failed: " + e.getMessage()); - } finally { - oi.release(); - } - - Change.Key changeKey; - final List idList = cherryPickCommit.getFooterLines( - FooterConstants.CHANGE_ID); - if (!idList.isEmpty()) { - final String idStr = idList.get(idList.size() - 1).trim(); - changeKey = new Change.Key(idStr); - } else { - changeKey = new Change.Key("I" + computedChangeId.name()); - } - - Branch.NameKey newDest = - new Branch.NameKey(change.getProject(), destRef.getName()); - List destChanges = queryProvider.get() - .setLimit(2) - .byBranchKey(newDest, changeKey); - if (destChanges.size() > 1) { - throw new InvalidChangeOperationException("Several changes with key " - + changeKey + " reside on the same branch. " - + "Cannot create a new patch set."); - } else if (destChanges.size() == 1) { - // The change key exists on the destination branch. The cherry pick - // will be added as a new patch set. - return insertPatchSet(git, revWalk, destChanges.get(0).change(), - cherryPickCommit, refControl, identifiedUser); - } else { - // Change key not found on destination branch. We can create a new - // change. - Change newChange = createNewChange(git, revWalk, changeKey, project, - destRef, cherryPickCommit, refControl, - identifiedUser, change.getTopic()); - - addMessageToSourceChange(change, patch.getId(), destinationBranch, - cherryPickCommit, identifiedUser, refControl); - - addMessageToDestinationChange(newChange, change.getDest().getShortName(), - identifiedUser, refControl); - - return newChange.getId(); - } - } finally { - revWalk.release(); - } - } finally { - git.close(); - } } private Change.Id insertPatchSet(Repository git, RevWalk revWalk, Change change, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index ad1e16046b..ee28ed204c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -153,7 +153,7 @@ public class ConsistencyChecker { return Result.create(c, problems); } finally { if (rw != null) { - rw.release(); + rw.close(); } if (repo != null) { repo.close(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index 18552028fd..4dffd67619 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -158,71 +158,63 @@ public class CreateChange implements } Project.NameKey project = rsrc.getNameKey(); - Repository git = gitManager.openRepository(project); - - try { - RevWalk rw = new RevWalk(git); - try { - ObjectId parentCommit; - if (input.baseChange != null) { - List changes = changeUtil.findChanges(input.baseChange); - if (changes.size() != 1) { - throw new InvalidChangeOperationException( - "Base change not found: " + input.baseChange); - } - Change change = Iterables.getOnlyElement(changes); - if (!rsrc.getControl().controlFor(change).isVisible(db.get())) { - throw new InvalidChangeOperationException( - "Base change not found: " + input.baseChange); - } - PatchSet ps = db.get().patchSets().get( - new PatchSet.Id(change.getId(), - change.currentPatchSetId().get())); - parentCommit = ObjectId.fromString(ps.getRevision().get()); - } else { - Ref destRef = git.getRef(refName); - if (destRef == null) { - throw new UnprocessableEntityException(String.format( - "Branch %s does not exist.", refName)); - } - parentCommit = destRef.getObjectId(); + try (Repository git = gitManager.openRepository(project); + RevWalk rw = new RevWalk(git)) { + ObjectId parentCommit; + if (input.baseChange != null) { + List changes = changeUtil.findChanges(input.baseChange); + if (changes.size() != 1) { + throw new InvalidChangeOperationException( + "Base change not found: " + input.baseChange); } - RevCommit mergeTip = rw.parseCommit(parentCommit); - - Timestamp now = TimeUtil.nowTs(); - IdentifiedUser me = (IdentifiedUser) userProvider.get(); - PersonIdent author = me.newCommitterIdent(now, serverTimeZone); - - ObjectId id = ChangeIdUtil.computeChangeId(mergeTip.getTree(), - mergeTip, author, author, input.subject); - String commitMessage = ChangeIdUtil.insertId(input.subject, id); - - RevCommit c = newCommit(git, rw, author, mergeTip, commitMessage); - - Change change = new Change( - getChangeId(id, c), - new Change.Id(db.get().nextChangeId()), - me.getAccountId(), - new Branch.NameKey(project, refName), - now); - - ChangeInserter ins = - changeInserterFactory.create(refControl.getProjectControl(), - change, c); - - validateCommit(git, refControl, c, me, ins); - updateRef(git, rw, c, change, ins.getPatchSet()); - - change.setTopic(input.topic); - ins.setDraft(input.status != null && input.status == ChangeStatus.DRAFT); - ins.insert(); - - return Response.created(json.format(change.getId())); - } finally { - rw.release(); + Change change = Iterables.getOnlyElement(changes); + if (!rsrc.getControl().controlFor(change).isVisible(db.get())) { + throw new InvalidChangeOperationException( + "Base change not found: " + input.baseChange); + } + PatchSet ps = db.get().patchSets().get( + new PatchSet.Id(change.getId(), + change.currentPatchSetId().get())); + parentCommit = ObjectId.fromString(ps.getRevision().get()); + } else { + Ref destRef = git.getRef(refName); + if (destRef == null) { + throw new UnprocessableEntityException(String.format( + "Branch %s does not exist.", refName)); + } + parentCommit = destRef.getObjectId(); } - } finally { - git.close(); + RevCommit mergeTip = rw.parseCommit(parentCommit); + + Timestamp now = TimeUtil.nowTs(); + IdentifiedUser me = (IdentifiedUser) userProvider.get(); + PersonIdent author = me.newCommitterIdent(now, serverTimeZone); + + ObjectId id = ChangeIdUtil.computeChangeId(mergeTip.getTree(), + mergeTip, author, author, input.subject); + String commitMessage = ChangeIdUtil.insertId(input.subject, id); + + RevCommit c = newCommit(git, rw, author, mergeTip, commitMessage); + + Change change = new Change( + getChangeId(id, c), + new Change.Id(db.get().nextChangeId()), + me.getAccountId(), + new Branch.NameKey(project, refName), + now); + + ChangeInserter ins = + changeInserterFactory.create(refControl.getProjectControl(), + change, c); + + validateCommit(git, refControl, c, me, ins); + updateRef(git, rw, c, change, ins.getPatchSet()); + + change.setTopic(input.topic); + ins.setDraft(input.status != null && input.status == ChangeStatus.DRAFT); + ins.insert(); + + return Response.created(json.format(change.getId())); } } @@ -275,8 +267,7 @@ public class CreateChange implements PersonIdent authorIdent, RevCommit mergeTip, String commitMessage) throws IOException { RevCommit emptyCommit; - ObjectInserter oi = git.newObjectInserter(); - try { + try (ObjectInserter oi = git.newObjectInserter()) { CommitBuilder commit = new CommitBuilder(); commit.setTreeId(mergeTip.getTree().getId()); commit.setParentId(mergeTip); @@ -284,8 +275,6 @@ public class CreateChange implements commit.setCommitter(authorIdent); commit.setMessage(commitMessage); emptyCommit = rw.parseCommit(insert(oi, commit)); - } finally { - oi.release(); } return emptyCommit; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java index 5335cb97cb..a8e1793fee 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java @@ -58,64 +58,57 @@ public class FileContentUtil { public BinaryResult getContent(ProjectState project, ObjectId revstr, String path) throws ResourceNotFoundException, IOException { - Repository repo = openRepository(project); - try { - RevWalk rw = new RevWalk(repo); - try { - RevCommit commit = rw.parseCommit(revstr); - ObjectReader reader = rw.getObjectReader(); - TreeWalk tw = TreeWalk.forPath(reader, path, commit.getTree()); - if (tw == null) { - throw new ResourceNotFoundException(); - } - - org.eclipse.jgit.lib.FileMode mode = tw.getFileMode(0); - ObjectId id = tw.getObjectId(0); - if (mode == org.eclipse.jgit.lib.FileMode.GITLINK) { - return BinaryResult.create(id.name()) - .setContentType(X_GIT_GITLINK) - .base64(); - } - - final ObjectLoader obj = repo.open(id, OBJ_BLOB); - byte[] raw; - try { - raw = obj.getCachedBytes(MAX_SIZE); - } catch (LargeObjectException e) { - raw = null; - } - - BinaryResult result; - if (raw != null) { - result = BinaryResult.create(raw); - } else { - result = asBinaryResult(obj); - } - - String type; - if (mode == org.eclipse.jgit.lib.FileMode.SYMLINK) { - type = X_GIT_SYMLINK; - } else { - type = registry.getMimeType(path, raw).toString(); - type = resolveContentType(project, path, FileMode.FILE, type); - } - return result.setContentType(type).base64(); - } finally { - rw.release(); + try (Repository repo = openRepository(project); + RevWalk rw = new RevWalk(repo)) { + RevCommit commit = rw.parseCommit(revstr); + ObjectReader reader = rw.getObjectReader(); + TreeWalk tw = TreeWalk.forPath(reader, path, commit.getTree()); + if (tw == null) { + throw new ResourceNotFoundException(); } - } finally { - repo.close(); + + org.eclipse.jgit.lib.FileMode mode = tw.getFileMode(0); + ObjectId id = tw.getObjectId(0); + if (mode == org.eclipse.jgit.lib.FileMode.GITLINK) { + return BinaryResult.create(id.name()) + .setContentType(X_GIT_GITLINK) + .base64(); + } + + final ObjectLoader obj = repo.open(id, OBJ_BLOB); + byte[] raw; + try { + raw = obj.getCachedBytes(MAX_SIZE); + } catch (LargeObjectException e) { + raw = null; + } + + BinaryResult result; + if (raw != null) { + result = BinaryResult.create(raw); + } else { + result = asBinaryResult(obj); + } + + String type; + if (mode == org.eclipse.jgit.lib.FileMode.SYMLINK) { + type = X_GIT_SYMLINK; + } else { + type = registry.getMimeType(path, raw).toString(); + type = resolveContentType(project, path, FileMode.FILE, type); + } + return result.setContentType(type).base64(); } } private static BinaryResult asBinaryResult(final ObjectLoader obj) { - @SuppressWarnings("resource") BinaryResult result = new BinaryResult() { @Override public void writeTo(OutputStream os) throws IOException { obj.copyTo(os); } - }.setContentLength(obj.getSize()); + }; + result.setContentLength(obj.getSize()); return result; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java index 1c0b0a4eac..e19b6a91b8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java @@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountPatchReview; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; @@ -178,30 +179,24 @@ public class Files implements ChildCollection { private List query(RevisionResource resource) throws RepositoryNotFoundException, IOException { - Repository git = - gitManager.openRepository(resource.getChange().getProject()); - try { - TreeWalk tw = new TreeWalk(git); - try { - RevCommit c = new RevWalk(tw.getObjectReader()) - .parseCommit(ObjectId.fromString( - resource.getPatchSet().getRevision().get())); + Project.NameKey project = resource.getChange().getProject(); + try (Repository git = gitManager.openRepository(project); + ObjectReader or = git.newObjectReader(); + RevWalk rw = new RevWalk(or); + TreeWalk tw = new TreeWalk(or)) { + RevCommit c = rw.parseCommit( + ObjectId.fromString(resource.getPatchSet().getRevision().get())); - tw.addTree(c.getTree()); - tw.setRecursive(true); - List paths = new ArrayList<>(); - while (tw.next() && paths.size() < 20) { - String s = tw.getPathString(); - if (s.contains(query)) { - paths.add(s); - } + tw.addTree(c.getTree()); + tw.setRecursive(true); + List paths = new ArrayList<>(); + while (tw.next() && paths.size() < 20) { + String s = tw.getPathString(); + if (s.contains(query)) { + paths.add(s); } - return paths; - } finally { - tw.release(); } - } finally { - git.close(); + return paths; } } @@ -261,76 +256,69 @@ public class Files implements ChildCollection { private List copy(Set paths, PatchSet.Id old, RevisionResource resource, Account.Id userId) throws IOException, PatchListNotAvailableException, OrmException { - Repository git = - gitManager.openRepository(resource.getChange().getProject()); - try { - ObjectReader reader = git.newObjectReader(); - try { - PatchList oldList = patchListCache.get( - resource.getChange(), - db.get().patchSets().get(old)); - - PatchList curList = patchListCache.get( - resource.getChange(), - resource.getPatchSet()); - - int sz = paths.size(); - List inserts = Lists.newArrayListWithCapacity(sz); - List pathList = Lists.newArrayListWithCapacity(sz); - + Project.NameKey project = resource.getChange().getProject(); + try (Repository git = gitManager.openRepository(project); + ObjectReader reader = git.newObjectReader(); RevWalk rw = new RevWalk(reader); - TreeWalk tw = new TreeWalk(reader); - tw.setFilter(PathFilterGroup.createFromStrings(paths)); - tw.setRecursive(true); - int o = tw.addTree(rw.parseCommit(oldList.getNewId()).getTree()); - int c = tw.addTree(rw.parseCommit(curList.getNewId()).getTree()); + TreeWalk tw = new TreeWalk(reader)) { + PatchList oldList = patchListCache.get( + resource.getChange(), + db.get().patchSets().get(old)); - int op = -1; - if (oldList.getOldId() != null) { - op = tw.addTree(rw.parseTree(oldList.getOldId())); - } + PatchList curList = patchListCache.get( + resource.getChange(), + resource.getPatchSet()); - int cp = -1; - if (curList.getOldId() != null) { - cp = tw.addTree(rw.parseTree(curList.getOldId())); - } + int sz = paths.size(); + List inserts = Lists.newArrayListWithCapacity(sz); + List pathList = Lists.newArrayListWithCapacity(sz); - while (tw.next()) { - String path = tw.getPathString(); - if (tw.getRawMode(o) != 0 && tw.getRawMode(c) != 0 - && tw.idEqual(o, c) - && paths.contains(path)) { - // File exists in previously reviewed oldList and in curList. - // File content is identical. - inserts.add(new AccountPatchReview( - new Patch.Key( - resource.getPatchSet().getId(), - path), - userId)); - pathList.add(path); - } else if (op >= 0 && cp >= 0 - && tw.getRawMode(o) == 0 && tw.getRawMode(c) == 0 - && tw.getRawMode(op) != 0 && tw.getRawMode(cp) != 0 - && tw.idEqual(op, cp) - && paths.contains(path)) { - // File was deleted in previously reviewed oldList and curList. - // File exists in ancestor of oldList and curList. - // File content is identical in ancestors. - inserts.add(new AccountPatchReview( - new Patch.Key( - resource.getPatchSet().getId(), - path), - userId)); - pathList.add(path); - } - } - db.get().accountPatchReviews().insert(inserts); - return pathList; - } finally { - reader.release(); + tw.setFilter(PathFilterGroup.createFromStrings(paths)); + tw.setRecursive(true); + int o = tw.addTree(rw.parseCommit(oldList.getNewId()).getTree()); + int c = tw.addTree(rw.parseCommit(curList.getNewId()).getTree()); + + int op = -1; + if (oldList.getOldId() != null) { + op = tw.addTree(rw.parseTree(oldList.getOldId())); } - } finally { - git.close(); + + int cp = -1; + if (curList.getOldId() != null) { + cp = tw.addTree(rw.parseTree(curList.getOldId())); + } + + while (tw.next()) { + String path = tw.getPathString(); + if (tw.getRawMode(o) != 0 && tw.getRawMode(c) != 0 + && tw.idEqual(o, c) + && paths.contains(path)) { + // File exists in previously reviewed oldList and in curList. + // File content is identical. + inserts.add(new AccountPatchReview( + new Patch.Key( + resource.getPatchSet().getId(), + path), + userId)); + pathList.add(path); + } else if (op >= 0 && cp >= 0 + && tw.getRawMode(o) == 0 && tw.getRawMode(c) == 0 + && tw.getRawMode(op) != 0 && tw.getRawMode(cp) != 0 + && tw.idEqual(op, cp) + && paths.contains(path)) { + // File was deleted in previously reviewed oldList and curList. + // File exists in ancestor of oldList and curList. + // File content is identical in ancestors. + inserts.add(new AccountPatchReview( + new Patch.Key( + resource.getPatchSet().getId(), + path), + userId)); + pathList.add(path); + } + } + db.get().accountPatchReviews().insert(inserts); + return pathList; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetArchive.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetArchive.java index d602c47d6d..913f69e26e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetArchive.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetArchive.java @@ -126,7 +126,7 @@ public class GetArchive implements RestReadView { @Override public void close() throws IOException { - rw.release(); + rw.close(); repo.close(); } }; @@ -139,7 +139,7 @@ public class GetArchive implements RestReadView { return bin; } finally { if (close) { - rw.release(); + rw.close(); } } } finally { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPatch.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPatch.java index 4a8be84379..afd11569ff 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPatch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPatch.java @@ -94,15 +94,16 @@ public class GetPatch implements RestReadView { private void format(OutputStream out) throws IOException { out.write(formatEmailHeader(commit).getBytes(UTF_8)); - DiffFormatter fmt = new DiffFormatter(out); - fmt.setRepository(repo); - fmt.format(base.getTree(), commit.getTree()); - fmt.flush(); + try (DiffFormatter fmt = new DiffFormatter(out)) { + fmt.setRepository(repo); + fmt.format(base.getTree(), commit.getTree()); + fmt.flush(); + } } @Override public void close() throws IOException { - rw.release(); + rw.close(); repo.close(); } }; @@ -123,7 +124,7 @@ public class GetPatch implements RestReadView { return bin; } finally { if (close) { - rw.release(); + rw.close(); } } } finally { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java index f58a2a0805..8c01403924 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java @@ -80,19 +80,12 @@ public class GetRelated implements RestReadView { @Override public RelatedInfo apply(RevisionResource rsrc) throws RepositoryNotFoundException, IOException, OrmException { - Repository git = gitMgr.openRepository(rsrc.getChange().getProject()); - try { + try (Repository git = gitMgr.openRepository(rsrc.getChange().getProject()); + RevWalk rw = new RevWalk(git)) { Ref ref = git.getRef(rsrc.getChange().getDest().get()); - RevWalk rw = new RevWalk(git); - try { - RelatedInfo info = new RelatedInfo(); - info.changes = walk(rsrc, rw, ref); - return info; - } finally { - rw.release(); - } - } finally { - git.close(); + RelatedInfo info = new RelatedInfo(); + info.changes = walk(rsrc, rw, ref); + return info; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedIn.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedIn.java index e0cdebb89a..7e9bb14747 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedIn.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/IncludedIn.java @@ -19,6 +19,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.project.ChangeControl; @@ -55,26 +56,19 @@ class IncludedIn implements RestReadView { ChangeControl ctl = rsrc.getControl(); PatchSet ps = db.get().patchSets().get(ctl.getChange().currentPatchSetId()); - Repository r = - repoManager.openRepository(ctl.getProject().getNameKey()); - try { - RevWalk rw = new RevWalk(r); + Project.NameKey project = ctl.getProject().getNameKey(); + try (Repository r = repoManager.openRepository(project); + RevWalk rw = new RevWalk(r)) { + rw.setRetainBody(false); + RevCommit rev; try { - rw.setRetainBody(false); - RevCommit rev; - try { - rev = rw.parseCommit(ObjectId.fromString(ps.getRevision().get())); - } catch (IncorrectObjectTypeException err) { - throw new BadRequestException(err.getMessage()); - } catch (MissingObjectException err) { - throw new ResourceConflictException(err.getMessage()); - } - return new IncludedInInfo(IncludedInResolver.resolve(r, rw, rev)); - } finally { - rw.release(); + rev = rw.parseCommit(ObjectId.fromString(ps.getRevision().get())); + } catch (IncorrectObjectTypeException err) { + throw new BadRequestException(err.getMessage()); + } catch (MissingObjectException err) { + throw new ResourceConflictException(err.getMessage()); } - } finally { - r.close(); + return new IncludedInInfo(IncludedInResolver.resolve(r, rw, rev)); } } @@ -87,4 +81,4 @@ class IncludedIn implements RestReadView { tags = in.getTags(); } } -} \ No newline at end of file +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java index 5f1ed5bde5..d6b6ab8376 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java @@ -225,8 +225,7 @@ public class MergeabilityCacheImpl implements MergeabilityCache { } try { Map refs = key.load.repo.getAllRefs(); - RevWalk rw = CodeReviewCommit.newRevWalk(key.load.repo); - try { + try (RevWalk rw = CodeReviewCommit.newRevWalk(key.load.repo)) { RevFlag canMerge = rw.newFlag("CAN_MERGE"); CodeReviewCommit rev = parse(rw, key.commit); rev.add(canMerge); @@ -243,8 +242,6 @@ public class MergeabilityCacheImpl implements MergeabilityCache { canMerge, accepted, key.load.dest).dryRun(tip, rev); - } finally { - rw.release(); } } finally { key.load = null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java index f80a4740c4..6b49206f31 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java @@ -119,14 +119,9 @@ public class RebaseChange { "Cannot rebase: New patch sets are not allowed to be added to change: " + changeId.toString()); } - Repository git = null; - RevWalk rw = null; - ObjectInserter inserter = null; - try { - git = gitManager.openRepository(change.getProject()); - rw = new RevWalk(git); - inserter = git.newObjectInserter(); - + try (Repository git = gitManager.openRepository(change.getProject()); + RevWalk rw = new RevWalk(git); + ObjectInserter inserter = git.newObjectInserter()) { String baseRev = newBaseRev; if (baseRev == null) { baseRev = findBaseRevision(patchSetId, db.get(), @@ -149,16 +144,6 @@ public class RebaseChange { committerIdent, true, ValidatePolicy.GERRIT); } catch (MergeConflictException e) { throw new IOException(e.getMessage()); - } finally { - if (inserter != null) { - inserter.release(); - } - if (rw != null) { - rw.release(); - } - if (git != null) { - git.close(); - } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 87895e9f01..d48d0e4b07 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -29,6 +29,7 @@ import com.google.gerrit.extensions.restapi.RawInput; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; @@ -115,26 +116,20 @@ public class ChangeEditModifier { } IdentifiedUser me = (IdentifiedUser) currentUser.get(); - Repository repo = gitManager.openRepository(change.getProject()); String refPrefix = editRefPrefix(me.getAccountId(), change.getId()); - try { + try (Repository repo = gitManager.openRepository(change.getProject())) { Map refs = repo.getRefDatabase().getRefs(refPrefix); if (!refs.isEmpty()) { throw new ResourceConflictException("edit already exists"); } - RevWalk rw = new RevWalk(repo); - try { + try (RevWalk rw = new RevWalk(repo)) { ObjectId revision = ObjectId.fromString(ps.getRevision().get()); String editRefName = editRefName(me.getAccountId(), change.getId(), ps.getId()); return update(repo, me, editRefName, rw, ObjectId.zeroId(), revision); - } finally { - rw.release(); } - } finally { - repo.close(); } } @@ -159,59 +154,51 @@ public class ChangeEditModifier { IdentifiedUser me = (IdentifiedUser) currentUser.get(); String refName = editRefName(me.getAccountId(), change.getId(), current.getId()); - Repository repo = gitManager.openRepository(change.getProject()); - try { - RevWalk rw = new RevWalk(repo); + try (Repository repo = gitManager.openRepository(change.getProject()); + RevWalk rw = new RevWalk(repo); + ObjectInserter inserter = repo.newObjectInserter()) { BatchRefUpdate ru = repo.getRefDatabase().newBatchUpdate(); - ObjectInserter inserter = repo.newObjectInserter(); - try { - RevCommit editCommit = edit.getEditCommit(); - if (editCommit.getParentCount() == 0) { - throw new InvalidChangeOperationException( - "Rebase edit against root commit not supported"); - } - RevCommit tip = rw.parseCommit(ObjectId.fromString( - current.getRevision().get())); - ThreeWayMerger m = MergeStrategy.RESOLVE.newMerger(repo, true); - m.setObjectInserter(inserter); - m.setBase(ObjectId.fromString( - edit.getBasePatchSet().getRevision().get())); - - if (m.merge(tip, editCommit)) { - ObjectId tree = m.getResultTreeId(); - - CommitBuilder commit = new CommitBuilder(); - commit.setTreeId(tree); - for (int i = 0; i < tip.getParentCount(); i++) { - commit.addParentId(tip.getParent(i)); - } - commit.setAuthor(editCommit.getAuthorIdent()); - commit.setCommitter(new PersonIdent( - editCommit.getCommitterIdent(), TimeUtil.nowTs())); - commit.setMessage(editCommit.getFullMessage()); - ObjectId newEdit = inserter.insert(commit); - inserter.flush(); - - ru.addCommand(new ReceiveCommand(ObjectId.zeroId(), newEdit, - refName)); - ru.addCommand(new ReceiveCommand(edit.getRef().getObjectId(), - ObjectId.zeroId(), edit.getRefName())); - ru.execute(rw, NullProgressMonitor.INSTANCE); - for (ReceiveCommand cmd : ru.getCommands()) { - if (cmd.getResult() != ReceiveCommand.Result.OK) { - throw new IOException("failed: " + cmd); - } - } - } else { - // TODO(davido): Allow to resolve conflicts inline - throw new ResourceConflictException("merge conflict"); - } - } finally { - rw.release(); - inserter.release(); + RevCommit editCommit = edit.getEditCommit(); + if (editCommit.getParentCount() == 0) { + throw new InvalidChangeOperationException( + "Rebase edit against root commit not supported"); + } + RevCommit tip = rw.parseCommit(ObjectId.fromString( + current.getRevision().get())); + ThreeWayMerger m = MergeStrategy.RESOLVE.newMerger(repo, true); + m.setObjectInserter(inserter); + m.setBase(ObjectId.fromString( + edit.getBasePatchSet().getRevision().get())); + + if (m.merge(tip, editCommit)) { + ObjectId tree = m.getResultTreeId(); + + CommitBuilder commit = new CommitBuilder(); + commit.setTreeId(tree); + for (int i = 0; i < tip.getParentCount(); i++) { + commit.addParentId(tip.getParent(i)); + } + commit.setAuthor(editCommit.getAuthorIdent()); + commit.setCommitter(new PersonIdent( + editCommit.getCommitterIdent(), TimeUtil.nowTs())); + commit.setMessage(editCommit.getFullMessage()); + ObjectId newEdit = inserter.insert(commit); + inserter.flush(); + + ru.addCommand(new ReceiveCommand(ObjectId.zeroId(), newEdit, + refName)); + ru.addCommand(new ReceiveCommand(edit.getRef().getObjectId(), + ObjectId.zeroId(), edit.getRefName())); + ru.execute(rw, NullProgressMonitor.INSTANCE); + for (ReceiveCommand cmd : ru.getCommands()) { + if (cmd.getResult() != ReceiveCommand.Result.OK) { + throw new IOException("failed: " + cmd); + } + } + } else { + // TODO(davido): Allow to resolve conflicts inline + throw new ResourceConflictException("merge conflict"); } - } finally { - repo.close(); } } @@ -240,23 +227,16 @@ public class ChangeEditModifier { } IdentifiedUser me = (IdentifiedUser) currentUser.get(); - Repository repo = gitManager.openRepository(edit.getChange().getProject()); - try { - RevWalk rw = new RevWalk(repo); - ObjectInserter inserter = repo.newObjectInserter(); - try { - String refName = edit.getRefName(); - ObjectId commit = createCommit(me, inserter, prevEdit, - prevEdit.getTree(), - msg); - inserter.flush(); - return update(repo, me, refName, rw, prevEdit, commit); - } finally { - rw.release(); - inserter.release(); - } - } finally { - repo.close(); + Project.NameKey project = edit.getChange().getProject(); + try (Repository repo = gitManager.openRepository(project); + RevWalk rw = new RevWalk(repo); + ObjectInserter inserter = repo.newObjectInserter()) { + String refName = edit.getRefName(); + ObjectId commit = createCommit(me, inserter, prevEdit, + prevEdit.getTree(), + msg); + inserter.flush(); + return update(repo, me, refName, rw, prevEdit, commit); } } @@ -333,36 +313,28 @@ public class ChangeEditModifier { throw new AuthException("Authentication required"); } IdentifiedUser me = (IdentifiedUser) currentUser.get(); - Repository repo = gitManager.openRepository(edit.getChange().getProject()); - try { - RevWalk rw = new RevWalk(repo); - ObjectInserter inserter = repo.newObjectInserter(); - ObjectReader reader = repo.newObjectReader(); - try { - String refName = edit.getRefName(); - RevCommit prevEdit = edit.getEditCommit(); - ObjectId newTree = writeNewTree(op, - rw, - inserter, - prevEdit, - reader, - file, - newFile, - toBlob(inserter, content)); - if (ObjectId.equals(newTree, prevEdit.getTree())) { - throw new InvalidChangeOperationException("no changes were made"); - } - - ObjectId commit = createCommit(me, inserter, prevEdit, newTree); - inserter.flush(); - return update(repo, me, refName, rw, prevEdit, commit); - } finally { - rw.release(); - inserter.release(); - reader.release(); + Project.NameKey project = edit.getChange().getProject(); + try (Repository repo = gitManager.openRepository(project); + RevWalk rw = new RevWalk(repo); + ObjectInserter inserter = repo.newObjectInserter(); + ObjectReader reader = repo.newObjectReader()) { + String refName = edit.getRefName(); + RevCommit prevEdit = edit.getEditCommit(); + ObjectId newTree = writeNewTree(op, + rw, + inserter, + prevEdit, + reader, + file, + newFile, + toBlob(inserter, content)); + if (ObjectId.equals(newTree, prevEdit.getTree())) { + throw new InvalidChangeOperationException("no changes were made"); } - } finally { - repo.close(); + + ObjectId commit = createCommit(me, inserter, prevEdit, newTree); + inserter.flush(); + return update(repo, me, refName, rw, prevEdit, commit); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java index 3635b5656e..3d2aa6f4e0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java @@ -109,8 +109,7 @@ public class ChangeEditUtil { */ public Optional byChange(Change change, IdentifiedUser user) throws IOException { - Repository repo = gitManager.openRepository(change.getProject()); - try { + try (Repository repo = gitManager.openRepository(change.getProject())) { String editRefPrefix = editRefPrefix(user.getAccountId(), change.getId()); Map refs = repo.getRefDatabase().getRefs(editRefPrefix); if (refs.isEmpty()) { @@ -121,16 +120,11 @@ public class ChangeEditUtil { // where there is more than one ref, we could silently delete all but the // current one. Ref ref = Iterables.getOnlyElement(refs.values()); - RevWalk rw = new RevWalk(repo); - try { + try (RevWalk rw = new RevWalk(repo)) { RevCommit commit = rw.parseCommit(ref.getObjectId()); PatchSet basePs = getBasePatchSet(change, ref); return Optional.of(new ChangeEdit(user, change, ref, commit, basePs)); - } finally { - rw.release(); } - } finally { - repo.close(); } } @@ -150,29 +144,19 @@ public class ChangeEditUtil { NoSuchChangeException, IOException, InvalidChangeOperationException, OrmException, ResourceConflictException { Change change = edit.getChange(); - Repository repo = gitManager.openRepository(change.getProject()); - try { - RevWalk rw = new RevWalk(repo); - ObjectInserter inserter = repo.newObjectInserter(); - try { - - PatchSet basePatchSet = edit.getBasePatchSet(); - if (!basePatchSet.getId().equals(change.currentPatchSetId())) { - throw new ResourceConflictException( - "only edit for current patch set can be published"); - } - - insertPatchSet(edit, change, repo, rw, basePatchSet, - squashEdit(rw, inserter, edit.getEditCommit(), basePatchSet)); - } finally { - inserter.release(); - rw.release(); + try (Repository repo = gitManager.openRepository(change.getProject()); + RevWalk rw = new RevWalk(repo); + ObjectInserter inserter = repo.newObjectInserter()) { + PatchSet basePatchSet = edit.getBasePatchSet(); + if (!basePatchSet.getId().equals(change.currentPatchSetId())) { + throw new ResourceConflictException( + "only edit for current patch set can be published"); } + insertPatchSet(edit, change, repo, rw, basePatchSet, + squashEdit(rw, inserter, edit.getEditCommit(), basePatchSet)); // TODO(davido): This should happen in the same BatchRefUpdate. deleteRef(repo, edit); - } finally { - repo.close(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java index 21465ef2c6..d8e56b3310 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java @@ -101,46 +101,38 @@ public class BanCommit { NoteMap banCommitNotes = NoteMap.newEmptyMap(); // Add a note for each banned commit to notes. final Project.NameKey project = projectControl.getProject().getNameKey(); - final Repository repo = repoManager.openRepository(project); - try { - final RevWalk revWalk = new RevWalk(repo); - final ObjectInserter inserter = repo.newObjectInserter(); - try { - ObjectId noteId = null; - for (final ObjectId commitToBan : commitsToBan) { - try { - revWalk.parseCommit(commitToBan); - } catch (MissingObjectException e) { - // Ignore exception, non-existing commits can be banned. - } catch (IncorrectObjectTypeException e) { - result.notACommit(commitToBan); - continue; - } - if (noteId == null) { - noteId = createNoteContent(reason, inserter); - } - banCommitNotes.set(commitToBan, noteId); + try (Repository repo = repoManager.openRepository(project); + RevWalk revWalk = new RevWalk(repo); + ObjectInserter inserter = repo.newObjectInserter()) { + ObjectId noteId = null; + for (final ObjectId commitToBan : commitsToBan) { + try { + revWalk.parseCommit(commitToBan); + } catch (MissingObjectException e) { + // Ignore exception, non-existing commits can be banned. + } catch (IncorrectObjectTypeException e) { + result.notACommit(commitToBan); + continue; } - NotesBranchUtil notesBranchUtil = - notesBranchUtilFactory.create(project, repo, inserter); - NoteMap newlyCreated = - notesBranchUtil.commitNewNotes(banCommitNotes, REFS_REJECT_COMMITS, - createPersonIdent(), buildCommitMessage(commitsToBan, reason)); - - for (Note n : banCommitNotes) { - if (newlyCreated.contains(n)) { - result.commitBanned(n); - } else { - result.commitAlreadyBanned(n); - } + if (noteId == null) { + noteId = createNoteContent(reason, inserter); } - return result; - } finally { - revWalk.release(); - inserter.release(); + banCommitNotes.set(commitToBan, noteId); } - } finally { - repo.close(); + NotesBranchUtil notesBranchUtil = + notesBranchUtilFactory.create(project, repo, inserter); + NoteMap newlyCreated = + notesBranchUtil.commitNewNotes(banCommitNotes, REFS_REJECT_COMMITS, + createPersonIdent(), buildCommitMessage(commitsToBan, reason)); + + for (Note n : banCommitNotes) { + if (newlyCreated.contains(n)) { + result.commitBanned(n); + } else { + result.commitAlreadyBanned(n); + } + } + return result; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 7d3e1b3644..05e864b2ac 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -332,10 +332,10 @@ public class MergeOp { throw new MergeException("Cannot query the database", e); } finally { if (inserter != null) { - inserter.release(); + inserter.close(); } if (rw != null) { - rw.release(); + rw.close(); } if (repo != null) { repo.close(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java index f1b0a78de7..fb96a6b5de 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java @@ -153,8 +153,8 @@ public class NotesBranchUtil { } updateRef(notesBranch); } finally { - revWalk.release(); - reader.release(); + revWalk.close(); + reader.close(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java index 0e5d9fb4dd..3cf9fed69d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java @@ -382,7 +382,7 @@ public class SubmoduleOp { + subscriber.get(), e); } finally { if (recRw != null) { - recRw.release(); + recRw.close(); } if (pdb != null) { pdb.close(); @@ -392,8 +392,7 @@ public class SubmoduleOp { private static DirCache readTree(final Repository pdb, final Ref branch) throws MissingObjectException, IncorrectObjectTypeException, IOException { - final RevWalk rw = new RevWalk(pdb); - try { + try (RevWalk rw = new RevWalk(pdb)) { final DirCache dc = DirCache.newInCore(); final DirCacheBuilder b = dc.builder(); b.addTree(new byte[0], // no prefix path @@ -401,8 +400,6 @@ public class SubmoduleOp { pdb.newObjectReader(), rw.parseTree(branch.getObjectId())); b.finish(); return dc; - } finally { - rw.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/TagSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/TagSet.java index 799e22053c..a003235483 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/TagSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/TagSet.java @@ -147,7 +147,7 @@ class TagSet { } } finally { if (rw != null) { - rw.release(); + rw.close(); } } } @@ -157,9 +157,8 @@ class TagSet { return; } - TagWalk rw = new TagWalk(git); - rw.setRetainBody(false); - try { + try (TagWalk rw = new TagWalk(git)) { + rw.setRetainBody(false); for (Ref ref : git.getRefDatabase().getRefs(RefDatabase.ALL).values()) { if (skip(ref)) { continue; @@ -188,8 +187,6 @@ class TagSet { } } catch (IOException e) { log.warn("Error building tags for repository " + projectName, e); - } finally { - rw.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java index 8dab448762..b905f67c87 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java @@ -122,7 +122,7 @@ public abstract class VersionedMetaData { revision = id != null ? new RevWalk(reader).parseCommit(id) : null; onLoad(); } finally { - reader.release(); + reader.close(); reader = null; } } @@ -319,13 +319,14 @@ public abstract class VersionedMetaData { public void close() { newTree = null; + rw.close(); if (inserter != null) { - inserter.release(); + inserter.close(); inserter = null; } if (reader != null) { - reader.release(); + reader.close(); reader = null; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/SiteIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/SiteIndexer.java index a0c27c4625..c0522d5ea2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/SiteIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/SiteIndexer.java @@ -323,41 +323,36 @@ public class SiteIndexer { getPathsAndIndex(id); } } finally { - walk.release(); + walk.close(); } return null; } private void getPathsAndIndex(ObjectId b) throws Exception { List cds = Lists.newArrayList(byId.get(b)); - try { + try (DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) { RevCommit bCommit = walk.parseCommit(b); RevTree bTree = bCommit.getTree(); RevTree aTree = aFor(bCommit, walk); - DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE); - try { - df.setRepository(repo); - if (!cds.isEmpty()) { - List paths = (aTree != null) - ? getPaths(df.scan(aTree, bTree)) - : Collections.emptyList(); - Iterator cdit = cds.iterator(); - for (ChangeData cd ; cdit.hasNext(); cdit.remove()) { - cd = cdit.next(); - try { - cd.setCurrentFilePaths(paths); - indexer.index(cd); - done.update(1); - if (verboseWriter != null) { - verboseWriter.println("Reindexed change " + cd.getId()); - } - } catch (Exception e) { - fail("Failed to index change " + cd.getId(), true, e); + df.setRepository(repo); + if (!cds.isEmpty()) { + List paths = (aTree != null) + ? getPaths(df.scan(aTree, bTree)) + : Collections.emptyList(); + Iterator cdit = cds.iterator(); + for (ChangeData cd ; cdit.hasNext(); cdit.remove()) { + cd = cdit.next(); + try { + cd.setCurrentFilePaths(paths); + indexer.index(cd); + done.update(1); + if (verboseWriter != null) { + verboseWriter.println("Reindexed change " + cd.getId()); } + } catch (Exception e) { + fail("Failed to index change " + cd.getId(), true, e); } } - } finally { - df.release(); } } catch (Exception e) { fail("Failed to index commit " + b.name(), false, e); @@ -396,13 +391,10 @@ public class SiteIndexer { } private ObjectId emptyTree() throws IOException { - ObjectInserter oi = repo.newObjectInserter(); - try { + try (ObjectInserter oi = repo.newObjectInserter()) { ObjectId id = oi.insert(Constants.OBJ_TREE, new byte[] {}); oi.flush(); return id; - } finally { - oi.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index 405b3a8c55..ac2345565a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -396,28 +396,28 @@ public abstract class ChangeEmail extends NotificationEmail { TemporaryBuffer.Heap buf = new TemporaryBuffer.Heap(args.settings.maximumDiffSize); - DiffFormatter fmt = new DiffFormatter(buf); - Repository git; - try { - git = args.server.openRepository(change.getProject()); - } catch (IOException e) { - log.error("Cannot open repository to format patch", e); - return ""; - } - try { - fmt.setRepository(git); - fmt.setDetectRenames(true); - fmt.format(patchList.getOldId(), patchList.getNewId()); - return RawParseUtils.decode(buf.toByteArray()); - } catch (IOException e) { - if (JGitText.get().inMemoryBufferLimitExceeded.equals(e.getMessage())) { + try (DiffFormatter fmt = new DiffFormatter(buf)) { + Repository git; + try { + git = args.server.openRepository(change.getProject()); + } catch (IOException e) { + log.error("Cannot open repository to format patch", e); return ""; } - log.error("Cannot format patch", e); - return ""; - } finally { - fmt.release(); - git.close(); + try { + fmt.setRepository(git); + fmt.setDetectRenames(true); + fmt.format(patchList.getOldId(), patchList.getNewId()); + return RawParseUtils.decode(buf.toByteArray()); + } catch (IOException e) { + if (JGitText.get().inMemoryBufferLimitExceeded.equals(e.getMessage())) { + return ""; + } + log.error("Cannot format patch", e); + return ""; + } finally { + git.close(); + } } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/PatchSetNotificationSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/PatchSetNotificationSender.java index 667d834422..53efa1eaff 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/PatchSetNotificationSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/PatchSetNotificationSender.java @@ -82,15 +82,11 @@ public class PatchSetNotificationSender { final Change updatedChange, final PatchSet updatedPatchSet, final LabelTypes labelTypes) throws OrmException, IOException { - final Repository git = repoManager.openRepository(updatedChange.getProject()); - try { - final RevWalk revWalk = new RevWalk(git); + try (Repository git = repoManager.openRepository(updatedChange.getProject())) { final RevCommit commit; - try { + try (RevWalk revWalk = new RevWalk(git)) { commit = revWalk.parseCommit(ObjectId.fromString( updatedPatchSet.getRevision().get())); - } finally { - revWalk.release(); } final PatchSetInfo info = patchSetInfoFactory.get(commit, updatedPatchSet.getId()); final List footerLines = commit.getFooterLines(); @@ -134,8 +130,6 @@ public class PatchSetNotificationSender { log.error("Cannot send email for new patch set " + updatedPatchSet.getId(), e); } } - } finally { - git.close(); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index e97111d02f..6d4f3a4225 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -272,9 +272,9 @@ public class ChangeNotes extends AbstractChangeNotes { loadDefaults(); return; } - RevWalk walk = new RevWalk(reader); - try (ChangeNotesParser parser = - new ChangeNotesParser(change, rev, walk, repoManager)) { + try (RevWalk walk = new RevWalk(reader); + ChangeNotesParser parser = + new ChangeNotesParser(change, rev, walk, repoManager)) { parser.parseAll(); if (parser.status != null) { @@ -301,8 +301,6 @@ public class ChangeNotes extends AbstractChangeNotes { this.allPastReviewers = ImmutableList.copyOf(parser.allPastReviewers); submitRecords = ImmutableList.copyOf(parser.submitRecords); - } finally { - walk.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java index 1053f09bb1..76dfdc8a13 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java @@ -200,12 +200,9 @@ public class ChangeRebuilder { private void writeToBatch(BatchMetaDataUpdate batch, AbstractChangeUpdate update, Repository repo) throws IOException, OrmException { - ObjectInserter inserter = repo.newObjectInserter(); - try { + try (ObjectInserter inserter = repo.newObjectInserter()) { update.setInserter(inserter); update.writeCommit(batch); - } finally { - inserter.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java index d40358bbac..20d6c4aa3d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java @@ -132,16 +132,14 @@ public class DraftCommentNotes extends AbstractChangeNotes { return; } - RevWalk walk = new RevWalk(reader); - try (DraftCommentNotesParser parser = new DraftCommentNotesParser( - getChangeId(), walk, rev, repoManager, draftsProject, author)) { + try (RevWalk walk = new RevWalk(reader); + DraftCommentNotesParser parser = new DraftCommentNotesParser( + getChangeId(), walk, rev, repoManager, draftsProject, author)) { parser.parseDraftComments(); buildCommentTable(draftBaseComments, parser.draftBaseComments); buildCommentTable(draftPsComments, parser.draftPsComments); noteMap = parser.noteMap; - } finally { - walk.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java index 2e63a76a03..8740a6b75f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchFile.java @@ -48,9 +48,8 @@ public class PatchFile { this.repo = repo; this.entry = patchList.get(fileName); - final ObjectReader reader = repo.newObjectReader(); - try { - final RevWalk rw = new RevWalk(reader); + try (ObjectReader reader = repo.newObjectReader(); + RevWalk rw = new RevWalk(reader)) { final RevCommit bCommit = rw.parseCommit(patchList.getNewId()); if (Patch.COMMIT_MSG.equals(fileName)) { @@ -74,8 +73,6 @@ public class PatchFile { } bTree = bCommit.getTree(); } - } finally { - reader.release(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index 74c5bbe77f..40a51aa1bd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -133,9 +133,9 @@ public class PatchListLoader extends CacheLoader { private PatchList readPatchList(final PatchListKey key, final Repository repo) throws IOException, PatchListNotAvailableException { final RawTextComparator cmp = comparatorFor(key.getWhitespace()); - final ObjectReader reader = repo.newObjectReader(); - try { - final RevWalk rw = new RevWalk(reader); + try (ObjectReader reader = repo.newObjectReader(); + RevWalk rw = new RevWalk(reader); + DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE)) { final RevCommit b = rw.parseCommit(key.getNewId()); final RevObject a = aFor(key, repo, rw, b); @@ -155,7 +155,6 @@ public class PatchListLoader extends CacheLoader { RevTree aTree = rw.parseTree(a); RevTree bTree = b.getTree(); - DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE); df.setRepository(repo); df.setDiffComparator(cmp); df.setDetectRenames(true); @@ -187,8 +186,6 @@ public class PatchListLoader extends CacheLoader { } return new PatchList(a, b, againstParent, entries.toArray(new PatchListEntry[entries.size()])); - } finally { - reader.release(); } } @@ -326,8 +323,7 @@ public class PatchListLoader extends CacheLoader { } ResolveMerger m = (ResolveMerger) mergeStrategy.newMerger(repo, true); - final ObjectInserter ins = repo.newObjectInserter(); - try { + try (ObjectInserter ins = repo.newObjectInserter()) { DirCache dc = DirCache.newInCore(); m.setDirCache(dc); m.setObjectInserter(new ObjectInserter.Filter() { @@ -452,19 +448,14 @@ public class PatchListLoader extends CacheLoader { } return rw.lookupTree(treeId); - } finally { - ins.release(); } } private static ObjectId emptyTree(final Repository repo) throws IOException { - ObjectInserter oi = repo.newObjectInserter(); - try { + try (ObjectInserter oi = repo.newObjectInserter()) { ObjectId id = oi.insert(Constants.OBJ_TREE, new byte[] {}); oi.flush(); return id; - } finally { - oi.release(); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index d2316647b1..ea427eb4f3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java @@ -119,7 +119,7 @@ class PatchScriptBuilder { try { return build(content, comments, history); } finally { - reader.release(); + reader.close(); } } @@ -514,9 +514,10 @@ class PatchScriptBuilder { if (path == null || within == null) { return null; } - final RevWalk rw = new RevWalk(reader); - final RevTree tree = rw.parseTree(within); - return TreeWalk.forPath(reader, path, tree); + try (RevWalk rw = new RevWalk(reader)) { + final RevTree tree = rw.parseTree(within); + return TreeWalk.forPath(reader, path, tree); + } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java index 340300af19..1e4ffce1a4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java @@ -85,17 +85,12 @@ public class PatchSetInfoFactory { } catch (IOException e) { throw new PatchSetInfoNotAvailableException(e); } - try { - final RevWalk rw = new RevWalk(repo); - try { - final RevCommit src = - rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get())); - PatchSetInfo info = get(src, patchSet.getId()); - info.setParents(toParentInfos(src.getParents(), rw)); - return info; - } finally { - rw.release(); - } + try (RevWalk rw = new RevWalk(repo)) { + final RevCommit src = + rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get())); + PatchSetInfo info = get(src, patchSet.getId()); + info.setParents(toParentInfos(src.getParents(), rw)); + return info; } catch (IOException e) { throw new PatchSetInfoNotAvailableException(e); } finally { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java index 1939c84faa..882e25f6f1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/Text.java @@ -44,44 +44,45 @@ public class Text extends RawText { public static final Text EMPTY = new Text(NO_BYTES); public static Text forCommit(ObjectReader reader, AnyObjectId commitId) throws IOException { - RevWalk rw = new RevWalk(reader); - RevCommit c; - if (commitId instanceof RevCommit) { - c = (RevCommit) commitId; - } else { - c = rw.parseCommit(commitId); - } - - StringBuilder b = new StringBuilder(); - switch (c.getParentCount()) { - case 0: - break; - case 1: { - RevCommit p = c.getParent(0); - rw.parseBody(p); - b.append("Parent: "); - b.append(reader.abbreviate(p, 8).name()); - b.append(" ("); - b.append(p.getShortMessage()); - b.append(")\n"); - break; + try (RevWalk rw = new RevWalk(reader)) { + RevCommit c; + if (commitId instanceof RevCommit) { + c = (RevCommit) commitId; + } else { + c = rw.parseCommit(commitId); } - default: - for (int i = 0; i < c.getParentCount(); i++) { - RevCommit p = c.getParent(i); + + StringBuilder b = new StringBuilder(); + switch (c.getParentCount()) { + case 0: + break; + case 1: { + RevCommit p = c.getParent(0); rw.parseBody(p); - b.append(i == 0 ? "Merge Of: " : " "); + b.append("Parent: "); b.append(reader.abbreviate(p, 8).name()); b.append(" ("); b.append(p.getShortMessage()); b.append(")\n"); + break; } + default: + for (int i = 0; i < c.getParentCount(); i++) { + RevCommit p = c.getParent(i); + rw.parseBody(p); + b.append(i == 0 ? "Merge Of: " : " "); + b.append(reader.abbreviate(p, 8).name()); + b.append(" ("); + b.append(p.getShortMessage()); + b.append(")\n"); + } + } + appendPersonIdent(b, "Author", c.getAuthorIdent()); + appendPersonIdent(b, "Commit", c.getCommitterIdent()); + b.append("\n"); + b.append(c.getFullMessage()); + return new Text(b.toString().getBytes("UTF-8")); } - appendPersonIdent(b, "Author", c.getAuthorIdent()); - appendPersonIdent(b, "Commit", c.getCommitterIdent()); - b.append("\n"); - b.append(c.getFullMessage()); - return new Text(b.toString().getBytes("UTF-8")); } private static void appendPersonIdent(StringBuilder b, String field, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CommitsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CommitsCollection.java index e5e7bed21c..4879bb7b88 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CommitsCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CommitsCollection.java @@ -65,26 +65,19 @@ public class CommitsCollection implements throw new ResourceNotFoundException(id); } - Repository repo = repoManager.openRepository(parent.getNameKey()); - try { - RevWalk rw = new RevWalk(repo); - try { - RevCommit commit = rw.parseCommit(objectId); - rw.parseBody(commit); - if (!parent.getControl().canReadCommit(db.get(), rw, commit)) { - throw new ResourceNotFoundException(id); - } - for (int i = 0; i < commit.getParentCount(); i++) { - rw.parseBody(rw.parseCommit(commit.getParent(i))); - } - return new CommitResource(parent, commit); - } catch (MissingObjectException | IncorrectObjectTypeException e) { + try (Repository repo = repoManager.openRepository(parent.getNameKey()); + RevWalk rw = new RevWalk(repo)) { + RevCommit commit = rw.parseCommit(objectId); + rw.parseBody(commit); + if (!parent.getControl().canReadCommit(db.get(), rw, commit)) { throw new ResourceNotFoundException(id); - } finally { - rw.release(); } - } finally { - repo.close(); + for (int i = 0; i < commit.getParentCount(); i++) { + rw.parseBody(rw.parseCommit(commit.getParent(i))); + } + return new CommitResource(parent, commit); + } catch (MissingObjectException | IncorrectObjectTypeException e) { + throw new ResourceNotFoundException(id); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java index bdc67ac774..d6e93f08c5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java @@ -99,11 +99,8 @@ class DeleteBranches implements RestModifyView { for (String branch : input.branches) { batchUpdate.addCommand(createDeleteCommand(project, r, branch)); } - RevWalk rw = new RevWalk(r); - try { + try (RevWalk rw = new RevWalk(r)) { batchUpdate.execute(rw, NullProgressMonitor.INSTANCE); - } finally { - rw.release(); } StringBuilder errorMessages = new StringBuilder(); for (ReceiveCommand command : batchUpdate.getCommands()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetHead.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetHead.java index 0530a4cdf8..2efd257bcf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetHead.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetHead.java @@ -49,9 +49,7 @@ public class GetHead implements RestReadView { @Override public String apply(ProjectResource rsrc) throws AuthException, ResourceNotFoundException, IOException { - Repository repo = null; - try { - repo = repoManager.openRepository(rsrc.getNameKey()); + try (Repository repo = repoManager.openRepository(rsrc.getNameKey())) { Ref head = repo.getRef(Constants.HEAD); if (head == null) { throw new ResourceNotFoundException(Constants.HEAD); @@ -62,8 +60,7 @@ public class GetHead implements RestReadView { } throw new AuthException("not allowed to see HEAD"); } else if (head.getObjectId() != null) { - RevWalk rw = new RevWalk(repo); - try { + try (RevWalk rw = new RevWalk(repo)) { RevCommit commit = rw.parseCommit(head.getObjectId()); if (rsrc.getControl().canReadCommit(db.get(), rw, commit)) { return head.getObjectId().name(); @@ -74,17 +71,11 @@ public class GetHead implements RestReadView { return head.getObjectId().name(); } throw new AuthException("not allowed to see HEAD"); - } finally { - rw.release(); } } throw new ResourceNotFoundException(Constants.HEAD); } catch (RepositoryNotFoundException e) { throw new ResourceNotFoundException(rsrc.getName()); - } finally { - if (repo != null) { - repo.close(); - } } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListDashboards.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListDashboards.java index 07fd095f74..456abfcdf5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListDashboards.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListDashboards.java @@ -82,53 +82,45 @@ class ListDashboards implements RestReadView { private List scan(ProjectControl ctl, String project, boolean setDefault) throws ResourceNotFoundException, IOException { - Repository git; - try { - git = gitManager.openRepository(ctl.getProject().getNameKey()); + Project.NameKey projectName = ctl.getProject().getNameKey(); + try (Repository git = gitManager.openRepository(projectName); + RevWalk rw = new RevWalk(git)) { + List all = Lists.newArrayList(); + for (Ref ref : git.getRefDatabase().getRefs(REFS_DASHBOARDS).values()) { + if (ctl.controlForRef(ref.getName()).canRead()) { + all.addAll(scanDashboards(ctl.getProject(), git, rw, ref, + project, setDefault)); + } + } + return all; } catch (RepositoryNotFoundException e) { throw new ResourceNotFoundException(); } - try { - RevWalk rw = new RevWalk(git); - try { - List all = Lists.newArrayList(); - for (Ref ref : git.getRefDatabase().getRefs(REFS_DASHBOARDS).values()) { - if (ctl.controlForRef(ref.getName()).canRead()) { - all.addAll(scanDashboards(ctl.getProject(), git, rw, ref, - project, setDefault)); - } - } - return all; - } finally { - rw.release(); - } - } finally { - git.close(); - } } private List scanDashboards(Project definingProject, Repository git, RevWalk rw, Ref ref, String project, boolean setDefault) throws IOException { List list = Lists.newArrayList(); - TreeWalk tw = new TreeWalk(rw.getObjectReader()); - tw.addTree(rw.parseTree(ref.getObjectId())); - tw.setRecursive(true); - while (tw.next()) { - if (tw.getFileMode(0) == FileMode.REGULAR_FILE) { - try { - list.add(DashboardsCollection.parse( - definingProject, - ref.getName().substring(REFS_DASHBOARDS.length()), - tw.getPathString(), - new BlobBasedConfig(null, git, tw.getObjectId(0)), - project, - setDefault)); - } catch (ConfigInvalidException e) { - log.warn(String.format( - "Cannot parse dashboard %s:%s:%s: %s", - definingProject.getName(), ref.getName(), tw.getPathString(), - e.getMessage())); + try (TreeWalk tw = new TreeWalk(rw.getObjectReader())) { + tw.addTree(rw.parseTree(ref.getObjectId())); + tw.setRecursive(true); + while (tw.next()) { + if (tw.getFileMode(0) == FileMode.REGULAR_FILE) { + try { + list.add(DashboardsCollection.parse( + definingProject, + ref.getName().substring(REFS_DASHBOARDS.length()), + tw.getPathString(), + new BlobBasedConfig(null, git, tw.getObjectId(0)), + project, + setDefault)); + } catch (ConfigInvalidException e) { + log.warn(String.format( + "Cannot parse dashboard %s:%s:%s: %s", + definingProject.getName(), ref.getName(), tw.getPathString(), + e.getMessage())); + } } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java index e12b38a2f0..b01e563e1e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ListTags.java @@ -98,26 +98,17 @@ public class ListTags implements RestReadView { public TagInfo get(ProjectResource resource, IdString id) throws ResourceNotFoundException, IOException { - Repository repo = getRepository(resource.getNameKey()); - - String tagName = id.get(); - if (!tagName.startsWith(Constants.R_TAGS)) { - tagName = Constants.R_TAGS + tagName; - } - - try { - RevWalk rw = new RevWalk(repo); - try { - Ref ref = repo.getRefDatabase().getRef(tagName); - if (ref != null && !visibleTags(resource.getControl(), repo, - ImmutableMap.of(ref.getName(), ref)).isEmpty()) { - return createTagInfo(ref, rw); - } - } finally { - rw.dispose(); + try (Repository repo = getRepository(resource.getNameKey()); + RevWalk rw = new RevWalk(repo)) { + String tagName = id.get(); + if (!tagName.startsWith(Constants.R_TAGS)) { + tagName = Constants.R_TAGS + tagName; + } + Ref ref = repo.getRefDatabase().getRef(tagName); + if (ref != null && !visibleTags(resource.getControl(), repo, + ImmutableMap.of(ref.getName(), ref)).isEmpty()) { + return createTagInfo(ref, rw); } - } finally { - repo.close(); } throw new ResourceNotFoundException(id); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/PerformCreateProject.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/PerformCreateProject.java index 03c954b54a..689920b6d1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/PerformCreateProject.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/PerformCreateProject.java @@ -270,8 +270,7 @@ public class PerformCreateProject { private void createEmptyCommits(final Repository repo, final Project.NameKey project, final List refs) throws IOException { - ObjectInserter oi = repo.newObjectInserter(); - try { + try (ObjectInserter oi = repo.newObjectInserter()) { CommitBuilder cb = new CommitBuilder(); cb.setTreeId(oi.insert(Constants.OBJ_TREE, new byte[] {})); cb.setAuthor(metaDataUpdateFactory.getUserPersonIdent()); @@ -300,8 +299,6 @@ public class PerformCreateProject { "Cannot create empty commit for " + createProjectArgs.getProjectName(), e); throw e; - } finally { - oi.release(); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index ecc3f30076..e4e94a12f8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -523,18 +523,11 @@ public class ChangeData { return false; } String sha1 = ps.getRevision().get(); - Repository repo = repoManager.openRepository(change().getProject()); - try { - RevWalk walk = new RevWalk(repo); - try { - RevCommit c = walk.parseCommit(ObjectId.fromString(sha1)); - commitMessage = c.getFullMessage(); - commitFooters = c.getFooterLines(); - } finally { - walk.release(); - } - } finally { - repo.close(); + try (Repository repo = repoManager.openRepository(change().getProject()); + RevWalk walk = new RevWalk(repo)) { + RevCommit c = walk.parseCommit(ObjectId.fromString(sha1)); + commitMessage = c.getFullMessage(); + commitFooters = c.getFooterLines(); } return true; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java index 87c33a2129..207074620f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java @@ -107,37 +107,24 @@ class ConflictsPredicate extends OrPredicate { if (conflicts != null) { return conflicts; } - try { - Repository repo = + try (Repository repo = args.repoManager.openRepository(otherChange.getProject()); - try { - RevWalk rw = CodeReviewCommit.newRevWalk(repo); - try { - RevFlag canMergeFlag = rw.newFlag("CAN_MERGE"); - CodeReviewCommit commit = - (CodeReviewCommit) rw.parseCommit(changeDataCache.getTestAgainst()); - SubmitStrategy strategy = - args.submitStrategyFactory.create(submitType, - db.get(), repo, rw, null, canMergeFlag, - getAlreadyAccepted(repo, rw, commit), - otherChange.getDest()); - CodeReviewCommit otherCommit = - (CodeReviewCommit) rw.parseCommit(other); - otherCommit.add(canMergeFlag); - conflicts = !strategy.dryRun(commit, otherCommit); - args.conflictsCache.put(conflictsKey, conflicts); - return conflicts; - } catch (MergeException e) { - throw new IllegalStateException(e); - } catch (NoSuchProjectException e) { - throw new IllegalStateException(e); - } finally { - rw.release(); - } - } finally { - repo.close(); - } - } catch (IOException e) { + RevWalk rw = CodeReviewCommit.newRevWalk(repo)) { + RevFlag canMergeFlag = rw.newFlag("CAN_MERGE"); + CodeReviewCommit commit = + (CodeReviewCommit) rw.parseCommit(changeDataCache.getTestAgainst()); + SubmitStrategy strategy = + args.submitStrategyFactory.create(submitType, + db.get(), repo, rw, null, canMergeFlag, + getAlreadyAccepted(repo, rw, commit), + otherChange.getDest()); + CodeReviewCommit otherCommit = + (CodeReviewCommit) rw.parseCommit(other); + otherCommit.add(canMergeFlag); + conflicts = !strategy.dryRun(commit, otherCommit); + args.conflictsCache.put(conflictsKey, conflicts); + return conflicts; + } catch (MergeException | NoSuchProjectException | IOException e) { throw new IllegalStateException(e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RevWalkPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RevWalkPredicate.java index fd57a44f40..0947fae24e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RevWalkPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/RevWalkPredicate.java @@ -102,18 +102,9 @@ public abstract class RevWalkPredicate extends OperatorPredicate { Arguments args = new Arguments(patchSet, revision, objectId, change, projectName); - try { - final Repository repo = repoManager.openRepository(projectName); - try { - final RevWalk rw = new RevWalk(repo); - try { - return match(repo, rw, args); - } finally { - rw.release(); - } - } finally { - repo.close(); - } + try (Repository repo = repoManager.openRepository(projectName); + RevWalk rw = new RevWalk(repo)) { + return match(repo, rw, args); } catch (RepositoryNotFoundException e) { log.error("Repository \"" + projectName.get() + "\" unknown.", e); } catch (IOException e) { diff --git a/gerrit-server/src/main/java/gerrit/PRED_commit_edits_2.java b/gerrit-server/src/main/java/gerrit/PRED_commit_edits_2.java index 2c7949c191..509faf0313 100644 --- a/gerrit-server/src/main/java/gerrit/PRED_commit_edits_2.java +++ b/gerrit-server/src/main/java/gerrit/PRED_commit_edits_2.java @@ -73,11 +73,10 @@ public class PRED_commit_edits_2 extends Predicate.P2 { PatchList pl = StoredValues.PATCH_LIST.get(engine); Repository repo = StoredValues.REPOSITORY.get(engine); - final ObjectReader reader = repo.newObjectReader(); - final RevTree aTree; - final RevTree bTree; - try { - final RevWalk rw = new RevWalk(reader); + try (ObjectReader reader = repo.newObjectReader(); + RevWalk rw = new RevWalk(reader)) { + final RevTree aTree; + final RevTree bTree; final RevCommit bCommit = rw.parseCommit(pl.getNewId()); if (pl.getOldId() != null) { @@ -129,8 +128,6 @@ public class PRED_commit_edits_2 extends Predicate.P2 { } } catch (IOException err) { throw new JavaException(this, 1, err); - } finally { - reader.release(); } return engine.fail(); @@ -161,4 +158,4 @@ public class PRED_commit_edits_2 extends Predicate.P2 { } return new Text(reader.open(tw.getObjectId(0), Constants.OBJ_BLOB)); } -} \ No newline at end of file +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/IncludedInResolverTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/IncludedInResolverTest.java index dc9724c782..4cd31abdea 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/IncludedInResolverTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/IncludedInResolverTest.java @@ -86,6 +86,8 @@ public class IncludedInResolverTest extends RepositoryTestCase { */ + // TODO(dborowitz): Use try/finally when this doesn't double-close the repo. + @SuppressWarnings("resource") Git git = new Git(db); revWalk = new RevWalk(db); // Version 1.0 @@ -129,7 +131,7 @@ public class IncludedInResolverTest extends RepositoryTestCase { @Override @After public void tearDown() throws Exception { - revWalk.release(); + revWalk.close(); super.tearDown(); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java index f4f989fc96..c2f3f6f4cc 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java @@ -118,34 +118,37 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { public void testEmptyCommit() throws Exception { expect(schemaFactory.open()).andReturn(schema); - final Repository realDb = createWorkRepository(); - final Git git = new Git(realDb); + try (Repository realDb = createWorkRepository()) { + // TODO(dborowitz): Use try/finally when this doesn't double-close the repo. + @SuppressWarnings("resource") + final Git git = new Git(realDb); - final RevCommit mergeTip = git.commit().setMessage("test").call(); + final RevCommit mergeTip = git.commit().setMessage("test").call(); - final Branch.NameKey branchNameKey = - new Branch.NameKey(new Project.NameKey("test-project"), "test-branch"); + final Branch.NameKey branchNameKey = + new Branch.NameKey(new Project.NameKey("test-project"), "test-branch"); - expect(urlProvider.get()).andReturn("http://localhost:8080"); + expect(urlProvider.get()).andReturn("http://localhost:8080"); - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - final ResultSet emptySubscriptions = - new ListResultSet<>(new ArrayList()); - expect(subscriptions.bySubmodule(branchNameKey)).andReturn( - emptySubscriptions); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + final ResultSet emptySubscriptions = + new ListResultSet<>(new ArrayList()); + expect(subscriptions.bySubmodule(branchNameKey)).andReturn( + emptySubscriptions); - schema.close(); + schema.close(); - doReplay(); + doReplay(); - final SubmoduleOp submoduleOp = - new SubmoduleOp(branchNameKey, mergeTip, new RevWalk(realDb), urlProvider, - schemaFactory, realDb, null, new ArrayList(), null, null, - null, null, null, null); + final SubmoduleOp submoduleOp = + new SubmoduleOp(branchNameKey, mergeTip, new RevWalk(realDb), urlProvider, + schemaFactory, realDb, null, new ArrayList(), null, null, + null, null, null, null); - submoduleOp.update(); + submoduleOp.update(); - doVerify(); + doVerify(); + } } /** @@ -588,85 +591,89 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { public void testOneSubscriberToUpdate() throws Exception { expect(schemaFactory.open()).andReturn(schema); - final Repository sourceRepository = createWorkRepository(); - final Git sourceGit = new Git(sourceRepository); + try (Repository sourceRepository = createWorkRepository(); + Repository targetRepository = createWorkRepository()) { + // TODO(dborowitz): Use try/finally when this doesn't double-close the repo. + @SuppressWarnings("resource") + final Git sourceGit = new Git(sourceRepository); + // TODO(dborowitz): Use try/finally when this doesn't double-close the repo. + @SuppressWarnings("resource") + final Git targetGit = new Git(targetRepository); - addRegularFileToIndex("file.txt", "test content", sourceRepository); + addRegularFileToIndex("file.txt", "test content", sourceRepository); - final RevCommit sourceMergeTip = - sourceGit.commit().setMessage("test").call(); + final RevCommit sourceMergeTip = + sourceGit.commit().setMessage("test").call(); - final Branch.NameKey sourceBranchNameKey = - new Branch.NameKey(new Project.NameKey("source-project"), - "refs/heads/master"); + final Branch.NameKey sourceBranchNameKey = + new Branch.NameKey(new Project.NameKey("source-project"), + "refs/heads/master"); - final CodeReviewCommit codeReviewCommit = - new CodeReviewCommit(sourceMergeTip.toObjectId()); - final Change submittedChange = new Change( - new Change.Key(sourceMergeTip.toObjectId().getName()), new Change.Id(1), - new Account.Id(1), sourceBranchNameKey, TimeUtil.nowTs()); + final CodeReviewCommit codeReviewCommit = + new CodeReviewCommit(sourceMergeTip.toObjectId()); + final Change submittedChange = new Change( + new Change.Key(sourceMergeTip.toObjectId().getName()), new Change.Id(1), + new Account.Id(1), sourceBranchNameKey, TimeUtil.nowTs()); - final Map mergedCommits = new HashMap<>(); - mergedCommits.put(submittedChange.getId(), codeReviewCommit); + final Map mergedCommits = new HashMap<>(); + mergedCommits.put(submittedChange.getId(), codeReviewCommit); - final List submitted = new ArrayList<>(); - submitted.add(submittedChange); + final List submitted = new ArrayList<>(); + submitted.add(submittedChange); - final Repository targetRepository = createWorkRepository(); - final Git targetGit = new Git(targetRepository); + addGitLinkToIndex("a", sourceMergeTip.copy(), targetRepository); - addGitLinkToIndex("a", sourceMergeTip.copy(), targetRepository); + targetGit.commit().setMessage("test").call(); - targetGit.commit().setMessage("test").call(); + final Branch.NameKey targetBranchNameKey = + new Branch.NameKey(new Project.NameKey("target-project"), + sourceBranchNameKey.get()); - final Branch.NameKey targetBranchNameKey = - new Branch.NameKey(new Project.NameKey("target-project"), - sourceBranchNameKey.get()); + expect(urlProvider.get()).andReturn("http://localhost:8080"); - expect(urlProvider.get()).andReturn("http://localhost:8080"); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + final ResultSet subscribers = + new ListResultSet<>(Collections + .singletonList(new SubmoduleSubscription(targetBranchNameKey, + sourceBranchNameKey, "source-project"))); + expect(subscriptions.bySubmodule(sourceBranchNameKey)).andReturn( + subscribers); - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - final ResultSet subscribers = - new ListResultSet<>(Collections - .singletonList(new SubmoduleSubscription(targetBranchNameKey, - sourceBranchNameKey, "source-project"))); - expect(subscriptions.bySubmodule(sourceBranchNameKey)).andReturn( - subscribers); + expect(repoManager.openRepository(targetBranchNameKey.getParentKey())) + .andReturn(targetRepository).anyTimes(); - expect(repoManager.openRepository(targetBranchNameKey.getParentKey())) - .andReturn(targetRepository).anyTimes(); + Capture ruCapture = new Capture<>(); + gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()), + capture(ruCapture)); + changeHooks.doRefUpdatedHook(eq(targetBranchNameKey), + anyObject(RefUpdate.class), EasyMock.isNull()); - Capture ruCapture = new Capture<>(); - gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()), - capture(ruCapture)); - changeHooks.doRefUpdatedHook(eq(targetBranchNameKey), - anyObject(RefUpdate.class), EasyMock.isNull()); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + final ResultSet emptySubscriptions = + new ListResultSet<>(new ArrayList()); + expect(subscriptions.bySubmodule(targetBranchNameKey)).andReturn( + emptySubscriptions); - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - final ResultSet emptySubscriptions = - new ListResultSet<>(new ArrayList()); - expect(subscriptions.bySubmodule(targetBranchNameKey)).andReturn( - emptySubscriptions); + schema.close(); - schema.close(); + final PersonIdent myIdent = + new PersonIdent("test-user", "test-user@email.com"); - final PersonIdent myIdent = - new PersonIdent("test-user", "test-user@email.com"); + doReplay(); - doReplay(); + final SubmoduleOp submoduleOp = + new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk( + sourceRepository), urlProvider, schemaFactory, sourceRepository, + new Project(sourceBranchNameKey.getParentKey()), submitted, + mergedCommits, myIdent, repoManager, gitRefUpdated, null, + changeHooks); - final SubmoduleOp submoduleOp = - new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk( - sourceRepository), urlProvider, schemaFactory, sourceRepository, - new Project(sourceBranchNameKey.getParentKey()), submitted, - mergedCommits, myIdent, repoManager, gitRefUpdated, null, - changeHooks); + submoduleOp.update(); - submoduleOp.update(); - - doVerify(); - RefUpdate ru = ruCapture.getValue(); - assertEquals(ru.getName(), targetBranchNameKey.get()); + doVerify(); + RefUpdate ru = ruCapture.getValue(); + assertEquals(ru.getName(), targetBranchNameKey.get()); + } } /** @@ -693,86 +700,90 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { public void testAvoidingCircularReference() throws Exception { expect(schemaFactory.open()).andReturn(schema); - final Repository sourceRepository = createWorkRepository(); - final Git sourceGit = new Git(sourceRepository); + try (Repository sourceRepository = createWorkRepository(); + Repository targetRepository = createWorkRepository()) { + // TODO(dborowitz): Use try/finally when this doesn't double-close the repo. + @SuppressWarnings("resource") + final Git sourceGit = new Git(sourceRepository); + // TODO(dborowitz): Use try/finally when this doesn't double-close the repo. + @SuppressWarnings("resource") + final Git targetGit = new Git(targetRepository); - addRegularFileToIndex("file.txt", "test content", sourceRepository); + addRegularFileToIndex("file.txt", "test content", sourceRepository); - final RevCommit sourceMergeTip = - sourceGit.commit().setMessage("test").call(); + final RevCommit sourceMergeTip = + sourceGit.commit().setMessage("test").call(); - final Branch.NameKey sourceBranchNameKey = - new Branch.NameKey(new Project.NameKey("source-project"), - "refs/heads/master"); + final Branch.NameKey sourceBranchNameKey = + new Branch.NameKey(new Project.NameKey("source-project"), + "refs/heads/master"); - final CodeReviewCommit codeReviewCommit = - new CodeReviewCommit(sourceMergeTip.toObjectId()); - final Change submittedChange = new Change( - new Change.Key(sourceMergeTip.toObjectId().getName()), new Change.Id(1), - new Account.Id(1), sourceBranchNameKey, TimeUtil.nowTs()); + final CodeReviewCommit codeReviewCommit = + new CodeReviewCommit(sourceMergeTip.toObjectId()); + final Change submittedChange = new Change( + new Change.Key(sourceMergeTip.toObjectId().getName()), new Change.Id(1), + new Account.Id(1), sourceBranchNameKey, TimeUtil.nowTs()); - final Map mergedCommits = new HashMap<>(); - mergedCommits.put(submittedChange.getId(), codeReviewCommit); + final Map mergedCommits = new HashMap<>(); + mergedCommits.put(submittedChange.getId(), codeReviewCommit); - final List submitted = new ArrayList<>(); - submitted.add(submittedChange); + final List submitted = new ArrayList<>(); + submitted.add(submittedChange); - final Repository targetRepository = createWorkRepository(); - final Git targetGit = new Git(targetRepository); + addGitLinkToIndex("a", sourceMergeTip.copy(), targetRepository); - addGitLinkToIndex("a", sourceMergeTip.copy(), targetRepository); + targetGit.commit().setMessage("test").call(); - targetGit.commit().setMessage("test").call(); + final Branch.NameKey targetBranchNameKey = + new Branch.NameKey(new Project.NameKey("target-project"), + sourceBranchNameKey.get()); - final Branch.NameKey targetBranchNameKey = - new Branch.NameKey(new Project.NameKey("target-project"), - sourceBranchNameKey.get()); + expect(urlProvider.get()).andReturn("http://localhost:8080"); - expect(urlProvider.get()).andReturn("http://localhost:8080"); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + final ResultSet subscribers = + new ListResultSet<>(Collections + .singletonList(new SubmoduleSubscription(targetBranchNameKey, + sourceBranchNameKey, "source-project"))); + expect(subscriptions.bySubmodule(sourceBranchNameKey)).andReturn( + subscribers); - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - final ResultSet subscribers = - new ListResultSet<>(Collections - .singletonList(new SubmoduleSubscription(targetBranchNameKey, - sourceBranchNameKey, "source-project"))); - expect(subscriptions.bySubmodule(sourceBranchNameKey)).andReturn( - subscribers); + expect(repoManager.openRepository(targetBranchNameKey.getParentKey())) + .andReturn(targetRepository).anyTimes(); - expect(repoManager.openRepository(targetBranchNameKey.getParentKey())) - .andReturn(targetRepository).anyTimes(); + Capture ruCapture = new Capture<>(); + gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()), + capture(ruCapture)); + changeHooks.doRefUpdatedHook(eq(targetBranchNameKey), + anyObject(RefUpdate.class), EasyMock.isNull()); - Capture ruCapture = new Capture<>(); - gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()), - capture(ruCapture)); - changeHooks.doRefUpdatedHook(eq(targetBranchNameKey), - anyObject(RefUpdate.class), EasyMock.isNull()); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + final ResultSet incorrectSubscriptions = + new ListResultSet<>(Collections + .singletonList(new SubmoduleSubscription(sourceBranchNameKey, + targetBranchNameKey, "target-project"))); + expect(subscriptions.bySubmodule(targetBranchNameKey)).andReturn( + incorrectSubscriptions); - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - final ResultSet incorrectSubscriptions = - new ListResultSet<>(Collections - .singletonList(new SubmoduleSubscription(sourceBranchNameKey, - targetBranchNameKey, "target-project"))); - expect(subscriptions.bySubmodule(targetBranchNameKey)).andReturn( - incorrectSubscriptions); + schema.close(); - schema.close(); + final PersonIdent myIdent = + new PersonIdent("test-user", "test-user@email.com"); - final PersonIdent myIdent = - new PersonIdent("test-user", "test-user@email.com"); + doReplay(); - doReplay(); + final SubmoduleOp submoduleOp = + new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk( + sourceRepository), urlProvider, schemaFactory, sourceRepository, + new Project(sourceBranchNameKey.getParentKey()), submitted, + mergedCommits, myIdent, repoManager, gitRefUpdated, null, changeHooks); - final SubmoduleOp submoduleOp = - new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk( - sourceRepository), urlProvider, schemaFactory, sourceRepository, - new Project(sourceBranchNameKey.getParentKey()), submitted, - mergedCommits, myIdent, repoManager, gitRefUpdated, null, changeHooks); + submoduleOp.update(); - submoduleOp.update(); - - doVerify(); - RefUpdate ru = ruCapture.getValue(); - assertEquals(ru.getName(), targetBranchNameKey.get()); + doVerify(); + RefUpdate ru = ruCapture.getValue(); + assertEquals(ru.getName(), targetBranchNameKey.get()); + } } /** @@ -862,67 +873,70 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { final List previousSubscriptions) throws Exception { expect(schemaFactory.open()).andReturn(schema); - final Repository realDb = createWorkRepository(); - final Git git = new Git(realDb); + try (Repository realDb = createWorkRepository()) { + // TODO(dborowitz): Use try/finally when this doesn't double-close the repo. + @SuppressWarnings("resource") + final Git git = new Git(realDb); - addRegularFileToIndex(".gitmodules", gitModulesFileContent, realDb); + addRegularFileToIndex(".gitmodules", gitModulesFileContent, realDb); - final RevCommit mergeTip = git.commit().setMessage("test").call(); + final RevCommit mergeTip = git.commit().setMessage("test").call(); - expect(urlProvider.get()).andReturn("http://localhost:8080").times(2); + expect(urlProvider.get()).andReturn("http://localhost:8080").times(2); - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - expect(subscriptions.bySuperProject(mergedBranch)).andReturn( - new ListResultSet<>(previousSubscriptions)); - - SortedSet existingProjects = new TreeSet<>(); - - for (SubmoduleSubscription extracted : extractedSubscriptions) { - existingProjects.add(extracted.getSubmodule().getParentKey()); - } - - for (int index = 0; index < extractedSubscriptions.size(); index++) { - expect(repoManager.list()).andReturn(existingProjects); - } - - final Set alreadySubscribeds = new HashSet<>(); - for (SubmoduleSubscription s : extractedSubscriptions) { - if (previousSubscriptions.contains(s)) { - alreadySubscribeds.add(s); - } - } - - final Set subscriptionsToRemove = - new HashSet<>(previousSubscriptions); - final List subscriptionsToInsert = - new ArrayList<>(extractedSubscriptions); - - subscriptionsToRemove.removeAll(subscriptionsToInsert); - subscriptionsToInsert.removeAll(alreadySubscribeds); - - if (!subscriptionsToRemove.isEmpty()) { expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - subscriptions.delete(subscriptionsToRemove); + expect(subscriptions.bySuperProject(mergedBranch)).andReturn( + new ListResultSet<>(previousSubscriptions)); + + SortedSet existingProjects = new TreeSet<>(); + + for (SubmoduleSubscription extracted : extractedSubscriptions) { + existingProjects.add(extracted.getSubmodule().getParentKey()); + } + + for (int index = 0; index < extractedSubscriptions.size(); index++) { + expect(repoManager.list()).andReturn(existingProjects); + } + + final Set alreadySubscribeds = new HashSet<>(); + for (SubmoduleSubscription s : extractedSubscriptions) { + if (previousSubscriptions.contains(s)) { + alreadySubscribeds.add(s); + } + } + + final Set subscriptionsToRemove = + new HashSet<>(previousSubscriptions); + final List subscriptionsToInsert = + new ArrayList<>(extractedSubscriptions); + + subscriptionsToRemove.removeAll(subscriptionsToInsert); + subscriptionsToInsert.removeAll(alreadySubscribeds); + + if (!subscriptionsToRemove.isEmpty()) { + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + subscriptions.delete(subscriptionsToRemove); + } + + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + subscriptions.insert(subscriptionsToInsert); + + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + expect(subscriptions.bySubmodule(mergedBranch)).andReturn( + new ListResultSet<>(new ArrayList())); + + schema.close(); + + doReplay(); + + final SubmoduleOp submoduleOp = + new SubmoduleOp(mergedBranch, mergeTip, new RevWalk(realDb), + urlProvider, schemaFactory, realDb, new Project(mergedBranch + .getParentKey()), new ArrayList(), null, null, + repoManager, null, null, null); + + submoduleOp.update(); } - - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - subscriptions.insert(subscriptionsToInsert); - - expect(schema.submoduleSubscriptions()).andReturn(subscriptions); - expect(subscriptions.bySubmodule(mergedBranch)).andReturn( - new ListResultSet<>(new ArrayList())); - - schema.close(); - - doReplay(); - - final SubmoduleOp submoduleOp = - new SubmoduleOp(mergedBranch, mergeTip, new RevWalk(realDb), - urlProvider, schemaFactory, realDb, new Project(mergedBranch - .getParentKey()), new ArrayList(), null, null, - repoManager, null, null, null); - - submoduleOp.update(); } /** diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java index 53d9fb1c99..c41c4ecf0c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesParserTest.java @@ -43,7 +43,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { @After public void tearDownTestRepo() throws Exception { - walk.release(); + walk.close(); } @Test @@ -176,8 +176,7 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { private RevCommit writeCommit(String body, PersonIdent author) throws Exception { - ObjectInserter ins = testRepo.getRepository().newObjectInserter(); - try { + try (ObjectInserter ins = testRepo.getRepository().newObjectInserter()) { CommitBuilder cb = new CommitBuilder(); cb.setAuthor(author); cb.setCommitter(new PersonIdent(serverIdent, author.getWhen())); @@ -188,8 +187,6 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest { RevCommit commit = walk.parseCommit(id); walk.parseBody(commit); return commit; - } finally { - ins.release(); } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index db17f8059b..aea966a281 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -348,13 +348,10 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { hashtags.add("tag2"); update.setHashtags(hashtags); update.commit(); - RevWalk walk = new RevWalk(repo); - try { + try (RevWalk walk = new RevWalk(repo)) { RevCommit commit = walk.parseCommit(update.getRevision()); walk.parseBody(commit); assertTrue(commit.getFullMessage().endsWith("Hashtags: tag1,tag2\n")); - } finally { - walk.release(); } } @@ -433,8 +430,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update2.putApproval("Code-Review", (short) 2); update2.writeCommit(batch); - RevWalk rw = new RevWalk(repo); - try { + try (RevWalk rw = new RevWalk(repo)) { batch.commit(); bru.execute(rw, NullProgressMonitor.INSTANCE); @@ -464,7 +460,6 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { notesWithApprovals.close(); } finally { batch.close(); - rw.release(); } } @@ -506,11 +501,8 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { assertEquals(update1.getRefName(), cmds.get(0).getRefName()); assertEquals(update2.getRefName(), cmds.get(1).getRefName()); - RevWalk rw = new RevWalk(repo); - try { + try (RevWalk rw = new RevWalk(repo)) { bru.execute(rw, NullProgressMonitor.INSTANCE); - } finally { - rw.release(); } assertEquals(ReceiveCommand.Result.OK, cmds.get(0).getResult()); @@ -712,43 +704,44 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { ChangeNotes notes = newNotes(c); - RevWalk walk = new RevWalk(repo); - ArrayList notesInTree = - Lists.newArrayList(notes.getNoteMap().iterator()); - Note note = Iterables.getOnlyElement(notesInTree); + try (RevWalk walk = new RevWalk(repo)) { + ArrayList notesInTree = + Lists.newArrayList(notes.getNoteMap().iterator()); + Note note = Iterables.getOnlyElement(notesInTree); - byte[] bytes = - walk.getObjectReader().open( - note.getData(), Constants.OBJ_BLOB).getBytes(); - String noteString = new String(bytes, UTF_8); - assertEquals("Patch-set: 1\n" - + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n" - + "File: file1\n" - + "\n" - + "1:1-2:1\n" - + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" - + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid1\n" - + "Bytes: 9\n" - + "comment 1\n" - + "\n" - + "2:1-3:1\n" - + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" - + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid2\n" - + "Bytes: 9\n" - + "comment 2\n" - + "\n" - + "File: file2\n" - + "\n" - + "3:1-4:1\n" - + CommentsInNotesUtil.formatTime(serverIdent, time3) + "\n" - + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid3\n" - + "Bytes: 9\n" - + "comment 3\n" - + "\n", - noteString); + byte[] bytes = + walk.getObjectReader().open( + note.getData(), Constants.OBJ_BLOB).getBytes(); + String noteString = new String(bytes, UTF_8); + assertEquals("Patch-set: 1\n" + + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n" + + "File: file1\n" + + "\n" + + "1:1-2:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid1\n" + + "Bytes: 9\n" + + "comment 1\n" + + "\n" + + "2:1-3:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid2\n" + + "Bytes: 9\n" + + "comment 2\n" + + "\n" + + "File: file2\n" + + "\n" + + "3:1-4:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time3) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid3\n" + + "Bytes: 9\n" + + "comment 3\n" + + "\n", + noteString); + } } @Test @@ -782,34 +775,35 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { ChangeNotes notes = newNotes(c); - RevWalk walk = new RevWalk(repo); - ArrayList notesInTree = - Lists.newArrayList(notes.getNoteMap().iterator()); - Note note = Iterables.getOnlyElement(notesInTree); + try (RevWalk walk = new RevWalk(repo)) { + ArrayList notesInTree = + Lists.newArrayList(notes.getNoteMap().iterator()); + Note note = Iterables.getOnlyElement(notesInTree); - byte[] bytes = - walk.getObjectReader().open( - note.getData(), Constants.OBJ_BLOB).getBytes(); - String noteString = new String(bytes, UTF_8); - assertEquals("Base-for-patch-set: 1\n" - + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n" - + "File: file1\n" - + "\n" - + "1:1-2:1\n" - + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" - + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid1\n" - + "Bytes: 9\n" - + "comment 1\n" - + "\n" - + "2:1-3:1\n" - + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" - + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid2\n" - + "Bytes: 9\n" - + "comment 2\n" - + "\n", - noteString); + byte[] bytes = + walk.getObjectReader().open( + note.getData(), Constants.OBJ_BLOB).getBytes(); + String noteString = new String(bytes, UTF_8); + assertEquals("Base-for-patch-set: 1\n" + + "Revision: abcd1234abcd1234abcd1234abcd1234abcd1234\n" + + "File: file1\n" + + "\n" + + "1:1-2:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid1\n" + + "Bytes: 9\n" + + "comment 1\n" + + "\n" + + "2:1-3:1\n" + + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" + + "Author: Other Account <2@gerrit>\n" + + "UUID: uuid2\n" + + "Bytes: 9\n" + + "comment 2\n" + + "\n", + noteString); + } } @Test diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java index 86f9702f83..328509ae7c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java @@ -242,13 +242,10 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { if (id instanceof RevCommit) { return (RevCommit) id; } - RevWalk walk = new RevWalk(repo); - try { + try (RevWalk walk = new RevWalk(repo)) { RevCommit commit = walk.parseCommit(id); walk.parseBody(commit); return commit; - } finally { - walk.release(); } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/tools/hooks/CommitMsgHookTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/tools/hooks/CommitMsgHookTest.java index 17fe068581..9c8e86aa9d 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/tools/hooks/CommitMsgHookTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/tools/hooks/CommitMsgHookTest.java @@ -423,21 +423,17 @@ public class CommitMsgHookTest extends HookTestCase { } private DirCacheEntry file(final String name) throws IOException { - final ObjectInserter oi = repository.newObjectInserter(); - try { + try (ObjectInserter oi = repository.newObjectInserter()) { final DirCacheEntry e = new DirCacheEntry(name); e.setFileMode(FileMode.REGULAR_FILE); e.setObjectId(oi.insert(Constants.OBJ_BLOB, Constants.encode(name))); oi.flush(); return e; - } finally { - oi.release(); } } private void setHEAD() throws Exception { - final ObjectInserter oi = repository.newObjectInserter(); - try { + try (ObjectInserter oi = repository.newObjectInserter()) { final CommitBuilder commit = new CommitBuilder(); commit.setTreeId(oi.insert(Constants.OBJ_TREE, new byte[] {})); commit.setAuthor(author); @@ -456,8 +452,6 @@ public class CommitMsgHookTest extends HookTestCase { default: fail(Constants.HEAD + " did not change: " + ref.getResult()); } - } finally { - oi.release(); } } } diff --git a/plugins/reviewnotes b/plugins/reviewnotes index ba824869c6..603b7b3885 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit ba824869c6b24348647f26e04cf80e1ae82266ec +Subproject commit 603b7b3885a1d2953ca891f58d2a6095e72e5313 From 123ae3474287701f473a14f7fc13f3d804fb0856 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 25 Mar 2015 20:29:40 +0000 Subject: [PATCH 15/36] Remove unused imports Change-Id: I02a8e3a4968492ee0282d8d8daa68b6e47193304 --- .../main/java/com/google/gerrit/pgm/util/SiteProgram.java | 6 ------ .../java/com/google/gerrit/httpd/WebAppInitializer.java | 2 -- 2 files changed, 8 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java index 7e0aa1e53b..9763b1a3f6 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java @@ -25,7 +25,6 @@ import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GerritServerConfigModule; import com.google.gerrit.server.config.SitePath; -import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.git.LocalDiskRepositoryManager; import com.google.gerrit.server.schema.DataSourceModule; import com.google.gerrit.server.schema.DataSourceProvider; @@ -42,21 +41,16 @@ import com.google.inject.Injector; import com.google.inject.Key; import com.google.inject.Module; import com.google.inject.Provider; -import com.google.inject.ProvisionException; import com.google.inject.TypeLiteral; import com.google.inject.name.Named; import com.google.inject.name.Names; import com.google.inject.spi.Message; import com.google.inject.util.Providers; -import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.storage.file.FileBasedConfig; -import org.eclipse.jgit.util.FS; import org.kohsuke.args4j.Option; import java.io.File; -import java.io.IOException; import java.lang.annotation.Annotation; import java.sql.Connection; import java.sql.SQLException; diff --git a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java index a0728e9acd..c579f45728 100644 --- a/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java +++ b/gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java @@ -56,9 +56,7 @@ import com.google.gerrit.server.schema.DataSourceType; import com.google.gerrit.server.schema.DatabaseModule; import com.google.gerrit.server.schema.SchemaModule; import com.google.gerrit.server.schema.SchemaVersionCheck; -import com.google.gerrit.server.securestore.SecureStore; import com.google.gerrit.server.securestore.SecureStoreClassName; -import com.google.gerrit.server.securestore.SecureStoreProvider; import com.google.gerrit.server.ssh.NoSshModule; import com.google.gerrit.server.ssh.SshAddressesModule; import com.google.gerrit.solr.SolrIndexModule; From 87b782b16bc876a44692e955cd764f78ee8af9f9 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 18 Mar 2015 07:54:10 +0100 Subject: [PATCH 16/36] Add log messages to troubleshoot OAuth/OpenID linking Change-Id: Ic8e13eb570d66e144520c29cd65308ce1f1d15c1 --- .../com/google/gerrit/httpd/auth/oauth/OAuthSession.java | 5 +++++ .../google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java | 1 + 2 files changed, 6 insertions(+) diff --git a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java index 27a1f57144..6e3ea7aad6 100644 --- a/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java +++ b/gerrit-oauth/src/main/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java @@ -124,6 +124,7 @@ class OAuthSession { if (claimedId != null && actualId != null) { if (claimedId.equals(actualId)) { // Both link to the same account, that's what we expected. + log.debug("OAuth2: claimed identity equals current id"); } else { // This is (for now) a fatal error. There are two records // for what might be the same user. @@ -138,6 +139,8 @@ class OAuthSession { } else if (claimedId != null && actualId == null) { // Claimed account already exists: link to it. // + log.info("OAuth2: linking claimed identity to {}", + claimedId.toString()); try { accountManager.link(claimedId, areq); } catch (OrmException e) { @@ -148,6 +151,8 @@ class OAuthSession { return; } } + } else { + log.debug("OAuth2: claimed identity is empty"); } areq.setUserName(user.getUserName()); areq.setEmailAddress(user.getEmailAddress()); diff --git a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java index 1681bc248e..44b58c39ed 100644 --- a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java +++ b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java @@ -157,6 +157,7 @@ class OpenIdServiceImpl { final AuthRequest aReq; try { aReq = manager.authenticate(state.discovered, state.retTo.toString()); + log.debug("OpenID: openid-realm={}", state.contextUrl); aReq.setRealm(state.contextUrl); if (requestRegistration(aReq)) { From 4a3ab5fe5210c0a967d19ffcb1c678c37450551b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 2 Mar 2015 08:33:42 -0500 Subject: [PATCH 17/36] Do not return 403 when clicking on Gitweb breadcrumb A message "Forbidden" was displayed when clicking on any part of the breadcrumb trail displayed at the top of Gitweb pages. That happened for projects with parent folders because browsing parent folders is not supported by Gerrit. Now when the user clicks on the parent folder, redirect to Gerrit projects list screen with the parent folder path as the filter. Change-Id: I86dfb3f29d8da6ee02efc95470673fe70a1f2d3e --- .../com/google/gerrit/httpd/gitweb/GitWebServlet.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java index f593caca84..8f0a2025a6 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitWebServlet.java @@ -29,6 +29,7 @@ package com.google.gerrit.httpd.gitweb; +import com.google.gerrit.common.PageLinks; import com.google.gerrit.common.data.GerritConfig; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.httpd.GitWebConfig; @@ -79,6 +80,8 @@ class GitWebServlet extends HttpServlet { private static final Logger log = LoggerFactory.getLogger(GitWebServlet.class); + private static final String PROJECT_LIST_ACTION = "project_list"; + private final Set deniedActions; private final int bufferSize = 8192; private final File gitwebCgi; @@ -119,7 +122,6 @@ class GitWebServlet extends HttpServlet { deniedActions.add("forks"); deniedActions.add("opml"); - deniedActions.add("project_list"); deniedActions.add("project_index"); _env = new EnvList(); @@ -363,6 +365,12 @@ class GitWebServlet extends HttpServlet { return; } + if (params.get("a").equals(PROJECT_LIST_ACTION)) { + rsp.sendRedirect(req.getContextPath() + "/#" + PageLinks.ADMIN_PROJECTS + + "?filter=" + Url.encode(params.get("pf") + "/")); + return; + } + String name = params.get("p"); if (name == null) { rsp.sendError(HttpServletResponse.SC_NOT_FOUND); From a86b3fc40bb60345ce784bec90e9891fc5a7245f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 26 Mar 2015 16:14:55 +0100 Subject: [PATCH 18/36] Release notes for Gerrit 2.10.2 Change-Id: Ie1f5c51e498864cbc719f5cd4b1a511f87067381 --- ReleaseNotes/ReleaseNotes-2.10.2.txt | 31 ++++++++++++++++++++++++++++ ReleaseNotes/index.txt | 1 + 2 files changed, 32 insertions(+) create mode 100644 ReleaseNotes/ReleaseNotes-2.10.2.txt diff --git a/ReleaseNotes/ReleaseNotes-2.10.2.txt b/ReleaseNotes/ReleaseNotes-2.10.2.txt new file mode 100644 index 0000000000..2ca5505deb --- /dev/null +++ b/ReleaseNotes/ReleaseNotes-2.10.2.txt @@ -0,0 +1,31 @@ +Release notes for Gerrit 2.10.2 +=============================== + +There are no schema changes from link:ReleaseNotes-2.10.1.html[2.10.1]. + +Download: +link:https://gerrit-releases.storage.googleapis.com/gerrit-2.10.2.war[ +https://gerrit-releases.storage.googleapis.com/gerrit-2.10.2.war] + +Bug Fixes +--------- + +* Work around MyersDiff infinite loop in PatchListLoader. If the MyersDiff diff +doesn't finish within 5 seconds, interrupt it and fall back to a different diff +algorithm. From the user perspective, the only difference when the infinite +loop is detected is that the files in the commit will not be compared in-depth, +which will result in bigger edit regions. + +Secondary Index +--------------- + +* Online reindexing: log the number of done/failed changes in the error_log. +Administrators can use the logged information to decide whether to activate the +new index version or not. + +Gitweb +------ + +* Do not return `Forbidden` when clicking on Gitweb breadcrumb. Now when the +user clicks on the parent folder, redirect to Gerrit projects list screen with +the parent folder path as the filter. diff --git a/ReleaseNotes/index.txt b/ReleaseNotes/index.txt index bc5677cd99..02aa9be64b 100644 --- a/ReleaseNotes/index.txt +++ b/ReleaseNotes/index.txt @@ -4,6 +4,7 @@ Gerrit Code Review - Release Notes [[2_10]] Version 2.10.x -------------- +* link:ReleaseNotes-2.10.2.html[2.10.2] * link:ReleaseNotes-2.10.1.html[2.10.1] * link:ReleaseNotes-2.10.html[2.10] From 329c32347336214b496f2846d05a80fe927c673e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 26 Mar 2015 16:20:01 +0100 Subject: [PATCH 19/36] Update version to 2.10.2 Change-Id: I1d2c4096151a8451c6cff0dc136eea84808a0779 --- VERSION | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-archetype/pom.xml | 2 +- gerrit-plugin-gwt-archetype/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-plugin-js-archetype/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/VERSION b/VERSION index a2eeb3966c..5016f9433a 100644 --- a/VERSION +++ b/VERSION @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = '2.10.1' +GERRIT_VERSION = '2.10.2' diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 1ab7bf326d..112d46859a 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.10.1 + 2.10.2 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index 741b70fe93..530bf7b177 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.10.1 + 2.10.2 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-archetype/pom.xml b/gerrit-plugin-archetype/pom.xml index 287f12f49b..b546c1465f 100644 --- a/gerrit-plugin-archetype/pom.xml +++ b/gerrit-plugin-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-archetype - 2.10.1 + 2.10.2 Gerrit Code Review - Plugin Archetype Maven Archetype for Gerrit Plugins http://code.google.com/p/gerrit/ diff --git a/gerrit-plugin-gwt-archetype/pom.xml b/gerrit-plugin-gwt-archetype/pom.xml index 3f57e62195..a35b7edc67 100644 --- a/gerrit-plugin-gwt-archetype/pom.xml +++ b/gerrit-plugin-gwt-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-gwt-archetype - 2.10.1 + 2.10.2 Gerrit Code Review - Web UI GWT Plugin Archetype Maven Archetype for Gerrit Web UI GWT Plugins http://code.google.com/p/gerrit/ diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index f3e52195de..b499c963ca 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.10.1 + 2.10.2 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-plugin-js-archetype/pom.xml b/gerrit-plugin-js-archetype/pom.xml index 7fe1ee84ad..d10d907d33 100644 --- a/gerrit-plugin-js-archetype/pom.xml +++ b/gerrit-plugin-js-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-js-archetype - 2.10.1 + 2.10.2 Gerrit Code Review - Web UI JavaScript Plugin Archetype Maven Archetype for Gerrit Web UI JavaScript Plugins http://code.google.com/p/gerrit/ diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index 78b351183b..e61ce0ee28 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.10.1 + 2.10.2 war Gerrit Code Review - WAR Gerrit WAR From 3b489e65c560d9cd24dfc3a6cea6acbfaf6085cc Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Sun, 29 Mar 2015 20:56:09 +0100 Subject: [PATCH 20/36] Update 2.11 release notes Change-Id: Id3edae9b4fc5602cc16a5761e8d9b5d483fb9b37 --- ReleaseNotes/ReleaseNotes-2.11.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ReleaseNotes/ReleaseNotes-2.11.txt b/ReleaseNotes/ReleaseNotes-2.11.txt index 576bcf3982..a976612e20 100644 --- a/ReleaseNotes/ReleaseNotes-2.11.txt +++ b/ReleaseNotes/ReleaseNotes-2.11.txt @@ -8,7 +8,8 @@ link:https://gerrit-releases.storage.googleapis.com/gerrit-2.11.war[ https://gerrit-releases.storage.googleapis.com/gerrit-2.11.war] Gerrit 2.11 includes the bug fixes done with -link:ReleaseNotes-2.10.1.html[Gerrit 2.10.1]. +link:ReleaseNotes-2.10.1.html[Gerrit 2.10.1] and +link:ReleaseNotes-2.10.2.html[Gerrit 2.10.2]. These bug fixes are *not* listed in these release notes. From ba87b42804f0dfeccc27abb21eee8cfef8f19ec3 Mon Sep 17 00:00:00 2001 From: Hector Caballero Date: Wed, 11 Mar 2015 16:52:21 -0400 Subject: [PATCH 21/36] Fix F5 shortcut being hijacked by edit topic in Firefox In change screen, if the F5 key was pressed the topic text field opened instead of refreshing the page. In Google Chrome, F5 works as intended. The problem seems to be the way GWT manages the keypress events for some keys in different browsers and an open issue addresses this: https://code.google.com/p/google-web-toolkit/issues/detail?id=1061 This patch is a workaround to avoid the problem in Firefox and all Gecko based browsers until the GWT issue is resolved. Bug: Issue 2815 Change-Id: I41a4c117d39db7437ed352d10a804d8e8ada8f07 --- .../java/com/google/gerrit/client/change/ChangeScreen.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java index 8a27fd049a..b875772c7c 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java @@ -1091,6 +1091,12 @@ public class ChangeScreen extends Screen { keysAction.add(new KeyCommand(0, 't', Util.C.keyEditTopic()) { @Override public void onKeyPress(KeyPressEvent event) { + // In Firefox this event is mistakenly called when F5 is pressed so + // differentiate F5 from 't' by checking the charCode(F5=0, t=116). + if (event.getNativeEvent().getCharCode() == 0) { + Window.Location.reload(); + return; + } topic.onEdit(); } }); From 4f4056b20d9c0da7c719317c4d253a415bec2265 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 1 Apr 2015 12:36:45 -0700 Subject: [PATCH 22/36] Fix NPE in MergeabilityCache.getIfPresent LoadingCache.getIfPresent is explicitly @Nullable. Unfortunately, when it returned null, it was being unboxed to a boolean, causing an NPE. This bug has been around since the mergeability implementation was rewritten, but has likely not been very common because mergeability bits are computed pretty aggressively (if asynchronously). Change-Id: Ie3d8e16db76b28056832c002ab0899072d0ea356 (cherry picked from commit 4193e33184ec966e0ad0e96358f1426fe508671a) --- .../com/google/gerrit/server/change/MergeabilityCache.java | 4 ++-- .../google/gerrit/server/change/MergeabilityCacheImpl.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCache.java index c8add605ca..547a5009a2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCache.java @@ -33,7 +33,7 @@ public interface MergeabilityCache { } @Override - public boolean getIfPresent(ObjectId commit, Ref intoRef, + public Boolean getIfPresent(ObjectId commit, Ref intoRef, SubmitType submitType, String mergeStrategy) { throw new UnsupportedOperationException("Mergeability checking disabled"); } @@ -42,6 +42,6 @@ public interface MergeabilityCache { public boolean get(ObjectId commit, Ref intoRef, SubmitType submitType, String mergeStrategy, Branch.NameKey dest, Repository repo, ReviewDb db); - public boolean getIfPresent(ObjectId commit, Ref intoRef, + public Boolean getIfPresent(ObjectId commit, Ref intoRef, SubmitType submitType, String mergeStrategy); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java index d6b6ab8376..a78b6624b6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java @@ -304,7 +304,7 @@ public class MergeabilityCacheImpl implements MergeabilityCache { } @Override - public boolean getIfPresent(ObjectId commit, Ref intoRef, + public Boolean getIfPresent(ObjectId commit, Ref intoRef, SubmitType submitType, String mergeStrategy) { return cache.getIfPresent( new EntryKey(commit, toId(intoRef), submitType, mergeStrategy)); From bb34c344965701caa43e19f5b4fa96cd5bcf192c Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 26 Mar 2015 12:16:31 -0700 Subject: [PATCH 23/36] Change edit shortcut from 'm' to 'e' I keep missing the shift key when typing shift-m to mark a file as reviewed and move to next file. With 'm' bound to open file in editor this is launching me into the edit screen, which is annoying to get out of. Swap to 'e' which is currently not bound to anything. Change-Id: Ia92e1b71cda34bd7d5fd77e8d0423670114e1a0c (cherry picked from commit cb9cc54762c70229615a6dba4d462ad1aa5cc43f) --- .../java/com/google/gerrit/client/diff/SideBySide.java | 8 ++++---- .../com/google/gerrit/client/patches/PatchConstants.java | 2 +- .../gerrit/client/patches/PatchConstants.properties | 3 +-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java index 9566a0b559..d06b59e05d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide.java @@ -371,7 +371,7 @@ public class SideBySide extends Screen { .on("O", commentManager.toggleOpenBox(cm)) .on("Enter", commentManager.toggleOpenBox(cm)) .on("N", maybeNextVimSearch(cm)) - .on("M", modifyInEditScreen(cm)) + .on("E", openEditScreen(cm)) .on("P", chunkManager.diffChunkNav(cm, Direction.PREV)) .on("Shift-A", diffTable.toggleA()) .on("Shift-M", header.reviewedAndNext()) @@ -486,9 +486,9 @@ public class SideBySide extends Screen { new NoOpKeyCommand(KeyCommand.M_SHIFT, 'p', PatchUtil.C.commentPrev())); keysNavigation.add( new NoOpKeyCommand(KeyCommand.M_CTRL, 'f', Gerrit.C.keySearch())); - keysNavigation.add( - new NoOpKeyCommand(0, 'm', PatchUtil.C.modifyInEditScreen())); + keysAction = new KeyCommandSet(Gerrit.C.sectionActions()); + keysAction.add(new NoOpKeyCommand(0, 'e', PatchUtil.C.openEditScreen())); keysAction.add(new NoOpKeyCommand(0, KeyCodes.KEY_ENTER, PatchUtil.C.expandComment())); keysAction.add(new NoOpKeyCommand(0, 'o', PatchUtil.C.expandComment())); @@ -938,7 +938,7 @@ public class SideBySide extends Screen { } } - private Runnable modifyInEditScreen(final CodeMirror cm) { + private Runnable openEditScreen(final CodeMirror cm) { return new Runnable() { @Override public void run() { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java index 239d2d588b..39aadc3393 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.java @@ -58,7 +58,7 @@ public interface PatchConstants extends Constants { String toggleIntraline(); String showPreferences(); - String modifyInEditScreen(); + String openEditScreen(); String toggleReviewed(); String markAsReviewedAndGoToNext(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties index 8f026be78d..2f68822825 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchConstants.properties @@ -40,8 +40,7 @@ toggleSideA = Toggle left side toggleIntraline = Toggle intraline difference showPreferences = Show diff preferences -modifyInEditScreen = Modify in edit screen - +openEditScreen = Edit file in browser toggleReviewed = Toggle the reviewed flag markAsReviewedAndGoToNext = Mark patch as reviewed and go to next unreviewed patch From 314c44882245b19cbea0b4037588f330800d6cab Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 2 Apr 2015 10:28:10 -0700 Subject: [PATCH 24/36] Recommend --data-binary flag when testing submit rules from curl Change-Id: I446750051f9f8be7fdd8ef7a93307303c6746a2d (cherry picked from commit b42e303d387287d3de4ca867cde745003af4357a) --- Documentation/rest-api-changes.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 49ebb49a84..98dd21c3a3 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -2574,6 +2574,17 @@ describing the permutations that satisfy the tested submit rule. ] ---- +When testing with the `curl` command line client the +`--data-binary @rules.pl` flag should be used to ensure +all LFs are included in the Prolog code: + +---- + curl -X POST \ + -H 'Content-Type: text/plain; charset=UTF-8' \ + --data-binary @rules.pl \ + http://.../test.submit_rule +---- + [[list-drafts]] === List Drafts -- From e2921b62f6c09d574a25aaa079d538ac499ef382 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 4 Mar 2015 22:36:10 +0100 Subject: [PATCH 25/36] Revert "Downgrade SSHD to 0.9.0-4-g5967cfd" All versions of SSHD since release 0.10 were suffering from exhaustion of thread pool. Number of valuable features had to be reverted to downgrade the SSHD version to 0.9. This blocking bug [1] was fixed [2] and released in 0.14.0. Update to the new version of SSHD and revert the downgrade. This reverts commit bde8e9ac6f26a85c1a757ac0fa298f8b0c3c5783. [1] https://issues.apache.org/jira/browse/SSHD-348 [2] https://git-wip-us.apache.org/repos/asf?p=mina-sshd.git;a=commitdiff;h=964e76890cf56da4491199860d0ea8276fbd26a6 Change-Id: Ib5faf1df0cb6bde2e2cd554c9311cc5e55095b04 --- Documentation/config-gerrit.txt | 8 +++++ .../com/google/gerrit/pgm/libraries.config | 12 +++---- .../server/contact/EncryptedContactStore.java | 13 ++++++-- .../com/google/gerrit/sshd/SshDaemon.java | 31 ++++++++++++++++--- lib/bouncycastle/BUCK | 8 ++--- lib/mina/BUCK | 9 +++--- 6 files changed, 59 insertions(+), 22 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 3368db9f58..f43ea16ee2 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2863,6 +2863,14 @@ namespace. To alias `replication start` to `gerrit replicate`: [[sshd]] === Section sshd +[[sshd.backend]]sshd.backend:: ++ +Starting from version 0.9.0 Apache SSHD project added support for NIO2 +IoSession. To use the new NIO2 session the `backend` option must be set +to `NIO2`. ++ +By default, `MINA`. + [[sshd.listenAddress]]sshd.listenAddress:: + Specifies the local addresses the internal SSHD should listen diff --git a/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/libraries.config b/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/libraries.config index b5e702f3d4..16bceeeb51 100644 --- a/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/libraries.config +++ b/gerrit-pgm/src/main/resources/com/google/gerrit/pgm/libraries.config @@ -15,16 +15,16 @@ # Version should match lib/bouncycastle/BUCK [library "bouncyCastleProvider"] - name = Bouncy Castle Crypto Provider v149 - url = http://www.bouncycastle.org/download/bcprov-jdk15on-149.jar - sha1 = f5155f04330459104b79923274db5060c1057b99 + name = Bouncy Castle Crypto Provider v151 + url = http://www.bouncycastle.org/download/bcprov-jdk15on-151.jar + sha1 = 9ab8afcc2842d5ef06eb775a0a2b12783b99aa80 remove = bcprov-.*[.]jar # Version should match lib/bouncycastle/BUCK [library "bouncyCastleSSL"] - name = Bouncy Castle Crypto SSL v149 - url = http://www.bouncycastle.org/download/bcpkix-jdk15on-149.jar - sha1 = 924cc7ad2f589630c97b918f044296ebf1bb6855 + name = Bouncy Castle Crypto SSL v151 + url = http://www.bouncycastle.org/download/bcpkix-jdk15on-151.jar + sha1 = 6c8c1f61bf27a09f9b1a8abc201523669bba9597 needs = bouncyCastleProvider remove = bcpkix-.*[.]jar diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java index 8c1fdb6b79..f43e976e81 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java @@ -38,6 +38,9 @@ import org.bouncycastle.openpgp.PGPPublicKey; import org.bouncycastle.openpgp.PGPPublicKeyRing; import org.bouncycastle.openpgp.PGPPublicKeyRingCollection; import org.bouncycastle.openpgp.PGPUtil; +import org.bouncycastle.openpgp.bc.BcPGPPublicKeyRingCollection; +import org.bouncycastle.openpgp.operator.bc.BcPGPDataEncryptorBuilder; +import org.bouncycastle.openpgp.operator.bc.BcPublicKeyKeyEncryptionMethodGenerator; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -167,12 +170,16 @@ class EncryptedContactStore implements ContactStore { } } - @SuppressWarnings("deprecation") private final PGPEncryptedDataGenerator cpk() throws NoSuchProviderException, PGPException { + final BcPGPDataEncryptorBuilder builder = + new BcPGPDataEncryptorBuilder(PGPEncryptedData.CAST5) + .setSecureRandom(prng); PGPEncryptedDataGenerator cpk = - new PGPEncryptedDataGenerator(PGPEncryptedData.CAST5, true, prng, "BC"); - cpk.addMethod(dest); + new PGPEncryptedDataGenerator(builder, true); + final BcPublicKeyKeyEncryptionMethodGenerator methodGenerator = + new BcPublicKeyKeyEncryptionMethodGenerator(dest); + cpk.addMethod(methodGenerator); return cpk; } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java index 60514429f8..7f3612bb2c 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java @@ -45,6 +45,7 @@ import org.apache.sshd.common.ForwardingFilter; import org.apache.sshd.common.KeyExchange; import org.apache.sshd.common.KeyPairProvider; import org.apache.sshd.common.NamedFactory; +import org.apache.sshd.common.RequestHandler; import org.apache.sshd.common.Session; import org.apache.sshd.common.Signature; import org.apache.sshd.common.SshdSocketAddress; @@ -67,10 +68,11 @@ import org.apache.sshd.common.forward.TcpipServerChannel; import org.apache.sshd.common.future.CloseFuture; import org.apache.sshd.common.future.SshFutureListener; import org.apache.sshd.common.io.IoAcceptor; -import org.apache.sshd.common.io.IoServiceFactory; +import org.apache.sshd.common.io.IoServiceFactoryFactory; import org.apache.sshd.common.io.IoSession; -import org.apache.sshd.common.io.mina.MinaServiceFactory; +import org.apache.sshd.common.io.mina.MinaServiceFactoryFactory; import org.apache.sshd.common.io.mina.MinaSession; +import org.apache.sshd.common.io.nio2.Nio2ServiceFactoryFactory; import org.apache.sshd.common.mac.HMACMD5; import org.apache.sshd.common.mac.HMACMD596; import org.apache.sshd.common.mac.HMACSHA1; @@ -79,6 +81,7 @@ import org.apache.sshd.common.random.BouncyCastleRandom; import org.apache.sshd.common.random.JceRandom; import org.apache.sshd.common.random.SingletonRandomFactory; import org.apache.sshd.common.session.AbstractSession; +import org.apache.sshd.common.session.ConnectionService; import org.apache.sshd.common.signature.SignatureDSA; import org.apache.sshd.common.signature.SignatureRSA; import org.apache.sshd.common.util.Buffer; @@ -91,6 +94,10 @@ import org.apache.sshd.server.auth.UserAuthPublicKey; import org.apache.sshd.server.auth.gss.GSSAuthenticator; import org.apache.sshd.server.auth.gss.UserAuthGSS; import org.apache.sshd.server.channel.ChannelSession; +import org.apache.sshd.server.global.CancelTcpipForwardHandler; +import org.apache.sshd.server.global.KeepAliveHandler; +import org.apache.sshd.server.global.NoMoreSessionsHandler; +import org.apache.sshd.server.global.TcpipForwardHandler; import org.apache.sshd.server.kex.DHG1; import org.apache.sshd.server.kex.DHG14; import org.apache.sshd.server.session.SessionFactory; @@ -193,8 +200,13 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { final String kerberosPrincipal = cfg.getString( "sshd", null, "kerberosPrincipal"); - System.setProperty(IoServiceFactory.class.getName(), - MinaServiceFactory.class.getName()); + SshSessionBackend backend = cfg.getEnum( + "sshd", null, "backend", SshSessionBackend.MINA); + + System.setProperty(IoServiceFactoryFactory.class.getName(), + backend == SshSessionBackend.MINA + ? MinaServiceFactoryFactory.class.getName() + : Nio2ServiceFactoryFactory.class.getName()); if (SecurityUtils.isBouncyCastleRegistered()) { initProviderBouncyCastle(); @@ -251,6 +263,12 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { return new GerritServerSession(server, ioSession); } }); + setGlobalRequestHandlers(Arrays.> asList( + new KeepAliveHandler(), + new NoMoreSessionsHandler(), + new TcpipForwardHandler(), + new CancelTcpipForwardHandler() + )); hostKeys = computeHostKeys(); } @@ -587,6 +605,11 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { @Override public SshFile getFile(String file) { return null; + } + + @Override + public FileSystemView getNormalizedView() { + return this; }}; } }); diff --git a/lib/bouncycastle/BUCK b/lib/bouncycastle/BUCK index 99f960e81c..d1ec48de1b 100644 --- a/lib/bouncycastle/BUCK +++ b/lib/bouncycastle/BUCK @@ -2,19 +2,19 @@ include_defs('//lib/maven.defs') # This version must match the version that also appears in # gerrit-pgm/src/main/resources/com/google/gerrit/pgm/libraries.config -VERSION = '1.49' +VERSION = '1.51' maven_jar( name = 'bcprov', id = 'org.bouncycastle:bcprov-jdk15on:' + VERSION, - sha1 = 'f5155f04330459104b79923274db5060c1057b99', + sha1 = '9ab8afcc2842d5ef06eb775a0a2b12783b99aa80', license = 'DO_NOT_DISTRIBUTE', #'bouncycastle' ) maven_jar( name = 'bcpg', id = 'org.bouncycastle:bcpg-jdk15on:' + VERSION, - sha1 = '081d84be5b125e1997ab0e2244d1a2276b5de76c', + sha1 = 'b5fa4c280dfbf8bf7c260bc1e78044c7a1de5133', license = 'DO_NOT_DISTRIBUTE', #'bouncycastle' deps = [':bcprov'], ) @@ -22,7 +22,7 @@ maven_jar( maven_jar( name = 'bcpkix', id = 'org.bouncycastle:bcpkix-jdk15on:' + VERSION, - sha1 = '924cc7ad2f589630c97b918f044296ebf1bb6855', + sha1 = '6c8c1f61bf27a09f9b1a8abc201523669bba9597', license = 'DO_NOT_DISTRIBUTE', #'bouncycastle' deps = [':bcprov'], ) diff --git a/lib/mina/BUCK b/lib/mina/BUCK index d866807627..0c9b41ea75 100644 --- a/lib/mina/BUCK +++ b/lib/mina/BUCK @@ -8,18 +8,17 @@ EXCLUDE = [ maven_jar( name = 'sshd', - id = 'org.apache.sshd:sshd-core:0.9.0-4-g5967cfd', - sha1 = '449ec11c4417b295dbf1661585a50c6ec7d9a452', + id = 'org.apache.sshd:sshd-core:0.14.0', + sha1 = 'cb12fa1b1b07fb5ce3aa4f99b189743897bd4fca', license = 'Apache2.0', deps = [':core'], exclude = EXCLUDE, - repository = GERRIT, ) maven_jar( name = 'core', - id = 'org.apache.mina:mina-core:2.0.7', - sha1 = 'c878e2aa82de748474a624ec3933e4604e446dec', + id = 'org.apache.mina:mina-core:2.0.8', + sha1 = 'd6ff69fa049aeaecdf0c04cafbb1ab53b7487883', license = 'Apache2.0', exclude = EXCLUDE, ) From f8a988f91cd7741ff5aeb1c7e2e65184fb89c091 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 4 Mar 2015 22:05:38 +0100 Subject: [PATCH 26/36] Revert "Revert "SSHD: Prevent double authentication for the same public key"" This reverts commit c7dedf989cf1717548b0793490d0be9506c1bc2e. Change-Id: Ic4295ee58db8e0eb8869e526988ac4a3758370ee --- .../sshd/CachingPublicKeyAuthenticator.java | 72 +++++++++++++++++++ .../gerrit/sshd/DatabasePubKeyAuth.java | 23 +++--- .../com/google/gerrit/sshd/SshModule.java | 2 +- 3 files changed, 86 insertions(+), 11 deletions(-) create mode 100644 gerrit-sshd/src/main/java/com/google/gerrit/sshd/CachingPublicKeyAuthenticator.java diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CachingPublicKeyAuthenticator.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CachingPublicKeyAuthenticator.java new file mode 100644 index 0000000000..f315cff3b8 --- /dev/null +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CachingPublicKeyAuthenticator.java @@ -0,0 +1,72 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.sshd; + +import com.google.inject.Inject; +import com.google.inject.Singleton; + +import org.apache.sshd.common.Session; +import org.apache.sshd.common.SessionListener; +import org.apache.sshd.server.PublickeyAuthenticator; +import org.apache.sshd.server.session.ServerSession; + +import java.security.PublicKey; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +@Singleton +public class CachingPublicKeyAuthenticator implements PublickeyAuthenticator, + SessionListener { + + private final PublickeyAuthenticator authenticator; + private final Map> sessionCache; + + @Inject + public CachingPublicKeyAuthenticator(DatabasePubKeyAuth authenticator) { + this.authenticator = authenticator; + this.sessionCache = new ConcurrentHashMap<>(); + } + + @Override + public boolean authenticate(String username, PublicKey key, + ServerSession session) { + Map m = sessionCache.get(session); + if (m == null) { + m = new HashMap<>(); + sessionCache.put(session, m); + session.addListener(this); + } + if (m.containsKey(key)) { + return m.get(key); + } + boolean r = authenticator.authenticate(username, key, session); + m.put(key, r); + return r; + } + + @Override + public void sessionCreated(Session session) { + } + + @Override + public void sessionEvent(Session sesssion, Event event) { + } + + @Override + public void sessionClosed(Session session) { + sessionCache.remove(session); + } +} diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java index a2a56320d9..9c05262fec 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java @@ -14,13 +14,16 @@ package com.google.gerrit.sshd; +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.gerrit.common.FileUtil; +import com.google.common.base.Preconditions; import com.google.gerrit.reviewdb.client.AccountSshKey; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PeerDaemonUser; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.inject.Inject; -import com.google.inject.Singleton; import org.apache.commons.codec.binary.Base64; import org.apache.sshd.common.KeyPairProvider; @@ -48,7 +51,6 @@ import java.util.Set; /** * Authenticates by public key through {@link AccountSshKey} entities. */ -@Singleton class DatabasePubKeyAuth implements PublickeyAuthenticator { private static final Logger log = LoggerFactory.getLogger(DatabasePubKeyAuth.class); @@ -92,10 +94,11 @@ class DatabasePubKeyAuth implements PublickeyAuthenticator { } } - public boolean authenticate(String username, - final PublicKey suppliedKey, final ServerSession session) { - final SshSession sd = session.getAttribute(SshSession.KEY); - + @Override + public boolean authenticate(String username, PublicKey suppliedKey, + ServerSession session) { + SshSession sd = session.getAttribute(SshSession.KEY); + Preconditions.checkState(sd.getCurrentUser() == null); if (PeerDaemonUser.USER_NAME.equals(username)) { if (myHostKeys.contains(suppliedKey) || getPeerKeys().contains(suppliedKey)) { @@ -112,10 +115,10 @@ class DatabasePubKeyAuth implements PublickeyAuthenticator { username = username.toLowerCase(Locale.US); } - final Iterable keyList = sshKeyCache.get(username); - final SshKeyCacheEntry key = find(keyList, suppliedKey); + Iterable keyList = sshKeyCache.get(username); + SshKeyCacheEntry key = find(keyList, suppliedKey); if (key == null) { - final String err; + String err; if (keyList == SshKeyCacheImpl.NO_SUCH_USER) { err = "user-not-found"; } else if (keyList == SshKeyCacheImpl.NO_KEYS) { @@ -133,7 +136,7 @@ class DatabasePubKeyAuth implements PublickeyAuthenticator { // security check to ensure there aren't two users sharing the same // user name on the server. // - for (final SshKeyCacheEntry otherKey : keyList) { + for (SshKeyCacheEntry otherKey : keyList) { if (!key.getAccount().equals(otherKey.getAccount())) { sd.authenticationError(username, "keys-cross-accounts"); return false; diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java index 50ab639b3e..7dd12b0cfd 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java @@ -81,7 +81,7 @@ public class SshModule extends LifecycleModule { bind(QueueProvider.class).to(CommandExecutorQueueProvider.class).in(SINGLETON); bind(GSSAuthenticator.class).to(GerritGSSAuthenticator.class); - bind(PublickeyAuthenticator.class).to(DatabasePubKeyAuth.class); + bind(PublickeyAuthenticator.class).to(CachingPublicKeyAuthenticator.class); bind(ModuleGenerator.class).to(SshAutoRegisterModuleGenerator.class); bind(SshPluginStarterCallback.class); From c8172b20e878685a6fb1adf30d13d628cf8e95ca Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 4 Mar 2015 22:37:10 +0100 Subject: [PATCH 27/36] Revert "Revert "SSHD: Allow ECDSA based public key authentication"" This reverts commit ef74c883e6cc07c0a55f14960dfc31b0feafc39e. Change-Id: I365c57365b4ea7271f104b8040f951b088cf80ab --- .../src/main/java/com/google/gerrit/sshd/SshDaemon.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java index 7f3612bb2c..595b5d26c1 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java @@ -83,6 +83,7 @@ import org.apache.sshd.common.random.SingletonRandomFactory; import org.apache.sshd.common.session.AbstractSession; import org.apache.sshd.common.session.ConnectionService; import org.apache.sshd.common.signature.SignatureDSA; +import org.apache.sshd.common.signature.SignatureECDSA; import org.apache.sshd.common.signature.SignatureRSA; import org.apache.sshd.common.util.Buffer; import org.apache.sshd.common.util.SecurityUtils; @@ -510,7 +511,11 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { private void initSignatures() { setSignatureFactories(Arrays.> asList( - new SignatureDSA.Factory(), new SignatureRSA.Factory())); + new SignatureDSA.Factory(), + new SignatureRSA.Factory(), + new SignatureECDSA.NISTP256Factory(), + new SignatureECDSA.NISTP384Factory(), + new SignatureECDSA.NISTP521Factory())); } private void initCompression() { From 985201b5f959a32b18458822b54efd47b1408f44 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 4 Mar 2015 22:37:33 +0100 Subject: [PATCH 28/36] Revert "Revert "Allow configuration of SSH rekey values"" This reverts commit 3435c536a6024fc2a92610be452ab4d85ae5268c. Change-Id: I4efe2e209ff05e68d8add596025622e76646bfde --- Documentation/config-gerrit.txt | 18 ++++++++++++++++++ .../java/com/google/gerrit/sshd/SshDaemon.java | 9 +++++++++ 2 files changed, 27 insertions(+) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index f43ea16ee2..5b1817a843 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3072,6 +3072,24 @@ programmatic configuration. + By default, true. +[[sshd.rekeyBytesLimit]]sshd.rekeyBytesLimit:: ++ +The SSH daemon will issue a rekeying after a certain amount of data. +This configuration option allows you to tweak that setting. ++ +By default, 1073741824 (bytes, 1GB). ++ +The rekeyBytesLimit cannot be set to lower than 32. + +[[sshd.rekeyTimeLimit]]sshd.rekeyTimeLimit:: ++ +The SSH daemon will issue a rekeying after a certain amount of time. +This configuration option allows you to tweak that setting. ++ +By default, 1h. ++ +Set to 0 to disable this check. + [[suggest]] === Section suggest diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java index 595b5d26c1..5ef4765bb5 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java @@ -189,6 +189,15 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { IDLE_TIMEOUT, String.valueOf(SECONDS.toMillis(idleTimeoutSeconds))); + long rekeyTimeLimit = ConfigUtil.getTimeUnit(cfg, "sshd", null, + "rekeyTimeLimit", 3600, SECONDS); + getProperties().put( + REKEY_TIME_LIMIT, + String.valueOf(SECONDS.toMillis(rekeyTimeLimit))); + + getProperties().put(REKEY_BYTES_LIMIT, + String.valueOf(cfg.getLong("sshd", "rekeyBytesLimit", 1024 * 1024 * 1024 /* 1GB */))); + final int maxConnectionsPerUser = cfg.getInt("sshd", "maxConnectionsPerUser", 64); if (0 < maxConnectionsPerUser) { From 5758f18ee5abef50a2ea998e3b90c8f65aec6e34 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Mon, 30 Mar 2015 11:28:55 -0700 Subject: [PATCH 29/36] Work around asciidoctor handling of nested ` and * In asciidoctor, the markup `refs/heads/foo/*` for [...] `refs/heads/sandbox/${username}/*`. If you do, renders as refs/heads/foo/ for [...] refs/heads/sandbox/${username}/. If you do, which is problematic in a few ways: - it is invalid HTML - the asterisks are swallowed - a section of unrelated text is made bold Since asciidoc 8.4.1, by contrast, backticks introduce an inline literal inside which markup is not interpreted (see git.git commit v1.7.10.3~16^2, "docs: stop using asciidoc no-inline-literal", 2012-04-26). We can emulate that behavior in asciidoctor by explicitly introducing inline literal context using '+'. https://github.com/asciidoctor/asciidoctor/issues/718 has more details. Change-Id: Iab31e33f92929a1ce824919b1cdfc93aeb0581a9 --- Documentation/access-control.txt | 32 ++++++++++---------- Documentation/config-gerrit.txt | 4 +-- Documentation/config-gitweb.txt | 2 +- Documentation/config-project-config.txt | 2 +- Documentation/dev-plugins.txt | 4 +-- Documentation/error-prohibited-by-gerrit.txt | 20 ++++++------ Documentation/intro-project-owner.txt | 8 ++--- Documentation/prolog-cookbook.txt | 12 ++++---- Documentation/user-changeid.txt | 4 +-- Documentation/user-dashboards.txt | 2 +- Documentation/user-upload.txt | 8 ++--- 11 files changed, 49 insertions(+), 49 deletions(-) diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index cd9bb4d614..b082339112 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -210,8 +210,8 @@ Reference-level access control is also possible. Permissions can be set on a single reference name to match one branch (e.g. `refs/heads/master`), or on a reference namespace -(e.g. `refs/heads/*`) to match any branch starting with that -prefix. So a permission with `refs/heads/*` will match +(e.g. `+refs/heads/*+`) to match any branch starting with that +prefix. So a permission with `+refs/heads/*+` will match `refs/heads/master` and `refs/heads/experimental`, etc. Reference names can also be described with a regular expression @@ -227,7 +227,7 @@ particular regular expression flavor. References can have the current user name automatically included, creating dynamic access controls that change to match the currently logged in user. For example to provide a personal sandbox space -to all developers, `refs/heads/sandbox/${username}/*` allowing +to all developers, `+refs/heads/sandbox/${username}/*+` allowing the user 'joe' to use 'refs/heads/sandbox/joe/foo'. When evaluating a reference-level access right, Gerrit will use @@ -405,19 +405,19 @@ link:user-upload.html#push_create[Upload changes] page. ==== refs/publish/* -`refs/publish/*` is an alternative name to `refs/for/*` when pushing new changes +`+refs/publish/*+` is an alternative name to `+refs/for/*+` when pushing new changes and patch sets. ==== refs/drafts/* -Push to `refs/drafts/*` creates a change like push to `refs/for/*`, except the +Push to `+refs/drafts/*+` creates a change like push to `+refs/for/*+`, except the resulting change remains hidden from public review. You then have the option of adding individual reviewers before making the change public to all. The change page will have a 'Publish' button which allows you to convert individual draft patch sets of a change into public patch sets for review. -To block push permission to `refs/drafts/*` the following permission rule can +To block push permission to `+refs/drafts/*+` the following permission rule can be configured: ==== @@ -464,18 +464,18 @@ branch permissions, allowing the holder of both to create new branches as well as bypass review for new commits on that branch. To push lightweight (non-annotated) tags, grant -`Create Reference` for reference name `refs/tags/*`, as lightweight +`Create Reference` for reference name `+refs/tags/*+`, as lightweight tags are implemented just like branches in Git. For example, to grant the possibility to create new branches under the namespace `foo`, you have to grant this permission on -`refs/heads/foo/*` for the group that should have it. +`+refs/heads/foo/*+` for the group that should have it. Finally, if you plan to grant each user a personal namespace in where they are free to create as many branches as they wish, you should grant the create reference permission so it's possible to create new branches. This is done by using the special `${username}` keyword in the reference pattern, e.g. -`refs/heads/sandbox/${username}/*`. If you do, it's also recommended +`+refs/heads/sandbox/${username}/*+`. If you do, it's also recommended you grant the users the push force permission to be able to clean up stale branches. @@ -547,7 +547,7 @@ they are a member of, just like for any other user. Ownership over a particular branch subspace may be delegated by entering a branch pattern. To delegate control over all branches that begin with `qa/` to the QA group, add `Owner` category -for reference `refs/heads/qa/*`. Members of the QA group can +for reference `+refs/heads/qa/*+`. Members of the QA group can further refine access, but only for references that begin with `refs/heads/qa/`. See <> to find out more about this role. @@ -600,15 +600,15 @@ a new commit on their local system, so in practice they must also have the `Read` access granted to upload a change. For an open source, public Gerrit installation, it is common to -grant `Read` and `Push` for `refs/for/refs/heads/*` +grant `Read` and `Push` for `+refs/for/refs/heads/*+` to `Registered Users` in the `All-Projects` ACL. For more private installations, its common to simply grant `Read` and -`Push` for `refs/for/refs/heads/*` to all users of a project. +`Push` for `+refs/for/refs/heads/*+` to all users of a project. * Force option + The force option has no function when granted to a branch in the -`refs/for/refs/heads/*` namespace. +`+refs/for/refs/heads/*+` namespace. [[category_push_merge]] @@ -661,11 +661,11 @@ must be also granted in addition to `Push Annotated Tag`. To push lightweight (non annotated) tags, grant <> for reference name -`refs/tags/*`, as lightweight tags are implemented just like +`+refs/tags/*+`, as lightweight tags are implemented just like branches in Git. To delete or overwrite an existing tag, grant `Push` with the force -option enabled for reference name `refs/tags/*`, as deleting a tag +option enabled for reference name `+refs/tags/*+`, as deleting a tag requires the same permission as deleting a branch. @@ -1026,7 +1026,7 @@ Suggested access rights to grant: == Enforcing site wide access policies -By granting the <> access right on the `refs/*` to a +By granting the <> access right on the `+refs/*+` to a group, Gerrit administrators can delegate the responsibility of maintaining access rights for that project to that group. diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 2c8b5dd81a..a4f661abdd 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2576,8 +2576,8 @@ are specified in the link:#container[container section]: If set to true, files with the MIME type `` will be sent as direct downloads to the user's browser, rather than being wrapped up inside of zipped archives. The type name may be a complete type -name, e.g. `image/gif`, a generic media type, e.g. `image/*`, -or the wildcard `*/*` to match all types. +name, e.g. `image/gif`, a generic media type, e.g. `+image/*+`, +or the wildcard `+*/*+` to match all types. + By default, false for all MIME types. diff --git a/Documentation/config-gitweb.txt b/Documentation/config-gitweb.txt index d1cee572a5..23111844aa 100644 --- a/Documentation/config-gitweb.txt +++ b/Documentation/config-gitweb.txt @@ -215,7 +215,7 @@ gitweb CGI. The CGI's `$projectroot` should be the same directory as gerrit.basePath, or a fairly current replica. If a replica is -being used, ensure it uses a full mirror, so the `refs/changes/*` +being used, ensure it uses a full mirror, so the `+refs/changes/*+` namespace is available. ---- diff --git a/Documentation/config-project-config.txt b/Documentation/config-project-config.txt index 086687511f..276117bc37 100644 --- a/Documentation/config-project-config.txt +++ b/Documentation/config-project-config.txt @@ -64,7 +64,7 @@ As you can see, there are several sections. The link:#project-section[+project+ section] appears once per project. The link:#access-section[+access+ section] appears once per reference pattern, -such as `refs/*` or `refs/heads/*`. Only one access section per pattern is +such as `+refs/*+` or `+refs/heads/*+`. Only one access section per pattern is allowed. You will find examples of keys and values in each category section <>. diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 8bfac13b37..f9b66cb3d4 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -1692,7 +1692,7 @@ class HelloServlet extends HttpServlet { ---- The auto registration only works for standard servlet mappings like -`/foo` or `/foo/*`. Regex style bindings must use a Guice ServletModule +`/foo` or `+/foo/*+`. Regex style bindings must use a Guice ServletModule to register the HTTP servlets and declare it explicitly in the manifest with the `Gerrit-HttpModule` attribute: @@ -1819,7 +1819,7 @@ BranchWebLinks will appear in the branch list in the last column. == Documentation If a plugin does not register a filter or servlet to handle URLs -`/Documentation/*` or `/static/*`, the core Gerrit server will +`+/Documentation/*+` or `+/static/*+`, the core Gerrit server will automatically export these resources over HTTP from the plugin JAR. Static resources under the `static/` directory in the JAR will be diff --git a/Documentation/error-prohibited-by-gerrit.txt b/Documentation/error-prohibited-by-gerrit.txt index 4c7bf22a29..3d9bbad2b6 100644 --- a/Documentation/error-prohibited-by-gerrit.txt +++ b/Documentation/error-prohibited-by-gerrit.txt @@ -9,32 +9,32 @@ In particular this error occurs: 1. if you push a commit for code review to a branch for which you don't have upload permissions (access right link:access-control.html#category_push_review['Push'] on - `refs/for/refs/heads/*`) + `+refs/for/refs/heads/*+`) 2. if you bypass code review without link:access-control.html#category_push_direct['Push'] access right - on `refs/heads/*` + on `+refs/heads/*+` 3. if you bypass code review pushing to a non-existing branch without link:access-control.html#category_create['Create Reference'] access - right on `refs/heads/*` + right on `+refs/heads/*+` 4. if you push an annotated tag without link:access-control.html#category_push_annotated['Push Annotated Tag'] - access right on 'refs/tags/*' + access right on `+refs/tags/*+` 5. if you push a signed tag without link:access-control.html#category_push_signed['Push Signed Tag'] - access right on 'refs/tags/*' + access right on `+refs/tags/*+` 6. if you push a lightweight tag without the access right link:access-control.html#category_create['Create - Reference'] for the reference name 'refs/tags/*' + Reference'] for the reference name `+refs/tags/*+` 7. if you push a tag with somebody else as tagger and you don't have the link:access-control.html#category_forge_committer['Forge Committer'] - access right for the reference name 'refs/tags/*' + access right for the reference name `+refs/tags/*+` 8. if you push to a project that is in state 'Read Only' For new users it often happens that they accidentally try to bypass code review. The push then fails with the error message 'prohibited by Gerrit' because the project didn't allow to bypass code review. -Bypassing the code review is done by pushing directly to refs/heads/* -(e.g. refs/heads/master) instead of pushing to refs/for/* (e.g. -refs/for/master). Details about how to push commits for code review +Bypassing the code review is done by pushing directly to `+refs/heads/*+` +(e.g. `refs/heads/master`) instead of pushing to `+refs/for/*+` (e.g. +`refs/for/master`). Details about how to push commits for code review are explained link:user-upload.html#push_create[here]. diff --git a/Documentation/intro-project-owner.txt b/Documentation/intro-project-owner.txt index c55a43cb6f..461aa91a10 100644 --- a/Documentation/intro-project-owner.txt +++ b/Documentation/intro-project-owner.txt @@ -10,7 +10,7 @@ workflows for a project. Being project owner means that you own a project in Gerrit. Technically this is expressed by having the link:access-control.html#category_owner[Owner] access right on -`refs/*` on that project. As project owner you have the permission to +`+refs/*+` on that project. As project owner you have the permission to edit the access control list and the project settings of the project. It also means that you should get familiar with these settings so that you can adapt them to the needs of your project. @@ -127,12 +127,12 @@ Access rights can be assigned on a concrete ref, e.g. `refs/heads/master` but also on ref patterns and regular expressions for ref names. -A ref pattern ends with `/*` and describes a complete ref name -namespace, e.g. access rights assigned on `refs/heads/*` apply to all +A ref pattern ends with `+/*+` and describes a complete ref name +namespace, e.g. access rights assigned on `+refs/heads/*+` apply to all branches. Regular expressions must start with `^`, e.g. access rights assigned -on `^refs/heads/rel-.*` would apply to all `rel-*` branches. +on `+^refs/heads/rel-.*+` would apply to all `+rel-*+` branches. [[groups]] === Groups diff --git a/Documentation/prolog-cookbook.txt b/Documentation/prolog-cookbook.txt index 168c04245e..43caa8384d 100644 --- a/Documentation/prolog-cookbook.txt +++ b/Documentation/prolog-cookbook.txt @@ -987,11 +987,11 @@ submit_type(cherry_pick). ---- [[SubmitTypePerBranch]] -=== Example 2: `Fast Forward Only` for all `refs/heads/stable*` branches -For all `refs/heads/stable*` branches we would like to enforce the `Fast +=== Example 2: `Fast Forward Only` for all `+refs/heads/stable*+` branches +For all `+refs/heads/stable*+` branches we would like to enforce the `Fast Forward Only` submit type. A reason for this decision may be a need to never break the build in the stable branches. For all other branches, those not -matching the `refs/heads/stable*` pattern, we would like to use the project's +matching the `+refs/heads/stable*+` pattern, we would like to use the project's default submit type as defined on the project settings page. `rules.pl` @@ -1004,13 +1004,13 @@ submit_type(T) :- gerrit:project_default_submit_type(T) ---- The first `submit_type` predicate defines the `Fast Forward Only` submit type -for `refs/heads/stable.*` branches. The second `submit_type` predicate returns +for `+refs/heads/stable.*+` branches. The second `submit_type` predicate returns the project's default submit type. === Example 3: Don't require `Fast Forward Only` if only documentation was changed Like in the previous example we want the `Fast Forward Only` submit type for the -`refs/heads/stable*` branches. However, if only documentation was changed -(only `*.txt` files), then we allow project's default submit type for such +`+refs/heads/stable*+` branches. However, if only documentation was changed +(only `+*.txt+` files), then we allow project's default submit type for such changes. `rules.pl` diff --git a/Documentation/user-changeid.txt b/Documentation/user-changeid.txt index b0e365f95f..44ca6e0770 100644 --- a/Documentation/user-changeid.txt +++ b/Documentation/user-changeid.txt @@ -70,8 +70,8 @@ For more details, see link:cmd-hook-commit-msg.html[commit-msg]. Change Upload -------------- -During upload by pushing to `refs/for/*`, `refs/drafts/*` or -`refs/heads/*`, Gerrit will try to find an existing review the +During upload by pushing to `+refs/for/*+`, `+refs/drafts/*+` or +`+refs/heads/*+`, Gerrit will try to find an existing review the uploaded commit relates to. For an existing review to match, the following properties have to match: diff --git a/Documentation/user-dashboards.txt b/Documentation/user-dashboards.txt index 52916b9096..f6db6cf1e0 100644 --- a/Documentation/user-dashboards.txt +++ b/Documentation/user-dashboards.txt @@ -58,7 +58,7 @@ to changes for the current user: == Project Dashboards It is possible to share custom dashboards at a project level. To do -this define the dashboards in a `refs/meta/dashboards/*` branch of the +this define the dashboards in a `+refs/meta/dashboards/*+` branch of the project. For each dashboard create a config file. The file path/name will be used as name (equivalent to a title in a custom dashboard) for the dashboard. diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index 46aa535a53..de9a2fbf33 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -319,9 +319,9 @@ be rewritten. Gerrit restricts direct pushes that bypass review to: -* `refs/heads/*`: any branch can be updated, created, deleted, +* `+refs/heads/*+`: any branch can be updated, created, deleted, or rewritten by the pusher. -* `refs/tags/*`: annotated tag objects pointing to any other type +* `+refs/tags/*+`: annotated tag objects pointing to any other type of Git object can be created. To push branches, the proper access rights must be configured first. @@ -445,8 +445,8 @@ As Gerrit implements the entire SSH and Git server stack within its own process space, Gerrit maintains complete control over how the repository is updated, and what responses are sent to the `git push` client invoked by the end-user, or by `repo upload`. This allows -Gerrit to provide magical refs, such as `refs/for/*` for new -change submission and `refs/changes/*` for change replacement. +Gerrit to provide magical refs, such as `+refs/for/*+` for new +change submission and `+refs/changes/*+` for change replacement. When a push request is received to create a ref in one of these namespaces Gerrit performs its own logic to update the database, and then lies to the client about the result of the operation. From 92c70260855a77b2e44443920966658b15f98ffa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Fri, 27 Mar 2015 14:54:05 -0400 Subject: [PATCH 30/36] Print proper name for ListenableFutureTask in show-queue command Any Gerrit WorkQueue.Executor that is decorated by a Guava MoreExecutors.listeningDecorator was not printing the proper task name in show-queue command. Even if the task defined a toString method, the toString method of the ListenableFutureTask was called instead. Since the task that we need to call the toString is wrapped into a FutureTask which is also wrapped into a ListenableFutureTask, there is no clean way to call proper toString other than modifying both classes to delegate the toString to task they are wrapping. Modify WorkQueue.Task.toString method to call the proper toString method by reflection when the task is a ListenableFutureTask. Change-Id: I551a89e88c4961b7412ff732bf47e2e3e9f3352f --- .../google/gerrit/server/git/WorkQueue.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java index 8c11aec0ee..161fa9e72a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.git; import com.google.common.collect.Lists; +import com.google.common.util.concurrent.ListenableFutureTask; import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.reviewdb.client.Project.NameKey; @@ -28,6 +29,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.lang.Thread.UncaughtExceptionHandler; +import java.lang.reflect.Field; import java.util.ArrayList; import java.util.Collection; import java.util.Date; @@ -378,6 +380,26 @@ public class WorkQueue { @Override public String toString() { + //This is a workaround to be able to print a proper name when the task + //is wrapped into a ListenableFutureTask. + if (runnable instanceof ListenableFutureTask) { + String errorMessage; + try { + for (Field field : ListenableFutureTask.class.getSuperclass() + .getDeclaredFields()) { + if (field.getType().isAssignableFrom(Callable.class)) { + field.setAccessible(true); + return ((Callable) field.get(runnable)).toString(); + } + } + errorMessage = "Cannot find wrapped Callable field"; + } catch (SecurityException | IllegalArgumentException + | IllegalAccessException e) { + errorMessage = "Cannot call toString on Callable field"; + } + log.debug("Cannot get a proper name for ListenableFutureTask: {}", + errorMessage); + } return runnable.toString(); } } From 5feff068cf06eb2dbb49919b07197ca089f1946a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 30 Mar 2015 10:21:38 -0400 Subject: [PATCH 31/36] Print proper name for mergeability check tasks in show-queue command Change-Id: I51e41c5eb52edddd3a7abc72e00cde248b68254d --- .../google/gerrit/server/change/MergeabilityChecker.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java index cc20608b89..0a6db2be2b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java @@ -302,6 +302,12 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener { this.force = force; } + @Override + public String toString() { + return "mergeability-check-change-" + change.getId().get() + "-project-" + + change.getDest().getParentKey(); + } + @Override public Boolean call() throws Exception { mergeabilityCheckQueue.updatingMergeabilityFlag(change, force); From 2ec9cf967754f99a314646d9398fe888b54ea9a0 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 13 Nov 2014 10:59:53 +0900 Subject: [PATCH 32/36] Don't use deprecated PGPPublicKeyRingCollection constructor The PGPPublicKeyRingCollection constructor was deprecated in Bouncycastle v1.51. Use BcPGPPublicKeyRingCollection instead. Change-Id: If718a6eab13ff991fae3b8334c53f2bc6227f061 --- .../com/google/gerrit/server/contact/EncryptedContactStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java index f43e976e81..d82180deaf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java @@ -114,7 +114,7 @@ class EncryptedContactStore implements ContactStore { private static PGPPublicKeyRingCollection readPubRing(final File pub) { try (InputStream fin = new FileInputStream(pub); InputStream in = PGPUtil.getDecoderStream(fin)) { - return new PGPPublicKeyRingCollection(in); + return new BcPGPPublicKeyRingCollection(in); } catch (IOException e) { throw new ProvisionException("Cannot read " + pub, e); } catch (PGPException e) { From 97978135e0d6cb0eb084e0af24abedd7f40f29ee Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 6 Apr 2015 22:38:33 +0900 Subject: [PATCH 33/36] Remove unused imports Change-Id: I079fc5767954d9ac795de459a9174d45825fe5e5 --- .../java/com/google/gerrit/client/change/CherryPickAction.java | 1 - .../main/java/com/google/gerrit/server/auth/ldap/Helper.java | 2 -- .../com/google/gerrit/server/index/ChangeBatchIndexer.java | 1 - .../main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java | 3 --- 4 files changed, 7 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/CherryPickAction.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/CherryPickAction.java index 6b442990dd..9f64fa05c7 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/CherryPickAction.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/CherryPickAction.java @@ -25,7 +25,6 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gwt.event.logical.shared.CloseEvent; import com.google.gwt.user.client.ui.Button; -import com.google.gwt.user.client.ui.FocusWidget; import com.google.gwt.user.client.ui.PopupPanel; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java index f8ecf257b0..618aa7d7f1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java @@ -17,7 +17,6 @@ package com.google.gerrit.server.auth.ldap; import com.google.common.base.Throwables; import com.google.common.cache.Cache; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Maps; import com.google.gerrit.common.data.ParameterizedString; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.account.AccountException; @@ -38,7 +37,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.Map; import java.util.Properties; import java.util.Set; import java.util.concurrent.TimeUnit; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java index be87ea0b68..fac1c55dfa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeBatchIndexer.java @@ -47,7 +47,6 @@ import org.eclipse.jgit.diff.DiffFormatter; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; -import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.ProgressMonitor; diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java index 9c05262fec..cc7b637321 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java @@ -14,9 +14,6 @@ package com.google.gerrit.sshd; -import static java.nio.charset.StandardCharsets.UTF_8; - -import com.google.gerrit.common.FileUtil; import com.google.common.base.Preconditions; import com.google.gerrit.reviewdb.client.AccountSshKey; import com.google.gerrit.server.IdentifiedUser; From 9edec9279a6cb820e5753892c9722fbacf56c1c7 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 30 Oct 2014 10:05:03 +0900 Subject: [PATCH 34/36] Fix unused exception throws in EncryptedContactStore Change-Id: I7e3f13699a55e457c498010b9806b60dbf163ad4 --- .../server/contact/EncryptedContactStore.java | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java index d82180deaf..f1f374c935 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/contact/EncryptedContactStore.java @@ -52,7 +52,6 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.URL; import java.security.NoSuchAlgorithmException; -import java.security.NoSuchProviderException; import java.security.SecureRandom; import java.sql.Timestamp; import java.text.SimpleDateFormat; @@ -97,11 +96,7 @@ class EncryptedContactStore implements ContactStore { // try { encrypt("test", new Date(0), "test".getBytes("UTF-8")); - } catch (NoSuchProviderException e) { - throw new ProvisionException("PGP encryption not available", e); - } catch (PGPException e) { - throw new ProvisionException("PGP encryption not available", e); - } catch (IOException e) { + } catch (PGPException | IOException e) { throw new ProvisionException("PGP encryption not available", e); } } @@ -158,20 +153,13 @@ class EncryptedContactStore implements ContactStore { u.put("account_id", String.valueOf(account.getId().get())); u.put("data", encStr); connFactory.open(storeUrl).store(u.toString().getBytes("UTF-8")); - } catch (IOException e) { - log.error("Cannot store encrypted contact information", e); - throw new ContactInformationStoreException(e); - } catch (PGPException e) { - log.error("Cannot store encrypted contact information", e); - throw new ContactInformationStoreException(e); - } catch (NoSuchProviderException e) { + } catch (IOException | PGPException e) { log.error("Cannot store encrypted contact information", e); throw new ContactInformationStoreException(e); } } - private final PGPEncryptedDataGenerator cpk() - throws NoSuchProviderException, PGPException { + private final PGPEncryptedDataGenerator cpk() { final BcPGPDataEncryptorBuilder builder = new BcPGPDataEncryptorBuilder(PGPEncryptedData.CAST5) .setSecureRandom(prng); @@ -184,7 +172,7 @@ class EncryptedContactStore implements ContactStore { } private byte[] encrypt(final String name, final Date date, - final byte[] rawText) throws NoSuchProviderException, PGPException, + final byte[] rawText) throws PGPException, IOException { final byte[] zText = compress(name, date, rawText); From 050d698403c7f5051eeda8b04c5ba0ff5821e540 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 6 Apr 2015 22:52:30 +0900 Subject: [PATCH 35/36] PatchListLoader: Don't use deprecated TemporaryBuffer.LocalFile constructor Change-Id: Iec0cae61e7c47d65278b9fc95328dfda90854e3a --- .../java/com/google/gerrit/server/patch/PatchListLoader.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index d9e796958c..94050d7317 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -382,7 +382,8 @@ public class PatchListLoader extends CacheLoader { Map resolved = new HashMap<>(); for (Map.Entry> entry : r.entrySet()) { MergeResult p = entry.getValue(); - TemporaryBuffer buf = new TemporaryBuffer.LocalFile(10 * 1024 * 1024); + TemporaryBuffer buf = + new TemporaryBuffer.LocalFile(null, 10 * 1024 * 1024); try { fmt.formatMerge(buf, p, "BASE", oursName, theirsName, "UTF-8"); buf.close(); From ce3cf6c83a3664a1ba73002006e64c1a65ca4309 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 7 Apr 2015 09:57:20 +0900 Subject: [PATCH 36/36] SshDaemon: Don't use deprecated IoAcceptor.dispose() Change-Id: I971a5326c19b0274bfe28f84acff7dc76a664f3f --- .../src/main/java/com/google/gerrit/sshd/SshDaemon.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java index 5ef4765bb5..7b910b2665 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java @@ -328,8 +328,10 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { public synchronized void stop() { if (acceptor != null) { try { - acceptor.dispose(); + acceptor.close(true).await(); log.info("Stopped Gerrit SSHD"); + } catch (InterruptedException e) { + log.warn("Exception caught while closing", e); } finally { acceptor = null; }