From bfa06218e4175a20ed11bb5dae36f37ae6829dc2 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 4 Apr 2013 16:06:39 +0200 Subject: [PATCH] Add refs/for/%submit to auto-merge during push Teams that want to use Gerrit's submit strategies to handle contention on busy branches can use %submit to create a change and have it immediately submitted, if the caller has Submit permission on refs/for/. If the merge fails the change stays open. Change-Id: I98bcefb51ea5363b011e3e57f89f567b73e16103 Signed-off-by: Edwin Kempin --- Documentation/access-control.txt | 4 + Documentation/user-upload.txt | 20 ++ .../google/gerrit/acceptance/git/GitUtil.java | 13 +- .../acceptance/git/PushForReviewIT.java | 179 +++-------- .../gerrit/acceptance/git/PushOneCommit.java | 179 +++++++++++ .../gerrit/acceptance/git/SubmitOnPushIT.java | 300 ++++++++++++++++++ .../google/gerrit/server/change/Submit.java | 49 +-- .../gerrit/server/git/CommitMergeStatus.java | 2 +- .../gerrit/server/git/ReceiveCommits.java | 69 +++- 9 files changed, 653 insertions(+), 162 deletions(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index 9ef6e13306..1012e5a9ec 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -641,6 +641,10 @@ In order to submit, all labels (such as `Verified` and `Code-Review`, above) must enable submit, and also must not block it. See above for details on each label. +To link:user-upload.html#auto_merge[immediately submit a change on push] +the caller needs to have the Submit permission on `refs/for/` +(e.g. on `refs/for/refs/heads/master`). + [[category_view_drafts]] View Drafts diff --git a/Documentation/user-upload.txt b/Documentation/user-upload.txt index 55ab89521a..83ce6ce393 100644 --- a/Documentation/user-upload.txt +++ b/Documentation/user-upload.txt @@ -333,6 +333,26 @@ grant nothing at all. This ensures that accidental pushes don't make undesired changes to the public repository. +[[auto_merge]] +Auto-Merge during Push +~~~~~~~~~~~~~~~~~~~~~~ + +Changes can be directly submitted on push. This is primarily useful +for teams that don't want to do code review but want to use Gerrit's +submit strategies to handle contention on busy branches. Using +`%submit` creates a change and submits it immediately, if the caller +has link:access-control.html#category_submit[Submit] permission on +`refs/for/` (e.g. on `refs/for/refs/heads/master`). + +==== + git push ssh://john.doe@git.example.com:29418/kernel/common HEAD:refs/for/master%submit +==== + +On auto-merge of a change neither labels nor submit rules are checked. +If the merge fails the change stays open, but when pushing a new patch +set the merge can be reattempted by using `%submit` again. + + repo upload ----------- diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java index 9faf32a8c1..c8158c07e4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java @@ -107,11 +107,17 @@ public class GitUtil { public static String createCommit(Git git, PersonIdent i, String msg) throws GitAPIException, IOException { - return createCommit(git, i, msg, true); + return createCommit(git, i, msg, true, false); } - public static String createCommit(Git git, PersonIdent i, String msg, - boolean insertChangeId) throws GitAPIException, IOException { + public static void amendCommit(Git git, PersonIdent i, String msg, String changeId) + throws GitAPIException, IOException { + msg = ChangeIdUtil.insertId(msg, ObjectId.fromString(changeId.substring(1))); + createCommit(git, i, msg, false, true); + } + + private static String createCommit(Git git, PersonIdent i, String msg, + boolean insertChangeId, boolean amend) throws GitAPIException, IOException { ObjectId changeId = null; if (insertChangeId) { changeId = computeChangeId(git, i, msg); @@ -119,6 +125,7 @@ public class GitUtil { } final CommitCommand commitCmd = git.commit(); + commitCmd.setAmend(amend); commitCmd.setAuthor(i); commitCmd.setCommitter(i); commitCmd.setMessage(msg); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushForReviewIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushForReviewIT.java index f90535078f..7af4c552f3 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushForReviewIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushForReviewIT.java @@ -14,27 +14,16 @@ package com.google.gerrit.acceptance.git; -import static com.google.gerrit.acceptance.git.GitUtil.add; import static com.google.gerrit.acceptance.git.GitUtil.cloneProject; -import static com.google.gerrit.acceptance.git.GitUtil.createCommit; import static com.google.gerrit.acceptance.git.GitUtil.createProject; import static com.google.gerrit.acceptance.git.GitUtil.initSsh; -import static com.google.gerrit.acceptance.git.GitUtil.pushHead; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import com.google.common.base.Function; -import com.google.common.base.Strings; -import com.google.common.collect.Iterables; import com.google.common.collect.Lists; -import com.google.common.collect.Sets; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AccountCreator; import com.google.gerrit.acceptance.SshSession; import com.google.gerrit.acceptance.TestAccount; -import com.google.gerrit.reviewdb.client.Account; 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.reviewdb.server.ReviewDb; import com.google.gwtorm.server.OrmException; @@ -45,9 +34,6 @@ import com.jcraft.jsch.JSchException; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; -import org.eclipse.jgit.transport.PushResult; -import org.eclipse.jgit.transport.RemoteRefUpdate; -import org.eclipse.jgit.transport.RemoteRefUpdate.Status; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -56,9 +42,7 @@ import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; import java.io.IOException; -import java.util.Arrays; import java.util.List; -import java.util.Set; @RunWith(Parameterized.class) public class PushForReviewIT extends AbstractDaemonTest { @@ -127,30 +111,24 @@ public class PushForReviewIT extends AbstractDaemonTest { @Test public void testPushForMaster() throws GitAPIException, OrmException, IOException { - PushOneCommit push = new PushOneCommit(); - String ref = "refs/for/master"; - PushResult r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.NEW, PushOneCommit.SUBJECT, null); + PushOneCommit.Result r = pushTo("refs/for/master"); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, null); } @Test public void testPushForMasterWithTopic() throws GitAPIException, OrmException, IOException { // specify topic in ref - PushOneCommit push = new PushOneCommit(); String topic = "my/topic"; - String ref = "refs/for/master/" + topic; - PushResult r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.NEW, PushOneCommit.SUBJECT, topic); + PushOneCommit.Result r = pushTo("refs/for/master/" + topic); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, topic); // specify topic as option - push = new PushOneCommit(); - ref = "refs/for/master%topic=" + topic; - r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.NEW, PushOneCommit.SUBJECT, topic); + r = pushTo("refs/for/master%topic=" + topic); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, topic); } @Test @@ -158,30 +136,24 @@ public class PushForReviewIT extends AbstractDaemonTest { IOException, JSchException { // cc one user TestAccount user = accounts.create("user", "user@example.com", "User"); - PushOneCommit push = new PushOneCommit(); String topic = "my/topic"; - String ref = "refs/for/master/" + topic + "%cc=" + user.email; - PushResult r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.NEW, PushOneCommit.SUBJECT, topic); + PushOneCommit.Result r = pushTo("refs/for/master/" + topic + "%cc=" + user.email); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, topic); // cc several users TestAccount user2 = accounts.create("another-user", "another.user@example.com", "Another User"); - push = new PushOneCommit(); - ref = "refs/for/master/" + topic + "%cc=" + admin.email + ",cc=" + user.email - + ",cc=" + user2.email; - r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.NEW, PushOneCommit.SUBJECT, topic); + r = pushTo("refs/for/master/" + topic + "%cc=" + admin.email + ",cc=" + + user.email + ",cc=" + user2.email); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, topic); // cc non-existing user String nonExistingEmail = "non.existing@example.com"; - push = new PushOneCommit(); - ref = "refs/for/master/" + topic + "%cc=" + admin.email + ",cc=" - + nonExistingEmail + ",cc=" + user.email; - r = push.to(ref); - assertErrorStatus(r, "user \"" + nonExistingEmail + "\" not found", ref); + r = pushTo("refs/for/master/" + topic + "%cc=" + admin.email + ",cc=" + + nonExistingEmail + ",cc=" + user.email); + r.assertErrorStatus("user \"" + nonExistingEmail + "\" not found"); } @Test @@ -189,121 +161,52 @@ public class PushForReviewIT extends AbstractDaemonTest { OrmException, IOException, JSchException { // add one reviewer TestAccount user = accounts.create("user", "user@example.com", "User"); - PushOneCommit push = new PushOneCommit(); String topic = "my/topic"; - String ref = "refs/for/master/" + topic + "%r=" + user.email; - PushResult r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.NEW, PushOneCommit.SUBJECT, - topic, user); + PushOneCommit.Result r = pushTo("refs/for/master/" + topic + "%r=" + user.email); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, topic, user); // add several reviewers TestAccount user2 = accounts.create("another-user", "another.user@example.com", "Another User"); - push = new PushOneCommit(); - ref = "refs/for/master/" + topic + "%r=" + admin.email + ",r=" + user.email - + ",r=" + user2.email; - r = push.to(ref); - assertOkStatus(r, ref); + r = pushTo("refs/for/master/" + topic + "%r=" + admin.email + ",r=" + user.email + + ",r=" + user2.email); + r.assertOkStatus(); // admin is the owner of the change and should not appear as reviewer - assertChange(push.changeId, Change.Status.NEW, PushOneCommit.SUBJECT, - topic, user, user2); + r.assertChange(Change.Status.NEW, topic, user, user2); // add non-existing user as reviewer String nonExistingEmail = "non.existing@example.com"; - push = new PushOneCommit(); - ref = "refs/for/master/" + topic + "%r=" + admin.email + ",r=" - + nonExistingEmail + ",r=" + user.email; - r = push.to(ref); - assertErrorStatus(r, "user \"" + nonExistingEmail + "\" not found", ref); + r = pushTo("refs/for/master/" + topic + "%r=" + admin.email + ",r=" + + nonExistingEmail + ",r=" + user.email); + r.assertErrorStatus("user \"" + nonExistingEmail + "\" not found"); } @Test public void testPushForMasterAsDraft() throws GitAPIException, OrmException, IOException { // create draft by pushing to 'refs/drafts/' - PushOneCommit push = new PushOneCommit(); - String ref = "refs/drafts/master"; - PushResult r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.DRAFT, PushOneCommit.SUBJECT, null); + PushOneCommit.Result r = pushTo("refs/drafts/master"); + r.assertOkStatus(); + r.assertChange(Change.Status.DRAFT, null); // create draft by using 'draft' option - push = new PushOneCommit(); - ref = "refs/for/master%draft"; - r = push.to(ref); - assertOkStatus(r, ref); - assertChange(push.changeId, Change.Status.DRAFT, PushOneCommit.SUBJECT, null); + r = pushTo("refs/for/master%draft"); + r.assertOkStatus(); + r.assertChange(Change.Status.DRAFT, null); } @Test public void testPushForNonExistingBranch() throws GitAPIException, OrmException, IOException { - PushOneCommit push = new PushOneCommit(); String branchName = "non-existing"; - String ref = "refs/for/" + branchName; - PushResult r = push.to(ref); - assertErrorStatus(r, "branch " + branchName + " not found", ref); + PushOneCommit.Result r = pushTo("refs/for/" + branchName); + r.assertErrorStatus("branch " + branchName + " not found"); } - private void assertChange(String changeId, Change.Status expectedStatus, - String expectedSubject, String expectedTopic, - TestAccount... expectedReviewers) throws OrmException { - Change c = - Iterables.getOnlyElement(db.changes().byKey(new Change.Key(changeId)).toList()); - assertEquals(expectedSubject, c.getSubject()); - assertEquals(expectedStatus, c.getStatus()); - assertEquals(expectedTopic, Strings.emptyToNull(c.getTopic())); - assertReviewers(c, expectedReviewers); - } - - private void assertReviewers(Change c, TestAccount... expectedReviewers) - throws OrmException { - Set expectedReviewerIds = - Sets.newHashSet(Lists.transform(Arrays.asList(expectedReviewers), - new Function() { - @Override - public Account.Id apply(TestAccount a) { - return a.id; - } - })); - - for (PatchSetApproval psa : db.patchSetApprovals().byPatchSet( - c.currentPatchSetId())) { - assertTrue("unexpected reviewer " + psa.getAccountId(), - expectedReviewerIds.remove(psa.getAccountId())); - } - assertTrue("missing reviewers: " + expectedReviewerIds, - expectedReviewerIds.isEmpty()); - } - - private static void assertOkStatus(PushResult result, String ref) { - assertStatus(Status.OK, null, result, ref); - } - - private static void assertErrorStatus(PushResult result, - String expectedMessage, String ref) { - assertStatus(Status.REJECTED_OTHER_REASON, expectedMessage, result, ref); - } - - private static void assertStatus(Status expectedStatus, - String expectedMessage, PushResult result, String ref) { - RemoteRefUpdate refUpdate = result.getRemoteUpdate(ref); - assertEquals(refUpdate.getMessage() + "\n" + result.getMessages(), - expectedStatus, refUpdate.getStatus()); - assertEquals(expectedMessage, refUpdate.getMessage()); - } - - private class PushOneCommit { - final static String FILE_NAME = "a.txt"; - final static String FILE_CONTENT = "some content"; - final static String SUBJECT = "test commit"; - String changeId; - - public PushResult to(String ref) throws GitAPIException, IOException { - add(git, FILE_NAME, FILE_CONTENT); - changeId = createCommit(git, admin.getIdent(), SUBJECT); - return pushHead(git, ref); - } + private PushOneCommit.Result pushTo(String ref) throws GitAPIException, + IOException { + PushOneCommit push = new PushOneCommit(db, admin.getIdent()); + return push.to(git, ref); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java new file mode 100644 index 0000000000..fb1b59206d --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java @@ -0,0 +1,179 @@ +// Copyright (C) 2013 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.git; + +import static com.google.gerrit.acceptance.git.GitUtil.add; +import static com.google.gerrit.acceptance.git.GitUtil.amendCommit; +import static com.google.gerrit.acceptance.git.GitUtil.createCommit; +import static com.google.gerrit.acceptance.git.GitUtil.pushHead; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import com.google.common.base.Function; +import com.google.common.base.Strings; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import com.google.common.collect.Sets; +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gwtorm.server.OrmException; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.transport.PushResult; +import org.eclipse.jgit.transport.RemoteRefUpdate; +import org.eclipse.jgit.transport.RemoteRefUpdate.Status; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Set; + +public class PushOneCommit { + public final static String SUBJECT = "test commit"; + + private final static String FILE_NAME = "a.txt"; + private final static String FILE_CONTENT = "some content"; + + private final ReviewDb db; + private final PersonIdent i; + + private final String subject; + private final String fileName; + private final String content; + private String changeId; + + public PushOneCommit(ReviewDb db, PersonIdent i) { + this(db, i, SUBJECT, FILE_NAME, FILE_CONTENT); + } + + public PushOneCommit(ReviewDb db, PersonIdent i, String subject, + String fileName, String content) { + this(db, i, subject, fileName, content, null); + } + + public PushOneCommit(ReviewDb db, PersonIdent i, String subject, + String fileName, String content, String changeId) { + this.db = db; + this.i = i; + this.subject = subject; + this.fileName = fileName; + this.content = content; + this.changeId = changeId; + } + + public Result to(Git git, String ref) + throws GitAPIException, IOException { + add(git, fileName, content); + if (changeId != null) { + amendCommit(git, i, subject, changeId); + } else { + changeId = createCommit(git, i, subject); + } + return new Result(db, ref, pushHead(git, ref), changeId, subject); + } + + public static class Result { + private final ReviewDb db; + private final String ref; + private final PushResult result; + private final String changeId; + private final String subject; + + private Result(ReviewDb db, String ref, PushResult result, String changeId, + String subject) { + this.db = db; + this.ref = ref; + this.result = result; + this.changeId = changeId; + this.subject = subject; + } + + public PatchSet.Id getPatchSetId() throws OrmException { + return Iterables.getOnlyElement( + db.changes().byKey(new Change.Key(changeId))).currentPatchSetId(); + } + + public String getChangeId() { + return changeId; + } + + public void assertChange(Change.Status expectedStatus, + String expectedTopic, TestAccount... expectedReviewers) + throws OrmException { + Change c = + Iterables.getOnlyElement(db.changes().byKey(new Change.Key(changeId)).toList()); + assertEquals(subject, c.getSubject()); + assertEquals(expectedStatus, c.getStatus()); + assertEquals(expectedTopic, Strings.emptyToNull(c.getTopic())); + assertReviewers(c, expectedReviewers); + } + + private void assertReviewers(Change c, TestAccount... expectedReviewers) + throws OrmException { + Set expectedReviewerIds = + Sets.newHashSet(Lists.transform(Arrays.asList(expectedReviewers), + new Function() { + @Override + public Account.Id apply(TestAccount a) { + return a.id; + } + })); + + for (PatchSetApproval psa : db.patchSetApprovals().byPatchSet( + c.currentPatchSetId())) { + assertTrue("unexpected reviewer " + psa.getAccountId(), + expectedReviewerIds.remove(psa.getAccountId())); + } + assertTrue("missing reviewers: " + expectedReviewerIds, + expectedReviewerIds.isEmpty()); + } + + public void assertOkStatus() { + assertStatus(Status.OK, null); + } + + public void assertErrorStatus(String expectedMessage) { + assertStatus(Status.REJECTED_OTHER_REASON, expectedMessage); + } + + private void assertStatus(Status expectedStatus, String expectedMessage) { + RemoteRefUpdate refUpdate = result.getRemoteUpdate(ref); + assertEquals(message(refUpdate), + expectedStatus, refUpdate.getStatus()); + assertEquals(expectedMessage, refUpdate.getMessage()); + } + + public void assertMessage(String expectedMessage) { + RemoteRefUpdate refUpdate = result.getRemoteUpdate(ref); + assertTrue(message(refUpdate), message(refUpdate).toLowerCase().contains( + expectedMessage.toLowerCase())); + } + + private String message(RemoteRefUpdate refUpdate) { + StringBuilder b = new StringBuilder(); + if (refUpdate.getMessage() != null) { + b.append(refUpdate.getMessage()); + b.append("\n"); + } + b.append(result.getMessages()); + return b.toString(); + } + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java new file mode 100644 index 0000000000..fa85927d8e --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java @@ -0,0 +1,300 @@ +// Copyright (C) 2013 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.git; + +import static com.google.gerrit.acceptance.git.GitUtil.cloneProject; +import static com.google.gerrit.acceptance.git.GitUtil.createProject; +import static com.google.gerrit.acceptance.git.GitUtil.initSsh; +import static org.junit.Assert.assertEquals; + +import com.google.common.collect.Iterables; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.AccountCreator; +import com.google.gerrit.acceptance.SshSession; +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.common.data.AccessSection; +import com.google.gerrit.common.data.Permission; +import com.google.gerrit.common.data.PermissionRule; +import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.account.GroupCache; +import com.google.gerrit.server.git.CommitMergeStatus; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.project.ProjectCache; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.SchemaFactory; +import com.google.inject.Inject; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.RefSpec; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; +import java.util.List; + +public class SubmitOnPushIT extends AbstractDaemonTest { + + @Inject + private AccountCreator accounts; + + @Inject + private SchemaFactory reviewDbProvider; + + @Inject + private GitRepositoryManager repoManager; + + @Inject + private MetaDataUpdate.Server metaDataUpdateFactory; + + @Inject + private ProjectCache projectCache; + + @Inject + private GroupCache groupCache; + + @Inject + private @GerritPersonIdent PersonIdent serverIdent; + + private TestAccount admin; + private Project.NameKey project; + private Git git; + private ReviewDb db; + + @Before + public void setUp() throws Exception { + admin = + accounts.create("admin", "admin@example.com", "Administrator", + "Administrators"); + + project = new Project.NameKey("p"); + initSsh(admin); + SshSession sshSession = new SshSession(admin); + createProject(sshSession, project.get()); + git = cloneProject(sshSession.getUrl() + "/" + project.get()); + sshSession.close(); + + db = reviewDbProvider.open(); + } + + @After + public void cleanup() { + db.close(); + } + + @Test + public void submitOnPush() throws GitAPIException, OrmException, + IOException, ConfigInvalidException { + grantSubmit(project, "refs/for/refs/heads/master"); + PushOneCommit.Result r = pushTo("refs/for/master%submit"); + r.assertOkStatus(); + r.assertChange(Change.Status.MERGED, null, admin); + assertSubmitApproval(r.getPatchSetId()); + assertCommit(project, "refs/heads/master"); + } + + @Test + public void submitOnPushToRefsMetaConfig() throws GitAPIException, + OrmException, IOException, ConfigInvalidException { + grantSubmit(project, "refs/for/refs/meta/config"); + + git.fetch().setRefSpecs(new RefSpec("refs/meta/config:refs/meta/config")).call(); + ObjectId objectId = git.getRepository().getRef("refs/meta/config").getObjectId(); + git.checkout().setName(objectId.getName()).call(); + + PushOneCommit.Result r = pushTo("refs/for/refs/meta/config%submit"); + r.assertOkStatus(); + r.assertChange(Change.Status.MERGED, null, admin); + assertSubmitApproval(r.getPatchSetId()); + assertCommit(project, "refs/meta/config"); + } + + @Test + public void submitOnPushMergeConflict() throws GitAPIException, OrmException, + IOException, ConfigInvalidException { + String master = "refs/heads/master"; + ObjectId objectId = git.getRepository().getRef(master).getObjectId(); + push(master, "one change", "a.txt", "some content"); + git.checkout().setName(objectId.getName()).call(); + + grantSubmit(project, "refs/for/refs/heads/master"); + PushOneCommit.Result r = + push("refs/for/master%submit", "other change", "a.txt", "other content"); + r.assertOkStatus(); + r.assertChange(Change.Status.NEW, null, admin); + r.assertMessage(CommitMergeStatus.PATH_CONFLICT.getMessage()); + } + + @Test + public void submitOnPushSuccessfulMerge() throws GitAPIException, OrmException, + IOException, ConfigInvalidException { + String master = "refs/heads/master"; + ObjectId objectId = git.getRepository().getRef(master).getObjectId(); + push(master, "one change", "a.txt", "some content"); + git.checkout().setName(objectId.getName()).call(); + + grantSubmit(project, "refs/for/refs/heads/master"); + PushOneCommit.Result r = + push("refs/for/master%submit", "other change", "b.txt", "other content"); + r.assertOkStatus(); + r.assertChange(Change.Status.MERGED, null, admin); + assertMergeCommit(master, "other change"); + } + + @Test + public void submitOnPushNewPatchSet() throws GitAPIException, + OrmException, IOException, ConfigInvalidException { + PushOneCommit.Result r = + push("refs/for/master", PushOneCommit.SUBJECT, "a.txt", "some content"); + + grantSubmit(project, "refs/for/refs/heads/master"); + r = push("refs/for/master%submit", PushOneCommit.SUBJECT, "a.txt", + "other content", r.getChangeId()); + r.assertOkStatus(); + r.assertChange(Change.Status.MERGED, null, admin); + Change c = Iterables.getOnlyElement(db.changes().byKey( + new Change.Key(r.getChangeId())).toList()); + assertEquals(2, db.patchSets().byChange(c.getId()).toList().size()); + assertSubmitApproval(r.getPatchSetId()); + assertCommit(project, "refs/heads/master"); + } + + @Test + public void submitOnPushNotAllowed_Error() throws GitAPIException, + OrmException, IOException { + PushOneCommit.Result r = pushTo("refs/for/master%submit"); + r.assertErrorStatus("submit not allowed"); + } + + @Test + public void submitOnPushNewPatchSetNotAllowed_Error() throws GitAPIException, + OrmException, IOException, ConfigInvalidException { + PushOneCommit.Result r = + push("refs/for/master", PushOneCommit.SUBJECT, "a.txt", "some content"); + + r = push("refs/for/master%submit", PushOneCommit.SUBJECT, "a.txt", + "other content", r.getChangeId()); + r.assertErrorStatus("submit not allowed"); + } + + @Test + public void submitOnPushingDraft_Error() throws GitAPIException, + OrmException, IOException { + PushOneCommit.Result r = pushTo("refs/for/master%draft,submit"); + r.assertErrorStatus("cannot submit draft"); + } + + @Test + public void submitOnPushToNonExistingBranch_Error() throws GitAPIException, + OrmException, IOException { + String branchName = "non-existing"; + PushOneCommit.Result r = pushTo("refs/for/" + branchName + "%submit"); + r.assertErrorStatus("branch " + branchName + " not found"); + } + + private void grantSubmit(Project.NameKey project, String ref) + throws RepositoryNotFoundException, IOException, ConfigInvalidException { + MetaDataUpdate md = metaDataUpdateFactory.create(project); + md.setMessage("Grant submit on " + ref); + ProjectConfig config = ProjectConfig.read(md); + AccessSection s = config.getAccessSection(ref, true); + Permission p = s.getPermission(Permission.SUBMIT, true); + AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators")); + p.add(new PermissionRule(config.resolve(adminGroup))); + config.commit(md); + projectCache.evict(config.getProject()); + } + + private void assertSubmitApproval(PatchSet.Id patchSetId) throws OrmException { + List approvals = db.patchSetApprovals().byPatchSet(patchSetId).toList(); + assertEquals(1, approvals.size()); + PatchSetApproval a = approvals.get(0); + assertEquals(PatchSetApproval.LabelId.SUBMIT.get(), a.getLabel()); + assertEquals(1, a.getValue()); + assertEquals(admin.id, a.getAccountId()); + } + + private void assertCommit(Project.NameKey project, String branch) throws IOException { + Repository r = repoManager.openRepository(project); + try { + RevWalk rw = new RevWalk(r); + try { + RevCommit c = rw.parseCommit(r.getRef(branch).getObjectId()); + assertEquals(PushOneCommit.SUBJECT, c.getShortMessage()); + assertEquals(admin.email, c.getAuthorIdent().getEmailAddress()); + assertEquals(admin.email, c.getCommitterIdent().getEmailAddress()); + } finally { + rw.release(); + } + } finally { + r.close(); + } + } + + private void assertMergeCommit(String branch, String subject) throws IOException { + Repository r = repoManager.openRepository(project); + try { + RevWalk rw = new RevWalk(r); + try { + RevCommit c = rw.parseCommit(r.getRef(branch).getObjectId()); + assertEquals(2, c.getParentCount()); + assertEquals("Merge \"" + subject + "\"", c.getShortMessage()); + assertEquals(admin.email, c.getAuthorIdent().getEmailAddress()); + assertEquals(serverIdent.getEmailAddress(), c.getCommitterIdent().getEmailAddress()); + } finally { + rw.release(); + } + } finally { + r.close(); + } + } + + private PushOneCommit.Result pushTo(String ref) throws GitAPIException, + IOException { + PushOneCommit push = new PushOneCommit(db, admin.getIdent()); + return push.to(git, ref); + } + + private PushOneCommit.Result push(String ref, String subject, + String fileName, String content) throws GitAPIException, IOException { + PushOneCommit push = + new PushOneCommit(db, admin.getIdent(), subject, fileName, content); + return push.to(git, ref); + } + + private PushOneCommit.Result push(String ref, String subject, + String fileName, String content, String changeId) throws GitAPIException, + IOException { + PushOneCommit push = new PushOneCommit(db, admin.getIdent(), subject, + fileName, content, changeId); + return push.to(git, ref); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index c63bf5d3a7..2b51b0a802 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -106,6 +106,10 @@ public class Submit implements RestModifyView { checkSubmitRule(rsrc); change = submit(rsrc, caller); + if (change == null) { + throw new ResourceConflictException("change is " + + status(dbProvider.get().changes().get(rsrc.getChange().getId()))); + } if (input.waitForMerge) { mergeQueue.merge(change.getDest()); @@ -123,21 +127,7 @@ public class Submit implements RestModifyView { case MERGED: return new Output(Status.MERGED, change); case NEW: - // If the merge was attempted and it failed the system usually - // writes a comment as a ChangeMessage and sets status to NEW. - // Find the relevant message and report that as the conflict. - final Timestamp before = rsrc.getChange().getLastUpdatedOn(); - ChangeMessage msg = Iterables.getFirst(Iterables.filter( - Lists.reverse(dbProvider.get().changeMessages() - .byChange(change.getId()) - .toList()), - new Predicate() { - @Override - public boolean apply(ChangeMessage input) { - return input.getAuthor() == null - && input.getWrittenOn().getTime() >= before.getTime(); - } - }), null); + ChangeMessage msg = getConflictMessage(rsrc); if (msg != null) { throw new ResourceConflictException(msg.getMessage()); } @@ -146,8 +136,30 @@ public class Submit implements RestModifyView { } } - private Change submit(RevisionResource rsrc, IdentifiedUser caller) - throws OrmException, ResourceConflictException { + /** + * If the merge was attempted and it failed the system usually writes a + * comment as a ChangeMessage and sets status to NEW. Find the relevant + * message and return it. + */ + public ChangeMessage getConflictMessage(RevisionResource rsrc) + throws OrmException { + final Timestamp before = rsrc.getChange().getLastUpdatedOn(); + ChangeMessage msg = Iterables.getFirst(Iterables.filter( + Lists.reverse(dbProvider.get().changeMessages() + .byChange(rsrc.getChange().getId()) + .toList()), + new Predicate() { + @Override + public boolean apply(ChangeMessage input) { + return input.getAuthor() == null + && input.getWrittenOn().getTime() >= before.getTime(); + } + }), null); + return msg; + } + + public Change submit(RevisionResource rsrc, IdentifiedUser caller) + throws OrmException { final Timestamp timestamp = new Timestamp(System.currentTimeMillis()); Change change = rsrc.getChange(); ReviewDb db = dbProvider.get(); @@ -169,8 +181,7 @@ public class Submit implements RestModifyView { } }); if (change == null) { - throw new ResourceConflictException("change is " - + status(db.changes().get(rsrc.getChange().getId()))); + return null; } db.commit(); } finally { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java index 69dcb156db..8595472517 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java @@ -14,7 +14,7 @@ package com.google.gerrit.server.git; -enum CommitMergeStatus { +public enum CommitMergeStatus { /** */ CLEAN_MERGE("Change has been successfully merged into the git repository."), diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 9dd5a4f052..2d8c4c55bf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -59,6 +59,9 @@ import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.change.ChangeInserter; +import com.google.gerrit.server.change.ChangeResource; +import com.google.gerrit.server.change.RevisionResource; +import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.TrackingFooters; @@ -87,6 +90,7 @@ import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; import org.eclipse.jgit.errors.IncorrectObjectTypeException; @@ -277,6 +281,8 @@ public class ReceiveCommits { private Map allRefs; private final SubmoduleOp.Factory subOpFactory; + private final Provider submitProvider; + private final MergeQueue mergeQueue; private final List messages = new ArrayList(); private ListMultimap errors = LinkedListMultimap.create(); @@ -316,7 +322,9 @@ public class ReceiveCommits { ReceiveConfig config, @Assisted final ProjectControl projectControl, @Assisted final Repository repo, - final SubmoduleOp.Factory subOpFactory) throws IOException { + final SubmoduleOp.Factory subOpFactory, + final Provider submitProvider, + final MergeQueue mergeQueue) throws IOException { this.currentUser = (IdentifiedUser) projectControl.getCurrentUser(); this.db = db; this.schemaFactory = schemaFactory; @@ -351,6 +359,8 @@ public class ReceiveCommits { this.rejectCommits = loadRejectCommitsMap(); this.subOpFactory = subOpFactory; + this.submitProvider = submitProvider; + this.mergeQueue = mergeQueue; this.messageSender = new ReceivePackMessageSender(); @@ -1000,6 +1010,9 @@ public class ReceiveCommits { @Option(name = "--draft", usage = "mark new/update changes as draft") boolean draft; + @Option(name = "--submit", usage = "immediately submit the change") + boolean submit; + @Option(name = "-r", metaVar = "EMAIL", usage = "add reviewer to changes") void reviewer(Account.Id id) { reviewer.add(id); @@ -1024,6 +1037,10 @@ public class ReceiveCommits { return draft; } + boolean isSubmit() { + return submit; + } + MailRecipients getMailRecipients() { return new MailRecipients(reviewer, cc); } @@ -1120,6 +1137,16 @@ public class ReceiveCommits { return; } + if (magicBranch.isDraft() && magicBranch.isSubmit()) { + reject(cmd, "cannot submit draft"); + return; + } + + if (magicBranch.isSubmit() && !projectControl.controlForRef( + MagicBranch.NEW_CHANGE + ref).canSubmit()) { + reject(cmd, "submit not allowed"); + } + // Validate that the new commits are connected with the target // branch. If they aren't, we want to abort. We do this check by // looking to see if we can compute a merge base between the new @@ -1493,6 +1520,41 @@ public class ReceiveCommits { return "send-email newchange"; } })); + + if (magicBranch != null && magicBranch.isSubmit()) { + submit(projectControl.controlFor(change), ps); + } + } + } + + private void submit(ChangeControl changeCtl, PatchSet ps) throws OrmException { + Submit submit = submitProvider.get(); + RevisionResource rsrc = new RevisionResource(new ChangeResource(changeCtl), ps); + Change c = submit.submit(rsrc, currentUser); + if (c == null) { + addError("Submitting change " + changeCtl.getChange().getChangeId() + + " failed."); + } else { + addMessage(""); + mergeQueue.merge(c.getDest()); + c = db.changes().get(c.getId()); + switch (c.getStatus()) { + case SUBMITTED: + addMessage("Change " + c.getChangeId() + " submitted."); + break; + case MERGED: + addMessage("Change " + c.getChangeId() + " merged."); + break; + case NEW: + ChangeMessage msg = submit.getConflictMessage(rsrc); + if (msg != null) { + addMessage("Change " + c.getChangeId() + ": " + msg.getMessage()); + break; + } + default: + addMessage("change " + c.getChangeId() + " is " + + c.getStatus().name().toLowerCase()); + } } } @@ -1865,6 +1927,11 @@ public class ReceiveCommits { return "send-email newpatchset"; } })); + + if (magicBranch != null && magicBranch.isSubmit()) { + submit(changeCtl, newPatchSet); + } + return newPatchSet.getId(); } }