Merge "Add support for specifying the cherry-pick base commit"

Documentation/rest-api-changes.txt

@@ -5688,6 +5688,9 @@ The `CherryPickInput` entity contains information for cherry-picking a change to
 |Field Name         ||Description
 |`message`          ||Commit message for the cherry-picked change
 |`destination`      ||Destination branch
+|`base`             |optional|
+40-hex digit SHA-1 of the commit which will be the parent commit of the newly created change.
+If set, it must be a merged commit or a change revision on the destination branch.
 |`parent`           |optional, defaults to 1|
 Number of the parent relative to which the cherry-pick should be considered.
 |`notify`           |optional|

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -1255,4 +1255,24 @@ public abstract class AbstractDaemonTest {
     String res = new String(os.toByteArray(), UTF_8);
     assertThat(res).isEqualTo(expectedContent);
   }
+
+  protected RevCommit createNewCommitWithoutChangeId(String branch, String file, String content)
+      throws Exception {
+    try (Repository repo = repoManager.openRepository(project);
+        RevWalk walk = new RevWalk(repo)) {
+      Ref ref = repo.exactRef(branch);
+      RevCommit tip = null;
+      if (ref != null) {
+        tip = walk.parseCommit(ref.getObjectId());
+      }
+      TestRepository<?> testSrcRepo = new TestRepository<>(repo);
+      TestRepository<?>.BranchBuilder builder = testSrcRepo.branch(branch);
+      RevCommit revCommit =
+          tip == null
+              ? builder.commit().message("commit 1").add(file, content).create()
+              : builder.commit().parent(tip).message("commit 1").add(file, content).create();
+      assertThat(GitUtil.getChangeId(testSrcRepo, revCommit).isPresent()).isFalse();
+      return revCommit;
+    }
+  }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java

@@ -75,6 +75,7 @@ import com.google.gerrit.extensions.restapi.ETagView;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.extensions.webui.PatchSetWebLink;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Branch;
@@ -673,6 +674,122 @@ public class RevisionIT extends AbstractDaemonTest {
     assertNotifyTo(userToNotify);
   }
 
