Change RevertSubmission's subject for revert of reverts

Currently, if a user tries reverting a submission, the subject for all
changes of that new revert submission will be 'Revert "Change x"'.
Also, if you try to revert that newly created submission, the subject
for all changes of that new revert submission will be 'Revert "Revert
"Change x""' (while "Change x" depends on the change).

This is clearly not good, since it doesn't scale well with 2 or more
reverts. This change makes it so that it never shows "Revert Revert" for
changes, but rather changes the convention to "Revert^N". E.g, instead
of 'Revert "Revert "Change x""' the subject is 'Revert^2 "Change x"'.

Also, the limit for length of subject was reduced by 3 to 60 max length
to account for the addition of "^NN" in the subject.

Another problem is the fact that Git's default for Git Revert is the same
as the current behavior in Gerrit ("Revert Revert Change X"). I'm trying
to get in touch with them to change the default behavior. For now, it's
okay if I make this change specifically for RevertSubmission, and keep
Revert functionality for both Git and Gerrit consistent (the cost
is that Revert Submission and Revert are not very consistent this way).

Change-Id: Id5e4b9ab20165548b24aec9b9046ff5a9a9c4e7b

Documentation/rest-api-changes.txt

@@ -1520,8 +1520,14 @@ the error message is contained in the response body.
 Creates open revert changes for all of the changes of a certain submission.
 
 The subject of each revert change will be 'Revert "<subject-of-reverted-change"'.
-If the subject is above 63 characters, the subject will be cut to 59 characters
-with "..." in the end.
+If the subject is above 60 characters, the subject will be cut to 56 characters
+with "..." in the end. However, whenever reverting the submission of a revert
+submission, the subject will be shortened from
+'Revert "Revert "<subject-of-reverted-change""' to
+'Revert^2 "<subject-of-reverted-change"'. Also, for every future revert submission,
+the only difference in the subject will be the number of the revert (instead of
+Revert^2 the subject will change to Revert^3 and so on).
+There are no guarantees about the subjects if the users change the default subjects.
 
 Details for the revert can be specified in the request body inside a link:#revert-input[
 RevertInput] The topic of all created revert changes will be

java/com/google/gerrit/server/change/ChangeMessages.java

@@ -23,6 +23,9 @@ public class ChangeMessages extends TranslationBundle {
   }
 
   public String revertChangeDefaultMessage;
+  public String revertSubmissionDefaultMessage;
+  public String revertSubmissionUserMessage;
+  public String revertSubmissionOfRevertSubmissionUserMessage;
 
   public String reviewerCantSeeChange;
   public String reviewerInvalid;

java/com/google/gerrit/server/restapi/change/RevertSubmission.java

@@ -85,6 +85,8 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Queue;
 import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import org.apache.commons.lang.RandomStringUtils;
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -120,6 +122,9 @@ public class RevertSubmission
 
   private CherryPickInput cherryPickInput;
   private List<ChangeInfo> results;
