Allow amending commits in ApplyPatch API
This commit adds a 'amend' option to the ApplyPatch API. If set,
the revision commit will be amended instead of taken as parent. This
is equivalent to `git commit --am`. It means that the base's parent
will be taken as parent and base's tree will be used to apply the
patch.
This is useful for use cases where callers want to iteratively rework
patch sets.
Release-Notes: skip
Change-Id: I8d38f72585494f1f85eb47ace4968c9e557bb34e
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 40d714d..7cbfea6 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -18,6 +18,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;
@@ -69,6 +70,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;
@@ -211,7 +213,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.
@@ -221,16 +223,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);
@@ -293,7 +293,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);
