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
diff --git a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
index d0b1b66..c2caa01 100644
--- a/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
+++ b/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
@@ -133,9 +133,7 @@
edits = new ArrayList<>(content.getEdits());
ImmutableSet<Edit> 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 @@
}
}
+ 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 c85e8e4..c27d637 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.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;
@@ -401,6 +402,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";