+  @Test
+  public void cherryPickToMergedChangeRevision() throws Exception {
+    createBranch(new NameKey(project, "foo"));
+
+    PushOneCommit.Result dstChange = createChange(testRepo, "foo", SUBJECT, "b.txt", "b", "t");
+    dstChange.assertOkStatus();
+
+    merge(dstChange);
+
+    PushOneCommit.Result result = createChange(testRepo, "foo", SUBJECT, "b.txt", "c", "t");
+    result.assertOkStatus();
+    merge(result);
+
+    PushOneCommit.Result srcChange = createChange();
+
+    CherryPickInput input = new CherryPickInput();
+    input.destination = "foo";
+    input.base = dstChange.getCommit().name();
+    input.message = srcChange.getCommit().getFullMessage();
+    ChangeInfo changeInfo =
+        gApi.changes().id(srcChange.getChangeId()).current().cherryPick(input).get();
+    assertCherryPickResult(changeInfo, input, srcChange.getChangeId());
+  }
+
+  @Test
+  public void cherryPickToOpenChangeRevision() throws Exception {
+    createBranch(new NameKey(project, "foo"));
+
+    PushOneCommit.Result dstChange = createChange(testRepo, "foo", SUBJECT, "b.txt", "b", "t");
+    dstChange.assertOkStatus();
+
+    PushOneCommit.Result srcChange = createChange();
+
+    CherryPickInput input = new CherryPickInput();
+    input.destination = "foo";
+    input.base = dstChange.getCommit().name();
+    input.message = srcChange.getCommit().getFullMessage();
+    ChangeInfo changeInfo =
+        gApi.changes().id(srcChange.getChangeId()).current().cherryPick(input).get();
+    assertCherryPickResult(changeInfo, input, srcChange.getChangeId());
+  }
+
+  @Test
+  public void cherryPickToNonVisibleChangeFails() throws Exception {
+    createBranch(new NameKey(project, "foo"));
+
+    PushOneCommit.Result dstChange = createChange(testRepo, "foo", SUBJECT, "b.txt", "b", "t");
+    dstChange.assertOkStatus();
+
+    gApi.changes().id(dstChange.getChangeId()).setPrivate(true, null);
+
+    PushOneCommit.Result srcChange = createChange();
+
+    CherryPickInput input = new CherryPickInput();
+    input.destination = "foo";
+    input.base = dstChange.getCommit().name();
+    input.message = srcChange.getCommit().getFullMessage();
+
+    setApiUser(user);
+    exception.expect(UnprocessableEntityException.class);
+    exception.expectMessage(
+        String.format("Commit %s does not exist on branch refs/heads/foo", input.base));
+    gApi.changes().id(srcChange.getChangeId()).current().cherryPick(input).get();
+  }
+
+  @Test
+  public void cherryPickToAbandonedChangeFails() throws Exception {
+    PushOneCommit.Result change1 = createChange();
+    PushOneCommit.Result change2 = createChange();
+    gApi.changes().id(change2.getChangeId()).abandon();
+
+    CherryPickInput input = new CherryPickInput();
+    input.destination = "master";
+    input.base = change2.getCommit().name();
+    input.message = change1.getCommit().getFullMessage();
+
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage(
+        String.format(
+            "Change %s with commit %s is %s",
+            change2.getChange().getId().get(), input.base, ChangeStatus.ABANDONED));
+    gApi.changes().id(change1.getChangeId()).current().cherryPick(input);
+  }
+
+  @Test
+  public void cherryPickWithInvalidBaseFails() throws Exception {
+    PushOneCommit.Result change1 = createChange();
+
+    CherryPickInput input = new CherryPickInput();
+    input.destination = "master";
+    input.base = "invalid-sha1";
+    input.message = change1.getCommit().getFullMessage();
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage(String.format("Base %s doesn't represent a valid SHA-1", input.base));
+    gApi.changes().id(change1.getChangeId()).current().cherryPick(input);
+  }
+
+  @Test
+  public void cherryPickToCommitWithoutChangeId() throws Exception {
+    RevCommit commit1 = createNewCommitWithoutChangeId("refs/heads/foo", "a.txt", "content 1");
+
+    createNewCommitWithoutChangeId("refs/heads/foo", "a.txt", "content 2");
+
+    PushOneCommit.Result srcChange = createChange("subject", "b.txt", "b");
+    srcChange.assertOkStatus();
+
+    CherryPickInput input = new CherryPickInput();
+    input.destination = "foo";
+    input.base = commit1.name();
+    input.message = srcChange.getCommit().getFullMessage();
+    ChangeInfo changeInfo =
+        gApi.changes().id(srcChange.getChangeId()).current().cherryPick(input).get();
+    assertCherryPickResult(changeInfo, input, srcChange.getChangeId());
+  }
+
   @Test
   public void canRebase() throws Exception {
     PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
@@ -1086,6 +1203,16 @@ public class RevisionIT extends AbstractDaemonTest {
         .containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.getId()));
   }
 
