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();