opendev/gerrit
Code Issues Proposed changes

Allow fixes to refer to several files

Browse Source 
Suggested fixes may now also refer to other files than the one on which
the robot comment was placed. In addition, a fix may modify several
files.

Whether a file exists or not is only verified when a fix is applied.
The check isn't done when a robot comment is added in order to not
slow down the operation.

Change-Id: Id26232019b42dfe8286c9b0696f00b8a376482b0
This commit is contained in:
Alice Kober-Sotzek
2017-03-16 18:02:15 +01:00
parent a28db80825
commit 791f4af42c
15 changed files with 367 additions and 144 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
Documentation/rest-api-changes.txt
View File

@@ -5737,8 +5737,8 @@ fix. It will be generated automatically and hence will be ignored if it's set
for input objects.
|`description` ||A description of the suggested fix.
|`replacements` ||A list of <<fix-replacement-info,FixReplacementInfo>>
entities indicating how the content of the file on which the comment was placed
should be modified. They should refer to non-overlapping regions.
entities indicating how the content of one or several files should be modified.
Within a file, they should refer to non-overlapping regions.
|==========================
[[fix-replacement-info]]
@@ -5749,8 +5749,8 @@ replaced by another content.
[options="header",cols="1,6"]
|==========================
|Field Name |Description
|`path` |The path of the file which should be modified. Modifications
are only allowed for the file on which the corresponding comment was placed.
|`path` |The path of the file which should be modified. Any file in
the repository may be modified.
|`range` |A <<comment-range,CommentRange>> 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

137
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
View File

