Merge "patch:apply - fix base commit picking"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 2cb4ecf..0162c48 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2233,7 +2233,7 @@
link:#applypatchpatchset-input[ApplyPatchPatchSetInput] entity.
If a base commit is given, the patch is applied on top of it. Otherwise, the
-patch is applied on top of the target change's branch tip.
+patch is applied on top of the target change's original parent.
Applying the patch will fail if the destination change is closed, or in case of any conflicts.
diff --git a/java/com/google/gerrit/server/restapi/change/ApplyPatch.java b/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
index c6e1e5e..f368eb9 100644
--- a/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
@@ -16,6 +16,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.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
@@ -24,6 +25,7 @@
import com.google.gerrit.extensions.api.changes.ApplyPatchPatchSetInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.PreconditionFailedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -139,9 +141,21 @@
destChange.change().getStatus().name()));
}
- RevCommit baseCommit =
- CommitUtil.getBaseCommit(
- project.get(), queryProvider.get(), revWalk, destRef, input.base);
+ RevCommit baseCommit;
+ if (!Strings.isNullOrEmpty(input.base)) {
+ baseCommit =
+ CommitUtil.getBaseCommit(
+ project.get(), queryProvider.get(), revWalk, destRef, input.base);
+ } else {
+ RevCommit latestPatchset = revWalk.parseCommit(destChange.currentPatchSet().commitId());
+ if (latestPatchset.getParentCount() != 1) {
+ throw new BadRequestException(
+ String.format(
+ "Cannot parse base commit for a change with none or multiple parents. Change ID: %s.",
+ destChange.getId()));
+ }
+ baseCommit = revWalk.parseCommit(latestPatchset.getParent(0));
+ }
ObjectId treeId = ApplyPatchUtil.applyPatch(repo, oi, input.patch, baseCommit);
Instant now = TimeUtil.now();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
index a1b9e3a..b451656 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
@@ -17,6 +17,9 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
+import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_COMMIT;
+import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_FILES;
+import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -47,6 +50,7 @@
import com.google.inject.Inject;
import java.io.IOException;
import org.eclipse.jgit.api.errors.PatchApplyException;
+import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.util.Base64;
import org.junit.Test;
@@ -167,7 +171,7 @@
ChangeInfo result = applyPatch(in);
BinaryResult resultPatch = gApi.changes().id(result.id).current().patch();
- assertThat(removeHeader(resultPatch)).isEqualTo(removeHeader(originalPatch));
+ assertThat(cleanPatch(resultPatch)).isEqualTo(cleanPatch(originalPatch));
}
@Test
@@ -189,52 +193,54 @@
ChangeInfo result = applyPatch(in);
BinaryResult resultPatch = gApi.changes().id(result.id).current().patch();
- assertThat(removeHeader(resultPatch)).isEqualTo(removeHeader(originalPatch));
+ assertThat(cleanPatch(resultPatch)).isEqualTo(cleanPatch(originalPatch));
}
@Test
public void applyGerritBasedPatchUsingRest_success() throws Exception {
String head = getHead(repo(), HEAD).name();
- createBranchWithRevision(BranchNameKey.create(project, "branch"), head);
- PushOneCommit.Result baseCommit = createChange("Add file", ADDED_FILE_NAME, ADDED_FILE_CONTENT);
- baseCommit.assertOkStatus();
createBranchWithRevision(BranchNameKey.create(project, DESTINATION_BRANCH), head);
+ PushOneCommit.Result destChange = createChange("refs/for/" + DESTINATION_BRANCH);
+ createBranchWithRevision(BranchNameKey.create(project, "branch"), head);
+ PushOneCommit.Result baseCommit =
+ createChange(testRepo, "branch", "Add file", ADDED_FILE_NAME, ADDED_FILE_CONTENT, "");
+ baseCommit.assertOkStatus();
RestResponse patchResp =
userRestSession.get("/changes/" + baseCommit.getChangeId() + "/revisions/current/patch");
patchResp.assertOK();
String originalPatch = new String(Base64.decode(patchResp.getEntityContent()), UTF_8);
ApplyPatchPatchSetInput in = buildInput(originalPatch);
- PushOneCommit.Result destChange = createChange();
RestResponse resp =
adminRestSession.post("/changes/" + destChange.getChangeId() + "/patch:apply", in);
resp.assertOK();
BinaryResult resultPatch = gApi.changes().id(destChange.getChangeId()).current().patch();
- assertThat(removeHeader(resultPatch)).isEqualTo(removeHeader(originalPatch));
+ assertThat(cleanPatch(resultPatch)).isEqualTo(cleanPatch(originalPatch));
}
@Test
public void applyGerritBasedPatchUsingRestWithEncodedPatch_success() throws Exception {
String head = getHead(repo(), HEAD).name();
- createBranchWithRevision(BranchNameKey.create(project, "branch"), head);
- PushOneCommit.Result baseCommit = createChange("Add file", ADDED_FILE_NAME, ADDED_FILE_CONTENT);
- baseCommit.assertOkStatus();
createBranchWithRevision(BranchNameKey.create(project, DESTINATION_BRANCH), head);
+ PushOneCommit.Result destChange = createChange("refs/for/" + DESTINATION_BRANCH);
+ createBranchWithRevision(BranchNameKey.create(project, "branch"), head);
+ PushOneCommit.Result baseCommit =
+ createChange(testRepo, "branch", "Add file", ADDED_FILE_NAME, ADDED_FILE_CONTENT, "");
+ baseCommit.assertOkStatus();
RestResponse patchResp =
userRestSession.get("/changes/" + baseCommit.getChangeId() + "/revisions/current/patch");
patchResp.assertOK();
String originalEncodedPatch = patchResp.getEntityContent();
String originalDecodedPatch = new String(Base64.decode(patchResp.getEntityContent()), UTF_8);
ApplyPatchPatchSetInput in = buildInput(originalEncodedPatch);
- PushOneCommit.Result destChange = createChange();
RestResponse resp =
adminRestSession.post("/changes/" + destChange.getChangeId() + "/patch:apply", in);
resp.assertOK();
BinaryResult resultPatch = gApi.changes().id(destChange.getChangeId()).current().patch();
- assertThat(removeHeader(resultPatch)).isEqualTo(removeHeader(originalDecodedPatch));
+ assertThat(cleanPatch(resultPatch)).isEqualTo(cleanPatch(originalDecodedPatch));
}
@Test
@@ -395,6 +401,45 @@
GitPersonSubject.assertThat(person).email().isEqualTo(admin.email());
}
+ @Test
+ public void applyPatchWithExplicitBase_OverrideParentId() throws Exception {
+ PushOneCommit.Result inputParent = createChange("Input parent", "file1", "content");
+ PushOneCommit.Result parent = createChange("Parent Change", "file2", "content");
+ parent.assertOkStatus();
+ PushOneCommit.Result dest = createChange("Destination Change", "file3", "content");
+ ApplyPatchPatchSetInput in = buildInput(ADDED_FILE_DIFF);
+ in.base = inputParent.getCommit().name();
+
+ gApi.changes().id(dest.getChangeId()).applyPatch(in);
+
+ ChangeInfo result = get(dest.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
+ assertThat(result.revisions.get(result.currentRevision).commit.parents.get(0).commit)
+ .isEqualTo(inputParent.getCommit().name());
+
+ BinaryResult resultPatch = gApi.changes().id(dest.getChangeId()).current().patch();
+ assertThat(cleanPatch(resultPatch)).isEqualTo(ADDED_FILE_DIFF.trim());
+ }
+
+ @Test
+ public void applyPatchWithNoExplicitBase_overwritesLatestPatch() throws Exception {
+ PushOneCommit.Result dest = createChange("Destination Change", "ps1.txt", "ps1 content");
+ RevCommit originalParentCommit = dest.getCommit().getParent(0);
+ ApplyPatchPatchSetInput in = buildInput(ADDED_FILE_DIFF);
+
+ gApi.changes().id(dest.getChangeId()).applyPatch(in);
+
+ ChangeInfo result = get(dest.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT, CURRENT_FILES);
+ assertThat(result.revisions.get(result.currentRevision).commit.parents.get(0).commit)
+ .isEqualTo(originalParentCommit.name());
+ assertThat(result.revisions.get(result.currentRevision).files.keySet())
+ .containsExactly(ADDED_FILE_NAME);
+ assertDiffForNewFile(
+ fetchDiffForFile(result, ADDED_FILE_NAME),
+ result.currentRevision,
+ ADDED_FILE_NAME,
+ ADDED_FILE_CONTENT);
+ }
+
private void initDestBranch() throws Exception {
String head = getHead(repo(), HEAD).name();
createBranchWithRevision(BranchNameKey.create(project, ApplyPatchIT.DESTINATION_BRANCH), head);
@@ -425,11 +470,19 @@
return gApi.changes().id(result.id).current().file(fileName).diff();
}
- private String removeHeader(BinaryResult bin) throws IOException {
- return removeHeader(bin.asString());
+ private String cleanPatch(BinaryResult bin) throws IOException {
+ return cleanPatch(bin.asString());
}
- private String removeHeader(String s) {
- return s.substring(s.lastIndexOf("\ndiff --git"), s.length() - 1);
+ private String cleanPatch(String s) {
+ return s
+ // Remove the header
+ .substring(s.lastIndexOf("\ndiff --git"), s.length() - 1)
+ // Remove "index NN..NN lines
+ .replaceAll("(?m)^index.*", "")
+ // Remove empty lines
+ .replaceAll("\n+", "\n")
+ // Trim
+ .trim();
}
}
