Merge branch 'stable-3.0' into stable-3.1

* stable-3.0:
  Open relative commentlinks in same tab
  VersionedMetaData: Emit debug log after update has been committed
  VersionedMetaData: Trim message sent to log
  ChangeIdHandler: Make assumption on number of query results explicit
  ChangeIdHandler: Remove unnecessary 'final' modifiers
  Update git submodules
  AbstractQueryChangesTest: Add method to create change as private
  AbstractQueryChangesTest: Fix comment in byPrivate
  AbstractQueryChangesTest: Use overloaded newChange method where possible
  AbstractQueryChangesTest: Extend testing of visibleto predicate
  ChangeQueryBuilder: Rename status_open to statusOpen
  ChangeQueryBuilder: Rename is_visible to isVisible
  ChangeNotificationsIT: Fix eclipse warning(s) about static use
  Extract duplicated code of {Ssh|Http}LogJsonLayout
  Add option to log SSH events in JSON format
  Remove duplicated constants storing key names of Ssh logs
  Add option to log HTTP logging events in JSON format
  CacheMetrics: Make F_NAME a final constant in class scope
  Fix more cases where Collection.isEmpty() should be used
  Use Set instead of EnumSet in method signatures
  Replace negation of anyMatch() with noneMatch()
  AllExternalIds: Replace lambdas with method references
  PostReview: Replace filter and findFirst with anyMatch
  XContentBuilder: Use UTF_8 from Java's StandardCharsets rather than Guava
  Update git submodules
  Update git submodules
  Update git submodules
  QueryDocumentationExecutor: Make WEIGHTS final and use ImmutableMap
  BooleanProjectConfigTransformations: Make MAPPER final
  TestSubmitRuleInfo: Make INSTANCE final
  TrackingFootersProvider: Make constants final
  ChangeCleanupConfig: Make constants final
  SetParent: Simplify lambda expression
  ProjectsConsistencyChecker: Simplify lambda expression
  Remove redundant null checks used with "instanceof"
  Remove unnecessary array creation for varargs parameters
  Remove redundant parentheses in lambda statements
  PreferencesBox: Remove unnecessary boxing to Boolean
  GitwebServlet: Rename bufferSize to BUFFER_SIZE and make static
  GetPatch: Make FILE_NOT_FOUND static
  DeleteZombieCommentsRefs: Make constants static
  AbstractNotificationTest: Make constants static and rename
  TabFile: Make TRIM final
  Remove redundant calls to toString()
  OpenIdServiceImpl: Fix incorrect null comparison of Optional
  Consistently use isEmpty to check for empty collection
  PageLinks: Reuse ADMIN_PROJECTS constant
  PageLinks: Fix typo in method name
  FlushCaches: Use isEmpty to check for empty list
  JGitMetricModule: Replace anonymous Supplier instances with method references
  Adapt to deprecation of WindowCacheStats methods in JGit
  Documentation: Fix collapsed words and broken link
  Use Flogger instead of System.out.println in DeleteZombieCommentsRefs
  Fix a dependency injection runtime error in DeleteZombieDrafts pgm
  Update git submodules
  Bazel: Move runtime dependencies only from deps to runtime_deps
  Update rename project documentation in project owner guide
  Fix issues with caching edited commit message
  Add "edit" button to diff view
  Remove egit plugin from project owner guide Documentation
  Error Prone: Enable and fix OperatorPrecedence
  Bazel: Remove superfluous dependencies flagged by unused_deps
  GerritBaseTests: Activate test logging
  Revert "Enable optional DEBUG level logs for query tests"
  Allow to control the Gerrit log level for running tests from system var
  Revert "Acceptance: set log threshold level for tests"
  Reduce log level for tests to INFO
  Bazel: Update time attribute of file entries in plugin artifact
  Add Jetty connection metrics
  Add additional JGit WindowCache metrics
  Update JGit to 5.1.13.202002110435-r
  BucketedCallback: fix prune() to remove unset sub-metrics from registry
  Modify draft ref updates commits to point to an empty parent

