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