@@ -22,6 +22,7 @@ import static com.google.gerrit.extensions.common.RobotCommentInfoSubject.assert
import static java.util.stream.Collectors.toList;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
@@ -53,9 +54,11 @@ import org.junit.Test;
public class RobotCommentsIT extends AbstractDaemonTest {
private static final String FILE_NAME = "file_to_fix.txt";
private static final String FILE_NAME2 = "another_file_to_fix.txt";
private static final String FILE_CONTENT =
"First line\nSecond line\nThird line\nFourth line\nFifth line\nSixth line"
+ "\nSeventh line\nEighth line\nNinth line\nTenth line\n";
private static final String FILE_CONTENT2 = "1st line\n2nd line\n3rd line\n";
private String changeId;
private FixReplacementInfo fixReplacementInfo;
@@ -64,8 +67,14 @@ public class RobotCommentsIT extends AbstractDaemonTest {
@Before
public void setUp() throws Exception {
PushOneCommit.Result changeResult =
createChange("Provide a file which can be used for fixes", FILE_NAME, FILE_CONTENT);
PushOneCommit push =
pushFactory.create(
db,
admin.getIdent(),
testRepo,
"Provide files which can be used for fixes",
ImmutableMap.of(FILE_NAME, FILE_CONTENT, FILE_NAME2, FILE_CONTENT2));
PushOneCommit.Result changeResult = push.to("refs/for/master");
changeId = changeResult.getChangeId();
fixReplacementInfo = createFixReplacementInfo();
@@ -276,21 +285,6 @@ public class RobotCommentsIT extends AbstractDaemonTest {
addRobotComment(changeId, withFixRobotCommentInput);
}
@Test
public void pathOfFixReplacementMustReferToFileOfComment() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
fixReplacementInfo.path = "anotherFile.txt";
exception.expect(BadRequestException.class);
exception.expectMessage(
String.format(
"Replacements may only be specified "
+ "for the file %s on which the robot comment was added",
withFixRobotCommentInput.path));
addRobotComment(changeId, withFixRobotCommentInput);
}
@Test
public void rangeOfFixReplacementIsAcceptedAsIs() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
@@ -332,7 +326,8 @@ public class RobotCommentsIT extends AbstractDaemonTest {
}
@Test
public void rangesOfFixReplacementsOfSameFixSuggestionMayNotOverlap() throws Exception {
public void rangesOfFixReplacementsOfSameFixSuggestionForSameFileMayNotOverlap()
throws Exception {
assume().that(notesMigration.enabled()).isTrue();
FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
@@ -355,7 +350,33 @@ public class RobotCommentsIT extends AbstractDaemonTest {
}
@Test
public void rangesOfFixReplacementsOfDifferentFixSuggestionsMayOverlap() throws Exception {
public void rangesOfFixReplacementsOfSameFixSuggestionForDifferentFileMayOverlap()
throws Exception {
assume().that(notesMigration.enabled()).isTrue();
FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
fixReplacementInfo1.path = FILE_NAME;
fixReplacementInfo1.range = createRange(2, 0, 3, 1);
fixReplacementInfo1.replacement = "First modification\n";
FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
fixReplacementInfo2.path = FILE_NAME2;
fixReplacementInfo2.range = createRange(3, 0, 4, 0);
fixReplacementInfo2.replacement = "Second modification\n";
FixSuggestionInfo fixSuggestionInfo =
createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
withFixRobotCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
addRobotComment(changeId, withFixRobotCommentInput);
List<RobotCommentInfo> robotCommentInfos = getRobotComments();
assertThatList(robotCommentInfos).onlyElement().fixSuggestions().hasSize(1);
}
@Test
public void rangesOfFixReplacementsOfDifferentFixSuggestionsForSameFileMayOverlap()
throws Exception {
assume().that(notesMigration.enabled()).isTrue();
FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
@@ -621,6 +642,84 @@ public class RobotCommentsIT extends AbstractDaemonTest {
+ "Seventh line\nSome other modified content\nNinth line\nTenth line\n");
}
@Test
public void fixReferringToDifferentFileThanRobotCommentCanBeApplied() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
fixReplacementInfo.path = FILE_NAME2;
fixReplacementInfo.range = createRange(2, 0, 3, 0);
fixReplacementInfo.replacement = "Modified content\n";
addRobotComment(changeId, withFixRobotCommentInput);
List<RobotCommentInfo> robotCommentInfos = getRobotComments();
List<String> fixIds = getFixIds(robotCommentInfos);
String fixId = Iterables.getOnlyElement(fixIds);
gApi.changes().id(changeId).current().applyFix(fixId);
Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
BinaryResultSubject.assertThat(file)
.value()
.asString()
.isEqualTo("1st line\nModified content\n3rd line\n");
}
@Test
public void fixInvolvingTwoFilesCanBeApplied() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
fixReplacementInfo1.path = FILE_NAME;
fixReplacementInfo1.range = createRange(2, 0, 3, 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";
FixSuggestionInfo fixSuggestionInfo =
createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
withFixRobotCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
addRobotComment(changeId, withFixRobotCommentInput);
List<RobotCommentInfo> robotCommentInfos = getRobotComments();
List<String> fixIds = getFixIds(robotCommentInfos);
String fixId = Iterables.getOnlyElement(fixIds);
gApi.changes().id(changeId).current().applyFix(fixId);
Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
BinaryResultSubject.assertThat(file)
.value()
.asString()
.isEqualTo(
"First line\nFirst modification\nThird line\nFourth line\nFifth line\nSixth line\n"
+ "Seventh line\nEighth line\nNinth line\nTenth line\n");
Optional<BinaryResult> file2 = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
BinaryResultSubject.assertThat(file2)
.value()
.asString()
.isEqualTo("Different file modification\n2nd line\n3rd line\n");
}
@Test
public void fixReferringToNonExistentFileCannotBeApplied() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
fixReplacementInfo.path = "a_non_existent_file.txt";
fixReplacementInfo.range = createRange(1, 0, 2, 0);
fixReplacementInfo.replacement = "Modified content\n";
addRobotComment(changeId, withFixRobotCommentInput);
List<RobotCommentInfo> robotCommentInfos = getRobotComments();
List<String> fixIds = getFixIds(robotCommentInfos);
String fixId = Iterables.getOnlyElement(fixIds);
exception.expect(ResourceNotFoundException.class);
gApi.changes().id(changeId).current().applyFix(fixId);
}
@Test
public void fixOnPreviousPatchSetWithoutChangeEditCannotBeApplied() throws Exception {
assume().that(notesMigration.enabled()).isTrue();

1
gerrit-server/BUILD
View File

@@ -149,6 +149,7 @@ java_library(
deps = [
":server",
"//gerrit-extension-api:api",
"//gerrit-test-util:test_util",
"//lib:truth",
],
)

7
gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java
View File

@@ -34,6 +34,7 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -68,12 +69,12 @@ public class ApplyFix implements RestModifyView<FixResource, Void> {
ObjectId patchSetCommitId = ObjectId.fromString(patchSet.getRevision().get());
try (Repository repository = gitRepositoryManager.openRepository(project)) {
TreeModification treeModification =
fixReplacementInterpreter.toTreeModification(
List<TreeModification> treeModifications =
fixReplacementInterpreter.toTreeModifications(
repository, projectState, patchSetCommitId, fixResource.getFixReplacements());
ChangeEdit changeEdit =
changeEditModifier.combineWithModifiedPatchSetTree(
repository, revisionResource.getControl(), patchSet, treeModification);
repository, revisionResource.getControl(), patchSet, treeModifications);
EditInfo editInfo = changeEditJson.toEditInfo(changeEdit, false);
return Response.ok(editInfo);
} catch (InvalidChangeOperationException e) {

54
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.CommentsUtil.setCommentRevId;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
@@ -426,7 +427,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private <T extends CommentInput> void cleanUpComments(Map<String, List<T>> commentsPerPath) {
private static <T extends CommentInput> void cleanUpComments(
Map<String, List<T>> commentsPerPath) {
Iterator<List<T>> mapValueIterator = commentsPerPath.values().iterator();
while (mapValueIterator.hasNext()) {
List<T> comments = mapValueIterator.next();
@@ -442,7 +444,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private <T extends CommentInput> void cleanUpComments(List<T> comments) {
private static <T extends CommentInput> void cleanUpComments(List<T> comments) {
Iterator<T> commentsIterator = comments.iterator();
while (commentsIterator.hasNext()) {
T comment = commentsIterator.next();
@@ -481,7 +483,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
return new HashSet<>(changeData.filePaths(revision.getPatchSet()));
}
private void ensurePathRefersToAvailableOrMagicFile(
private static void ensurePathRefersToAvailableOrMagicFile(
String path, Set<String> availableFilePaths, PatchSet.Id patchSetId)
throws BadRequestException {
if (!availableFilePaths.contains(path) && !Patch.isMagic(path)) {
@@ -490,14 +492,15 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureLineIsNonNegative(Integer line, String path) throws BadRequestException {
private static void ensureLineIsNonNegative(Integer line, String path)
throws BadRequestException {
if (line != null && line < 0) {
throw new BadRequestException(
String.format("negative line number %d not allowed on %s", line, path));
}
}
private <T extends CommentInput> void ensureCommentNotOnMagicFilesOfAutoMerge(
private static <T extends CommentInput> void ensureCommentNotOnMagicFilesOfAutoMerge(
String path, T comment) throws BadRequestException {
if (Patch.isMagic(path) && comment.side == Side.PARENT && comment.parent == null) {
throw new BadRequestException(String.format("cannot comment on %s on auto-merge", path));
@@ -519,14 +522,15 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
checkComments(revision, in);
}
private void ensureRobotIdIsSet(String robotId, String commentPath) throws BadRequestException {
private static void ensureRobotIdIsSet(String robotId, String commentPath)
throws BadRequestException {
if (robotId == null) {
throw new BadRequestException(
String.format("robotId is missing for robot comment on %s", commentPath));
}
}
private void ensureRobotRunIdIsSet(String robotRunId, String commentPath)
private static void ensureRobotRunIdIsSet(String robotRunId, String commentPath)
throws BadRequestException {
if (robotRunId == null) {
throw new BadRequestException(
@@ -534,7 +538,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureFixSuggestionsAreAddable(
private static void ensureFixSuggestionsAreAddable(
List<FixSuggestionInfo> fixSuggestionInfos, String commentPath) throws BadRequestException {
if (fixSuggestionInfos == null) {
return;
@@ -546,7 +550,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureDescriptionIsSet(String commentPath, String description)
private static void ensureDescriptionIsSet(String commentPath, String description)
throws BadRequestException {
if (description == null) {
throw new BadRequestException(
@@ -556,21 +560,25 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureFixReplacementsAreAddable(
private static void ensureFixReplacementsAreAddable(
String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
ensureReplacementsArePresent(commentPath, fixReplacementInfos);
for (FixReplacementInfo fixReplacementInfo : fixReplacementInfos) {
ensureReplacementPathIsSet(commentPath, fixReplacementInfo.path);
ensureReplacementPathRefersToFileOfComment(commentPath, fixReplacementInfo.path);
ensureRangeIsSet(commentPath, fixReplacementInfo.range);
ensureRangeIsValid(commentPath, fixReplacementInfo.range);
ensureReplacementStringIsSet(commentPath, fixReplacementInfo.replacement);
}
ensureRangesDoNotOverlap(commentPath, fixReplacementInfos);
Map<String, List<FixReplacementInfo>> replacementsPerFilePath =
fixReplacementInfos.stream().collect(groupingBy(fixReplacement -> fixReplacement.path));
for (List<FixReplacementInfo> sameFileReplacements : replacementsPerFilePath.values()) {
ensureRangesDoNotOverlap(commentPath, sameFileReplacements);
}
}
private void ensureReplacementsArePresent(
private static void ensureReplacementsArePresent(
String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
if (fixReplacementInfos == null || fixReplacementInfos.isEmpty()) {
throw new BadRequestException(
@@ -581,7 +589,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureReplacementPathIsSet(String commentPath, String replacementPath)
private static void ensureReplacementPathIsSet(String commentPath, String replacementPath)
throws BadRequestException {
if (replacementPath == null) {
throw new BadRequestException(
@@ -591,18 +599,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureReplacementPathRefersToFileOfComment(
String commentPath, String replacementPath) throws BadRequestException {
if (!Objects.equals(commentPath, replacementPath)) {
throw new BadRequestException(
String.format(
"Replacements may only be "
+ "specified for the file %s on which the robot comment was added",
commentPath));
}
}
private void ensureRangeIsSet(String commentPath, Range range) throws BadRequestException {
private static void ensureRangeIsSet(String commentPath, Range range) throws BadRequestException {
if (range == null) {
throw new BadRequestException(
String.format(
@@ -610,7 +607,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureRangeIsValid(String commentPath, Range range) throws BadRequestException {
private static void ensureRangeIsValid(String commentPath, Range range)
throws BadRequestException {
if (range == null) {
return;
}
@@ -626,7 +624,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
private void ensureReplacementStringIsSet(String commentPath, String replacement)
private static void ensureReplacementStringIsSet(String commentPath, String replacement)
throws BadRequestException {
if (replacement == null) {
throw new BadRequestException(

18
gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.edit;
import static com.google.common.base.Preconditions.checkState;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
@@ -44,6 +45,7 @@ import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.List;
import java.util.Optional;
import java.util.TimeZone;
import org.eclipse.jgit.lib.BatchRefUpdate;
@@ -307,7 +309,7 @@ public class ChangeEditModifier {
RevCommit baseCommit =
optionalChangeEdit.map(ChangeEdit::getEditCommit).orElse(basePatchSetCommit);
ObjectId newTreeId = createNewTree(repository, baseCommit, treeModification);
ObjectId newTreeId = createNewTree(repository, baseCommit, ImmutableList.of(treeModification));
String commitMessage = baseCommit.getFullMessage();
Timestamp nowTimestamp = TimeUtil.nowTs();
@@ -322,14 +324,14 @@ public class ChangeEditModifier {
}
/**
* Applies the indicated modification to the specified patch set. If a change edit exists and is
* Applies the indicated modifications to the specified patch set. If a change edit exists and is
* based on the same patch set, the modified patch set tree is merged with the change edit. If the
* change edit doesn't exist, a new one will be created.
*
* @param repository the affected Git repository
* @param changeControl the {@code ChangeControl} of the change to which the patch set belongs
* @param patchSet the {@code PatchSet} which should be modified
* @param treeModification the modification which should be applied
* @param treeModifications 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
@@ -341,7 +343,7 @@ public class ChangeEditModifier {
Repository repository,
ChangeControl changeControl,
PatchSet patchSet,
TreeModification treeModification)
List<TreeModification> treeModifications)
throws AuthException, IOException, InvalidChangeOperationException, MergeConflictException,
OrmException {
ensureAuthenticatedAndPermitted(changeControl);
@@ -350,13 +352,13 @@ public class ChangeEditModifier {
ensureAllowedPatchSet(changeControl, optionalChangeEdit, patchSet);
RevCommit patchSetCommit = lookupCommit(repository, patchSet);
ObjectId newTreeId = createNewTree(repository, patchSetCommit, treeModification);
ObjectId newTreeId = createNewTree(repository, patchSetCommit, treeModifications);
if (optionalChangeEdit.isPresent()) {
ChangeEdit changeEdit = optionalChangeEdit.get();
newTreeId = merge(repository, changeEdit, newTreeId);
if (ObjectId.equals(newTreeId, changeEdit.getEditCommit().getTree())) {
// Modification is already contained in the change edit.
// Modifications are already contained in the change edit.
return changeEdit;
}
}
@@ -459,10 +461,10 @@ public class ChangeEditModifier {
}
private static ObjectId createNewTree(
Repository repository, RevCommit baseCommit, TreeModification treeModification)
Repository repository, RevCommit baseCommit, List<TreeModification> treeModifications)
throws IOException, InvalidChangeOperationException {
TreeCreator treeCreator = new TreeCreator(baseCommit);
treeCreator.addTreeModification(treeModification);
treeCreator.addTreeModifications(treeModifications);
ObjectId newTreeId = treeCreator.createNewTreeAndGetId(repository);
if (ObjectId.equals(newTreeId, baseCommit.getTree())) {

4
gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java
View File

@@ -54,8 +54,8 @@ public class ChangeFileContentModification implements TreeModification {
return Collections.singletonList(changeContentEdit);
}
@VisibleForTesting
String getFilePath() {
@Override
public String getFilePath() {
return filePath;
}

5
gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/DeleteFileModification.java
View File

@@ -34,4 +34,9 @@ public class DeleteFileModification implements TreeModification {
DirCacheEditor.DeletePath deletePathEdit = new DirCacheEditor.DeletePath(filePath);
return Collections.singletonList(deletePathEdit);
}
@Override
public String getFilePath() {
return filePath;
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/RenameFileModification.java
View File

@@ -52,4 +52,9 @@ public class RenameFileModification implements TreeModification {
}
}
}
@Override
public String getFilePath() {
return newFilePath;
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/RestoreFileModification.java
View File

@@ -58,4 +58,9 @@ public class RestoreFileModification implements TreeModification {
}
}
}
@Override
public String getFilePath() {
return filePath;
}
}

12
gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/TreeCreator.java
View File

@@ -36,8 +36,6 @@ import org.eclipse.jgit.revwalk.RevCommit;
public class TreeCreator {
private final RevCommit baseCommit;
// At the moment, a list wouldn't be necessary as only one modification is
// applied per created tree. This is going to change in the near future.
private final List<TreeModification> treeModifications = new ArrayList<>();
public TreeCreator(RevCommit baseCommit) {
@@ -45,14 +43,14 @@ public class TreeCreator {
}
/**
* Apply a modification to the tree which is taken as a basis. If this method is called multiple
* Apply modifications to the tree which is taken as a basis. If this method is called multiple
* times, the modifications are applied subsequently in exactly the order they were provided.
*
* @param treeModification a modification which should be applied to the base tree
* @param treeModifications modifications which should be applied to the base tree
*/
public void addTreeModification(TreeModification treeModification) {
checkNotNull(treeModification, "treeModification must not be null");
treeModifications.add(treeModification);
public void addTreeModifications(List<TreeModification> treeModifications) {
checkNotNull(treeModifications, "treeModifications must not be null");
this.treeModifications.addAll(treeModifications);
}
/**

11
gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/TreeModification.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.edit.tree;
import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.util.List;
import org.eclipse.jgit.dircache.DirCacheEditor;
@@ -35,4 +36,14 @@ public interface TreeModification {
*/
List<DirCacheEditor.PathEdit> getPathEdits(Repository repository, RevCommit baseCommit)
throws IOException;
/**
* Indicates a file path which is affected by this {@code TreeModification}. If the modification
* refers to several file paths (e.g. renaming a file), returning either of them is appropriate as
* long as the returned value is deterministic.
*
* @return an affected file path
*/
@VisibleForTesting
String getFilePath();
}

31
gerrit-server/src/main/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java
View File

@@ -14,7 +14,6 @@
package com.google.gerrit.server.fixes;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.gerrit.common.RawInputUtil;
@@ -33,6 +32,8 @@ import java.io.IOException;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -51,31 +52,39 @@ public class FixReplacementInterpreter {
}
/**
* Transforms the given {@code FixReplacement}s into a {@code TreeModification}.
* Transforms the given {@code FixReplacement}s into {@code TreeModification}s.
*
* @param repository the affected Git repository
* @param projectState the affected project
* @param patchSetCommitId the patch set which should be modified
* @param fixReplacements the replacements which should be applied
* @return a {@code TreeModification} representing the given replacements
* @return a list of {@code TreeModification}s representing the given replacements
* @throws ResourceNotFoundException if a file to which one of the replacements refers doesn't
* exist
* @throws ResourceConflictException if the replacements can't be transformed into a {@code
* TreeModification}
* @throws ResourceConflictException if the replacements can't be transformed into {@code
* TreeModification}s
*/
public TreeModification toTreeModification(
public List<TreeModification> toTreeModifications(
Repository repository,
ProjectState projectState,
ObjectId patchSetCommitId,
List<FixReplacement> fixReplacements)
throws ResourceNotFoundException, IOException, ResourceConflictException {
checkNotNull(fixReplacements, "Fix replacements must not be null");
checkArgument(!fixReplacements.isEmpty(), "Fix replacements must not be empty");
// For a fix suggestion, we allow only fix replacements for the same file.
String filePath = fixReplacements.get(0).path;
return toTreeModification(
repository, projectState, patchSetCommitId, filePath, fixReplacements);
Map<String, List<FixReplacement>> fixReplacementsPerFilePath =
fixReplacements
.stream()
.collect(Collectors.groupingBy(fixReplacement -> fixReplacement.path));
List<TreeModification> treeModifications = new ArrayList<>();
for (Map.Entry<String, List<FixReplacement>> entry : fixReplacementsPerFilePath.entrySet()) {
TreeModification treeModification =
toTreeModification(
repository, projectState, patchSetCommitId, entry.getKey(), entry.getValue());
treeModifications.add(treeModification);
}
return treeModifications;
}
private TreeModification toTreeModification(

8
gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/TreeModificationSubject.java
View File

@@ -19,6 +19,8 @@ import static com.google.common.truth.Truth.assertAbout;
import com.google.common.truth.FailureStrategy;
import com.google.common.truth.Subject;
import com.google.common.truth.SubjectFactory;
import com.google.gerrit.truth.ListSubject;
import java.util.List;
public class TreeModificationSubject extends Subject<TreeModificationSubject, TreeModification> {
@@ -36,6 +38,12 @@ public class TreeModificationSubject extends Subject<TreeModificationSubject, Tr
return assertAbout(TREE_MODIFICATION_SUBJECT_FACTORY).that(treeModification);
}
public static ListSubject<TreeModificationSubject, TreeModification> assertThatList(
List<TreeModification> treeModifications) {
return ListSubject.assertThat(treeModifications, TreeModificationSubject::assertThat)
.named("treeModifications");
}
private TreeModificationSubject(
FailureStrategy failureStrategy, TreeModification treeModification) {
super(failureStrategy, treeModification);

205
gerrit-server/src/test/java/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java
View File

@@ -14,7 +14,7 @@
package com.google.gerrit.server.fixes;
import static com.google.gerrit.server.edit.tree.TreeModificationSubject.assertThat;
import static com.google.gerrit.server.edit.tree.TreeModificationSubject.assertThatList;
import static org.easymock.EasyMock.createMock;
import static org.easymock.EasyMock.replay;
@@ -26,6 +26,9 @@ import com.google.gerrit.reviewdb.client.FixReplacement;
import com.google.gerrit.server.change.FileContentUtil;
import com.google.gerrit.server.edit.tree.TreeModification;
import com.google.gerrit.server.project.ProjectState;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import org.easymock.EasyMock;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -42,7 +45,8 @@ public class FixReplacementInterpreterTest {
private final Repository repository = createMock(Repository.class);
private final ProjectState projectState = createMock(ProjectState.class);
private final ObjectId patchSetCommitId = createMock(ObjectId.class);
private final String filePath = "an/arbitrary/file.txt";
private final String filePath1 = "an/arbitrary/file.txt";
private final String filePath2 = "another/arbitrary/file.txt";
private FixReplacementInterpreter fixReplacementInterpreter;
@@ -52,24 +56,57 @@ public class FixReplacementInterpreterTest {
}
@Test
public void treeModificationTargetsCorrectFile() throws Exception {
public void noReplacementsResultInNoTreeModifications() throws Exception {
List<TreeModification> treeModifications = toTreeModifications();
assertThatList(treeModifications).isEmpty();
}
@Test
public void treeModificationsTargetCorrectFiles() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(1, 1, 3, 2), "Modified content");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(1, 6, 3, 2), "Modified content");
FixReplacement fixReplacement2 =
new FixReplacement(filePath1, new Range(3, 5, 3, 5), "Second modification");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
FixReplacement fixReplacement3 =
new FixReplacement(filePath2, new Range(2, 0, 3, 0), "Another modified content");
mockFileContent(filePath2, "1st line\n2nd line\n3rd line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement);
assertThat(treeModification).asChangeFileContentModification().filePath().isEqualTo(filePath);
List<TreeModification> treeModifications =
toTreeModifications(fixReplacement, fixReplacement3, fixReplacement2);
List<TreeModification> sortedTreeModifications = getSortedCopy(treeModifications);
assertThatList(sortedTreeModifications)
.element(0)
.asChangeFileContentModification()
.filePath()
.isEqualTo(filePath1);
assertThatList(sortedTreeModifications)
.element(0)
.asChangeFileContentModification()
.newContent()
.startsWith("First");
assertThatList(sortedTreeModifications)
.element(1)
.asChangeFileContentModification()
.filePath()
.isEqualTo(filePath2);
assertThatList(sortedTreeModifications)
.element(1)
.asChangeFileContentModification()
.newContent()
.startsWith("1st");
}
@Test
public void replacementsCanDeleteALine() throws Exception {
FixReplacement fixReplacement = new FixReplacement(filePath, new Range(2, 0, 3, 0), "");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
FixReplacement fixReplacement = new FixReplacement(filePath1, new Range(2, 0, 3, 0), "");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement);
assertThat(treeModification)
List<TreeModification> treeModifications = toTreeModifications(fixReplacement);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo("First line\nThird line\n");
@@ -78,12 +115,13 @@ public class FixReplacementInterpreterTest {
@Test
public void replacementsCanAddALine() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(2, 0, 2, 0), "A new line\n");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(2, 0, 2, 0), "A new line\n");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement);
assertThat(treeModification)
List<TreeModification> treeModifications = toTreeModifications(fixReplacement);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo("First line\nA new line\nSecond line\nThird line\n");
@@ -91,12 +129,13 @@ public class FixReplacementInterpreterTest {
@Test
public void replacementsMaySpanMultipleLines() throws Exception {
FixReplacement fixReplacement = new FixReplacement(filePath, new Range(1, 6, 3, 1), "and t");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
FixReplacement fixReplacement = new FixReplacement(filePath1, new Range(1, 6, 3, 1), "and t");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement);
assertThat(treeModification)
List<TreeModification> treeModifications = toTreeModifications(fixReplacement);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo("First and third line\n");
@@ -104,14 +143,16 @@ public class FixReplacementInterpreterTest {
@Test
public void replacementsMayOccurOnSameLine() throws Exception {
FixReplacement fixReplacement1 = new FixReplacement(filePath, new Range(2, 0, 2, 6), "A");
FixReplacement fixReplacement1 = new FixReplacement(filePath1, new Range(2, 0, 2, 6), "A");
FixReplacement fixReplacement2 =
new FixReplacement(filePath, new Range(2, 7, 2, 11), "modification");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(2, 7, 2, 11), "modification");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement1, fixReplacement2);
assertThat(treeModification)
List<TreeModification> treeModifications =
toTreeModifications(fixReplacement1, fixReplacement2);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo("First line\nA modification\nThird line\n");
@@ -120,13 +161,16 @@ public class FixReplacementInterpreterTest {
@Test
public void replacementsMayTouch() throws Exception {
FixReplacement fixReplacement1 =
new FixReplacement(filePath, new Range(1, 6, 2, 7), "modified ");
FixReplacement fixReplacement2 = new FixReplacement(filePath, new Range(2, 7, 3, 5), "content");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(1, 6, 2, 7), "modified ");
FixReplacement fixReplacement2 =
new FixReplacement(filePath1, new Range(2, 7, 3, 5), "content");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement1, fixReplacement2);
assertThat(treeModification)
List<TreeModification> treeModifications =
toTreeModifications(fixReplacement1, fixReplacement2);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo("First modified content line\n");
@@ -135,25 +179,54 @@ public class FixReplacementInterpreterTest {
@Test
public void replacementsCanAddContentAtEndOfFile() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(4, 0, 4, 0), "New content");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(4, 0, 4, 0), "New content");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement);
assertThat(treeModification)
List<TreeModification> treeModifications = toTreeModifications(fixReplacement);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo("First line\nSecond line\nThird line\nNew content");
}
@Test
public void lineSeparatorCanBeChanged() throws Exception {
FixReplacement fixReplacement = new FixReplacement(filePath, new Range(2, 11, 3, 0), "\r");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
public void replacementsCanModifySeveralFilesInAnyOrder() throws Exception {
FixReplacement fixReplacement1 =
new FixReplacement(filePath1, new Range(1, 1, 3, 2), "Modified content");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
FixReplacement fixReplacement2 =
new FixReplacement(filePath2, new Range(2, 0, 3, 0), "First modification\n");
FixReplacement fixReplacement3 =
new FixReplacement(filePath2, new Range(3, 0, 4, 0), "Second modification\n");
mockFileContent(filePath2, "1st line\n2nd line\n3rd line\n");
replay(fileContentUtil);
TreeModification treeModification = toTreeModification(fixReplacement);
assertThat(treeModification)
List<TreeModification> treeModifications =
toTreeModifications(fixReplacement3, fixReplacement1, fixReplacement2);
List<TreeModification> sortedTreeModifications = getSortedCopy(treeModifications);
assertThatList(sortedTreeModifications)
.element(0)
.asChangeFileContentModification()
.newContent()
.isEqualTo("FModified contentird line\n");
assertThatList(sortedTreeModifications)
.element(1)
.asChangeFileContentModification()
.newContent()
.isEqualTo("1st line\nFirst modification\nSecond modification\n");
}
@Test
public void lineSeparatorCanBeChanged() throws Exception {
FixReplacement fixReplacement = new FixReplacement(filePath1, new Range(2, 11, 3, 0), "\r");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
List<TreeModification> treeModifications = toTreeModifications(fixReplacement);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo("First line\nSecond line\rThird line\n");
@@ -162,17 +235,18 @@ public class FixReplacementInterpreterTest {
@Test
public void replacementsDoNotNeedToBeOrderedAccordingToRange() throws Exception {
FixReplacement fixReplacement1 =
new FixReplacement(filePath, new Range(1, 0, 2, 0), "1st modification\n");
new FixReplacement(filePath1, new Range(1, 0, 2, 0), "1st modification\n");
FixReplacement fixReplacement2 =
new FixReplacement(filePath, new Range(3, 0, 4, 0), "2nd modification\n");
new FixReplacement(filePath1, new Range(3, 0, 4, 0), "2nd modification\n");
FixReplacement fixReplacement3 =
new FixReplacement(filePath, new Range(4, 0, 5, 0), "3rd modification\n");
mockFileContent(filePath, "First line\nSecond line\nThird line\nFourth line\nFifth line\n");
new FixReplacement(filePath1, new Range(4, 0, 5, 0), "3rd modification\n");
mockFileContent(filePath1, "First line\nSecond line\nThird line\nFourth line\nFifth line\n");
replay(fileContentUtil);
TreeModification treeModification =
toTreeModification(fixReplacement2, fixReplacement1, fixReplacement3);
assertThat(treeModification)
List<TreeModification> treeModifications =
toTreeModifications(fixReplacement2, fixReplacement1, fixReplacement3);
assertThatList(treeModifications)
.onlyElement()
.asChangeFileContentModification()
.newContent()
.isEqualTo(
@@ -182,61 +256,61 @@ public class FixReplacementInterpreterTest {
@Test
public void replacementsMustNotReferToNotExistingLine() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(5, 0, 5, 0), "A new line\n");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(5, 0, 5, 0), "A new line\n");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
expectedException.expect(ResourceConflictException.class);
toTreeModification(fixReplacement);
toTreeModifications(fixReplacement);
}
@Test
public void replacementsMustNotReferToZeroLine() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(0, 0, 0, 0), "A new line\n");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(0, 0, 0, 0), "A new line\n");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
expectedException.expect(ResourceConflictException.class);
toTreeModification(fixReplacement);
toTreeModifications(fixReplacement);
}
@Test
public void replacementsMustNotReferToNotExistingOffsetOfIntermediateLine() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(1, 0, 1, 11), "modified");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(1, 0, 1, 11), "modified");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
expectedException.expect(ResourceConflictException.class);
toTreeModification(fixReplacement);
toTreeModifications(fixReplacement);
}
@Test
public void replacementsMustNotReferToNotExistingOffsetOfLastLine() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(3, 0, 3, 11), "modified");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(3, 0, 3, 11), "modified");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
expectedException.expect(ResourceConflictException.class);
toTreeModification(fixReplacement);
toTreeModifications(fixReplacement);
}
@Test
public void replacementsMustNotReferToNegativeOffset() throws Exception {
FixReplacement fixReplacement =
new FixReplacement(filePath, new Range(1, -1, 1, 5), "modified");
mockFileContent(filePath, "First line\nSecond line\nThird line\n");
new FixReplacement(filePath1, new Range(1, -1, 1, 5), "modified");
mockFileContent(filePath1, "First line\nSecond line\nThird line\n");
replay(fileContentUtil);
expectedException.expect(ResourceConflictException.class);
toTreeModification(fixReplacement);
toTreeModifications(fixReplacement);
}
private void mockFileContent(String filePath, String fileContent) throws Exception {
@@ -245,8 +319,15 @@ public class FixReplacementInterpreterTest {
.andReturn(BinaryResult.create(fileContent));
}
private TreeModification toTreeModification(FixReplacement... fixReplacements) throws Exception {
return fixReplacementInterpreter.toTreeModification(
private List<TreeModification> toTreeModifications(FixReplacement... fixReplacements)
throws Exception {
return fixReplacementInterpreter.toTreeModifications(
repository, projectState, patchSetCommitId, ImmutableList.copyOf(fixReplacements));
}
private static List<TreeModification> getSortedCopy(List<TreeModification> treeModifications) {
List<TreeModification> sortedTreeModifications = new ArrayList<>(treeModifications);
sortedTreeModifications.sort(Comparator.comparing(TreeModification::getFilePath));
return sortedTreeModifications;
}
}