Change-Id: I0039bf12f018706b400f905491a515e2e957b3ec

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.
 

Documentation/dev-bazel.txt

@@ -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]

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]

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
 

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<NotifyType, TestAccount> watchers = new HashMap<>();
     private final Map<String, TestAccount> 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)) {

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",

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<? extends Annotation> 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<String, Level> LOG_LEVELS =
-      ImmutableMap.<String, Level>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 + "/";

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) {

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();
       }

java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java

@@ -392,7 +392,7 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
 
     @Override
     public ResultSet<V> read() {
-      return readImpl((doc) -> AbstractElasticIndex.this.fromDocument(doc, opts.fields()));
+      return readImpl(doc -> AbstractElasticIndex.this.fromDocument(doc, opts.fields()));
     }
 
     @Override

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 {

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<ListAccountsOption> options = EnumSet.noneOf(ListAccountsOption.class);
+    private Set<ListAccountsOption> options = EnumSet.noneOf(ListAccountsOption.class);
 
     /** Execute query and return a list of accounts. */
     public abstract List<AccountInfo> get() throws RestApiException;
@@ -185,7 +186,7 @@ public interface Accounts {
     }
 
     /** Set options on the request, replacing existing options. */
-    public QueryRequest withOptions(EnumSet<ListAccountsOption> options) {
+    public QueryRequest withOptions(Set<ListAccountsOption> options) {
       this.options = options;
       return this;
     }
@@ -206,7 +207,7 @@ public interface Accounts {
       return suggest;
     }
 
-    public EnumSet<ListAccountsOption> getOptions() {
+    public Set<ListAccountsOption> getOptions() {
       return options;
     }
   }

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<ChangeEditDetailOption> options() {
+    public Set<ChangeEditDetailOption> options() {
       return options;
     }
   }

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<ListChangesOption> options = EnumSet.noneOf(ListChangesOption.class);
+    private Set<ListChangesOption> options = EnumSet.noneOf(ListChangesOption.class);
     private ListMultimap<String, String> pluginOptions = ArrayListMultimap.create();
 
     public abstract List<ChangeInfo> get() throws RestApiException;
@@ -116,7 +117,7 @@ public interface Changes {
     }
 
     /** Set options on the request, replacing existing options. */
-    public QueryRequest withOptions(EnumSet<ListChangesOption> options) {
+    public QueryRequest withOptions(Set<ListChangesOption> options) {
       this.options = options;
       return this;
     }
@@ -149,7 +150,7 @@ public interface Changes {
       return start;
     }
 
-    public EnumSet<ListChangesOption> getOptions() {
+    public Set<ListChangesOption> getOptions() {
       return options;
     }
 

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<ListGroupsOption> getOptions() {
+    public Set<ListGroupsOption> getOptions() {
       return options;
     }
 
@@ -224,7 +225,7 @@ public interface Groups {
     private String query;
     private int limit;
     private int start;
-    private EnumSet<ListGroupsOption> options = EnumSet.noneOf(ListGroupsOption.class);
+    private Set<ListGroupsOption> options = EnumSet.noneOf(ListGroupsOption.class);
 
     /** Execute query and returns the matched groups as list. */
     public abstract List<GroupInfo> get() throws RestApiException;
@@ -266,7 +267,7 @@ public interface Groups {
     }
 
     /** Set options on the request, replacing existing options. */
-    public QueryRequest withOptions(EnumSet<ListGroupsOption> options) {
+    public QueryRequest withOptions(Set<ListGroupsOption> options) {
       this.options = options;
       return this;
     }
@@ -283,7 +284,7 @@ public interface Groups {
       return start;
     }
 
-    public EnumSet<ListGroupsOption> getOptions() {
+    public Set<ListGroupsOption> getOptions() {
       return options;
     }
   }

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

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.

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<String> 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);

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;

java/com/google/gerrit/lucene/LuceneChangeIndex.java

@@ -577,7 +577,7 @@ public class LuceneChangeIndex implements ChangeIndex {
 
   private void decodeReviewedBy(ListMultimap<String, IndexableField> doc, ChangeData cd) {
     Collection<IndexableField> reviewedBy = doc.get(REVIEWEDBY_FIELD);
-    if (reviewedBy.size() > 0) {
+    if (!reviewedBy.isEmpty()) {
       Set<Account.Id> accounts = Sets.newHashSetWithExpectedSize(reviewedBy.size());
       for (IndexableField r : reviewedBy) {
         int id = r.numericValue().intValue();

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()));
     }

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",

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<V> implements BucketedMetric {
   }
 
   void doPrune() {
-    cells.entrySet().removeIf(objectValueGaugeEntry -> !objectValueGaugeEntry.getValue().set);
+    Set<Map.Entry<Object, BucketedCallback<V>.ValueGauge>> entries = cells.entrySet();
+    for (Map.Entry<Object, ValueGauge> e : entries) {
+      if (!e.getValue().set) {
+        entries.remove(e);
+        registry.remove(submetric(e.getKey()));
+      }
+    }
   }
 
   void doEndSet() {

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<String, Long> 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<String, Long> cacheMap = WindowCacheStats.getStats().getOpenByteCountPerRepository();
+          if (cacheMap.isEmpty()) {
+            repoEnt.forceCreate("");
+          } else {
+            cacheMap.entrySet().stream()
+                .sorted(Map.Entry.<String, Long>comparingByValue().reversed())
+                .limit(MAX_REPO_COUNT)
+                .forEach(e -> repoEnt.set(e.getKey(), e.getValue()));
+            repoEnt.prune();
+          }
+        });
   }
 }

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",

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));
           }
         });

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",

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

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);
+    }
+  }
+}

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<Integer> busyThreads =
         metrics.newCallbackMetric(
             "http/server/jetty/threadpool/active_threads",
             Integer.class,
-            new Description("Active threads").setGauge().setUnit("threads"));
+            new Description("Active threads").setGauge());
     CallbackMetric0<Integer> reservedThreads =
         metrics.newCallbackMetric(
             "http/server/jetty/threadpool/reserved_threads",
             Integer.class,
-            new Description("Reserved threads").setGauge().setUnit("threads"));
+            new Description("Reserved threads").setGauge());
     CallbackMetric0<Integer> 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<Boolean> lowOnThreads =
         metrics.newCallbackMetric(
             "http/server/jetty/threadpool/is_low_on_threads",
             Boolean.class,
             new Description("Whether thread pool is low on threads").setGauge());
