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/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/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 1f0c601bdd..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; @@ -53,10 +51,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"); @@ -124,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()); } @@ -257,7 +251,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..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; @@ -61,6 +60,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; @@ -71,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; @@ -145,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 { @@ -202,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(); @@ -265,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"); @@ -273,6 +261,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( @@ -283,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/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-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..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 @@ -65,6 +65,8 @@ public class SubmitByCherryPickIT extends AbstractSubmit { assertCurrentRevision(change2.getChangeId(), 2, newHead); assertSubmitter(change2.getChangeId(), 1); assertSubmitter(change2.getChangeId(), 2); + assertPersonEquals(admin.getIdent(), newHead.getAuthorIdent()); + assertPersonEquals(admin.getIdent(), newHead.getCommitterIdent()); } @Test @@ -106,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 @@ -146,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 @@ -162,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 @@ -231,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 @@ -238,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/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..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 @@ -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 @@ -56,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 a9beefec50..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 @@ -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 @@ -44,23 +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.getShortMessage()).isEqualTo( + 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()); - - tip = tip.getParent(0); - assertThat(tip.getParent(1).getShortMessage()).isEqualTo( + assertThat(tip.getParent(0).getShortMessage()).isEqualTo( change3.getCommit().getShortMessage()); - tip = tip.getParent(0); - assertThat(tip.getShortMessage()).isEqualTo( - change2.getCommit().getShortMessage()); + assertPersonEquals(admin.getIdent(), tip.getAuthorIdent()); + assertPersonEquals(serverIdent.get(), tip.getCommitterIdent()); - assertThat(tip.getParent(0).getId()).isEqualTo(initialHead.getId()); + assertNew(change2.getChangeId()); } @Test @@ -184,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()); @@ -191,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 bb26a3043a..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 @@ -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 @@ -107,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-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/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..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 @@ -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; @@ -106,31 +88,19 @@ 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; } } - 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 +111,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 +170,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); @@ -225,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) { @@ -374,203 +339,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..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 @@ -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() { @@ -379,40 +388,11 @@ 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 { - 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 +408,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 +427,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 +439,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 +473,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 +502,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 +521,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 +625,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 +653,8 @@ 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.NEW) { + logDebug("Change {} is not new: {}", changeId, chg.getStatus()); continue; } if (chg.currentPatchSetId() == null) { @@ -762,7 +738,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 +857,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 +868,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 +881,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 +1079,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.merge(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..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 @@ -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); @@ -1802,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/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..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 @@ -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; } @@ -177,8 +177,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { approvals.put(label, Optional. absent()); } - public void submit(Iterable submitRecords) { - status = Change.Status.SUBMITTED; + public void merge(Iterable submitRecords) { + 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..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,17 +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)); - } - 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 49b61cc6bd..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)), @@ -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" @@ -173,14 +173,14 @@ 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(); 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()); }