Merge branch 'stable-2.15'

* stable-2.15:
  Return early for branches for which we do not want to create review notes

Change-Id: I508f717cd3d5d776e4b4f53534dcd734e7dfd678
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/CreateReviewNotes.java b/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/CreateReviewNotes.java
index 300baa4..43dd34f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/CreateReviewNotes.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/CreateReviewNotes.java
@@ -14,7 +14,7 @@
 
 package com.googlesource.gerrit.plugins.reviewnotes;
 
-import com.google.gerrit.common.Nullable;
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
 import com.google.gerrit.reviewdb.client.Change;
@@ -24,10 +24,10 @@
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.config.AnonymousCowardName;
-import com.google.gerrit.server.config.CanonicalWebUrl;
+import com.google.gerrit.server.config.UrlFormatter;
 import com.google.gerrit.server.git.LockFailureException;
 import com.google.gerrit.server.git.NotesBranchUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
@@ -56,12 +56,9 @@
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 class CreateReviewNotes {
-
-  private static final Logger log = LoggerFactory.getLogger(CreateReviewNotes.class);
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   interface Factory {
     CreateReviewNotes create(ReviewDb reviewDb, Project.NameKey project, Repository git);
@@ -75,10 +72,9 @@
   private final LabelTypes labelTypes;
   private final ApprovalsUtil approvalsUtil;
   private final ChangeNotes.Factory notesFactory;
-  private final IdentifiedUser.GenericFactory userFactory;
   private final NotesBranchUtil.Factory notesBranchUtilFactory;
   private final Provider<InternalChangeQuery> queryProvider;
-  private final String canonicalWebUrl;
+  private final UrlFormatter urlFormatter;
   private final ReviewDb reviewDb;
   private final Project.NameKey project;
   private final Repository git;
@@ -95,10 +91,9 @@
       ProjectCache projectCache,
       ApprovalsUtil approvalsUtil,
       ChangeNotes.Factory notesFactory,
-      IdentifiedUser.GenericFactory userFactory,
       NotesBranchUtil.Factory notesBranchUtilFactory,
       Provider<InternalChangeQuery> queryProvider,
-      @Nullable @CanonicalWebUrl String canonicalWebUrl,
+      UrlFormatter urlFormatter,
       @Assisted ReviewDb reviewDb,
       @Assisted Project.NameKey project,
       @Assisted Repository git) {
@@ -107,8 +102,8 @@
     this.anonymousCowardName = anonymousCowardName;
     ProjectState projectState = projectCache.get(project);
     if (projectState == null) {
-      log.error(
-          "Could not obtain available labels for project {}."
+      logger.atSevere().log(
+          "Could not obtain available labels for project %s."
               + " Expect missing labels in its review notes.",
           project.get());
       this.labelTypes = new LabelTypes(Collections.<LabelType>emptyList());
@@ -117,10 +112,9 @@
     }
     this.approvalsUtil = approvalsUtil;
     this.notesFactory = notesFactory;
-    this.userFactory = userFactory;
     this.notesBranchUtilFactory = notesBranchUtilFactory;
     this.queryProvider = queryProvider;
-    this.canonicalWebUrl = canonicalWebUrl;
+    this.urlFormatter = urlFormatter;
     this.reviewDb = reviewDb;
     this.project = project;
     this.git = git;
@@ -143,7 +137,7 @@
           markUninteresting(git, branch, rw, oldObjectId);
         }
       } catch (Exception e) {
-        log.error(e.getMessage(), e);
+        logger.atSevere().withCause(e).log(e.getMessage());
         return;
       }
 
@@ -162,11 +156,8 @@
             getMessage().append("* ").append(c.getShortMessage()).append("\n");
           }
         } else {
-          if (log.isDebugEnabled()) {
-            log.debug(
-                "no note for this commit since it is a direct push {}",
-                c.getName().substring(0, 7));
-          }
+          logger.atFine().log(
+              "no note for this commit since it is a direct push %s", c.getName().substring(0, 7));
         }
       }
     }
@@ -207,8 +198,9 @@
     }
   }
 