+    CallbackMetric0<Long> connections =
+        metrics.newCallbackMetric(
+            "http/server/jetty/connections/connections",
+            Long.class,
+            new Description("The current number of open connections").setGauge());
+    CallbackMetric0<Long> connectionsTotal =
+        metrics.newCallbackMetric(
+            "http/server/jetty/connections/connections_total",
+            Long.class,
+            new Description("The total number of connections opened").setGauge());
+    CallbackMetric0<Long> 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<Double> 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<Double> 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<Long> receivedMessages =
+        metrics.newCallbackMetric(
+            "http/server/jetty/connections/received_messages",
+            Long.class,
+            new Description("The total number of messages received").setGauge());
+    CallbackMetric0<Long> sentMessages =
+        metrics.newCallbackMetric(
+            "http/server/jetty/connections/sent_messages",
+            Long.class,
+            new Description("The total number of messages sent").setGauge());
+    CallbackMetric0<Long> 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<Long> 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.<CallbackMetric<?>>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());
         });
   }
 }

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)) {

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);
     }

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);
               }
             });

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'",

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<Change.Id> {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   private final Provider<InternalChangeQuery> queryProvider;
 
   @Inject
   public ChangeIdHandler(
       // TODO(dborowitz): Not sure whether this is injectable here.
       Provider<InternalChangeQuery> queryProvider,
-      @Assisted final CmdLineParser parser,
-      @Assisted final OptionDef option,
-      @Assisted final Setter<Change.Id> setter) {
+      @Assisted CmdLineParser parser,
+      @Assisted OptionDef option,
+      @Assisted Setter<Change.Id> 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<String> tokens = Splitter.on(',').splitToList(token);
+    String token = params.getParameter(0);
+    List<String> tokens = Splitter.on(',').splitToList(token);
     if (tokens.size() != 3) {
       throw new CmdLineException(
           owner, localizable("change should be specified as <project>,<branch>,<change-id>"));
     }
 
     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<ChangeData> 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) {

java/com/google/gerrit/server/cache/CacheMetrics.java

@@ -32,10 +32,11 @@ import java.util.Set;
 
 @Singleton
 public class CacheMetrics {
+  private static final Field<String> F_NAME =
+      Field.ofString("cache_name", Metadata.Builder::cacheName).build();
+
   @Inject
   public CacheMetrics(MetricMaker metrics, DynamicMap<Cache<?, ?>> cacheMap) {
-    Field<String> F_NAME = Field.ofString("cache_name", Metadata.Builder::cacheName).build();
-
     CallbackMetric1<String, Long> memEnt =
         metrics.newCallbackMetric(
             "caches/memory_cached",

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"

java/com/google/gerrit/server/config/TrackingFootersProvider.java

@@ -34,10 +34,10 @@ public class TrackingFootersProvider implements Provider<TrackingFooters> {
 
   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<TrackingFooter> trackingFooters = new ArrayList<>();
 
   @Inject

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<String, Float> WEIGHTS =
+  private static final ImmutableMap<String, Float> WEIGHTS =
       ImmutableMap.of(
           Constants.TITLE_FIELD, 2.0f,
           Constants.DOC_FIELD, 1.0f);

java/com/google/gerrit/server/events/StreamEventsApiListener.java

@@ -230,7 +230,7 @@ public class StreamEventsApiListener
   }
 
   String[] hashtagArray(Collection<String> hashtags) {
-    if (hashtags != null && hashtags.size() > 0) {
+    if (hashtags != null && !hashtags.isEmpty()) {
       return Sets.newHashSet(hashtags).toArray(new String[hashtags.size()]);
     }
     return null;

java/com/google/gerrit/server/git/GroupCollector.java

@@ -141,7 +141,7 @@ public class GroupCollector {
     checkState(!done, "visit() called after getGroups()");
     Set<RevCommit> 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());

java/com/google/gerrit/server/git/NotifyConfig.java

@@ -50,11 +50,11 @@ public class NotifyConfig implements Comparable<NotifyConfig> {
     return types.contains(type) || types.contains(NotifyType.ALL);
   }
 
-  public EnumSet<NotifyType> getNotify() {
+  public Set<NotifyType> getNotify() {
     return types;
   }
 
-  public void setTypes(EnumSet<NotifyType> newTypes) {
+  public void setTypes(Set<NotifyType> newTypes) {
     types = EnumSet.copyOf(newTypes);
   }
 

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;

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);

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();

java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java

@@ -78,7 +78,7 @@ public class ChangeIndexRewriter implements IndexRewriter<ChangeData> {
    * @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<Change.Status> getPossibleStatus(Predicate<ChangeData> in) {
+  public static Set<Change.Status> getPossibleStatus(Predicate<ChangeData> in) {
     EnumSet<Change.Status> s = extractStatus(in);
     return s != null ? s : EnumSet.allOf(Change.Status.class);
   }

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",
     ],
 )

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;
+  }
+}

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);
+  }
 }

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) {

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.

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();

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<Ref> draftRefs = allUsersRepo.getRefDatabase().getRefsByPrefix(DRAFT_REFS_PREFIX);
     List<Ref> 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);
   }
 }

