commit 368547f70fb8cd211cc7f995a802dbe077e25cba Author: Alice Kober-Sotzek Date: Fri Mar 13 22:00:41 2020 +0100 Support 'Apply fix' for the commit message When we introduced suggested fixes, we only intended and implemented them for actual files. Magic files such as /COMMIT_MSG, which represents the commit message, were not supported. However, we didn't add any validation which rejected the use of magic files for suggested fixes. As a consequence, people wrote bots which did suggest fixes for the /COMMIT_MSG file, in the good belief that they could thus adjust the commit message. Users later reported Gerrit bugs that they couldn't apply such fixes (getting an "Error 404: Not Found" for the missing /COMMIT_MSG file). This change finally adds the functionality to apply fixes for the commit message. Such fixes may additionally modify real files. There is one caveat, though. Several different fixes modifying the commit message can't be applied one after the other. In addition, if a user already modified the commit message in an existing change edit, a fix modifying the commit message can't be applied. Both situations require an actual merge of the text of the commit message. Git doesn't support this out of the box. We'll add this special functionality in the follow-up change (I6c76ff3c6). As the /COMMIT_MSG file contains some unmodifiable header lines before the actual commit message, we need to take those header lines into account when we apply the fix. Depending on the type of commit (merge vs. non-merge), the number of header lines varies. Hence, we needed to extract the code which generates the content of Gerrit's magic files in I5f8b93673 and can now determine the number of header lines on the fly. Bug: Issue 13266 Change-Id: I9658c0768d55347daab4306cb1e4b59204893c48 diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index f25e27d..adb5466 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -7123,7 +7123,10 @@ replaced by another content. |========================== |Field Name |Description |`path` |The path of the file which should be modified. Any file in -the repository may be modified. +the repository may be modified. The commit message can be modified via the +magic file `/COMMIT_MSG` though only the part below the generated header of +that magic file can be modified. References to the header lines will result in +errors when the fix is applied. |`range` |A <> indicating which content of the file should be replaced. Lines in the file are assumed to be separated by the line feed character, the carriage return character, the carriage return diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 80846d9..f8f02c6 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -16,7 +16,6 @@ package com.google.gerrit.server.edit; import static com.google.gerrit.server.project.ProjectCache.illegalState; -import com.google.common.collect.ImmutableList; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; @@ -324,7 +323,7 @@ public class ChangeEditModifier { * @param repository the affected Git repository * @param notes the {@link ChangeNotes} of the change to which the patch set belongs * @param patchSet the {@code PatchSet} which should be modified - * @param treeModifications the modifications which should be applied + * @param commitModification the modifications which should be applied * @return the resulting {@code ChangeEdit} * @throws AuthException if the user isn't authenticated or not allowed to use change edits * @throws InvalidChangeOperationException if the existing change edit is based on another patch @@ -336,16 +335,11 @@ public class ChangeEditModifier { Repository repository, ChangeNotes notes, PatchSet patchSet, - List treeModifications) + CommitModification commitModification) throws AuthException, BadRequestException, IOException, InvalidChangeOperationException, PermissionBackendException, ResourceConflictException { return modifyCommit( - repository, - notes, - new ModificationIntention.PatchsetCommit(patchSet), - CommitModification.builder() - .treeModifications(ImmutableList.copyOf(treeModifications)) - .build()); + repository, notes, new ModificationIntention.PatchsetCommit(patchSet), commitModification); } private ChangeEdit modifyCommit( diff --git a/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java b/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java index 72a5176..cce289a 100644 --- a/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java +++ b/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java @@ -14,26 +14,33 @@ package com.google.gerrit.server.fixes; +import static com.google.common.collect.ImmutableList.toImmutableList; import static java.util.Objects.requireNonNull; import static java.util.stream.Collectors.groupingBy; +import com.google.common.collect.ImmutableList; import com.google.gerrit.common.RawInputUtil; +import com.google.gerrit.entities.Comment.Range; import com.google.gerrit.entities.FixReplacement; +import com.google.gerrit.entities.Patch; 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.server.change.FileContentUtil; +import com.google.gerrit.server.edit.CommitModification; import com.google.gerrit.server.edit.tree.ChangeFileContentModification; import com.google.gerrit.server.edit.tree.TreeModification; +import com.google.gerrit.server.patch.MagicFile; import com.google.gerrit.server.project.ProjectState; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; -import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Objects; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Repository; /** An interpreter for {@code FixReplacement}s. */ @@ -60,7 +67,7 @@ public class FixReplacementInterpreter { * @throws ResourceConflictException if the replacements can't be transformed into {@code * TreeModification}s */ - public List toTreeModifications( + public CommitModification toCommitModification( Repository repository, ProjectState projectState, ObjectId patchSetCommitId, @@ -72,14 +79,63 @@ public class FixReplacementInterpreter { Map> fixReplacementsPerFilePath = fixReplacements.stream().collect(groupingBy(fixReplacement -> fixReplacement.path)); - List treeModifications = new ArrayList<>(); + CommitModification.Builder modificationBuilder = CommitModification.builder(); for (Map.Entry> entry : fixReplacementsPerFilePath.entrySet()) { - TreeModification treeModification = - toTreeModification( - repository, projectState, patchSetCommitId, entry.getKey(), entry.getValue()); - treeModifications.add(treeModification); + if (Objects.equals(entry.getKey(), Patch.COMMIT_MSG)) { + String newCommitMessage = + getNewCommitMessage(repository, patchSetCommitId, entry.getValue()); + modificationBuilder.newCommitMessage(newCommitMessage); + } else { + TreeModification treeModification = + toTreeModification( + repository, projectState, patchSetCommitId, entry.getKey(), entry.getValue()); + modificationBuilder.addTreeModification(treeModification); + } } - return treeModifications; + return modificationBuilder.build(); + } + + private static String getNewCommitMessage( + Repository repository, ObjectId patchSetCommitId, List fixReplacements) + throws ResourceConflictException, IOException { + try (ObjectReader reader = repository.newObjectReader()) { + // In the magic /COMMIT_MSG file, the actual commit message is placed after some generated + // header lines. -> Need to find out to which actual line of the commit message a replacement + // refers. + MagicFile commitMessageFile = MagicFile.forCommitMessage(reader, patchSetCommitId); + int commitMessageStartLine = commitMessageFile.getStartLineOfModifiableContent(); + // Line numbers are 1-based. -> Add 1 to not move first line. + // Move up for any additionally found lines. + int necessaryRangeShift = -commitMessageStartLine + 1; + ImmutableList adjustedReplacements = + shiftRangesBy(fixReplacements, necessaryRangeShift); + if (referToNonPositiveLine(adjustedReplacements)) { + throw new ResourceConflictException( + String.format("The header of the %s file cannot be modified.", Patch.COMMIT_MSG)); + } + String commitMessage = commitMessageFile.modifiableContent(); + return FixCalculator.getNewFileContent(commitMessage, adjustedReplacements); + } + } + + private static ImmutableList shiftRangesBy( + List fixReplacements, int shiftedAmount) { + return fixReplacements.stream() + .map(replacement -> shiftRangesBy(replacement, shiftedAmount)) + .collect(toImmutableList()); + } + + private static FixReplacement shiftRangesBy(FixReplacement fixReplacement, int shiftedAmount) { + Range adjustedRange = new Range(fixReplacement.range); + adjustedRange.startLine += shiftedAmount; + adjustedRange.endLine += shiftedAmount; + return new FixReplacement(fixReplacement.path, adjustedRange, fixReplacement.replacement); + } + + private static boolean referToNonPositiveLine(List adjustedReplacements) { + return adjustedReplacements.stream() + .map(replacement -> replacement.range) + .anyMatch(range -> range.startLine <= 0); } private TreeModification toTreeModification( diff --git a/java/com/google/gerrit/server/restapi/change/ApplyFix.java b/java/com/google/gerrit/server/restapi/change/ApplyFix.java index 40e4fc2..9224154 100644 --- a/java/com/google/gerrit/server/restapi/change/ApplyFix.java +++ b/java/com/google/gerrit/server/restapi/change/ApplyFix.java @@ -30,7 +30,7 @@ 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.tree.TreeModification; +import com.google.gerrit.server.edit.CommitModification; import com.google.gerrit.server.fixes.FixReplacementInterpreter; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -40,7 +40,6 @@ import com.google.gerrit.server.project.ProjectState; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; -import java.util.List; import org.eclipse.jgit.lib.Repository; @Singleton @@ -76,12 +75,12 @@ public class ApplyFix implements RestModifyView { PatchSet patchSet = revisionResource.getPatchSet(); try (Repository repository = gitRepositoryManager.openRepository(project)) { - List treeModifications = - fixReplacementInterpreter.toTreeModifications( + CommitModification commitModification = + fixReplacementInterpreter.toCommitModification( repository, projectState, patchSet.commitId(), fixResource.getFixReplacements()); ChangeEdit changeEdit = changeEditModifier.combineWithModifiedPatchSetTree( - repository, revisionResource.getNotes(), patchSet, treeModifications); + repository, revisionResource.getNotes(), patchSet, commitModification); return Response.ok(changeEditJson.toEditInfo(changeEdit, false)); } catch (InvalidChangeOperationException e) { throw new ResourceConflictException(e.getMessage()); diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java index 1ad952c..b9b5f29 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java @@ -1054,10 +1054,7 @@ public class RobotCommentsIT extends AbstractDaemonTest { fixReplacementInfo.range = createRange(3, 1, 3, 3); testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); - List robotCommentInfos = getRobotComments(); - - List fixIds = getFixIds(robotCommentInfos); - String fixId = Iterables.getOnlyElement(fixIds); + String fixId = Iterables.getOnlyElement(getFixIds(getRobotComments())); gApi.changes().id(changeId).current().applyFix(fixId); @@ -1066,6 +1063,117 @@ public class RobotCommentsIT extends AbstractDaemonTest { } @Test + public void fixOnCommitMessageCanBeApplied() throws Exception { + // Set a dedicated commit message. + String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n"; + gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage); + gApi.changes().id(changeId).edit().publish(); + + withFixRobotCommentInput.path = Patch.COMMIT_MSG; + fixReplacementInfo.path = Patch.COMMIT_MSG; + fixReplacementInfo.replacement = "Modified line\n"; + fixReplacementInfo.range = createRange(7, 0, 8, 0); + + testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); + String fixId = Iterables.getOnlyElement(getFixIds(getRobotComments())); + + gApi.changes().id(changeId).current().applyFix(fixId); + + String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage(); + assertThat(commitMessage).isEqualTo("Modified line\nLine 2 of commit message\n"); + } + + @Test + public void fixOnHeaderPartOfCommitMessageCannotBeApplied() throws Exception { + // Set a dedicated commit message. + String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n"; + gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage); + gApi.changes().id(changeId).edit().publish(); + + withFixRobotCommentInput.path = Patch.COMMIT_MSG; + fixReplacementInfo.path = Patch.COMMIT_MSG; + fixReplacementInfo.replacement = "Modified line\n"; + fixReplacementInfo.range = createRange(1, 0, 2, 0); + + testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); + String fixId = Iterables.getOnlyElement(getFixIds(getRobotComments())); + + ResourceConflictException exception = + assertThrows( + ResourceConflictException.class, + () -> gApi.changes().id(changeId).current().applyFix(fixId)); + assertThat(exception).hasMessageThat().contains("header"); + } + + @Test + public void fixContainingSeveralModificationsOfCommitMessageCanBeApplied() throws Exception { + // Set a dedicated commit message. + String originalCommitMessage = + "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n"; + gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage); + gApi.changes().id(changeId).edit().publish(); + + FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo(); + fixReplacementInfo1.path = Patch.COMMIT_MSG; + fixReplacementInfo1.range = createRange(7, 0, 8, 0); + fixReplacementInfo1.replacement = "Modified line 1\n"; + + FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo(); + fixReplacementInfo2.path = Patch.COMMIT_MSG; + fixReplacementInfo2.range = createRange(9, 0, 10, 0); + fixReplacementInfo2.replacement = "Modified line 3\n"; + + FixSuggestionInfo fixSuggestionInfo = + createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2); + withFixRobotCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo); + withFixRobotCommentInput.path = Patch.COMMIT_MSG; + + testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); + String fixId = Iterables.getOnlyElement(getFixIds(getRobotComments())); + + gApi.changes().id(changeId).current().applyFix(fixId); + + String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage(); + assertThat(commitMessage) + .isEqualTo("Modified line 1\nLine 2 of commit message\nModified line 3\n"); + } + + @Test + public void fixModifyingTheCommitMessageAndAFileCanBeApplied() throws Exception { + // Set a dedicated commit message. + String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n"; + gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage); + gApi.changes().id(changeId).edit().publish(); + + FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo(); + fixReplacementInfo1.path = Patch.COMMIT_MSG; + fixReplacementInfo1.range = createRange(7, 0, 8, 0); + fixReplacementInfo1.replacement = "Modified line 1\n"; + + FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo(); + fixReplacementInfo2.path = FILE_NAME2; + fixReplacementInfo2.range = createRange(1, 0, 2, 0); + fixReplacementInfo2.replacement = "File modification\n"; + + FixSuggestionInfo fixSuggestionInfo = + createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2); + withFixRobotCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo); + + testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); + String fixId = Iterables.getOnlyElement(getFixIds(getRobotComments())); + + gApi.changes().id(changeId).current().applyFix(fixId); + + String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage(); + assertThat(commitMessage).isEqualTo("Modified line 1\nLine 2 of commit message\n"); + Optional file = gApi.changes().id(changeId).edit().getFile(FILE_NAME2); + BinaryResultSubject.assertThat(file) + .value() + .asString() + .isEqualTo("File modification\n2nd line\n3rd line\n"); + } + + @Test public void applyingFixTwiceIsIdempotent() throws Exception { fixReplacementInfo.path = FILE_NAME; fixReplacementInfo.replacement = "Modified content"; diff --git a/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java b/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java index e5ef6af..a3e86a3 100644 --- a/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java +++ b/javatests/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.fixes; import static com.google.gerrit.server.edit.tree.TreeModificationSubject.assertThatList; +import static com.google.gerrit.truth.OptionalSubject.assertThat; import static java.util.Comparator.comparing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -24,6 +25,7 @@ import com.google.gerrit.entities.Comment.Range; import com.google.gerrit.entities.FixReplacement; import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.server.change.FileContentUtil; +import com.google.gerrit.server.edit.CommitModification; import com.google.gerrit.server.edit.tree.TreeModification; import com.google.gerrit.server.project.ProjectState; import java.util.ArrayList; @@ -50,8 +52,9 @@ public class FixReplacementInterpreterTest { @Test public void noReplacementsResultInNoTreeModifications() throws Exception { - List treeModifications = toTreeModifications(); - assertThatList(treeModifications).isEmpty(); + CommitModification commitModification = toCommitModification(); + assertThatList(commitModification.treeModifications()).isEmpty(); + assertThat(commitModification.newCommitMessage()).isEmpty(); } @Test @@ -60,7 +63,8 @@ public class FixReplacementInterpreterTest { new FixReplacement(filePath1, new Range(1, 1, 3, 2), "Modified content"); mockFileContent(filePath1, "First line\nSecond line\nThird line\n"); - List treeModifications = toTreeModifications(fixReplacement); + CommitModification commitModification = toCommitModification(fixReplacement); + ImmutableList treeModifications = commitModification.treeModifications(); assertThatList(treeModifications) .onlyElement() .asChangeFileContentModification() @@ -84,9 +88,10 @@ public class FixReplacementInterpreterTest { new FixReplacement(filePath2, new Range(2, 0, 3, 0), "Another modified content"); mockFileContent(filePath2, "1st line\n2nd line\n3rd line\n"); - List treeModifications = - toTreeModifications(fixReplacement, fixReplacement3, fixReplacement2); - List sortedTreeModifications = getSortedCopy(treeModifications); + CommitModification commitModification = + toCommitModification(fixReplacement, fixReplacement3, fixReplacement2); + List sortedTreeModifications = + getSortedCopy(commitModification.treeModifications()); assertThatList(sortedTreeModifications) .element(0) .asChangeFileContentModification() @@ -120,9 +125,10 @@ public class FixReplacementInterpreterTest { new FixReplacement(filePath2, new Range(3, 0, 4, 0), "Second modification\n"); mockFileContent(filePath2, "1st line\n2nd line\n3rd line\n"); - List treeModifications = - toTreeModifications(fixReplacement3, fixReplacement1, fixReplacement2); - List sortedTreeModifications = getSortedCopy(treeModifications); + CommitModification commitModification = + toCommitModification(fixReplacement3, fixReplacement1, fixReplacement2); + List sortedTreeModifications = + getSortedCopy(commitModification.treeModifications()); assertThatList(sortedTreeModifications) .element(0) .asChangeFileContentModification() @@ -140,9 +146,9 @@ public class FixReplacementInterpreterTest { .thenReturn(BinaryResult.create(fileContent)); } - private List toTreeModifications(FixReplacement... fixReplacements) + private CommitModification toCommitModification(FixReplacement... fixReplacements) throws Exception { - return fixReplacementInterpreter.toTreeModifications( + return fixReplacementInterpreter.toCommitModification( repository, projectState, patchSetCommitId, ImmutableList.copyOf(fixReplacements)); }