From 8792bd67d75fcff96acdf0656bf841b532eb75ce Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 10 Jul 2015 13:28:31 -0700 Subject: [PATCH 1/3] Test author and committer for different submit strategies Change-Id: Ia8e1f7cd008de95585101354000576a99852abaf --- .../google/gerrit/acceptance/AbstractDaemonTest.java | 6 ++++++ .../google/gerrit/acceptance/git/SubmitOnPushIT.java | 6 +----- .../gerrit/acceptance/rest/change/AbstractSubmit.java | 11 +++++++++++ .../acceptance/rest/change/SubmitByCherryPickIT.java | 3 +++ .../acceptance/rest/change/SubmitByFastForwardIT.java | 2 ++ .../acceptance/rest/change/SubmitByMergeAlwaysIT.java | 2 ++ .../rest/change/SubmitByMergeIfNecessaryIT.java | 4 ++++ .../rest/change/SubmitByRebaseIfNecessaryIT.java | 4 ++++ 8 files changed, 33 insertions(+), 5 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 8ac8a88a18..a4cbe72b4f 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -39,6 +39,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.AnonymousUser; +import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.OutputFormat; import com.google.gerrit.server.account.GroupCache; @@ -67,6 +68,7 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.Transport; import org.junit.AfterClass; @@ -148,6 +150,10 @@ public abstract class AbstractDaemonTest { @Inject private Provider anonymousUser; + @Inject + @GerritPersonIdent + protected Provider serverIdent; + protected TestRepository testRepo; protected GerritServer server; protected TestAccount admin; 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 index 1f0c601bdd..15c8a98289 100644 --- 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 @@ -53,10 +53,6 @@ public class SubmitOnPushIT extends AbstractDaemonTest { @Inject private ChangeNotes.Factory changeNotesFactory; - @Inject - @GerritPersonIdent - private PersonIdent serverIdent; - @Test public void submitOnPush() throws Exception { grant(Permission.SUBMIT, project, "refs/for/refs/heads/master"); @@ -257,7 +253,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { assertThat(c.getShortMessage()).isEqualTo("Merge \"" + subject + "\""); assertThat(c.getAuthorIdent().getEmailAddress()).isEqualTo(admin.email); assertThat(c.getCommitterIdent().getEmailAddress()).isEqualTo( - serverIdent.getEmailAddress()); + serverIdent.get().getEmailAddress()); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 15288966b4..20507fe013 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -61,6 +61,7 @@ import org.eclipse.jgit.diff.DiffFormatter; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; @@ -273,6 +274,16 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { assertThat(new Account.Id(cr.all.get(0)._accountId)).isEqualTo(admin.getId()); } + protected void assertPersonEquals(PersonIdent expected, + PersonIdent actual) { + assertThat(actual.getEmailAddress()) + .isEqualTo(expected.getEmailAddress()); + assertThat(actual.getName()) + .isEqualTo(expected.getName()); + assertThat(actual.getTimeZone()) + .isEqualTo(expected.getTimeZone()); + } + protected void assertSubmitter(String changeId, int psId) throws OrmException { ChangeNotes cn = notesFactory.create( diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java index b54c5d6ed3..d4d190e276 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java @@ -65,6 +65,9 @@ public class SubmitByCherryPickIT extends AbstractSubmit { assertCurrentRevision(change2.getChangeId(), 2, newHead); assertSubmitter(change2.getChangeId(), 1); assertSubmitter(change2.getChangeId(), 2); + assertPersonEquals(admin.getIdent(), newHead.getAuthorIdent()); + // TODO: Check the committer, too. + // assertPersonEquals(admin.getIdent(), newHead.getCommitterIdent()); } @Test diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java index 4fad2ab5be..26557895fe 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java @@ -56,6 +56,8 @@ public class SubmitByFastForwardIT extends AbstractSubmit { assertThat(head.getParent(0).getId()).isEqualTo(change.getCommitId()); assertSubmitter(change.getChangeId(), 1); assertSubmitter(change2.getChangeId(), 1); + assertPersonEquals(admin.getIdent(), head.getAuthorIdent()); + assertPersonEquals(admin.getIdent(), head.getCommitterIdent()); } @Test diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java index ebd3d3c8ba..64dad35384 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java @@ -41,6 +41,8 @@ public class SubmitByMergeAlwaysIT extends AbstractSubmitByMerge { assertThat(head.getParent(0)).isEqualTo(oldHead); assertThat(head.getParent(1)).isEqualTo(change.getCommitId()); assertSubmitter(change.getChangeId(), 1); + assertPersonEquals(admin.getIdent(), head.getAuthorIdent()); + assertPersonEquals(serverIdent.get(), head.getCommitterIdent()); } @Test diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java index a9beefec50..81cf489cf4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java @@ -29,6 +29,8 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge { assertThat(head.getId()).isEqualTo(change.getCommitId()); assertThat(head.getParent(0)).isEqualTo(oldHead); assertSubmitter(change.getChangeId(), 1); + assertPersonEquals(admin.getIdent(), head.getAuthorIdent()); + assertPersonEquals(admin.getIdent(), head.getCommitterIdent()); } @Test @@ -61,6 +63,8 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge { change2.getCommit().getShortMessage()); assertThat(tip.getParent(0).getId()).isEqualTo(initialHead.getId()); + assertPersonEquals(admin.getIdent(), tip.getAuthorIdent()); + assertPersonEquals(admin.getIdent(), tip.getCommitterIdent()); } @Test diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java index bb26a3043a..96372eb21d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java @@ -43,6 +43,8 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit { assertApproved(change.getChangeId()); assertCurrentRevision(change.getChangeId(), 1, head); assertSubmitter(change.getChangeId(), 1); + assertPersonEquals(admin.getIdent(), head.getAuthorIdent()); + assertPersonEquals(admin.getIdent(), head.getCommitterIdent()); } @Test @@ -65,6 +67,8 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit { assertCurrentRevision(change2.getChangeId(), 2, head); assertSubmitter(change2.getChangeId(), 1); assertSubmitter(change2.getChangeId(), 2); + assertPersonEquals(admin.getIdent(), head.getAuthorIdent()); + assertPersonEquals(serverIdent.get(), head.getCommitterIdent()); } @Test From 7063a4b8f0659d8b22bb62a349872e5350fb6a7b Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 10 Jul 2015 13:30:21 -0700 Subject: [PATCH 2/3] Merging Changes: Do not enter submitted state Recently the MergeQueue was removed, such that the submit process will directly tell users the result of their submission (merge or reject). For that changes were submitted and directly merged after. If problems (such as a server going down, power loss etc.) occur between setting the submitted state and the actual merge being performed, these changes would be stuck in the submitted state, which is annoying UI wise, but the user could just try to resubmit once the problems are solved. This change moves changes directly from NEW to MERGED if possible or rejects otherwise. We still need to keep lots of code in the Submit class to record the Submit label. Eventually this can be moved into the MergeOp as well. The MergeOp doesn't rely on the submitted state as well any more but just integrates the given changes. So changes which are submitted, but not merged on the same branch are left untouched now. So the tests which are testing exactly these corner cases of submitted changes on the same branch are adapted. Currently the test `submitMultipleChanges` is rather testing that the other changes on the same target branch are left alone. Instead we want to test if the submit strategy can submit changes both via fast forward as well as merging if it's necessary. After applying this change the name of `submitMultipleChanges` is correct again. There is no access to the submitter when running a submit strategy as it is not in the submitted state any more, so we need another way of transporting the information of who is trying to integrate the change to the submit strategy, so we do that via another argument in SubmitStrategy$Argument. The CherryPick strategy has a slight change in behavior as well. The committer for a cherry-picked change will be the server instead of the submitter as this aligns better with the other submit strategies, creating new commits on behalf of the user. Change-Id: I9a0c111c1cb3544774b2783c4f76318e2634faf2 --- .../google/gerrit/acceptance/TestAccount.java | 2 +- .../gerrit/acceptance/git/SubmitOnPushIT.java | 4 +- .../rest/change/AbstractSubmit.java | 36 ++- .../rest/change/SubmitByCherryPickIT.java | 82 ++---- .../rest/change/SubmitByMergeAlwaysIT.java | 21 +- .../change/SubmitByMergeIfNecessaryIT.java | 42 ++- .../change/SubmitByRebaseIfNecessaryIT.java | 2 +- .../acceptance/rest/project/GetCommitIT.java | 4 +- .../server/change/MergeabilityCacheImpl.java | 3 +- .../google/gerrit/server/change/Submit.java | 246 +---------------- .../google/gerrit/server/git/ChangeSet.java | 14 +- .../com/google/gerrit/server/git/MergeOp.java | 255 +++++++++++++----- .../google/gerrit/server/git/MergeUtil.java | 95 ++----- .../gerrit/server/git/ReceiveCommits.java | 10 +- .../server/git/strategy/CherryPick.java | 38 +-- .../server/git/strategy/FastForwardOnly.java | 8 +- .../server/git/strategy/MergeAlways.java | 19 +- .../server/git/strategy/MergeIfNecessary.java | 20 +- .../git/strategy/RebaseIfNecessary.java | 22 +- .../server/git/strategy/SubmitStrategy.java | 16 +- .../git/strategy/SubmitStrategyFactory.java | 4 +- .../gerrit/server/notedb/ChangeUpdate.java | 4 +- .../query/change/ConflictsPredicate.java | 2 +- .../query/change/InternalChangeQuery.java | 7 - .../notedb/CommitMessageOutputTest.java | 4 +- 25 files changed, 361 insertions(+), 599 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestAccount.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestAccount.java index e7b5834d78..4a6d22d692 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestAccount.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/TestAccount.java @@ -79,7 +79,7 @@ public class TestAccount { } public PersonIdent getIdent() { - return new PersonIdent(username, email); + return new PersonIdent(fullName, email); } public String getHttpUrl(GerritServer server) { 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 index 15c8a98289..9a8bb51a10 100644 --- 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 @@ -26,14 +26,12 @@ 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.server.ApprovalsUtil; -import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.git.CommitMergeStatus; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; @@ -120,7 +118,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { PushOneCommit.Result r = push("refs/for/master%submit", "other change", "a.txt", "other content"); r.assertErrorStatus(); - r.assertChange(Change.Status.NEW, null, admin); + r.assertChange(Change.Status.NEW, null); r.assertMessage(CommitMergeStatus.PATH_CONFLICT.getMessage()); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 20507fe013..68fd13a81e 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -41,7 +41,6 @@ import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.LabelInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.LabelId; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; @@ -72,8 +71,6 @@ import org.junit.Test; import java.io.ByteArrayOutputStream; import java.io.IOException; -import java.sql.Timestamp; -import java.util.Collections; import java.util.List; import java.util.Map; @@ -146,9 +143,11 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { change1.assertChange(Change.Status.MERGED, "test-topic", admin); change2.assertChange(Change.Status.MERGED, "test-topic", admin); change3.assertChange(Change.Status.MERGED, "test-topic", admin); + // Check for the exact change to have the correct submitter. + assertSubmitter(change3); + // Also check submitters for changes submitted via the topic relationship. assertSubmitter(change1); assertSubmitter(change2); - assertSubmitter(change3); } private void assertSubmitter(PushOneCommit.Result change) throws Exception { @@ -203,22 +202,6 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { submit(changeId, HttpStatus.SC_CONFLICT); } - protected void submitStatusOnly(String changeId) throws Exception { - approve(changeId); - Change c = queryProvider.get().byKeyPrefix(changeId).get(0).change(); - c.setStatus(Change.Status.SUBMITTED); - db.changes().update(Collections.singleton(c)); - db.patchSetApprovals().insert(Collections.singleton( - new PatchSetApproval( - new PatchSetApproval.Key( - c.currentPatchSetId(), - admin.id, - LabelId.SUBMIT), - (short) 1, - new Timestamp(System.currentTimeMillis())))); - indexer.index(db, c); - } - private void submit(String changeId, int expectedStatus) throws Exception { approve(changeId); SubmitInput subm = new SubmitInput(); @@ -266,6 +249,10 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } } + protected void assertNew(String changeId) throws Exception { + assertThat(get(changeId).status).isEqualTo(ChangeStatus.NEW); + } + protected void assertApproved(String changeId) throws Exception { ChangeInfo c = get(changeId, DETAILED_LABELS); LabelInfo cr = c.labels.get("Code-Review"); @@ -294,6 +281,15 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { assertThat(submitter.getAccountId()).isEqualTo(admin.getId()); } + protected void assertNoSubmitter(String changeId, int psId) + throws OrmException { + ChangeNotes cn = notesFactory.create( + getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change()); + PatchSetApproval submitter = approvalsUtil.getSubmitter( + db, cn, new PatchSet.Id(cn.getChangeId(), psId)); + assertThat(submitter).isNull(); + } + protected void assertCherryPick(TestRepository testRepo, boolean contentMerge) throws IOException { assertRebase(testRepo, contentMerge); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java index d4d190e276..b99b30a128 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java @@ -66,8 +66,7 @@ public class SubmitByCherryPickIT extends AbstractSubmit { assertSubmitter(change2.getChangeId(), 1); assertSubmitter(change2.getChangeId(), 2); assertPersonEquals(admin.getIdent(), newHead.getAuthorIdent()); - // TODO: Check the committer, too. - // assertPersonEquals(admin.getIdent(), newHead.getCommitterIdent()); + assertPersonEquals(admin.getIdent(), newHead.getCommitterIdent()); } @Test @@ -109,7 +108,7 @@ public class SubmitByCherryPickIT extends AbstractSubmit { submitWithConflict(change2.getChangeId()); assertThat(getRemoteHead()).isEqualTo(oldHead); assertCurrentRevision(change2.getChangeId(), 1, change2.getCommitId()); - assertSubmitter(change2.getChangeId(), 1); + assertNoSubmitter(change2.getChangeId(), 1); } @Test @@ -149,7 +148,7 @@ public class SubmitByCherryPickIT extends AbstractSubmit { submitWithConflict(change3.getChangeId()); assertThat(getRemoteHead()).isEqualTo(oldHead); assertCurrentRevision(change3.getChangeId(), 1, change3.getCommitId()); - assertSubmitter(change3.getChangeId(), 1); + assertNoSubmitter(change3.getChangeId(), 1); } @Test @@ -165,24 +164,17 @@ public class SubmitByCherryPickIT extends AbstractSubmit { testRepo.reset(initialHead); PushOneCommit.Result change4 = createChange("Change 4", "d", "d"); - submitStatusOnly(change2.getChangeId()); - submitStatusOnly(change3.getChangeId()); + approve(change2.getChangeId()); + approve(change3.getChangeId()); submit(change4.getChangeId()); List log = getRemoteLog(); assertThat(log.get(0).getShortMessage()).isEqualTo( change4.getCommit().getShortMessage()); - assertThat(log.get(0).getParent(0)).isEqualTo(log.get(1)); + assertThat(log.get(1).getId()).isEqualTo(initialHead.getId()); - assertThat(log.get(1).getShortMessage()).isEqualTo( - change3.getCommit().getShortMessage()); - assertThat(log.get(1).getParent(0)).isEqualTo(log.get(2)); - - assertThat(log.get(2).getShortMessage()).isEqualTo( - change2.getCommit().getShortMessage()); - assertThat(log.get(2).getParent(0)).isEqualTo(log.get(3)); - - assertThat(log.get(3).getId()).isEqualTo(initialHead.getId()); + assertNew(change2.getChangeId()); + assertNew(change3.getChangeId()); } @Test @@ -234,6 +226,7 @@ public class SubmitByCherryPickIT extends AbstractSubmit { // Tip has not changed. List log = getRemoteLog(); assertThat(log.get(0)).isEqualTo(initialHead.getId()); + assertNoSubmitter(change3.getChangeId(), 1); } @Test @@ -241,57 +234,16 @@ public class SubmitByCherryPickIT extends AbstractSubmit { RevCommit initialHead = getRemoteHead(); testRepo.reset(initialHead); - createChange("Change 2", "b", "b"); + PushOneCommit.Result change2 = createChange("Change 2", "b", "b"); PushOneCommit.Result change3 = createChange("Change 3", "c", "c"); - createChange("Change 4", "d", "d"); - PushOneCommit.Result change5 = createChange("Change 5", "e", "e"); + PushOneCommit.Result change4 = createChange("Change 5", "e", "e"); - // Out of the above, only submit 3 and 5. - submitStatusOnly(change3.getChangeId()); - submit(change5.getChangeId()); + // Out of the above, only submit 4. 2,3 are not related to 4 + // by topic or ancestor (due to cherrypicking!) + approve(change3.getChangeId()); + submit(change4.getChangeId()); - ChangeInfo info3 = get(change3.getChangeId()); - assertThat(info3.status).isEqualTo(ChangeStatus.MERGED); - - List log = getRemoteLog(); - assertThat(log.get(0).getShortMessage()) - .isEqualTo(change5.getCommit().getShortMessage()); - assertThat(log.get(1).getShortMessage()) - .isEqualTo(change3.getCommit().getShortMessage()); - assertThat(log.get(2).getShortMessage()) - .isEqualTo(initialHead.getShortMessage()); - } - - @Test - public void submitChangeAfterParentFailsDueToConflict() throws Exception { - RevCommit initialHead = getRemoteHead(); - - testRepo.reset(initialHead); - PushOneCommit.Result change2 = createChange("Change 2", "b", "b1"); - submit(change2.getChangeId()); - - testRepo.reset(initialHead); - PushOneCommit.Result change3 = createChange("Change 3", "b", "b2"); - assertThat(change3.getCommit().getParent(0)).isEqualTo(initialHead); - PushOneCommit.Result change4 = createChange("Change 3", "c", "c3"); - - submitStatusOnly(change3.getChangeId()); - submitStatusOnly(change4.getChangeId()); - - // Merge fails; change3 contains the delta "b1" -> "b2", which cannot be - // applied against tip. - // As change4 sits on top of change 3 we need to trigger submission there - // to include it into the mergeing - submitWithConflict(change4.getChangeId()); - - // change4 is a clean merge, so should succeed in the same run where change3 - // failed. - ChangeInfo info4 = get(change4.getChangeId()); - assertThat(info4.status).isEqualTo(ChangeStatus.MERGED); - List log = getRemoteLog(); - assertThat(log.get(0).getShortMessage()) - .isEqualTo(change4.getCommit().getShortMessage()); - assertThat(log.get(1).getShortMessage()) - .isEqualTo(change2.getCommit().getShortMessage()); + assertNew(change2.getChangeId()); + assertNew(change3.getChangeId()); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java index 64dad35384..eb1d16b70d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeAlwaysIT.java @@ -58,23 +58,22 @@ public class SubmitByMergeAlwaysIT extends AbstractSubmitByMerge { testRepo.reset(initialHead); PushOneCommit.Result change4 = createChange("Change 4", "d", "d"); - submitStatusOnly(change2.getChangeId()); - submitStatusOnly(change3.getChangeId()); + approve(change2.getChangeId()); + approve(change3.getChangeId()); submit(change4.getChangeId()); List log = getRemoteLog(); RevCommit tip = log.get(0); assertThat(tip.getParent(1).getShortMessage()).isEqualTo( change4.getCommit().getShortMessage()); - - tip = tip.getParent(0); - assertThat(tip.getParent(1).getShortMessage()).isEqualTo( - change3.getCommit().getShortMessage()); - - tip = tip.getParent(0); - assertThat(tip.getParent(1).getShortMessage()).isEqualTo( - change2.getCommit().getShortMessage()); - + assertThat(tip.getParent(0).getShortMessage()).isEqualTo( + initialHead.getShortMessage()); assertThat(tip.getParent(0).getId()).isEqualTo(initialHead.getId()); + + assertPersonEquals(admin.getIdent(), tip.getAuthorIdent()); + assertPersonEquals(serverIdent.get(), tip.getCommitterIdent()); + + assertNew(change2.getChangeId()); + assertNew(change3.getChangeId()); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java index 81cf489cf4..032cb7d58b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java @@ -46,25 +46,32 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge { testRepo.reset(initialHead); PushOneCommit.Result change4 = createChange("Change 4", "d", "d"); - submitStatusOnly(change2.getChangeId()); - submitStatusOnly(change3.getChangeId()); - submit(change4.getChangeId()); + // Change 2 stays untouched. + approve(change2.getChangeId()); + // Change 3 is a fast-forward, no need to merge. + submit(change3.getChangeId()); RevCommit tip = getRemoteLog().get(0); - assertThat(tip.getParent(1).getShortMessage()).isEqualTo( - change4.getCommit().getShortMessage()); - - tip = tip.getParent(0); - assertThat(tip.getParent(1).getShortMessage()).isEqualTo( - change3.getCommit().getShortMessage()); - - tip = tip.getParent(0); assertThat(tip.getShortMessage()).isEqualTo( - change2.getCommit().getShortMessage()); - - assertThat(tip.getParent(0).getId()).isEqualTo(initialHead.getId()); + change3.getCommit().getShortMessage()); + assertThat(tip.getParent(0).getId()).isEqualTo( + initialHead.getId()); assertPersonEquals(admin.getIdent(), tip.getAuthorIdent()); assertPersonEquals(admin.getIdent(), tip.getCommitterIdent()); + + // We need to merge change 4. + submit(change4.getChangeId()); + + tip = getRemoteLog().get(0); + assertThat(tip.getParent(1).getShortMessage()).isEqualTo( + change4.getCommit().getShortMessage()); + assertThat(tip.getParent(0).getShortMessage()).isEqualTo( + change3.getCommit().getShortMessage()); + + assertPersonEquals(admin.getIdent(), tip.getAuthorIdent()); + assertPersonEquals(serverIdent.get(), tip.getCommitterIdent()); + + assertNew(change2.getChangeId()); } @Test @@ -188,6 +195,10 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge { initialHead2.getShortMessage()); assertThat(tip3.getShortMessage()).isEqualTo( change3Conflict.getCommit().getShortMessage()); + assertNoSubmitter(change1a.getChangeId(), 1); + assertNoSubmitter(change2a.getChangeId(), 1); + assertNoSubmitter(change2b.getChangeId(), 1); + assertNoSubmitter(change3.getChangeId(), 1); } else { assertThat(tip1.getShortMessage()).isEqualTo( change1b.getCommit().getShortMessage()); @@ -195,6 +206,9 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge { initialHead2.getShortMessage()); assertThat(tip3.getShortMessage()).isEqualTo( change3Conflict.getCommit().getShortMessage()); + assertNoSubmitter(change2a.getChangeId(), 1); + assertNoSubmitter(change2b.getChangeId(), 1); + assertNoSubmitter(change3.getChangeId(), 1); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java index 96372eb21d..d1d3bdf84b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java @@ -111,6 +111,6 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit { RevCommit head = getRemoteHead(); assertThat(head).isEqualTo(oldHead); assertCurrentRevision(change2.getChangeId(), 1, change2.getCommitId()); - assertSubmitter(change2.getChangeId(), 1); + assertNoSubmitter(change2.getChangeId(), 1); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetCommitIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetCommitIT.java index d3677ab900..7044ad0f22 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetCommitIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetCommitIT.java @@ -102,9 +102,9 @@ public class GetCommitIT extends AbstractDaemonTest { assertThat(info.subject).isEqualTo("test commit"); assertThat(info.message).isEqualTo( "test commit\n\nChange-Id: " + r.getChangeId() + "\n"); - assertThat(info.author.name).isEqualTo("admin"); + assertThat(info.author.name).isEqualTo("Administrator"); assertThat(info.author.email).isEqualTo("admin@example.com"); - assertThat(info.committer.name).isEqualTo("admin"); + assertThat(info.committer.name).isEqualTo("Administrator"); assertThat(info.committer.email).isEqualTo("admin@example.com"); CommitInfo parent = Iterables.getOnlyElement(info.parents); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java index a840651f55..14aa5b65b8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java @@ -244,7 +244,8 @@ public class MergeabilityCacheImpl implements MergeabilityCache { null /*inserter*/, canMerge, accepted, - key.load.dest).dryRun(tip, rev); + key.load.dest, + null).dryRun(tip, rev); } } finally { key.load = null; 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 6624bd4038..15e181baff 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 @@ -18,17 +18,12 @@ import static com.google.gerrit.common.data.SubmitRecord.Status.OK; import com.google.common.base.MoreObjects; import com.google.common.base.Optional; -import com.google.common.base.Preconditions; import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.collect.FluentIterable; -import com.google.common.collect.HashBasedTable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; -import com.google.common.collect.Maps; -import com.google.common.collect.Table; -import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.ParameterizedString; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.extensions.api.changes.SubmitInput; @@ -38,34 +33,24 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.extensions.webui.UiAction; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; -import com.google.gerrit.reviewdb.client.LabelId; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; -import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ProjectUtil; import com.google.gerrit.server.account.AccountsCollection; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.ChangeSet; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.git.LabelNormalizer; import com.google.gerrit.server.git.MergeOp; -import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate; -import com.google.gerrit.server.index.ChangeIndexer; -import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; -import com.google.gwtorm.server.AtomicUpdate; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmRuntimeException; import com.google.inject.Inject; @@ -73,15 +58,12 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import org.eclipse.jgit.errors.RepositoryNotFoundException; -import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.PersonIdent; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; -import java.sql.Timestamp; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -120,17 +102,11 @@ public class Submit implements RestModifyView, } } - private final PersonIdent serverIdent; private final Provider dbProvider; private final GitRepositoryManager repoManager; - private final IdentifiedUser.GenericFactory userFactory; private final ChangeData.Factory changeDataFactory; - private final ChangeUpdate.Factory updateFactory; - private final ApprovalsUtil approvalsUtil; private final ChangeMessagesUtil cmUtil; private final Provider mergeOpProvider; - private final ChangeIndexer indexer; - private final LabelNormalizer labelNormalizer; private final AccountsCollection accounts; private final ChangesCollection changes; private final String label; @@ -141,34 +117,22 @@ public class Submit implements RestModifyView, private final Provider queryProvider; @Inject - Submit(@GerritPersonIdent PersonIdent serverIdent, - Provider dbProvider, + Submit(Provider dbProvider, GitRepositoryManager repoManager, - IdentifiedUser.GenericFactory userFactory, ChangeData.Factory changeDataFactory, - ChangeUpdate.Factory updateFactory, - ApprovalsUtil approvalsUtil, ChangeMessagesUtil cmUtil, Provider mergeOpProvider, AccountsCollection accounts, ChangesCollection changes, - ChangeIndexer indexer, - LabelNormalizer labelNormalizer, @GerritServerConfig Config cfg, Provider queryProvider) { - this.serverIdent = serverIdent; this.dbProvider = dbProvider; this.repoManager = repoManager; - this.userFactory = userFactory; this.changeDataFactory = changeDataFactory; - this.updateFactory = updateFactory; - this.approvalsUtil = approvalsUtil; this.cmUtil = cmUtil; this.mergeOpProvider = mergeOpProvider; this.accounts = accounts; this.changes = changes; - this.indexer = indexer; - this.labelNormalizer = labelNormalizer; this.label = MoreObjects.firstNonNull( Strings.emptyToNull(cfg.getString("change", null, "submitLabel")), "Submit"); @@ -212,7 +176,16 @@ public class Submit implements RestModifyView, rsrc.getPatchSet().getRevision().get())); } - ChangeSet submittedChanges = ChangeSet.create(submit(rsrc, caller, false)); + List changes; + if (submitWholeTopic && !Strings.isNullOrEmpty(change.getTopic())) { + changes = new ArrayList<>(); + for (ChangeData cd : getChangesByTopic(change.getTopic())) { + changes.add(cd.change()); + } + } else { + changes = Arrays.asList(change); + } + ChangeSet submittedChanges = ChangeSet.create(changes); try { mergeOpProvider.get().merge(submittedChanges, caller, true); @@ -374,203 +347,6 @@ public class Submit implements RestModifyView, .orNull(); } - private Change submitToDatabase(final ReviewDb db, final Change.Id changeId, - final Timestamp timestamp) throws OrmException, - ResourceConflictException { - Change ret = db.changes().atomicUpdate(changeId, - new AtomicUpdate() { - @Override - public Change update(Change change) { - if (change.getStatus().isOpen()) { - change.setStatus(Change.Status.SUBMITTED); - change.setLastUpdatedOn(timestamp); - return change; - } - return null; - } - }); - if (ret != null) { - return ret; - } else { - throw new ResourceConflictException("change " + changeId + " is " - + status(db.changes().get(changeId))); - } - } - - private Change submitThisChange(RevisionResource rsrc, IdentifiedUser caller, - boolean force) throws ResourceConflictException, OrmException, - IOException { - ReviewDb db = dbProvider.get(); - ChangeData cd = changeDataFactory.create(db, rsrc.getControl()); - List submitRecords = checkSubmitRule(cd, - rsrc.getPatchSet(), force); - - final Timestamp timestamp = TimeUtil.nowTs(); - Change change = rsrc.getChange(); - ChangeUpdate update = updateFactory.create(rsrc.getControl(), timestamp); - update.submit(submitRecords); - - db.changes().beginTransaction(change.getId()); - try { - BatchMetaDataUpdate batch = approve(rsrc.getPatchSet().getId(), - cd.changeControl(), update, caller, timestamp); - // Write update commit after all normalized label commits. - batch.write(update, new CommitBuilder()); - change = submitToDatabase(db, change.getId(), timestamp); - db.commit(); - } finally { - db.rollback(); - } - indexer.index(db, change); - return change; - } - - private List submitWholeTopic(RevisionResource rsrc, IdentifiedUser caller, - boolean force, String topic) throws ResourceConflictException, OrmException, - IOException { - Preconditions.checkNotNull(topic); - final Timestamp timestamp = TimeUtil.nowTs(); - - ReviewDb db = dbProvider.get(); - ChangeData cd = changeDataFactory.create(db, rsrc.getControl()); - - List changesByTopic = getChangesByTopic(topic); - String problems = problemsForSubmittingChanges(changesByTopic, caller); - if (problems != null) { - throw new ResourceConflictException(problems); - } - - Change change = rsrc.getChange(); - ChangeUpdate update = updateFactory.create(rsrc.getControl(), timestamp); - - List submitRecords = checkSubmitRule(cd, - rsrc.getPatchSet(), force); - update.submit(submitRecords); - - db.changes().beginTransaction(change.getId()); - try { - for (ChangeData c : changesByTopic) { - BatchMetaDataUpdate batch = approve(c.currentPatchSet().getId(), - c.changeControl(), update, caller, timestamp); - // Write update commit after all normalized label commits. - batch.write(update, new CommitBuilder()); - submitToDatabase(db, c.getId(), timestamp); - } - db.commit(); - } finally { - db.rollback(); - } - List ids = new ArrayList<>(changesByTopic.size()); - List ret = new ArrayList<>(changesByTopic.size()); - for (ChangeData c : changesByTopic) { - ids.add(c.getId()); - ret.add(c.change()); - } - indexer.indexAsync(ids).checkedGet(); - - return ret; - } - - public List submit(RevisionResource rsrc, IdentifiedUser caller, - boolean force) throws ResourceConflictException, OrmException, - IOException { - String topic = rsrc.getChange().getTopic(); - if (submitWholeTopic && !Strings.isNullOrEmpty(topic)) { - return submitWholeTopic(rsrc, caller, force, topic); - } else { - return Arrays.asList(submitThisChange(rsrc, caller, force)); - } - } - - private BatchMetaDataUpdate approve(PatchSet.Id psId, ChangeControl control, - ChangeUpdate update, IdentifiedUser caller, Timestamp timestamp) - throws OrmException { - Map byKey = Maps.newHashMap(); - for (PatchSetApproval psa : - approvalsUtil.byPatchSet(dbProvider.get(), control, psId)) { - if (!byKey.containsKey(psa.getKey())) { - byKey.put(psa.getKey(), psa); - } - } - - PatchSetApproval submit = ApprovalsUtil.getSubmitter(psId, byKey.values()); - if (submit == null - || !submit.getAccountId().equals(caller.getAccountId())) { - submit = new PatchSetApproval( - new PatchSetApproval.Key( - psId, - caller.getAccountId(), - LabelId.SUBMIT), - (short) 1, TimeUtil.nowTs()); - byKey.put(submit.getKey(), submit); - } - submit.setValue((short) 1); - submit.setGranted(timestamp); - - // Flatten out existing approvals for this patch set based upon the current - // permissions. Once the change is closed the approvals are not updated at - // presentation view time, except for zero votes used to indicate a reviewer - // was added. So we need to make sure votes are accurate now. This way if - // permissions get modified in the future, historical records stay accurate. - LabelNormalizer.Result normalized = - labelNormalizer.normalize(control, byKey.values()); - - // TODO(dborowitz): Don't use a label in notedb; just check when status - // change happened. - update.putApproval(submit.getLabel(), submit.getValue()); - - dbProvider.get().patchSetApprovals().upsert(normalized.getNormalized()); - dbProvider.get().patchSetApprovals().delete(normalized.deleted()); - - try { - return saveToBatch(control, update, normalized, timestamp); - } catch (IOException e) { - throw new OrmException(e); - } - } - - private BatchMetaDataUpdate saveToBatch(ChangeControl ctl, - ChangeUpdate callerUpdate, LabelNormalizer.Result normalized, - Timestamp timestamp) throws IOException { - Table> byUser = HashBasedTable.create(); - for (PatchSetApproval psa : normalized.updated()) { - byUser.put(psa.getAccountId(), psa.getLabel(), - Optional.of(psa.getValue())); - } - for (PatchSetApproval psa : normalized.deleted()) { - byUser.put(psa.getAccountId(), psa.getLabel(), Optional. absent()); - } - - BatchMetaDataUpdate batch = callerUpdate.openUpdate(); - for (Account.Id accountId : byUser.rowKeySet()) { - if (!accountId.equals(callerUpdate.getUser().getAccountId())) { - ChangeUpdate update = updateFactory.create( - ctl.forUser(userFactory.create(dbProvider, accountId)), timestamp); - update.setSubject("Finalize approvals at submit"); - putApprovals(update, byUser.row(accountId)); - - CommitBuilder commit = new CommitBuilder(); - commit.setCommitter(new PersonIdent(serverIdent, timestamp)); - batch.write(update, commit); - } - } - - putApprovals(callerUpdate, - byUser.row(callerUpdate.getUser().getAccountId())); - return batch; - } - - private static void putApprovals(ChangeUpdate update, - Map> approvals) { - for (Map.Entry> e : approvals.entrySet()) { - if (e.getValue().isPresent()) { - update.putApproval(e.getKey(), e.getValue().get()); - } else { - update.removeApproval(e.getKey()); - } - } - } - private List checkSubmitRule(ChangeData cd, PatchSet patchSet, boolean force) throws ResourceConflictException, OrmException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java index b906ba7b35..6c3b49908f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java @@ -33,18 +33,22 @@ public abstract class ChangeSet { ImmutableSetMultimap.builder(); ImmutableSetMultimap.Builder pcb = ImmutableSetMultimap.builder(); + ImmutableSetMultimap.Builder cbb = + ImmutableSetMultimap.builder(); for (Change c : changes) { - Project.NameKey project = c.getDest().getParentKey(); + Branch.NameKey branch = c.getDest(); + Project.NameKey project = branch.getParentKey(); pb.add(project); - bb.add(c.getDest()); + bb.add(branch); ib.add(c.getId()); - pbb.put(project, c.getDest()); + pbb.put(project, branch); pcb.put(project, c.getId()); + cbb.put(branch, c.getId()); } return new AutoValue_ChangeSet(pb.build(), bb.build(), - ib.build(), pbb.build(), pcb.build()); + ib.build(), pbb.build(), pcb.build(), cbb.build()); } public static ChangeSet create(Change change) { @@ -58,6 +62,8 @@ public abstract class ChangeSet { branchesByProject(); public abstract ImmutableSetMultimap changesByProject(); + public abstract ImmutableSetMultimap + changesByBranch(); @Override public int hashCode() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index b792bce4cc..ff3d992536 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -20,11 +20,12 @@ import static org.eclipse.jgit.lib.RefDatabase.ALL; import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.HashBasedTable; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; -import com.google.common.collect.Lists; -import com.google.common.collect.Sets; +import com.google.common.collect.Maps; +import com.google.common.collect.Table; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.SubmitRecord; @@ -35,6 +36,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.LabelId; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; @@ -44,15 +46,14 @@ import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.RemotePeer; import com.google.gerrit.server.account.AccountCache; -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.GerritRequestModule; import com.google.gerrit.server.config.RequestScopedReviewDbProvider; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate; import com.google.gerrit.server.git.strategy.SubmitStrategy; import com.google.gerrit.server.git.strategy.SubmitStrategyFactory; import com.google.gerrit.server.git.validators.MergeValidationException; @@ -89,6 +90,7 @@ import com.jcraft.jsch.HostKey; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; @@ -105,6 +107,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.net.SocketAddress; +import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -144,6 +147,7 @@ public class MergeOp { private final GitReferenceUpdated gitRefUpdated; private final GitRepositoryManager repoManager; private final IdentifiedUser.GenericFactory identifiedUserFactory; + private final LabelNormalizer labelNormalizer; private final MergedSender.Factory mergedSenderFactory; private final MergeSuperSet mergeSuperSet; private final MergeValidators.Factory mergeValidatorsFactory; @@ -151,12 +155,13 @@ public class MergeOp { private final ProjectCache projectCache; private final InternalChangeQuery internalChangeQuery; private final SchemaFactory schemaFactory; - private final Submit submit; + private final PersonIdent serverIdent; private final SubmitStrategyFactory submitStrategyFactory; private final Provider subOpProvider; private final TagCache tagCache; private final WorkQueue workQueue; + private final Map> records; private final Map commits; private final PerThreadRequestScope.Scoper threadScoper; private String logPrefix; @@ -185,6 +190,7 @@ public class MergeOp { GitReferenceUpdated gitRefUpdated, GitRepositoryManager repoManager, IdentifiedUser.GenericFactory identifiedUserFactory, + LabelNormalizer labelNormalizer, MergedSender.Factory mergedSenderFactory, MergeSuperSet mergeSuperSet, MergeValidators.Factory mergeValidatorsFactory, @@ -192,7 +198,7 @@ public class MergeOp { ProjectCache projectCache, InternalChangeQuery internalChangeQuery, SchemaFactory schemaFactory, - Submit submit, + @GerritPersonIdent PersonIdent serverIdent, SubmitStrategyFactory submitStrategyFactory, Provider subOpProvider, TagCache tagCache, @@ -208,6 +214,7 @@ public class MergeOp { this.gitRefUpdated = gitRefUpdated; this.repoManager = repoManager; this.identifiedUserFactory = identifiedUserFactory; + this.labelNormalizer = labelNormalizer; this.mergedSenderFactory = mergedSenderFactory; this.mergeSuperSet = mergeSuperSet; this.mergeValidatorsFactory = mergeValidatorsFactory; @@ -215,7 +222,7 @@ public class MergeOp { this.projectCache = projectCache; this.internalChangeQuery = internalChangeQuery; this.schemaFactory = schemaFactory; - this.submit = submit; + this.serverIdent = serverIdent; this.submitStrategyFactory = submitStrategyFactory; this.subOpProvider = subOpProvider; this.tagCache = tagCache; @@ -223,6 +230,8 @@ public class MergeOp { commits = new HashMap<>(); pendingRefUpdates = new HashMap<>(); openBranches = new HashMap<>(); + pendingRefUpdates = new HashMap<>(); + records = new HashMap<>(); mergeTips = new HashMap<>(); Injector child = injector.createChildInjector(new AbstractModule() { @@ -384,35 +393,7 @@ public class MergeOp { throw new OrmException("Change " + cd.change().getChangeId() + " is in state " + cd.change().getStatus()); } else { - checkSubmitRule(cd); - } - } - } - - // For historic reasons we will first go into the submitted state - // TODO(sbeller): remove this when we get rid of Change.Status.SUBMITTED - private void submitAllChanges(ChangeSet cs, IdentifiedUser caller, - boolean force) throws OrmException, ResourceConflictException, - IOException { - for (Change.Id id : cs.ids()) { - ChangeData cd = changeDataFactory.create(db, id); - switch (cd.change().getStatus()) { - case ABANDONED: - throw new ResourceConflictException("Change " + cd.getId() + - " was abandoned while processing this change set"); - case DRAFT: - throw new ResourceConflictException("Cannot submit draft " + cd.getId()); - case NEW: - RevisionResource rsrc = - new RevisionResource(new ChangeResource(cd.changeControl(), null), - cd.currentPatchSet()); - logDebug("Submitting change id {}", cd.change().getId()); - submit.submit(rsrc, caller, force); - break; - case MERGED: - // we're racing here, but having it already merged is fine. - case SUBMITTED: - // ok + records.put(cd.change().getId(), checkSubmitRule(cd)); } } } @@ -428,17 +409,11 @@ public class MergeOp { ChangeSet cs = mergeSuperSet.completeChangeSet(db, changes); logDebug("Calculated to merge {}", cs); if (checkPermissions) { - logDebug("Submitting all calculated changes while " - + "enforcing submit rules"); - submitAllChanges(cs, caller, false); logDebug("Checking permissions"); checkPermissions(cs); - } else { - logDebug("Submitting all calculated changes ignoring submit rules"); - submitAllChanges(cs, caller, true); } try { - integrateIntoHistory(cs); + integrateIntoHistory(cs, caller); } catch (MergeException e) { logError("Merge Conflict", e); throw new ResourceConflictException("Merge Conflict", e); @@ -453,10 +428,10 @@ public class MergeOp { } } - private void integrateIntoHistory(ChangeSet cs) + private void integrateIntoHistory(ChangeSet cs, IdentifiedUser caller) throws MergeException, NoSuchChangeException, ResourceConflictException { logDebug("Beginning merge attempt on {}", cs); - Map> toSubmit = + Map> toSubmit = new HashMap<>(); try { openSchema(); @@ -465,24 +440,25 @@ public class MergeOp { openRepository(project); for (Branch.NameKey branch : cs.branchesByProject().get(project)) { setDestProject(branch); - ListMultimap submitting = - validateChangeList(internalChangeQuery.submitted(branch)); + + List cds = new ArrayList<>(); + for (Change.Id id : cs.changesByBranch().get(branch)) { + cds.add(changeDataFactory.create(db, id)); + } + ListMultimap submitting = + validateChangeList(cds); toSubmit.put(branch, submitting); Set submitTypes = new HashSet<>(submitting.keySet()); for (SubmitType submitType : submitTypes) { SubmitStrategy strategy = createStrategy(branch, submitType, - getBranchTip(branch)); + getBranchTip(branch), caller); MergeTip mergeTip = preMerge(strategy, submitting.get(submitType), getBranchTip(branch)); mergeTips.put(branch, mergeTip); - if (submitType != SubmitType.CHERRY_PICK) { - // For cherry picking we have relaxed atomic guarantees - // as traditionally Gerrit kept going cherry picking if one - // failed. We want to keep it for now. - updateChangeStatus(submitting.get(submitType), branch, true); - } + updateChangeStatus(submitting.get(submitType), branch, + true, caller); } inserter.flush(); } @@ -498,9 +474,10 @@ public class MergeOp { pendingRefUpdates.remove(branch); setDestProject(branch); - ListMultimap submitting = toSubmit.get(branch); + ListMultimap submitting = toSubmit.get(branch); for (SubmitType submitType : submitting.keySet()) { - updateChangeStatus(submitting.get(submitType), branch, false); + updateChangeStatus(submitting.get(submitType), branch, + false, caller); updateSubmoduleSubscriptions(subOp, branch, getBranchTip(branch)); } if (update != null) { @@ -526,15 +503,15 @@ public class MergeOp { } private MergeTip preMerge(SubmitStrategy strategy, - List submitted, CodeReviewCommit branchTip) - throws MergeException { + List submitted, CodeReviewCommit branchTip) + throws MergeException, OrmException { logDebug("Running submit strategy {} for {} commits {}", strategy.getClass().getSimpleName(), submitted.size(), submitted); List toMerge = new ArrayList<>(submitted.size()); - for (Change c : submitted) { - CodeReviewCommit commit = commits.get(c.getId()); + for (ChangeData cd : submitted) { + CodeReviewCommit commit = commits.get(cd.change().getId()); checkState(commit != null, - "commit for %s not found by validateChangeList", c.getId()); + "commit for %s not found by validateChangeList", cd.change().getId()); toMerge.add(commit); } MergeTip mergeTip = strategy.run(branchTip, toMerge); @@ -545,10 +522,10 @@ public class MergeOp { } private SubmitStrategy createStrategy(Branch.NameKey destBranch, - SubmitType submitType, CodeReviewCommit branchTip) + SubmitType submitType, CodeReviewCommit branchTip, IdentifiedUser caller) throws MergeException, NoSuchProjectException { return submitStrategyFactory.create(submitType, db, repo, rw, inserter, - canMergeFlag, getAlreadyAccepted(branchTip), destBranch); + canMergeFlag, getAlreadyAccepted(branchTip), destBranch, caller); } private void openRepository(Project.NameKey name) @@ -649,10 +626,10 @@ public class MergeOp { return alreadyAccepted; } - private ListMultimap validateChangeList( + private ListMultimap validateChangeList( List submitted) throws MergeException { logDebug("Validating {} changes", submitted.size()); - ListMultimap toSubmit = ArrayListMultimap.create(); + ListMultimap toSubmit = ArrayListMultimap.create(); Map allRefs; try { @@ -677,8 +654,9 @@ public class MergeOp { throw new MergeException("Failed to validate changes", e); } Change.Id changeId = cd.getId(); - if (chg.getStatus() != Change.Status.SUBMITTED) { - logDebug("Change {} is not submitted: {}", changeId, chg.getStatus()); + if (chg.getStatus() != Change.Status.SUBMITTED + && chg.getStatus() != Change.Status.NEW) { + logDebug("Change {} is not new or submitted: {}", changeId, chg.getStatus()); continue; } if (chg.currentPatchSetId() == null) { @@ -762,7 +740,7 @@ public class MergeOp { } commit.add(canMergeFlag); - toSubmit.put(submitType, chg); + toSubmit.put(submitType, cd); } logDebug("Submitting on this run: {}", toSubmit); return toSubmit; @@ -881,9 +859,10 @@ public class MergeOp { return ""; } - private void updateChangeStatus(List submitted, - Branch.NameKey destBranch, boolean dryRun) - throws NoSuchChangeException, MergeException, ResourceConflictException { + private void updateChangeStatus(List submitted, + Branch.NameKey destBranch, boolean dryRun, IdentifiedUser caller) + throws NoSuchChangeException, MergeException, ResourceConflictException, + OrmException { if (!dryRun) { logDebug("Updating change status for {} changes", submitted.size()); } else { @@ -891,7 +870,8 @@ public class MergeOp { submitted.size()); } MergeTip mergeTip = mergeTips.get(destBranch); - for (Change c : submitted) { + for (ChangeData cd : submitted) { + Change c = cd.change(); CodeReviewCommit commit = commits.get(c.getId()); CommitMergeStatus s = commit != null ? commit.getStatusCode() : null; if (s == null) { @@ -903,6 +883,14 @@ public class MergeOp { continue; } + if (!dryRun) { + try { + setApproval(cd, caller); + } catch (IOException e) { + throw new OrmException(e); + } + } + String txt = s.getMessage(); logDebug("Status of change {} ({}) on {}: {}", c.getId(), commit.name(), c.getDest(), s); @@ -1093,6 +1081,127 @@ public class MergeOp { }); } + private void setApproval(ChangeData cd, IdentifiedUser user) + throws OrmException, IOException { + Timestamp timestamp = TimeUtil.nowTs(); + ChangeControl control = cd.changeControl(); + PatchSet.Id psId = cd.currentPatchSet().getId(); + PatchSet.Id psIdNewRev = commits.get(cd.change().getId()) + .change().currentPatchSetId(); + + logDebug("Add approval for " + cd + " from user " + user); + ChangeUpdate update = updateFactory.create(control, timestamp); + List record = records.get(cd.change().getId()); + if (record != null) { + update.submit(record); + } + db.changes().beginTransaction(cd.change().getId()); + try { + BatchMetaDataUpdate batch = approve(control, psId, user, + update, timestamp); + batch.write(update, new CommitBuilder()); + + // If the submit strategy created a new revision (rebase, cherry-pick) + // approve that as well + if (!psIdNewRev.equals(psId)) { + batch = approve(control, psIdNewRev, user, + update, timestamp); + // Write update commit after all normalized label commits. + batch.write(update, new CommitBuilder()); + } + db.commit(); + } finally { + db.rollback(); + } + indexer.index(db, cd.change()); + } + + private BatchMetaDataUpdate approve(ChangeControl control, PatchSet.Id psId, + IdentifiedUser user, ChangeUpdate update, Timestamp timestamp) + throws OrmException { + Map byKey = Maps.newHashMap(); + for (PatchSetApproval psa : + approvalsUtil.byPatchSet(db, control, psId)) { + if (!byKey.containsKey(psa.getKey())) { + byKey.put(psa.getKey(), psa); + } + } + + PatchSetApproval submit = new PatchSetApproval( + new PatchSetApproval.Key( + psId, + user.getAccountId(), + LabelId.SUBMIT), + (short) 1, TimeUtil.nowTs()); + byKey.put(submit.getKey(), submit); + submit.setValue((short) 1); + submit.setGranted(timestamp); + + // Flatten out existing approvals for this patch set based upon the current + // permissions. Once the change is closed the approvals are not updated at + // presentation view time, except for zero votes used to indicate a reviewer + // was added. So we need to make sure votes are accurate now. This way if + // permissions get modified in the future, historical records stay accurate. + LabelNormalizer.Result normalized = + labelNormalizer.normalize(control, byKey.values()); + + // TODO(dborowitz): Don't use a label in notedb; just check when status + // change happened. + update.putApproval(submit.getLabel(), submit.getValue()); + logDebug("Adding submit label " + submit); + + db.patchSetApprovals().upsert(normalized.getNormalized()); + db.patchSetApprovals().delete(normalized.deleted()); + + try { + return saveToBatch(control, update, normalized, timestamp); + } catch (IOException e) { + throw new OrmException(e); + } + } + + private BatchMetaDataUpdate saveToBatch(ChangeControl ctl, + ChangeUpdate callerUpdate, LabelNormalizer.Result normalized, + Timestamp timestamp) throws IOException { + Table> byUser = HashBasedTable.create(); + for (PatchSetApproval psa : normalized.updated()) { + byUser.put(psa.getAccountId(), psa.getLabel(), + Optional.of(psa.getValue())); + } + for (PatchSetApproval psa : normalized.deleted()) { + byUser.put(psa.getAccountId(), psa.getLabel(), Optional. absent()); + } + + BatchMetaDataUpdate batch = callerUpdate.openUpdate(); + for (Account.Id accountId : byUser.rowKeySet()) { + if (!accountId.equals(callerUpdate.getUser().getAccountId())) { + ChangeUpdate update = updateFactory.create( + ctl.forUser(identifiedUserFactory.create(accountId)), timestamp); + update.setSubject("Finalize approvals at submit"); + putApprovals(update, byUser.row(accountId)); + + CommitBuilder commit = new CommitBuilder(); + commit.setCommitter(new PersonIdent(serverIdent, timestamp)); + batch.write(update, commit); + } + } + + putApprovals(callerUpdate, + byUser.row(callerUpdate.getUser().getAccountId())); + return batch; + } + + private static void putApprovals(ChangeUpdate update, + Map> approvals) { + for (Map.Entry> e : approvals.entrySet()) { + if (e.getValue().isPresent()) { + update.putApproval(e.getKey(), e.getValue().get()); + } else { + update.removeApproval(e.getKey()); + } + } + } + private void sendMergedEmail(final Change c, final PatchSetApproval from) { workQueue.getDefaultQueue() .submit(new Runnable() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java index da38a5891b..c5ead541db 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java @@ -71,16 +71,13 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.io.InputStream; import java.io.UnsupportedEncodingException; -import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; -import java.util.TimeZone; public class MergeUtil { private static final Logger log = LoggerFactory.getLogger(MergeUtil.class); @@ -167,10 +164,6 @@ public class MergeUtil { return result; } - public PatchSetApproval getSubmitter(CodeReviewCommit c) { - return approvalsUtil.getSubmitter(db.get(), c.notes(), c.getPatchsetId()); - } - public RevCommit createCherryPickFromCommit(Repository repo, ObjectInserter inserter, RevCommit mergeTip, RevCommit originalCommit, PersonIdent cherryPickCommitterIdent, String commitMsg, RevWalk rw) @@ -347,52 +340,6 @@ public class MergeUtil { return false; } - public PersonIdent computeMergeCommitAuthor(final PersonIdent myIdent, - final RevWalk rw, final List codeReviewCommits) { - PatchSetApproval submitter = null; - for (final CodeReviewCommit c : codeReviewCommits) { - PatchSetApproval s = getSubmitter(c); - if (submitter == null - || (s != null && s.getGranted().compareTo(submitter.getGranted()) > 0)) { - submitter = s; - } - } - - // Try to use the submitter's identity for the merge commit author. - // If all of the commits being merged are created by the submitter, - // prefer the identity line they used in the commits rather than the - // preferred identity stored in the user account. This way the Git - // commit records are more consistent internally. - // - PersonIdent authorIdent; - if (submitter != null) { - IdentifiedUser who = - identifiedUserFactory.create(submitter.getAccountId()); - Set emails = new HashSet<>(); - for (RevCommit c : codeReviewCommits) { - try { - rw.parseBody(c); - } catch (IOException e) { - log.warn("Cannot parse commit " + c.name(), e); - continue; - } - emails.add(c.getAuthorIdent().getEmailAddress()); - } - - final Timestamp dt = submitter.getGranted(); - final TimeZone tz = myIdent.getTimeZone(); - if (emails.size() == 1 && who.hasEmailAddress(emails.iterator().next())) { - authorIdent = - new PersonIdent(codeReviewCommits.get(0).getAuthorIdent(), dt, tz); - } else { - authorIdent = who.newCommitterIdent(dt, tz); - } - } else { - authorIdent = myIdent; - } - return authorIdent; - } - public boolean canMerge(final MergeSorter mergeSorter, final Repository repo, final CodeReviewCommit mergeTip, final CodeReviewCommit toMerge) @@ -497,16 +444,15 @@ public class MergeUtil { }; } - public CodeReviewCommit mergeOneCommit(final PersonIdent myIdent, - final Repository repo, final RevWalk rw, final ObjectInserter inserter, - final RevFlag canMergeFlag, final Branch.NameKey destBranch, - final CodeReviewCommit mergeTip, final CodeReviewCommit n) - throws MergeException { + public CodeReviewCommit mergeOneCommit(PersonIdent author, + PersonIdent committer, Repository repo, RevWalk rw, + ObjectInserter inserter, RevFlag canMergeFlag, Branch.NameKey destBranch, + CodeReviewCommit mergeTip, CodeReviewCommit n) throws MergeException { final ThreeWayMerger m = newThreeWayMerger(repo, inserter); try { if (m.merge(new AnyObjectId[] {mergeTip, n})) { - return writeMergeCommit(myIdent, rw, inserter, canMergeFlag, destBranch, - mergeTip, m.getResultTreeId(), n); + return writeMergeCommit(author, committer, rw, inserter, canMergeFlag, + destBranch, mergeTip, m.getResultTreeId(), n); } else { failed(rw, canMergeFlag, mergeTip, n, CommitMergeStatus.PATH_CONFLICT); } @@ -549,12 +495,12 @@ public class MergeUtil { return failed; } - public CodeReviewCommit writeMergeCommit(final PersonIdent myIdent, - final RevWalk rw, final ObjectInserter inserter, - final RevFlag canMergeFlag, final Branch.NameKey destBranch, - final CodeReviewCommit mergeTip, final ObjectId treeId, - final CodeReviewCommit n) throws IOException, - MissingObjectException, IncorrectObjectTypeException { + public CodeReviewCommit writeMergeCommit(PersonIdent author, + PersonIdent committer, RevWalk rw, ObjectInserter inserter, + RevFlag canMergeFlag, Branch.NameKey destBranch, + CodeReviewCommit mergeTip, ObjectId treeId, CodeReviewCommit n) + throws IOException, MissingObjectException, + IncorrectObjectTypeException { final List merged = new ArrayList<>(); rw.resetRetain(canMergeFlag); rw.markStart(n); @@ -582,13 +528,11 @@ public class MergeUtil { } } - PersonIdent authorIdent = computeMergeCommitAuthor(myIdent, rw, merged); - final CommitBuilder mergeCommit = new CommitBuilder(); mergeCommit.setTreeId(treeId); mergeCommit.setParentIds(mergeTip, n); - mergeCommit.setAuthor(authorIdent); - mergeCommit.setCommitter(myIdent); + mergeCommit.setAuthor(author); + mergeCommit.setCommitter(committer); mergeCommit.setMessage(msgbuf.toString()); CodeReviewCommit mergeResult = @@ -691,7 +635,7 @@ public class MergeUtil { return id; } - public PatchSetApproval markCleanMerges(final RevWalk rw, + public void markCleanMerges(final RevWalk rw, final RevFlag canMergeFlag, final CodeReviewCommit mergeTip, final Set alreadyAccepted) throws MergeException { if (mergeTip == null) { @@ -699,12 +643,10 @@ public class MergeUtil { // at the start of the merge process. We also elected to merge nothing, // probably due to missing dependencies. Nothing was cleanly merged. // - return null; + return; } try { - PatchSetApproval submitApproval = null; - rw.resetRetain(canMergeFlag); rw.sort(RevSort.TOPO); rw.sort(RevSort.REVERSE, true); @@ -717,13 +659,8 @@ public class MergeUtil { while ((c = (CodeReviewCommit) rw.next()) != null) { if (c.getPatchsetId() != null) { c.setStatusCode(CommitMergeStatus.CLEAN_MERGE); - if (submitApproval == null) { - submitApproval = getSubmitter(c); - } } } - - return submitApproval; } catch (IOException e) { throw new MergeException("Cannot mark clean merges", e); } 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 93bb18f553..e01e28a765 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 @@ -1782,16 +1782,10 @@ public class ReceiveCommits { } private void submit(ChangeControl changeCtl, PatchSet ps) - throws OrmException, IOException, ResourceConflictException { + throws OrmException, ResourceConflictException { Submit submit = submitProvider.get(); RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps); - List changes; - try { - // Force submit even if submit rule evaluation fails. - changes = submit.submit(rsrc, currentUser, true); - } catch (ResourceConflictException e) { - throw new IOException(e); - } + List changes = Lists.newArrayList(rsrc.getChange()); try { mergeOpProvider.get().merge(ChangeSet.create(changes), (IdentifiedUser) changeCtl.getCurrentUser(), false); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java index 2d229a977a..0e740f8e6f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java @@ -23,7 +23,6 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ChangeUtil; -import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CommitMergeStatus; @@ -132,14 +131,15 @@ public class CherryPick extends SubmitStrategy { if (args.rw.isMergedInto(mergeTip.getCurrentTip(), n)) { mergeTip.moveTipTo(n, n); } else { - CodeReviewCommit result = args.mergeUtil.mergeOneCommit( - args.serverIdent.get(), args.repo, args.rw, args.inserter, + PersonIdent myIdent = args.serverIdent.get(); + CodeReviewCommit result = args.mergeUtil.mergeOneCommit(myIdent, + myIdent, args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, mergeTip.getCurrentTip(), n); mergeTip.moveTipTo(result, n); } - PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges(args.rw, - args.canMergeFlag, mergeTip.getCurrentTip(), args.alreadyAccepted); - setRefLogIdent(submitApproval); + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, + mergeTip.getCurrentTip(), args.alreadyAccepted); + setRefLogIdent(); } else { // One or more dependencies were not met. The status was already marked on // the commit so we have nothing further to perform at this time. @@ -153,33 +153,19 @@ public class CherryPick extends SubmitStrategy { args.rw.parseBody(n); - PatchSetApproval submitAudit = args.mergeUtil.getSubmitter(n); - - IdentifiedUser cherryPickUser; - PersonIdent serverNow = args.serverIdent.get(); - PersonIdent cherryPickCommitterIdent; - if (submitAudit != null) { - cherryPickUser = - args.identifiedUserFactory.create(submitAudit.getAccountId()); - cherryPickCommitterIdent = cherryPickUser.newCommitterIdent( - serverNow.getWhen(), serverNow.getTimeZone()); - } else { - cherryPickUser = args.identifiedUserFactory.create(n.change().getOwner()); - cherryPickCommitterIdent = serverNow; - } - String cherryPickCmtMsg = args.mergeUtil.createCherryPickCommitMessage(n); + PersonIdent committer = args.caller.newCommitterIdent( + TimeUtil.nowTs(), args.serverIdent.get().getTimeZone()); CodeReviewCommit newCommit = (CodeReviewCommit) args.mergeUtil.createCherryPickFromCommit(args.repo, - args.inserter, mergeTip, n, cherryPickCommitterIdent, - cherryPickCmtMsg, args.rw); + args.inserter, mergeTip, n, committer, cherryPickCmtMsg, args.rw); PatchSet.Id id = ChangeUtil.nextPatchSetId(args.repo, n.change().currentPatchSetId()); PatchSet ps = new PatchSet(id); ps.setCreatedOn(TimeUtil.nowTs()); - ps.setUploader(cherryPickUser.getAccountId()); + ps.setUploader(args.caller.getAccountId()); ps.setRevision(new RevId(newCommit.getId().getName())); RefUpdate ru; @@ -220,9 +206,9 @@ public class CherryPick extends SubmitStrategy { newCommit.copyFrom(n); newCommit.setStatusCode(CommitMergeStatus.CLEAN_PICK); newCommit.setControl( - args.changeControlFactory.controlFor(n.change(), cherryPickUser)); + args.changeControlFactory.controlFor(n.change(), args.caller)); newCommits.put(newCommit.getPatchsetId().getParentKey(), newCommit); - setRefLogIdent(submitAudit); + setRefLogIdent(); return newCommit; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java index 7ff2107196..f7d8ab173c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java @@ -14,7 +14,6 @@ package com.google.gerrit.server.git.strategy; -import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CommitMergeStatus; import com.google.gerrit.server.git.MergeException; @@ -43,10 +42,9 @@ public class FastForwardOnly extends SubmitStrategy { n.setStatusCode(CommitMergeStatus.NOT_FAST_FORWARD); } - PatchSetApproval submitApproval = - args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, newMergeTipCommit, - args.alreadyAccepted); - setRefLogIdent(submitApproval); + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, + newMergeTipCommit, args.alreadyAccepted); + setRefLogIdent(); return mergeTip; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java index 3c13af9cab..d3a72e3c9e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java @@ -14,11 +14,12 @@ package com.google.gerrit.server.git.strategy; -import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.MergeException; import com.google.gerrit.server.git.MergeTip; +import org.eclipse.jgit.lib.PersonIdent; + import java.util.Collection; import java.util.List; @@ -42,17 +43,19 @@ public class MergeAlways extends SubmitStrategy { } while (!sorted.isEmpty()) { CodeReviewCommit mergedFrom = sorted.remove(0); + PersonIdent serverIdent = args.serverIdent.get(); + PersonIdent caller = args.caller.newCommitterIdent( + serverIdent.getWhen(), serverIdent.getTimeZone()); CodeReviewCommit newTip = - args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw, - args.inserter, args.canMergeFlag, args.destBranch, mergeTip.getCurrentTip(), - mergedFrom); + args.mergeUtil.mergeOneCommit(caller, serverIdent, + args.repo, args.rw, args.inserter, args.canMergeFlag, + args.destBranch, mergeTip.getCurrentTip(), mergedFrom); mergeTip.moveTipTo(newTip, mergedFrom); } - final PatchSetApproval submitApproval = - args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, mergeTip.getCurrentTip(), - args.alreadyAccepted); - setRefLogIdent(submitApproval); + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, + mergeTip.getCurrentTip(), args.alreadyAccepted); + setRefLogIdent(); return mergeTip; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java index b49cb0a8ce..688fa3e576 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java @@ -14,11 +14,12 @@ package com.google.gerrit.server.git.strategy; -import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.MergeException; import com.google.gerrit.server.git.MergeTip; +import org.eclipse.jgit.lib.PersonIdent; + import java.util.Collection; import java.util.List; @@ -48,18 +49,19 @@ public class MergeIfNecessary extends SubmitStrategy { // For every other commit do a pair-wise merge. while (!sorted.isEmpty()) { CodeReviewCommit mergedFrom = sorted.remove(0); + PersonIdent serverIdent = args.serverIdent.get(); + PersonIdent caller = args.caller.newCommitterIdent( + serverIdent.getWhen(), serverIdent.getTimeZone()); branchTip = - args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, - args.rw, args.inserter, args.canMergeFlag, args.destBranch, - branchTip, mergedFrom); + args.mergeUtil.mergeOneCommit(caller, serverIdent, + args.repo, args.rw, args.inserter, args.canMergeFlag, + args.destBranch, branchTip, mergedFrom); mergeTip.moveTipTo(branchTip, mergedFrom); } - final PatchSetApproval submitApproval = - args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, branchTip, - args.alreadyAccepted); - setRefLogIdent(submitApproval); - + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, branchTip, + args.alreadyAccepted); + setRefLogIdent(); return mergeTip; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java index dd981ad054..f9102ec4f3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java @@ -18,7 +18,6 @@ import com.google.common.collect.Lists; 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.server.IdentifiedUser; import com.google.gerrit.server.change.PatchSetInserter.ValidatePolicy; import com.google.gerrit.server.change.RebaseChange; import com.google.gerrit.server.git.CodeReviewCommit; @@ -33,6 +32,7 @@ import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.PersonIdent; import java.io.IOException; import java.util.Collection; @@ -84,15 +84,11 @@ public class RebaseIfNecessary extends SubmitStrategy { } else { try { - IdentifiedUser uploader = - args.identifiedUserFactory.create(args.mergeUtil - .getSubmitter(n).getAccountId()); PatchSet newPatchSet = rebaseChange.rebase(args.repo, args.rw, args.inserter, - n.change(), n.getPatchsetId(), uploader, + n.change(), n.getPatchsetId(), args.caller, mergeTip.getCurrentTip(), args.mergeUtil, args.serverIdent.get(), false, ValidatePolicy.NONE); - List approvals = Lists.newArrayList(); for (PatchSetApproval a : args.approvalsUtil.byPatchSet(args.db, n.getControl(), n.getPatchsetId())) { @@ -109,13 +105,13 @@ public class RebaseIfNecessary extends SubmitStrategy { newPatchSet.getId())); mergeTip.getCurrentTip().copyFrom(n); mergeTip.getCurrentTip().setControl( - args.changeControlFactory.controlFor(n.change(), uploader)); + args.changeControlFactory.controlFor(n.change(), args.caller)); mergeTip.getCurrentTip().setPatchsetId(newPatchSet.getId()); mergeTip.getCurrentTip().setStatusCode( CommitMergeStatus.CLEAN_REBASE); newCommits.put(newPatchSet.getId().getParentKey(), mergeTip.getCurrentTip()); - setRefLogIdent(args.mergeUtil.getSubmitter(n)); + setRefLogIdent(); } catch (MergeConflictException e) { n.setStatusCode(CommitMergeStatus.REBASE_MERGE_CONFLICT); } catch (NoSuchChangeException | OrmException | IOException @@ -135,15 +131,15 @@ public class RebaseIfNecessary extends SubmitStrategy { if (args.rw.isMergedInto(mergeTip.getCurrentTip(), n)) { mergeTip.moveTipTo(n, n); } else { + PersonIdent myIdent = args.serverIdent.get(); mergeTip.moveTipTo( - args.mergeUtil.mergeOneCommit(args.serverIdent.get(), + args.mergeUtil.mergeOneCommit(myIdent, myIdent, args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, mergeTip.getCurrentTip(), n), n); } - PatchSetApproval submitApproval = - args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, - mergeTip.getCurrentTip(), args.alreadyAccepted); - setRefLogIdent(submitApproval); + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, + mergeTip.getCurrentTip(), args.alreadyAccepted); + setRefLogIdent(); } catch (IOException e) { throw new MergeException("Cannot merge " + n.name(), e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java index b25b17e425..f1eb8374dd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java @@ -14,10 +14,11 @@ package com.google.gerrit.server.git.strategy; +import static com.google.common.base.Preconditions.checkState; + import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.IdentifiedUser; @@ -66,6 +67,7 @@ public abstract class SubmitStrategy { protected final MergeUtil mergeUtil; protected final ChangeIndexer indexer; protected final MergeSorter mergeSorter; + protected final IdentifiedUser caller; Arguments(IdentifiedUser.GenericFactory identifiedUserFactory, Provider serverIdent, ReviewDb db, @@ -73,7 +75,7 @@ public abstract class SubmitStrategy { RevWalk rw, ObjectInserter inserter, RevFlag canMergeFlag, Set alreadyAccepted, Branch.NameKey destBranch, ApprovalsUtil approvalsUtil, MergeUtil mergeUtil, - ChangeIndexer indexer) { + ChangeIndexer indexer, IdentifiedUser caller) { this.identifiedUserFactory = identifiedUserFactory; this.serverIdent = serverIdent; this.db = db; @@ -89,6 +91,7 @@ public abstract class SubmitStrategy { this.mergeUtil = mergeUtil; this.indexer = indexer; this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag); + this.caller = caller; } } @@ -115,6 +118,7 @@ public abstract class SubmitStrategy { public final MergeTip run(final CodeReviewCommit currentTip, final Collection toMerge) throws MergeException { refLogIdent = null; + checkState(args.caller != null); return _run(currentTip, toMerge); } @@ -182,13 +186,11 @@ public abstract class SubmitStrategy { /** * Set the ref log identity if it wasn't set yet. - * - * @param submitApproval the approval that submitted the patch set */ - protected final void setRefLogIdent(PatchSetApproval submitApproval) { - if (refLogIdent == null && submitApproval != null) { + protected final void setRefLogIdent() { + if (refLogIdent == null) { refLogIdent = args.identifiedUserFactory.create( - submitApproval.getAccountId()) .newRefLogIdent(); + args.caller.getAccountId()).newRefLogIdent(); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java index ffe351b53d..b87499a87d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java @@ -88,14 +88,14 @@ public class SubmitStrategyFactory { public SubmitStrategy create(final SubmitType submitType, final ReviewDb db, final Repository repo, final RevWalk rw, final ObjectInserter inserter, final RevFlag canMergeFlag, final Set alreadyAccepted, - final Branch.NameKey destBranch) + final Branch.NameKey destBranch, final IdentifiedUser caller) throws MergeException, NoSuchProjectException { ProjectState project = getProject(destBranch); final SubmitStrategy.Arguments args = new SubmitStrategy.Arguments(identifiedUserFactory, myIdent, db, changeControlFactory, repo, rw, inserter, canMergeFlag, alreadyAccepted, destBranch,approvalsUtil, - mergeUtilFactory.create(project), indexer); + mergeUtilFactory.create(project), indexer, caller); switch (submitType) { case CHERRY_PICK: return new CherryPick(args, patchSetInfoFactory, gitRefUpdated); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 43c232be6d..236b03978d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -164,7 +164,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } public void setStatus(Change.Status status) { - checkArgument(status != Change.Status.SUBMITTED, + checkArgument(status != Change.Status.MERGED, "use submit(Iterable)"); this.status = status; } @@ -178,7 +178,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { } public void submit(Iterable submitRecords) { - status = Change.Status.SUBMITTED; + this.status = Change.Status.MERGED; this.submitRecords = ImmutableList.copyOf(submitRecords); checkArgument(!this.submitRecords.isEmpty(), "no submit records specified at submit time"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java index 207074620f..e63b05cf05 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ConflictsPredicate.java @@ -117,7 +117,7 @@ class ConflictsPredicate extends OrPredicate { args.submitStrategyFactory.create(submitType, db.get(), repo, rw, null, canMergeFlag, getAlreadyAccepted(repo, rw, commit), - otherChange.getDest()); + otherChange.getDest(), null); CodeReviewCommit otherCommit = (CodeReviewCommit) rw.parseCommit(other); otherCommit.add(canMergeFlag); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java index a4d4467c89..61ef5ff7b2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java @@ -108,13 +108,6 @@ public class InternalChangeQuery { return query(project(project)); } - public List submitted(Branch.NameKey branch) throws OrmException { - return query(and( - ref(branch), - project(branch.getParentKey()), - status(Change.Status.SUBMITTED))); - } - public List allSubmitted() throws OrmException { return query(status(Change.Status.SUBMITTED)); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java index 49b61cc6bd..ead0ff07af 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java @@ -121,7 +121,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { assertBodyEquals("Submit patch set 1\n" + "\n" + "Patch-set: 1\n" - + "Status: submitted\n" + + "Status: merged\n" + "Submitted-with: NOT_READY\n" + "Submitted-with: OK: Verified: Change Owner <1@gerrit>\n" + "Submitted-with: NEED: Code-Review\n" @@ -180,7 +180,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { assertBodyEquals("Submit patch set 1\n" + "\n" + "Patch-set: 1\n" - + "Status: submitted\n" + + "Status: merged\n" + "Submitted-with: RULE_ERROR Problem with patch set: 1\n", update.getRevision()); } From 0d3cab063408577a8afdae49e3d3e35fdcf1a214 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 10 Jul 2015 13:32:57 -0700 Subject: [PATCH 3/3] Remove the state SUBMITTED altogether There is a new schema to move all submitted changes to new. Change-Id: I9f3b015a59fc2446a98a03866330b0d4e22bbf3d --- Documentation/json.txt | 3 -- Documentation/rest-api-changes.txt | 9 ++--- .../rest/change/CreateChangeIT.java | 2 +- .../com/google/gerrit/common/PageLinks.java | 1 - .../extensions/client/ChangeStatus.java | 29 +------------- .../client/changes/ChangeConstants.java | 1 - .../client/changes/ChangeConstants.properties | 1 - .../google/gerrit/client/changes/Util.java | 2 - .../google/gerrit/reviewdb/client/Change.java | 31 +-------------- .../google/gerrit/server/change/Submit.java | 12 +----- .../com/google/gerrit/server/git/MergeOp.java | 10 ++--- .../gerrit/server/git/ReceiveCommits.java | 3 -- .../gerrit/server/notedb/ChangeUpdate.java | 2 +- .../query/change/InternalChangeQuery.java | 4 -- .../gerrit/server/schema/SchemaVersion.java | 2 +- .../gerrit/server/schema/Schema_109.java | 39 +++++++++++++++++++ .../gerrit/server/index/IndexRewriteTest.java | 11 +++--- .../gerrit/server/notedb/ChangeNotesTest.java | 6 +-- .../notedb/CommitMessageOutputTest.java | 4 +- 19 files changed, 64 insertions(+), 108 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_109.java diff --git a/Documentation/json.txt b/Documentation/json.txt index 8ccd03b88a..32fa47216c 100644 --- a/Documentation/json.txt +++ b/Documentation/json.txt @@ -43,9 +43,6 @@ status:: Current state of this change. DRAFT;; Change is a draft change that only consists of draft patchsets. - SUBMITTED;; Change has been submitted and is in the merge queue. - It may be waiting for one or more dependencies. - MERGED;; Change has been merged to its branch. ABANDONED;; Change was abandoned by its owner or administrator. diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 03849937f9..4224c76ac5 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -3756,8 +3756,7 @@ The `refs/heads/` prefix is omitted. |`subject` || The subject of the change (header line of the commit message). |`status` || -The status of the change (`NEW`, `SUBMITTED`, `MERGED`, `ABANDONED`, -`DRAFT`). +The status of the change (`NEW`, `MERGED`, `ABANDONED`, `DRAFT`). |`created` || The link:rest-api.html#timestamp[timestamp] of when the change was created. @@ -4322,7 +4321,7 @@ link:#commit-info[CommitInfo] entity. |`_revision_number` |optional|The revision number. |`_current_revision_number`|optional|The current revision number. |`status` |optional|The status of the change. The status of -the change is one of (`NEW`, `SUBMITTED`, `MERGED`, `ABANDONED`, `DRAFT`). +the change is one of (`NEW`, `MERGED`, `ABANDONED`, `DRAFT`). |=========================== [[related-changes-info]] @@ -4530,8 +4529,8 @@ after submitting. |========================== |Field Name ||Description |`status` || -The status of the change after submitting, can be `MERGED` or -`SUBMITTED`.+ +The status of the change after submitting is `MERGED`. ++ As `wait_for_merge` in the link:#submit-input[SubmitInput] is deprecated and the request always waits for the merge to be completed, you can expect `MERGED` to be returned here. diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index ee17797579..d0bcfd96b4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -56,7 +56,7 @@ public class CreateChangeIT extends AbstractDaemonTest { @Test public void createEmptyChange_InvalidStatus() throws Exception { - ChangeInfo ci = newChangeInfo(ChangeStatus.SUBMITTED); + ChangeInfo ci = newChangeInfo(ChangeStatus.MERGED); assertCreateFails(ci, BadRequestException.class, "unsupported change status"); } diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java b/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java index b8046075dc..80ca561d01 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/PageLinks.java @@ -135,7 +135,6 @@ public class PageLinks { case MERGED: return "status:merged"; case NEW: - case SUBMITTED: default: return "status:open"; } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ChangeStatus.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ChangeStatus.java index f3fc887c74..56ebf9da12 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ChangeStatus.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/ChangeStatus.java @@ -28,39 +28,12 @@ public enum ChangeStatus { *

* Changes in the NEW state can be moved to: *

    - *
  • {@link #SUBMITTED} - when the Submit Patch Set action is used; + *
  • {@link #MERGED} - when the Submit Patch Set action is used; *
  • {@link #ABANDONED} - when the Abandon action is used. *
*/ NEW, - /** - * Change is open, but has been submitted to the merge queue. - * - *

- * A change enters the SUBMITTED state when an authorized user presses the - * "submit" action through the web UI, requesting that Gerrit merge the - * change's current patch set into the destination branch. - * - *

- * Typically a change resides in the SUBMITTED for only a brief sub-second - * period while the merge queue fires and the destination branch is updated. - * However, if a dependency commit (directly or transitively) is not yet - * merged into the branch, the change will hang in the SUBMITTED state - * indefinitely. - * - *

- * Changes in the SUBMITTED state can be moved to: - *

    - *
  • {@link #NEW} - when a replacement patch set is supplied, OR when a - * merge conflict is detected; - *
  • {@link #MERGED} - when the change has been successfully merged into - * the destination branch; - *
  • {@link #ABANDONED} - when the Abandon action is used. - *
- */ - SUBMITTED, - /** * Change is a draft change that only consists of draft patchsets. * diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java index 2727ae2695..759e7228c5 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java @@ -18,7 +18,6 @@ import com.google.gwt.i18n.client.Constants; public interface ChangeConstants extends Constants { String statusLongNew(); - String statusLongSubmitted(); String statusLongMerged(); String statusLongAbandoned(); String statusLongDraft(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties index 8db9319b97..ca29773fa2 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties @@ -1,5 +1,4 @@ statusLongNew = Review in Progress -statusLongSubmitted = Submitted, Merge Pending statusLongMerged = Merged statusLongAbandoned = Abandoned statusLongDraft = Draft diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/Util.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/Util.java index d34492c77c..e6d3dbe47e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/Util.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/Util.java @@ -34,8 +34,6 @@ public class Util { return C.statusLongDraft(); case NEW: return C.statusLongNew(); - case SUBMITTED: - return C.statusLongSubmitted(); case MERGED: return C.statusLongMerged(); case ABANDONED: diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java index 97fde9ded9..c6643af36d 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Change.java @@ -251,8 +251,6 @@ public final class Change { private static final char MIN_OPEN = 'a'; /** Database constant for {@link Status#NEW}. */ public static final char STATUS_NEW = 'n'; - /** Database constant for {@link Status#SUBMITTED}. */ - public static final char STATUS_SUBMITTED = 's'; /** Database constant for {@link Status#DRAFT}. */ public static final char STATUS_DRAFT = 'd'; /** Maximum database status constant for an open change. */ @@ -285,39 +283,12 @@ public final class Change { *

* Changes in the NEW state can be moved to: *

    - *
  • {@link #SUBMITTED} - when the Submit Patch Set action is used; + *
  • {@link #MERGED} - when the Submit Patch Set action is used; *
  • {@link #ABANDONED} - when the Abandon action is used. *
*/ NEW(STATUS_NEW, ChangeStatus.NEW), - /** - * Change is open, but has been submitted to the merge queue. - * - *

- * A change enters the SUBMITTED state when an authorized user presses the - * "submit" action through the web UI, requesting that Gerrit merge the - * change's current patch set into the destination branch. - * - *

- * Typically a change resides in the SUBMITTED for only a brief sub-second - * period while the merge queue fires and the destination branch is updated. - * However, if a dependency commit (a {@link PatchSetAncestor}, directly or - * transitively) is not yet merged into the branch, the change will hang in - * the SUBMITTED state indefinitely. - * - *

- * Changes in the SUBMITTED state can be moved to: - *

    - *
  • {@link #NEW} - when a replacement patch set is supplied, OR when a - * merge conflict is detected; - *
  • {@link #MERGED} - when the change has been successfully merged into - * the destination branch; - *
  • {@link #ABANDONED} - when the Abandon action is used. - *
- */ - SUBMITTED(STATUS_SUBMITTED, ChangeStatus.SUBMITTED), - /** * Change is a draft change that only consists of draft patchsets. * 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 15e181baff..b9e9b3eac0 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 @@ -88,16 +88,10 @@ public class Submit implements RestModifyView, private static final String CLICK_FAILURE_TOOLTIP = "Clicking the button would fail."; - public enum Status { - SUBMITTED, MERGED - } - public static class Output { - public Status status; transient Change change; - private Output(Status s, Change c) { - status = s; + private Output(Change c) { change = c; } } @@ -198,10 +192,8 @@ public class Submit implements RestModifyView, throw new ResourceConflictException("change is deleted"); } switch (change.getStatus()) { - case SUBMITTED: - return new Output(Status.SUBMITTED, change); case MERGED: - return new Output(Status.MERGED, change); + return new Output(change); case NEW: ChangeMessage msg = getConflictMessage(rsrc); if (msg != null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index ff3d992536..cc2ded6e49 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -388,8 +388,7 @@ public class MergeOp { throws ResourceConflictException, OrmException { for (Change.Id id : cs.ids()) { ChangeData cd = changeDataFactory.create(db, id); - if (cd.change().getStatus() != Change.Status.NEW - && cd.change().getStatus() != Change.Status.SUBMITTED){ + if (cd.change().getStatus() != Change.Status.NEW){ throw new OrmException("Change " + cd.change().getChangeId() + " is in state " + cd.change().getStatus()); } else { @@ -654,9 +653,8 @@ public class MergeOp { throw new MergeException("Failed to validate changes", e); } Change.Id changeId = cd.getId(); - if (chg.getStatus() != Change.Status.SUBMITTED - && chg.getStatus() != Change.Status.NEW) { - logDebug("Change {} is not new or submitted: {}", changeId, chg.getStatus()); + if (chg.getStatus() != Change.Status.NEW) { + logDebug("Change {} is not new: {}", changeId, chg.getStatus()); continue; } if (chg.currentPatchSetId() == null) { @@ -1093,7 +1091,7 @@ public class MergeOp { ChangeUpdate update = updateFactory.create(control, timestamp); List record = records.get(cd.change().getId()); if (record != null) { - update.submit(record); + update.merge(record); } db.changes().beginTransaction(cd.change().getId()); try { 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 e01e28a765..c21e8c097f 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 @@ -1796,9 +1796,6 @@ public class ReceiveCommits { for (Change c : changes) { 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; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 236b03978d..cb35619591 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -177,7 +177,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { approvals.put(label, Optional. absent()); } - public void submit(Iterable submitRecords) { + public void merge(Iterable submitRecords) { this.status = Change.Status.MERGED; this.submitRecords = ImmutableList.copyOf(submitRecords); checkArgument(!this.submitRecords.isEmpty(), diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java index 61ef5ff7b2..77a24cc5b5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java @@ -108,10 +108,6 @@ public class InternalChangeQuery { return query(project(project)); } - public List allSubmitted() throws OrmException { - return query(status(Change.Status.SUBMITTED)); - } - public List byBranchOpen(Branch.NameKey branch) throws OrmException { return query(and( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java index c1be4f86c5..038da50194 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java @@ -32,7 +32,7 @@ import java.util.List; /** A version of the database schema. */ public abstract class SchemaVersion { /** The current schema version. */ - public static final Class C = Schema_108.class; + public static final Class C = Schema_109.class; public static int getBinaryVersion() { return guessVersion(C); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_109.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_109.java new file mode 100644 index 0000000000..fa8af3e8de --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_109.java @@ -0,0 +1,39 @@ +// Copyright (C) 2015 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.server.schema; + +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.StatementExecutor; +import com.google.inject.Inject; +import com.google.inject.Provider; + +public class Schema_109 extends SchemaVersion { + + @Inject + Schema_109(Provider prior) { + super(prior); + } + + @Override + protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException { + String cmd = "UPDATE changes SET status = 'n' WHERE status = 's';"; + ui.message("Running " + cmd); + try (StatementExecutor e = newExecutor(db)) { + e.execute(cmd); + } + ui.message("done"); + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java index 8d7866d54e..ac6d805f53 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriteTest.java @@ -19,7 +19,6 @@ import static com.google.gerrit.reviewdb.client.Change.Status.ABANDONED; import static com.google.gerrit.reviewdb.client.Change.Status.DRAFT; import static com.google.gerrit.reviewdb.client.Change.Status.MERGED; import static com.google.gerrit.reviewdb.client.Change.Status.NEW; -import static com.google.gerrit.reviewdb.client.Change.Status.SUBMITTED; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; @@ -183,17 +182,17 @@ public class IndexRewriteTest { public void testGetPossibleStatus() throws Exception { assertEquals(EnumSet.allOf(Change.Status.class), status("file:a")); assertEquals(EnumSet.of(NEW), status("is:new")); - assertEquals(EnumSet.of(SUBMITTED, DRAFT, MERGED, ABANDONED), + assertEquals(EnumSet.of(DRAFT, MERGED, ABANDONED), status("-is:new")); assertEquals(EnumSet.of(NEW, MERGED), status("is:new OR is:merged")); EnumSet none = EnumSet.noneOf(Change.Status.class); assertEquals(none, status("is:new is:merged")); - assertEquals(none, status("(is:new is:draft) (is:merged is:submitted)")); - assertEquals(none, status("(is:new is:draft) (is:merged is:submitted)")); + assertEquals(none, status("(is:new is:draft) (is:merged)")); + assertEquals(none, status("(is:new is:draft) (is:merged)")); - assertEquals(EnumSet.of(MERGED, SUBMITTED), - status("(is:new is:draft) OR (is:merged OR is:submitted)")); + assertEquals(EnumSet.of(MERGED), + status("(is:new is:draft) OR (is:merged)")); } @Test diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index 01f964b80c..f19987fc26 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -287,7 +287,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { ChangeUpdate update = newUpdate(c, changeOwner); update.setSubject("Submit patch set 1"); - update.submit(ImmutableList.of( + update.merge(ImmutableList.of( submitRecord("NOT_READY", null, submitLabel("Verified", "OK", changeOwner.getAccountId()), submitLabel("Code-Review", "NEED", null)), @@ -314,7 +314,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { Change c = newChange(); ChangeUpdate update = newUpdate(c, changeOwner); update.setSubject("Submit patch set 1"); - update.submit(ImmutableList.of( + update.merge(ImmutableList.of( submitRecord("OK", null, submitLabel("Code-Review", "OK", otherUser.getAccountId())))); update.commit(); @@ -322,7 +322,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { incrementPatchSet(c); update = newUpdate(c, changeOwner); update.setSubject("Submit patch set 2"); - update.submit(ImmutableList.of( + update.merge(ImmutableList.of( submitRecord("OK", null, submitLabel("Code-Review", "OK", changeOwner.getAccountId())))); update.commit(); diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java index ead0ff07af..c20690233f 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java @@ -108,7 +108,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { ChangeUpdate update = newUpdate(c, changeOwner); update.setSubject("Submit patch set 1"); - update.submit(ImmutableList.of( + update.merge(ImmutableList.of( submitRecord("NOT_READY", null, submitLabel("Verified", "OK", changeOwner.getAccountId()), submitLabel("Code-Review", "NEED", null)), @@ -173,7 +173,7 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest { ChangeUpdate update = newUpdate(c, changeOwner); update.setSubject("Submit patch set 1"); - update.submit(ImmutableList.of( + update.merge(ImmutableList.of( submitRecord("RULE_ERROR", "Problem with patch set:\n1"))); update.commit();