diff --git a/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java index 1e4b733ba0..13392d8068 100644 --- a/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java +++ b/java/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImpl.java @@ -348,7 +348,7 @@ public class ChangeOperationsImpl implements ChangeOperations { ChangeNotes changeNotes, TestPatchsetCreation patchsetCreation, Timestamp now) - throws IOException { + throws IOException, BadRequestException { ObjectId oldPatchsetCommitId = changeNotes.getCurrentPatchSet().commitId(); RevCommit oldPatchsetCommit = repository.parseCommit(oldPatchsetCommitId); @@ -356,7 +356,10 @@ public class ChangeOperationsImpl implements ChangeOperations { createNewTree( repository, revWalk, oldPatchsetCommitId, patchsetCreation.treeModifications()); - String commitMessage = oldPatchsetCommit.getFullMessage(); + String commitMessage = + correctCommitMessage( + changeNotes.getChange().getKey().get(), + patchsetCreation.commitMessage().orElseGet(oldPatchsetCommit::getFullMessage)); ImmutableList parentCommitIds = getParents(oldPatchsetCommit); PersonIdent author = getAuthor(oldPatchsetCommit); @@ -364,6 +367,19 @@ public class ChangeOperationsImpl implements ChangeOperations { return createCommit(objectInserter, tree, parentCommitIds, author, committer, commitMessage); } + private String correctCommitMessage(String oldChangeId, String desiredCommitMessage) + throws BadRequestException { + String commitMessage = CommitMessageUtil.checkAndSanitizeCommitMessage(desiredCommitMessage); + + // Remove initial 'I' and treat the rest as ObjectId. This is not the cleanest approach but + // unfortunately, we don't seem to have other utility code which takes the string-based + // change-id and ensures that it is part of the commit message. + ObjectId id = ObjectId.fromString(oldChangeId.substring(1)); + commitMessage = ChangeIdUtil.insertId(commitMessage, id, false); + + return commitMessage; + } + private PersonIdent getAuthor(RevCommit oldPatchsetCommit) { return Optional.ofNullable(oldPatchsetCommit.getAuthorIdent()).orElse(serverIdent); } diff --git a/java/com/google/gerrit/acceptance/testsuite/change/TestPatchsetCreation.java b/java/com/google/gerrit/acceptance/testsuite/change/TestPatchsetCreation.java index 571b1e12fe..0152cd155b 100644 --- a/java/com/google/gerrit/acceptance/testsuite/change/TestPatchsetCreation.java +++ b/java/com/google/gerrit/acceptance/testsuite/change/TestPatchsetCreation.java @@ -19,11 +19,14 @@ import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.testsuite.ThrowingFunction; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.server.edit.tree.TreeModification; +import java.util.Optional; /** Initial attributes of the patchset. If not provided, arbitrary values will be used. */ @AutoValue public abstract class TestPatchsetCreation { + public abstract Optional commitMessage(); + public abstract ImmutableList treeModifications(); abstract ThrowingFunction patchsetCreator(); @@ -36,6 +39,8 @@ public abstract class TestPatchsetCreation { @AutoValue.Builder public abstract static class Builder { + public abstract Builder commitMessage(String commitMessage); + /** Modified file of the patchset. The file content is specified via the returned builder. */ public FileContentBuilder file(String filePath) { return new FileContentBuilder<>(this, filePath, treeModificationsBuilder()::add); diff --git a/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java index 485a00740c..e293edf3f1 100644 --- a/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java +++ b/javatests/com/google/gerrit/acceptance/testsuite/change/ChangeOperationsImplTest.java @@ -362,6 +362,58 @@ public class ChangeOperationsImplTest extends AbstractDaemonTest { assertThat(patchsetRevision.kind).isEqualTo(ChangeKind.NO_CHANGE); } + @Test + public void newPatchsetCanHaveUpdatedCommitMessage() throws Exception { + Change.Id changeId = changeOperations.newChange().commitMessage("Old message").create(); + + changeOperations.change(changeId).newPatchset().commitMessage("New message").create(); + + ChangeInfo change = getChangeFromServer(changeId); + CommitInfo currentPatchsetCommit = change.revisions.get(change.currentRevision).commit; + assertThat(currentPatchsetCommit).message().startsWith("New message"); + } + + @Test + public void updatedCommitMessageOfNewPatchsetAutomaticallyKeepsChangeId() throws Exception { + Change.Id numericChangeId = changeOperations.newChange().commitMessage("Old message").create(); + String changeId = changeOperations.change(numericChangeId).get().changeId(); + + changeOperations.change(numericChangeId).newPatchset().commitMessage("New message").create(); + + ChangeInfo change = getChangeFromServer(numericChangeId); + CommitInfo currentPatchsetCommit = change.revisions.get(change.currentRevision).commit; + assertThat(currentPatchsetCommit).message().contains("Change-Id: " + changeId); + } + + @Test + public void newPatchsetCanHaveDifferentChangeIdFooter() throws Exception { + Change.Id numericChangeId = + changeOperations + .newChange() + .commitMessage("Old message\n\nChange-Id: I1111111111111111111111111111111111111111") + .create(); + + // Specifying another change-id is not an officially supported behavior of Gerrit but we might + // need this for some test scenarios and hence we support it in the test API. + changeOperations + .change(numericChangeId) + .newPatchset() + .commitMessage("New message\n\nChange-Id: I0123456789012345678901234567890123456789") + .create(); + + ChangeInfo change = getChangeFromServer(numericChangeId); + CommitInfo currentPatchsetCommit = change.revisions.get(change.currentRevision).commit; + assertThat(currentPatchsetCommit) + .message() + .contains("Change-Id: I0123456789012345678901234567890123456789"); + assertThat(currentPatchsetCommit) + .message() + .doesNotContain("Change-Id: I1111111111111111111111111111111111111111"); + // Actual change-id should not have been updated. + String changeId = changeOperations.change(numericChangeId).get().changeId(); + assertThat(changeId).isEqualTo("I1111111111111111111111111111111111111111"); + } + @Test public void newPatchsetCanHaveReplacedFileContent() throws Exception { Change.Id changeId = changeOperations.newChange().file("file1").content("Line 1").create();