Merge "Allow to set base_change for CreateMergePatchSet"

Documentation/rest-api-changes.txt

@@ -6446,7 +6446,10 @@ The new subject for the change, if not specified, will reuse the current patch
 set's subject
 |`inheritParent`      |optional, default to `false`|
 Use the current patch set's first parent as the merge tip when set to `true`.
-Otherwise, use the current branch tip of the destination branch.
+|`base_change`        |optional|
+A link:#change-id[\{change-id\}] that identifies a change. When `inheritParent`
+is `false`, the merge tip will be the current patch set of the `base_change` if
+it's set. Otherwise, the current branch tip of the destination branch will be used.
 |`merge`              ||
 The detail of the source commit for merge as a link:#merge-input[MergeInput]
 entity.

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

@@ -655,6 +655,10 @@ public abstract class AbstractDaemonTest {
     return push.to("refs/for/" + branch + "/" + name(topic));
   }
 
+  protected BranchApi createBranch(String branch) throws Exception {
+    return createBranch(new Branch.NameKey(project, branch));
+  }
+
   protected BranchApi createBranch(Branch.NameKey branch) throws Exception {
     return gApi.projects()
         .name(branch.getParentKey().get())

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -117,6 +117,7 @@ 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.RestApiException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Branch;
@@ -2552,7 +2553,7 @@ public class ChangeIT extends AbstractDaemonTest {
     String parent = currentMaster.getCommit().getName();
 
     // push a commit into dev branch
-    createBranch(new Branch.NameKey(project, "dev"));
+    createBranch("dev");
     PushOneCommit.Result changeA =
         pushFactory
             .create(db, user.getIdent(), testRepo, "change A", "A.txt", "A content")
@@ -2588,7 +2589,7 @@ public class ChangeIT extends AbstractDaemonTest {
     currentMaster.assertOkStatus();
 
     // push a commit into dev branch
-    createBranch(new Branch.NameKey(project, "dev"));
+    createBranch("dev");
     PushOneCommit.Result changeA =
         pushFactory
             .create(db, user.getIdent(), testRepo, "change A", "A.txt", "A content")
@@ -2612,6 +2613,70 @@ public class ChangeIT extends AbstractDaemonTest {
         .isNotEqualTo(currentMaster.getCommit().getName());
   }
 
+  @Test
+  public void createMergePatchSetCannotBaseOnInvisibleChange() throws Exception {
+    RevCommit initialHead = getRemoteHead();
+    createBranch("foo");
+    createBranch("bar");
+
+    // Create a merged commit on 'foo' branch.
+    merge(createChange("refs/for/foo"));
+
+    // Create the base change on 'bar' branch.
+    testRepo.reset(initialHead);
+    String baseChange = createChange("refs/for/bar").getChangeId();
+    gApi.changes().id(baseChange).setPrivate(true, "set private");
+
+    // Create the destination change on 'master' branch.
+    setApiUser(user);
+    testRepo.reset(initialHead);
+    String changeId = createChange().getChangeId();
+
+    exception.expect(UnprocessableEntityException.class);
+    exception.expectMessage("Read not permitted for " + baseChange);
+    gApi.changes().id(changeId).createMergePatchSet(createMergePatchSetInput(baseChange));
+  }
+
+  @Test
+  public void createMergePatchSetBaseOnChange() throws Exception {
+    RevCommit initialHead = getRemoteHead();
+    createBranch("foo");
+    createBranch("bar");
+
+    // Create a merged commit on 'foo' branch.
+    merge(createChange("refs/for/foo"));
+
+    // Create the base change on 'bar' branch.
+    testRepo.reset(initialHead);
+    PushOneCommit.Result result = createChange("refs/for/bar");
+    String baseChange = result.getChangeId();
+    String expectedParent = result.getCommit().getName();
+
+    // Create the destination change on 'master' branch.
+    testRepo.reset(initialHead);
+    String changeId = createChange().getChangeId();
+
+    gApi.changes().id(changeId).createMergePatchSet(createMergePatchSetInput(baseChange));
+
+    ChangeInfo changeInfo =
+        gApi.changes().id(changeId).get(ALL_REVISIONS, CURRENT_COMMIT, CURRENT_REVISION);
+    assertThat(changeInfo.revisions.size()).isEqualTo(2);
+    assertThat(changeInfo.subject).isEqualTo("create ps2");
+    assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.parents.get(0).commit)
+        .isEqualTo(expectedParent);
+  }
+
+  private MergePatchSetInput createMergePatchSetInput(String baseChange) {
+    MergeInput mergeInput = new MergeInput();
+    mergeInput.source = "foo";
+    MergePatchSetInput in = new MergePatchSetInput();
+    in.merge = mergeInput;
+    in.subject = "create ps2";
+    in.inheritParent = false;
+    in.baseChange = baseChange;
+    return in;
+  }
+
   @Test
   public void checkLabelsForUnsubmittedChange() throws Exception {
     PushOneCommit.Result r = createChange();

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/MergePatchSetInput.java

@@ -17,5 +17,6 @@ package com.google.gerrit.extensions.common;
 public class MergePatchSetInput {
   public String subject;
   public boolean inheritParent;
+  public String baseChange;
   public MergeInput merge;
 }

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

@@ -26,6 +26,7 @@ import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.common.MergeInput;
+import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.Response;
@@ -194,8 +195,10 @@ public class CreateChange
           throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
         }
         ChangeNotes change = Iterables.getOnlyElement(notes);
-        if (!permissionBackend.user(user).change(change).database(db).test(ChangePermission.READ)) {
-          throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
+        try {
+          permissionBackend.user(user).change(change).database(db).check(ChangePermission.READ);
+        } catch (AuthException e) {
+          throw new UnprocessableEntityException("Read not permitted for " + input.baseChange);
         }
         PatchSet ps = psUtil.current(db.get(), change);
         parentCommit = ObjectId.fromString(ps.getRevision().get());

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

@@ -16,22 +16,26 @@ package com.google.gerrit.server.change;
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.MergeInput;
 import com.google.gerrit.extensions.common.MergePatchSetInput;
+import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MergeConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
 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.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
@@ -40,7 +44,9 @@ import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.git.GitRepositoryManager;
 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.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.CommitsCollection;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -56,6 +62,7 @@ import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.sql.Timestamp;
+import java.util.List;
 import java.util.TimeZone;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
@@ -80,6 +87,8 @@ public class CreateMergePatchSet
   private final MergeUtil.Factory mergeUtilFactory;
   private final PatchSetInserter.Factory patchSetInserterFactory;
   private final ProjectCache projectCache;
+  private final ChangeFinder changeFinder;
+  private final PermissionBackend permissionBackend;
 
   @Inject
   CreateMergePatchSet(
@@ -93,7 +102,9 @@ public class CreateMergePatchSet
       MergeUtil.Factory mergeUtilFactory,
       RetryHelper retryHelper,
       PatchSetInserter.Factory patchSetInserterFactory,
-      ProjectCache projectCache) {
+      ProjectCache projectCache,
+      ChangeFinder changeFinder,
+      PermissionBackend permissionBackend) {
     super(retryHelper);
     this.db = db;
     this.gitManager = gitManager;
@@ -105,6 +116,8 @@ public class CreateMergePatchSet
     this.mergeUtilFactory = mergeUtilFactory;
     this.patchSetInserterFactory = patchSetInserterFactory;
     this.projectCache = projectCache;
+    this.changeFinder = changeFinder;
+    this.permissionBackend = permissionBackend;
   }
 
   @Override
@@ -118,6 +131,7 @@ public class CreateMergePatchSet
     if (merge == null || Strings.isNullOrEmpty(merge.source)) {
       throw new BadRequestException("merge.source must be non-empty");
     }
+    in.baseChange = Strings.nullToEmpty(in.baseChange).trim();
 
     PatchSet ps = psUtil.current(db.get(), rsrc.getNotes());
     ProjectState projectState = projectCache.checkedGet(rsrc.getProject());
@@ -135,7 +149,16 @@ public class CreateMergePatchSet
             "cannot find source commit: " + merge.source + " to merge.");
       }
 
-      RevCommit currentPsCommit = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+      RevCommit currentPsCommit;
+      List<String> groups = null;
+      if (!in.inheritParent && !in.baseChange.isEmpty()) {
+        PatchSet basePS = findBasePatchSet(in.baseChange);
+        currentPsCommit = rw.parseCommit(ObjectId.fromString(basePS.getRevision().get()));
+        groups = basePS.getGroups();
+      } else {
+        currentPsCommit = rw.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+      }
+
       Timestamp now = TimeUtil.nowTs();
       IdentifiedUser me = user.get().asIdentifiedUser();
       PersonIdent author = me.newCommitterIdent(now, serverTimeZone);
@@ -157,13 +180,15 @@ public class CreateMergePatchSet
           patchSetInserterFactory.create(rsrc.getNotes(), nextPsId, newCommit);
       try (BatchUpdate bu = updateFactory.create(db.get(), project, me, now)) {
         bu.setRepository(git, rw, oi);
-        bu.addOp(
-            rsrc.getId(),
-            psInserter
-                .setMessage("Uploaded patch set " + nextPsId.get() + ".")
-                .setNotify(NotifyHandling.NONE)
-                .setCheckAddPatchSetPermission(false)
-                .setNotify(NotifyHandling.NONE));
+        psInserter
+            .setMessage("Uploaded patch set " + nextPsId.get() + ".")
+            .setNotify(NotifyHandling.NONE)
+            .setCheckAddPatchSetPermission(false)
+            .setNotify(NotifyHandling.NONE);
+        if (groups != null) {
+          psInserter.setGroups(groups);
+        }
+        bu.addOp(rsrc.getId(), psInserter);
         bu.execute();
       }
 
@@ -172,6 +197,21 @@ public class CreateMergePatchSet
     }
   }
 
+  private PatchSet findBasePatchSet(String baseChange)
+      throws PermissionBackendException, OrmException, UnprocessableEntityException {
+    List<ChangeNotes> notes = changeFinder.find(baseChange);
+    if (notes.size() != 1) {
+      throw new UnprocessableEntityException("Base change not found: " + baseChange);
+    }
+    ChangeNotes change = Iterables.getOnlyElement(notes);
+    try {
+      permissionBackend.user(user).change(change).database(db).check(ChangePermission.READ);
+    } catch (AuthException e) {
+      throw new UnprocessableEntityException("Read not permitted for " + baseChange);
+    }
+    return psUtil.current(db.get(), change);
+  }
+
   private RevCommit createMergeCommit(
       MergePatchSetInput in,
       ProjectState projectState,
@@ -190,6 +230,8 @@ public class CreateMergePatchSet
     if (in.inheritParent) {
       // inherit first parent from previous patch set
       parentCommit = currentPsCommit.getParent(0);
+    } else if (!in.baseChange.isEmpty()) {
+      parentCommit = currentPsCommit.getId();
     } else {
       // get the current branch tip of destination branch
       Ref destRef = git.getRefDatabase().exactRef(dest.get());