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
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index a5dffd1..7a907ba 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -13,10 +13,8 @@
'POST /changes'
--
-The change info link:#change-info[ChangeInfo] entity must be provided in the
-request body. Only the following attributes are honored: `project`,
-`branch`, `subject`, `status` and `topic`. The first three attributes are
-mandatory. Valid values for status are: `DRAFT` and `NEW`.
+The change input link:#change-input[ChangeInput] entity must be provided in the
+request body.
.Request
----
@@ -4016,9 +4014,29 @@
|`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]]
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 6ac86a5..486ca51 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -48,6 +48,7 @@
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 @@
@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 @@
}
}
+ @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>() {
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 1e0dc12..063a795 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
@@ -23,6 +23,7 @@
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 @@
@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 @@
@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 @@
@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 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 @@
}
}
- private ChangeInfo newChangeInfo(ChangeStatus status) {
- ChangeInfo in = new ChangeInfo();
+ private ChangeInput newChangeInput(ChangeStatus status) {
+ ChangeInput in = new ChangeInput();
in.project = project.get();
in.branch = "master";
in.subject = "Empty change";
@@ -136,7 +137,7 @@
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 @@
return out;
}
- private void assertCreateFails(ChangeInfo in,
+ private void assertCreateFails(ChangeInput in,
Class<? extends RestApiException> errType, String errSubstring)
throws Exception {
exception.expect(errType);
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java
index da8aeb2..b1381e7 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/Changes.java
@@ -16,6 +16,7 @@
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 @@
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 @@
}
@Override
- public ChangeApi create(ChangeInfo in) throws RestApiException {
+ public ChangeApi create(ChangeInput in) throws RestApiException {
throw new NotImplementedException();
}
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
index d697502..34b19c2 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -43,7 +43,6 @@
public Integer insertions;
public Integer deletions;
- public String baseChange;
public int _number;
public AccountInfo owner;
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ChangeInput.java
new file mode 100644
index 0000000..9f0df93
--- /dev/null
+++ b/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;
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java
index bb01dea..5485052 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java
@@ -23,6 +23,7 @@
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 @@
}
@Override
- public ChangeApi create(ChangeInfo in) throws RestApiException {
+ public ChangeApi create(ChangeInput in) throws RestApiException {
try {
ChangeInfo out = createChange.apply(
TopLevelResource.INSTANCE, in).value();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
index d47eb8b..c273524 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
@@ -20,9 +20,11 @@
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.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 @@
@Singleton
public class CreateChange implements
- RestModifyView<TopLevelResource, ChangeInfo> {
+ RestModifyView<TopLevelResource, ChangeInput> {
private final Provider<ReviewDb> db;
private final GitRepositoryManager gitManager;
@@ -117,7 +120,7 @@
}
@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 @@
groups = ps.getGroups();
} else {
Ref destRef = git.getRefDatabase().exactRef(refName);
- if (destRef == null) {
- throw new UnprocessableEntityException(String.format(
- "Branch %s does not exist.", refName));
+ if (destRef != null) {
+ if (Boolean.TRUE.equals(input.newBranch)) {
+ 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 @@
PersonIdent authorIdent, RevCommit mergeTip, String commitMessage)
throws IOException {
CommitBuilder commit = new CommitBuilder();
- commit.setTreeId(mergeTip.getTree().getId());
- commit.setParentId(mergeTip);
+ if (mergeTip == null) {
+ 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 @@
inserter.flush();
return id;
}
+
+ private static ObjectId emptyTreeId(ObjectInserter inserter)
+ throws IOException {
+ ObjectId id = inserter.insert(new TreeFormatter());
+ inserter.flush();
+ return id;
+ }
}
