diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 38c47372db..020141a424 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1632,6 +1632,21 @@ Default on JGit is 10 MiB on all platforms. + Common unit suffixes of 'k', 'm', or 'g' are supported. +[[core.packedGitUseStrongRefs]]core.packedGitUseStrongRefs:: ++ +Set to `true` in order to use strong references to reference packfile +pages cached in the WindowCache. Otherwise SoftReferences are used. +If this option is set to `false`, the Java garbage collector will +flush the WindowCache to free memory if the used heap comes close to +the maximum heap size. This has the advantage that it can quickly +reclaim memory which was used by the WindowCache but comes at the +price that the previously cached pack file content needs to be again +copied from the file system cache to the Gerrit process. +Setting this option to `true` prevents flushing the WindowCache +which provides more predictable performance. ++ +Default is `false`. + [[core.deltaBaseCaseLimit]]core.deltaBaseCacheLimit:: + Maximum number of bytes to reserve for caching base objects @@ -3498,7 +3513,8 @@ By default unset. [[log.jsonLogging]]log.jsonLogging:: + -If set to true, enables error logging in JSON format (file name: "logs/error_log.json"). +If set to true, enables error, ssh and http logging in JSON format (file name: +"logs/{error|sshd|httpd}_log.json"). + Defaults to false. diff --git a/Documentation/dev-bazel.txt b/Documentation/dev-bazel.txt index 5a03082923..8c2b0f91a2 100644 --- a/Documentation/dev-bazel.txt +++ b/Documentation/dev-bazel.txt @@ -281,7 +281,7 @@ The WAR file will be placed in: Debugging tests: ---- - bazel test --test_output=streamed --test_filter=com.gerrit.TestClass.testMethod testTarget + bazel test --test_output=streamed --test_filter=com.gerrit.TestClass.testMethod testTarget ---- Debug test example: @@ -364,21 +364,25 @@ If Docker is not available, the Elasticsearch tests will be skipped. Note that Bazel currently does not show link:https://github.com/bazelbuild/bazel/issues/3476[the skipped tests]. -[[debug]] -=== Index Query Tests +[[logging]] +=== Controlling logging level -The `DEBUG` log level can optionally be enabled for the index query tests. That log level applies to -both Elasticsearch and Lucene tests. +Per default, logging level is set to `INFO` level for all tests. The `DEBUG` +log level can be enabled for the tests. -In Eclipse, set `-Ddebug=true` as a VM argument under the Run Configuration's `Arguments` tab. - -With `bazel`, here is an example for the Lucene `account` test: +In IDE, set `-Dgerrit.logLevel=debug` as a VM argument. With `bazel`, pass +`GERRIT_LOG_LEVEL=debug` environment variable: ---- -bazel test --jvmopt='-Ddebug=true' \ -javatests/com/google/gerrit/server/query/account:lucene_query_test + bazel test --test_filter=com.gerrit.server.notedb.ChangeNotesTest \ + --test_env=GERRIT_LOG_LEVEL=debug \ + javatests/com/google/gerrit/server:server_tests ---- +The log results can be found in: +`bazel-testlogs/javatests/com/google/gerrit/server/server_tests/test.log`. + + == Dependencies Dependency JARs are normally downloaded as needed, but you can @@ -580,7 +584,6 @@ To use the binary from the Bazel build, you need to use the `run_npm_binary.py` wrapper script. For an example, see the use of `crisper` in `tools/bzl/js.bzl`. - [[RBE]] == Google Remote Build Support @@ -615,8 +618,6 @@ bazel test --config=remote \ ``` - - GERRIT ------ Part of link:index.html[Gerrit Code Review] diff --git a/Documentation/intro-project-owner.txt b/Documentation/intro-project-owner.txt index 1f98291382..f7aed5ed7b 100644 --- a/Documentation/intro-project-owner.txt +++ b/Documentation/intro-project-owner.txt @@ -586,13 +586,6 @@ The `download-commands` plugin provides the default download commands Gerrit administrators may configure which of the commands are shown on the change screen. -- link:https://gerrit-review.googlesource.com/admin/repos/plugins/egit[ - egit] plugin: -+ -The `egit` plugin provides the change ref as a download command, which is -needed for downloading a change from within -link:https://www.eclipse.org/egit/[EGit]. - - link:https://gerrit-review.googlesource.com/admin/repos/plugins/project-download-commands[ project-download-commands] plugin: + @@ -759,7 +752,16 @@ link:project-configuration.html#project-state[project state] to `ReadOnly` or Gerrit core does not support the renaming of projects. -As workaround you can perform the following steps: +If the link:https://gerrit-review.googlesource.com/admin/repos/plugins/rename-project[rename-project] +plugin is installed, projects can be renamed using the +link:https://gerrit.googlesource.com/plugins/rename-project/+/refs/heads/master/src/main/resources/Documentation/cmd-rename.md[rename-project] +ssh command. + +Find details about prerequisites in the +link:https://gerrit.googlesource.com/plugins/rename-project/+/refs/heads/master/src/main/resources/Documentation/about.md[plugin documentation]. + +If you don't want to use the rename-project plugin you can perform the following steps as +a workaround: . link:#project-creation[Create a new project] with the new name. . link:#import-history[Import the history of the old project]. @@ -768,11 +770,6 @@ As workaround you can perform the following steps: Please note that a drawback of this workaround is that the whole review history (changes, review comments) is lost. -Alternatively, you can use the -link:https://gerrit.googlesource.com/plugins/importer/[importer] plugin -to copy the project _including the review history_, and then -link:#project-deletion[delete the old project]. - GERRIT ------ Part of link:index.html[Gerrit Code Review] diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index d10e574a71..a360220e18 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -67,6 +67,15 @@ objects needing finalization. ==== Jetty +* `http/server/jetty/connections/connections`: The current number of open connections +* `http/server/jetty/connections/connections_total`: The total number of connections opened +* `http/server/jetty/connections/connections_duration_max`: The max duration of a connection in ms +* `http/server/jetty/connections/connections_duration_mean`: The mean duration of a connection in ms +* `http/server/jetty/connections/connections_duration_stdev`: The standard deviation of the duration of a connection in ms +* `http/server/jetty/connections/received_messages`: The total number of messages received +* `http/server/jetty/connections/sent_messages`: The total number of messages sent +* `http/server/jetty/connections/received_bytes`: Total number of bytes received by tracked connections +* `http/server/jetty/connections/sent_bytes`: Total number of bytes sent by tracked connections" * `http/server/jetty/threadpool/active_threads`: Active threads * `http/server/jetty/threadpool/idle_threads`: Idle threads * `http/server/jetty/threadpool/reserved_threads`: Reserved threads @@ -137,6 +146,18 @@ topic submissions that concluded successfully. * `jgit/block_cache/cache_used`: Bytes of memory retained in JGit block cache. * `jgit/block_cache/open_files`: File handles held open by JGit block cache. +* `avg_load_time` Average time to load a cache entry for JGit block cache. +* `eviction_count` : Cache evictions for JGit block cache. +* `eviction_ratio` : Cache eviction ratio for JGit block cache. +* `hit_count` : Cache hits for JGit block cache. +* `hit_ratio` : Cache hit ratio for JGit block cache. +* `load_failure_count` : Failed cache loads for JGit block cache. +* `load_failure_ratio` : Failed cache load ratio for JGit block cache. +* `load_success_count` : Successful cache loads for JGit block cache. +* `miss_count` : Cache misses for JGit block cache. +* `miss_ratio` : Cache miss ratio for JGit block cache. +* `cache_used_per_repository` : Bytes of memory retained per repository for the top N repositories +having most data in the cache. The number N of reported repositories is limited to 1000. === Git diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java index 088de23962..a372089444 100644 --- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java +++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java @@ -319,6 +319,9 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { } protected class StagedUsers { + public static final String REVIEWER_BY_EMAIL = "reviewerByEmail@example.com"; + public static final String CC_BY_EMAIL = "ccByEmail@example.com"; + public final TestAccount owner; public final TestAccount author; public final TestAccount uploader; @@ -327,8 +330,6 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { public final TestAccount starrer; public final TestAccount assignee; public final TestAccount watchingProjectOwner; - public final String reviewerByEmail = "reviewerByEmail@example.com"; - public final String ccerByEmail = "ccByEmail@example.com"; private final Map watchers = new HashMap<>(); private final Map accountsByEmail = new HashMap<>(); @@ -429,9 +430,9 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest { ReviewInput in = ReviewInput.noScore() .reviewer(reviewer.email()) - .reviewer(reviewerByEmail) + .reviewer(REVIEWER_BY_EMAIL) .reviewer(ccer.email(), ReviewerState.CC, false) - .reviewer(ccerByEmail, ReviewerState.CC, false); + .reviewer(CC_BY_EMAIL, ReviewerState.CC, false); ReviewResult result = gApi.changes().id(r.getChangeId()).current().review(in); supportReviewersByEmail = true; if (result.reviewers.values().stream().anyMatch(v -> v.error != null)) { diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD index e719f8377f..646d8f0765 100644 --- a/java/com/google/gerrit/acceptance/BUILD +++ b/java/com/google/gerrit/acceptance/BUILD @@ -49,8 +49,6 @@ DEPLOY_ENV = [ "//lib/guice:guice-servlet", "//lib/mail", "//lib/mina:sshd", - "//lib/log:impl-log4j", - "//lib/log:log4j", "//lib:guava", "//lib/bouncycastle:bcpg", "//lib/bouncycastle:bcprov", diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index 678bc315c1..a234e00ec1 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java @@ -18,13 +18,11 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static java.lang.annotation.RetentionPolicy.RUNTIME; import static java.util.Objects.requireNonNull; -import static org.apache.log4j.Logger.getLogger; import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.gerrit.acceptance.testsuite.account.AccountOperations; import com.google.gerrit.acceptance.testsuite.account.AccountOperationsImpl; import com.google.gerrit.acceptance.testsuite.group.GroupOperations; @@ -50,6 +48,7 @@ import com.google.gerrit.server.util.SystemLog; import com.google.gerrit.testing.FakeEmailSender; import com.google.gerrit.testing.InMemoryRepositoryManager; import com.google.gerrit.testing.SshMode; +import com.google.gerrit.testing.TestLoggingActivator; import com.google.inject.AbstractModule; import com.google.inject.BindingAnnotation; import com.google.inject.Injector; @@ -75,11 +74,6 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.stream.Stream; -import org.apache.log4j.ConsoleAppender; -import org.apache.log4j.Level; -import org.apache.log4j.LogManager; -import org.apache.log4j.Logger; -import org.apache.log4j.PatternLayout; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.RepositoryCache; import org.eclipse.jgit.util.FS; @@ -117,8 +111,7 @@ public class GerritServer implements AutoCloseable { null, // @GerritConfig is only valid on methods. null, // @GerritConfigs is only valid on methods. null, // @GlobalPluginConfig is only valid on methods. - null, // @GlobalPluginConfigs is only valid on methods. - getLogLevelThresholdAnnotation(testDesc)); + null); // @GlobalPluginConfigs is only valid on methods. } public static Description forTestMethod( @@ -154,8 +147,7 @@ public class GerritServer implements AutoCloseable { testDesc.getAnnotation(GerritConfig.class), testDesc.getAnnotation(GerritConfigs.class), testDesc.getAnnotation(GlobalPluginConfig.class), - testDesc.getAnnotation(GlobalPluginConfigs.class), - getLogLevelThresholdAnnotation(testDesc)); + testDesc.getAnnotation(GlobalPluginConfigs.class)); } private static boolean has(Class annotation, Class clazz) { @@ -177,14 +169,6 @@ public class GerritServer implements AutoCloseable { return null; } - private static Level getLogLevelThresholdAnnotation(org.junit.runner.Description testDesc) { - LogThreshold logLevelThreshold = testDesc.getTestClass().getAnnotation(LogThreshold.class); - if (logLevelThreshold == null) { - return Level.DEBUG; - } - return Level.toLevel(logLevelThreshold.level()); - } - abstract org.junit.runner.Description testDescription(); @Nullable @@ -224,8 +208,6 @@ public class GerritServer implements AutoCloseable { @Nullable abstract GlobalPluginConfigs pluginConfigs(); - abstract Level logLevelThreshold(); - private void checkValidAnnotations() { if (useClockStep() != null && useSystemTime()) { throw new IllegalStateException("Use either @UseClockStep or @UseSystemTime, not both"); @@ -262,46 +244,6 @@ public class GerritServer implements AutoCloseable { } } - private static final ImmutableMap LOG_LEVELS = - ImmutableMap.builder() - .put("com.google.gerrit", Level.DEBUG) - - // Silence non-critical messages from MINA SSHD. - .put("org.apache.mina", Level.WARN) - .put("org.apache.sshd.common", Level.WARN) - .put("org.apache.sshd.server", Level.WARN) - .put("org.apache.sshd.common.keyprovider.FileKeyPairProvider", Level.INFO) - .put("com.google.gerrit.sshd.GerritServerSession", Level.WARN) - - // Silence non-critical messages from mime-util. - .put("eu.medsea.mimeutil", Level.WARN) - - // Silence non-critical messages from openid4java. - .put("org.apache.xml", Level.WARN) - .put("org.openid4java", Level.WARN) - .put("org.openid4java.consumer.ConsumerManager", Level.FATAL) - .put("org.openid4java.discovery.Discovery", Level.ERROR) - .put("org.openid4java.server.RealmVerifier", Level.ERROR) - .put("org.openid4java.message.AuthSuccess", Level.ERROR) - - // Silence non-critical messages from c3p0 (if used). - .put("com.mchange.v2.c3p0", Level.WARN) - .put("com.mchange.v2.resourcepool", Level.WARN) - .put("com.mchange.v2.sql", Level.WARN) - - // Silence non-critical messages from apache.http. - .put("org.apache.http", Level.WARN) - - // Silence non-critical messages from Jetty. - .put("org.eclipse.jetty", Level.WARN) - - // Silence non-critical messages from JGit. - .put("org.eclipse.jgit.transport.PacketLineIn", Level.WARN) - .put("org.eclipse.jgit.transport.PacketLineOut", Level.WARN) - .put("org.eclipse.jgit.internal.storage.file.FileSnapshot", Level.WARN) - .put("org.eclipse.jgit.util.FS", Level.WARN) - .build(); - private static boolean forceLocalDisk() { String value = Strings.nullToEmpty(System.getenv("GERRIT_FORCE_LOCAL_DISK")); if (value.isEmpty()) { @@ -417,7 +359,7 @@ public class GerritServer implements AutoCloseable { throws Exception { checkArgument(site != null, "site is required (even for in-memory server"); desc.checkValidAnnotations(); - configureLogging(desc.logLevelThreshold()); + TestLoggingActivator.configureLogging(); CyclicBarrier serverStarted = new CyclicBarrier(2); Daemon daemon = new Daemon( @@ -518,25 +460,6 @@ public class GerritServer implements AutoCloseable { return new GerritServer(desc, site, createTestInjector(daemon), daemon, daemonService); } - private static void configureLogging(Level threshold) { - LogManager.resetConfiguration(); - - PatternLayout layout = new PatternLayout(); - layout.setConversionPattern("%-5p %c %x: %m%n"); - - ConsoleAppender dst = new ConsoleAppender(); - dst.setLayout(layout); - dst.setTarget("System.err"); - dst.setThreshold(threshold); - dst.activateOptions(); - - Logger root = LogManager.getRootLogger(); - root.removeAllAppenders(); - root.addAppender(dst); - - LOG_LEVELS.entrySet().stream().forEach(e -> getLogger(e.getKey()).setLevel(e.getValue())); - } - private static void mergeTestConfig(Config cfg) { String forceEphemeralPort = String.format("%s:0", getLocalHost().getHostName()); String url = "http://" + forceEphemeralPort + "/"; diff --git a/java/com/google/gerrit/common/PageLinks.java b/java/com/google/gerrit/common/PageLinks.java index 9f06364f83..38de5b15a1 100644 --- a/java/com/google/gerrit/common/PageLinks.java +++ b/java/com/google/gerrit/common/PageLinks.java @@ -90,16 +90,16 @@ public class PageLinks { return ADMIN_PROJECTS + p.get(); } - public static String toProjectAcceess(Project.NameKey p) { - return "/admin/projects/" + p.get() + ",access"; + public static String toProjectAccess(Project.NameKey p) { + return ADMIN_PROJECTS + p.get() + ",access"; } public static String toProjectBranches(Project.NameKey p) { - return "/admin/projects/" + p.get() + ",branches"; + return ADMIN_PROJECTS + p.get() + ",branches"; } public static String toProjectTags(Project.NameKey p) { - return "/admin/projects/" + p.get() + ",tags"; + return ADMIN_PROJECTS + p.get() + ",tags"; } public static String toAccountQuery(String fullname, Status status) { diff --git a/java/com/google/gerrit/common/data/LabelType.java b/java/com/google/gerrit/common/data/LabelType.java index f9cd5626b9..90b0930386 100644 --- a/java/com/google/gerrit/common/data/LabelType.java +++ b/java/com/google/gerrit/common/data/LabelType.java @@ -128,7 +128,7 @@ public class LabelType { maxNegative = Short.MIN_VALUE; maxPositive = Short.MAX_VALUE; - if (values.size() > 0) { + if (!values.isEmpty()) { if (values.get(0).getValue() < 0) { maxNegative = values.get(0).getValue(); } diff --git a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java index 864839a20d..da5ae92cb4 100644 --- a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java +++ b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java @@ -392,7 +392,7 @@ abstract class AbstractElasticIndex implements Index { @Override public ResultSet read() { - return readImpl((doc) -> AbstractElasticIndex.this.fromDocument(doc, opts.fields())); + return readImpl(doc -> AbstractElasticIndex.this.fromDocument(doc, opts.fields())); } @Override diff --git a/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java b/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java index 061a37321a..9c44583e15 100644 --- a/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java +++ b/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java @@ -14,6 +14,7 @@ package com.google.gerrit.elasticsearch.builders; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.time.format.DateTimeFormatter.ISO_INSTANT; import com.fasterxml.jackson.core.JsonEncoding; @@ -21,7 +22,6 @@ import com.fasterxml.jackson.core.JsonFactory; import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.core.json.JsonReadFeature; import com.fasterxml.jackson.core.json.JsonWriteFeature; -import com.google.common.base.Charsets; import java.io.ByteArrayOutputStream; import java.io.Closeable; import java.io.IOException; @@ -139,7 +139,7 @@ public final class XContentBuilder implements Closeable { public String string() { close(); byte[] bytesArray = bos.toByteArray(); - return new String(bytesArray, Charsets.UTF_8); + return new String(bytesArray, UTF_8); } private void writeValue(Object value) throws IOException { diff --git a/java/com/google/gerrit/extensions/api/accounts/Accounts.java b/java/com/google/gerrit/extensions/api/accounts/Accounts.java index db7f506e9f..15fca9a46c 100644 --- a/java/com/google/gerrit/extensions/api/accounts/Accounts.java +++ b/java/com/google/gerrit/extensions/api/accounts/Accounts.java @@ -21,6 +21,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import java.util.Arrays; import java.util.EnumSet; import java.util.List; +import java.util.Set; public interface Accounts { /** @@ -138,7 +139,7 @@ public interface Accounts { private int limit; private int start; private boolean suggest; - private EnumSet options = EnumSet.noneOf(ListAccountsOption.class); + private Set options = EnumSet.noneOf(ListAccountsOption.class); /** Execute query and return a list of accounts. */ public abstract List get() throws RestApiException; @@ -185,7 +186,7 @@ public interface Accounts { } /** Set options on the request, replacing existing options. */ - public QueryRequest withOptions(EnumSet options) { + public QueryRequest withOptions(Set options) { this.options = options; return this; } @@ -206,7 +207,7 @@ public interface Accounts { return suggest; } - public EnumSet getOptions() { + public Set getOptions() { return options; } } diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java index 25eb7a83a3..8b1ae3c2c6 100644 --- a/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java +++ b/java/com/google/gerrit/extensions/api/changes/ChangeEditApi.java @@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.RawInput; import com.google.gerrit.extensions.restapi.RestApiException; import java.util.EnumSet; import java.util.Optional; +import java.util.Set; /** * An API for the change edit of a change. A change edit is similar to a patch set and will become @@ -51,7 +52,7 @@ public interface ChangeEditApi { return base; } - public EnumSet options() { + public Set options() { return options; } } diff --git a/java/com/google/gerrit/extensions/api/changes/Changes.java b/java/com/google/gerrit/extensions/api/changes/Changes.java index bcb49de1d6..9b9a8a4ce4 100644 --- a/java/com/google/gerrit/extensions/api/changes/Changes.java +++ b/java/com/google/gerrit/extensions/api/changes/Changes.java @@ -24,6 +24,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import java.util.Arrays; import java.util.EnumSet; import java.util.List; +import java.util.Set; public interface Changes { /** @@ -78,7 +79,7 @@ public interface Changes { private int limit; private int start; private boolean isNoLimit; - private EnumSet options = EnumSet.noneOf(ListChangesOption.class); + private Set options = EnumSet.noneOf(ListChangesOption.class); private ListMultimap pluginOptions = ArrayListMultimap.create(); public abstract List get() throws RestApiException; @@ -116,7 +117,7 @@ public interface Changes { } /** Set options on the request, replacing existing options. */ - public QueryRequest withOptions(EnumSet options) { + public QueryRequest withOptions(Set options) { this.options = options; return this; } @@ -149,7 +150,7 @@ public interface Changes { return start; } - public EnumSet getOptions() { + public Set getOptions() { return options; } diff --git a/java/com/google/gerrit/extensions/api/groups/Groups.java b/java/com/google/gerrit/extensions/api/groups/Groups.java index 86c2d772fb..81b5f47485 100644 --- a/java/com/google/gerrit/extensions/api/groups/Groups.java +++ b/java/com/google/gerrit/extensions/api/groups/Groups.java @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.EnumSet; import java.util.List; import java.util.Map; +import java.util.Set; public interface Groups { /** @@ -166,7 +167,7 @@ public interface Groups { return this; } - public EnumSet getOptions() { + public Set getOptions() { return options; } @@ -224,7 +225,7 @@ public interface Groups { private String query; private int limit; private int start; - private EnumSet options = EnumSet.noneOf(ListGroupsOption.class); + private Set options = EnumSet.noneOf(ListGroupsOption.class); /** Execute query and returns the matched groups as list. */ public abstract List get() throws RestApiException; @@ -266,7 +267,7 @@ public interface Groups { } /** Set options on the request, replacing existing options. */ - public QueryRequest withOptions(EnumSet options) { + public QueryRequest withOptions(Set options) { this.options = options; return this; } @@ -283,7 +284,7 @@ public interface Groups { return start; } - public EnumSet getOptions() { + public Set getOptions() { return options; } } diff --git a/java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java b/java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java index bd7ebcb091..4170797ea5 100644 --- a/java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java +++ b/java/com/google/gerrit/extensions/common/TestSubmitRuleInfo.java @@ -32,7 +32,7 @@ public class TestSubmitRuleInfo { public static class None { private None() {} - public static None INSTANCE = new None(); + public static final None INSTANCE = new None(); } @Override diff --git a/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java b/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java index b09dad0365..dcfb61413e 100644 --- a/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java +++ b/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java @@ -192,7 +192,7 @@ class OpenIdServiceImpl { // We might already have this account on file. Look for it. // try { - return accountManager.lookup(aReq.getIdentity()) == null; + return !accountManager.lookup(aReq.getIdentity()).isPresent(); } catch (AccountException e) { logger.atWarning().withCause(e).log("Cannot determine if user account exists"); return true; @@ -333,7 +333,7 @@ class OpenIdServiceImpl { areq.setEmailAddress(fetchRsp.getAttributeValue("Email")); } - if (openIdDomains != null && openIdDomains.size() > 0) { + if (openIdDomains != null && !openIdDomains.isEmpty()) { // Administrator limited email domains, which can be used for OpenID. // Login process will only work if the passed email matches one // of these domains. diff --git a/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java b/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java index 2507163ec5..4fabb18928 100644 --- a/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java +++ b/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java @@ -96,9 +96,9 @@ class GitwebServlet extends HttpServlet { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private static final String PROJECT_LIST_ACTION = "project_list"; + private static final int BUFFER_SIZE = 8192; private final Set deniedActions; - private final int bufferSize = 8192; private final Path gitwebCgi; private final URI gitwebUrl; private final LocalDiskRepositoryManager repoManager; @@ -504,11 +504,11 @@ class GitwebServlet extends HttpServlet { proc.getOutputStream().close(); } - try (InputStream in = new BufferedInputStream(proc.getInputStream(), bufferSize)) { + try (InputStream in = new BufferedInputStream(proc.getInputStream(), BUFFER_SIZE)) { readCgiHeaders(rsp, in); try (OutputStream out = rsp.getOutputStream()) { - final byte[] buf = new byte[bufferSize]; + final byte[] buf = new byte[BUFFER_SIZE]; int n; while ((n = in.read(buf)) > 0) { out.write(buf, 0, n); @@ -643,7 +643,7 @@ class GitwebServlet extends HttpServlet { () -> { try { try { - final byte[] buf = new byte[bufferSize]; + final byte[] buf = new byte[BUFFER_SIZE]; int remaining = contentLength; while (0 < remaining) { final int max = Math.max(buf.length, remaining); diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index f5ece53973..83a21792c4 100644 --- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -1358,7 +1358,7 @@ public class RestApiServlet extends HttpServlet { for (String p : Splitter.on('/').split(path)) { out.add(IdString.fromUrl(p)); } - if (out.size() > 0 && out.get(out.size() - 1).isEmpty()) { + if (!out.isEmpty() && out.get(out.size() - 1).isEmpty()) { out.remove(out.size() - 1); } return out; diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 16d66b64fe..e576d73f7c 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -577,7 +577,7 @@ public class LuceneChangeIndex implements ChangeIndex { private void decodeReviewedBy(ListMultimap doc, ChangeData cd) { Collection reviewedBy = doc.get(REVIEWEDBY_FIELD); - if (reviewedBy.size() > 0) { + if (!reviewedBy.isEmpty()) { Set accounts = Sets.newHashSetWithExpectedSize(reviewedBy.size()); for (IndexableField r : reviewedBy) { int id = r.numericValue().intValue(); diff --git a/java/com/google/gerrit/mail/RawMailParser.java b/java/com/google/gerrit/mail/RawMailParser.java index b7e2030b2b..9c89d199f9 100644 --- a/java/com/google/gerrit/mail/RawMailParser.java +++ b/java/com/google/gerrit/mail/RawMailParser.java @@ -69,7 +69,7 @@ public class RawMailParser { } // Add From, To and Cc - if (mimeMessage.getFrom() != null && mimeMessage.getFrom().size() > 0) { + if (mimeMessage.getFrom() != null && !mimeMessage.getFrom().isEmpty()) { Mailbox from = mimeMessage.getFrom().get(0); messageBuilder.from(new Address(from.getName(), from.getAddress())); } diff --git a/java/com/google/gerrit/metrics/dropwizard/BUILD b/java/com/google/gerrit/metrics/dropwizard/BUILD index 307980939f..4b3859f7a8 100644 --- a/java/com/google/gerrit/metrics/dropwizard/BUILD +++ b/java/com/google/gerrit/metrics/dropwizard/BUILD @@ -8,7 +8,6 @@ java_library( "//java/com/google/gerrit/common:annotations", "//java/com/google/gerrit/extensions:api", "//java/com/google/gerrit/metrics", - "//java/com/google/gerrit/pgm/http/jetty", "//java/com/google/gerrit/server", "//lib:args4j", "//lib:guava", diff --git a/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java b/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java index e33c242999..68f8021f4e 100644 --- a/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java +++ b/java/com/google/gerrit/metrics/dropwizard/BucketedCallback.java @@ -22,6 +22,7 @@ import com.google.common.collect.Maps; import com.google.gerrit.metrics.Description; import com.google.gerrit.metrics.Field; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; /** Abstract callback metric broken down into buckets. */ @@ -66,7 +67,13 @@ abstract class BucketedCallback implements BucketedMetric { } void doPrune() { - cells.entrySet().removeIf(objectValueGaugeEntry -> !objectValueGaugeEntry.getValue().set); + Set.ValueGauge>> entries = cells.entrySet(); + for (Map.Entry e : entries) { + if (!e.getValue().set) { + entries.remove(e); + registry.remove(submetric(e.getKey())); + } + } } void doEndSet() { diff --git a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java index 3819786b51..62b24972b3 100644 --- a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java +++ b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java @@ -14,12 +14,18 @@ package com.google.gerrit.metrics.proc; +import com.google.gerrit.metrics.CallbackMetric1; import com.google.gerrit.metrics.Description; import com.google.gerrit.metrics.Description.Units; +import com.google.gerrit.metrics.Field; import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.server.logging.Metadata; +import java.util.Map; import org.eclipse.jgit.storage.file.WindowCacheStats; public class JGitMetricModule extends MetricModule { + private static final long MAX_REPO_COUNT = 1000; + @Override protected void configure(MetricMaker metrics) { metrics.newCallbackMetric( @@ -28,12 +34,99 @@ public class JGitMetricModule extends MetricModule { new Description("Bytes of memory retained in JGit block cache.") .setGauge() .setUnit(Units.BYTES), - () -> WindowCacheStats.getStats().getOpenByteCount()); + WindowCacheStats.getStats()::getOpenByteCount); metrics.newCallbackMetric( "jgit/block_cache/open_files", Long.class, new Description("File handles held open by JGit block cache.").setGauge().setUnit("fds"), - () -> WindowCacheStats.getStats().getOpenFileCount()); + WindowCacheStats.getStats()::getOpenFileCount); + + metrics.newCallbackMetric( + "jgit/block_cache/avg_load_time", + Double.class, + new Description("Average time to load a cache entry for JGit block cache.") + .setGauge() + .setUnit(Units.NANOSECONDS), + WindowCacheStats.getStats()::getAverageLoadTime); + + metrics.newCallbackMetric( + "jgit/block_cache/eviction_count", + Long.class, + new Description("Cache evictions for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getEvictionCount); + + metrics.newCallbackMetric( + "jgit/block_cache/eviction_ratio", + Double.class, + new Description("Cache eviction ratio for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getEvictionRatio); + + metrics.newCallbackMetric( + "jgit/block_cache/hit_count", + Long.class, + new Description("Cache hits for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getHitCount); + + metrics.newCallbackMetric( + "jgit/block_cache/hit_ratio", + Double.class, + new Description("Cache hit ratio for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getHitRatio); + + metrics.newCallbackMetric( + "jgit/block_cache/load_failure_count", + Long.class, + new Description("Failed cache loads for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getLoadFailureCount); + + metrics.newCallbackMetric( + "jgit/block_cache/load_failure_ratio", + Double.class, + new Description("Failed cache load ratio for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getLoadFailureRatio); + + metrics.newCallbackMetric( + "jgit/block_cache/load_success_count", + Long.class, + new Description("Successfull cache loads for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getLoadSuccessCount); + + metrics.newCallbackMetric( + "jgit/block_cache/miss_count", + Long.class, + new Description("Cache misses for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getMissCount); + + metrics.newCallbackMetric( + "jgit/block_cache/miss_ratio", + Double.class, + new Description("Cache miss ratio for JGit block cache.").setGauge(), + WindowCacheStats.getStats()::getMissRatio); + + CallbackMetric1 repoEnt = + metrics.newCallbackMetric( + "jgit/block_cache/cache_used_per_repository", + Long.class, + new Description( + "Bytes of memory retained per repository for the top repositories " + + "having most data in the cache.") + .setGauge() + .setUnit("byte"), + Field.ofString("repository_name", Metadata.Builder::projectName).build()); + metrics.newTrigger( + repoEnt, + () -> { + Map cacheMap = WindowCacheStats.getStats().getOpenByteCountPerRepository(); + if (cacheMap.isEmpty()) { + repoEnt.forceCreate(""); + } else { + cacheMap.entrySet().stream() + .sorted(Map.Entry.comparingByValue().reversed()) + .limit(MAX_REPO_COUNT) + .forEach(e -> repoEnt.set(e.getKey(), e.getValue())); + repoEnt.prune(); + } + }); } } diff --git a/java/com/google/gerrit/pgm/BUILD b/java/com/google/gerrit/pgm/BUILD index aa4df82618..8b8f13c1df 100644 --- a/java/com/google/gerrit/pgm/BUILD +++ b/java/com/google/gerrit/pgm/BUILD @@ -22,6 +22,7 @@ java_library( "//java/com/google/gerrit/launcher", "//java/com/google/gerrit/lifecycle", "//java/com/google/gerrit/lucene", + "//java/com/google/gerrit/metrics", "//java/com/google/gerrit/metrics/dropwizard", "//java/com/google/gerrit/pgm/http/jetty", "//java/com/google/gerrit/pgm/init", diff --git a/java/com/google/gerrit/pgm/DeleteZombieDrafts.java b/java/com/google/gerrit/pgm/DeleteZombieDrafts.java index 90a60c101d..c08e9999db 100644 --- a/java/com/google/gerrit/pgm/DeleteZombieDrafts.java +++ b/java/com/google/gerrit/pgm/DeleteZombieDrafts.java @@ -14,6 +14,8 @@ package com.google.gerrit.pgm; +import com.google.gerrit.metrics.DisabledMetricMaker; +import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.pgm.init.api.ConsoleUI; import com.google.gerrit.pgm.util.SiteProgram; import com.google.gerrit.server.config.GerritServerConfigModule; @@ -68,6 +70,7 @@ public class DeleteZombieDrafts extends SiteProgram { bind(String.class) .annotatedWith(SecureStoreClassName.class) .toProvider(Providers.of(getConfiguredSecureStoreClass())); + bind(MetricMaker.class).to(DisabledMetricMaker.class); install(new FactoryModuleBuilder().build(DeleteZombieCommentsRefs.Factory.class)); } }); diff --git a/java/com/google/gerrit/pgm/http/jetty/BUILD b/java/com/google/gerrit/pgm/http/jetty/BUILD index 32247fbc60..6ceb24243a 100644 --- a/java/com/google/gerrit/pgm/http/jetty/BUILD +++ b/java/com/google/gerrit/pgm/http/jetty/BUILD @@ -10,9 +10,11 @@ java_library( "//java/com/google/gerrit/lifecycle", "//java/com/google/gerrit/metrics", "//java/com/google/gerrit/server", + "//java/com/google/gerrit/server/logging", "//java/com/google/gerrit/server/util/time", "//java/com/google/gerrit/sshd", "//java/com/google/gerrit/util/http", + "//lib:gson", "//lib:guava", "//lib:jgit", "//lib:servlet-api", diff --git a/java/com/google/gerrit/pgm/http/jetty/HttpLog.java b/java/com/google/gerrit/pgm/http/jetty/HttpLog.java index 809f7bc29c..4e4c93b57b 100644 --- a/java/com/google/gerrit/pgm/http/jetty/HttpLog.java +++ b/java/com/google/gerrit/pgm/http/jetty/HttpLog.java @@ -17,6 +17,7 @@ package com.google.gerrit.pgm.http.jetty; import com.google.common.base.Strings; import com.google.gerrit.httpd.GetUserFilter; import com.google.gerrit.httpd.restapi.LogRedactUtil; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.util.SystemLog; import com.google.gerrit.server.util.time.TimeUtil; import com.google.inject.Inject; @@ -28,11 +29,13 @@ import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.RequestLog; import org.eclipse.jetty.server.Response; import org.eclipse.jetty.util.component.AbstractLifeCycle; +import org.eclipse.jgit.lib.Config; /** Writes the {@code httpd_log} file with per-request data. */ class HttpLog extends AbstractLifeCycle implements RequestLog { private static final Logger log = Logger.getLogger(HttpLog.class); private static final String LOG_NAME = "httpd_log"; + private static final String JSON_SUFFIX = ".json"; interface HttpLogFactory { HttpLog get(); @@ -52,8 +55,20 @@ class HttpLog extends AbstractLifeCycle implements RequestLog { private final AsyncAppender async; @Inject - HttpLog(SystemLog systemLog) { - async = systemLog.createAsyncAppender(LOG_NAME, new HttpLogLayout()); + HttpLog(SystemLog systemLog, @GerritServerConfig Config config) { + boolean json = config.getBoolean("log", "jsonLogging", false); + boolean text = config.getBoolean("log", "textLogging", true) || !json; + + async = new AsyncAppender(); + + if (text) { + async.addAppender(systemLog.createAsyncAppender(LOG_NAME, new HttpLogLayout())); + } + + if (json) { + async.addAppender( + systemLog.createAsyncAppender(LOG_NAME + JSON_SUFFIX, new HttpLogJsonLayout())); + } } @Override diff --git a/java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java b/java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java new file mode 100644 index 0000000000..33916acc20 --- /dev/null +++ b/java/com/google/gerrit/pgm/http/jetty/HttpLogJsonLayout.java @@ -0,0 +1,72 @@ +// Copyright (C) 2020 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.pgm.http.jetty; + +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_CONTENT_LENGTH; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_HOST; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_METHOD; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_PROTOCOL; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_REFERER; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_RESOURCE; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_STATUS; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_USER; +import static com.google.gerrit.pgm.http.jetty.HttpLog.P_USER_AGENT; + +import com.google.gerrit.server.logging.JsonLayout; +import com.google.gerrit.server.logging.JsonLogEntry; +import java.time.format.DateTimeFormatter; +import org.apache.log4j.spi.LoggingEvent; + +public class HttpLogJsonLayout extends JsonLayout { + + @Override + public DateTimeFormatter createDateTimeFormatter() { + return DateTimeFormatter.ofPattern("dd/MMM/yyyy:HH:mm:ss,SSS Z"); + } + + @Override + public JsonLogEntry toJsonLogEntry(LoggingEvent event) { + return new HttpJsonLogEntry(event); + } + + @SuppressWarnings("unused") + private class HttpJsonLogEntry extends JsonLogEntry { + public String host; + public String thread; + public String user; + public String timestamp; + public String method; + public String resource; + public String protocol; + public String status; + public String contentLength; + public String referer; + public String userAgent; + + public HttpJsonLogEntry(LoggingEvent event) { + this.host = getMdcString(event, P_HOST); + this.thread = event.getThreadName(); + this.user = getMdcString(event, P_USER); + this.timestamp = formatDate(event.getTimeStamp()); + this.method = getMdcString(event, P_METHOD); + this.resource = getMdcString(event, P_RESOURCE); + this.protocol = getMdcString(event, P_PROTOCOL); + this.status = getMdcString(event, P_STATUS); + this.contentLength = getMdcString(event, P_CONTENT_LENGTH); + this.referer = getMdcString(event, P_REFERER); + this.userAgent = getMdcString(event, P_USER_AGENT); + } + } +} diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java b/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java index b6a2d38710..80edd45e29 100644 --- a/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java +++ b/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java @@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableSet; import com.google.gerrit.metrics.CallbackMetric; import com.google.gerrit.metrics.CallbackMetric0; import com.google.gerrit.metrics.Description; +import com.google.gerrit.metrics.Description.Units; import com.google.gerrit.metrics.MetricMaker; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -45,27 +46,83 @@ public class JettyMetrics { metrics.newCallbackMetric( "http/server/jetty/threadpool/idle_threads", Integer.class, - new Description("Idle threads").setGauge().setUnit("threads")); + new Description("Idle threads").setGauge()); CallbackMetric0 busyThreads = metrics.newCallbackMetric( "http/server/jetty/threadpool/active_threads", Integer.class, - new Description("Active threads").setGauge().setUnit("threads")); + new Description("Active threads").setGauge()); CallbackMetric0 reservedThreads = metrics.newCallbackMetric( "http/server/jetty/threadpool/reserved_threads", Integer.class, - new Description("Reserved threads").setGauge().setUnit("threads")); + new Description("Reserved threads").setGauge()); CallbackMetric0 queueSize = metrics.newCallbackMetric( "http/server/jetty/threadpool/queue_size", Integer.class, - new Description("Queued requests waiting for a thread").setGauge().setUnit("requests")); + new Description("Queued requests waiting for a thread").setGauge()); CallbackMetric0 lowOnThreads = metrics.newCallbackMetric( "http/server/jetty/threadpool/is_low_on_threads", Boolean.class, new Description("Whether thread pool is low on threads").setGauge()); + CallbackMetric0 connections = + metrics.newCallbackMetric( + "http/server/jetty/connections/connections", + Long.class, + new Description("The current number of open connections").setGauge()); + CallbackMetric0 connectionsTotal = + metrics.newCallbackMetric( + "http/server/jetty/connections/connections_total", + Long.class, + new Description("The total number of connections opened").setGauge()); + CallbackMetric0 connectionDurationMax = + metrics.newCallbackMetric( + "http/server/jetty/connections/connections_duration_max", + Long.class, + new Description("The max duration of a connection") + .setGauge() + .setUnit(Units.MILLISECONDS)); + CallbackMetric0 connectionDurationMean = + metrics.newCallbackMetric( + "http/server/jetty/connections/connections_duration_mean", + Double.class, + new Description("The mean duration of a connection") + .setGauge() + .setUnit(Units.MILLISECONDS)); + CallbackMetric0 connectionDurationStDev = + metrics.newCallbackMetric( + "http/server/jetty/connections/connections_duration_stdev", + Double.class, + new Description("The standard deviation of the duration of a connection") + .setGauge() + .setUnit(Units.MILLISECONDS)); + CallbackMetric0 receivedMessages = + metrics.newCallbackMetric( + "http/server/jetty/connections/received_messages", + Long.class, + new Description("The total number of messages received").setGauge()); + CallbackMetric0 sentMessages = + metrics.newCallbackMetric( + "http/server/jetty/connections/sent_messages", + Long.class, + new Description("The total number of messages sent").setGauge()); + CallbackMetric0 receivedBytes = + metrics.newCallbackMetric( + "http/server/jetty/connections/received_bytes", + Long.class, + new Description("Total number of bytes received by tracked connections") + .setGauge() + .setUnit(Units.BYTES)); + CallbackMetric0 sentBytes = + metrics.newCallbackMetric( + "http/server/jetty/connections/sent_bytes", + Long.class, + new Description("Total number of bytes sent by tracked connections") + .setGauge() + .setUnit(Units.BYTES)); + JettyServer.Metrics jettyMetrics = jetty.getMetrics(); metrics.newTrigger( ImmutableSet.>of( @@ -76,7 +133,16 @@ public class JettyMetrics { maxPoolSize, poolSize, queueSize, - lowOnThreads), + lowOnThreads, + connections, + connectionsTotal, + connectionDurationMax, + connectionDurationMean, + connectionDurationStDev, + receivedMessages, + sentMessages, + receivedBytes, + sentBytes), () -> { minPoolSize.set(jettyMetrics.getMinThreads()); maxPoolSize.set(jettyMetrics.getMaxThreads()); @@ -86,6 +152,15 @@ public class JettyMetrics { reservedThreads.set(jettyMetrics.getReservedThreads()); queueSize.set(jettyMetrics.getQueueSize()); lowOnThreads.set(jettyMetrics.isLowOnThreads()); + connections.set(jettyMetrics.getConnections()); + connectionsTotal.set(jettyMetrics.getConnectionsTotal()); + connectionDurationMax.set(jettyMetrics.getConnectionDurationMax()); + connectionDurationMean.set(jettyMetrics.getConnectionDurationMean()); + connectionDurationStDev.set(jettyMetrics.getConnectionDurationStdDev()); + receivedMessages.set(jettyMetrics.getReceivedMessages()); + sentMessages.set(jettyMetrics.getSentMessages()); + receivedBytes.set(jettyMetrics.getReceivedBytes()); + sentBytes.set(jettyMetrics.getSentBytes()); }); } } diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java index 5c892820fb..0bbb51d198 100644 --- a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java +++ b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java @@ -43,6 +43,7 @@ import java.util.concurrent.TimeUnit; import javax.servlet.DispatcherType; import javax.servlet.Filter; import org.eclipse.jetty.http.HttpScheme; +import org.eclipse.jetty.io.ConnectionStatistics; import org.eclipse.jetty.jmx.MBeanContainer; import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.ForwardedRequestCustomizer; @@ -116,9 +117,11 @@ public class JettyServer { static class Metrics { private final QueuedThreadPool threadPool; + private ConnectionStatistics connStats; - Metrics(QueuedThreadPool threadPool) { + Metrics(QueuedThreadPool threadPool, ConnectionStatistics connStats) { this.threadPool = threadPool; + this.connStats = connStats; } public int getIdleThreads() { @@ -152,12 +155,49 @@ public class JettyServer { public boolean isLowOnThreads() { return threadPool.isLowOnThreads(); } + + public long getConnections() { + return connStats.getConnections(); + } + + public long getConnectionsTotal() { + return connStats.getConnectionsTotal(); + } + + public long getConnectionDurationMax() { + return connStats.getConnectionDurationMax(); + } + + public double getConnectionDurationMean() { + return connStats.getConnectionDurationMean(); + } + + public double getConnectionDurationStdDev() { + return connStats.getConnectionDurationStdDev(); + } + + public long getReceivedMessages() { + return connStats.getReceivedMessages(); + } + + public long getSentMessages() { + return connStats.getSentMessages(); + } + + public long getReceivedBytes() { + return connStats.getReceivedBytes(); + } + + public long getSentBytes() { + return connStats.getSentBytes(); + } } private final SitePaths site; private final Server httpd; private final Metrics metrics; private boolean reverseProxy; + private ConnectionStatistics connStats; @Inject JettyServer( @@ -171,7 +211,11 @@ public class JettyServer { QueuedThreadPool pool = threadPool(cfg, threadSettingsConfig); httpd = new Server(pool); httpd.setConnectors(listen(httpd, cfg)); - metrics = new Metrics(pool); + connStats = new ConnectionStatistics(); + for (Connector connector : httpd.getConnectors()) { + connector.addBean(connStats); + } + metrics = new Metrics(pool, connStats); Handler app = makeContext(env, cfg); if (cfg.getBoolean("httpd", "requestLog", !reverseProxy)) { diff --git a/java/com/google/gerrit/pgm/init/api/Section.java b/java/com/google/gerrit/pgm/init/api/Section.java index cbf32a19cf..b5d35f40e6 100644 --- a/java/com/google/gerrit/pgm/init/api/Section.java +++ b/java/com/google/gerrit/pgm/init/api/Section.java @@ -69,7 +69,7 @@ public class Section { all.addAll(Arrays.asList(flags.cfg.getStringList(section, subsection, name))); if (value != null) { - if (all.size() == 0 || all.size() == 1) { + if (all.isEmpty() || all.size() == 1) { flags.cfg.setString(section, subsection, name, value); } else { all.set(0, value); @@ -78,7 +78,7 @@ public class Section { } else if (all.size() == 1) { flags.cfg.unset(section, subsection, name); - } else if (all.size() != 0) { + } else if (!all.isEmpty()) { all.remove(0); flags.cfg.setStringList(section, subsection, name, all); } diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java index 6a16a53023..4fe9daae82 100644 --- a/java/com/google/gerrit/server/account/AccountManager.java +++ b/java/com/google/gerrit/server/account/AccountManager.java @@ -462,8 +462,8 @@ public class AccountManager { } if (filteredExtIdsByScheme.size() > 1 - || !filteredExtIdsByScheme.stream() - .anyMatch(e -> e.key().equals(who.getExternalIdKey()))) { + || filteredExtIdsByScheme.stream() + .noneMatch(e -> e.key().equals(who.getExternalIdKey()))) { u.deleteExternalIds(filteredExtIdsByScheme); } }); diff --git a/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java b/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java index 19582da317..db350c6fe4 100644 --- a/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java +++ b/java/com/google/gerrit/server/account/AccountsConsistencyChecker.java @@ -37,8 +37,8 @@ public class AccountsConsistencyChecker { for (AccountState accountState : accounts.all()) { Account account = accountState.account(); if (account.preferredEmail() != null) { - if (!accountState.externalIds().stream() - .anyMatch(e -> account.preferredEmail().equals(e.email()))) { + if (accountState.externalIds().stream() + .noneMatch(e -> account.preferredEmail().equals(e.email()))) { addError( String.format( "Account '%s' has no external ID for its preferred email '%s'", diff --git a/java/com/google/gerrit/server/args4j/ChangeIdHandler.java b/java/com/google/gerrit/server/args4j/ChangeIdHandler.java index ddcd4dbdc4..448c654105 100644 --- a/java/com/google/gerrit/server/args4j/ChangeIdHandler.java +++ b/java/com/google/gerrit/server/args4j/ChangeIdHandler.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.args4j; import static com.google.gerrit.util.cli.Localizable.localizable; import com.google.common.base.Splitter; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; @@ -35,34 +36,42 @@ import org.kohsuke.args4j.spi.Parameters; import org.kohsuke.args4j.spi.Setter; public class ChangeIdHandler extends OptionHandler { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private final Provider queryProvider; @Inject public ChangeIdHandler( // TODO(dborowitz): Not sure whether this is injectable here. Provider queryProvider, - @Assisted final CmdLineParser parser, - @Assisted final OptionDef option, - @Assisted final Setter setter) { + @Assisted CmdLineParser parser, + @Assisted OptionDef option, + @Assisted Setter setter) { super(parser, option, setter); this.queryProvider = queryProvider; } @Override public final int parseArguments(Parameters params) throws CmdLineException { - final String token = params.getParameter(0); - final List tokens = Splitter.on(',').splitToList(token); + String token = params.getParameter(0); + List tokens = Splitter.on(',').splitToList(token); if (tokens.size() != 3) { throw new CmdLineException( owner, localizable("change should be specified as ,,")); } try { - final Change.Key key = Change.Key.parse(tokens.get(2)); - final Project.NameKey project = Project.nameKey(tokens.get(0)); - final BranchNameKey branch = BranchNameKey.create(project, tokens.get(1)); - for (ChangeData cd : queryProvider.get().byBranchKey(branch, key)) { - setter.addValue(cd.getId()); + Change.Key key = Change.Key.parse(tokens.get(2)); + Project.NameKey project = Project.nameKey(tokens.get(0)); + BranchNameKey branch = BranchNameKey.create(project, tokens.get(1)); + List changes = queryProvider.get().byBranchKey(branch, key); + if (!changes.isEmpty()) { + if (changes.size() > 1) { + String msg = "\"%s\": resolves to multiple changes"; + logger.atSevere().log(msg, token); + throw new CmdLineException(owner, localizable(msg), token); + } + setter.addValue(changes.get(0).getId()); return 1; } } catch (IllegalArgumentException e) { diff --git a/java/com/google/gerrit/server/cache/CacheMetrics.java b/java/com/google/gerrit/server/cache/CacheMetrics.java index 1ef5a3b953..5d309525db 100644 --- a/java/com/google/gerrit/server/cache/CacheMetrics.java +++ b/java/com/google/gerrit/server/cache/CacheMetrics.java @@ -32,10 +32,11 @@ import java.util.Set; @Singleton public class CacheMetrics { + private static final Field F_NAME = + Field.ofString("cache_name", Metadata.Builder::cacheName).build(); + @Inject public CacheMetrics(MetricMaker metrics, DynamicMap> cacheMap) { - Field F_NAME = Field.ofString("cache_name", Metadata.Builder::cacheName).build(); - CallbackMetric1 memEnt = metrics.newCallbackMetric( "caches/memory_cached", diff --git a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java index 4d41ed7b1c..0a38ee8326 100644 --- a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java +++ b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java @@ -25,12 +25,12 @@ import org.eclipse.jgit.lib.Config; @Singleton public class ChangeCleanupConfig { - private static String SECTION = "changeCleanup"; - private static String KEY_ABANDON_AFTER = "abandonAfter"; - private static String KEY_ABANDON_IF_MERGEABLE = "abandonIfMergeable"; - private static String KEY_ABANDON_MESSAGE = "abandonMessage"; - private static String KEY_CLEANUP_ACCOUNT_PATCH_REVIEW = "cleanupAccountPatchReview"; - private static String DEFAULT_ABANDON_MESSAGE = + private static final String SECTION = "changeCleanup"; + private static final String KEY_ABANDON_AFTER = "abandonAfter"; + private static final String KEY_ABANDON_IF_MERGEABLE = "abandonIfMergeable"; + private static final String KEY_ABANDON_MESSAGE = "abandonMessage"; + private static final String KEY_CLEANUP_ACCOUNT_PATCH_REVIEW = "cleanupAccountPatchReview"; + private static final String DEFAULT_ABANDON_MESSAGE = "Auto-Abandoned due to inactivity, see " + "${URL}\n" + "\n" diff --git a/java/com/google/gerrit/server/config/TrackingFootersProvider.java b/java/com/google/gerrit/server/config/TrackingFootersProvider.java index 2114b1a31b..1611da902d 100644 --- a/java/com/google/gerrit/server/config/TrackingFootersProvider.java +++ b/java/com/google/gerrit/server/config/TrackingFootersProvider.java @@ -34,10 +34,10 @@ public class TrackingFootersProvider implements Provider { private static final int MAX_LENGTH = 10; - private static String TRACKING_ID_TAG = "trackingid"; - private static String FOOTER_TAG = "footer"; - private static String SYSTEM_TAG = "system"; - private static String REGEX_TAG = "match"; + private static final String TRACKING_ID_TAG = "trackingid"; + private static final String FOOTER_TAG = "footer"; + private static final String SYSTEM_TAG = "system"; + private static final String REGEX_TAG = "match"; private final List trackingFooters = new ArrayList<>(); @Inject diff --git a/java/com/google/gerrit/server/documentation/QueryDocumentationExecutor.java b/java/com/google/gerrit/server/documentation/QueryDocumentationExecutor.java index a15b42987f..59ae6f872f 100644 --- a/java/com/google/gerrit/server/documentation/QueryDocumentationExecutor.java +++ b/java/com/google/gerrit/server/documentation/QueryDocumentationExecutor.java @@ -22,7 +22,6 @@ import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; import java.util.List; -import java.util.Map; import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; import org.apache.lucene.analysis.standard.StandardAnalyzer; @@ -42,7 +41,7 @@ import org.apache.lucene.store.RAMDirectory; public class QueryDocumentationExecutor { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private static Map WEIGHTS = + private static final ImmutableMap WEIGHTS = ImmutableMap.of( Constants.TITLE_FIELD, 2.0f, Constants.DOC_FIELD, 1.0f); diff --git a/java/com/google/gerrit/server/events/StreamEventsApiListener.java b/java/com/google/gerrit/server/events/StreamEventsApiListener.java index b040f38f38..18b6a5e453 100644 --- a/java/com/google/gerrit/server/events/StreamEventsApiListener.java +++ b/java/com/google/gerrit/server/events/StreamEventsApiListener.java @@ -230,7 +230,7 @@ public class StreamEventsApiListener } String[] hashtagArray(Collection hashtags) { - if (hashtags != null && hashtags.size() > 0) { + if (hashtags != null && !hashtags.isEmpty()) { return Sets.newHashSet(hashtags).toArray(new String[hashtags.size()]); } return null; diff --git a/java/com/google/gerrit/server/git/GroupCollector.java b/java/com/google/gerrit/server/git/GroupCollector.java index c284f7f4e8..dd3919870b 100644 --- a/java/com/google/gerrit/server/git/GroupCollector.java +++ b/java/com/google/gerrit/server/git/GroupCollector.java @@ -141,7 +141,7 @@ public class GroupCollector { checkState(!done, "visit() called after getGroups()"); Set interestingParents = getInterestingParents(c); - if (interestingParents.size() == 0) { + if (interestingParents.isEmpty()) { // All parents are uninteresting: treat this commit as the root of a new // group of related changes. groups.put(c, c.name()); diff --git a/java/com/google/gerrit/server/git/NotifyConfig.java b/java/com/google/gerrit/server/git/NotifyConfig.java index 2ca2744a51..429f15a95f 100644 --- a/java/com/google/gerrit/server/git/NotifyConfig.java +++ b/java/com/google/gerrit/server/git/NotifyConfig.java @@ -50,11 +50,11 @@ public class NotifyConfig implements Comparable { return types.contains(type) || types.contains(NotifyType.ALL); } - public EnumSet getNotify() { + public Set getNotify() { return types; } - public void setTypes(EnumSet newTypes) { + public void setTypes(Set newTypes) { types = EnumSet.copyOf(newTypes); } diff --git a/java/com/google/gerrit/server/git/meta/TabFile.java b/java/com/google/gerrit/server/git/meta/TabFile.java index 4c0378a0ec..64ae6dd4b1 100644 --- a/java/com/google/gerrit/server/git/meta/TabFile.java +++ b/java/com/google/gerrit/server/git/meta/TabFile.java @@ -33,7 +33,7 @@ public class TabFile { String parse(String str); } - public static Parser TRIM = String::trim; + public static final Parser TRIM = String::trim; protected static class Row { public String left; diff --git a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java index 38c8d7dafc..8ab2779a45 100644 --- a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java +++ b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java @@ -412,7 +412,7 @@ public abstract class VersionedMetaData { // read the subject line and use it as reflog message ru.setRefLogMessage("commit: " + reader.readLine(), true); } - logger.atFine().log("Saving commit: " + message); + logger.atFine().log("Saving commit '%s' on project '%s'", message.trim(), projectName); inserter.flush(); RefUpdate.Result result = ru.update(); switch (result) { @@ -420,6 +420,9 @@ public abstract class VersionedMetaData { case FAST_FORWARD: revision = rw.parseCommit(ru.getNewObjectId()); update.fireGitRefUpdatedEvent(ru); + logger.atFine().log( + "Saved commit '%s' as revision '%s' on project '%s'", + message.trim(), revision.name(), projectName); return revision; case LOCK_FAILURE: throw new LockFailureException(errorMsg(ru, db.getDirectory()), ru); diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index c6c9b393a9..ead76ccacc 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -2267,7 +2267,7 @@ class ReceiveCommits { return Collections.emptyList(); } - if (changes.size() == 0) { + if (changes.isEmpty()) { if (!isValidChangeId(p.changeKey.get())) { reject(magicBranch.cmd, "invalid Change-Id"); return Collections.emptyList(); diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java index 7e1353cfc7..63c52977a6 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java +++ b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java @@ -78,7 +78,7 @@ public class ChangeIndexRewriter implements IndexRewriter { * @return the maximal set of statuses that any changes matching the input predicates may have, * based on examining boolean and {@link ChangeStatusPredicate}s. */ - public static EnumSet getPossibleStatus(Predicate in) { + public static Set getPossibleStatus(Predicate in) { EnumSet s = extractStatus(in); return s != null ? s : EnumSet.allOf(Change.Status.class); } diff --git a/java/com/google/gerrit/server/logging/BUILD b/java/com/google/gerrit/server/logging/BUILD index 2c2341f1ea..7af34f7cd7 100644 --- a/java/com/google/gerrit/server/logging/BUILD +++ b/java/com/google/gerrit/server/logging/BUILD @@ -10,11 +10,13 @@ java_library( "//java/com/google/gerrit/common:annotations", "//java/com/google/gerrit/extensions:api", "//java/com/google/gerrit/server/util/time", + "//lib:gson", "//lib:guava", "//lib:jgit", "//lib/auto:auto-value", "//lib/auto:auto-value-annotations", "//lib/flogger:api", "//lib/guice", + "//lib/log:log4j", ], ) diff --git a/java/com/google/gerrit/server/logging/JsonLayout.java b/java/com/google/gerrit/server/logging/JsonLayout.java new file mode 100644 index 0000000000..3eb451547b --- /dev/null +++ b/java/com/google/gerrit/server/logging/JsonLayout.java @@ -0,0 +1,72 @@ +// Copyright (C) 2020 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.logging; + +import com.google.gson.FieldNamingPolicy; +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import java.time.Instant; +import java.time.LocalDateTime; +import java.time.OffsetDateTime; +import java.time.ZoneId; +import java.time.ZoneOffset; +import java.time.ZonedDateTime; +import java.time.format.DateTimeFormatter; +import org.apache.log4j.Layout; +import org.apache.log4j.spi.LoggingEvent; + +public abstract class JsonLayout extends Layout { + private final DateTimeFormatter dateFormatter; + private final Gson gson; + private final ZoneOffset timeOffset; + + public JsonLayout() { + dateFormatter = createDateTimeFormatter(); + timeOffset = OffsetDateTime.now().getOffset(); + + gson = newGson(); + } + + public abstract DateTimeFormatter createDateTimeFormatter(); + + public abstract JsonLogEntry toJsonLogEntry(LoggingEvent event); + + @Override + public String format(LoggingEvent event) { + return gson.toJson(toJsonLogEntry(event)) + "\n"; + } + + private static Gson newGson() { + GsonBuilder gb = + new GsonBuilder() + .setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES) + .disableHtmlEscaping(); + return gb.create(); + } + + public String formatDate(long now) { + return ZonedDateTime.of( + LocalDateTime.ofInstant(Instant.ofEpochMilli(now), timeOffset), ZoneId.systemDefault()) + .format(dateFormatter); + } + + @Override + public void activateOptions() {} + + @Override + public boolean ignoresThrowable() { + return false; + } +} diff --git a/java/com/google/gerrit/acceptance/LogThreshold.java b/java/com/google/gerrit/server/logging/JsonLogEntry.java similarity index 52% rename from java/com/google/gerrit/acceptance/LogThreshold.java rename to java/com/google/gerrit/server/logging/JsonLogEntry.java index 36831f3f1a..bc16c70e09 100644 --- a/java/com/google/gerrit/acceptance/LogThreshold.java +++ b/java/com/google/gerrit/server/logging/JsonLogEntry.java @@ -1,4 +1,4 @@ -// Copyright (C) 2019 The Android Open Source Project +// Copyright (C) 2020 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. @@ -11,19 +11,13 @@ // 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.acceptance; -import static java.lang.annotation.ElementType.METHOD; -import static java.lang.annotation.ElementType.TYPE; -import static java.lang.annotation.RetentionPolicy.RUNTIME; +package com.google.gerrit.server.logging; -import java.lang.annotation.Inherited; -import java.lang.annotation.Retention; -import java.lang.annotation.Target; +import org.apache.log4j.spi.LoggingEvent; -@Target({TYPE, METHOD}) -@Retention(RUNTIME) -@Inherited -public @interface LogThreshold { - String level() default "DEBUG"; +public abstract class JsonLogEntry { + public String getMdcString(LoggingEvent event, String key) { + return (String) event.getMDC(key); + } } diff --git a/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/java/com/google/gerrit/server/mail/send/ProjectWatch.java index 934a0a00a0..2530d7e7fc 100644 --- a/java/com/google/gerrit/server/mail/send/ProjectWatch.java +++ b/java/com/google/gerrit/server/mail/send/ProjectWatch.java @@ -244,7 +244,7 @@ public class ProjectWatch { qb = args.queryBuilder.asUser(args.anonymousUser); } else { qb = args.queryBuilder.asUser(user); - p = qb.is_visible(); + p = qb.isVisible(); } if (filter != null) { diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index ce88f072a4..ee3ccd6d01 100644 --- a/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -184,6 +184,14 @@ public abstract class AbstractChangeUpdate { protected abstract String getRefName(); + protected void setParentCommit(CommitBuilder cb, ObjectId parentCommitId) { + if (!parentCommitId.equals(ObjectId.zeroId())) { + cb.setParentId(parentCommitId); + } else { + cb.setParentIds(); // Ref is currently nonexistent, commit has no parents. + } + } + /** * Whether to allow bypassing the check that an update does not exceed the max update count on an * object. @@ -224,11 +232,7 @@ public abstract class AbstractChangeUpdate { } cb.setAuthor(authorIdent); cb.setCommitter(new PersonIdent(serverIdent, when)); - if (!curr.equals(z)) { - cb.setParentId(curr); - } else { - cb.setParentIds(); // Ref is currently nonexistent, commit has no parents. - } + setParentCommit(cb, curr); if (cb.getTreeId() == null) { if (curr.equals(z)) { cb.setTreeId(emptyTree(ins)); // No parent, assume empty tree. diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index b55ce49655..05fdee9a6e 100644 --- a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -286,6 +286,11 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { return RefNames.refsDraftComments(getId(), accountId); } + @Override + protected void setParentCommit(CommitBuilder cb, ObjectId parentCommitId) { + cb.setParentIds(); // Draft updates should not keep history of parent commits + } + @Override public boolean isEmpty() { return delete.isEmpty() && put.isEmpty(); diff --git a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java index 9ec1d690ac..128e185bb6 100644 --- a/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java +++ b/java/com/google/gerrit/server/notedb/DeleteZombieCommentsRefs.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb; import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.common.collect.Iterables; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Change; import com.google.gerrit.git.RefUpdateUtil; import com.google.gerrit.server.config.AllUsersName; @@ -42,9 +43,12 @@ import org.eclipse.jgit.transport.ReceiveCommand; * and not get deleted. These refs point to an empty tree. */ public class DeleteZombieCommentsRefs { - private final String EMPTY_TREE_ID = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"; - private final String DRAFT_REFS_PREFIX = "refs/draft-comments"; - private final int CHUNK_SIZE = 100; // log progress after deleting every CHUNK_SIZE refs + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private static final String EMPTY_TREE_ID = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"; + private static final String DRAFT_REFS_PREFIX = "refs/draft-comments"; + private static final int CHUNK_SIZE = 100; // log progress after deleting every CHUNK_SIZE refs + private final GitRepositoryManager repoManager; private final AllUsersName allUsers; private final int cleanupPercentage; @@ -70,18 +74,15 @@ public class DeleteZombieCommentsRefs { List draftRefs = allUsersRepo.getRefDatabase().getRefsByPrefix(DRAFT_REFS_PREFIX); List zombieRefs = filterZombieRefs(draftRefs); - System.out.println( - String.format( - "Found a total of %d zombie draft refs in %s repo.", - zombieRefs.size(), allUsers.get())); + logger.atInfo().log( + "Found a total of %d zombie draft refs in %s repo.", zombieRefs.size(), allUsers.get()); - System.out.println(String.format("Cleanup percentage = %d", cleanupPercentage)); + logger.atInfo().log("Cleanup percentage = %d", cleanupPercentage); zombieRefs = zombieRefs.stream() .filter(ref -> Change.Id.fromAllUsersRef(ref.getName()).get() % 100 < cleanupPercentage) .collect(toImmutableList()); - System.out.println( - String.format("Number of zombie refs to be cleaned = %d", zombieRefs.size())); + logger.atInfo().log("Number of zombie refs to be cleaned = %d", zombieRefs.size()); long zombieRefsCnt = zombieRefs.size(); long deletedRefsCnt = 0; @@ -124,7 +125,7 @@ public class DeleteZombieCommentsRefs { } private void logProgress(long deletedRefsCount, long allRefsCount, long elapsed) { - System.out.format( + logger.atInfo().log( "Deleted %d/%d zombie draft refs (%d seconds)\n", deletedRefsCount, allRefsCount, elapsed); } } diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java index a525e29da8..e0c5927f96 100644 --- a/java/com/google/gerrit/server/permissions/PermissionBackend.java +++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java @@ -501,12 +501,12 @@ public abstract class PermissionBackend { public Set testLabels(Collection types) throws PermissionBackendException { requireNonNull(types, "LabelType"); - return test(types.stream().flatMap((t) -> valuesOf(t).stream()).collect(toSet())); + return test(types.stream().flatMap(t -> valuesOf(t).stream()).collect(toSet())); } private static Set valuesOf(LabelType label) { return label.getValues().stream() - .map((v) -> new LabelPermission.WithValue(label, v)) + .map(v -> new LabelPermission.WithValue(label, v)) .collect(toSet()); } } diff --git a/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java b/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java index 3a73d0cb2a..ae9828aaa8 100644 --- a/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java +++ b/java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java @@ -27,7 +27,7 @@ import java.util.HashSet; /** Provides transformations to get and set BooleanProjectConfigs from the API. */ public class BooleanProjectConfigTransformations { - private static ImmutableMap MAPPER = + private static final ImmutableMap MAPPER = ImmutableMap.builder() .put( BooleanProjectConfig.USE_CONTRIBUTOR_AGREEMENTS, diff --git a/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java b/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java index f6aba34c46..a3b4126c55 100644 --- a/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java +++ b/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java @@ -239,7 +239,7 @@ public class ProjectsConsistencyChecker { predicates.add(new CommitPredicate(commit.name())); } - if (predicates.size() > 0) { + if (!predicates.isEmpty()) { // Execute the query with the remaining predicates that were collected. autoCloseableChanges.addAll( executeQueryAndAutoCloseChanges( @@ -266,13 +266,12 @@ public class ProjectsConsistencyChecker { List queryResult = retryHelper.execute( ActionType.INDEX_QUERY, - () -> { - // Execute the query. - return changeQueryProvider - .get() - .setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET) - .query(and(basePredicate, or(predicates))); - }, + () -> + // Execute the query. + changeQueryProvider + .get() + .setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET) + .query(and(basePredicate, or(predicates))), StorageException.class::isInstance); // Result for this query that we want to return to the client. diff --git a/java/com/google/gerrit/server/query/account/InternalAccountQuery.java b/java/com/google/gerrit/server/query/account/InternalAccountQuery.java index b23326007e..091edca081 100644 --- a/java/com/google/gerrit/server/query/account/InternalAccountQuery.java +++ b/java/com/google/gerrit/server/query/account/InternalAccountQuery.java @@ -71,7 +71,7 @@ public class InternalAccountQuery extends InternalQuery accountStates = byExternalId(externalId); if (accountStates.size() == 1) { return accountStates.get(0); - } else if (accountStates.size() > 0) { + } else if (!accountStates.isEmpty()) { StringBuilder msg = new StringBuilder(); msg.append("Ambiguous external ID ").append(externalId).append(" for accounts: "); Joiner.on(", ") diff --git a/java/com/google/gerrit/server/query/change/AndChangeSource.java b/java/com/google/gerrit/server/query/change/AndChangeSource.java index 4a3b936605..749204f3f1 100644 --- a/java/com/google/gerrit/server/query/change/AndChangeSource.java +++ b/java/com/google/gerrit/server/query/change/AndChangeSource.java @@ -35,9 +35,7 @@ public class AndChangeSource extends AndSource implements ChangeData @Override public boolean hasChange() { - return source != null - && source instanceof ChangeDataSource - && ((ChangeDataSource) source).hasChange(); + return source instanceof ChangeDataSource && ((ChangeDataSource) source).hasChange(); } @Override diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index d2fc77da2c..cdf1243907 100644 --- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -484,7 +484,7 @@ public class ChangeQueryBuilder extends QueryBuilder status_open() { + public Predicate statusOpen() { return ChangeStatusPredicate.open(); } @@ -533,7 +533,7 @@ public class ChangeQueryBuilder extends QueryBuilder visibleto(String who) throws QueryParseException, IOException, ConfigInvalidException { if (isSelf(who)) { - return is_visible(); + return isVisible(); } try { return Predicate.or( @@ -977,7 +977,7 @@ public class ChangeQueryBuilder extends QueryBuilder is_visible() throws QueryParseException { + public Predicate isVisible() throws QueryParseException { return visibleto(args.getUser()); } diff --git a/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java b/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java index 218a89d6a0..3a43fd3669 100644 --- a/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java +++ b/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java @@ -77,13 +77,13 @@ public class IsWatchedByPredicate extends AndPredicate { } else if (f != null) { r.add(f); } else { - r.add(builder.status_open()); + r.add(builder.statusOpen()); } } if (r.isEmpty()) { return ImmutableList.of(ChangeIndexPredicate.none()); } else if (checkIsVisible) { - return ImmutableList.of(or(r), builder.is_visible()); + return ImmutableList.of(or(r), builder.isVisible()); } else { return ImmutableList.of(or(r)); } diff --git a/java/com/google/gerrit/server/restapi/account/DeleteExternalIds.java b/java/com/google/gerrit/server/restapi/account/DeleteExternalIds.java index 82b445f774..0b51bf836b 100644 --- a/java/com/google/gerrit/server/restapi/account/DeleteExternalIds.java +++ b/java/com/google/gerrit/server/restapi/account/DeleteExternalIds.java @@ -70,7 +70,7 @@ public class DeleteExternalIds implements RestModifyView { throw new IllegalStateException("unknown change type: " + ps.getChangeType()); } - if (ps.getPatchHeader().size() > 0) { + if (!ps.getPatchHeader().isEmpty()) { result.diffHeader = ps.getPatchHeader(); } result.content = content.lines; diff --git a/java/com/google/gerrit/server/restapi/change/GetPatch.java b/java/com/google/gerrit/server/restapi/change/GetPatch.java index 66ccef3a30..187ebcee19 100644 --- a/java/com/google/gerrit/server/restapi/change/GetPatch.java +++ b/java/com/google/gerrit/server/restapi/change/GetPatch.java @@ -43,7 +43,7 @@ import org.kohsuke.args4j.Option; public class GetPatch implements RestReadView { private final GitRepositoryManager repoManager; - private final String FILE_NOT_FOUND = "File not found: %s."; + private static final String FILE_NOT_FOUND = "File not found: %s."; @Option(name = "--zip") private boolean zip; diff --git a/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java b/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java index caa256f6db..214a0015a1 100644 --- a/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java +++ b/java/com/google/gerrit/server/restapi/change/SubmittedTogether.java @@ -46,6 +46,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.EnumSet; import java.util.List; +import java.util.Set; import org.kohsuke.args4j.Option; public class SubmittedTogether implements RestReadView { @@ -97,12 +98,12 @@ public class SubmittedTogether implements RestReadView { this.sorter = sorter; } - public SubmittedTogether addListChangesOption(EnumSet o) { + public SubmittedTogether addListChangesOption(Set o) { jsonOpt.addAll(o); return this; } - public SubmittedTogether addSubmittedTogetherOption(EnumSet o) { + public SubmittedTogether addSubmittedTogetherOption(Set o) { options.addAll(o); return this; } diff --git a/java/com/google/gerrit/server/restapi/group/ListGroups.java b/java/com/google/gerrit/server/restapi/group/ListGroups.java index 899ed006bc..d583b8e3b3 100644 --- a/java/com/google/gerrit/server/restapi/group/ListGroups.java +++ b/java/com/google/gerrit/server/restapi/group/ListGroups.java @@ -86,7 +86,7 @@ public class ListGroups implements RestReadView { private final Groups groups; private final GroupResolver groupResolver; - private EnumSet options = EnumSet.noneOf(ListGroupsOption.class); + private Set options = EnumSet.noneOf(ListGroupsOption.class); private boolean visibleToAll; private Account.Id user; private boolean owned; @@ -235,7 +235,7 @@ public class ListGroups implements RestReadView { this.groupResolver = groupResolver; } - public void setOptions(EnumSet options) { + public void setOptions(Set options) { this.options = options; } diff --git a/java/com/google/gerrit/server/restapi/project/ListProjects.java b/java/com/google/gerrit/server/restapi/project/ListProjects.java index 345340bde6..63842827f3 100644 --- a/java/com/google/gerrit/server/restapi/project/ListProjects.java +++ b/java/com/google/gerrit/server/restapi/project/ListProjects.java @@ -350,7 +350,7 @@ public class ListProjects implements RestReadView { queries.add(String.format("(state:%s)", state.name())); } - return Joiner.on(" AND ").join(queries).toString(); + return Joiner.on(" AND ").join(queries); } private SortedMap applyAsQuery(String query) throws BadRequestException { diff --git a/java/com/google/gerrit/server/restapi/project/SetParent.java b/java/com/google/gerrit/server/restapi/project/SetParent.java index 4cf0182b8b..a610dd42b3 100644 --- a/java/com/google/gerrit/server/restapi/project/SetParent.java +++ b/java/com/google/gerrit/server/restapi/project/SetParent.java @@ -165,12 +165,7 @@ public class SetParent throw new ResourceConflictException("cannot set parent to self"); } - if (Iterables.tryFind( - parent.tree(), - p -> { - return p.getNameKey().equals(project); - }) - .isPresent()) { + if (Iterables.tryFind(parent.tree(), p -> p.getNameKey().equals(project)).isPresent()) { throw new ResourceConflictException( "cycle exists between " + project.get() + " and " + parent.getName()); } diff --git a/java/com/google/gerrit/server/update/RetryingRestCollectionModifyView.java b/java/com/google/gerrit/server/update/RetryingRestCollectionModifyView.java index 96c2ed37e8..bce1209bb9 100644 --- a/java/com/google/gerrit/server/update/RetryingRestCollectionModifyView.java +++ b/java/com/google/gerrit/server/update/RetryingRestCollectionModifyView.java @@ -45,7 +45,7 @@ public abstract class RetryingRestCollectionModifyView< .onAutoTrace(traceId::set) .build(); return retryHelper - .execute((updateFactory) -> applyImpl(updateFactory, parentResource, input), retryOptions) + .execute(updateFactory -> applyImpl(updateFactory, parentResource, input), retryOptions) .traceId(traceId.get()); } catch (Exception e) { Throwables.throwIfInstanceOf(e, RestApiException.class); diff --git a/java/com/google/gerrit/server/update/RetryingRestModifyView.java b/java/com/google/gerrit/server/update/RetryingRestModifyView.java index 275dc55a56..56c3eec095 100644 --- a/java/com/google/gerrit/server/update/RetryingRestModifyView.java +++ b/java/com/google/gerrit/server/update/RetryingRestModifyView.java @@ -40,7 +40,7 @@ public abstract class RetryingRestModifyView .onAutoTrace(traceId::set) .build(); return retryHelper - .execute((updateFactory) -> applyImpl(updateFactory, resource, input), retryOptions) + .execute(updateFactory -> applyImpl(updateFactory, resource, input), retryOptions) .traceId(traceId.get()); } catch (Exception e) { Throwables.throwIfInstanceOf(e, RestApiException.class); diff --git a/java/com/google/gerrit/sshd/SshLog.java b/java/com/google/gerrit/sshd/SshLog.java index e8df4416c0..2b3052c8f9 100644 --- a/java/com/google/gerrit/sshd/SshLog.java +++ b/java/com/google/gerrit/sshd/SshLog.java @@ -45,15 +45,18 @@ import org.eclipse.jgit.lib.Config; @Singleton class SshLog implements LifecycleListener, GerritConfigListener { private static final Logger log = Logger.getLogger(SshLog.class); - private static final String LOG_NAME = "sshd_log"; - private static final String P_SESSION = "session"; - private static final String P_USER_NAME = "userName"; - private static final String P_ACCOUNT_ID = "accountId"; - private static final String P_WAIT = "queueWaitTime"; - private static final String P_EXEC = "executionTime"; - private static final String P_STATUS = "status"; - private static final String P_AGENT = "agent"; - private static final String P_MESSAGE = "message"; + + private static final String JSON_SUFFIX = ".json"; + + protected static final String LOG_NAME = "sshd_log"; + protected static final String P_SESSION = "session"; + protected static final String P_USER_NAME = "userName"; + protected static final String P_ACCOUNT_ID = "accountId"; + protected static final String P_WAIT = "queueWaitTime"; + protected static final String P_EXEC = "executionTime"; + protected static final String P_STATUS = "status"; + protected static final String P_AGENT = "agent"; + protected static final String P_MESSAGE = "message"; private final Provider session; private final Provider context; @@ -61,6 +64,9 @@ class SshLog implements LifecycleListener, GerritConfigListener { private final GroupAuditService auditService; private final SystemLog systemLog; + private final boolean json; + private final boolean text; + private final Object lock = new Object(); @Inject @@ -75,6 +81,9 @@ class SshLog implements LifecycleListener, GerritConfigListener { this.auditService = auditService; this.systemLog = systemLog; + this.json = config.getBoolean("log", "jsonLogging", false); + this.text = config.getBoolean("log", "textLogging", true) || !json; + if (config.getBoolean("sshd", "requestLog", true)) { enableLogging(); } @@ -84,7 +93,16 @@ class SshLog implements LifecycleListener, GerritConfigListener { public boolean enableLogging() { synchronized (lock) { if (async == null) { - async = systemLog.createAsyncAppender(LOG_NAME, new SshLogLayout()); + async = new AsyncAppender(); + + if (text) { + async.addAppender(systemLog.createAsyncAppender(LOG_NAME, new SshLogLayout())); + } + + if (json) { + async.addAppender( + systemLog.createAsyncAppender(LOG_NAME + JSON_SUFFIX, new SshLogJsonLayout())); + } return true; } return false; diff --git a/java/com/google/gerrit/sshd/SshLogJsonLayout.java b/java/com/google/gerrit/sshd/SshLogJsonLayout.java new file mode 100644 index 0000000000..8495ed105f --- /dev/null +++ b/java/com/google/gerrit/sshd/SshLogJsonLayout.java @@ -0,0 +1,96 @@ +// Copyright (C) 2020 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 static com.google.gerrit.sshd.SshLog.P_ACCOUNT_ID; +import static com.google.gerrit.sshd.SshLog.P_AGENT; +import static com.google.gerrit.sshd.SshLog.P_EXEC; +import static com.google.gerrit.sshd.SshLog.P_MESSAGE; +import static com.google.gerrit.sshd.SshLog.P_SESSION; +import static com.google.gerrit.sshd.SshLog.P_STATUS; +import static com.google.gerrit.sshd.SshLog.P_USER_NAME; +import static com.google.gerrit.sshd.SshLog.P_WAIT; + +import com.google.gerrit.server.logging.JsonLayout; +import com.google.gerrit.server.logging.JsonLogEntry; +import java.time.format.DateTimeFormatter; +import org.apache.log4j.spi.LoggingEvent; + +public class SshLogJsonLayout extends JsonLayout { + + @Override + public DateTimeFormatter createDateTimeFormatter() { + return DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss,SSS Z"); + } + + @Override + public JsonLogEntry toJsonLogEntry(LoggingEvent event) { + return new SshJsonLogEntry(event); + } + + @SuppressWarnings("unused") + private class SshJsonLogEntry extends JsonLogEntry { + public String timestamp; + public String session; + public String thread; + public String user; + public String accountId; + public String message; + public String waitTime; + public String execTime; + public String status; + public String agent; + public String timeNegotiating; + public String timeSearchReuse; + public String timeSearchSizes; + public String timeCounting; + public String timeCompressing; + public String timeWriting; + public String timeTotal; + public String bitmapIndexMisses; + public String deltasTotal; + public String objectsTotal; + public String bytesTotal; + + public SshJsonLogEntry(LoggingEvent event) { + this.timestamp = formatDate(event.getTimeStamp()); + this.session = getMdcString(event, P_SESSION); + this.thread = event.getThreadName(); + this.user = getMdcString(event, P_USER_NAME); + this.accountId = getMdcString(event, P_ACCOUNT_ID); + this.message = (String) event.getMessage(); + this.waitTime = getMdcString(event, P_WAIT); + this.execTime = getMdcString(event, P_EXEC); + this.status = getMdcString(event, P_STATUS); + this.agent = getMdcString(event, P_AGENT); + + String metricString = getMdcString(event, P_MESSAGE); + if (metricString != null && !metricString.isEmpty()) { + String[] ssh_metrics = metricString.split(" "); + this.timeNegotiating = ssh_metrics[0]; + this.timeSearchReuse = ssh_metrics[1]; + this.timeSearchSizes = ssh_metrics[2]; + this.timeCounting = ssh_metrics[3]; + this.timeCompressing = ssh_metrics[4]; + this.timeWriting = ssh_metrics[5]; + this.timeTotal = ssh_metrics[6]; + this.bitmapIndexMisses = ssh_metrics[7]; + this.deltasTotal = ssh_metrics[8]; + this.objectsTotal = ssh_metrics[9]; + this.bytesTotal = ssh_metrics[10]; + } + } + } +} diff --git a/java/com/google/gerrit/sshd/SshLogLayout.java b/java/com/google/gerrit/sshd/SshLogLayout.java index f16dd732e7..c676be99e5 100644 --- a/java/com/google/gerrit/sshd/SshLogLayout.java +++ b/java/com/google/gerrit/sshd/SshLogLayout.java @@ -14,6 +14,15 @@ package com.google.gerrit.sshd; +import static com.google.gerrit.sshd.SshLog.P_ACCOUNT_ID; +import static com.google.gerrit.sshd.SshLog.P_AGENT; +import static com.google.gerrit.sshd.SshLog.P_EXEC; +import static com.google.gerrit.sshd.SshLog.P_MESSAGE; +import static com.google.gerrit.sshd.SshLog.P_SESSION; +import static com.google.gerrit.sshd.SshLog.P_STATUS; +import static com.google.gerrit.sshd.SshLog.P_USER_NAME; +import static com.google.gerrit.sshd.SshLog.P_WAIT; + import java.text.SimpleDateFormat; import java.util.Calendar; import java.util.TimeZone; @@ -23,15 +32,6 @@ import org.eclipse.jgit.util.QuotedString; public final class SshLogLayout extends Layout { - private static final String P_SESSION = "session"; - private static final String P_USER_NAME = "userName"; - private static final String P_ACCOUNT_ID = "accountId"; - private static final String P_WAIT = "queueWaitTime"; - private static final String P_EXEC = "executionTime"; - private static final String P_STATUS = "status"; - private static final String P_AGENT = "agent"; - private static final String P_MESSAGE = "message"; - private final Calendar calendar; private long lastTimeMillis; private final char[] lastTimeString = new char[20]; diff --git a/java/com/google/gerrit/sshd/commands/FlushCaches.java b/java/com/google/gerrit/sshd/commands/FlushCaches.java index 0c9bbb5b67..2afc009a20 100644 --- a/java/com/google/gerrit/sshd/commands/FlushCaches.java +++ b/java/com/google/gerrit/sshd/commands/FlushCaches.java @@ -57,14 +57,14 @@ final class FlushCaches extends SshCommand { protected void run() throws Failure { try { if (list) { - if (all || caches.size() > 0) { + if (all || !caches.isEmpty()) { throw die("cannot use --list with --all or --cache"); } doList(); return; } - if (all && caches.size() > 0) { + if (all && !caches.isEmpty()) { throw die("cannot combine --all and --cache"); } else if (!all && caches.size() == 1 && caches.contains("all")) { caches.clear(); diff --git a/java/com/google/gerrit/testing/BUILD b/java/com/google/gerrit/testing/BUILD index 5e2fbeaded..c610d073e2 100644 --- a/java/com/google/gerrit/testing/BUILD +++ b/java/com/google/gerrit/testing/BUILD @@ -44,6 +44,8 @@ java_library( "//lib/flogger:api", "//lib/guice", "//lib/guice:guice-servlet", + "//lib/log:impl-log4j", + "//lib/log:log4j", "//lib/truth", ], ) diff --git a/java/com/google/gerrit/testing/GerritTestName.java b/java/com/google/gerrit/testing/GerritTestName.java index d003289998..d287837430 100644 --- a/java/com/google/gerrit/testing/GerritTestName.java +++ b/java/com/google/gerrit/testing/GerritTestName.java @@ -15,6 +15,7 @@ package com.google.gerrit.testing; import com.google.common.base.CharMatcher; +import org.junit.BeforeClass; import org.junit.rules.TestName; import org.junit.rules.TestRule; import org.junit.runner.Description; @@ -23,6 +24,11 @@ import org.junit.runners.model.Statement; public class GerritTestName implements TestRule { private final TestName delegate = new TestName(); + @BeforeClass + public static void beforeClassTest() { + TestLoggingActivator.configureLogging(); + } + public String getSanitizedMethodName() { String name = delegate.getMethodName().toLowerCase(); name = diff --git a/java/com/google/gerrit/testing/TestLoggingActivator.java b/java/com/google/gerrit/testing/TestLoggingActivator.java new file mode 100644 index 0000000000..2510602264 --- /dev/null +++ b/java/com/google/gerrit/testing/TestLoggingActivator.java @@ -0,0 +1,94 @@ +// Copyright (C) 2020 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.testing; + +import static org.apache.log4j.Logger.getLogger; + +import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; +import org.apache.log4j.ConsoleAppender; +import org.apache.log4j.Level; +import org.apache.log4j.LogManager; +import org.apache.log4j.Logger; +import org.apache.log4j.PatternLayout; + +public class TestLoggingActivator { + private static final ImmutableMap LOG_LEVELS = + ImmutableMap.builder() + .put("com.google.gerrit", getGerritLogLevel()) + + // Silence non-critical messages from MINA SSHD. + .put("org.apache.mina", Level.WARN) + .put("org.apache.sshd.common", Level.WARN) + .put("org.apache.sshd.server", Level.WARN) + .put("org.apache.sshd.common.keyprovider.FileKeyPairProvider", Level.INFO) + .put("com.google.gerrit.sshd.GerritServerSession", Level.WARN) + + // Silence non-critical messages from mime-util. + .put("eu.medsea.mimeutil", Level.WARN) + + // Silence non-critical messages from openid4java. + .put("org.apache.xml", Level.WARN) + .put("org.openid4java", Level.WARN) + .put("org.openid4java.consumer.ConsumerManager", Level.FATAL) + .put("org.openid4java.discovery.Discovery", Level.ERROR) + .put("org.openid4java.server.RealmVerifier", Level.ERROR) + .put("org.openid4java.message.AuthSuccess", Level.ERROR) + + // Silence non-critical messages from c3p0 (if used). + .put("com.mchange.v2.c3p0", Level.WARN) + .put("com.mchange.v2.resourcepool", Level.WARN) + .put("com.mchange.v2.sql", Level.WARN) + + // Silence non-critical messages from apache.http. + .put("org.apache.http", Level.WARN) + + // Silence non-critical messages from Jetty. + .put("org.eclipse.jetty", Level.WARN) + + // Silence non-critical messages from JGit. + .put("org.eclipse.jgit.transport.PacketLineIn", Level.WARN) + .put("org.eclipse.jgit.transport.PacketLineOut", Level.WARN) + .put("org.eclipse.jgit.internal.storage.file.FileSnapshot", Level.WARN) + .put("org.eclipse.jgit.util.FS", Level.WARN) + .build(); + + private static Level getGerritLogLevel() { + String value = Strings.nullToEmpty(System.getenv("GERRIT_LOG_LEVEL")); + if (value.isEmpty()) { + value = Strings.nullToEmpty(System.getProperty("gerrit.logLevel")); + } + return Level.toLevel(value, Level.INFO); + } + + public static void configureLogging() { + LogManager.resetConfiguration(); + + PatternLayout layout = new PatternLayout(); + layout.setConversionPattern("%-5p %c %x: %m%n"); + + ConsoleAppender dst = new ConsoleAppender(); + dst.setLayout(layout); + dst.setTarget("System.err"); + dst.setThreshold(Level.DEBUG); + dst.activateOptions(); + + Logger root = LogManager.getRootLogger(); + root.removeAllAppenders(); + root.addAppender(dst); + + LOG_LEVELS.entrySet().stream().forEach(e -> getLogger(e.getKey()).setLevel(e.getValue())); + } +} diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java index b1194b14d0..7ac803eae9 100644 --- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -29,6 +29,7 @@ import com.google.common.collect.Lists; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Patch; @@ -80,6 +81,7 @@ import org.junit.Test; public class CommentsIT extends AbstractDaemonTest { @Inject private ChangeNoteUtil noteUtil; @Inject private FakeEmailSender email; + @Inject private ProjectOperations projectOperations; @Inject private Provider changes; @Inject private Provider postReview; @Inject private RequestScopeOperations requestScopeOperations; @@ -385,6 +387,39 @@ public class CommentsIT extends AbstractDaemonTest { assertThat(list.stream().map(infoToInput(file))).containsExactlyElementsIn(expectedComments); } + /** + * This test makes sure that the commits in the refs/draft-comments ref in NoteDb have no parent + * commits. This is important so that each new draft update (add, modify, delete) does not keep + * track of previous history. + */ + @Test + public void commitsInDraftCommentsRefHaveNoParent() throws Exception { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + String draftRefName = RefNames.refsDraftComments(r.getChange().getId(), user.id()); + + DraftInput comment1 = newDraft("file_1", Side.REVISION, 1, "comment 1"); + CommentInfo commentInfo1 = addDraft(changeId, revId, comment1); + assertThat(getHeadOfDraftCommentsRef(draftRefName).getParentCount()).isEqualTo(0); + + DraftInput comment2 = newDraft("file_2", Side.REVISION, 2, "comment 2"); + CommentInfo commentInfo2 = addDraft(changeId, revId, comment2); + assertThat(getHeadOfDraftCommentsRef(draftRefName).getParentCount()).isEqualTo(0); + + deleteDraft(changeId, revId, commentInfo1.id); + assertThat(getHeadOfDraftCommentsRef(draftRefName).getParentCount()).isEqualTo(0); + assertThat( + getDraftComments(changeId, revId).values().stream() + .flatMap(List::stream) + .map(commentInfo -> commentInfo.message)) + .containsExactly("comment 2"); + + deleteDraft(changeId, revId, commentInfo2.id); + assertThat(projectOperations.project(allUsers).hasHead(draftRefName)).isFalse(); + assertThat(getDraftComments(changeId, revId).values().stream().flatMap(List::stream)).isEmpty(); + } + @Test public void putDraft() throws Exception { for (Integer line : lines) { @@ -1110,6 +1145,12 @@ public class CommentsIT extends AbstractDaemonTest { } } + private RevCommit getHeadOfDraftCommentsRef(String refName) throws Exception { + try (Repository repo = repoManager.openRepository(allUsers)) { + return getHead(repo, refName); + } + } + private static String extractComments(String msg) { // Extract lines between start "....." and end "-- ". Pattern p = Pattern.compile(".*[.]{5}\n+(.*)\\n+-- \n.*", Pattern.DOTALL); diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java index ae92beab86..2dc1e246c7 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java @@ -104,7 +104,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("abandon", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ABANDONED_CHANGES) .noOneElse(); @@ -119,7 +119,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("abandon", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ABANDONED_CHANGES) .noOneElse(); @@ -135,7 +135,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("abandon", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ABANDONED_CHANGES) .noOneElse(); @@ -151,7 +151,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("abandon", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, other) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ABANDONED_CHANGES) .noOneElse(); @@ -165,7 +165,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("abandon", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -217,7 +217,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("abandon", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ABANDONED_CHANGES) .noOneElse(); @@ -238,7 +238,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("abandon", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ABANDONED_CHANGES) .noOneElse(); @@ -284,7 +284,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(reviewer) .cc(sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -308,7 +308,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(reviewer) .cc(sc.owner, sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -333,7 +333,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(reviewer) .cc(sc.owner, sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -358,7 +358,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(reviewer) .cc(sc.owner, sc.reviewer, other) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -382,7 +382,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(email) .cc(sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -438,7 +438,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(reviewer) .cc(sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); } @@ -451,7 +451,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(reviewer) .cc(sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -475,7 +475,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(reviewer) .cc(sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -533,7 +533,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to("nonexistent@example.com") .cc(sc.reviewer) - .cc(sc.ccerByEmail, sc.reviewerByEmail) + .cc(StagedUsers.CC_BY_EMAIL, StagedUsers.REVIEWER_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -555,7 +555,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .cc("nonexistent@example.com") .cc(sc.reviewer) - .cc(sc.ccerByEmail, sc.reviewerByEmail) + .cc(StagedUsers.CC_BY_EMAIL, StagedUsers.REVIEWER_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -641,7 +641,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -656,7 +656,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -671,7 +671,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -686,7 +686,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -702,7 +702,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -718,7 +718,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, other) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -732,7 +732,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -776,7 +776,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -802,7 +802,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -836,7 +836,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -850,7 +850,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -873,7 +873,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -888,7 +888,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -903,7 +903,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer, other) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -912,7 +912,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newchange", sc) .to(other) .cc(sc.reviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1082,7 +1082,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteReviewer", sc) .to(extraReviewer) .cc(extraCcer, sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1098,7 +1098,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteReviewer", sc) .to(sc.owner, extraReviewer) .cc(extraCcer, sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1114,7 +1114,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteReviewer", sc) .to(sc.owner, extraReviewer) .cc(extraCcer, sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1131,7 +1131,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteReviewer", sc) .to(sc.owner, extraReviewer) .cc(admin, extraCcer, sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1147,7 +1147,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteReviewer", sc) .to(extraCcer) .cc(sc.reviewer, sc.ccer, extraReviewer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1163,7 +1163,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteReviewer", sc) .to(extraReviewer) .cc(extraCcer, sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1222,7 +1222,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteReviewer", sc) .to(extraReviewer) .cc(extraCcer, sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1250,7 +1250,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { @Test public void deleteReviewerByEmailFromWipChange() throws Exception { StagedChange sc = stageWipChangeWithExtraReviewer(); - gApi.changes().id(sc.changeId).reviewer(sc.reviewerByEmail).remove(); + gApi.changes().id(sc.changeId).reviewer(StagedUsers.REVIEWER_BY_EMAIL).remove(); assertThat(sender).didNotSend(); } @@ -1317,7 +1317,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("deleteVote", sc) .cc(sc.reviewer, sc.ccer, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1335,7 +1335,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteVote", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1352,7 +1352,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteVote", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1370,7 +1370,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteVote", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, admin, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1386,7 +1386,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("deleteVote", sc) .cc(sc.reviewer, sc.ccer, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1402,7 +1402,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("deleteVote", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1445,7 +1445,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("deleteVote", sc) .cc(sc.reviewer, sc.ccer, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1461,7 +1461,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("deleteVote", sc) .cc(sc.reviewer, sc.ccer, extraReviewer, extraCcer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -1524,7 +1524,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .that(sender) .sent("merged", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS, SUBMITTED_CHANGES) .noOneElse(); @@ -1540,7 +1540,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("merged", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS, SUBMITTED_CHANGES) .noOneElse(); @@ -1555,7 +1555,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("merged", sc) .to(sc.owner) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS, SUBMITTED_CHANGES) .noOneElse(); @@ -1570,7 +1570,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("merged", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS, SUBMITTED_CHANGES) .noOneElse(); @@ -1585,7 +1585,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("merged", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1667,7 +1667,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1683,7 +1683,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner. .to(sc.reviewer, other) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1699,7 +1699,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .notTo(sc.owner) // TODO(logan): This shouldn't be sent *from* the owner. .to(sc.reviewer, other) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1716,7 +1716,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .to(sc.reviewer) .to(other) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1732,7 +1732,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .to(sc.reviewer) .to(other) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1791,7 +1791,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1806,7 +1806,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1829,7 +1829,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer, newReviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1853,7 +1853,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer, newReviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1868,7 +1868,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1893,7 +1893,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1908,7 +1908,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.owner, sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1923,7 +1923,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.owner, sc.reviewer, other) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -1938,7 +1938,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.owner, sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -1952,7 +1952,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.owner, sc.reviewer) .cc(sc.ccer, other) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .noOneElse(); assertThat(sender).didNotSend(); } @@ -2018,7 +2018,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("newpatchset", sc) .to(sc.reviewer) .cc(sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(NEW_PATCHSETS) .noOneElse(); @@ -2061,7 +2061,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("restore", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2075,7 +2075,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("restore", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2089,7 +2089,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("restore", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2104,7 +2104,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("restore", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2119,7 +2119,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("restore", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2134,7 +2134,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("restore", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, admin) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2173,7 +2173,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("revert", sc) .cc(sc.reviewer, sc.ccer, admin) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2198,7 +2198,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("revert", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, admin) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2223,7 +2223,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("revert", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer, admin) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2248,7 +2248,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("revert", sc) .to(sc.owner) .cc(other, sc.reviewer, sc.ccer, admin) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2285,7 +2285,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .to(sc.assignee) .noOneElse(); assertThat(sender).didNotSend(); @@ -2298,7 +2300,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("setassignee", sc) .cc(sc.owner) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .to(sc.assignee) .noOneElse(); assertThat(sender).didNotSend(); @@ -2310,7 +2314,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assign(sc, admin, sc.assignee); assertThat(sender) .sent("setassignee", sc) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .to(sc.assignee) .noOneElse(); assertThat(sender).didNotSend(); @@ -2323,7 +2329,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("setassignee", sc) .cc(admin) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .to(sc.assignee) .noOneElse(); assertThat(sender).didNotSend(); @@ -2335,7 +2343,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assign(sc, sc.owner, sc.owner); assertThat(sender) .sent("setassignee", sc) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .noOneElse(); assertThat(sender).didNotSend(); } @@ -2349,7 +2359,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .to(sc.assignee) .noOneElse(); assertThat(sender).didNotSend(); @@ -2363,7 +2375,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assign(sc, sc.owner, sc.owner); assertThat(sender) .sent("setassignee", sc) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .noOneElse(); assertThat(sender).didNotSend(); } @@ -2374,7 +2388,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .to(sc.assignee) .noOneElse(); assertThat(sender).didNotSend(); @@ -2386,7 +2402,9 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assign(sc, sc.owner, sc.assignee); assertThat(sender) .sent("setassignee", sc) - .cc(sc.reviewerByEmail, sc.ccerByEmail) // TODO(logan): This is probably not intended! + .cc( + StagedUsers.REVIEWER_BY_EMAIL, + StagedUsers.CC_BY_EMAIL) // TODO(logan): This is probably not intended! .to(sc.assignee) .noOneElse(); assertThat(sender).didNotSend(); @@ -2416,7 +2434,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { assertThat(sender) .sent("comment", sc) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); @@ -2432,7 +2450,7 @@ public class ChangeNotificationsIT extends AbstractNotificationTest { .sent("comment", sc) .to(sc.owner) .cc(sc.reviewer, sc.ccer) - .cc(sc.reviewerByEmail, sc.ccerByEmail) + .cc(StagedUsers.REVIEWER_BY_EMAIL, StagedUsers.CC_BY_EMAIL) .bcc(sc.starrer) .bcc(ALL_COMMENTS) .noOneElse(); diff --git a/javatests/com/google/gerrit/pgm/BUILD b/javatests/com/google/gerrit/pgm/BUILD index 49371f4e43..14457070e5 100644 --- a/javatests/com/google/gerrit/pgm/BUILD +++ b/javatests/com/google/gerrit/pgm/BUILD @@ -6,7 +6,6 @@ junit_tests( srcs = glob(["**/*.java"]), deps = [ "//java/com/google/gerrit/pgm", - "//java/com/google/gerrit/pgm/http/jetty", "//java/com/google/gerrit/pgm/init", "//java/com/google/gerrit/pgm/init/api", "//java/com/google/gerrit/server", diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index 31d20480c5..e7f0812c24 100644 --- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java @@ -91,14 +91,10 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Optional; -import org.apache.log4j.Level; -import org.apache.log4j.LogManager; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.junit.After; -import org.junit.AfterClass; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; @@ -150,20 +146,6 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { protected abstract Injector createInjector(); - @BeforeClass - public static void setLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.DEBUG); - } - } - - @AfterClass - public static void resetLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.INFO); - } - } - @Before public void setUpInjector() throws Exception { lifecycle = new LifecycleManager(); diff --git a/javatests/com/google/gerrit/server/query/account/BUILD b/javatests/com/google/gerrit/server/query/account/BUILD index da37f26cfb..5c910a04f3 100644 --- a/javatests/com/google/gerrit/server/query/account/BUILD +++ b/javatests/com/google/gerrit/server/query/account/BUILD @@ -23,10 +23,8 @@ java_library( "//lib:guava", "//lib:jgit", "//lib/guice", - "//lib/log:log4j", "//lib/truth", "//lib/truth:truth-java8-extension", - "//resources:log4j-config", ], ) diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index aa305b292d..32104b76f0 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -130,8 +130,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; -import org.apache.log4j.Level; -import org.apache.log4j.LogManager; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; @@ -143,9 +141,7 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.util.SystemReader; import org.junit.After; -import org.junit.AfterClass; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Ignore; import org.junit.Test; @@ -208,20 +204,6 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { protected abstract Injector createInjector(); - @BeforeClass - public static void setLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.DEBUG); - } - } - - @AfterClass - public static void resetLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.INFO); - } - } - @Before public void setUpInjector() throws Exception { lifecycle = new LifecycleManager(); @@ -443,7 +425,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { gApi.changes().id(change1.getChangeId()).setPrivate(true, null); - // Change1 is not private, but should be still visible to its owner. + // Change1 is private, but should be still visible to its owner. assertQuery("is:open", change1, change2); assertQuery("is:private", change1); @@ -941,11 +923,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { public void byLabel() throws Exception { accountManager.authenticate(AuthRequest.forUser("anotheruser")); TestRepository repo = createProject("repo"); - ChangeInserter ins = newChange(repo, null, null, null, null, false); - ChangeInserter ins2 = newChange(repo, null, null, null, null, false); - ChangeInserter ins3 = newChange(repo, null, null, null, null, false); - ChangeInserter ins4 = newChange(repo, null, null, null, null, false); - ChangeInserter ins5 = newChange(repo, null, null, null, null, false); + ChangeInserter ins = newChange(repo); + ChangeInserter ins2 = newChange(repo); + ChangeInserter ins3 = newChange(repo); + ChangeInserter ins4 = newChange(repo); + ChangeInserter ins5 = newChange(repo); Change reviewMinus2Change = insert(repo, ins); gApi.changes().id(reviewMinus2Change.getId().get()).current().review(ReviewInput.reject()); @@ -1046,11 +1028,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { .update(); ReviewInput reviewVerified = new ReviewInput().label("Verified", 1); - ChangeInserter ins = newChange(repo, null, null, null, null, false); - ChangeInserter ins2 = newChange(repo, null, null, null, null, false); - ChangeInserter ins3 = newChange(repo, null, null, null, null, false); - ChangeInserter ins4 = newChange(repo, null, null, null, null, false); - ChangeInserter ins5 = newChange(repo, null, null, null, null, false); + ChangeInserter ins = newChange(repo); + ChangeInserter ins2 = newChange(repo); + ChangeInserter ins3 = newChange(repo); + ChangeInserter ins4 = newChange(repo); + ChangeInserter ins5 = newChange(repo); // CR+1 Change reviewCRplus1 = insert(repo, ins); @@ -1091,7 +1073,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { @Test public void byLabelNotOwner() throws Exception { TestRepository repo = createProject("repo"); - ChangeInserter ins = newChange(repo, null, null, null, null, false); + ChangeInserter ins = newChange(repo); Account.Id user1 = createAccount("user1"); Change reviewPlus1Change = insert(repo, ins); @@ -1788,25 +1770,27 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { public void visible() throws Exception { TestRepository repo = createProject("repo"); Change change1 = insert(repo, newChange(repo)); - Change change2 = insert(repo, newChange(repo)); - - gApi.changes().id(change2.getChangeId()).setPrivate(true, "private"); + Change change2 = insert(repo, newChangePrivate(repo)); String q = "project:repo"; - assertQuery(q, change2, change1); - // Second user cannot see first user's private change. - Account.Id user2 = - accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId(); + // Bad request for query with non-existent user + assertThatQueryException(q + " visibleto:notexisting"); + + // Current user can see all changes + assertQuery(q, change2, change1); + assertQuery(q + " visibleto:self", change2, change1); + + // Second user cannot see first user's private change + Account.Id user2 = createAccount("anotheruser"); assertQuery(q + " visibleto:" + user2.get(), change1); + assertQuery(q + " visibleto:anotheruser", change1); String g1 = createGroup("group1", "Administrators"); gApi.groups().id(g1).addMembers("anotheruser"); assertQuery(q + " visibleto:" + g1, change1); - requestContext.setContext( - newRequestContext( - accountManager.authenticate(AuthRequest.forUser("anotheruser")).getAccountId())); + requestContext.setContext(newRequestContext(user2)); assertQuery("is:visible", change1); } @@ -3085,12 +3069,12 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { } protected ChangeInserter newChange(TestRepository repo) throws Exception { - return newChange(repo, null, null, null, null, false); + return newChange(repo, null, null, null, null, false, false); } protected ChangeInserter newChangeForCommit(TestRepository repo, RevCommit commit) throws Exception { - return newChange(repo, commit, null, null, null, false); + return newChange(repo, commit, null, null, null, false, false); } protected ChangeInserter newChangeWithFiles(TestRepository repo, String... paths) @@ -3104,21 +3088,25 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { protected ChangeInserter newChangeForBranch(TestRepository repo, String branch) throws Exception { - return newChange(repo, null, branch, null, null, false); + return newChange(repo, null, branch, null, null, false, false); } protected ChangeInserter newChangeWithStatus(TestRepository repo, Change.Status status) throws Exception { - return newChange(repo, null, null, status, null, false); + return newChange(repo, null, null, status, null, false, false); } protected ChangeInserter newChangeWithTopic(TestRepository repo, String topic) throws Exception { - return newChange(repo, null, null, null, topic, false); + return newChange(repo, null, null, null, topic, false, false); } protected ChangeInserter newChangeWorkInProgress(TestRepository repo) throws Exception { - return newChange(repo, null, null, null, null, true); + return newChange(repo, null, null, null, null, true, false); + } + + protected ChangeInserter newChangePrivate(TestRepository repo) throws Exception { + return newChange(repo, null, null, null, null, false, true); } protected ChangeInserter newChange( @@ -3127,7 +3115,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { @Nullable String branch, @Nullable Change.Status status, @Nullable String topic, - boolean workInProgress) + boolean workInProgress, + boolean isPrivate) throws Exception { if (commit == null) { commit = repo.parseBody(repo.commit().message("message").create()); @@ -3145,7 +3134,8 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { .setValidate(false) .setStatus(status) .setTopic(topic) - .setWorkInProgress(workInProgress); + .setWorkInProgress(workInProgress) + .setPrivate(isPrivate); return ins; } diff --git a/javatests/com/google/gerrit/server/query/change/BUILD b/javatests/com/google/gerrit/server/query/change/BUILD index c122a9e356..d0162d3810 100644 --- a/javatests/com/google/gerrit/server/query/change/BUILD +++ b/javatests/com/google/gerrit/server/query/change/BUILD @@ -32,9 +32,7 @@ java_library( "//lib:jgit", "//lib:jgit-junit", "//lib/guice", - "//lib/log:log4j", "//lib/truth", - "//resources:log4j-config", ], ) diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index 9b01f8d15e..d80eac0c47 100644 --- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java @@ -68,12 +68,8 @@ import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Optional; -import org.apache.log4j.Level; -import org.apache.log4j.LogManager; import org.junit.After; -import org.junit.AfterClass; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; @@ -119,20 +115,6 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests { protected abstract Injector createInjector(); - @BeforeClass - public static void setLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.DEBUG); - } - } - - @AfterClass - public static void resetLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.INFO); - } - } - @Before public void setUpInjector() throws Exception { lifecycle = new LifecycleManager(); diff --git a/javatests/com/google/gerrit/server/query/group/BUILD b/javatests/com/google/gerrit/server/query/group/BUILD index 8f6fd6dc0d..e14350f093 100644 --- a/javatests/com/google/gerrit/server/query/group/BUILD +++ b/javatests/com/google/gerrit/server/query/group/BUILD @@ -20,10 +20,8 @@ java_library( "//lib:guava", "//lib:jgit", "//lib/guice", - "//lib/log:log4j", "//lib/truth", "//lib/truth:truth-java8-extension", - "//resources:log4j-config", ], ) diff --git a/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java b/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java index e142e0c7bf..dfd7928bf0 100644 --- a/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java +++ b/javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java @@ -68,12 +68,8 @@ import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Set; -import org.apache.log4j.Level; -import org.apache.log4j.LogManager; import org.junit.After; -import org.junit.AfterClass; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; @@ -115,20 +111,6 @@ public abstract class AbstractQueryProjectsTest extends GerritServerTests { protected abstract Injector createInjector(); - @BeforeClass - public static void setLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.DEBUG); - } - } - - @AfterClass - public static void resetLogLevel() { - if (Boolean.getBoolean("debug")) { - LogManager.getRootLogger().setLevel(Level.INFO); - } - } - @Before public void setUpInjector() throws Exception { lifecycle = new LifecycleManager(); diff --git a/javatests/com/google/gerrit/server/query/project/BUILD b/javatests/com/google/gerrit/server/query/project/BUILD index 996be2ff20..984d82470f 100644 --- a/javatests/com/google/gerrit/server/query/project/BUILD +++ b/javatests/com/google/gerrit/server/query/project/BUILD @@ -21,9 +21,7 @@ java_library( "//lib:guava", "//lib:jgit", "//lib/guice", - "//lib/log:log4j", "//lib/truth", - "//resources:log4j-config", ], ) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html index 40a02a3156..7f97fec3f0 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html @@ -137,11 +137,13 @@ limitations under the License. align-items: center; display: flex; } - .diffModeSelector { + .diffModeSelector, + .editButton { align-items: center; display: flex; } - .diffModeSelector span { + .diffModeSelector span, + .editButton span { margin-right: var(--spacing-xs); } .diffModeSelector.hide, @@ -153,6 +155,9 @@ limitations under the License. text-transform: none; } } + .editButtona a { + text-decoration: none; + } @media screen and (max-width: 50em) { header { padding: var(--spacing-s) var(--spacing-l); @@ -284,6 +289,14 @@ limitations under the License. on-click="_toggleBlame">[[_computeBlameToggleLabel(_isBlameLoaded, _isBlameLoading)]] +
Diff view: arg === undefined)) { + return ''; + } + return Gerrit.Nav.getEditUrlForDiff( + change, path, patchRange.patchNum); + }, + /** * Gives an object representing the target of navigating either left or * right through the change. The resulting object will have one of the @@ -1171,5 +1179,9 @@ _handleReloadingDiffPreference() { this._getDiffPreferences(); }, + + _computeIsLoggedIn(loggedIn) { + return loggedIn ? true : false; + }, }); })(); diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js index 75c02019e4..ee41103ae9 100644 --- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js +++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.js @@ -85,13 +85,34 @@ if (newContent.length) { this.$.storage.setEditableContentItem(this.storageKey, newContent); } else { + // This does not really happen, because we don't clear newContent + // after saving (see below). So this only occurs when the user clears + // all the content in the editable textarea. But cleans + // up itself after one day, so we are not so concerned about leaving + // some garbage behind. this.$.storage.eraseEditableContentItem(this.storageKey); } }, STORAGE_DEBOUNCE_INTERVAL_MS); }, _editingChanged(editing) { - if (!editing) { return; } + // This method is for initializing _newContent when you start editing. + // Restoring content from local storage is not perfect and has + // some issues: + // + // 1. When you start editing in multiple tabs, then we are vulnerable to + // race conditions between the tabs. + // 2. The stored content is keyed by revision, so when you upload a new + // patchset and click "reload" and then click "cancel" on the content- + // editable, then you won't be able to recover the content anymore. + // + // Because of these issues we believe that it is better to only recover + // content from local storage when you enter editing mode for the first + // time. Otherwise it is better to just keep the last editing state from + // the same session. + if (!editing || this._newContent) { + return; + } let content; if (this.storageKey) { @@ -126,12 +147,15 @@ return true; } - return disabled || (content === newContent); + return disabled || !newContent || content === newContent; }, _handleSave(e) { e.preventDefault(); this.fire('editable-content-save', {content: this._newContent}); + // It would be nice, if we would set this._newContent = undefined here, + // but we can only do that when we are sure that the save operation has + // succeeded. }, _handleCancel(e) { diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html index 24db69f7d7..ee5adb0c6b 100644 --- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html +++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.html @@ -65,11 +65,11 @@ limitations under the License. MockInteractions.tap(element.$$('gr-button:not([primary])')); }); - test('enabling editing updates edit field contents', () => { + test('enabling editing keeps old content', () => { element.content = 'current content'; - element._newContent = 'stale content'; + element._newContent = 'old content'; element.editing = true; - assert.equal(element._newContent, 'current content'); + assert.equal(element._newContent, 'old content'); }); test('disabling editing does not update edit field contents', () => { diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js index 6eb9f4680a..48a615a25f 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js +++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js @@ -69,10 +69,16 @@ this._handleParseResult.bind(this), this.removeZeroWidthSpace); parser.parse(content); - // Ensure that links originating from HTML commentlink configs open in a - // new tab. @see Issue 5567 + // Ensure that external links originating from HTML commentlink configs + // open in a new tab. @see Issue 5567 + // Ensure links to the same host originating from commentlink configs + // open in the same tab. @see Issue 4616 output.querySelectorAll('a').forEach(anchor => { - anchor.setAttribute('target', '_blank'); + if (anchor.hostname === window.location.hostname) { + anchor.setAttribute('target', '_self'); + } else { + anchor.setAttribute('target', '_blank'); + } anchor.setAttribute('rel', 'noopener'); }); }, diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html index e60dd423b7..99f4f7ed2f 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html +++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html @@ -144,7 +144,7 @@ limitations under the License. const linkEl = element.$.output.childNodes[1]; assert.equal(textNode.textContent, prefix); const url = '/q/' + changeID; - assert.equal(linkEl.target, '_blank'); + assert.equal(linkEl.target, '_self'); // Since url is a path, the host is added automatically. assert.isTrue(linkEl.href.endsWith(url)); assert.equal(linkEl.textContent, changeID); @@ -162,7 +162,7 @@ limitations under the License. const linkEl = element.$.output.childNodes[1]; assert.equal(textNode.textContent, prefix); const url = '/r/q/' + changeID; - assert.equal(linkEl.target, '_blank'); + assert.equal(linkEl.target, '_self'); // Since url is a path, the host is added automatically. assert.isTrue(linkEl.href.endsWith(url)); assert.equal(linkEl.textContent, changeID); @@ -203,7 +203,7 @@ limitations under the License. assert.equal(textNode.textContent, prefix); - assert.equal(changeLinkEl.target, '_blank'); + assert.equal(changeLinkEl.target, '_self'); assert.isTrue(changeLinkEl.href.endsWith(changeUrl)); assert.equal(changeLinkEl.textContent, changeID); diff --git a/tools/bzl/plugin.bzl b/tools/bzl/plugin.bzl index ed64d1b775..3c6d597f7b 100644 --- a/tools/bzl/plugin.bzl +++ b/tools/bzl/plugin.bzl @@ -49,16 +49,21 @@ def gerrit_plugin( # TODO(davido): Remove manual merge of manifest file when this feature # request is implemented: https://github.com/bazelbuild/bazel/issues/2009 + # TODO(davido): Remove manual touch command when this issue is resolved: + # https://github.com/bazelbuild/bazel/issues/10789 genrule2( name = name + target_suffix, stamp = 1, srcs = ["%s__non_stamped_deploy.jar" % name], cmd = " && ".join([ + "TZ=UTC", + "export TZ", "GEN_VERSION=$$(cat bazel-out/stable-status.txt | grep -w STABLE_BUILD_%s_LABEL | cut -d ' ' -f 2)" % dir_name.upper(), "cd $$TMP", "unzip -q $$ROOT/$<", "echo \"Implementation-Version: $$GEN_VERSION\n$$(cat META-INF/MANIFEST.MF)\" > META-INF/MANIFEST.MF", - "zip -qr $$ROOT/$@ .", + "find . -exec touch '{}' ';'", + "zip -Xqr $$ROOT/$@ .", ]), outs = ["%s%s.jar" % (name, target_suffix)], visibility = ["//visibility:public"],