+  private static void assertCherryPickResult(
+      ChangeInfo changeInfo, CherryPickInput input, String srcChangeId) throws Exception {
+    assertThat(changeInfo.changeId).isEqualTo(srcChangeId);
+    assertThat(changeInfo.revisions.keySet()).containsExactly(changeInfo.currentRevision);
+    RevisionInfo revisionInfo = changeInfo.revisions.get(changeInfo.currentRevision);
+    assertThat(revisionInfo.commit.message).isEqualTo(input.message);
+    assertThat(revisionInfo.commit.parents).hasSize(1);
+    assertThat(revisionInfo.commit.parents.get(0).commit).isEqualTo(input.base);
+  }
+
   private PushOneCommit.Result updateChange(PushOneCommit.Result r, String content)
       throws Exception {
     PushOneCommit push =

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.acceptance.rest.change;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.common.truth.Truth8.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.common.data.Permission.READ;
 import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
@@ -27,7 +26,6 @@ import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.PushOneCommit.Result;
 import com.google.gerrit.acceptance.RestResponse;
@@ -59,11 +57,9 @@ import com.google.gerrit.testutil.TestTimeUtil;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-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;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -340,7 +336,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
     input.message = "it goes to foo branch";
     gApi.projects().name(project.get()).branch(input.destination).create(new BranchInput());
 
-    RevCommit revCommit = createNewCommitWithoutChangeId();
+    RevCommit revCommit = createNewCommitWithoutChangeId("refs/heads/master", "a.txt", "content");
     ChangeInfo changeInfo =
         gApi.projects().name(project.get()).commit(revCommit.getName()).cherryPick(input).get();
 
@@ -384,25 +380,6 @@ public class CreateChangeIT extends AbstractDaemonTest {
     assertThat(revInfo.commit.message).isEqualTo(input.message + "\n");
   }
 
-  private RevCommit createNewCommitWithoutChangeId() throws Exception {
-    try (Repository repo = repoManager.openRepository(project);
-        RevWalk walk = new RevWalk(repo)) {
-      Ref ref = repo.exactRef("refs/heads/master");
-      RevCommit tip = null;
-      if (ref != null) {
-        tip = walk.parseCommit(ref.getObjectId());
-      }
-      TestRepository<?> testSrcRepo = new TestRepository<>(repo);
-      TestRepository<?>.BranchBuilder builder = testSrcRepo.branch("refs/heads/master");
-      RevCommit revCommit =
-          tip == null
-              ? builder.commit().message("commit 1").add("a.txt", "content").create()
-              : builder.commit().parent(tip).message("commit 1").add("a.txt", "content").create();
-      assertThat(GitUtil.getChangeId(testSrcRepo, revCommit)).isEmpty();
-      return revCommit;
-    }
-  }
-
   private ChangeInput newChangeInput(ChangeStatus status) {
     ChangeInput in = new ChangeInput();
     in.project = project.get();

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java

@@ -18,7 +18,10 @@ import java.util.Map;
 
 public class CherryPickInput {
   public String message;
+  // Cherry-pick destination branch, which will be the destination of the newly created change.
   public String destination;
+  // 40-hex digit SHA-1 of the commit which will be the parent commit of the newly created change.
+  public String base;
   public Integer parent;
 
   public NotifyHandling notify = NotifyHandling.NONE;

gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java

@@ -84,8 +84,7 @@ public class CherryPick
       throw new AuthException(capable.getMessage());
     }
 
-    String refName = RefNames.fullName(input.destination);
+    RefControl refControl = projectControl.controlForRef(RefNames.fullName(input.destination));
-    RefControl refControl = projectControl.controlForRef(refName);
     if (!refControl.canUpload()) {
       throw new AuthException(
           "Not allowed to cherry pick "
@@ -97,12 +96,7 @@ public class CherryPick
     try {
       Change.Id cherryPickedChangeId =
           cherryPickChange.cherryPick(
-              updateFactory,
+              updateFactory, revision.getChange(), revision.getPatchSet(), input, refControl);
-              revision.getChange(),
-              revision.getPatchSet(),
-              input,
-              refName,
-              refControl);
       return json.noOptions().format(revision.getProject(), cherryPickedChangeId);
     } catch (InvalidChangeOperationException e) {
       throw new BadRequestException(e.getMessage());

gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java

@@ -21,9 +21,12 @@ import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.api.changes.CherryPickInput;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 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.Branch;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Change.Status;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
@@ -59,18 +62,21 @@ import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.List;
 import java.util.TimeZone;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.ChangeIdUtil;
 
 @Singleton
 public class CherryPickChange {
 
-  private final Provider<ReviewDb> db;
+  private final Provider<ReviewDb> dbProvider;
   private final Sequences seq;
   private final Provider<InternalChangeQuery> queryProvider;
   private final GitRepositoryManager gitManager;
@@ -85,7 +91,7 @@ public class CherryPickChange {
 
   @Inject
   CherryPickChange(
-      Provider<ReviewDb> db,
+      Provider<ReviewDb> dbProvider,
       Sequences seq,
       Provider<InternalChangeQuery> queryProvider,
       @GerritPersonIdent PersonIdent myIdent,
@@ -97,7 +103,7 @@ public class CherryPickChange {
       ChangeMessagesUtil changeMessagesUtil,
       PatchSetUtil psUtil,
       NotifyUtil notifyUtil) {
-    this.db = db;
+    this.dbProvider = dbProvider;
     this.seq = seq;
     this.queryProvider = queryProvider;
     this.gitManager = gitManager;
@@ -116,7 +122,6 @@ public class CherryPickChange {
       Change change,
       PatchSet patch,
       CherryPickInput input,
-      String ref,
       RefControl refControl)
       throws OrmException, IOException, InvalidChangeOperationException, IntegrationException,
           UpdateException, RestApiException {
@@ -129,7 +134,6 @@ public class CherryPickChange {
         change.getProject(),
         ObjectId.fromString(patch.getRevision().get()),
         input,
-        ref,
         refControl);
   }
 
@@ -142,17 +146,10 @@ public class CherryPickChange {
       Project.NameKey project,
       ObjectId sourceCommit,
       CherryPickInput input,
-      String targetRef,
+      RefControl destRefControl)
-      RefControl targetRefControl)
       throws OrmException, IOException, InvalidChangeOperationException, IntegrationException,
           UpdateException, RestApiException {
 
-    if (Strings.isNullOrEmpty(targetRef)) {
-      throw new InvalidChangeOperationException(
-          "Cherry Pick: Destination branch cannot be null or empty");
-    }
-
-    String destinationBranch = RefNames.shortName(targetRef);
     IdentifiedUser identifiedUser = user.get();
     try (Repository git = gitManager.openRepository(project);
         // This inserter and revwalk *must* be passed to any BatchUpdates
@@ -161,13 +158,14 @@ public class CherryPickChange {
         ObjectInserter oi = git.newObjectInserter();
         ObjectReader reader = oi.newReader();
         CodeReviewRevWalk revWalk = CodeReviewCommit.newRevWalk(reader)) {
-      Ref destRef = git.getRefDatabase().exactRef(targetRef);
+      String destRefName = destRefControl.getRefName();
+      Ref destRef = git.getRefDatabase().exactRef(destRefName);
       if (destRef == null) {
         throw new InvalidChangeOperationException(
-            String.format("Branch %s does not exist.", destinationBranch));
+            String.format("Branch %s does not exist.", destRefName));
       }
 
-      CodeReviewCommit mergeTip = revWalk.parseCommit(destRef.getObjectId());
+      RevCommit baseCommit = getBaseCommit(destRef, project.get(), revWalk, input.base);
 
       CodeReviewCommit commitToCherryPick = revWalk.parseCommit(sourceCommit);
 
@@ -185,7 +183,7 @@ public class CherryPickChange {
       final ObjectId computedChangeId =
           ChangeIdUtil.computeChangeId(
               commitToCherryPick.getTree(),
-              mergeTip,
+              baseCommit,
               commitToCherryPick.getAuthorIdent(),
               committerIdent,
               input.message);
@@ -193,14 +191,14 @@ public class CherryPickChange {
 
       CodeReviewCommit cherryPickCommit;
       try {
-        ProjectState projectState = targetRefControl.getProjectControl().getProjectState();
+        ProjectState projectState = destRefControl.getProjectControl().getProjectState();
         cherryPickCommit =
             mergeUtilFactory
                 .create(projectState)
                 .createCherryPickFromCommit(
                     oi,
                     git.getConfig(),
-                    mergeTip,
+                    baseCommit,
                     commitToCherryPick,
                     committerIdent,
                     commitMessage,
@@ -227,14 +225,15 @@ public class CherryPickChange {
                   + " reside on the same branch. "
                   + "Cannot create a new patch set.");
         }
-        try (BatchUpdate bu = batchUpdateFactory.create(db.get(), project, identifiedUser, now)) {
+        try (BatchUpdate bu =
+            batchUpdateFactory.create(dbProvider.get(), project, identifiedUser, now)) {
           bu.setRepository(git, revWalk, oi);
           Change.Id result;
           if (destChanges.size() == 1) {
             // The change key exists on the destination branch. The cherry pick
             // will be added as a new patch set.
             ChangeControl destCtl =
-                targetRefControl.getProjectControl().controlFor(destChanges.get(0).notes());
+                destRefControl.getProjectControl().controlFor(destChanges.get(0).notes());
             result = insertPatchSet(bu, git, destCtl, cherryPickCommit, input);
           } else {
             // Change key not found on destination branch. We can create a new
@@ -247,7 +246,7 @@ public class CherryPickChange {
                 createNewChange(
                     bu,
                     cherryPickCommit,
-                    targetRefControl.getRefName(),
+                    destRefControl.getRefName(),
                     newTopic,
                     sourceBranch,
                     sourceCommit,
@@ -257,7 +256,10 @@ public class CherryPickChange {
               bu.addOp(
                   sourceChangeId,
                   new AddMessageToSourceChangeOp(
-                      changeMessagesUtil, sourcePatchId, destinationBranch, cherryPickCommit));
+                      changeMessagesUtil,
+                      sourcePatchId,
+                      RefNames.shortName(destRefName),
+                      cherryPickCommit));
             }
           }
           bu.execute();
@@ -269,6 +271,49 @@ public class CherryPickChange {
     }
   }
 
+  private RevCommit getBaseCommit(Ref destRef, String project, RevWalk revWalk, String base)
+      throws RestApiException, IOException, OrmException {
+    RevCommit destRefTip = revWalk.parseCommit(destRef.getObjectId());
+    // The tip commit of the destination ref is the default base for the newly created change.
+    if (Strings.isNullOrEmpty(base)) {
+      return destRefTip;
+    }
+
+    ObjectId baseObjectId;
+    try {
+      baseObjectId = ObjectId.fromString(base);
+    } catch (InvalidObjectIdException e) {
+      throw new BadRequestException(String.format("Base %s doesn't represent a valid SHA-1", base));
+    }
+
+    RevCommit baseCommit = revWalk.parseCommit(baseObjectId);
+    InternalChangeQuery changeQuery = queryProvider.get();
+    changeQuery.enforceVisibility(true);
+    List<ChangeData> changeDatas = changeQuery.byBranchCommit(project, destRef.getName(), base);
+
+    if (changeDatas.isEmpty()) {
+      if (revWalk.isMergedInto(baseCommit, destRefTip)) {
+        // The base commit is a merged commit with no change associated.
+        return baseCommit;
+      }
+      throw new UnprocessableEntityException(
+          String.format("Commit %s does not exist on branch %s", base, destRef.getName()));
+    } else if (changeDatas.size() != 1) {
+      throw new ResourceConflictException("Multiple changes found for commit " + base);
+    }
+
+    Change change = changeDatas.get(0).change();
+    Change.Status status = change.getStatus();
+    if (status == Status.NEW || status == Status.MERGED) {
+      // The base commit is a valid change revision.
+      return baseCommit;
+    }
+
+    throw new ResourceConflictException(
+        String.format(
+            "Change %s with commit %s is %s", change.getChangeId(), base, status.asChangeStatus()));
+  }
+
   private Change.Id insertPatchSet(
       BatchUpdate bu,
       Repository git,
@@ -278,7 +323,7 @@ public class CherryPickChange {
       throws IOException, OrmException, BadRequestException {
     Change destChange = destCtl.getChange();
     PatchSet.Id psId = ChangeUtil.nextPatchSetId(git, destChange.currentPatchSetId());
-    PatchSet current = psUtil.current(db.get(), destCtl.getNotes());
+    PatchSet current = psUtil.current(dbProvider.get(), destCtl.getNotes());
 
     PatchSetInserter inserter = patchSetInserterFactory.create(destCtl, psId, cherryPickCommit);
     inserter

gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java

@@ -85,7 +85,7 @@ public class CherryPickCommit
     try {
       Change.Id cherryPickedChangeId =
           cherryPickChange.cherryPick(
-              updateFactory, null, null, null, null, project, commit, input, refName, refControl);
+              updateFactory, null, null, null, null, project, commit, input, refControl);
       return json.noOptions().format(project, cherryPickedChangeId);
     } catch (InvalidChangeOperationException e) {
       throw new BadRequestException(e.getMessage());