From cdb5010a98dd485725bd748060d786d651399076 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 17 Jan 2020 11:12:31 +0100 Subject: [PATCH 1/3] Set HTTP thread name to describe the request it is processing The HTTP worker threads had generic names: HTTP-100, HTTP-101, ... While we could conclude from the stack trace what this thread was doing we missed some important information like repository name, user name, etc... With this change the HTTP threads get descriptive names. For example, if there is an ongoing git-fetch operation we would see a thread named like: "HTTP POST /a/myProject/git-upload-pack (johndoe from 10.87.75.169)" This can be of a great help for Gerrit admins trying to figure out who is executing that many git-fetch requests and affecting the overall performance. Note that we cannot reliably tell that from the httpd_log as requests are logged only once they are finished. The show-queue command also doesn't show tasks being processing in the HTTP thread pool. As additional justification, SSH thread names are already named with a similar pattern i.e: "SSH git-upload-pack /myProject (johndoe)" Change-Id: I98cf112018732e5d4075568e7da8614ffe0495a4 --- .../gerrit/httpd/SetThreadNameFilter.java | 91 +++++++++++++++++++ .../gerrit/httpd/init/WebAppInitializer.java | 2 + java/com/google/gerrit/pgm/Daemon.java | 2 + 3 files changed, 95 insertions(+) create mode 100644 java/com/google/gerrit/httpd/SetThreadNameFilter.java diff --git a/java/com/google/gerrit/httpd/SetThreadNameFilter.java b/java/com/google/gerrit/httpd/SetThreadNameFilter.java new file mode 100644 index 0000000000..c7d977df63 --- /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 user; + + @Inject + public SetThreadNameFilter(Provider 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 c9cf2f4e85..c2d4a146ef 100644 --- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java +++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java @@ -32,6 +32,7 @@ import com.google.gerrit.httpd.RequestCleanupFilter; 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; @@ -418,6 +419,7 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi 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 8fb208b557..65707fa012 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -37,6 +37,7 @@ import com.google.gerrit.httpd.RequestCleanupFilter; 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; @@ -590,6 +591,7 @@ public class Daemon extends SiteProgram { } 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()); From a6cf72adc026371bf88bc63fee3a318de24e2284 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 9 Aug 2019 15:43:14 +0200 Subject: [PATCH 2/3] Fix an internal server error when diffing patch sets (non-merge commit) In specific constellations, users ran into an internal server error which left log statements such as: java.lang.IllegalStateException: nextB = 25; want 32 Add more tests to cover those situations and add a fix which avoids the error. In one case, this still does not result in the absolutely correct behavior but it's better than always running into a server error. Change-Id: I6feea7a37ddee39fa24378dae3fd9a5bbfb8a19b --- .../server/patch/PatchScriptBuilder.java | 6 + .../api/revision/RevisionDiffIT.java | 264 +++++++++++++++++- 2 files changed, 264 insertions(+), 6 deletions(-) diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index 61f0180879..d0b1b66cc6 100644 --- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java +++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java @@ -286,6 +286,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()) { 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 cbfc09fa74..c85e8e4fa6 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -40,6 +40,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; @@ -576,6 +577,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")); @@ -2373,9 +2477,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")); @@ -2393,6 +2495,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")); @@ -2405,6 +2613,44 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertThat(diffInfo).content().isEmpty(); } + // This behavior is likely a bug. A fix might not be easy as it might break syntax highlighting. + // TODO: Fix this issue or remove the broken parameter (at least in the documentation). + @Test + public void contextParameterIsIgnored() throws Exception { + addModifiedPatchSet( + changeId, FILE_NAME, content -> content.replace("Line 20\n", "Line twenty\n")); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME) + .withBase(initialPatchSetId) + .withContext(5) + .get(); + assertThat(diffInfo).content().element(0).commonLines().hasSize(19); + assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 20"); + assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line twenty"); + assertThat(diffInfo).content().element(2).commonLines().hasSize(81); + } + + // This behavior is likely a bug. A fix might not be easy as it might break syntax highlighting. + // TODO: Fix this issue or remove the broken parameter (at least in the documentation). + @Test + public void contextParameterIsIgnoredForUnmodifiedFileWithComment() throws Exception { + addModifiedPatchSet( + 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'."); + addCommentTo(changeId, previousPatchSetId, FILE_NAME, comment); + addModifiedPatchSet( + changeId, FILE_NAME2, content -> content.replace("2nd line\n", "Second line\n")); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME) + .withBase(previousPatchSetId) + .withContext(5) + .get(); + assertThat(diffInfo).content().element(0).commonLines().hasSize(101); + } + @Test public void requestingDiffForOldFileNameOfRenamedFileYieldsReasonableResult() throws Exception { addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 2\n", "Line two\n")); @@ -2435,9 +2681,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(); @@ -2466,6 +2710,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 = From 9078129f78b7e1c900b9a614c44bd420ee300777 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 29 Nov 2019 18:36:42 +0100 Subject: [PATCH 3/3] Switch off intraline diffs for some cases (invalid cache key sharing) This change switches off intraline diffs for COMMIT_MSG, MERGE_LIST, and submodule commit entries. There are some situations in which we use the exactly same IntraLineDiff cache key, though the resulting diff is different: 1) Diff between patch set A and B of change C for COMMIT_MSG. 2) Diff between patch set A and B of change C for MERGE_LIST. 3) Diff between patch set X and Y of change Z for a submodule commit, which updates the submodule repository (on same host) from commit A (= patch set A) to commit B (= patch set B). Depending on which diff is requested first, its result gets stored inside of the IntraLineDiff cache. When the other diffs are requested, they get the result of the first request even though that is a wrong diff for them. What makes the situation worse is that those wrong results from the IntraLineDiff cache also replace the regular (non-intraline) edits/diffs of the whole file. Users can observe this bug either by not being able to open such a wrong diff (due to an internal server error caused by trying to access content beyond its limits due to an edit in a higher line) or by seeing a wrong diff. According to design principles, cache keys should contain all values necessary to produce the cache result. For the IntraLineDiff cache, this principle is violated. That fact is directly apparent by the existence of the IntraLineDiffArgs which are passed in along the IntraLineDiffKey. There have probably been good reasons why those parameters were not included in the key (e.g. edits containing lots of data), which is also the reason why we can't directly include them in the key now. Instead, we'd need to totally rework the cache so that we can use a better cache key. We have ideas for this but it will take quite some effort and hence is not suited as a quick fix. Of course, we could now try to find a parameter we could additionally include in the key to fix the bug we identified. On the other hand, this would mean that we just paper over the real issue. In addition, we would need to be very careful as we don't have any tests for diffs relating to submodules and hardly any for COMMIT_MSG or MERGE_LIST, especially not for intraline diffs. Furthermore, we would need to flush the IntraLineDiff cache, which we typically try to avoid. As quick and easy mitigation, we decide to disable the intraline diffing for COMMIT_MSG, MERGE_LIST, and submodule commits. For COMMIT_MSG and MERGE_LIST, this effectively changes only the behavior when diffing patch sets against each other as otherwise those magic files are considered/shown as full file additions (which don't have intraline diffs). That's also the reason this bug doesn't occur in those situations: The IntralineDiff cache isn't called for them. One might argue that we could simply switch off the intraline diffing for two of the cases and keep it enabled for the remaining one as then only one type of request would use the cache key. As we can't know for sure that there isn't a further case with that same key without extremely deeply looking at the code, it's safer to switch off intraline diffing for all of them. Writing test cases for the error situations is tricky. Due to this, we only add one test which surfaces the issue when run in the intraline mode of the test class. Future test contributions especially for submodule commits would be welcome. Bug: Issue 7969 Bug: Issue 9296 Change-Id: Iaaf3a0b3cd86e80033c4472d0d5662b76daa3509 --- .../server/patch/PatchScriptBuilder.java | 14 +++++++++++--- .../api/revision/RevisionDiffIT.java | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index d0b1b66cc6..c2caa01b75 100644 --- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java +++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java @@ -133,9 +133,7 @@ class PatchScriptBuilder { edits = new ArrayList<>(content.getEdits()); ImmutableSet editsDueToRebase = content.getEditsDueToRebase(); - if (!isModify(content)) { - intralineDifferenceIsPossible = false; - } else if (diffPrefs.intralineDifference) { + if (isModify(content) && diffPrefs.intralineDifference && isIntralineModeAllowed(b)) { IntraLineDiff d = patchListCache.getIntraLineDiff( IntraLineDiffKey.create(a.id, b.id, diffPrefs.ignoreWhitespace), @@ -274,6 +272,16 @@ class PatchScriptBuilder { } } + 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(); diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index c85e8e4fa6..c27d637edf 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -19,6 +19,7 @@ import static com.google.common.truth.TruthJUnit.assume; 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.reviewdb.client.Patch.COMMIT_MSG; +import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST; import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toMap; @@ -400,6 +401,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";