java/com/google/gerrit/server/permissions/PermissionBackend.java

@@ -501,12 +501,12 @@ public abstract class PermissionBackend {
     public Set<LabelPermission.WithValue> testLabels(Collection<LabelType> 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<LabelPermission.WithValue> valuesOf(LabelType label) {
       return label.getValues().stream()
-          .map((v) -> new LabelPermission.WithValue(label, v))
+          .map(v -> new LabelPermission.WithValue(label, v))
           .collect(toSet());
     }
   }

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<BooleanProjectConfig, Mapper> MAPPER =
+  private static final ImmutableMap<BooleanProjectConfig, Mapper> MAPPER =
       ImmutableMap.<BooleanProjectConfig, Mapper>builder()
           .put(
               BooleanProjectConfig.USE_CONTRIBUTOR_AGREEMENTS,

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<ChangeData> queryResult =
           retryHelper.execute(
               ActionType.INDEX_QUERY,
-              () -> {
+              () ->
                   // Execute the query.
-                return changeQueryProvider
+                  changeQueryProvider
                       .get()
                       .setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET)
-                    .query(and(basePredicate, or(predicates)));
-              },
+                      .query(and(basePredicate, or(predicates))),
               StorageException.class::isInstance);
 
       // Result for this query that we want to return to the client.

java/com/google/gerrit/server/query/account/InternalAccountQuery.java

@@ -71,7 +71,7 @@ public class InternalAccountQuery extends InternalQuery<AccountState, InternalAc
     List<AccountState> 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(", ")

java/com/google/gerrit/server/query/change/AndChangeSource.java

@@ -35,9 +35,7 @@ public class AndChangeSource extends AndSource<ChangeData> implements ChangeData
 
   @Override
   public boolean hasChange() {
-    return source != null
-        && source instanceof ChangeDataSource
-        && ((ChangeDataSource) source).hasChange();
+    return source instanceof ChangeDataSource && ((ChangeDataSource) source).hasChange();
   }
 
   @Override

java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -484,7 +484,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     return ChangeStatusPredicate.parse(statusName);
   }
 
