opendev/gerrit
Code Issues Proposed changes

Remove some false negatives for edits due to rebase

Browse Source 
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
This commit is contained in:
Alice Kober-Sotzek
2018-02-22 16:43:00 +01:00
parent 2f1e39e362
commit d10c18f0c7
7 changed files with 164 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

112
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
View File

@@ -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<String, FileInfo> 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<String, FileInfo> 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<String, FileInfo> 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 =

18
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java
View File

@@ -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<Edit> inputEdits = new ArrayList<>(ImmutableList.of(inputEdit));
Set<Edit> 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);
}

11
gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffArgs.java
View File

@@ -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<Edit> edits,
Set<Edit> 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<Edit> deepCopyEdits(List<Edit> edits) {
return edits.stream().map(IntraLineDiffArgs::copy).collect(ImmutableList.toImmutableList());
}
private static ImmutableSet<Edit> deepCopyEdits(Set<Edit> 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<Edit> edits();
public abstract ImmutableSet<Edit> editsDueToRebase();
public abstract Project.NameKey project();
public abstract ObjectId commit();

2
gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineDiffKey.java
View File

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

23
gerrit-server/src/main/java/com/google/gerrit/server/patch/IntraLineLoader.java
View File

@@ -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<IntraLineDiff> {
public IntraLineDiff call() throws Exception {
Future<IntraLineDiff> 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<IntraLineDiff> {
}
}
static IntraLineDiff compute(Text aText, Text bText, ImmutableList<Edit> immutableEdits)
throws Exception {
static IntraLineDiff compute(
Text aText,
Text bText,
ImmutableList<Edit> immutableEdits,
ImmutableSet<Edit> immutableEditsDueToRebase) {
List<Edit> 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<IntraLineDiff> {
return new IntraLineDiff(edits);
}
private static void combineLineEdits(List<Edit> edits, Text a, Text b) {
private static void combineLineEdits(
List<Edit> edits, ImmutableSet<Edit> 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

7
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptBuilder.java
View File

@@ -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<Edit> 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(),

4
gerrit-server/src/test/java/com/google/gerrit/server/patch/IntraLineLoaderTest.java
View File

@@ -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<Edit> actualEdits = diff.getEdits();