Return ResourceConflict error when suggested fix can't be applied.
When a patch can't be applied, the ApplyPatchUtil.applyPatch returns
a result with some files missed in the new tree (e.g. this happens
when there is a merge conflict between patch and the target commit).
It caused internal server error when ApplyFix api is called, but the fix
can't be applied.
To fix this problem, this change checks errors returned by applyPatch
and throws ResourceConflictException if the patch can't be applied. To
make a error message more clear, it allows merge conflicts when applying
patch - as a result, the Error.isGitConflict is set if there is a merge
conflict (without setting allowConflicts to true Error.isGitConflict is
always false).
Google-Bug-Id: b/378863055
Release-Notes: skip
Change-Id: I6b8190d02af6c3d33fce8513c02da43830fe0d89
(cherry picked from commit b8a41d0b8850b290a6008532bfa746efb47dc8b9)
diff --git a/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java b/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java
index 4e05420..be1bced 100644
--- a/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyProvidedFix.java
@@ -56,6 +56,7 @@
import org.eclipse.jgit.lib.ObjectLoader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.patch.PatchApplier;
+import org.eclipse.jgit.patch.PatchApplier.Result.Error;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.treewalk.TreeWalk;
@@ -86,8 +87,13 @@
@Override
public Response<EditInfo> apply(
RevisionResource revisionResource, ApplyProvidedFixInput applyProvidedFixInput)
- throws AuthException, BadRequestException, ResourceConflictException, IOException,
- ResourceNotFoundException, PermissionBackendException, RestApiException {
+ throws AuthException,
+ BadRequestException,
+ ResourceConflictException,
+ IOException,
+ ResourceNotFoundException,
+ PermissionBackendException,
+ RestApiException {
if (applyProvidedFixInput == null) {
throw new BadRequestException("applyProvidedFixInput is required");
}
@@ -173,7 +179,8 @@
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.");
+ "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());
}
@@ -185,9 +192,20 @@
try (ObjectInserter oi = repository.newObjectInserter()) {
ApplyPatchInput inp = new ApplyPatchInput();
inp.patch = patch;
- inp.allowConflicts = false;
+ // Allow conflicts for showing more precise error message.
+ inp.allowConflicts = true;
result =
ApplyPatchUtil.applyPatch(repository, oi, inp, repository.parseCommit(targetCommit));
+ if (!result.getErrors().isEmpty()) {
+ String errorMessage =
+ (result.getErrors().stream().anyMatch(Error::isGitConflict)
+ ? "Merge conflict while applying a fix:\n"
+ : "Error while applying a fix:\n")
+ + result.getErrors().stream()
+ .map(Error::toString)
+ .collect(Collectors.joining("\n"));
+ throw new ResourceConflictException(errorMessage);
+ }
oi.flush();
for (String path : result.getPaths()) {
try (TreeWalk tw = TreeWalk.forPath(rw.getObjectReader(), path, result.getTreeId())) {
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java b/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java
index d66a0a5..effd96f 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/ApplyProvidedFixIT.java
@@ -378,8 +378,17 @@
.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");
+ "New line at the start\n"
+ + "First line\n"
+ + "Second line\n"
+ + "TModified contentrd line\n"
+ + "Fourth line\n"
+ + "Fifth line\n"
+ + "Sixth line\n"
+ + "Seventh line\n"
+ + "Eighth line\n"
+ + "Ninth line\n"
+ + "Tenth line\n");
applyProvidedFixInput = createApplyProvidedFixInput(FILE_NAME, "(1st)", 1, 5, 1, 5);
applyProvidedFixInput.originalPatchsetForFix = previousRevision;
@@ -390,8 +399,17 @@
.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");
+ "New line at the start\n"
+ + "First(1st) line\n"
+ + "Second line\n"
+ + "TModified contentrd line\n"
+ + "Fourth line\n"
+ + "Fifth line\n"
+ + "Sixth line\n"
+ + "Seventh line\n"
+ + "Eighth line\n"
+ + "Ninth line\n"
+ + "Tenth line\n");
}
@Test
@@ -440,8 +458,15 @@
.value()
.asString()
.isEqualTo(
- "Third line\nFourth line\nFifth line\n"
- + "Sixth line\nSeventh line\nEighth line\nNinth line\nFirst modification\nTenth line\n");
+ "Third line\n"
+ + "Fourth line\n"
+ + "Fifth line\n"
+ + "Sixth line\n"
+ + "Seventh line\n"
+ + "Eighth line\n"
+ + "Ninth line\n"
+ + "First modification\n"
+ + "Tenth line\n");
Optional<BinaryResult> file2 = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
BinaryResultSubject.assertThat(file2)
.value()
@@ -450,6 +475,44 @@
}
@Test
+ public void applyProvidedFixOnNewerPatchset_mergeConflictThrowsResourceConflictException()
+ throws Exception {
+ // Remember patch set and add another one.
+ int previousRevision = gApi.changes().id(changeId).get().currentRevisionNumber;
+ // Change first 2 lines;
+ String modifiedContent =
+ "abc\ndef\n"
+ + 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);
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 2, 0);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ ApplyProvidedFixInput applyProvidedFixInput = new ApplyProvidedFixInput();
+
+ applyProvidedFixInput.fixReplacementInfos = Arrays.asList(fixReplacementInfo1);
+ applyProvidedFixInput.originalPatchsetForFix = previousRevision;
+
+ ResourceConflictException thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(applyProvidedFixInput));
+
+ assertThat(thrown.getMessage()).contains("Merge conflict");
+ // Change edit must not be created
+ assertThat(gApi.changes().id(changeId).edit().get()).isEmpty();
+ }
+
+ @Test
public void applyProvidedFixOnCommitMessageCanBeAppliedToNewerPatchset() throws Exception {
// Set a dedicated commit message.
String footer = "\nChange-Id: " + changeId + "\n";
