Match account replacements by email in change messages.
Also added ref name to logs, so it is easier to debug the cases when
accounts can not be parsed/matched.
Change-Id: I5dbd3fb6e249ac8c68e9bd644a8611bcc6e9e115
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index 7d743dc..7f13731 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -58,7 +58,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
-import java.util.Map.Entry;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
@@ -386,7 +385,7 @@
RevCommit originalCommit;
boolean rewriteStarted = false;
- ChangeFixProgress changeFixProgress = new ChangeFixProgress();
+ ChangeFixProgress changeFixProgress = new ChangeFixProgress(ref.getName());
while ((originalCommit = revWalk.next()) != null) {
changeFixProgress.updateAuthorId =
@@ -538,7 +537,9 @@
return Optional.of(
"Assignee deleted: "
+ getPossibleAccountReplacement(
- changeFixProgress, oldAssignee, assigneeDeletedMatcher.group(1)));
+ changeFixProgress,
+ oldAssignee,
+ ParsedAccountInfo.create(assigneeDeletedMatcher.group(1))));
}
return Optional.empty();
}
@@ -549,7 +550,9 @@
return Optional.of(
"Assignee added: "
+ getPossibleAccountReplacement(
- changeFixProgress, newAssignee, assigneeAddedMatcher.group(1)));
+ changeFixProgress,
+ newAssignee,
+ ParsedAccountInfo.create(assigneeAddedMatcher.group(1))));
}
return Optional.empty();
}
@@ -561,9 +564,13 @@
String.format(
"Assignee changed from: %s to: %s",
getPossibleAccountReplacement(
- changeFixProgress, oldAssignee, assigneeChangedMatcher.group(1)),
+ changeFixProgress,
+ oldAssignee,
+ ParsedAccountInfo.create(assigneeChangedMatcher.group(1))),
getPossibleAccountReplacement(
- changeFixProgress, newAssignee, assigneeChangedMatcher.group(2))));
+ changeFixProgress,
+ newAssignee,
+ ParsedAccountInfo.create(assigneeChangedMatcher.group(2)))));
}
return Optional.empty();
}
@@ -599,7 +606,7 @@
"Removed %s by %s",
matcher.group(1),
getPossibleAccountReplacement(
- changeFixProgress, reviewer, getNameFromNameEmail(matcher.group(2)))));
+ changeFixProgress, reviewer, getAccountInfoFromNameEmail(matcher.group(2)))));
}
return Optional.empty();
}
@@ -612,21 +619,27 @@
}
String[] lines = originalChangeMessage.split("\\r?\\n");
StringBuilder fixedLines = new StringBuilder();
+ boolean anyFixed = false;
for (int i = 1; i < lines.length; i++) {
if (lines[i].isEmpty()) {
continue;
}
Matcher matcher = REMOVED_VOTES_CHANGE_MESSAGE_PATTERN.matcher(lines[i]);
+ String replacementLine = lines[i];
if (matcher.matches() && !NON_REPLACE_ACCOUNT_PATTERN.matcher(matcher.group(2)).matches()) {
- fixedLines.append(
+ anyFixed = true;
+ replacementLine =
String.format(
"* %s by %s\n",
matcher.group(1),
getPossibleAccountReplacement(
- changeFixProgress, Optional.empty(), getNameFromNameEmail(matcher.group(2)))));
+ changeFixProgress,
+ Optional.empty(),
+ getAccountInfoFromNameEmail(matcher.group(2))));
}
+ fixedLines.append(replacementLine);
}
- if (fixedLines.length() == 0) {
+ if (!anyFixed) {
return Optional.empty();
}
return Optional.of(REMOVED_VOTES_CHANGE_MESSAGE_START + "\n" + fixedLines);
@@ -687,7 +700,8 @@
while (onAddReviewerMatcher.find()) {
String reviewerName = normalizeOnCodeOwnerAddReviewerMatch(onAddReviewerMatcher.group(1));
String replacementName =
- getPossibleAccountReplacement(changeFixProgress, Optional.empty(), reviewerName);
+ getPossibleAccountReplacement(
+ changeFixProgress, Optional.empty(), ParsedAccountInfo.create(reviewerName));
onAddReviewerMatcher.appendReplacement(
sb, replacementName + ", who was added as reviewer owns the following files");
}
@@ -971,9 +985,10 @@
private Optional<Account.Id> parseIdent(ChangeFixProgress changeFixProgress, PersonIdent ident) {
Optional<Account.Id> account = NoteDbUtil.parseIdent(ident);
if (account.isPresent()) {
- changeFixProgress.parsedAccounts.putIfAbsent(account.get(), "");
+ changeFixProgress.parsedAccounts.putIfAbsent(account.get(), Optional.empty());
} else {
- logger.atWarning().log("Failed to parse id %s", ident);
+ logger.atWarning().log(
+ "Fixing ref %s, failed to parse id %s", changeFixProgress.changeMetaRef, ident);
}
return account;
}
@@ -1022,10 +1037,16 @@
return fixIdentResult;
}
- /** Extracts {@link Account#getName} from {@link Account#getNameEmail} */
- private String getNameFromNameEmail(String nameEmail) {
+ /** Extracts {@link ParsedAccountInfo} from {@link Account#getNameEmail} */
+ private ParsedAccountInfo getAccountInfoFromNameEmail(String nameEmail) {
Matcher nameEmailMatcher = NAME_EMAIL_PATTERN.matcher(nameEmail);
- return nameEmailMatcher.matches() ? nameEmailMatcher.group(1) : nameEmail;
+ if (!nameEmailMatcher.matches()) {
+ return ParsedAccountInfo.create(nameEmail);
+ }
+
+ return ParsedAccountInfo.create(
+ nameEmailMatcher.group(1),
+ nameEmailMatcher.group(2).substring(1, nameEmailMatcher.group(2).length() - 1));
}
/**
@@ -1038,39 +1059,73 @@
*
* @param changeFixProgress see {@link ChangeFixProgress}
* @param account account that should be used for replacement, if known
- * @param accountName {@link Account#getName} to replace.
+ * @param accountInfo {@link ParsedAccountInfo} to replace.
* @return replacement for {@code accountName}
*/
private String getPossibleAccountReplacement(
- ChangeFixProgress changeFixProgress, Optional<Account.Id> account, String accountName) {
+ ChangeFixProgress changeFixProgress,
+ Optional<Account.Id> account,
+ ParsedAccountInfo accountInfo) {
if (account.isPresent()) {
return AccountTemplateUtil.getAccountTemplate(account.get());
}
// Retrieve reviewer accounts from cache and try to match by their name.
- Map<Account.Id, AccountState> missingUserNameReviewers =
+ Map<Account.Id, AccountState> missingAccountStateReviewers =
accountCache.get(
changeFixProgress.parsedAccounts.entrySet().stream()
- .filter(entry -> entry.getValue().isEmpty())
+ .filter(entry -> !entry.getValue().isPresent())
.map(Map.Entry::getKey)
.collect(ImmutableSet.toImmutableSet()));
changeFixProgress.parsedAccounts.putAll(
- missingUserNameReviewers.entrySet().stream()
+ missingAccountStateReviewers.entrySet().stream()
.collect(
ImmutableMap.toImmutableMap(
- Map.Entry::getKey, e -> e.getValue().account().getName())));
- Set<Account.Id> possibleReplacements =
- changeFixProgress.parsedAccounts.entrySet().stream()
- .filter(e -> e.getValue().equals(accountName))
- .map(Entry::getKey)
- .collect(ImmutableSet.toImmutableSet());
+ Map.Entry::getKey, e -> Optional.ofNullable(e.getValue()))));
+ Map<Account.Id, AccountState> possibleReplacements = ImmutableMap.of();
+ if (accountInfo.email().isPresent()) {
+ possibleReplacements =
+ changeFixProgress.parsedAccounts.entrySet().stream()
+ .filter(
+ e ->
+ e.getValue().isPresent()
+ && Objects.equals(
+ e.getValue().get().account().preferredEmail(),
+ accountInfo.email().get()))
+ .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, e -> e.getValue().get()));
+ // Filter further so we match both email & name
+ if (possibleReplacements.size() > 1) {
+ logger.atWarning().log(
+ "Fixing ref %s, multiple accounts found with the same email address, while replacing %s",
+ changeFixProgress.changeMetaRef, accountInfo);
+ possibleReplacements =
+ possibleReplacements.entrySet().stream()
+ .filter(e -> Objects.equals(e.getValue().account().getName(), accountInfo.name()))
+ .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
+ }
+ }
+ if (possibleReplacements.isEmpty()) {
+ possibleReplacements =
+ changeFixProgress.parsedAccounts.entrySet().stream()
+ .filter(
+ e ->
+ e.getValue().isPresent()
+ && Objects.equals(
+ e.getValue().get().account().getName(), accountInfo.name()))
+ .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, e -> e.getValue().get()));
+ }
String replacementName = DEFAULT_ACCOUNT_REPLACEMENT;
if (possibleReplacements.isEmpty()) {
- logger.atWarning().log("Could not find reviewer account matching name %s", accountName);
+ logger.atWarning().log(
+ "Fixing ref %s, could not find reviewer account matching name %s",
+ changeFixProgress.changeMetaRef, accountInfo);
} else if (possibleReplacements.size() > 1) {
- logger.atWarning().log("Found multiple reviewer account matching name %s", accountName);
+ logger.atWarning().log(
+ "Fixing ref %s found multiple reviewer account matching name %s",
+ changeFixProgress.changeMetaRef, accountInfo);
} else {
replacementName =
- AccountTemplateUtil.getAccountTemplate(Iterables.getOnlyElement(possibleReplacements));
+ AccountTemplateUtil.getAccountTemplate(
+ Iterables.getOnlyElement(possibleReplacements.keySet()));
}
return replacementName;
}
@@ -1135,6 +1190,10 @@
* recent update.
*/
private static class ChangeFixProgress {
+
+ /** {@link RefNames#changeMetaRef} of the change that is being fixed. */
+ final String changeMetaRef;
+
/** Assignee at current commit update. */
Account.Id assigneeId = null;
@@ -1146,7 +1205,7 @@
* #accountCache} if needed by rewrite. Maps to empty string if was not requested from cache
* yet.
*/
- Map<Account.Id, String> parsedAccounts = new HashMap<>();
+ Map<Account.Id, Optional<AccountState>> parsedAccounts = new HashMap<>();
/** Id of the current commit in rewriter walk. */
ObjectId newTipId = null;
@@ -1160,5 +1219,29 @@
boolean isValidAfterFix = true;
List<CommitDiff> commitDiffs = new ArrayList<>();
+
+ public ChangeFixProgress(String changeMetaRef) {
+ this.changeMetaRef = changeMetaRef;
+ }
+ }
+
+ /**
+ * Account info parsed from {@link Account#getNameEmail}. See {@link
+ * #getAccountInfoFromNameEmail}.
+ */
+ @AutoValue
+ abstract static class ParsedAccountInfo {
+
+ static ParsedAccountInfo create(String fullName, String email) {
+ return new AutoValue_CommitRewriter_ParsedAccountInfo(fullName, Optional.ofNullable(email));
+ }
+
+ static ParsedAccountInfo create(String fullName) {
+ return new AutoValue_CommitRewriter_ParsedAccountInfo(fullName, Optional.empty());
+ }
+
+ abstract String name();
+
+ abstract Optional<String> email();
}
}
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index 26e1881..f316660 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -837,6 +837,118 @@
}
@Test
+ public void fixRemoveVoteChangeMessageWithNoFooterLabel_matchByEmail() throws Exception {
+ Change c = newChange();
+ ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
+ approvalUpdate.putApproval(VERIFIED, (short) +2);
+
+ approvalUpdate.putApprovalFor(otherUserId, VERIFIED, (short) -1);
+ approvalUpdate.commit();
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified+2 by Renamed Change Owner <change@owner.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ BackfillResult result = rewriter.backfillProject(project, repo, options);
+ assertThat(result.fixedRefDiff.keySet()).containsExactly(RefNames.changeMetaRef(c.getId()));
+
+ List<String> commitHistoryDiff = commitHistoryDiff(result, c.getId());
+ assertThat(commitHistoryDiff)
+ .containsExactly(
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Renamed Change Owner <change@owner.com>\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_1>\n");
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ assertThat(secondRunResult.fixedRefDiff.keySet()).isEmpty();
+ assertThat(secondRunResult.refsFailedToFix).isEmpty();
+ }
+
+ @Test
+ public void fixRemoveVoteChangeMessageWithNoFooterLabel_matchByName() throws Exception {
+ Change c = newChange();
+ ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
+ approvalUpdate.putApproval(VERIFIED, (short) +2);
+
+ approvalUpdate.putApprovalFor(otherUserId, VERIFIED, (short) -1);
+ approvalUpdate.commit();
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(c, /*changeMessage=*/ "Removed Verified+2 by Change Owner"),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ BackfillResult result = rewriter.backfillProject(project, repo, options);
+ assertThat(result.fixedRefDiff.keySet()).containsExactly(RefNames.changeMetaRef(c.getId()));
+
+ List<String> commitHistoryDiff = commitHistoryDiff(result, c.getId());
+ assertThat(commitHistoryDiff)
+ .containsExactly(
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Change Owner\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_1>\n");
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ assertThat(secondRunResult.fixedRefDiff.keySet()).isEmpty();
+ assertThat(secondRunResult.refsFailedToFix).isEmpty();
+ }
+
+ @Test
+ public void fixRemoveVoteChangeMessageWithNoFooterLabel_matchDuplicateAccounts()
+ throws Exception {
+ Account duplicateCodeOwner =
+ Account.builder(Account.id(4), TimeUtil.nowTs())
+ .setFullName(changeOwner.getName())
+ .setPreferredEmail("other@test.com")
+ .build();
+ accountCache.put(duplicateCodeOwner);
+ Change c = newChange();
+ ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
+ approvalUpdate.putApproval(VERIFIED, (short) +2);
+
+ approvalUpdate.putApprovalFor(duplicateCodeOwner.id(), VERIFIED, (short) -1);
+ approvalUpdate.commit();
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified+2 by Change Owner <other@test.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified+2 by Change Owner <change@owner.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified-1 by Change Owner <other@test.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ BackfillResult result = rewriter.backfillProject(project, repo, options);
+ assertThat(result.fixedRefDiff.keySet()).containsExactly(RefNames.changeMetaRef(c.getId()));
+
+ List<String> commitHistoryDiff = commitHistoryDiff(result, c.getId());
+ assertThat(commitHistoryDiff)
+ .containsExactly(
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Change Owner <other@test.com>\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_4>\n",
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Change Owner <change@owner.com>\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_1>\n",
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified-1 by Change Owner <other@test.com>\n"
+ + "+Removed Verified-1 by <GERRIT_ACCOUNT_4>\n");
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ assertThat(secondRunResult.fixedRefDiff.keySet()).isEmpty();
+ assertThat(secondRunResult.refsFailedToFix).isEmpty();
+ }
+
+ @Test
public void fixRemoveVotesChangeMessage() throws Exception {
Change c = newChange();
ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
