From 54bc983c19f3f52275603cd426c407ed0bb01a25 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 5 Jul 2017 14:44:36 -0700 Subject: [PATCH] Let cherrypick API optionally preserve reviewers/ccs I don't plan to change the default, or the action performed by the cherry-pick button in the UI, but adding this option to the API will allow API customers (like Chromium's "Reland" button) to carry forward some metadata from the original change to the newly-created one. Change-Id: I1d149d2758b825135460564dbf8038f4d7d52f0f --- Documentation/rest-api-changes.txt | 2 + .../acceptance/api/revision/RevisionIT.java | 49 +++++++++++++++- .../api/changes/CherryPickInput.java | 2 + .../server/change/CherryPickChange.java | 56 ++++++++++++------- .../server/change/CherryPickCommit.java | 2 - 5 files changed, 87 insertions(+), 24 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index d64054a7f4..762a916074 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -5808,6 +5808,8 @@ If not set, the default is `NONE`. |`notify_details` |optional| Additional information about whom to notify about the update as a map of recipient type to link:#notify-info[NotifyInfo] entity. +|`keep_reviewers` |optional, defaults to false| +If true, carries reviewers and ccs over from original change to newly created one. |=========================== [[comment-info]] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java index c442a30bba..7c9b832476 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -22,11 +22,11 @@ import static com.google.gerrit.acceptance.PushOneCommit.PATCH; import static com.google.gerrit.acceptance.PushOneCommit.PATCH_FILE_ONLY; import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS; -import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER; import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG; import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.stream.Collectors.toList; import static org.eclipse.jgit.lib.Constants.HEAD; import static org.junit.Assert.fail; @@ -54,6 +54,7 @@ import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.api.projects.BranchInput; import com.google.gerrit.extensions.client.ChangeStatus; import com.google.gerrit.extensions.client.ListChangesOption; +import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ApprovalInfo; @@ -673,6 +674,50 @@ public class RevisionIT extends AbstractDaemonTest { assertNotifyTo(userToNotify); } + @Test + public void cherryPickKeepReviewers() throws Exception { + createBranch(new Branch.NameKey(project, "stable")); + + // Change is created by 'admin'. + PushOneCommit.Result r = createChange(); + // Change is approved by 'admin2'. Change is CC'd to 'user'. + setApiUser(accountCreator.admin2()); + ReviewInput in = ReviewInput.approve(); + in.reviewer(user.email, ReviewerState.CC, true); + gApi.changes().id(r.getChangeId()).current().review(in); + + // Change is cherrypicked by 'user2'. + setApiUser(accountCreator.user2()); + CherryPickInput cin = new CherryPickInput(); + cin.message = "this need to go to stable"; + cin.destination = "stable"; + cin.keepReviewers = true; + Map> result = + gApi.changes() + .id(r.getChangeId()) + .current() + .cherryPick(cin) + .get() + .reviewers; + + // 'admin' should be a reviewer as the old owner. + // 'admin2' should be a reviewer as the old reviewer. + // 'user' should be on CC. + assertThat(result).containsKey(ReviewerState.REVIEWER); + List reviewers = + result.get(ReviewerState.REVIEWER).stream().map(a -> a._accountId).collect(toList()); + if (notesMigration.readChanges()) { + assertThat(result).containsKey(ReviewerState.CC); + List ccs = + result.get(ReviewerState.CC).stream().map(a -> a._accountId).collect(toList()); + assertThat(ccs).containsExactly(user.id.get()); + assertThat(reviewers).containsExactly(admin.id.get(), accountCreator.admin2().id.get()); + } else { + assertThat(reviewers) + .containsExactly(user.id.get(), admin.id.get(), accountCreator.admin2().id.get()); + } + } + @Test public void cherryPickToMergedChangeRevision() throws Exception { createBranch(new Branch.NameKey(project, "foo")); @@ -1246,7 +1291,7 @@ public class RevisionIT extends AbstractDaemonTest { ChangeMessageInfo message = Iterables.getLast(c.messages); assertThat(message.author._accountId).isEqualTo(admin.getId().get()); assertThat(message.message).isEqualTo("Removed Code-Review+1 by User \n"); - assertThat(getReviewers(c.reviewers.get(REVIEWER))) + assertThat(getReviewers(c.reviewers.get(ReviewerState.REVIEWER))) .containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.getId())); } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java index 75881d166f..694e06bca1 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java @@ -26,4 +26,6 @@ public class CherryPickInput { public NotifyHandling notify = NotifyHandling.NONE; public Map notifyDetails; + + public boolean keepReviewers; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java index fbb692c667..deb379da6e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java @@ -24,6 +24,7 @@ import com.google.gerrit.extensions.restapi.MergeConflictException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; +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.Change.Status; @@ -32,11 +33,13 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; +import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; @@ -44,6 +47,8 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.IntegrationException; import com.google.gerrit.server.git.MergeIdenticalTreeException; import com.google.gerrit.server.git.MergeUtil; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.ProjectState; @@ -60,7 +65,9 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import java.sql.Timestamp; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.TimeZone; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.InvalidObjectIdException; @@ -86,6 +93,8 @@ public class CherryPickChange { private final ChangeInserter.Factory changeInserterFactory; private final PatchSetInserter.Factory patchSetInserterFactory; private final MergeUtil.Factory mergeUtilFactory; + private final ChangeNotes.Factory changeNotesFactory; + private final ApprovalsUtil approvalsUtil; private final ChangeMessagesUtil changeMessagesUtil; private final PatchSetUtil psUtil; private final NotifyUtil notifyUtil; @@ -101,6 +110,8 @@ public class CherryPickChange { ChangeInserter.Factory changeInserterFactory, PatchSetInserter.Factory patchSetInserterFactory, MergeUtil.Factory mergeUtilFactory, + ChangeNotes.Factory changeNotesFactory, + ApprovalsUtil approvalsUtil, ChangeMessagesUtil changeMessagesUtil, PatchSetUtil psUtil, NotifyUtil notifyUtil) { @@ -113,6 +124,8 @@ public class CherryPickChange { this.changeInserterFactory = changeInserterFactory; this.patchSetInserterFactory = patchSetInserterFactory; this.mergeUtilFactory = mergeUtilFactory; + this.changeNotesFactory = changeNotesFactory; + this.approvalsUtil = approvalsUtil; this.changeMessagesUtil = changeMessagesUtil; this.psUtil = psUtil; this.notifyUtil = notifyUtil; @@ -128,10 +141,8 @@ public class CherryPickChange { UpdateException, RestApiException, ConfigInvalidException { return cherryPick( batchUpdateFactory, - change.getId(), + change, patch.getId(), - change.getDest(), - change.getTopic(), change.getProject(), ObjectId.fromString(patch.getRevision().get()), input, @@ -140,10 +151,8 @@ public class CherryPickChange { public Change.Id cherryPick( BatchUpdate.Factory batchUpdateFactory, - @Nullable Change.Id sourceChangeId, + @Nullable Change sourceChange, @Nullable PatchSet.Id sourcePatchId, - @Nullable Branch.NameKey sourceBranch, - @Nullable String sourceChangeTopic, Project.NameKey project, ObjectId sourceCommit, CherryPickInput input, @@ -240,22 +249,16 @@ public class CherryPickChange { // Change key not found on destination branch. We can create a new // change. String newTopic = null; - if (!Strings.isNullOrEmpty(sourceChangeTopic)) { - newTopic = sourceChangeTopic + "-" + newDest.getShortName(); + if (sourceChange != null && !Strings.isNullOrEmpty(sourceChange.getTopic())) { + newTopic = sourceChange.getTopic() + "-" + newDest.getShortName(); } result = createNewChange( - bu, - cherryPickCommit, - destRefControl.getRefName(), - newTopic, - sourceBranch, - sourceCommit, - input); + bu, cherryPickCommit, destRefName, newTopic, sourceChange, sourceCommit, input); - if (sourceChangeId != null && sourcePatchId != null) { + if (sourceChange != null && sourcePatchId != null) { bu.addOp( - sourceChangeId, + sourceChange.getId(), new AddMessageToSourceChangeOp( changeMessagesUtil, sourcePatchId, @@ -341,16 +344,29 @@ public class CherryPickChange { CodeReviewCommit cherryPickCommit, String refName, String topic, - Branch.NameKey sourceBranch, + @Nullable Change sourceChange, ObjectId sourceCommit, CherryPickInput input) throws OrmException, IOException, BadRequestException, ConfigInvalidException { Change.Id changeId = new Change.Id(seq.nextChangeId()); - ChangeInserter ins = - changeInserterFactory.create(changeId, cherryPickCommit, refName).setTopic(topic); + ChangeInserter ins = changeInserterFactory.create(changeId, cherryPickCommit, refName); + Branch.NameKey sourceBranch = sourceChange == null ? null : sourceChange.getDest(); ins.setMessage(messageForDestinationChange(ins.getPatchSetId(), sourceBranch, sourceCommit)) + .setTopic(topic) .setNotify(input.notify) .setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails)); + if (input.keepReviewers && sourceChange != null) { + ReviewerSet reviewerSet = + approvalsUtil.getReviewers( + dbProvider.get(), changeNotesFactory.createChecked(dbProvider.get(), sourceChange)); + Set reviewers = + new HashSet<>(reviewerSet.byState(ReviewerStateInternal.REVIEWER)); + reviewers.add(sourceChange.getOwner()); + reviewers.remove(user.get().getAccountId()); + Set ccs = new HashSet<>(reviewerSet.byState(ReviewerStateInternal.CC)); + ccs.remove(user.get().getAccountId()); + ins.setReviewers(reviewers).setExtraCC(ccs); + } bu.insertChange(ins); return changeId; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java index ac70e4acf1..c7ad77cca1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java @@ -94,8 +94,6 @@ public class CherryPickCommit updateFactory, null, null, - null, - null, projectName, commit, input,