CreateChange: Allow create change on new branch

We currently disallow creating a new change through CreateChange
endpoint against a non-existent branch, but we don't really have a
problem with that (except for preventing typos).

In this change, we split ChangeInfo into ChangeInfo and ChangeInput.
Added a new new_branch field in ChangeInput, default to false to
protect against typos, and when it's set to true, allow new change
against a non-existent branch.

Currently it's only implemented in the API, but not in the UI.

Change-Id: I6e661f036b121a33de183c126b763d29b5f2bc93

Documentation/rest-api-changes.txt

@@ -13,10 +13,8 @@ link:rest-api.html[REST API].
 'POST /changes'
 --
 
-The change info link:#change-info[ChangeInfo] entity must be provided in the
+The change input link:#change-input[ChangeInput] entity must be provided in the
-request body. Only the following attributes are honored: `project`,
+request body.
-`branch`, `subject`, `status` and `topic`. The first three attributes are
-mandatory. Valid values for status are: `DRAFT` and `NEW`.
 
 .Request
 ----
@@ -4016,9 +4014,29 @@ Only set on the last change that is returned.
 |`problems`           |optional|
 A list of link:#problem-info[ProblemInfo] entities describing potential
 problems with this change. Only set if link:#check[CHECK] is set.
+|==================================
+
+[[change-input]]
+=== ChangeInput
+The `ChangeInput` entity contains information about creating a new change.
+
+[options="header",cols="1,^1,5"]
+|==================================
+|Field Name           ||Description
+|`project`            ||The name of the project.
+|`branch`             ||
+The name of the target branch. +
+The `refs/heads/` prefix is omitted.
+|`subject`            ||
+The subject of the change (header line of the commit message).
+|`topic`              |optional|The topic to which this change belongs.
+|`status`             |optional, default to `NEW`|
+The status of the change (only `NEW` and `DRAFT` accepted here).
 |`base_change`        |optional|
 A link:#change-id[\{change-id\}] that identifies the base change for a create
-change operation. Only used for the link:#create-change[CreateChange] endpoint.
+change operation.
+|`new_branch`         |optional, default to `false`|
+Allow creating a new branch when set to `true`.
 |==================================
 
 [[change-message-info]]

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

@@ -48,6 +48,7 @@ import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ApprovalInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.common.GitPerson;
 import com.google.gerrit.extensions.common.LabelInfo;
