Merge branch 'stable-2.16'

* stable-2.16:
  GroupsUpdate: Add trace timer on group creation/update operations
  GroupsUpdate: Add debug log on cache eviction for group creation/update
  GroupsUpdate: Rename update... methods to evict...
  VersionedMetaData: Convert debug log of read/save file to trace timers
  TraceContext: Support fourth format parameter
  TestAccount: Add implementation of toString
  Correct URL of change message requests in docs
  Fixing Linux Kernel URL in Why Code Review Doc
  ExternalIdNotes: Only log 'reading external ID note map' when actually doing so
  RestApiException: Fix Javadoc
  Fix groups to prevent javascript errors with url

Change-Id: Ie5c73b3424c62b9e664ec16b7699ae4194a194cc

Documentation/intro-rockstar.txt

@@ -60,7 +60,7 @@ older (imperfect) revision is not lost. It can be found via the `git reflog`.
 At least two well-known open source projects insist on these practices:
 
 * link:http://git-scm.com/[Git]
-* link:http://www.kernel.org/category/about.html/[Linux Kernel]
+* link:http://www.kernel.org/category/about.html[Linux Kernel]
 
 However, contributors to these projects don’t refine and polish their changes
 in private until they’re perfect. Instead, polishing code is part of a review

Documentation/rest-api-changes.txt

@@ -2467,7 +2467,7 @@ As response a list of link:#change-message-info[ChangeMessageInfo] entities is r
 Retrieves a change message including link:#detailed-accounts[detailed account information].
 
 --
-'GET /changes/link:#change-id[\{change-id\}]/message/link:#change-message-id[\{change-message-id\}]'
+'GET /changes/link:#change-id[\{change-id\}]/messages/link:#change-message-id[\{change-message-id\}]'
 --
 
 As response a link:#change-message-info[ChangeMessageInfo] entity is returned.
@@ -2496,8 +2496,8 @@ As response a link:#change-message-info[ChangeMessageInfo] entity is returned.
 [[delete-change-message]]
 === Delete Change Message
 --
-'DELETE /changes/link:#change-id[\{change-id\}]/message/link:#change-message-id[\{change-message-id\}]' +
+'DELETE /changes/link:#change-id[\{change-id\}]/messages/link:#change-message-id[\{change-message-id\}]' +
-'POST /changes/link:#change-id[\{change-id\}]//message/link:#change-message-id[\{change-message-id\}]/delete'
+'POST /changes/link:#change-id[\{change-id\}]/messages/link:#change-message-id[\{change-message-id\}]/delete'
 --
 
 Deletes a change message by replacing the change message with a new message,
@@ -2513,14 +2513,14 @@ To delete a change message, send a DELETE request:
 
 .Request
 ----
-  DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/message/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780 HTTP/1.0
+  DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/messages/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780 HTTP/1.0
 ----
 
 To provide a reason for the deletion, use a POST request:
 
 .Request
 ----
