From 3058abceb8052ad6dcd6482db1cc41bf3675fd51 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 22 Jun 2017 16:52:58 +0200 Subject: [PATCH] Properly check commit message for equality This commit also moves the tests from RevisionIT to ChangeIT since there is no revision endpoint anymore and adds a test for equal commit messages. Change-Id: I17796e56273d1fd0664b94bcbb15da554525fc99 --- .../acceptance/api/change/ChangeIT.java | 93 +++++++++++++++++++ .../acceptance/api/revision/RevisionIT.java | 89 ------------------ .../gerrit/server/change/PutMessage.java | 2 +- 3 files changed, 94 insertions(+), 90 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index e4f8ff292d..8607c02dc4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -118,6 +118,7 @@ import com.google.gerrit.server.update.ChangeContext; import com.google.gerrit.testutil.FakeEmailSender.Message; import com.google.gerrit.testutil.TestTimeUtil; import com.google.inject.Inject; +import java.io.IOException; import java.sql.Timestamp; import java.util.ArrayList; import java.util.Arrays; @@ -2908,6 +2909,98 @@ public class ChangeIT extends AbstractDaemonTest { gApi.changes().id(result2.getChangeId()).current().submit(); } + @Test + public void changeCommitMessage() throws Exception { + // Tests mutating the commit message as both the owner of the change and a regular user with + // addPatchSet permission. Asserts that both cases succeed. + PushOneCommit.Result r = createChange(); + r.assertOkStatus(); + assertThat(getCommitMessage(r.getChangeId())) + .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); + + for (TestAccount acc : ImmutableList.of(admin, user)) { + setApiUser(acc); + String newMessage = + "modified commit by " + acc.username + "\n\nChange-Id: " + r.getChangeId() + "\n"; + gApi.changes().id(r.getChangeId()).setMessage(newMessage); + RevisionApi rApi = gApi.changes().id(r.getChangeId()).current(); + assertThat(rApi.files().keySet()).containsExactly("/COMMIT_MSG", "a.txt"); + assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage); + } + } + + @Test + public void changeCommitMessageWithNoChangeIdSucceedsIfChangeIdNotRequired() throws Exception { + ConfigInput configInput = new ConfigInput(); + configInput.requireChangeId = InheritableBoolean.FALSE; + gApi.projects().name(project.get()).config(configInput); + + PushOneCommit.Result r = createChange(); + r.assertOkStatus(); + assertThat(getCommitMessage(r.getChangeId())) + .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); + + String newMessage = "modified commit\n"; + gApi.changes().id(r.getChangeId()).setMessage(newMessage); + RevisionApi rApi = gApi.changes().id(r.getChangeId()).current(); + assertThat(rApi.files().keySet()).containsExactly("/COMMIT_MSG", "a.txt"); + assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage); + } + + @Test + public void changeCommitMessageWithNoChangeIdFails() throws Exception { + PushOneCommit.Result r = createChange(); + assertThat(getCommitMessage(r.getChangeId())) + .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); + exception.expect(ResourceConflictException.class); + exception.expectMessage("missing Change-Id footer"); + gApi.changes().id(r.getChangeId()).setMessage("modified commit\n"); + } + + @Test + public void changeCommitMessageWithWrongChangeIdFails() throws Exception { + PushOneCommit.Result otherChange = createChange(); + PushOneCommit.Result r = createChange(); + assertThat(getCommitMessage(r.getChangeId())) + .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); + exception.expect(ResourceConflictException.class); + exception.expectMessage("wrong Change-Id footer"); + gApi.changes() + .id(r.getChangeId()) + .setMessage("modified commit\n\nChange-Id: " + otherChange.getChangeId() + "\n"); + } + + @Test + public void changeCommitMessageWithoutPermissionFails() throws Exception { + // Create new project with clean permissions + Project.NameKey p = createProject("addPatchSetEdit"); + TestRepository userTestRepo = cloneProject(p, user); + // Block default permission + block(p, "refs/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS); + // Create change as user + PushOneCommit push = pushFactory.create(db, user.getIdent(), userTestRepo); + PushOneCommit.Result r = push.to("refs/for/master"); + r.assertOkStatus(); + // Try to change the commit message + exception.expect(AuthException.class); + exception.expectMessage("modifying commit message not permitted"); + gApi.changes().id(r.getChangeId()).setMessage("foo"); + } + + @Test + public void changeCommitMessageWithSameMessageFails() throws Exception { + PushOneCommit.Result r = createChange(); + assertThat(getCommitMessage(r.getChangeId())) + .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); + exception.expect(ResourceConflictException.class); + exception.expectMessage("new and existing commit message are the same"); + gApi.changes().id(r.getChangeId()).setMessage(getCommitMessage(r.getChangeId())); + } + + private String getCommitMessage(String changeId) throws RestApiException, IOException { + return gApi.changes().id(changeId).current().file("/COMMIT_MSG").content().asString(); + } + private void addComment( PushOneCommit.Result r, String message, diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java index 429ee0f84c..e92d255488 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -52,9 +52,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput; import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.api.projects.BranchInput; -import com.google.gerrit.extensions.api.projects.ConfigInput; import com.google.gerrit.extensions.client.ChangeStatus; -import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.AccountInfo; @@ -77,7 +75,6 @@ import com.google.gerrit.extensions.restapi.ETagView; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; -import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.extensions.webui.PatchSetWebLink; import com.google.gerrit.reviewdb.client.Account; @@ -85,13 +82,11 @@ import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Branch.NameKey; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSetApproval; -import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.change.GetRevisionActions; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; import java.io.ByteArrayOutputStream; -import java.io.IOException; import java.sql.Timestamp; import java.text.DateFormat; import java.text.SimpleDateFormat; @@ -104,8 +99,6 @@ import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Optional; -import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; -import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.RefUpdate; @@ -1210,84 +1203,6 @@ public class RevisionIT extends AbstractDaemonTest { .containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.getId())); } - @Test - public void changeCommitMessage() throws Exception { - // Tests mutating the commit message as both the owner of the change and a regular user with - // addPatchSet permission. Asserts that both cases succeed. - PushOneCommit.Result r = createChange(); - r.assertOkStatus(); - assertThat(getCommitMessage(r.getChangeId())) - .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); - - for (TestAccount acc : ImmutableList.of(admin, user)) { - setApiUser(acc); - String newMessage = - "modified commit by " + acc.username + "\n\nChange-Id: " + r.getChangeId() + "\n"; - gApi.changes().id(r.getChangeId()).setMessage(newMessage); - RevisionApi rApi = gApi.changes().id(r.getChangeId()).current(); - assertThat(rApi.files().keySet()).containsExactly("/COMMIT_MSG", "a.txt"); - assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage); - } - } - - @Test - public void changeCommitMessageWithNoChangeIdSucceedsIfChangeIdNotRequired() throws Exception { - ConfigInput configInput = new ConfigInput(); - configInput.requireChangeId = InheritableBoolean.FALSE; - gApi.projects().name(project.get()).config(configInput); - - PushOneCommit.Result r = createChange(); - r.assertOkStatus(); - assertThat(getCommitMessage(r.getChangeId())) - .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); - - String newMessage = "modified commit\n"; - gApi.changes().id(r.getChangeId()).setMessage(newMessage); - RevisionApi rApi = gApi.changes().id(r.getChangeId()).current(); - assertThat(rApi.files().keySet()).containsExactly("/COMMIT_MSG", "a.txt"); - assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage); - } - - @Test - public void changeCommitMessageWithNoChangeIdFails() throws Exception { - PushOneCommit.Result r = createChange(); - assertThat(getCommitMessage(r.getChangeId())) - .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); - exception.expect(ResourceConflictException.class); - exception.expectMessage("missing Change-Id footer"); - gApi.changes().id(r.getChangeId()).setMessage("modified commit\n"); - } - - @Test - public void changeCommitMessageWithWrongChangeIdFails() throws Exception { - PushOneCommit.Result otherChange = createChange(); - PushOneCommit.Result r = createChange(); - assertThat(getCommitMessage(r.getChangeId())) - .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n"); - exception.expect(ResourceConflictException.class); - exception.expectMessage("wrong Change-Id footer"); - gApi.changes() - .id(r.getChangeId()) - .setMessage("modified commit\n\nChange-Id: " + otherChange.getChangeId() + "\n"); - } - - @Test - public void changeCommitMessageWithoutPermissionFails() throws Exception { - // Create new project with clean permissions - Project.NameKey p = createProject("addPatchSetEdit"); - TestRepository userTestRepo = cloneProject(p, user); - // Block default permission - block(p, "refs/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS); - // Create change as user - PushOneCommit push = pushFactory.create(db, user.getIdent(), userTestRepo); - PushOneCommit.Result r = push.to("refs/for/master"); - r.assertOkStatus(); - // Try to change the commit message - exception.expect(AuthException.class); - exception.expectMessage("modifying commit message not permitted"); - gApi.changes().id(r.getChangeId()).setMessage("foo"); - } - private static void assertCherryPickResult( ChangeInfo changeInfo, CherryPickInput input, String srcChangeId) throws Exception { assertThat(changeInfo.changeId).isEqualTo(srcChangeId); @@ -1364,10 +1279,6 @@ public class RevisionIT extends AbstractDaemonTest { return li.all.stream().filter(a -> a._accountId == accountId).findFirst().get(); } - private String getCommitMessage(String changeId) throws RestApiException, IOException { - return gApi.changes().id(changeId).current().file("/COMMIT_MSG").content().asString(); - } - private static Iterable getReviewers(Collection r) { return Iterables.transform(r, a -> new Account.Id(a._accountId)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java index 0935c71b3e..dcb5766843 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java @@ -132,7 +132,7 @@ public class PutMessage RevCommit patchSetCommit = revWalk.parseCommit(ObjectId.fromString(ps.getRevision().get())); String currentCommitMessage = patchSetCommit.getFullMessage(); - if (input.equals(currentCommitMessage)) { + if (input.message.equals(currentCommitMessage)) { throw new ResourceConflictException("new and existing commit message are the same"); }