-  private void markUninteresting(Repository git, String branch, RevWalk rw, ObjectId oldObjectId) {
-    for (Ref r : git.getAllRefs().values()) {
+  private void markUninteresting(Repository git, String branch, RevWalk rw, ObjectId oldObjectId)
+      throws IOException {
+    for (Ref r : git.getRefDatabase().getRefs()) {
       try {
         if (r.getName().equals(branch)) {
           if (!ObjectId.zeroId().equals(oldObjectId)) {
@@ -265,9 +257,7 @@
     // commit time so we will be able to skip this normalization step.
     Change change = notes.getChange();
     PatchSetApproval submit = null;
-    for (PatchSetApproval a :
-        approvalsUtil.byPatchSet(
-            reviewDb, notes, userFactory.create(change.getOwner()), ps.getId(), null, null)) {
+    for (PatchSetApproval a : approvalsUtil.byPatchSet(reviewDb, notes, ps.getId(), null, null)) {
       if (a.getValue() == 0) {
         // Ignore 0 values.
       } else if (a.isLegacySubmit()) {
@@ -275,16 +265,23 @@
       } else {
         LabelType type = labelTypes.byLabel(a.getLabelId());
         if (type != null) {
-          fmt.appendApproval(type, a.getValue(), accountCache.get(a.getAccountId()).getAccount());
+          fmt.appendApproval(
+              type,
+              a.getValue(),
+              a.getAccountId(),
+              accountCache.get(a.getAccountId()).map(AccountState::getAccount));
         }
       }
     }
     if (submit != null) {
-      fmt.appendSubmittedBy(accountCache.get(submit.getAccountId()).getAccount());
+      fmt.appendSubmittedBy(
+          submit.getAccountId(),
+          accountCache.get(submit.getAccountId()).map(AccountState::getAccount));
       fmt.appendSubmittedAt(submit.getGranted());
     }
-    if (canonicalWebUrl != null) {
-      fmt.appendReviewedOn(canonicalWebUrl, ps.getId().getParentKey());
+
+    if (urlFormatter.getWebUrl().isPresent()) {
+      fmt.appendReviewedOn(urlFormatter, notes.getChange().getProject(), ps.getId().getParentKey());
     }
     fmt.appendProject(project.get());
     fmt.appendBranch(change.getDest().get());
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/HeaderFormatter.java b/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/HeaderFormatter.java
index 8633d5c..5559eeb 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/HeaderFormatter.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/HeaderFormatter.java
@@ -14,15 +14,21 @@
 
 package com.googlesource.gerrit.plugins.reviewnotes;
 
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.common.base.Strings;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelValue;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.config.UrlFormatter;
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
 import java.util.Calendar;
 import java.util.Date;
 import java.util.Locale;
+import java.util.Optional;
 import java.util.TimeZone;
 
 /**
@@ -44,38 +50,57 @@
     this.anonymousCowardName = anonymousCowardName;
   }
 
-  void appendChangeId(Change.Key changeKey) {
-    sb.append("Change-Id: ").append(changeKey.get()).append("\n");
-  }
-
-  void appendApproval(LabelType label, short value, Account user) {
+  /**
+   * Appends a header for an approval.
+   *
+   * @param label the label on which the approval was done
+   * @param value the voting value
+   * @param accountId the account ID of the approver
+   * @param account the account of the approver, can be {@link Optional#empty} if the account is
+   *     missing
+   */
+  void appendApproval(
+      LabelType label, short value, Account.Id accountId, Optional<Account> account) {
     sb.append(label.getName());
     sb.append(LabelValue.formatValue(value));
     sb.append(": ");
-    appendUserData(user);
+    appendUserData(accountId, account);
     sb.append("\n");
   }
 
-  private void appendUserData(Account user) {
+  /**
+   * Appends user data.
+   *
+   * @param accountId the ID of the account
+   * @param account the account, can be {link Optional#empty} if the account is missing
+   */
+  private void appendUserData(Account.Id accountId, Optional<Account> account) {
+    checkState(
+        !account.isPresent() || accountId.equals(account.get().getId()), "mismatching account IDs");
+
     boolean needSpace = false;
     boolean wroteData = false;
 
-    if (user.getFullName() != null && !user.getFullName().isEmpty()) {
-      sb.append(user.getFullName());
-      needSpace = true;
-      wroteData = true;
-    }
-
-    if (user.getPreferredEmail() != null && !user.getPreferredEmail().isEmpty()) {
-      if (needSpace) {
-        sb.append(" ");
+    if (account.isPresent()) {
+      String fullName = account.get().getFullName();
+      if (!Strings.isNullOrEmpty(fullName)) {
+        sb.append(fullName);
+        needSpace = true;
+        wroteData = true;
       }
-      sb.append("<").append(user.getPreferredEmail()).append(">");
-      wroteData = true;
+
+      String preferredEmail = account.get().getPreferredEmail();
+      if (!Strings.isNullOrEmpty(preferredEmail)) {
+        if (needSpace) {
+          sb.append(" ");
+        }
+        sb.append("<").append(preferredEmail).append(">");
+        wroteData = true;
+      }
     }
 
     if (!wroteData) {
-      sb.append(anonymousCowardName).append(" #").append(user.getId());
+      sb.append(anonymousCowardName).append(" #").append(accountId);
     }
   }
 
@@ -87,9 +112,16 @@
     sb.append("Branch: ").append(branch).append("\n");
   }
 
-  void appendSubmittedBy(Account user) {
+  /**
+   * Appends a header with the submitter information.
+   *
+   * @param accountId the account ID of the submitter
+   * @param account the account of the submitter, can be {@link Optional#empty()} if the account is
+   *     missing
+   */
+  void appendSubmittedBy(Account.Id accountId, Optional<Account> account) {
     sb.append("Submitted-by: ");
-    appendUserData(user);
+    appendUserData(accountId, account);
     sb.append("\n");
   }
 
@@ -97,8 +129,10 @@
     sb.append("Submitted-at: ").append(rfc2822DateFormatter.format(date)).append("\n");
   }
 
-  void appendReviewedOn(String canonicalWebUrl, Change.Id changeId) {
-    sb.append("Reviewed-on: ").append(canonicalWebUrl).append(changeId.get()).append("\n");
+  void appendReviewedOn(UrlFormatter urlFormatter, Project.NameKey project, Change.Id changeId) {
+    sb.append("Reviewed-on: ")
+        .append(urlFormatter.getChangeViewUrl(project, changeId).get())
+        .append("\n");
   }
 
   @Override
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/RefUpdateListener.java b/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/RefUpdateListener.java
index 17b7f81..d8e0e83 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/RefUpdateListener.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewnotes/RefUpdateListener.java
@@ -14,6 +14,7 @@
 
 package com.googlesource.gerrit.plugins.reviewnotes;
 
+import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Project;
@@ -31,12 +32,9 @@
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 class RefUpdateListener implements GitReferenceUpdatedListener {
-
-  private static final Logger log = LoggerFactory.getLogger(RefUpdateListener.class);
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final CreateReviewNotes.Factory reviewNotesFactory;
   private final SchemaFactory<ReviewDb> schema;
@@ -119,7 +117,7 @@
             return null;
           });
     } catch (RestApiException | UpdateException x) {
-      log.error(x.getMessage(), x);
+      logger.atSevere().withCause(x).log(x.getMessage());
     }
   }
 }