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