@@ -585,7 +586,7 @@ public class ChangeIT extends AbstractDaemonTest {
 
   @Test
   public void createEmptyChange() throws Exception {
-    ChangeInfo in = new ChangeInfo();
+    ChangeInput in = new ChangeInput();
     in.branch = Constants.MASTER;
     in.subject = "Create a change from the API";
     in.project = project.get();
@@ -981,6 +982,39 @@ public class ChangeIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void createEmptyChangeOnNonExistingBranch() throws Exception {
+    ChangeInput in = new ChangeInput();
+    in.branch = "foo";
+    in.subject = "Create a change on new branch from the API";
+    in.project = project.get();
+    in.newBranch = true;
+    ChangeInfo info = gApi
+        .changes()
+        .create(in)
+        .get();
+    assertThat(info.project).isEqualTo(in.project);
+    assertThat(info.branch).isEqualTo(in.branch);
+    assertThat(info.subject).isEqualTo(in.subject);
+    assertThat(Iterables.getOnlyElement(info.messages).message)
+        .isEqualTo("Uploaded patch set 1.");
+  }
+
+  @Test
+  public void createEmptyChangeOnExistingBranchWithNewBranch() throws Exception {
+    ChangeInput in = new ChangeInput();
+    in.branch = Constants.MASTER;
+    in.subject = "Create a change on new branch from the API";
+    in.project = project.get();
+    in.newBranch = true;
+
+    exception.expect(ResourceConflictException.class);
+    ChangeInfo info = gApi
+        .changes()
+        .create(in)
+        .get();
+  }
+
   private static Iterable<Account.Id> getReviewers(
       Collection<AccountInfo> r) {
     return Iterables.transform(r, new Function<AccountInfo, Account.Id>() {

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

@@ -23,6 +23,7 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -61,7 +62,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
 
   @Test
   public void createEmptyChange_MissingBranch() throws Exception {
-    ChangeInfo ci = new ChangeInfo();
+    ChangeInput ci = new ChangeInput();
     ci.project = project.get();
     assertCreateFails(ci, BadRequestException.class,
         "branch must be non-empty");
@@ -69,7 +70,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
 
   @Test
   public void createEmptyChange_MissingMessage() throws Exception {
-    ChangeInfo ci = new ChangeInfo();
+    ChangeInput ci = new ChangeInput();
     ci.project = project.get();
     ci.branch = "master";
     assertCreateFails(ci, BadRequestException.class,
@@ -78,26 +79,26 @@ public class CreateChangeIT extends AbstractDaemonTest {
 
   @Test
   public void createEmptyChange_InvalidStatus() throws Exception {
-    ChangeInfo ci = newChangeInfo(ChangeStatus.MERGED);
+    ChangeInput ci = newChangeInput(ChangeStatus.MERGED);
     assertCreateFails(ci, BadRequestException.class,
         "unsupported change status");
   }
 
   @Test
   public void createNewChange() throws Exception {
-    assertCreateSucceeds(newChangeInfo(ChangeStatus.NEW));
+    assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
   }
 
   @Test
   public void createDraftChange() throws Exception {
     assume().that(isAllowDrafts()).isTrue();
-    assertCreateSucceeds(newChangeInfo(ChangeStatus.DRAFT));
+    assertCreateSucceeds(newChangeInput(ChangeStatus.DRAFT));
   }
 
   @Test
   public void createDraftChangeNotAllowed() throws Exception {
     assume().that(isAllowDrafts()).isFalse();
-    ChangeInfo ci = newChangeInfo(ChangeStatus.DRAFT);
+    ChangeInput ci = newChangeInput(ChangeStatus.DRAFT);
     assertCreateFails(ci, MethodNotAllowedException.class,
         "draft workflow is disabled");
   }
@@ -106,7 +107,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
   public void notedbCommit() throws Exception {
     assume().that(notesMigration.enabled()).isTrue();
 
-    ChangeInfo c = assertCreateSucceeds(newChangeInfo(ChangeStatus.NEW));
+    ChangeInfo c = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
     try (Repository repo = repoManager.openMetadataRepository(project);
         RevWalk rw = new RevWalk(repo)) {
       RevCommit commit = rw.parseCommit(
@@ -126,8 +127,8 @@ public class CreateChangeIT extends AbstractDaemonTest {
     }
   }
 
-  private ChangeInfo newChangeInfo(ChangeStatus status) {
+  private ChangeInput newChangeInput(ChangeStatus status) {
-    ChangeInfo in = new ChangeInfo();
+    ChangeInput in = new ChangeInput();
     in.project = project.get();
     in.branch = "master";
     in.subject = "Empty change";
@@ -136,7 +137,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
     return in;
   }
 
-  private ChangeInfo assertCreateSucceeds(ChangeInfo in) throws Exception {
+  private ChangeInfo assertCreateSucceeds(ChangeInput in) throws Exception {
     ChangeInfo out = gApi.changes().create(in).get();
     assertThat(out.branch).isEqualTo(in.branch);
     assertThat(out.subject).isEqualTo(in.subject);
@@ -149,7 +150,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
     return out;
   }
 
-  private void assertCreateFails(ChangeInfo in,
+  private void assertCreateFails(ChangeInput in,
       Class<? extends RestApiException> errType, String errSubstring)
       throws Exception {
     exception.expect(errType);

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

@@ -16,6 +16,7 @@ package com.google.gerrit.extensions.api.changes;
 
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.restapi.NotImplementedException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 
@@ -58,7 +59,7 @@ public interface Changes {
   ChangeApi id(String project, String branch, String id)
       throws RestApiException;
 
-  ChangeApi create(ChangeInfo in) throws RestApiException;
+  ChangeApi create(ChangeInput in) throws RestApiException;
 
   QueryRequest query();
   QueryRequest query(String query);
@@ -156,7 +157,7 @@ public interface Changes {
     }
 
     @Override
-    public ChangeApi create(ChangeInfo in) throws RestApiException {
+    public ChangeApi create(ChangeInput in) throws RestApiException {
       throw new NotImplementedException();
     }
 

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

@@ -43,7 +43,6 @@ public class ChangeInfo {
   public Integer insertions;
   public Integer deletions;
 
-  public String baseChange;
   public int _number;
 
   public AccountInfo owner;

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

@@ -0,0 +1,28 @@
+// Copyright (C) 2016 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.extensions.common;
+
+import com.google.gerrit.extensions.client.ChangeStatus;
+
+public class ChangeInput {
+  public String project;
+  public String branch;
+  public String subject;
+
+  public String topic;
+  public ChangeStatus status;
+  public String baseChange;
+  public Boolean newBranch;
+}

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java

@@ -23,6 +23,7 @@ import com.google.gerrit.extensions.api.changes.ChangeApi;
 import com.google.gerrit.extensions.api.changes.Changes;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -90,7 +91,7 @@ class ChangesImpl implements Changes {
   }
 
   @Override
-  public ChangeApi create(ChangeInfo in) throws RestApiException {
+  public ChangeApi create(ChangeInput in) throws RestApiException {
     try {
       ChangeInfo out = createChange.apply(
           TopLevelResource.INSTANCE, in).value();

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

@@ -20,9 +20,11 @@ import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.Capable;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -61,6 +63,7 @@ import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.TreeFormatter;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.ChangeIdUtil;
@@ -74,7 +77,7 @@ import java.util.TimeZone;
 
 @Singleton
 public class CreateChange implements
-    RestModifyView<TopLevelResource, ChangeInfo> {
+    RestModifyView<TopLevelResource, ChangeInput> {
 
   private final Provider<ReviewDb> db;
   private final GitRepositoryManager gitManager;
@@ -117,7 +120,7 @@ public class CreateChange implements
   }
 
   @Override
-  public Response<ChangeInfo> apply(TopLevelResource parent, ChangeInfo input)
+  public Response<ChangeInfo> apply(TopLevelResource parent, ChangeInput input)
       throws OrmException, IOException, InvalidChangeOperationException,
       RestApiException, UpdateException {
     if (Strings.isNullOrEmpty(input.project)) {
@@ -178,24 +181,37 @@ public class CreateChange implements
         groups = ps.getGroups();
       } else {
         Ref destRef = git.getRefDatabase().exactRef(refName);
-        if (destRef == null) {
+        if (destRef != null) {
-          throw new UnprocessableEntityException(String.format(
+          if (Boolean.TRUE.equals(input.newBranch)) {
-              "Branch %s does not exist.", refName));
+            throw new ResourceConflictException(String.format(
+                "Branch %s already exists.", refName));
+          } else {
+            parentCommit = destRef.getObjectId();
+          }
+        } else {
+          if (Boolean.TRUE.equals(input.newBranch)) {
+            parentCommit = null;
+          } else {
+            throw new UnprocessableEntityException(String.format(
+                "Branch %s does not exist.", refName));
+          }
         }
-        parentCommit = destRef.getObjectId();
         groups = Collections.emptyList();
       }
-      RevCommit mergeTip = rw.parseCommit(parentCommit);
+      RevCommit mergeTip =
+          parentCommit == null ? null : rw.parseCommit(parentCommit);
 
       Timestamp now = TimeUtil.nowTs();
       IdentifiedUser me = user.get().asIdentifiedUser();
       PersonIdent author = me.newCommitterIdent(now, serverTimeZone);
 
-      ObjectId id = ChangeIdUtil.computeChangeId(mergeTip.getTree(),
-          mergeTip, author, author, input.subject);
-      String commitMessage = ChangeIdUtil.insertId(input.subject, id);
-
       try (ObjectInserter oi = git.newObjectInserter()) {
+        ObjectId treeId =
+            mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree();
+        ObjectId id = ChangeIdUtil.computeChangeId(treeId,
+            mergeTip, author, author, input.subject);
+        String commitMessage = ChangeIdUtil.insertId(input.subject, id);
+
         RevCommit c = newCommit(oi, rw, author, mergeTip, commitMessage);
 
         Change.Id changeId = new Change.Id(seq.nextChangeId());
@@ -227,8 +243,12 @@ public class CreateChange implements
       PersonIdent authorIdent, RevCommit mergeTip, String commitMessage)
       throws IOException {
     CommitBuilder commit = new CommitBuilder();
-    commit.setTreeId(mergeTip.getTree().getId());
+    if (mergeTip == null) {
-    commit.setParentId(mergeTip);
+      commit.setTreeId(emptyTreeId(oi));
+    } else {
+      commit.setTreeId(mergeTip.getTree().getId());
+      commit.setParentId(mergeTip);
+    }
     commit.setAuthor(authorIdent);
     commit.setCommitter(authorIdent);
     commit.setMessage(commitMessage);
@@ -242,4 +262,11 @@ public class CreateChange implements
     inserter.flush();
     return id;
   }
+
+  private static ObjectId emptyTreeId(ObjectInserter inserter)
+      throws IOException {
+    ObjectId id = inserter.insert(new TreeFormatter());
+    inserter.flush();
+    return id;
+  }
 }