Merge "Allow to apply fixes to a different patchsets"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 0305aaa..6279f02 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5816,7 +5816,12 @@
Applies a list of <<fix-replacement-info,FixReplacementInfo>> loaded from the
<<apply-provided-fix-input,ApplyProvidedFixInput>> entity. The fixes are passed as part of the request body. The
application of the fixes creates a new change edit. `Apply Provided Fix` can only be applied on the current
-patchset.
+patchset. To apply a fix that was suggested to a non-current patchset, set `originalPatchsetForFix` to a patchset
+number where the fix was suggested. When `originalPatchsetForFix` is set, gerrit generates a patch (using
+`fix-replacement-info` and `originalPatchsetForFix`) and then tries to apply it to the current patchset.
+If it is not possible to apply a patch, an error is returned (see below for the list of errors).
+If the provided fix modifies the commit message and the message was changed between `originalPatchsetForFix`
+and the current patchset, the request is rejected..
If one of the secondary emails associated with the user performing the operation was used as the
committer email in the current patch set, the same email will be used as the committer email in the
diff --git a/java/com/google/gerrit/extensions/common/ApplyProvidedFixInput.java b/java/com/google/gerrit/extensions/common/ApplyProvidedFixInput.java
index cd28d83..7d48fce 100644
--- a/java/com/google/gerrit/extensions/common/ApplyProvidedFixInput.java
+++ b/java/com/google/gerrit/extensions/common/ApplyProvidedFixInput.java
@@ -24,4 +24,6 @@
public ApplyProvidedFixInput() {}
public List<FixReplacementInfo> fixReplacementInfos;
+
+ public Integer originalPatchsetForFix;
}
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index 7a89fb3..48824fd 100644
--- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -532,7 +532,7 @@
return editBasePatchSet.id().equals(patchSet.id());
}
- private static ObjectId createNewTree(
+ public static ObjectId createNewTree(
Repository repository, RevCommit baseCommit, List<TreeModification> treeModifications)
throws BadRequestException, IOException, InvalidChangeOperationException {
if (treeModifications.isEmpty()) {
diff --git a/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java b/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java
index a55ef84b..4e05420 100644
--- a/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java
@@ -16,10 +16,12 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
+import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.entities.Comment.Range;
import com.google.gerrit.entities.FixReplacement;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.api.changes.ApplyPatchInput;
import com.google.gerrit.extensions.common.ApplyProvidedFixInput;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -27,15 +29,19 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.edit.ChangeEdit;
import com.google.gerrit.server.edit.ChangeEditJson;
import com.google.gerrit.server.edit.ChangeEditModifier;
import com.google.gerrit.server.edit.CommitModification;
+import com.google.gerrit.server.edit.tree.ChangeFileContentModification;
import com.google.gerrit.server.fixes.FixReplacementInterpreter;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.ApplyPatchUtil;
+import com.google.gerrit.server.patch.MagicFile;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectCache;
@@ -45,7 +51,14 @@
import java.io.IOException;
import java.util.List;
import java.util.stream.Collectors;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.patch.PatchApplier;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.TreeWalk;
/** Applies a fix that is provided as part of the request body. */
@Singleton
@@ -74,7 +87,7 @@
public Response<EditInfo> apply(
RevisionResource revisionResource, ApplyProvidedFixInput applyProvidedFixInput)
throws AuthException, BadRequestException, ResourceConflictException, IOException,
- ResourceNotFoundException, PermissionBackendException {
+ ResourceNotFoundException, PermissionBackendException, RestApiException {
if (applyProvidedFixInput == null) {
throw new BadRequestException("applyProvidedFixInput is required");
}
@@ -83,9 +96,19 @@
}
Project.NameKey project = revisionResource.getProject();
ProjectState projectState = projectCache.get(project).orElseThrow(illegalState(project));
- PatchSet patchSet = revisionResource.getPatchSet();
+ PatchSet targetPatchSet = revisionResource.getPatchSet();
ChangeNotes changeNotes = revisionResource.getNotes();
+ PatchSet originPatchSetForFix =
+ applyProvidedFixInput.originalPatchsetForFix != null
+ && applyProvidedFixInput.originalPatchsetForFix > 0
+ ? changeNotes
+ .getPatchSets()
+ .get(
+ PatchSet.id(
+ revisionResource.getChange().getId(),
+ applyProvidedFixInput.originalPatchsetForFix))
+ : targetPatchSet;
List<FixReplacement> fixReplacements =
applyProvidedFixInput.fixReplacementInfos.stream()
@@ -94,15 +117,87 @@
try (Repository repository = gitRepositoryManager.openRepository(project)) {
CommitModification commitModification =
- fixReplacementInterpreter.toCommitModification(
- repository, projectState, patchSet.commitId(), fixReplacements);
+ getCommitModification(
+ repository, projectState, originPatchSetForFix, targetPatchSet, fixReplacements);
ChangeEdit changeEdit =
changeEditModifier.combineWithModifiedPatchSetTree(
- repository, changeNotes, patchSet, commitModification);
+ repository, changeNotes, targetPatchSet, commitModification);
return Response.ok(changeEditJson.toEditInfo(changeEdit, false));
} catch (InvalidChangeOperationException e) {
throw new ResourceConflictException(e.getMessage());
}
}
+
+ /**
+ * Returns CommitModification for fixes and rebase it if the fix is for an older patchset.
+ *
+ * <p>The method creates CommitModification by applying {@code fixReplacements} to the {@code
+ * basePatchSetForFix}. If the {@code targetPatchSetForFix} is different from the {@code
+ * basePatchSetForFix}, CommitModification is created from the {@link PatchApplier.Result}, after
+ * applying the patch generated from {@code basePatchSetForFix} to the {@code
+ * targetPatchSetForFix}.
+ *
+ * <p>Note: if there is a fix for a commit message and commit messages are different in {@code
+ * basePatchSetForFix} and {@code targetPatchSetForFix}, the method can't move the fix to the
+ * {@code targetPatchSetForFix} and throws {@link ResourceConflictException}. This limitations
+ * exists because the method uses ApplyPatchUtil which operates only on files.
+ */
+ private CommitModification getCommitModification(
+ Repository repository,
+ ProjectState projectState,
+ PatchSet basePatchSetForFix,
+ PatchSet targetPatchSetForFix,
+ List<FixReplacement> fixReplacements)
+ throws IOException, InvalidChangeOperationException, RestApiException {
+ CommitModification originCommitModification =
+ fixReplacementInterpreter.toCommitModification(
+ repository, projectState, basePatchSetForFix.commitId(), fixReplacements);
+ if (basePatchSetForFix.id().equals(targetPatchSetForFix.id())) {
+ return originCommitModification;
+ }
+ RevCommit originCommit = repository.parseCommit(basePatchSetForFix.commitId());
+ ObjectId newTreeId =
+ ChangeEditModifier.createNewTree(
+ repository, originCommit, originCommitModification.treeModifications());
+ CommitModification.Builder resultBuilder = CommitModification.builder();
+ String patch;
+ try (RevWalk rw = new RevWalk(repository)) {
+ ObjectId targetCommit = targetPatchSetForFix.commitId();
+ if (originCommitModification.newCommitMessage().isPresent()) {
+ MagicFile originCommitMessageFile =
+ MagicFile.forCommitMessage(rw.getObjectReader(), originCommit);
+ String originCommitMessage = originCommitMessageFile.modifiableContent();
+ MagicFile targetCommitMessageFile =
+ MagicFile.forCommitMessage(rw.getObjectReader(), targetCommit);
+ String targetCommitMessage = targetCommitMessageFile.modifiableContent();
+ if (!originCommitMessage.equals(targetCommitMessage)) {
+ throw new ResourceConflictException(
+ "The fix attempts to modify commit message of an older patchset, but commit message has been updated in a newer patchset. The fix can't be applied.");
+ }
+ resultBuilder.newCommitMessage(originCommitModification.newCommitMessage().get());
+ }
+
+ patch =
+ ApplyPatchUtil.getResultPatch(
+ repository, repository.newObjectReader(), originCommit, rw.lookupTree(newTreeId));
+ PatchApplier.Result result;
+ try (ObjectInserter oi = repository.newObjectInserter()) {
+ ApplyPatchInput inp = new ApplyPatchInput();
+ inp.patch = patch;
+ inp.allowConflicts = false;
+ result =
+ ApplyPatchUtil.applyPatch(repository, oi, inp, repository.parseCommit(targetCommit));
+ oi.flush();
+ for (String path : result.getPaths()) {
+ try (TreeWalk tw = TreeWalk.forPath(rw.getObjectReader(), path, result.getTreeId())) {
+ ObjectLoader loader = rw.getObjectReader().open(tw.getObjectId(0));
+ resultBuilder.addTreeModification(
+ new ChangeFileContentModification(path, RawInputUtil.create(loader.getBytes())));
+ }
+ }
+ }
+ }
+ return resultBuilder.build();
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/change/PreviewFix.java b/java/com/google/gerrit/server/restapi/change/PreviewFix.java
index e771898..042f73d 100644
--- a/java/com/google/gerrit/server/restapi/change/PreviewFix.java
+++ b/java/com/google/gerrit/server/restapi/change/PreviewFix.java
@@ -128,6 +128,11 @@
if (applyProvidedFixInput.fixReplacementInfos == null) {
throw new BadRequestException("applyProvidedFixInput.fixReplacementInfos is required");
}
+ if (applyProvidedFixInput.originalPatchsetForFix != null
+ && applyProvidedFixInput.originalPatchsetForFix > 0) {
+ throw new BadRequestException(
+ "applyProvidedFixInput.originalPatchsetForFix is not supported on preview.");
+ }
PreviewFix previewFix = previewFixFactory.create(revisionResource);
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java b/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java
index b9ef0bf..d66a0a5 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java
@@ -357,6 +357,142 @@
}
@Test
+ public void applyProvidedFixesOnNewerPatchsetWithModifiedFile() throws Exception {
+ // Remember patch set and add another one.
+ int previousRevision = gApi.changes().id(changeId).get().currentRevisionNumber;
+ amendChange(
+ changeId,
+ "refs/for/master",
+ admin,
+ testRepo,
+ PushOneCommit.SUBJECT,
+ FILE_NAME,
+ "New line at the start\n" + FILE_CONTENT);
+ ApplyProvidedFixInput applyProvidedFixInput =
+ createApplyProvidedFixInput(FILE_NAME, "Modified content", 3, 1, 3, 3);
+ applyProvidedFixInput.originalPatchsetForFix = previousRevision;
+ gApi.changes().id(changeId).current().applyFix(applyProvidedFixInput);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "New line at the start\nFirst line\nSecond line\nTModified contentrd line\nFourth line\nFifth line\n"
+ + "Sixth line\nSeventh line\nEighth line\nNinth line\nTenth line\n");
+
+ applyProvidedFixInput = createApplyProvidedFixInput(FILE_NAME, "(1st)", 1, 5, 1, 5);
+ applyProvidedFixInput.originalPatchsetForFix = previousRevision;
+ gApi.changes().id(changeId).current().applyFix(applyProvidedFixInput);
+
+ file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "New line at the start\nFirst(1st) line\nSecond line\nTModified contentrd line\nFourth line\nFifth line\n"
+ + "Sixth line\nSeventh line\nEighth line\nNinth line\nTenth line\n");
+ }
+
+ @Test
+ public void applyProvidedFixWithTwoFilesOnNewerPatchsetWithModifiedFile() throws Exception {
+ // Remember patch set and add another one.
+ int previousRevision = gApi.changes().id(changeId).get().currentRevisionNumber;
+ // Remove first 2 lines;
+ String modifiedContent =
+ FILE_CONTENT.substring(FILE_CONTENT.indexOf("\n", FILE_CONTENT.indexOf("\n") + 1) + 1);
+ amendChange(
+ changeId,
+ "refs/for/master",
+ admin,
+ testRepo,
+ PushOneCommit.SUBJECT,
+ FILE_NAME,
+ modifiedContent);
+ amendChange(
+ changeId,
+ "refs/for/master",
+ admin,
+ testRepo,
+ PushOneCommit.SUBJECT,
+ FILE_NAME2,
+ "New line at the start\n" + FILE_CONTENT2);
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(10, 0, 10, 0);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME2;
+ fixReplacementInfo2.range = createRange(1, 0, 2, 0);
+ fixReplacementInfo2.replacement = "Different file modification\n";
+
+ ApplyProvidedFixInput applyProvidedFixInput = new ApplyProvidedFixInput();
+
+ applyProvidedFixInput.fixReplacementInfos =
+ Arrays.asList(fixReplacementInfo1, fixReplacementInfo2);
+ applyProvidedFixInput.originalPatchsetForFix = previousRevision;
+
+ gApi.changes().id(changeId).current().applyFix(applyProvidedFixInput);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "Third line\nFourth line\nFifth line\n"
+ + "Sixth line\nSeventh line\nEighth line\nNinth line\nFirst modification\nTenth line\n");
+ Optional<BinaryResult> file2 = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
+ BinaryResultSubject.assertThat(file2)
+ .value()
+ .asString()
+ .isEqualTo("New line at the start\nDifferent file modification\n2nd line\n3rd line\n");
+ }
+
+ @Test
+ public void applyProvidedFixOnCommitMessageCanBeAppliedToNewerPatchset() throws Exception {
+ // Set a dedicated commit message.
+ String footer = "\nChange-Id: " + changeId + "\n";
+ String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n" + footer;
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+ int previousRevision = gApi.changes().id(changeId).get().currentRevisionNumber;
+ // Upload a new patchset with the same commit message.
+ amendChange(changeId, originalCommitMessage, FILE_NAME, "a" + FILE_CONTENT);
+ ApplyProvidedFixInput applyProvidedFixInput =
+ createApplyProvidedFixInput(Patch.COMMIT_MSG, "Modified line\n", 7, 0, 8, 0);
+ applyProvidedFixInput.originalPatchsetForFix = previousRevision;
+
+ gApi.changes().id(changeId).current().applyFix(applyProvidedFixInput);
+ String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+ assertThat(commitMessage).isEqualTo("Modified line\nLine 2 of commit message\n" + footer);
+ }
+
+ @Test
+ public void applyProvidedFixOnCommitMessageRejectedIfNewerPatchsetHasDifferentCommitMessage()
+ throws Exception {
+ // Set a dedicated commit message.
+ String footer = "\nChange-Id: " + changeId + "\n";
+ String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n" + footer;
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+ int previousRevision = gApi.changes().id(changeId).get().currentRevisionNumber;
+ // Upload a new patchset with the same commit message.
+ amendChange(changeId, "a" + originalCommitMessage, FILE_NAME, "a" + FILE_CONTENT);
+ ApplyProvidedFixInput applyProvidedFixInput =
+ createApplyProvidedFixInput(Patch.COMMIT_MSG, "Modified line\n", 7, 0, 8, 0);
+ applyProvidedFixInput.originalPatchsetForFix = previousRevision;
+
+ ResourceConflictException thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(applyProvidedFixInput));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("commit message has been updated in a newer patchset");
+ }
+
+ @Test
public void applyProvidedFixOnCurrentPatchSetWithChangeEditOnPreviousPatchSetCannotBeApplied()
throws Exception {
// Create an empty change edit.
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/PreviewProvidedFixIT.java b/javatests/com/google/gerrit/acceptance/api/revision/PreviewProvidedFixIT.java
index c257e703..c5dba34 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/PreviewProvidedFixIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/PreviewProvidedFixIT.java
@@ -29,6 +29,7 @@
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.common.DiffInfo.IntraLineStatus;
import com.google.gerrit.extensions.common.FixReplacementInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import java.util.Arrays;
import java.util.List;
@@ -169,6 +170,19 @@
}
@Test
+ public void previewFixForDifferentPatchset() throws Exception {
+ int previousRevision = gApi.changes().id(changeId).get().currentRevisionNumber;
+ amendChange(changeId);
+ ApplyProvidedFixInput applyProvidedFixInput =
+ createApplyProvidedFixInput(FILE_NAME, "Modified content\n", 1, 0, 2, 0);
+ applyProvidedFixInput.originalPatchsetForFix = previousRevision;
+
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.changes().id(changeId).current().getFixPreview(applyProvidedFixInput));
+ }
+
+ @Test
public void previewFixForCommitMsg() throws Exception {
String footer = "Change-Id: " + changeId;
updateCommitMessage(
