Merge branch 'stable-3.0' into stable-3.1 * 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
diff --git a/java/com/google/gerrit/httpd/SetThreadNameFilter.java b/java/com/google/gerrit/httpd/SetThreadNameFilter.java new file mode 100644 index 0000000..c7d977d --- /dev/null +++ b/java/com/google/gerrit/httpd/SetThreadNameFilter.java
@@ -0,0 +1,91 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.httpd; + +import com.google.gerrit.server.CurrentUser; +import com.google.inject.Inject; +import com.google.inject.Module; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import com.google.inject.servlet.ServletModule; +import java.io.IOException; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; + +@Singleton +public class SetThreadNameFilter implements Filter { + private static final int MAX_PATH_LENGTH = 120; + + public static Module module() { + return new ServletModule() { + @Override + protected void configureServlets() { + filter("/*").through(SetThreadNameFilter.class); + } + }; + } + + private final Provider<CurrentUser> user; + + @Inject + public SetThreadNameFilter(Provider<CurrentUser> user) { + this.user = user; + } + + @Override + public void init(FilterConfig filterConfig) {} + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + Thread current = Thread.currentThread(); + String old = current.getName(); + try { + current.setName(computeName((HttpServletRequest) request)); + chain.doFilter(request, response); + } finally { + current.setName(old); + } + } + + private String computeName(HttpServletRequest req) { + StringBuilder s = new StringBuilder(); + s.append("HTTP "); + s.append(req.getMethod()); + s.append(" "); + s.append(req.getRequestURI()); + String query = req.getQueryString(); + if (query != null) { + s.append("?").append(query); + } + if (s.length() > MAX_PATH_LENGTH) { + s.delete(MAX_PATH_LENGTH, s.length()); + } + s.append(" ("); + s.append(user.get().getUserName().orElse("N/A")); + s.append(" from "); + s.append(req.getRemoteAddr()); + s.append(")"); + return s.toString(); + } + + @Override + public void destroy() {} +}
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java index e6504e2..0befbd3 100644 --- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java +++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -31,6 +31,7 @@ import com.google.gerrit.httpd.RequestContextFilter; import com.google.gerrit.httpd.RequestMetricsFilter; import com.google.gerrit.httpd.RequireSslFilter; +import com.google.gerrit.httpd.SetThreadNameFilter; import com.google.gerrit.httpd.WebModule; import com.google.gerrit.httpd.WebSshGlueModule; import com.google.gerrit.httpd.auth.oauth.OAuthModule; @@ -383,6 +384,7 @@ modules.add(sysInjector.getInstance(GerritAuthModule.class)); modules.add(sysInjector.getInstance(GitOverHttpModule.class)); modules.add(RequestCleanupFilter.module()); + modules.add(SetThreadNameFilter.module()); modules.add(AllRequestFilter.module()); modules.add(sysInjector.getInstance(WebModule.class)); modules.add(sysInjector.getInstance(RequireSslFilter.Module.class));
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index 88ccf18..2457b6a 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -35,6 +35,7 @@ import com.google.gerrit.httpd.RequestContextFilter; import com.google.gerrit.httpd.RequestMetricsFilter; import com.google.gerrit.httpd.RequireSslFilter; +import com.google.gerrit.httpd.SetThreadNameFilter; import com.google.gerrit.httpd.WebModule; import com.google.gerrit.httpd.WebSshGlueModule; import com.google.gerrit.httpd.auth.oauth.OAuthModule; @@ -547,6 +548,7 @@ } modules.add(RequestCleanupFilter.module()); modules.add(AllRequestFilter.module()); + modules.add(SetThreadNameFilter.module()); modules.add(sysInjector.getInstance(WebModule.class)); modules.add(sysInjector.getInstance(RequireSslFilter.Module.class)); modules.add(new HttpPluginModule());
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index f3c8eab..1c1c639 100644 --- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java +++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -133,7 +133,7 @@ edits = new ArrayList<>(content.getEdits()); ImmutableSet<Edit> editsDueToRebase = content.getEditsDueToRebase(); - if (isModify(content) && diffPrefs.intralineDifference) { + if (isModify(content) && diffPrefs.intralineDifference && isIntralineModeAllowed(b)) { IntraLineDiff d = patchListCache.getIntraLineDiff( IntraLineDiffKey.create(a.id, b.id, diffPrefs.ignoreWhitespace), @@ -267,6 +267,16 @@ } } + private static boolean isIntralineModeAllowed(Side side) { + // The intraline diff cache keys are the same for these cases. It's better to not show + // intraline results than showing completely wrong diffs or to run into a server error. + return !Patch.isMagic(side.path) && !isSubmoduleCommit(side.mode); + } + + private static boolean isSubmoduleCommit(FileMode mode) { + return mode.getObjectType() == Constants.OBJ_COMMIT; + } + private void correctForDifferencesInNewlineAtEnd() { // a.src.size() is the size ignoring a newline at the end whereas a.size() considers it. int aSize = a.src.size(); @@ -279,6 +289,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()) { 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..d4a4c45 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -17,6 +17,7 @@ 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.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; @@ -401,6 +403,24 @@ } @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"; String fileContent = "Line 1\nLine 2\nLine 3\n"; @@ -578,6 +598,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 +2497,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 +2516,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 +2659,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 +2701,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 +2730,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 =