+  private static final Pattern patternRevertSubject = Pattern.compile("Revert \"(.+)\"");
+  private static final Pattern patternRevertSubjectWithNum =
+      Pattern.compile("Revert\\^(\\d+) \"(.+)\"");
 
   @Inject
   RevertSubmission(
@@ -352,17 +357,43 @@ public class RevertSubmission
 
   private String getMessage(String initialMessage, ChangeNotes changeNotes) {
     String subject = changeNotes.getChange().getSubject();
-    if (subject.length() > 63) {
-      subject = subject.substring(0, 59) + "...";
+    if (subject.length() > 60) {
+      subject = subject.substring(0, 56) + "...";
     }
     if (initialMessage == null) {
+      initialMessage =
+          MessageFormat.format(
+              ChangeMessages.get().revertSubmissionDefaultMessage,
+              changeNotes.getCurrentPatchSet().commitId().name());
+    }
+
+    // For performance purposes: Almost all cases will end here.
+    if (!subject.startsWith("Revert")) {
       return MessageFormat.format(
-          ChangeMessages.get().revertChangeDefaultMessage,
-          subject,
+          ChangeMessages.get().revertSubmissionUserMessage, subject, initialMessage);
+    }
+
+    Matcher matcher = patternRevertSubjectWithNum.matcher(subject);
+
+    if (matcher.matches()) {
+      return MessageFormat.format(
+          ChangeMessages.get().revertSubmissionOfRevertSubmissionUserMessage,
+          Integer.valueOf(matcher.group(1)) + 1,
+          matcher.group(2),
           changeNotes.getCurrentPatchSet().commitId().name());
     }
 
-    return String.format("Revert \"%s\"\n\n%s", subject, initialMessage);
+    matcher = patternRevertSubject.matcher(subject);
+    if (matcher.matches()) {
+      return MessageFormat.format(
+          ChangeMessages.get().revertSubmissionOfRevertSubmissionUserMessage,
+          2,
+          matcher.group(1),
+          changeNotes.getCurrentPatchSet().commitId().name());
+    }
+
+    return MessageFormat.format(
+        ChangeMessages.get().revertSubmissionUserMessage, subject, initialMessage);
   }
 
   /**

javatests/com/google/gerrit/acceptance/api/change/RevertIT.java

@@ -746,7 +746,7 @@ public class RevertIT extends AbstractDaemonTest {
   @Test
   public void revertSubmissionRevertsChangeWithLongSubject() throws Exception {
     String changeTitle =
-        "This change has a very long title and therefore it will be cut to 50 characters when the"
+        "This change has a very long title and therefore it will be cut to 56 characters when the"
             + " revert change will revert this change";
     String result = createChange(changeTitle, "a.txt", "message").getChangeId();
     gApi.changes().id(result).current().review(ReviewInput.approve());
@@ -755,12 +755,12 @@ public class RevertIT extends AbstractDaemonTest {
     ChangeInfo revertChange =
         gApi.changes().id(result).revertSubmission(revertInput).revertChanges.get(0);
     assertThat(revertChange.subject)
-        .isEqualTo(String.format("Revert \"%s...\"", changeTitle.substring(0, 59)));
+        .isEqualTo(String.format("Revert \"%s...\"", changeTitle.substring(0, 56)));
     assertThat(gApi.changes().id(revertChange.id).current().commit(false).message)
         .isEqualTo(
             String.format(
                 "Revert \"%s...\"\n\nThis reverts commit %s.\n\nChange-Id: %s\n",
-                changeTitle.substring(0, 59),
+                changeTitle.substring(0, 56),
                 gApi.changes().id(result).get().currentRevision,
                 revertChange.changeId));
   }
@@ -1118,6 +1118,56 @@ public class RevertIT extends AbstractDaemonTest {
     assertThat(gApi.changes().id(revertChanges.get(1).id()).current().related().changes).hasSize(3);
   }
 
+  @Test
+  public void revertSubmissionSubjects() throws Exception {
+    String firstResult = createChange("first change", "a.txt", "message").getChangeId();
+    String secondResult = createChange("second change", "b.txt", "other").getChangeId();
+    approve(firstResult);
+    approve(secondResult);
+    gApi.changes().id(secondResult).current().submit();
+
+    List<ChangeApi> firstRevertChanges =
+        getChangeApis(gApi.changes().id(firstResult).revertSubmission());
+    assertThat(firstRevertChanges.get(0).get().subject).isEqualTo("Revert \"first change\"");
+    assertThat(firstRevertChanges.get(1).get().subject).isEqualTo("Revert \"second change\"");
+    approve(firstRevertChanges.get(0).id());
+    approve(firstRevertChanges.get(1).id());
+    gApi.changes().id(firstRevertChanges.get(0).id()).current().submit();
+
+    List<ChangeApi> secondRevertChanges =
+        getChangeApis(gApi.changes().id(firstRevertChanges.get(0).id()).revertSubmission());
+    assertThat(secondRevertChanges.get(0).get().subject).isEqualTo("Revert^2 \"second change\"");
+    assertThat(secondRevertChanges.get(1).get().subject).isEqualTo("Revert^2 \"first change\"");
+    approve(secondRevertChanges.get(0).id());
+    approve(secondRevertChanges.get(1).id());
+    gApi.changes().id(secondRevertChanges.get(0).id()).current().submit();
+
+    List<ChangeApi> thirdRevertChanges =
+        getChangeApis(gApi.changes().id(secondRevertChanges.get(0).id()).revertSubmission());
+    assertThat(thirdRevertChanges.get(0).get().subject).isEqualTo("Revert^3 \"first change\"");
+    assertThat(thirdRevertChanges.get(1).get().subject).isEqualTo("Revert^3 \"second change\"");
+  }
+
+  @Test
+  public void revertSubmissionWithUserChangedSubjects() throws Exception {
+    String firstResult = createChange("Revert^aa", "a.txt", "message").getChangeId();
+    String secondResult = createChange("Revert", "b.txt", "other").getChangeId();
+    String thirdResult = createChange("Revert^934 \"change x\"", "c.txt", "another").getChangeId();
+    String fourthResult = createChange("Revert^934", "d.txt", "last").getChangeId();
+    approve(firstResult);
+    approve(secondResult);
+    approve(thirdResult);
+    approve(fourthResult);
+    gApi.changes().id(fourthResult).current().submit();
+
+    List<ChangeApi> firstRevertChanges =
+        getChangeApis(gApi.changes().id(firstResult).revertSubmission());
+    assertThat(firstRevertChanges.get(0).get().subject).isEqualTo("Revert \"Revert^aa\"");
+    assertThat(firstRevertChanges.get(1).get().subject).isEqualTo("Revert \"Revert\"");
+    assertThat(firstRevertChanges.get(2).get().subject).isEqualTo("Revert^935 \"change x\"");
+    assertThat(firstRevertChanges.get(3).get().subject).isEqualTo("Revert \"Revert^934\"");
+  }
+
   @Override
   protected PushOneCommit.Result createChange() throws Exception {
     return createChange("refs/for/master");

resources/com/google/gerrit/server/change/ChangeMessages.properties

@@ -1,4 +1,7 @@
 revertChangeDefaultMessage = Revert \"{0}\"\n\nThis reverts commit {1}.
+revertSubmissionDefaultMessage = This reverts commit {0}.
+revertSubmissionUserMessage = Revert \"{0}\"\n\n{1}
+revertSubmissionOfRevertSubmissionUserMessage = Revert^{0} \"{1}\"\n\n{2}
 
 reviewerCantSeeChange = {0} does not have permission to see this change
 reviewerInvalid = {0} is not a valid user identifier