diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index c89a9ff8d2..3c4f78b484 100644 --- a/Documentation/rest-api-changes.txt +++ b/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| diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 73a66a4a5f..33c8e6e015 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/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; + } + } } 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 d44c618340..e92d255488 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 @@ -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 = 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 5fbae56fbb..578949a506 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 @@ -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(); 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 3ac36014fd..75881d166f 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 @@ -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; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java index 35aa4ad37c..95491f5f4e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java +++ b/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(refName); + RefControl refControl = projectControl.controlForRef(RefNames.fullName(input.destination)); 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, - revision.getChange(), - revision.getPatchSet(), - input, - refName, - refControl); + updateFactory, revision.getChange(), revision.getPatchSet(), input, refControl); return json.noOptions().format(revision.getProject(), cherryPickedChangeId); } catch (InvalidChangeOperationException e) { throw new BadRequestException(e.getMessage()); 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 7c0a7beca8..755379d655 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 @@ -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 db; + private final Provider dbProvider; private final Sequences seq; private final Provider queryProvider; private final GitRepositoryManager gitManager; @@ -85,7 +91,7 @@ public class CherryPickChange { @Inject CherryPickChange( - Provider db, + Provider dbProvider, Sequences seq, Provider 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 targetRefControl) + RefControl destRefControl) 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 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 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 b44a8b7ff8..1b63cb503c 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 @@ -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());