-  POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/message/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780/delete HTTP/1.0
+  POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/messages/aaee04dcb46bafc8be24d8aa70b3b1beb7df5780/delete HTTP/1.0
   Content-Type: application/json; charset=UTF-8
 
   {

java/com/google/gerrit/acceptance/TestAccount.java

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance;
 
 import static java.util.stream.Collectors.toList;
 
+import com.google.common.base.MoreObjects;
 import com.google.common.net.InetAddresses;
 import com.google.gerrit.mail.Address;
 import com.google.gerrit.reviewdb.client.Account;
@@ -71,4 +72,9 @@ public class TestAccount {
   public Account.Id getId() {
     return id;
   }
+
+  @Override
+  public String toString() {
+    return MoreObjects.toStringHelper(this).add("id", id).add("username", username).toString();
+  }
 }

java/com/google/gerrit/extensions/restapi/RestApiException.java

@@ -14,7 +14,7 @@
 
 package com.google.gerrit.extensions.restapi;
 
-/** Root exception type for JSON API failures. */
+/** Root exception type for REST API failures. */
 public class RestApiException extends Exception {
   private static final long serialVersionUID = 1L;
   private CacheControl caching = CacheControl.NONE;

java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java

@@ -656,9 +656,12 @@ public class ExternalIdNotes extends VersionedMetaData {
 
   @Override
   protected void onLoad() throws IOException, ConfigInvalidException {
-    logger.atFine().log("Reading external ID note map");
+    if (revision != null) {
-
+      logger.atFine().log("Reading external ID note map");
-    noteMap = revision != null ? NoteMap.read(reader, revision) : NoteMap.newEmptyMap();
+      noteMap = NoteMap.read(reader, revision);
+    } else {
+      noteMap = NoteMap.newEmptyMap();
+    }
 
     if (afterReadRevision != null) {
       afterReadRevision.run();

java/com/google/gerrit/server/git/meta/VersionedMetaData.java

@@ -17,10 +17,11 @@ package com.google.gerrit.server.git.meta;
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.base.MoreObjects;
-import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.git.LockFailureException;
 import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.logging.TraceContext.TraceTimer;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.StringReader;
@@ -64,8 +65,6 @@ import org.eclipse.jgit.util.RawParseUtils;
  * read from the repository, or format an update that can later be written back to the repository.
  */
 public abstract class VersionedMetaData {
-  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
   /**
    * Path information that does not hold references to any repository data structures, allowing the
    * application to retain this object for long periods of time.
@@ -497,10 +496,11 @@ public abstract class VersionedMetaData {
       return new byte[] {};
     }
 
-    logger.atFine().log(
+    try (TraceTimer timer =
-        "Read file '%s' from ref '%s' of project '%s' from revision '%s'",
+            TraceContext.newTimer(
-        fileName, getRefName(), projectName, revision.name());
+                "Read file '%s' from ref '%s' of project '%s' from revision '%s'",
-    try (TreeWalk tw = TreeWalk.forPath(reader, fileName, revision.getTree())) {
+                fileName, getRefName(), projectName, revision.name());
+        TreeWalk tw = TreeWalk.forPath(reader, fileName, revision.getTree())) {
       if (tw != null) {
         ObjectLoader obj = reader.open(tw.getObjectId(0), Constants.OBJ_BLOB);
         return obj.getCachedBytes(Integer.MAX_VALUE);
@@ -572,22 +572,24 @@ public abstract class VersionedMetaData {
   }
 
   protected void saveFile(String fileName, byte[] raw) throws IOException {
-    logger.atFine().log(
+    try (TraceTimer timer =
-        "Save file '%s' in ref '%s' of project '%s'", fileName, getRefName(), projectName);
+        TraceContext.newTimer(
-    DirCacheEditor editor = newTree.editor();
+            "Save file '%s' in ref '%s' of project '%s'", fileName, getRefName(), projectName)) {
-    if (raw != null && 0 < raw.length) {
+      DirCacheEditor editor = newTree.editor();
-      final ObjectId blobId = inserter.insert(Constants.OBJ_BLOB, raw);
+      if (raw != null && 0 < raw.length) {
-      editor.add(
+        final ObjectId blobId = inserter.insert(Constants.OBJ_BLOB, raw);
-          new PathEdit(fileName) {
+        editor.add(
-            @Override
+            new PathEdit(fileName) {
-            public void apply(DirCacheEntry ent) {
+              @Override
-              ent.setFileMode(FileMode.REGULAR_FILE);
+              public void apply(DirCacheEntry ent) {
-              ent.setObjectId(blobId);
+                ent.setFileMode(FileMode.REGULAR_FILE);
-            }
+                ent.setObjectId(blobId);
-          });
+              }
-    } else {
+            });
-      editor.add(new DeletePath(fileName));
+      } else {
+        editor.add(new DeletePath(fileName));
+      }
+      editor.finish();
     }
-    editor.finish();
   }
 }

java/com/google/gerrit/server/group/db/GroupsUpdate.java

@@ -19,6 +19,7 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.git.LockFailureException;
@@ -40,6 +41,8 @@ import com.google.gerrit.server.git.meta.MetaDataUpdate;
 import com.google.gerrit.server.group.GroupAuditService;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.index.group.GroupIndexer;
+import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.logging.TraceContext.TraceTimer;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.gwtorm.server.OrmDuplicateKeyException;
@@ -95,6 +98,8 @@ public class GroupsUpdate {
     GroupsUpdate createWithServerIdent();
   }
 
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
   private final GitRepositoryManager repoManager;
   private final AllUsersName allUsersName;
   private final GroupCache groupCache;
@@ -258,10 +263,14 @@ public class GroupsUpdate {
   public InternalGroup createGroup(
       InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate)
       throws OrmDuplicateKeyException, IOException, ConfigInvalidException {
-    InternalGroup createdGroup = createGroupInNoteDbWithRetry(groupCreation, groupUpdate);
+    try (TraceTimer timer =
-    updateCachesOnGroupCreation(createdGroup);
+        TraceContext.newTimer(
-    dispatchAuditEventsOnGroupCreation(createdGroup);
+            "Creating group '%s'", groupUpdate.getName().orElseGet(groupCreation::getNameKey))) {
-    return createdGroup;
+      InternalGroup createdGroup = createGroupInNoteDbWithRetry(groupCreation, groupUpdate);
+      evictCachesOnGroupCreation(createdGroup);
+      dispatchAuditEventsOnGroupCreation(createdGroup);
+      return createdGroup;
+    }
   }
 
   /**
@@ -276,16 +285,18 @@ public class GroupsUpdate {
    */
   public void updateGroup(AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate)
       throws OrmDuplicateKeyException, IOException, NoSuchGroupException, ConfigInvalidException {
-    Optional<Timestamp> updatedOn = groupUpdate.getUpdatedOn();
+    try (TraceTimer timer = TraceContext.newTimer("Updating group %s", groupUuid)) {
-    if (!updatedOn.isPresent()) {
+      Optional<Timestamp> updatedOn = groupUpdate.getUpdatedOn();
-      updatedOn = Optional.of(TimeUtil.nowTs());
+      if (!updatedOn.isPresent()) {
-      groupUpdate = groupUpdate.toBuilder().setUpdatedOn(updatedOn.get()).build();
+        updatedOn = Optional.of(TimeUtil.nowTs());
-    }
+        groupUpdate = groupUpdate.toBuilder().setUpdatedOn(updatedOn.get()).build();
+      }
 
-    UpdateResult result = updateGroupInNoteDbWithRetry(groupUuid, groupUpdate);
+      UpdateResult result = updateGroupInNoteDbWithRetry(groupUuid, groupUpdate);
-    updateNameInProjectConfigsIfNecessary(result);
+      updateNameInProjectConfigsIfNecessary(result);
-    updateCachesOnGroupUpdate(result);
+      evictCachesOnGroupUpdate(result);
-    dispatchAuditEventsOnGroupUpdate(result, updatedOn.get());
+      dispatchAuditEventsOnGroupUpdate(result, updatedOn.get());
+    }
   }
 
   private InternalGroup createGroupInNoteDbWithRetry(
@@ -424,7 +435,8 @@ public class GroupsUpdate {
         allUsersName, batchRefUpdate, currentUser.map(user -> user.state()).orElse(null));
   }
 
-  private void updateCachesOnGroupCreation(InternalGroup createdGroup) throws IOException {
+  private void evictCachesOnGroupCreation(InternalGroup createdGroup) throws IOException {
+    logger.atFine().log("evict caches on creation of group %s", createdGroup.getGroupUUID());
     // By UUID is used for the index and hence should be evicted before refreshing the index.
     groupCache.evict(createdGroup.getGroupUUID());
     indexer.get().index(createdGroup.getGroupUUID());
@@ -436,7 +448,8 @@ public class GroupsUpdate {
     createdGroup.getSubgroups().forEach(groupIncludeCache::evictParentGroupsOf);
   }
 
-  private void updateCachesOnGroupUpdate(UpdateResult result) throws IOException {
+  private void evictCachesOnGroupUpdate(UpdateResult result) throws IOException {
+    logger.atFine().log("evict caches on update of group %s", result.getGroupUuid());
     // By UUID is used for the index and hence should be evicted before refreshing the index.
     groupCache.evict(result.getGroupUuid());
     indexer.get().index(result.getGroupUuid());

java/com/google/gerrit/server/logging/TraceContext.java

@@ -206,6 +206,23 @@ public class TraceContext implements AutoCloseable {
     return new TraceTimer(format, arg1, arg2, arg3);
   }
 
+  /**
+   * Opens a new timer that logs the time for an operation if request tracing is enabled.
+   *
+   * <p>If request tracing is not enabled this is a no-op.
+   *
+   * @param format the message format string
+   * @param arg1 first argument for the message
+   * @param arg2 second argument for the message
+   * @param arg3 third argument for the message
+   * @param arg4 fourth argument for the message
+   * @return the trace timer
+   */
+  public static TraceTimer newTimer(
+      String format, Object arg1, Object arg2, Object arg3, Object arg4) {
+    return new TraceTimer(format, arg1, arg2, arg3, arg4);
+  }
+
   public static class TraceTimer implements AutoCloseable {
     private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
@@ -229,6 +246,16 @@ public class TraceContext implements AutoCloseable {
       this(elapsedMs -> logger.atFine().log(format + " (%d ms)", arg1, arg2, arg3, elapsedMs));
     }
 
+    private TraceTimer(
+        String format,
+        @Nullable Object arg1,
+        @Nullable Object arg2,
+        @Nullable Object arg3,
+        @Nullable Object arg4) {
+      this(
+          elapsedMs -> logger.atFine().log(format + " (%d ms)", arg1, arg2, arg3, arg4, elapsedMs));
+    }
+
     private TraceTimer(Consumer<Long> logFn) {
       this.logFn = logFn;
       this.stopwatch = Stopwatch.createStarted();