commit 9f1b0ea0392f5b706010e9b60830a4a7dac6c437 Author: Alice Kober-Sotzek Date: Thu Sep 24 11:45:04 2020 +0200 Allow to use 'Apply fix' several times on the commit message The previous change (I9658c0768) only added support for applying a fix for a previously unmodified commit message. That means that only one fix suggestion could be applied at all. In addition, if a user had already created a change edit and had slightly adjusted the commit message, no fix could be applied. This change adds the functionality to merge two commit messages. Thus several fixes which modify the commit message can be applied one after the other as long as they don't conflict with each other (= are at least one unmodified line apart from each other). The same holds if a user had already modified the commit message of an existing change edit. A fix containing several replacements touching the commit message doesn't have this restriction. For such a fix, the replacements only should not overlap but may even happen on the same line. If we ever add a functionality to apply several fixes together, we can avoid the need for merges and hence could recommend to users to use that batch approach when they have two fixes on the commit message which refer to the same line. Unfortunately, we don't have that functionality yet. Even though JGit doesn't have any built-in mechanism for merging commit messages, we can still use JGit's merge functionality for strings. Thus, we at least don't have to implement a merge algorithm ourselves. It puts us under the same restrictions as Git merges, though: Only modifications which have at least one unmodified separation line between them can be merged. That's the best we can do with JGit at the moment. Bug: Issue 13266 Change-Id: I6c76ff3c67ec9b8c6d94ec5e809b9022454c529d diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index f8f02c6..9d14a58 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.edit; import static com.google.gerrit.server.project.ProjectCache.illegalState; +import com.google.common.base.Charsets; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; @@ -55,6 +56,10 @@ import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.TimeZone; +import org.eclipse.jgit.diff.DiffAlgorithm; +import org.eclipse.jgit.diff.DiffAlgorithm.SupportedAlgorithm; +import org.eclipse.jgit.diff.RawText; +import org.eclipse.jgit.diff.RawTextComparator; import org.eclipse.jgit.dircache.InvalidPathException; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; @@ -64,6 +69,9 @@ import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.merge.MergeAlgorithm; +import org.eclipse.jgit.merge.MergeChunk; +import org.eclipse.jgit.merge.MergeResult; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ThreeWayMerger; import org.eclipse.jgit.revwalk.RevCommit; @@ -523,7 +531,8 @@ public class ChangeEditModifier { String getUnmodifiedCommitMessage(RevCommit commitToModify); - String mergeCommitMessageIfNecessary(String newCommitMessage, ObjectId commitToModify); + String mergeCommitMessageIfNecessary(String newCommitMessage, RevCommit commitToModify) + throws MergeConflictException; Optional getEditIfNoModification(ObjectId newTreeId, String newCommitMessage); @@ -573,7 +582,8 @@ public class ChangeEditModifier { } @Override - public String mergeCommitMessageIfNecessary(String newCommitMessage, ObjectId commitToModify) { + public String mergeCommitMessageIfNecessary(String newCommitMessage, RevCommit commitToModify) + throws MergeConflictException { if (ObjectId.isEqual(changeEdit.getEditCommit(), commitToModify)) { return newCommitMessage; } @@ -581,9 +591,35 @@ public class ChangeEditModifier { if (editCommitMessage.equals(newCommitMessage)) { return editCommitMessage; } - // Adjusted in follow-up change. - throw new UnsupportedOperationException( - "Merging different commit messages is not supported."); + return mergeCommitMessage(newCommitMessage, commitToModify, editCommitMessage); + } + + private String mergeCommitMessage( + String newCommitMessage, RevCommit commitToModify, String editCommitMessage) + throws MergeConflictException { + MergeAlgorithm mergeAlgorithm = + new MergeAlgorithm(DiffAlgorithm.getAlgorithm(SupportedAlgorithm.MYERS)); + RawText baseMessage = new RawText(commitToModify.getFullMessage().getBytes(Charsets.UTF_8)); + RawText oldMessage = new RawText(editCommitMessage.getBytes(Charsets.UTF_8)); + RawText newMessage = new RawText(newCommitMessage.getBytes(Charsets.UTF_8)); + RawTextComparator textComparator = RawTextComparator.DEFAULT; + MergeResult mergeResult = + mergeAlgorithm.merge(textComparator, baseMessage, oldMessage, newMessage); + if (mergeResult.containsConflicts()) { + throw new MergeConflictException( + "The chosen modification adjusted the commit message. However, the new commit message" + + " could not be merged with the commit message of the existing change edit." + + " Please manually apply the desired changes to the commit message of the change" + + " edit."); + } + + StringBuilder resultingCommitMessage = new StringBuilder(); + for (MergeChunk mergeChunk : mergeResult) { + RawText mergedMessagePart = mergeResult.getSequences().get(mergeChunk.getSequenceIndex()); + resultingCommitMessage.append( + mergedMessagePart.getString(mergeChunk.getBegin(), mergeChunk.getEnd(), false)); + } + return resultingCommitMessage.toString(); } @Override @@ -643,7 +679,7 @@ public class ChangeEditModifier { } @Override - public String mergeCommitMessageIfNecessary(String newCommitMessage, ObjectId commitToModify) { + public String mergeCommitMessageIfNecessary(String newCommitMessage, RevCommit commitToModify) { return newCommitMessage; } diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java index b9b5f29..b855485 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java @@ -1174,6 +1174,76 @@ public class RobotCommentsIT extends AbstractDaemonTest { } @Test + public void twoFixesOnCommitMessageCanBeAppliedOneAfterTheOther() 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"; + FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1); + + FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo(); + fixReplacementInfo2.path = Patch.COMMIT_MSG; + fixReplacementInfo2.range = createRange(9, 0, 10, 0); + fixReplacementInfo2.replacement = "Modified line 3\n"; + FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2); + + RobotCommentInput robotCommentInput1 = + TestCommentHelper.createRobotCommentInput(FILE_NAME, fixSuggestionInfo1); + RobotCommentInput robotCommentInput2 = + TestCommentHelper.createRobotCommentInput(FILE_NAME, fixSuggestionInfo2); + testCommentHelper.addRobotComment(changeId, robotCommentInput1); + testCommentHelper.addRobotComment(changeId, robotCommentInput2); + List fixIds = getFixIds(getRobotComments()); + + gApi.changes().id(changeId).current().applyFix(fixIds.get(0)); + gApi.changes().id(changeId).current().applyFix(fixIds.get(1)); + + 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 twoConflictingFixesOnCommitMessageCanNotBeAppliedOneAfterTheOther() 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"; + FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1); + + FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo(); + fixReplacementInfo2.path = Patch.COMMIT_MSG; + fixReplacementInfo2.range = createRange(7, 0, 10, 0); + fixReplacementInfo2.replacement = "Differently modified line 1\n"; + FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2); + + RobotCommentInput robotCommentInput1 = + TestCommentHelper.createRobotCommentInput(FILE_NAME, fixSuggestionInfo1); + RobotCommentInput robotCommentInput2 = + TestCommentHelper.createRobotCommentInput(FILE_NAME, fixSuggestionInfo2); + testCommentHelper.addRobotComment(changeId, robotCommentInput1); + testCommentHelper.addRobotComment(changeId, robotCommentInput2); + List fixIds = getFixIds(getRobotComments()); + + gApi.changes().id(changeId).current().applyFix(fixIds.get(0)); + assertThrows( + ResourceConflictException.class, + () -> gApi.changes().id(changeId).current().applyFix(fixIds.get(1))); + } + + @Test public void applyingFixTwiceIsIdempotent() throws Exception { fixReplacementInfo.path = FILE_NAME; fixReplacementInfo.replacement = "Modified content";