Merge "Allow amending commits in ApplyPatch API"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index d5d68b3f..f28d619 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6875,6 +6875,10 @@
caller.
|`response_format_options` |optional|
List of link:#query-options[query options] to format the response.
+|`amend` |optional|
+If true, the revision from the URL will be amended by the patch. This will use the tree of the
+revision, apply the patch and create a new commit whose tree is the resulting tree of the operation
+and whose parent(s) are the parent(s) of the revision. Cannot be used together with `base`.
|=================================
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 28ef0ed..3de4ec9 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -923,10 +923,11 @@
String subject,
String fileName,
String content,
- String topic)
+ @Nullable String topic)
throws Exception {
PushOneCommit push = pushFactory.create(admin.newIdent(), repo, subject, fileName, content);
- return push.to("refs/for/" + branch + "%topic=" + name(topic));
+ return push.to(
+ "refs/for/" + branch + (Strings.isNullOrEmpty(topic) ? "" : "%topic=" + name(topic)));
}
protected BranchApi createBranch(BranchNameKey branch) throws Exception {
diff --git a/java/com/google/gerrit/extensions/api/changes/ApplyPatchPatchSetInput.java b/java/com/google/gerrit/extensions/api/changes/ApplyPatchPatchSetInput.java
index cf114df..b843789 100644
--- a/java/com/google/gerrit/extensions/api/changes/ApplyPatchPatchSetInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ApplyPatchPatchSetInput.java
@@ -44,4 +44,11 @@
@Nullable public AccountInput author;
@Nullable public List<ListChangesOption> responseFormatOptions;
+
+ /**
+ * If {@code true}, the revision will be amended by the patch. This will use the tree of the
+ * revision, apply the patch and create a new commit whose tree is the resulting tree of the
+ * operation and whose parent(s) are the parent(s) of the revision.
+ */
+ @Nullable public Boolean amend;
}
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index 11d755b..74f1355 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -19,6 +19,7 @@
import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.CHANGE_MODIFICATION;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
@@ -70,6 +71,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.errors.MissingObjectException;
@@ -212,7 +214,7 @@
* @param oi ObjectInserter for inserting the newly created commit.
* @param authorIdent of the new commit
* @param committerIdent of the new commit
- * @param parentCommit of the new commit. Can be null.
+ * @param parents of the new commit. Can be empty.
* @param commitMessage for the new commit.
* @param treeId of the content for the new commit.
* @return the newly created commit.
@@ -222,16 +224,14 @@
ObjectInserter oi,
PersonIdent authorIdent,
PersonIdent committerIdent,
- @Nullable RevCommit parentCommit,
+ List<RevCommit> parents,
String commitMessage,
ObjectId treeId)
throws IOException {
logger.atFine().log("Creating commit with tree: %s", treeId.getName());
CommitBuilder commit = new CommitBuilder();
commit.setTreeId(treeId);
- if (parentCommit != null) {
- commit.setParentId(parentCommit);
- }
+ commit.setParentIds(parents.stream().map(RevCommit::getId).collect(Collectors.toList()));
commit.setAuthor(authorIdent);
commit.setCommitter(committerIdent);
commit.setMessage(commitMessage);
@@ -294,7 +294,12 @@
}
return createCommitWithTree(
- oi, authorIdent, committerIdent, commitToRevert, message, parentToCommitToRevert.getTree());
+ oi,
+ authorIdent,
+ committerIdent,
+ ImmutableList.of(commitToRevert),
+ message,
+ parentToCommitToRevert.getTree());
}
private Change.Id createRevertChangeFromCommit(
diff --git a/java/com/google/gerrit/server/patch/DiffUtil.java b/java/com/google/gerrit/server/patch/DiffUtil.java
index 500d3df..022614a 100644
--- a/java/com/google/gerrit/server/patch/DiffUtil.java
+++ b/java/com/google/gerrit/server/patch/DiffUtil.java
@@ -174,8 +174,8 @@
// Remove any lines which are not diff lines or file header lines - such index,
// hunk-headers, and context lines.
.replaceAll("(?m)^[^+-].*", "")
- .replaceAll("(?m)^[+]{3} [ab]/", "+++")
- .replaceAll("(?m)^-{3} [ab]/", "+++")
+ .replaceAll("(?m)^[+]{3} [ab]/", "+++ ")
+ .replaceAll("(?m)^-{3} [ab]/", "--- ")
// Remove empty lines
.replaceAll("\n+", "\n")
// Trim
diff --git a/java/com/google/gerrit/server/restapi/change/ApplyPatch.java b/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
index 58cd010..ff37fd2 100644
--- a/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
@@ -142,13 +142,18 @@
destChange.change().getStatus().name()));
}
- RevCommit latestPatchset = revWalk.parseCommit(destChange.currentPatchSet().commitId());
+ if (!Strings.isNullOrEmpty(input.base) && Boolean.TRUE.equals(input.amend)) {
+ throw new BadRequestException("amend only works with existing revisions. omit base.");
+ }
+ RevCommit latestPatchset = revWalk.parseCommit(destChange.currentPatchSet().commitId());
RevCommit baseCommit;
+ List<RevCommit> parents;
if (!Strings.isNullOrEmpty(input.base)) {
baseCommit =
CommitUtil.getBaseCommit(
project.get(), queryProvider.get(), revWalk, destRef, input.base);
+ parents = ImmutableList.of(baseCommit);
} else {
if (latestPatchset.getParentCount() != 1) {
throw new BadRequestException(
@@ -156,7 +161,13 @@
"Cannot parse base commit for a change with none or multiple parents. Change ID: %s.",
destChange.getId()));
}
- baseCommit = revWalk.parseCommit(latestPatchset.getParent(0));
+ if (Boolean.TRUE.equals(input.amend)) {
+ baseCommit = latestPatchset;
+ parents = ImmutableList.copyOf(baseCommit.getParents());
+ } else {
+ baseCommit = revWalk.parseCommit(latestPatchset.getParent(0));
+ parents = ImmutableList.of(baseCommit);
+ }
}
PatchApplier.Result applyResult =
ApplyPatchUtil.applyPatch(repo, oi, input.patch, baseCommit);
@@ -180,10 +191,9 @@
input.patch.patch,
ApplyPatchUtil.getResultPatch(repo, reader, baseCommit, revWalk.lookupTree(treeId)),
applyResult.getErrors());
-
ObjectId appliedCommit =
CommitUtil.createCommitWithTree(
- oi, authorIdent, committerIdent, baseCommit, commitMessage, treeId);
+ oi, authorIdent, committerIdent, parents, commitMessage, treeId);
CodeReviewCommit commit = revWalk.parseCommit(appliedCommit);
oi.flush();
diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java
index 4a70684..62752c2 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java
@@ -422,7 +422,12 @@
c =
rw.parseCommit(
CommitUtil.createCommitWithTree(
- oi, author, committer, mergeTip, appliedPatchCommitMessage, treeId));
+ oi,
+ author,
+ committer,
+ ImmutableList.of(mergeTip),
+ appliedPatchCommitMessage,
+ treeId));
} else {
// create an empty commit.
c = createEmptyCommit(oi, rw, author, committer, mergeTip, commitMessage);
@@ -595,14 +600,15 @@
CodeReviewRevWalk rw,
PersonIdent authorIdent,
PersonIdent committerIdent,
- RevCommit mergeTip,
+ @Nullable RevCommit mergeTip,
String commitMessage)
throws IOException {
logger.atFine().log("Creating empty commit");
ObjectId treeID = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree().getId();
+ List<RevCommit> parents = mergeTip == null ? ImmutableList.of() : ImmutableList.of(mergeTip);
return rw.parseCommit(
CommitUtil.createCommitWithTree(
- oi, authorIdent, committerIdent, mergeTip, commitMessage, treeID));
+ oi, authorIdent, committerIdent, parents, commitMessage, treeID));
}
private static ObjectId emptyTreeId(ObjectInserter inserter) throws IOException {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
index 6d51cb4..d9b01ad 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
@@ -35,6 +35,7 @@
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.accounts.AccountInput;
import com.google.gerrit.extensions.api.changes.ApplyPatchInput;
import com.google.gerrit.extensions.api.changes.ApplyPatchPatchSetInput;
@@ -46,10 +47,14 @@
import com.google.gerrit.extensions.common.GitPerson;
import com.google.gerrit.extensions.common.testing.GitPersonSubject;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.inject.Inject;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.util.Base64;
import org.junit.Test;
@@ -516,6 +521,154 @@
.isEqualTo("Default commit message\n\nChange-Id: " + result.changeId + "\n");
}
+ @Test
+ public void amendCommitWithValidTraditionalPatch_success() throws Exception {
+ final String fileName = "file_name.txt";
+ final String originalContent = "original line";
+ final String newContent = "new line\n";
+ final String diff =
+ "diff file_name.txt file_name.txt\n"
+ + "--- file_name.txt\n"
+ + "+++ file_name.txt\n"
+ + "@@ -1 +1 @@\n"
+ + "-original line\n"
+ + "+new line\n";
+
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, "Test", fileName, "foo");
+ PushOneCommit.Result base = push.to("refs/heads/foo");
+ base.assertOkStatus();
+
+ PushOneCommit.Result firstPatchSet =
+ createChange(
+ testRepo, "foo", "Add original file: " + fileName, fileName, originalContent, null);
+ firstPatchSet.assertOkStatus();
+
+ ApplyPatchPatchSetInput in = new ApplyPatchPatchSetInput();
+ in.patch = new ApplyPatchInput();
+ in.patch.patch = diff;
+ in.amend = true;
+ in.responseFormatOptions =
+ ImmutableList.of(ListChangesOption.CURRENT_REVISION, ListChangesOption.CURRENT_COMMIT);
+
+ ChangeInfo result = gApi.changes().id(firstPatchSet.getChangeId()).applyPatch(in);
+
+ // Parent of patch set 2 = parent of patch set 1, so we actually amended
+ assertThat(result.revisions.get(result.currentRevision).commit.parents.get(0).commit)
+ .isEqualTo(base.getCommit().getId().getName());
+ DiffInfo fileDiff = gApi.changes().id(result.id).current().file(fileName).diff();
+ assertDiffForFullyModifiedFile(fileDiff, result.currentRevision, fileName, "foo", newContent);
+ assertThat(gApi.changes().id(firstPatchSet.getChangeId()).current().commit(false).message)
+ .isEqualTo(firstPatchSet.getCommit().getFullMessage());
+ }
+
+ @Test
+ public void amendCantBeUsedWithBase() throws Exception {
+ final String diff =
+ "diff file_name.txt file_name.txt\n"
+ + "--- file_name.txt\n"
+ + "+++ file_name.txt\n"
+ + "@@ -1 +1 @@\n"
+ + "-original line\n"
+ + "+new line\n";
+ PushOneCommit.Result change = createChange();
+ ApplyPatchPatchSetInput in = new ApplyPatchPatchSetInput();
+ in.patch = new ApplyPatchInput();
+ in.patch.patch = diff;
+ in.amend = true;
+ in.base = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(change.getChangeId()).applyPatch(in));
+ assertThat(thrown)
+ .hasMessageThat()
+ .isEqualTo("amend only works with existing revisions. omit base.");
+ }
+
+ @Test
+ public void amendCommitWithConflict_appendErrorsToCommitMessage() throws Exception {
+ final String fileName = "file_name.txt";
+ final String originalContent = "original line";
+ final String diff =
+ "diff file_name.txt file_name.txt\n"
+ + "--- file_name.txt\n"
+ + "+++ file_name.txt\n"
+ + "@@ -1 +1 @@\n"
+ + "-xxx line\n"
+ + "+new line\n";
+
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, "Test", fileName, "foo");
+ PushOneCommit.Result base = push.to("refs/heads/foo");
+ base.assertOkStatus();
+
+ PushOneCommit.Result firstPatchSet =
+ createChange(
+ testRepo, "foo", "Add original file: " + fileName, fileName, originalContent, null);
+ firstPatchSet.assertOkStatus();
+
+ ApplyPatchPatchSetInput in = new ApplyPatchPatchSetInput();
+ in.patch = new ApplyPatchInput();
+ in.patch.patch = diff;
+ in.amend = true;
+ in.responseFormatOptions =
+ ImmutableList.of(ListChangesOption.CURRENT_REVISION, ListChangesOption.CURRENT_COMMIT);
+
+ ChangeInfo result = gApi.changes().id(firstPatchSet.getChangeId()).applyPatch(in);
+ assertThat(gApi.changes().id(result.id).current().commit(false).message)
+ .startsWith(
+ "Add original file: file_name.txt\n"
+ + "\n"
+ + "NOTE FOR REVIEWERS - errors occurred while applying the patch.\n"
+ + "PLEASE REVIEW CAREFULLY.\n"
+ + "Errors:\n"
+ + "Error applying patch in file_name.txt, hunk HunkHeader[1,1->1,1]: Hunk cannot be applied\n"
+ + "\n"
+ + "Original patch:\n"
+ + " diff file_name.txt file_name.txt\n"
+ + "--- file_name.txt\n"
+ + "+++ file_name.txt\n"
+ + "@@ -1 +1 @@\n"
+ + "-xxx line\n"
+ + "+new line");
+ }
+
+ @Test
+ public void amendCommitWithValidTraditionalPatchEmptyRepo_resourceNotFound() throws Exception {
+ final String fileName = "file_name.txt";
+ final String originalContent = "original line";
+ final String diff =
+ "diff file_name.txt file_name.txt\n"
+ + "--- file_name.txt\n"
+ + "+++ file_name.txt\n"
+ + "@@ -1 +1 @@\n"
+ + "-original line\n"
+ + "+new line\n";
+
+ Project.NameKey emptyProject = projectOperations.newProject().noEmptyCommit().create();
+ TestRepository<InMemoryRepository> emptyClone = cloneProject(emptyProject);
+ PushOneCommit.Result firstPatchSet =
+ createChange(
+ emptyClone,
+ "master",
+ "Add original file: " + fileName,
+ fileName,
+ originalContent,
+ null);
+ firstPatchSet.assertOkStatus();
+
+ ApplyPatchPatchSetInput in = new ApplyPatchPatchSetInput();
+ in.patch = new ApplyPatchInput();
+ in.patch.patch = diff;
+ in.amend = true;
+
+ Throwable error =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.changes().id(firstPatchSet.getChangeId()).applyPatch(in));
+ assertThat(error).hasMessageThat().contains("Branch refs/heads/master does not exist");
+ }
+
private void initDestBranch() throws Exception {
String head = getHead(repo(), HEAD).name();
createBranchWithRevision(BranchNameKey.create(project, ApplyPatchIT.DESTINATION_BRANCH), head);
