opendev/gerrit
Code Issues Proposed changes

Invalid subject for RevertSubmission

Browse Source 
Change I77949ad9f introduced a bug: When reverting multiple changes,
revertInput.message was used mutliple times and resulted in commit
messages that don't make sense, such as "Revert first change \n Revert
second change". It happened because the same variable was assigned and
the first line was added to it each revert.

The solution was very simple, just not to use the same variable multiple
times.

Change-Id: Ia6b45f786c54b634adad46dd910089f31811f7ed
This commit is contained in:
Gal Paikin
2019-12-23 15:29:01 +01:00
parent 9bf7a5130e
commit 0b5ab52226
2 changed files with 41 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
java/com/google/gerrit/server/restapi/change/RevertSubmission.java
View File

@@ -266,13 +266,14 @@ public class RevertSubmission
NoSuchProjectException, ConfigInvalidException {
String groupName = null;
String initialMessage = revertInput.message;
while (sortedChangesInProjectAndBranch.hasNext()) {
ChangeNotes changeNotes = sortedChangesInProjectAndBranch.next().data().notes();
if (cherryPickInput.base == null) {
cherryPickInput.base = getBase(changeNotes, commitIdsInProjectAndBranch).name();
}
revertInput.message = getMessage(revertInput, changeNotes);
revertInput.message = getMessage(initialMessage, changeNotes);
if (cherryPickInput.base.equals(changeNotes.getCurrentPatchSet().commitId().getName())) {
// This is the code in case this is the first revert of this project + branch, and the
// revert would be on top of the change being reverted.
@@ -348,19 +349,19 @@ public class RevertSubmission
return cherryPickInput;
}
private String getMessage(RevertInput revertInput, ChangeNotes changeNotes) {
private String getMessage(String initialMessage, ChangeNotes changeNotes) {
String subject = changeNotes.getChange().getSubject();
if (subject.length() > 63) {
subject = subject.substring(0, 59) + "...";
}
if (revertInput.message == null) {
if (initialMessage == null) {
return MessageFormat.format(
ChangeMessages.get().revertChangeDefaultMessage,
subject,
changeNotes.getCurrentPatchSet().commitId().name());
}
return String.format("Revert \"%s\"\n\n%s", subject, revertInput.message);
return String.format("Revert \"%s\"\n\n%s", subject, initialMessage);
}
/**

54
javatests/com/google/gerrit/acceptance/api/change/RevertIT.java
View File

@@ -693,36 +693,54 @@ public class RevertIT extends AbstractDaemonTest {
@Test
public void revertSubmissionWithSetMessage() throws Exception {
String result = createChange("change", "a.txt", "message").getChangeId();
gApi.changes().id(result).current().review(ReviewInput.approve());
gApi.changes().id(result).current().submit();
String firstResult = createChange("first change", "a.txt", "message").getChangeId();
String secondResult = createChange("second change", "b.txt", "message").getChangeId();
approve(firstResult);
approve(secondResult);
gApi.changes().id(secondResult).current().submit();
RevertInput revertInput = new RevertInput();
String commitMessage = "Message from input";
revertInput.message = commitMessage;
ChangeInfo revertChange =
gApi.changes().id(result).revertSubmission(revertInput).revertChanges.get(0);
assertThat(revertChange.subject).isEqualTo("Revert \"change\"");
assertThat(gApi.changes().id(revertChange.id).current().commit(false).message)
List<ChangeInfo> revertChanges =
gApi.changes().id(firstResult).revertSubmission(revertInput).revertChanges;
assertThat(revertChanges.get(0).subject).isEqualTo("Revert \"first change\"");
assertThat(gApi.changes().id(revertChanges.get(0).id).current().commit(false).message)
.isEqualTo(
String.format(
"Revert \"change\"\n\n%s\n\nChange-Id: %s\n",
commitMessage, revertChange.changeId));
"Revert \"first change\"\n\n%s\n\nChange-Id: %s\n",
commitMessage, revertChanges.get(0).changeId));
assertThat(revertChanges.get(1).subject).isEqualTo("Revert \"second change\"");
assertThat(gApi.changes().id(revertChanges.get(1).id).current().commit(false).message)
.isEqualTo(
String.format(
"Revert \"second change\"\n\n%s\n\nChange-Id: %s\n",
commitMessage, revertChanges.get(1).changeId));
}
@Test
public void revertSubmissionWithoutMessage() throws Exception {
String result = createChange("change", "a.txt", "message").getChangeId();
gApi.changes().id(result).current().review(ReviewInput.approve());
gApi.changes().id(result).current().submit();
String firstResult = createChange("first change", "a.txt", "message").getChangeId();
String secondResult = createChange("second change", "b.txt", "message").getChangeId();
approve(firstResult);
approve(secondResult);
gApi.changes().id(secondResult).current().submit();
RevertInput revertInput = new RevertInput();
ChangeInfo revertChange =
gApi.changes().id(result).revertSubmission(revertInput).revertChanges.get(0);
assertThat(revertChange.subject).isEqualTo("Revert \"change\"");
assertThat(gApi.changes().id(revertChange.id).current().commit(false).message)
List<ChangeInfo> revertChanges =
gApi.changes().id(firstResult).revertSubmission(revertInput).revertChanges;
assertThat(revertChanges.get(0).subject).isEqualTo("Revert \"first change\"");
assertThat(gApi.changes().id(revertChanges.get(0).id).current().commit(false).message)
.isEqualTo(
String.format(
"Revert \"change\"\n\nThis reverts commit %s.\n\nChange-Id: %s\n",
gApi.changes().id(result).get().currentRevision, revertChange.changeId));
"Revert \"first change\"\n\nThis reverts commit %s.\n\nChange-Id: %s\n",
gApi.changes().id(firstResult).get().currentRevision,
revertChanges.get(0).changeId));
assertThat(revertChanges.get(1).subject).isEqualTo("Revert \"second change\"");
assertThat(gApi.changes().id(revertChanges.get(1).id).current().commit(false).message)
.isEqualTo(
String.format(
"Revert \"second change\"\n\nThis reverts commit %s.\n\nChange-Id: %s\n",
gApi.changes().id(secondResult).get().currentRevision,
revertChanges.get(1).changeId));
}
@Test