diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index 0f228fe7cd..8b6b91a9c0 100644 --- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java +++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java @@ -272,6 +272,12 @@ class PatchScriptBuilder { 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 lastEdit = getLast(edits); if (isNewlineAtEndDeleted(a, b)) { Optional 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 24ac2096f2..2a6b42c8a4 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.ChangeType; 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; @@ -577,6 +578,109 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); } + @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")); @@ -2374,9 +2478,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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")); @@ -2394,6 +2496,112 @@ public class RevisionDiffIT extends AbstractDaemonTest { .inOrder(); } + @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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 @@ public class RevisionDiffIT extends AbstractDaemonTest { 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 =