-  public Predicate<ChangeData> status_open() {
+  public Predicate<ChangeData> statusOpen() {
     return ChangeStatusPredicate.open();
   }
 
@@ -533,7 +533,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     }
 
     if ("visible".equalsIgnoreCase(value)) {
-      return is_visible();
+      return isVisible();
     }
 
     if ("reviewed".equalsIgnoreCase(value)) {
@@ -942,7 +942,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
   public Predicate<ChangeData> 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<ChangeData, ChangeQueryBuil
         args.anonymousUserProvider);
   }
 
-  public Predicate<ChangeData> is_visible() throws QueryParseException {
+  public Predicate<ChangeData> isVisible() throws QueryParseException {
     return visibleto(args.getUser());
   }
 

java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java

@@ -77,13 +77,13 @@ public class IsWatchedByPredicate extends AndPredicate<ChangeData> {
       } 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));
     }

java/com/google/gerrit/server/restapi/account/DeleteExternalIds.java

@@ -70,7 +70,7 @@ public class DeleteExternalIds implements RestModifyView<AccountResource, List<S
       permissionBackend.currentUser().check(GlobalPermission.ACCESS_DATABASE);
     }
 
-    if (extIds == null || extIds.size() == 0) {
+    if (extIds == null || extIds.isEmpty()) {
       throw new BadRequestException("external IDs are required");
     }
 

java/com/google/gerrit/server/restapi/change/GetDiff.java

@@ -253,7 +253,7 @@ public class GetDiff implements RestReadView<FileResource> {
         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;

java/com/google/gerrit/server/restapi/change/GetPatch.java

@@ -43,7 +43,7 @@ import org.kohsuke.args4j.Option;
 public class GetPatch implements RestReadView<RevisionResource> {
   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;

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<ChangeResource> {
@@ -97,12 +98,12 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
     this.sorter = sorter;
   }
 
-  public SubmittedTogether addListChangesOption(EnumSet<ListChangesOption> o) {
+  public SubmittedTogether addListChangesOption(Set<ListChangesOption> o) {
     jsonOpt.addAll(o);
     return this;
   }
 
-  public SubmittedTogether addSubmittedTogetherOption(EnumSet<SubmittedTogetherOption> o) {
+  public SubmittedTogether addSubmittedTogetherOption(Set<SubmittedTogetherOption> o) {
     options.addAll(o);
     return this;
   }

java/com/google/gerrit/server/restapi/group/ListGroups.java

@@ -86,7 +86,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   private final Groups groups;
   private final GroupResolver groupResolver;
 
-  private EnumSet<ListGroupsOption> options = EnumSet.noneOf(ListGroupsOption.class);
+  private Set<ListGroupsOption> options = EnumSet.noneOf(ListGroupsOption.class);
   private boolean visibleToAll;
   private Account.Id user;
   private boolean owned;
@@ -235,7 +235,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     this.groupResolver = groupResolver;
   }
 
-  public void setOptions(EnumSet<ListGroupsOption> options) {
+  public void setOptions(Set<ListGroupsOption> options) {
     this.options = options;
   }
 

java/com/google/gerrit/server/restapi/project/ListProjects.java

@@ -350,7 +350,7 @@ public class ListProjects implements RestReadView<TopLevelResource> {
       queries.add(String.format("(state:%s)", state.name()));
     }
 
-    return Joiner.on(" AND ").join(queries).toString();
+    return Joiner.on(" AND ").join(queries);
   }
 
   private SortedMap<String, ProjectInfo> applyAsQuery(String query) throws BadRequestException {

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());
       }

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);

java/com/google/gerrit/server/update/RetryingRestModifyView.java

@@ -40,7 +40,7 @@ public abstract class RetryingRestModifyView<R extends RestResource, I, O>
               .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);

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<SshSession> session;
   private final Provider<Context> 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;

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];
+      }
+    }
+  }
+}

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];

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();

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",
     ],
 )

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 =

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<String, Level> LOG_LEVELS =
+      ImmutableMap.<String, Level>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()));
+  }
+}

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<ChangesCollection> changes;
   @Inject private Provider<PostReview> 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);

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();

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",

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();

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",
     ],
 )
 

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> 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> 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> 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> 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> 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> repo, String... paths)
@@ -3104,21 +3088,25 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   protected ChangeInserter newChangeForBranch(TestRepository<Repo> 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> 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> 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> 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> 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;
   }
 

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",
     ],
 )
 

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();

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",
     ],
 )
 

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();

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",
     ],
 )
 

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)]]</gr-button>
             <span class="separator"></span>
           </span>
+          <template is="dom-if" if="[[_computeIsLoggedIn(_loggedIn)]]">
+            <span class="editButton">
+              <a href$="[[_computeEditURL(_change, _patchRange, _path)]]">
+                <iron-icon icon="gr-icons:edit"></iron-icon>
+              </a>
+              <span class="separator"></span>
+            </span>
+          </template>
           <div class$="diffModeSelector [[_computeModeSelectHideClass(_isImageDiff)]]">
             <span>Diff view:</span>
             <gr-diff-mode-selector

polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js

@@ -566,6 +566,14 @@
       return this._getDiffUrl(this._change, this._patchRange, newPath.path);
     },
 
+    _computeEditURL(change, patchRange, path) {
+      if ([change, patchRange, path].some(arg => 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;
+    },
   });
 })();

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 <gr-storage> 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) {

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', () => {

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 => {
+        if (anchor.hostname === window.location.hostname) {
+          anchor.setAttribute('target', '_self');
+        } else {
           anchor.setAttribute('target', '_blank');
+        }
         anchor.setAttribute('rel', 'noopener');
       });
     },

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);
 

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"],