Avoid Matcher#find on all messages in CommitRewriter.
Since some of the matching logic is invoked on each change message, this is
problematic for particularly large change messages.
Only invoke Matcher#find after using a stricter matching criteria first
(either Matcher#match or change message tag)
Change-Id: I14e6680a10d46daa543a210801c23e83ad25537c
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index 7f13731..3cbe546 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_LABEL;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_REAL_USER;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WITH;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TAG;
import static com.google.gerrit.server.util.AccountTemplateUtil.ACCOUNT_TEMPLATE_PATTERN;
import static com.google.gerrit.server.util.AccountTemplateUtil.ACCOUNT_TEMPLATE_REGEX;
@@ -39,6 +40,7 @@
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.git.RefUpdateUtil;
import com.google.gerrit.json.OutputFormat;
+import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.externalids.ExternalId;
@@ -169,7 +171,8 @@
Pattern.compile("Assignee changed from: (.*) to: (.*)");
private static final Pattern REMOVED_REVIEWER_PATTERN =
- Pattern.compile("Removed (cc|reviewer) (.*)(\\.| with the following votes)");
+ Pattern.compile(
+ "Removed (cc|reviewer) (.*)(\\.| with the following votes:\n.*)", Pattern.DOTALL);
private static final Pattern REMOVED_VOTE_PATTERN = Pattern.compile("Removed (.*) by (.*)");
@@ -185,12 +188,18 @@
private static final Pattern ON_CODE_OWNER_ADD_REVIEWER_PATTERN =
Pattern.compile("(.*) who was added as reviewer owns the following files");
+
+ private static final String CODE_OWNER_ADD_REVIEWER_TAG =
+ ChangeMessagesUtil.AUTOGENERATED_BY_GERRIT_TAG_PREFIX + "code-owners:addReviewer";
+
private static final String ON_CODE_OWNER_APPROVAL_REGEX = "code-owner approved by (.*):";
private static final String ON_CODE_OWNER_OVERRIDE_REGEX =
"code-owners submit requirement .* overridden by (.*)";
private static final Pattern ON_CODE_OWNER_REVIEW_PATTERN =
Pattern.compile(ON_CODE_OWNER_APPROVAL_REGEX + "|" + ON_CODE_OWNER_OVERRIDE_REGEX);
+ private static final Pattern ON_CODE_OWNER_POST_REVIEW_PATTERN =
+ Pattern.compile("Patch Set [0-9]+:[\\s\\S]*By (voting|removing)[\\s\\S]*");
private static final Pattern REPLY_BY_REASON_PATTERN =
Pattern.compile("(.*) replied on the change");
@@ -583,7 +592,7 @@
}
Matcher matcher = REMOVED_REVIEWER_PATTERN.matcher(originalChangeMessage);
- if (matcher.find() && !ACCOUNT_TEMPLATE_PATTERN.matcher(matcher.group(2)).matches()) {
+ if (matcher.matches() && !ACCOUNT_TEMPLATE_PATTERN.matcher(matcher.group(2)).matches()) {
// Since we do not use change messages for reviewer updates on UI, it does not matter what we
// rewrite it to.
return Optional.of(originalChangeMessage.substring(0, matcher.end(1)));
@@ -731,7 +740,11 @@
if (Strings.isNullOrEmpty(originalMessage)) {
return Optional.empty();
}
-
+ Matcher onCodeOwnerPostReviewMatcher =
+ ON_CODE_OWNER_POST_REVIEW_PATTERN.matcher(originalMessage);
+ if (!onCodeOwnerPostReviewMatcher.matches()) {
+ return Optional.empty();
+ }
Matcher onCodeOwnerReviewMatcher = ON_CODE_OWNER_REVIEW_PATTERN.matcher(originalMessage);
while (onCodeOwnerReviewMatcher.find()) {
String accountName =
@@ -823,7 +836,9 @@
for (FooterLine fl : footerLines) {
String footerKey = fl.getKey();
String footerValue = fl.getValue();
- if (footerKey.equalsIgnoreCase(FOOTER_ASSIGNEE.getName())) {
+ if (footerKey.equalsIgnoreCase(FOOTER_TAG.getName())) {
+ fixProgress.tag = footerValue;
+ } else if (footerKey.equalsIgnoreCase(FOOTER_ASSIGNEE.getName())) {
Account.Id oldAssignee = fixProgress.assigneeId;
FixIdentResult fixedAssignee = null;
if (footerValue.equals("")) {
@@ -954,7 +969,8 @@
fixedChangeMessage =
fixCodeOwnersOnReviewChangeMessage(fixProgress.updateAuthorId, originalChangeMessage);
}
- if (!fixedChangeMessage.isPresent()) {
+ if (!fixedChangeMessage.isPresent()
+ && Objects.equals(fixProgress.tag, CODE_OWNER_ADD_REVIEWER_TAG)) {
fixedChangeMessage =
fixCodeOwnersOnAddReviewerChangeMessage(fixProgress, originalChangeMessage);
}
@@ -1194,6 +1210,9 @@
/** {@link RefNames#changeMetaRef} of the change that is being fixed. */
final String changeMetaRef;
+ /** Tag at current commit update. */
+ String tag = null;
+
/** Assignee at current commit update. */
Account.Id assigneeId = null;
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index f316660..19c2bcf 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -34,6 +34,7 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.json.OutputFormat;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.notedb.ChangeNoteUtil.AttentionStatusInNoteDb;
@@ -1603,19 +1604,20 @@
accountCache.put(duplicateReviewer);
Change c = newChange();
ImmutableList.Builder<ObjectId> commitsToFix = new ImmutableList.Builder<>();
- ChangeUpdate addReviewerUpdate = newUpdate(c, changeOwner);
+ ChangeUpdate addReviewerUpdate = newCodeOwnerAddReviewerUpdate(c, changeOwner);
addReviewerUpdate.putReviewer(reviewer.id(), REVIEWER);
addReviewerUpdate.commit();
- ChangeUpdate invalidOnAddReviewerUpdate = newUpdate(c, changeOwner);
+ ChangeUpdate invalidOnAddReviewerUpdate = newCodeOwnerAddReviewerUpdate(c, changeOwner);
invalidOnAddReviewerUpdate.setChangeMessage(
"Reviewer User who was added as reviewer owns the following files:\n"
+ " * file1.java\n"
+ " * file2.ts\n");
commitsToFix.add(invalidOnAddReviewerUpdate.commit());
- ChangeUpdate addOtherReviewerUpdate = newUpdate(c, changeOwner);
+ ChangeUpdate addOtherReviewerUpdate = newCodeOwnerAddReviewerUpdate(c, changeOwner);
addOtherReviewerUpdate.putReviewer(otherUserId, REVIEWER);
addOtherReviewerUpdate.commit();
- ChangeUpdate invalidOnAddReviewerMultipleReviewerUpdate = newUpdate(c, changeOwner);
+ ChangeUpdate invalidOnAddReviewerMultipleReviewerUpdate =
+ newCodeOwnerAddReviewerUpdate(c, changeOwner);
invalidOnAddReviewerMultipleReviewerUpdate.setChangeMessage(
"Reviewer User who was added as reviewer owns the following files:\n"
+ " * file1.java\n"
@@ -1624,17 +1626,17 @@
+ "\nMissing Reviewer who was added as reviewer owns the following files:\n"
+ " * file4.java\n");
commitsToFix.add(invalidOnAddReviewerMultipleReviewerUpdate.commit());
- ChangeUpdate addDuplicateReviewerUpdate = newUpdate(c, changeOwner);
+ ChangeUpdate addDuplicateReviewerUpdate = newCodeOwnerAddReviewerUpdate(c, changeOwner);
addDuplicateReviewerUpdate.putReviewer(duplicateReviewer.id(), REVIEWER);
addDuplicateReviewerUpdate.commit();
// Reviewer name resolves to multiple accounts in the same change
- ChangeUpdate onAddReviewerUpdateWithDuplicate = newUpdate(c, changeOwner);
+ ChangeUpdate onAddReviewerUpdateWithDuplicate = newCodeOwnerAddReviewerUpdate(c, changeOwner);
onAddReviewerUpdateWithDuplicate.setChangeMessage(
"Reviewer User who was added as reviewer owns the following files:\n"
+ " * file6.java\n");
commitsToFix.add(onAddReviewerUpdateWithDuplicate.commit());
- ChangeUpdate validOnAddReviewerUpdate = newUpdate(c, changeOwner);
+ ChangeUpdate validOnAddReviewerUpdate = newCodeOwnerAddReviewerUpdate(c, changeOwner);
validOnAddReviewerUpdate.setChangeMessage(
"Gerrit Account who was added as reviewer owns the following files:\n"
+ " * file1.java\n"
@@ -2249,6 +2251,13 @@
.collect(toImmutableList());
}
+ protected ChangeUpdate newCodeOwnerAddReviewerUpdate(Change c, CurrentUser user)
+ throws Exception {
+ ChangeUpdate update = newUpdate(c, user, true);
+ update.setTag("autogenerated:gerrit:code-owners:addReviewer");
+ return update;
+ }
+
private ImmutableList<String> commitHistoryDiff(BackfillResult result, Change.Id changeId) {
return result.fixedRefDiff.get(RefNames.changeMetaRef(changeId)).stream()
.map(CommitDiff::diff)