From d10c18f0c7900f16175052f2a6350ca07e70c8d3 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Thu, 22 Feb 2018 16:43:00 +0100 Subject: [PATCH 1/4] Remove some false negatives for edits due to rebase When intraline diffs are enabled, we apply some optimizations (see Ie9f4210c2). Those optimization modify identified edits. Unfortunately, this clashes with the highlighting of edits due to rebase. As we use JGit's Edit class to represent an edit, we don't have a way to pass any additional details along with an edit. For this reason, we compare all identified edits with the ones we know are introduced by a rebase right before we send the diff results out of the Gerrit server. This only works if the identified edits aren't modified after being computed by JGit's diff mechanisms. A side effect of this issue is that the number of added/deleted lines for a file on the change screen doesn't always match the number of added/deleted lines of the regular hunks shown on the diff screen when comparing patch sets. The reason is that we use the non-intraline diffs for the numbers shown on the change screen even when intraline diffs are shown to a user on the diff screen. To fix this, we don't apply the optimizations whenever an edit due to rebase is involved in a to-be-combined edit. As users shouldn't need to look closely at any edits/hunks which are introduced by a rebase, it should be innocuous that we don't apply the optimizations in that case. Ideally, we would switch our internal representation from Edit to a custom class which allows us to associate custom attributes with an edit. In that case, we could apply the optimizations again for to-be-combined edits which consist completely of edits due to rebase. Using the optimization for mixing edits with edits due to rebase still wouldn't be possible. As this refactoring takes some more effort, we at least get in this quick fix for the moment. This change needs to invalidate the IntraLineDiff cache. Change-Id: If68b68b4cebd243defdb71181326a1eceee95954 --- .../api/revision/RevisionDiffIT.java | 112 ++++++++++++++++++ .../server/change/PatchListCacheIT.java | 18 ++- .../server/patch/IntraLineDiffArgs.java | 11 +- .../gerrit/server/patch/IntraLineDiffKey.java | 2 +- .../gerrit/server/patch/IntraLineLoader.java | 23 +++- .../server/patch/PatchScriptBuilder.java | 7 +- .../server/patch/IntraLineLoaderTest.java | 4 +- 7 files changed, 164 insertions(+), 13 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index e39f9f5627..e89b9d86a1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -1210,6 +1210,118 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); } + @Test + public void closeNonRebaseHunksAreCombinedForIntralineOptimizations() throws Exception { + assume().that(intraline).isTrue(); + + String fileContent = FILE_CONTENT.replace("Line 5\n", "{\n"); + ObjectId commit2 = addCommit(commit1, FILE_NAME, fileContent); + rebaseChangeOn(changeId, commit2); + String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision; + + addModifiedPatchSet( + changeId, + FILE_NAME, + content -> content.replace("Line 4\n", "Line four\n").replace("Line 6\n", "Line six\n")); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get(); + assertThat(diffInfo).content().element(0).commonLines().hasSize(3); + assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 4", "{", "Line 6"); + assertThat(diffInfo) + .content() + .element(1) + .linesOfB() + .containsExactly("Line four", "{", "Line six"); + assertThat(diffInfo).content().element(1).isNotDueToRebase(); + assertThat(diffInfo).content().element(2).commonLines().hasSize(94); + + Map changedFiles = + gApi.changes().id(changeId).current().files(previousPatchSetId); + // Lines which weren't modified but are included in a hunk due to optimization don't count for + // the number of inserted/deleted lines. + assertThat(changedFiles.get(FILE_NAME)).linesInserted().isEqualTo(2); + assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(2); + } + + @Test + public void closeRebaseHunksAreNotCombinedForIntralineOptimizations() throws Exception { + assume().that(intraline).isTrue(); + + String fileContent = FILE_CONTENT.replace("Line 5\n", "{\n"); + ObjectId commit2 = addCommit(commit1, FILE_NAME, fileContent); + rebaseChangeOn(changeId, commit2); + String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision; + + String newFileContent = + fileContent.replace("Line 4\n", "Line four\n").replace("Line 6\n", "Line six\n"); + ObjectId commit3 = addCommit(commit1, FILE_NAME, newFileContent); + rebaseChangeOn(changeId, commit3); + + addModifiedPatchSet( + changeId, FILE_NAME, content -> content.replace("Line 20\n", "Line twenty\n")); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get(); + assertThat(diffInfo).content().element(0).commonLines().hasSize(3); + assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 4"); + assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line four"); + assertThat(diffInfo).content().element(1).isDueToRebase(); + assertThat(diffInfo).content().element(2).commonLines().hasSize(1); + assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line 6"); + assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line six"); + assertThat(diffInfo).content().element(3).isDueToRebase(); + assertThat(diffInfo).content().element(4).commonLines().hasSize(13); + assertThat(diffInfo).content().element(5).linesOfA().containsExactly("Line 20"); + assertThat(diffInfo).content().element(5).linesOfB().containsExactly("Line twenty"); + assertThat(diffInfo).content().element(5).isNotDueToRebase(); + assertThat(diffInfo).content().element(6).commonLines().hasSize(80); + + Map changedFiles = + gApi.changes().id(changeId).current().files(previousPatchSetId); + assertThat(changedFiles.get(FILE_NAME)).linesInserted().isEqualTo(1); + assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); + } + + @Test + public void closeRebaseAndNonRebaseHunksAreNotCombinedForIntralineOptimizations() + throws Exception { + assume().that(intraline).isTrue(); + + String fileContent = FILE_CONTENT.replace("Line 5\n", "{\n").replace("Line 7\n", "{\n"); + ObjectId commit2 = addCommit(commit1, FILE_NAME, fileContent); + rebaseChangeOn(changeId, commit2); + String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision; + + String newFileContent = + fileContent.replace("Line 4\n", "Line four\n").replace("Line 8\n", "Line eight\n"); + ObjectId commit3 = addCommit(commit1, FILE_NAME, newFileContent); + rebaseChangeOn(changeId, commit3); + + addModifiedPatchSet(changeId, FILE_NAME, content -> content.replace("Line 6\n", "Line six\n")); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get(); + assertThat(diffInfo).content().element(0).commonLines().hasSize(3); + assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 4"); + assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line four"); + assertThat(diffInfo).content().element(1).isDueToRebase(); + assertThat(diffInfo).content().element(2).commonLines().hasSize(1); + assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line 6"); + assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line six"); + assertThat(diffInfo).content().element(3).isNotDueToRebase(); + assertThat(diffInfo).content().element(4).commonLines().hasSize(1); + assertThat(diffInfo).content().element(5).linesOfA().containsExactly("Line 8"); + assertThat(diffInfo).content().element(5).linesOfB().containsExactly("Line eight"); + assertThat(diffInfo).content().element(5).isDueToRebase(); + assertThat(diffInfo).content().element(6).commonLines().hasSize(92); + + Map changedFiles = + gApi.changes().id(changeId).current().files(previousPatchSetId); + assertThat(changedFiles.get(FILE_NAME)).linesInserted().isEqualTo(1); + assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); + } + private void assertDiffForNewFile( PushOneCommit.Result pushResult, String path, String expectedContentSideB) throws Exception { DiffInfo diff = diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java index 4f61a56b53..cefde21121 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java @@ -21,6 +21,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.cache.Cache; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; @@ -39,7 +40,9 @@ import com.google.gerrit.server.patch.Text; import com.google.inject.Inject; import com.google.inject.name.Named; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.revwalk.RevCommit; @@ -186,19 +189,26 @@ public class PatchListCacheIT extends AbstractDaemonTest { } @Test - public void harmfulMutationsOfEditsAreNotPossibleForIntraLineDiffArgsAndCachedValue() - throws Exception { + public void harmfulMutationsOfEditsAreNotPossibleForIntraLineDiffArgsAndCachedValue() { String a = "First line\nSecond line\n"; String b = "1st line\n2nd line\n"; Text aText = new Text(a.getBytes(UTF_8)); Text bText = new Text(b.getBytes(UTF_8)); Edit inputEdit = new Edit(0, 2, 0, 2); List inputEdits = new ArrayList<>(ImmutableList.of(inputEdit)); + Set inputEditsDueToRebase = new HashSet<>(ImmutableSet.of(inputEdit)); IntraLineDiffKey diffKey = IntraLineDiffKey.create(ObjectId.zeroId(), ObjectId.zeroId(), Whitespace.IGNORE_NONE); IntraLineDiffArgs diffArgs = - IntraLineDiffArgs.create(aText, bText, inputEdits, project, ObjectId.zeroId(), "file.txt"); + IntraLineDiffArgs.create( + aText, + bText, + inputEdits, + inputEditsDueToRebase, + project, + ObjectId.zeroId(), + "file.txt"); IntraLineDiff intraLineDiff = patchListCache.getIntraLineDiff(diffKey, diffArgs); Edit outputEdit = Iterables.getOnlyElement(intraLineDiff.getEdits()); @@ -206,9 +216,11 @@ public class PatchListCacheIT extends AbstractDaemonTest { outputEdit.shift(5); inputEdit.shift(7); inputEdits.add(new Edit(43, 47, 50, 51)); + inputEditsDueToRebase.add(new Edit(53, 57, 60, 61)); Edit originalEdit = new Edit(0, 2, 0, 2); assertThat(diffArgs.edits()).containsExactly(originalEdit); + assertThat(diffArgs.editsDueToRebase()).containsExactly(originalEdit); assertThat(intraLineDiff.getEdits()).containsExactly(originalEdit); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffArgs.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffArgs.java index 882360ca54..4661485ddf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffArgs.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffArgs.java @@ -16,8 +16,10 @@ package com.google.gerrit.server.patch; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.Project; import java.util.List; +import java.util.Set; import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.lib.ObjectId; @@ -27,17 +29,22 @@ public abstract class IntraLineDiffArgs { Text aText, Text bText, List edits, + Set editsDueToRebase, Project.NameKey project, ObjectId commit, String path) { return new AutoValue_IntraLineDiffArgs( - aText, bText, deepCopyEdits(edits), project, commit, path); + aText, bText, deepCopyEdits(edits), deepCopyEdits(editsDueToRebase), project, commit, path); } private static ImmutableList deepCopyEdits(List edits) { return edits.stream().map(IntraLineDiffArgs::copy).collect(ImmutableList.toImmutableList()); } + private static ImmutableSet deepCopyEdits(Set edits) { + return edits.stream().map(IntraLineDiffArgs::copy).collect(ImmutableSet.toImmutableSet()); + } + private static Edit copy(Edit edit) { return new Edit(edit.getBeginA(), edit.getEndA(), edit.getBeginB(), edit.getEndB()); } @@ -48,6 +55,8 @@ public abstract class IntraLineDiffArgs { public abstract ImmutableList edits(); + public abstract ImmutableSet editsDueToRebase(); + public abstract Project.NameKey project(); public abstract ObjectId commit(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java index fbda00a46b..90f454fbb5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java @@ -21,7 +21,7 @@ import org.eclipse.jgit.lib.ObjectId; @AutoValue public abstract class IntraLineDiffKey implements Serializable { - public static final long serialVersionUID = 8L; + public static final long serialVersionUID = 9L; public static IntraLineDiffKey create(ObjectId aId, ObjectId bId, Whitespace whitespace) { return new AutoValue_IntraLineDiffKey(aId, bId, whitespace); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java index e5e1bad82a..d7ecc8a22d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.patch; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.inject.Inject; @@ -77,7 +78,9 @@ class IntraLineLoader implements Callable { public IntraLineDiff call() throws Exception { Future result = diffExecutor.submit( - () -> IntraLineLoader.compute(args.aText(), args.bText(), args.edits())); + () -> + IntraLineLoader.compute( + args.aText(), args.bText(), args.edits(), args.editsDueToRebase())); try { return result.get(timeoutMillis, TimeUnit.MILLISECONDS); } catch (InterruptedException | TimeoutException e) { @@ -104,10 +107,13 @@ class IntraLineLoader implements Callable { } } - static IntraLineDiff compute(Text aText, Text bText, ImmutableList immutableEdits) - throws Exception { + static IntraLineDiff compute( + Text aText, + Text bText, + ImmutableList immutableEdits, + ImmutableSet immutableEditsDueToRebase) { List edits = new ArrayList<>(immutableEdits); - combineLineEdits(edits, aText, bText); + combineLineEdits(edits, immutableEditsDueToRebase, aText, bText); for (int i = 0; i < edits.size(); i++) { Edit e = edits.get(i); @@ -256,11 +262,18 @@ class IntraLineLoader implements Callable { return new IntraLineDiff(edits); } - private static void combineLineEdits(List edits, Text a, Text b) { + private static void combineLineEdits( + List edits, ImmutableSet editsDueToRebase, Text a, Text b) { for (int j = 0; j < edits.size() - 1; ) { Edit c = edits.get(j); Edit n = edits.get(j + 1); + if (editsDueToRebase.contains(c) || editsDueToRebase.contains(n)) { + // Don't combine any edits which were identified as being introduced by a rebase as we would + // lose that information because of the combination. + continue; + } + // Combine edits that are really close together. Right now our rule // is, coalesce two line edits which are only one line apart if that // common context line is either a "pointless line", or is identical diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java index 8b86be2630..6f3e05594c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.patch; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.data.CommentDetail; import com.google.gerrit.common.data.PatchScript; import com.google.gerrit.common.data.PatchScript.DisplayMethod; @@ -135,6 +136,7 @@ class PatchScriptBuilder { b.resolve(a, bId); edits = new ArrayList<>(content.getEdits()); + ImmutableSet editsDueToRebase = content.getEditsDueToRebase(); if (!isModify(content)) { intralineDifferenceIsPossible = false; @@ -142,7 +144,8 @@ class PatchScriptBuilder { IntraLineDiff d = patchListCache.getIntraLineDiff( IntraLineDiffKey.create(a.id, b.id, diffPrefs.ignoreWhitespace), - IntraLineDiffArgs.create(a.src, b.src, edits, projectKey, bId, b.path)); + IntraLineDiffArgs.create( + a.src, b.src, edits, editsDueToRebase, projectKey, bId, b.path)); if (d != null) { switch (d.getStatus()) { case EDIT_LIST: @@ -214,7 +217,7 @@ class PatchScriptBuilder { a.dst, b.dst, edits, - content.getEditsDueToRebase(), + editsDueToRebase, a.displayMethod, b.displayMethod, a.mimeType.toString(), diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java index eb31abd1e4..2b660ed52a 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import java.util.List; import org.eclipse.jgit.diff.Edit; import org.eclipse.jgit.diff.EditList; @@ -149,7 +150,8 @@ public class IntraLineLoaderTest { Text aText = new Text(a.getBytes(UTF_8)); Text bText = new Text(b.getBytes(UTF_8)); - IntraLineDiff diff = IntraLineLoader.compute(aText, bText, ImmutableList.of(lines)); + IntraLineDiff diff = + IntraLineLoader.compute(aText, bText, ImmutableList.of(lines), ImmutableSet.of()); assertThat(diff.getStatus()).isEqualTo(IntraLineDiff.Status.EDIT_LIST); List actualEdits = diff.getEdits(); From 4d6e49598bc8edd3792b882f4523f250e45fe69b Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Thu, 22 Feb 2018 17:57:58 +0100 Subject: [PATCH 2/4] Remove some more false negatives for edits due to rebase The 'begin' value of JGit's Edit is inclusive while the 'end' value is exclusive. For the transformation of edits in EditTransformer, we were a bit too careful by treating the 'end' value as inclusive for determining whether hunks overlap. This resulted in some edits not being marked as due to rebase even though they could have been. This change requires an invalidation of the PatchList cache and hence also of the IntraLineDiff cache. Bug: Issue 6583 Change-Id: I18018676d0ab90ed45498e893896621edd7052e1 --- .../api/revision/RevisionDiffIT.java | 43 +++++++++++++++++++ .../gerrit/server/patch/EditTransformer.java | 4 +- .../gerrit/server/patch/IntraLineDiffKey.java | 2 +- .../gerrit/server/patch/PatchListKey.java | 2 +- 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index e89b9d86a1..7e3e0530bd 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -801,6 +801,49 @@ public class RevisionDiffIT extends AbstractDaemonTest { assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); } + @Test + public void rebaseHunksDirectlyTouchingHunksOfPatchSetsNotModifiedBetweenThemAreIdentified() + throws Exception { + // Add to hunks in a patch set and remove them in a further patch set to allow rebasing. + Function contentModification = + fileContent -> + fileContent.replace("Line 1\n", "Line one\n").replace("Line 3\n", "Line three\n"); + addModifiedPatchSet(changeId, FILE_NAME, contentModification); + String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision; + Function reverseContentModification = + fileContent -> + fileContent.replace("Line one\n", "Line 1\n").replace("Line three\n", "Line 3\n"); + addModifiedPatchSet(changeId, FILE_NAME, reverseContentModification); + + String newFileContent = FILE_CONTENT.replace("Line 2\n", "Line two\n"); + ObjectId commit2 = addCommit(commit1, FILE_NAME, newFileContent); + rebaseChangeOn(changeId, commit2); + + // Add the hunks again and modify another line so that we get a diff for the file. + // (Files with only edits due to rebase are filtered out.) + addModifiedPatchSet( + changeId, + FILE_NAME, + contentModification.andThen(fileContent -> fileContent.replace("Line 10\n", "Line ten\n"))); + + DiffInfo diffInfo = + getDiffRequest(changeId, CURRENT, FILE_NAME).withBase(previousPatchSetId).get(); + assertThat(diffInfo).content().element(0).commonLines().hasSize(1); + assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 2"); + assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line two"); + assertThat(diffInfo).content().element(1).isDueToRebase(); + assertThat(diffInfo).content().element(2).commonLines().hasSize(7); + assertThat(diffInfo).content().element(3).linesOfA().containsExactly("Line 10"); + assertThat(diffInfo).content().element(3).linesOfB().containsExactly("Line ten"); + assertThat(diffInfo).content().element(3).isNotDueToRebase(); + assertThat(diffInfo).content().element(4).commonLines().hasSize(90); + + Map changedFiles = + gApi.changes().id(changeId).current().files(previousPatchSetId); + assertThat(changedFiles.get(FILE_NAME)).linesInserted().isEqualTo(1); + assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1); + } + @Test public void multipleRebaseEditsMixedWithRegularEditsCanBeIdentified() throws Exception { addModifiedPatchSet( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/EditTransformer.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/EditTransformer.java index 271c7c3a40..9083ede9c4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/EditTransformer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/EditTransformer.java @@ -163,10 +163,10 @@ class EditTransformer { while (origIndex < originalEdits.size() && transIndex < transformingEdits.size()) { ContextAwareEdit originalEdit = originalEdits.get(origIndex); Edit transformingEdit = transformingEdits.get(transIndex); - if (transformingEdit.getEndA() < sideStrategy.getBegin(originalEdit)) { + if (transformingEdit.getEndA() <= sideStrategy.getBegin(originalEdit)) { shiftedAmount = transformingEdit.getEndB() - transformingEdit.getEndA(); transIndex++; - } else if (sideStrategy.getEnd(originalEdit) < transformingEdit.getBeginA()) { + } else if (sideStrategy.getEnd(originalEdit) <= transformingEdit.getBeginA()) { resultingEdits.add(sideStrategy.create(originalEdit, shiftedAmount, adjustedFilePath)); origIndex++; } else { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java index 90f454fbb5..a4d6822cfb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java @@ -21,7 +21,7 @@ import org.eclipse.jgit.lib.ObjectId; @AutoValue public abstract class IntraLineDiffKey implements Serializable { - public static final long serialVersionUID = 9L; + public static final long serialVersionUID = 10L; public static IntraLineDiffKey create(ObjectId aId, ObjectId bId, Whitespace whitespace) { return new AutoValue_IntraLineDiffKey(aId, bId, whitespace); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java index 73e82a188a..3f533ff6f4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java @@ -32,7 +32,7 @@ import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; public class PatchListKey implements Serializable { - public static final long serialVersionUID = 28L; + public static final long serialVersionUID = 29L; // TODO(aliceks): Get rid of this enum and the parameter in the PatchListKey as we only use one of // its values. From dee83fbc7c14826eea107dc4d3164dc6d3ed3e99 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Thu, 22 Feb 2018 18:30:41 +0100 Subject: [PATCH 3/4] Remove unnecessary 'algorithm' parameter of PatchListKey Because of I209cf815, the 'algorithm' parameter of PatchListKey isn't necessary anymore. As that change tried to avoid an invalidation of the PatchList cache, it wasn't removed at that time. Since I18018676 needs to invalidate the PatchList cache, we use the opportunity to get rid of this parameter. Change-Id: I540325c7694af395ee820b5c3d0dd9d56adf2318 --- .../gerrit/server/patch/DiffSummaryKey.java | 5 +- .../gerrit/server/patch/IntraLineDiffKey.java | 2 +- .../gerrit/server/patch/PatchListKey.java | 50 ++++--------------- .../gerrit/server/patch/PatchListLoader.java | 10 ++-- 4 files changed, 15 insertions(+), 52 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java index 0a02e36a78..8ce4dd33fe 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java @@ -19,7 +19,6 @@ import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull; import static org.eclipse.jgit.lib.ObjectIdSerialization.writeCanBeNull; import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull; -import com.google.common.base.Preconditions; import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; import java.io.IOException; import java.io.ObjectInputStream; @@ -41,7 +40,6 @@ public class DiffSummaryKey implements Serializable { private transient Whitespace whitespace; public static DiffSummaryKey fromPatchListKey(PatchListKey plk) { - Preconditions.checkArgument(plk.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF); return new DiffSummaryKey( plk.getOldId(), plk.getParentNum(), plk.getNewId(), plk.getWhitespace()); } @@ -54,8 +52,7 @@ public class DiffSummaryKey implements Serializable { } PatchListKey toPatchListKey() { - return new PatchListKey( - oldId, parentNum, newId, whitespace, PatchListKey.Algorithm.OPTIMIZED_DIFF); + return new PatchListKey(oldId, parentNum, newId, whitespace); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java index a4d6822cfb..b47bbfb050 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java @@ -21,7 +21,7 @@ import org.eclipse.jgit.lib.ObjectId; @AutoValue public abstract class IntraLineDiffKey implements Serializable { - public static final long serialVersionUID = 10L; + public static final long serialVersionUID = 11L; public static IntraLineDiffKey create(ObjectId aId, ObjectId bId, Whitespace whitespace) { return new AutoValue_IntraLineDiffKey(aId, bId, whitespace); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java index 3f533ff6f4..df5ad41982 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java @@ -32,19 +32,7 @@ import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; public class PatchListKey implements Serializable { - public static final long serialVersionUID = 29L; - - // TODO(aliceks): Get rid of this enum and the parameter in the PatchListKey as we only use one of - // its values. - public enum Algorithm { - PURE_TREE_DIFF, - OPTIMIZED_DIFF - } - - private static final ImmutableBiMap ALGORITHM_TYPES = - ImmutableBiMap.of( - Algorithm.PURE_TREE_DIFF, 'T', - Algorithm.OPTIMIZED_DIFF, 'O'); + public static final long serialVersionUID = 30L; public static final ImmutableBiMap WHITESPACE_TYPES = ImmutableBiMap.of( @@ -55,20 +43,19 @@ public class PatchListKey implements Serializable { static { checkState(WHITESPACE_TYPES.size() == Whitespace.values().length); - checkState(ALGORITHM_TYPES.size() == Algorithm.values().length); } public static PatchListKey againstDefaultBase(AnyObjectId newId, Whitespace ws) { - return new PatchListKey(null, newId, ws, Algorithm.OPTIMIZED_DIFF); + return new PatchListKey(null, newId, ws); } public static PatchListKey againstParentNum(int parentNum, AnyObjectId newId, Whitespace ws) { - return new PatchListKey(parentNum, newId, ws, Algorithm.OPTIMIZED_DIFF); + return new PatchListKey(parentNum, newId, ws); } public static PatchListKey againstCommit( AnyObjectId otherCommitId, AnyObjectId newId, Whitespace whitespace) { - return new PatchListKey(otherCommitId, newId, whitespace, Algorithm.OPTIMIZED_DIFF); + return new PatchListKey(otherCommitId, newId, whitespace); } /** @@ -93,34 +80,25 @@ public class PatchListKey implements Serializable { private transient ObjectId newId; private transient Whitespace whitespace; - private transient Algorithm algorithm; - private PatchListKey(AnyObjectId a, AnyObjectId b, Whitespace ws, Algorithm algorithm) { + private PatchListKey(AnyObjectId a, AnyObjectId b, Whitespace ws) { oldId = a != null ? a.copy() : null; newId = b.copy(); whitespace = ws; - this.algorithm = algorithm; } - private PatchListKey(int parentNum, AnyObjectId b, Whitespace ws, Algorithm algorithm) { + private PatchListKey(int parentNum, AnyObjectId b, Whitespace ws) { this.parentNum = Integer.valueOf(parentNum); newId = b.copy(); whitespace = ws; - this.algorithm = algorithm; } /** For use only by DiffSummaryKey. */ - PatchListKey( - ObjectId oldId, - Integer parentNum, - ObjectId newId, - Whitespace whitespace, - Algorithm algorithm) { + PatchListKey(ObjectId oldId, Integer parentNum, ObjectId newId, Whitespace whitespace) { this.oldId = oldId; this.parentNum = parentNum; this.newId = newId; this.whitespace = whitespace; - this.algorithm = algorithm; } /** Old side commit, or null to assume ancestor or combined merge. */ @@ -144,13 +122,9 @@ public class PatchListKey implements Serializable { return whitespace; } - public Algorithm getAlgorithm() { - return algorithm; - } - @Override public int hashCode() { - return Objects.hash(oldId, parentNum, newId, whitespace, algorithm); + return Objects.hash(oldId, parentNum, newId, whitespace); } @Override @@ -160,8 +134,7 @@ public class PatchListKey implements Serializable { return Objects.equals(oldId, k.oldId) && Objects.equals(parentNum, k.parentNum) && Objects.equals(newId, k.newId) - && whitespace == k.whitespace - && algorithm == k.algorithm; + && whitespace == k.whitespace; } return false; } @@ -179,8 +152,6 @@ public class PatchListKey implements Serializable { n.append(" "); } n.append(whitespace.name()); - n.append(" "); - n.append(algorithm.name()); n.append("]"); return n.toString(); } @@ -194,7 +165,6 @@ public class PatchListKey implements Serializable { throw new IOException("Invalid whitespace type: " + whitespace); } out.writeChar(c); - out.writeChar(ALGORITHM_TYPES.get(algorithm)); } private void readObject(ObjectInputStream in) throws IOException { @@ -207,7 +177,5 @@ public class PatchListKey implements Serializable { if (whitespace == null) { throw new IOException("Invalid whitespace type code: " + t); } - char algorithmCharacter = in.readChar(); - algorithm = ALGORITHM_TYPES.inverse().get(algorithmCharacter); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java index 8fce6d3739..9ffd985ff5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java @@ -187,12 +187,10 @@ public class PatchListLoader implements Callable { List diffEntries = df.scan(aTree, bTree); Multimap editsDueToRebasePerFilePath = ImmutableMultimap.of(); - if (key.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF) { - EditsDueToRebaseResult editsDueToRebaseResult = - determineEditsDueToRebase(aCommit, b, diffEntries, df, rw); - diffEntries = editsDueToRebaseResult.getRelevantOriginalDiffEntries(); - editsDueToRebasePerFilePath = editsDueToRebaseResult.getEditsDueToRebasePerFilePath(); - } + EditsDueToRebaseResult editsDueToRebaseResult = + determineEditsDueToRebase(aCommit, b, diffEntries, df, rw); + diffEntries = editsDueToRebaseResult.getRelevantOriginalDiffEntries(); + editsDueToRebasePerFilePath = editsDueToRebaseResult.getEditsDueToRebasePerFilePath(); List entries = new ArrayList<>(); entries.add( From 8271f4588c909e66746985cd621c7db3755c301d Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Mon, 2 Apr 2018 15:40:40 -0700 Subject: [PATCH 4/4] Detect RawInput correctly Some endpoints allow both JSON and raw input. parseRequest selects whether to parse to JSON using a reader or to provide the raw input as an InputStream based on the request's content-type. Since v2.15-rc0~1847^2 (Discard request HTTP bodies before writing response, 2017-03-16), on endpoints that permit raw input, we call getInputStream to obtain the rest of the response body and discard it before writing the response. When the request was JSON, this produces errors from Jetty, since calling getInputStream after getReader violates the servlet API: [HTTP-66] ERROR com.google.gerrit.httpd.restapi.RestApiServlet : Error in PUT /a/plugins/reviewers.jar java.lang.IllegalStateException: READER at org.eclipse.jetty.server.Request.getInputStream(Request.java:844) at javax.servlet.ServletRequestWrapper.getInputStream(ServletRequestWrapper.java:138) at javax.servlet.ServletRequestWrapper.getInputStream(ServletRequestWrapper.java:138) To fix it, instead of guessing whether this was a raw request based on whether the endpoint supports raw requests, use the parseRequest result to decide whether this is a raw request for which we need to discard any unconsumed content. Bug: Issue 8677 Change-Id: I1db69104f31e1c04b137d994523422a07ca5cf43 (cherry picked from commit 91136bb28ec45cbbd66e7d8aabe209a6faa7eb2a) --- .../gerrit/httpd/restapi/RestApiServlet.java | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index d1e4e88915..4d4ef8e92c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -403,7 +403,11 @@ public class RestApiServlet extends HttpServlet { Type type = inputType(m); inputRequestBody = parseRequest(req, type); result = m.apply(rsrc, inputRequestBody); - consumeRawInputRequestBody(req, type); + if (inputRequestBody instanceof RawInput) { + try (InputStream is = req.getInputStream()) { + ServletUtils.consumeRequestBody(is); + } + } } else { throw new ResourceNotFoundException(); } @@ -750,7 +754,9 @@ public class RestApiServlet extends HttpServlet { br.skip(Long.MAX_VALUE); } } - } else if (rawInputRequest(req, type)) { + } + String method = req.getMethod(); + if (("PUT".equals(method) || "POST".equals(method)) && acceptsRawInput(type)) { return parseRawInput(req, type); } else if ("DELETE".equals(req.getMethod()) && hasNoBody(req)) { return null; @@ -773,19 +779,6 @@ public class RestApiServlet extends HttpServlet { } } - private void consumeRawInputRequestBody(HttpServletRequest req, Type type) throws IOException { - if (rawInputRequest(req, type)) { - try (InputStream is = req.getInputStream()) { - ServletUtils.consumeRequestBody(is); - } - } - } - - private static boolean rawInputRequest(HttpServletRequest req, Type type) { - String method = req.getMethod(); - return ("PUT".equals(method) || "POST".equals(method)) && acceptsRawInput(type); - } - private static boolean hasNoBody(HttpServletRequest req) { int len = req.getContentLength(); String type = req.getContentType();