Merge "Fix Gerrit double-revert subject and message formatting"
diff --git a/java/com/google/gerrit/server/change/ChangeMessages.java b/java/com/google/gerrit/server/change/ChangeMessages.java
index c228ebe..aef39b1 100644
--- a/java/com/google/gerrit/server/change/ChangeMessages.java
+++ b/java/com/google/gerrit/server/change/ChangeMessages.java
@@ -18,7 +18,8 @@
public static String revertChangeDefaultMessage = "Revert \"{0}\"\n\nThis reverts commit {1}.";
public static String revertSubmissionDefaultMessage = "This reverts commit {0}.";
public static String revertSubmissionUserMessage = "Revert \"{0}\"\n\n{1}";
- public static String revertSubmissionOfRevertSubmissionUserMessage = "Revert^{0} \"{1}\"\n\n{2}";
+ public static String revertSubmissionOfRevertSubmissionUserMessage =
+ "Revert^{0} \"{1}\"\n\n{2}\n\n{3}";
public static String reviewerCantSeeChange = "{0} does not have permission to see this change";
public static String reviewerInvalid = "{0} is not a valid user identifier";
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index 291d5a9..c9c090d 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -74,6 +74,8 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.InvalidObjectIdException;
@@ -96,6 +98,11 @@
public class CommitUtil {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private static final Pattern patternRevertSubject = Pattern.compile("Revert \"(.+)\"");
+ private static final Pattern patternRevertSubjectWithNum =
+ Pattern.compile("Revert\\^(\\d+) \"(.+)\"");
+ private static final Pattern FOOTER_PATTERN = Pattern.compile("^[a-zA-Z0-9-]+:.*");
+
private final GitRepositoryManager repoManager;
private final Provider<PersonIdent> serverIdent;
private final Sequences seq;
@@ -245,7 +252,7 @@
return id;
}
- public static String getBugAndIssueFooters(RevCommit commit) {
+ private static String getBugAndIssueFooters(RevCommit commit) {
StringBuilder footers = new StringBuilder();
for (FooterLine footerLine : commit.getFooterLines()) {
String key = footerLine.getKey();
@@ -296,26 +303,22 @@
Change changeToRevert = notes.getChange();
String subject = changeToRevert.getSubject();
- if (subject.length() > 63) {
- subject = subject.substring(0, 59) + "...";
- }
- if (message == null) {
- message =
- MessageFormat.format(
- ChangeMessages.revertChangeDefaultMessage, subject, patch.commitId().name());
- }
+
+ message = getRevertMessage(message, subject, patch.commitId().name(), false);
String newFooters = getBugAndIssueFooters(commitToRevert);
if (!newFooters.isEmpty()) {
+ Set<String> messageLines =
+ Arrays.stream(message.split("\n"))
+ .map(String::trim)
+ .map(String::toLowerCase)
+ .collect(Collectors.toSet());
StringBuilder footersToAdd = new StringBuilder();
for (String footer : Splitter.on('\n').split(newFooters)) {
if (footer.isEmpty()) {
continue;
}
- boolean alreadyExists =
- Arrays.stream(message.split("\n"))
- .anyMatch(line -> line.trim().equalsIgnoreCase(footer.trim()));
- if (!alreadyExists) {
+ if (!messageLines.contains(footer.trim().toLowerCase())) {
footersToAdd.append(footer).append("\n");
}
}
@@ -324,7 +327,7 @@
String trimmedMsg = message.trim();
List<String> lines = Splitter.on('\n').splitToList(trimmedMsg);
String lastLine = lines.isEmpty() ? "" : lines.get(lines.size() - 1);
- boolean endsWithFooter = lastLine.matches("^[a-zA-Z0-9-]+:.*");
+ boolean endsWithFooter = FOOTER_PATTERN.matcher(lastLine).matches();
if (endsWithFooter) {
message = trimmedMsg + "\n" + footersToAdd.toString().trim();
@@ -356,6 +359,47 @@
return revertCommit;
}
+ public String getRevertMessage(
+ @Nullable String initialMessage, String subject, String commitId, boolean isSubmission) {
+ if (isSubmission) {
+ if (subject.length() > 60) {
+ subject = subject.substring(0, 56) + "...";
+ }
+ } else {
+ if (subject.length() > 63) {
+ subject = subject.substring(0, 59) + "...";
+ }
+ }
+
+ Matcher matcher = patternRevertSubjectWithNum.matcher(subject);
+ boolean matchesNum = matcher.matches();
+ if (!matchesNum) {
+ matcher = patternRevertSubject.matcher(subject);
+ }
+
+ if (matchesNum || matcher.matches()) {
+ int nextNum = matchesNum ? Integer.parseInt(matcher.group(1)) + 1 : 2;
+ String originalSubject = matchesNum ? matcher.group(2) : matcher.group(1);
+
+ return MessageFormat.format(
+ ChangeMessages.revertSubmissionOfRevertSubmissionUserMessage,
+ nextNum,
+ originalSubject,
+ MessageFormat.format(ChangeMessages.revertSubmissionDefaultMessage, commitId),
+ initialMessage != null ? initialMessage : "");
+ }
+
+ if (initialMessage != null) {
+ if (isSubmission) {
+ return MessageFormat.format(
+ ChangeMessages.revertSubmissionUserMessage, subject, initialMessage);
+ }
+ return initialMessage;
+ }
+
+ return MessageFormat.format(ChangeMessages.revertChangeDefaultMessage, subject, commitId);
+ }
+
private Change.Id createRevertChangeFromCommit(
CodeReviewCommit revertCommit,
RevertInput input,
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 35d0bf6..3d37424 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -48,7 +48,6 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.change.ChangeJson;
-import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.RevisionResource;
@@ -77,7 +76,6 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
-import java.text.MessageFormat;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Arrays;
@@ -87,8 +85,6 @@
import java.util.Locale;
import java.util.Optional;
import java.util.Set;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.apache.commons.lang3.RandomStringUtils;
import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -124,9 +120,6 @@
private CherryPickInput cherryPickInput;
private Change cherryPickBaseChange;
private List<ChangeInfo> results;
- private static final Pattern patternRevertSubject = Pattern.compile("Revert \"(.+)\"");
- private static final Pattern patternRevertSubjectWithNum =
- Pattern.compile("Revert\\^(\\d+) \"(.+)\"");
@Inject
RevertSubmission(
@@ -384,44 +377,11 @@
}
private String getMessage(String initialMessage, ChangeNotes changeNotes) {
- String subject = changeNotes.getChange().getSubject();
- if (subject.length() > 60) {
- subject = subject.substring(0, 56) + "...";
- }
- if (initialMessage == null) {
- initialMessage =
- MessageFormat.format(
- ChangeMessages.revertSubmissionDefaultMessage,
- changeNotes.getCurrentPatchSet().commitId().name());
- }
-
- // For performance purposes: Almost all cases will end here.
- if (!subject.startsWith("Revert")) {
- return MessageFormat.format(
- ChangeMessages.revertSubmissionUserMessage, subject, initialMessage);
- }
-
- Matcher matcher = patternRevertSubjectWithNum.matcher(subject);
-
- if (matcher.matches()) {
- return MessageFormat.format(
- ChangeMessages.revertSubmissionOfRevertSubmissionUserMessage,
- Integer.valueOf(matcher.group(1)) + 1,
- matcher.group(2),
- changeNotes.getCurrentPatchSet().commitId().name());
- }
-
- matcher = patternRevertSubject.matcher(subject);
- if (matcher.matches()) {
- return MessageFormat.format(
- ChangeMessages.revertSubmissionOfRevertSubmissionUserMessage,
- 2,
- matcher.group(1),
- changeNotes.getCurrentPatchSet().commitId().name());
- }
-
- return MessageFormat.format(
- ChangeMessages.revertSubmissionUserMessage, subject, initialMessage);
+ return commitUtil.getRevertMessage(
+ initialMessage,
+ changeNotes.getChange().getSubject(),
+ changeNotes.getCurrentPatchSet().commitId().name(),
+ true);
}
/**
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
index c849583..6de06b0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
@@ -348,6 +348,33 @@
}
@Test
+ public void revertOfRevertWithSetMessage() throws Exception {
+ PushOneCommit.Result result = createChange();
+ gApi.changes().id(result.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(result.getChangeId()).revision(result.getCommit().name()).submit();
+
+ // First revert
+ ChangeInfo revertChange = gApi.changes().id(result.getChangeId()).revert().get();
+ gApi.changes().id(revertChange.id).current().review(ReviewInput.approve());
+ gApi.changes().id(revertChange.id).current().submit();
+
+ // Revert of revert with custom message
+ RevertInput revertInput = new RevertInput();
+ revertInput.message = "Custom message for double revert";
+ ChangeInfo doubleRevertChange = gApi.changes().id(revertChange.id).revert(revertInput).get();
+
+ String actualSubject = doubleRevertChange.subject;
+ String commitMessage = gApi.changes().id(doubleRevertChange.id).current().commit(false).message;
+
+ assertThat(actualSubject)
+ .isEqualTo("Revert^2 \"" + result.getChange().change().getSubject() + "\"");
+ assertThat(commitMessage)
+ .startsWith("Revert^2 \"" + result.getChange().change().getSubject() + "\"");
+ assertThat(commitMessage).contains("Custom message for double revert");
+ assertThat(commitMessage).contains("This reverts commit " + revertChange.currentRevision);
+ }
+
+ @Test
public void revertWithSetMessageChangeIdIgnored() throws Exception {
PushOneCommit.Result result = createChange();
gApi.changes().id(result.getChangeId()).current().review(ReviewInput.approve());
