Merge "Fix an internal server error when diffing patch sets (non-merge commit)"
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index 0f228fe..8b6b91a 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -272,6 +272,12 @@
return;
}
+ if (edits.isEmpty() && (aSize != bSize)) {
+ // Only edits due to rebase were present. If we now added the edits for the newlines, the
+ // code which later assembles the file contents would fail.
+ return;
+ }
+
Optional<Edit> lastEdit = getLast(edits);
if (isNewlineAtEndDeleted(a, b)) {
Optional<Edit> lastLineEdit = lastEdit.filter(edit -> edit.getEndA() == aSize);
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index 24ac209..2a6b42c 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -41,6 +41,7 @@
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.testing.ConfigSuite;
import java.awt.image.BufferedImage;
import java.io.ByteArrayOutputStream;
@@ -578,6 +579,109 @@
}
@Test
+ public void addedNewlineAtEndOfFileIsMarkedInDiffWhenOtherwiseOnlyEditsDueToRebaseExist()
+ throws Exception {
+ addModifiedPatchSet(changeId, FILE_NAME, fileContent -> fileContent.concat("Line 101"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = FILE_CONTENT.replace("Line 70\n", "Line seventy\n");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ addModifiedPatchSet(changeId, FILE_NAME, fileContent -> fileContent.concat("\n"));
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_NONE)
+ .withBase(previousPatchSetId)
+ .get();
+ assertThat(diffInfo).content().element(0).commonLines().isNotEmpty();
+ assertThat(diffInfo).content().element(1).isNotNull(); // Line 70 modification
+ assertThat(diffInfo).content().element(2).commonLines().isNotEmpty();
+ assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line 101");
+ assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line 101", "");
+
+ assertThat(diffInfo).metaA().totalLineCount().isEqualTo(101);
+ assertThat(diffInfo).metaB().totalLineCount().isEqualTo(102);
+ }
+
+ @Test
+ // TODO: Fix this issue. This test documents the current behavior and ensures that we at least
+ // don't run into an internal server error.
+ public void addedNewlineAtEndOfFileIsNotIdentifiedAsDueToRebaseEvenThoughItShould()
+ throws Exception {
+ String baseFileContent = FILE_CONTENT.concat("Line 101");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, baseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ // Add a comment so that file contents are not 'skipped'. To be able to add a comment, touch
+ // (= modify) the file in the change.
+ addModifiedPatchSet(
+ changeId, FILE_NAME, fileContent -> fileContent.replace("Line 2\n", "Line two\n"));
+ CommentInput comment = createCommentInput(3, 0, 4, 0, "Comment to not skip file content.");
+ addCommentTo(changeId, CURRENT, FILE_NAME, comment);
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = baseFileContent.concat("\n");
+ ObjectId commit3 = addCommit(commit2, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit3);
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_ALL)
+ .withBase(previousPatchSetId)
+ .get();
+ assertThat(diffInfo).content().element(0).commonLines().isNotEmpty();
+ assertThat(diffInfo).content().element(1).linesOfA().isNull();
+ assertThat(diffInfo).content().element(1).linesOfB().containsExactly("");
+ // This should actually be isDueToRebase().
+ assertThat(diffInfo).content().element(1).isNotDueToRebase();
+ }
+
+ @Test
+ public void
+ addedNewlineAtEndOfFileIsMarkedWhenEditDueToRebaseIncreasedLineCountAndWhitespaceConsidered()
+ throws Exception {
+ addModifiedPatchSet(changeId, FILE_NAME, fileContent -> fileContent.concat("Line 101"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = FILE_CONTENT.replace("Line 70\n", "Line 70\nLine 70.5\n");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ addModifiedPatchSet(changeId, FILE_NAME, fileContent -> fileContent.concat("\n"));
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_NONE)
+ .withBase(previousPatchSetId)
+ .get();
+ assertThat(diffInfo).content().element(0).commonLines().isNotEmpty();
+ assertThat(diffInfo).content().element(1).isNotNull(); // Line 70.5 insertion
+ assertThat(diffInfo).content().element(2).commonLines().isNotEmpty();
+ assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line 101");
+ assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line 101", "");
+
+ assertThat(diffInfo).metaA().totalLineCount().isEqualTo(101);
+ assertThat(diffInfo).metaB().totalLineCount().isEqualTo(103);
+ }
+
+ @Test
+ // TODO: Fix this issue. This test documents the current behavior and ensures that we at least
+ // don't run into an internal server error.
+ public void
+ addedNewlineAtEndOfFileIsNotMarkedWhenEditDueToRebaseIncreasedLineCountAndWhitespaceIgnoredEvenThoughItShould()
+ throws Exception {
+ addModifiedPatchSet(changeId, FILE_NAME, fileContent -> fileContent.concat("Line 101"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = FILE_CONTENT.replace("Line 70\n", "Line 70\nLine 70.5\n");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ addModifiedPatchSet(changeId, FILE_NAME, fileContent -> fileContent.concat("\n"));
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_ALL)
+ .withBase(previousPatchSetId)
+ .get();
+ assertThat(diffInfo).content().element(0).numberOfSkippedLines().isGreaterThan(0);
+ }
+
+ @Test
public void addedLastLineWithoutNewlineBeforeAndAfterwardsIsMarkedInDiff() throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, fileContent -> fileContent.concat("Line 101"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
@@ -2374,9 +2478,7 @@
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- ReviewInput reviewInput = new ReviewInput();
- reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(comment));
- gApi.changes().id(changeId).revision(previousPatchSetId).review(reviewInput);
+ addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
addModifiedPatchSet(
changeId, FILE_NAME2, content -> content.replace("2nd line\n", "Second line\n"));
@@ -2395,6 +2497,112 @@
}
@Test
+ public void
+ diffOfFileWithOnlyRebaseHunksAndWithCommentAndConsideringWhitespaceReturnsFileContents()
+ throws Exception {
+ addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = FILE_CONTENT.replace("Line 70\n", "Line seventy\n");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
+ addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withBase(previousPatchSetId)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_NONE)
+ .get();
+ // We don't list the full file contents here as that is not the focus of this test.
+ assertThat(diffInfo)
+ .content()
+ .element(0)
+ .commonLines()
+ .containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
+ .inOrder();
+ }
+
+ @Test
+ public void diffOfFileWithOnlyRebaseHunksAndWithCommentAndIgnoringWhitespaceReturnsFileContents()
+ throws Exception {
+ addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = FILE_CONTENT.replace("Line 70\n", "Line seventy\n");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
+ addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withBase(previousPatchSetId)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_ALL)
+ .get();
+ // We don't list the full file contents here as that is not the focus of this test.
+ assertThat(diffInfo)
+ .content()
+ .element(0)
+ .commonLines()
+ .containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
+ .inOrder();
+ }
+
+ @Test
+ public void
+ diffOfFileWithMultilineRebaseHunkAddingNewlineAtEndOfFileAndWithCommentReturnsFileContents()
+ throws Exception {
+ String baseFileContent = FILE_CONTENT.concat("Line 101");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, baseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = baseFileContent.concat("\nLine 102\nLine 103\n");
+ ObjectId commit3 = addCommit(commit2, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit3);
+ CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
+ addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withBase(previousPatchSetId)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_NONE)
+ .get();
+ // We don't list the full file contents here as that is not the focus of this test.
+ assertThat(diffInfo)
+ .content()
+ .element(0)
+ .commonLines()
+ .containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
+ .inOrder();
+ }
+
+ @Test
+ public void
+ diffOfFileWithMultilineRebaseHunkRemovingNewlineAtEndOfFileAndWithCommentReturnsFileContents()
+ throws Exception {
+ addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
+ String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
+ String newBaseFileContent = FILE_CONTENT.concat("Line 101\nLine 103\nLine 104");
+ ObjectId commit2 = addCommit(commit1, FILE_NAME, newBaseFileContent);
+ rebaseChangeOn(changeId, commit2);
+ CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
+ addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
+
+ DiffInfo diffInfo =
+ getDiffRequest(changeId, CURRENT, FILE_NAME)
+ .withBase(previousPatchSetId)
+ .withWhitespace(DiffPreferencesInfo.Whitespace.IGNORE_NONE)
+ .get();
+ // We don't list the full file contents here as that is not the focus of this test.
+ assertThat(diffInfo)
+ .content()
+ .element(0)
+ .commonLines()
+ .containsAtLeast("Line 1", "Line two", "Line 3", "Line 4", "Line 5")
+ .inOrder();
+ }
+
+ @Test
public void diffOfNonExistentFileIsAnEmptyDiffResult() throws Exception {
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
@@ -2432,9 +2640,7 @@
changeId, FILE_NAME, content -> content.replace("Line 20\n", "Line twenty\n"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
CommentInput comment = createCommentInput(20, 0, 21, 0, "Should be 'Line 20'.");
- ReviewInput reviewInput = new ReviewInput();
- reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(comment));
- gApi.changes().id(changeId).revision(previousPatchSetId).review(reviewInput);
+ addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
addModifiedPatchSet(
changeId, FILE_NAME2, content -> content.replace("2nd line\n", "Second line\n"));
@@ -2476,9 +2682,7 @@
addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n"));
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
CommentInput comment = createCommentInput(2, 0, 3, 0, "Should be 'Line 2'.");
- ReviewInput reviewInput = new ReviewInput();
- reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(comment));
- gApi.changes().id(changeId).revision(previousPatchSetId).review(reviewInput);
+ addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment);
String newFilePath = "a_new_file.txt";
gApi.changes().id(changeId).edit().renameFile(FILE_NAME, newFilePath);
gApi.changes().id(changeId).edit().publish();
@@ -2507,6 +2711,14 @@
return comment;
}
+ private void addCommentTo(
+ String changeId, String previousPatchSetId, String fileName, CommentInput comment)
+ throws RestApiException {
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.comments = ImmutableMap.of(fileName, ImmutableList.of(comment));
+ gApi.changes().id(changeId).revision(previousPatchSetId).review(reviewInput);
+ }
+
private void assertDiffForNewFile(
PushOneCommit.Result pushResult, String path, String expectedContentSideB) throws Exception {
DiffInfo diff =