opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-3.0' into stable-3.1

Browse Source 
* stable-3.0:
  Switch off intraline diffs for some cases (invalid cache key sharing)
  Fix an internal server error when diffing patch sets (non-merge commit)
  Set HTTP thread name to describe the request it is processing

Change-Id: I2ec9d44bc7a9dd372e074a4030a7dd6b6d2ab657
This commit is contained in:
David Pursehouse
2020-01-21 17:18:00 +09:00
parent 1a20152774 9078129f78
commit eeb4e04b29
5 changed files with 352 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

249
javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.api.revision;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.google.gerrit.extensions.common.testing.DiffInfoSubject.assertThat;
import static com.google.gerrit.extensions.common.testing.FileInfoSubject.assertThat;
import static com.google.gerrit.git.ObjectIds.abbreviateName;
@@ -41,6 +42,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;
@@ -400,6 +402,24 @@ public class RevisionDiffIT extends AbstractDaemonTest {
assertThat(diff.metaB.lines).isEqualTo(1);
}
@Test
public void diffBetweenPatchSetsOfMergeCommitCanBeRetrievedForCommitMessageAndMergeList()
throws Exception {
PushOneCommit.Result result = createMergeCommitChange("refs/for/master", "my_file.txt");
String changeId = result.getChangeId();
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
addModifiedPatchSet(changeId, "my_file.txt", content -> content.concat("Line I\nLine II\n"));
// Call both of them in succession to ensure that they don't share the same cache keys.
DiffInfo commitMessageDiffInfo =
getDiffRequest(changeId, CURRENT, COMMIT_MSG).withBase(previousPatchSetId).get();
DiffInfo mergeListDiffInfo =
getDiffRequest(changeId, CURRENT, MERGE_LIST).withBase(previousPatchSetId).get();
assertThat(commitMessageDiffInfo).content().hasSize(3);
assertThat(mergeListDiffInfo).content().hasSize(1);
}
@Test
public void diffOfUnmodifiedFileMarksAllLinesAsCommon() throws Exception {
String filePath = "a_new_file.txt";
@@ -577,6 +597,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 +2497,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 +2515,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 +2659,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 +2701,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 +2730,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 =