opendev/gerrit
Code Issues Proposed changes

Fix bug excluding renamed or binary files from diff

Browse Source 
The new diff logic was too aggressive at hiding unrelated changed files.
As it only considered content modifications, files which were renamed
without modifying the content were considered as not being changed.
Added/modified/deleted binary files were affected as well because
content modifications aren't computed for them.

To fix this issue, the file context is considered in addition to
(optional) content modifications when determining which changed files
can be safely ignored. This also requires some special handling in
the diff logic for changed files without content modifications.

Change-Id: I708c12bd75e044f97949e9e9fe836b99fbca753a
This commit is contained in:
Alice Kober-Sotzek
2017-06-21 18:52:23 +02:00
parent 7e300a405d
commit e54d1c3a5f
5 changed files with 156 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -31,6 +31,9 @@ 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 java.awt.image.BufferedImage;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Arrays;
@@ -40,6 +43,7 @@ import java.util.Map;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import javax.imageio.ImageIO;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -84,11 +88,61 @@ public class RevisionDiffIT extends AbstractDaemonTest {
gApi.changes().id(changeId).edit().deleteFile(FILE_NAME);
gApi.changes().id(changeId).edit().publish();
Map<String, FileInfo> changedFiles = gApi.changes().id(changeId).current().files();
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, FILE_NAME);
DiffInfo diff = gApi.changes().id(changeId).current().file(FILE_NAME).diff();
assertThat(diff.metaA.lines).isEqualTo(100);
assertThat(diff.metaB).isNull();
}
@Test
public void addedFileIsIncludedInDiff() throws Exception {
String newFilePath = "a_new_file.txt";
String newFileContent = "arbitrary content";
gApi.changes().id(changeId).edit().modifyFile(newFilePath, RawInputUtil.create(newFileContent));
gApi.changes().id(changeId).edit().publish();
Map<String, FileInfo> changedFiles = gApi.changes().id(changeId).current().files();
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, newFilePath);
}
@Test
public void renamedFileIsIncludedInDiff() throws Exception {
String newFilePath = "a_new_file.txt";
gApi.changes().id(changeId).edit().renameFile(FILE_NAME, newFilePath);
gApi.changes().id(changeId).edit().publish();
Map<String, FileInfo> changedFiles = gApi.changes().id(changeId).current().files();
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, newFilePath);
}
@Test
public void addedBinaryFileIsIncludedInDiff() throws Exception {
String imageFileName = "an_image.png";
byte[] imageBytes = createRgbImage(255, 0, 0);
gApi.changes().id(changeId).edit().modifyFile(imageFileName, RawInputUtil.create(imageBytes));
gApi.changes().id(changeId).edit().publish();
Map<String, FileInfo> changedFiles = gApi.changes().id(changeId).current().files();
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, imageFileName);
}
@Test
public void modifiedBinaryFileIsIncludedInDiff() throws Exception {
String imageFileName = "an_image.png";
byte[] imageBytes1 = createRgbImage(255, 100, 0);
ObjectId commit2 = addCommit(commit1, imageFileName, imageBytes1);
rebaseChangeOn(changeId, commit2);
byte[] imageBytes2 = createRgbImage(0, 100, 255);
gApi.changes().id(changeId).edit().modifyFile(imageFileName, RawInputUtil.create(imageBytes2));
gApi.changes().id(changeId).edit().publish();
Map<String, FileInfo> changedFiles = gApi.changes().id(changeId).current().files();
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, imageFileName);
}
@Test
public void diffOnMergeCommitChange() throws Exception {
PushOneCommit.Result r = createMergeCommitChange("refs/for/master");
@@ -151,6 +205,29 @@ public class RevisionDiffIT extends AbstractDaemonTest {
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, FILE_NAME);
}
@Test
public void renamedUnrelatedFileIsIgnored_ForPatchSetDiffWithRebase_WhenEquallyModifiedInBoth()
throws Exception {
Function<String, String> contentModification =
fileContent -> fileContent.replace("1st line\n", "First line\n");
addModifiedPatchSet(changeId, FILE_NAME2, contentModification);
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
// Revert the modification to be able to rebase.
addModifiedPatchSet(
changeId, FILE_NAME2, fileContent -> fileContent.replace("First line\n", "1st line\n"));
String renamedFileName = "renamed_file.txt";
ObjectId commit2 = addCommitRenamingFile(commit1, FILE_NAME2, renamedFileName);
rebaseChangeOn(changeId, commit2);
addModifiedPatchSet(changeId, renamedFileName, contentModification);
addModifiedPatchSet(changeId, FILE_NAME, "Another line\n"::concat);
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, FILE_NAME);
}
@Test
public void filesNotTouchedByPatchSetsAndContainingOnlyRebaseHunksAreIgnored() throws Exception {
String newFileContent = FILE_CONTENT.replace("Line 10\n", "Line ten\n");
@@ -1070,6 +1147,18 @@ public class RevisionDiffIT extends AbstractDaemonTest {
return result.getCommit();
}
private ObjectId addCommit(ObjectId parentCommit, String filePath, byte[] fileContent)
throws Exception {
testRepo.reset(parentCommit);
PushOneCommit.Result result = createEmptyChange();
String changeId = result.getChangeId();
gApi.changes().id(changeId).edit().modifyFile(filePath, RawInputUtil.create(fileContent));
gApi.changes().id(changeId).edit().publish();
String currentRevision = gApi.changes().id(changeId).get().currentRevision;
GitUtil.fetch(testRepo, "refs/*:refs/*");
return ObjectId.fromString(currentRevision);
}
private ObjectId addCommitRemovingFiles(ObjectId parentCommit, String... removedFilePaths)
throws Exception {
testRepo.reset(parentCommit);
@@ -1109,4 +1198,18 @@ public class RevisionDiffIT extends AbstractDaemonTest {
}
gApi.changes().id(changeId).edit().publish();
}
private static byte[] createRgbImage(int red, int green, int blue) throws IOException {
BufferedImage bufferedImage = new BufferedImage(10, 20, BufferedImage.TYPE_INT_RGB);
for (int x = 0; x < bufferedImage.getWidth(); x++) {
for (int y = 0; y < bufferedImage.getHeight(); y++) {
int rgb = (red << 16) + (green << 8) + blue;
bufferedImage.setRGB(x, y, rgb);
}
}
ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
ImageIO.write(bufferedImage, "png", byteArrayOutputStream);
return byteArrayOutputStream.toByteArray();
}
}

36
gerrit-server/src/main/java/com/google/gerrit/server/patch/EditTransformer.java
View File

@@ -24,10 +24,12 @@ import static java.util.stream.Collectors.toMap;
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Multimap;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Stream;
import org.eclipse.jgit.diff.Edit;
@@ -82,25 +84,25 @@ class EditTransformer {
}
/**
* Returns the transformed {@code Edit}s per file path they modify in {@code treeB'}.
* Returns the transformed edits per file path they modify in {@code treeB'}.
*
* @return the transformed {@code Edit}s per file path
* @return the transformed edits per file path
*/
public Multimap<String, Edit> getEditsPerFilePath() {
public Multimap<String, ContextAwareEdit> getEditsPerFilePath() {
return edits
.stream()
.collect(
toMultimap(
ContextAwareEdit::getNewFilePath,
ContextAwareEdit::toEdit,
ArrayListMultimap::create));
ContextAwareEdit::getNewFilePath, Function.identity(), ArrayListMultimap::create));
}
private static Stream<ContextAwareEdit> toEdits(PatchListEntry patchListEntry) {
return patchListEntry
.getEdits()
.stream()
.map(edit -> ContextAwareEdit.create(patchListEntry, edit));
public static Stream<ContextAwareEdit> toEdits(PatchListEntry patchListEntry) {
ImmutableList<Edit> edits = patchListEntry.getEdits();
if (edits.isEmpty()) {
return Stream.of(ContextAwareEdit.createForNoContentEdit(patchListEntry));
}
return edits.stream().map(edit -> ContextAwareEdit.create(patchListEntry, edit));
}
private void transformEdits(List<PatchListEntry> transformingEntries, SideStrategy sideStrategy) {
@@ -187,6 +189,10 @@ class EditTransformer {
edit.getEndB());
}
static ContextAwareEdit createForNoContentEdit(PatchListEntry patchListEntry) {
return create(patchListEntry.getOldName(), patchListEntry.getNewName(), -1, -1, -1, -1);
}
static ContextAwareEdit create(
String oldFilePath, String newFilePath, int beginA, int endA, int beginB, int endB) {
String adjustedOldFilePath = MoreObjects.firstNonNull(oldFilePath, newFilePath);
@@ -206,8 +212,12 @@ class EditTransformer {
public abstract int getEndB();
public Edit toEdit() {
return new Edit(getBeginA(), getEndA(), getBeginB(), getEndB());
public Optional<Edit> toEdit() {
if (getBeginA() < 0) {
return Optional.empty();
}
return Optional.of(new Edit(getBeginA(), getEndA(), getBeginB(), getEndB()));
}
}

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 = 6L;
public static final long serialVersionUID = 7L;
public static IntraLineDiffKey create(ObjectId aId, ObjectId bId, Whitespace whitespace) {
return new AutoValue_IntraLineDiffKey(aId, bId, whitespace);

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

@@ -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 = 26L;
public static final long serialVersionUID = 27L;
public enum Algorithm {
PURE_TREE_DIFF,

43
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListLoader.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.patch;
import static com.google.common.base.Preconditions.checkArgument;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toSet;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.common.base.Throwables;
@@ -32,6 +33,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.InMemoryInserter;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.patch.EditTransformer.ContextAwareEdit;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
@@ -195,12 +197,13 @@ public class PatchListLoader implements Callable<PatchList> {
b,
comparisonType));
}
Multimap<String, Edit> editsDueToRebasePerFilePath =
Multimap<String, ContextAwareEdit> editsDueToRebasePerFilePath =
key.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF
? getEditsDueToRebasePerFilePath(aCommit, b)
: ImmutableMultimap.of();
for (DiffEntry diffEntry : diffEntries) {
Set<Edit> editsDueToRebase = getEditsDueToRebase(editsDueToRebasePerFilePath, diffEntry);
Set<ContextAwareEdit> editsDueToRebase =
getEditsDueToRebase(editsDueToRebasePerFilePath, diffEntry);
Optional<PatchListEntry> patchListEntry =
getPatchListEntry(reader, df, diffEntry, aTree, bTree, editsDueToRebase);
patchListEntry.ifPresent(entries::add);
@@ -211,11 +214,11 @@ public class PatchListLoader implements Callable<PatchList> {
}
/**
* Identifies the {@code Edit}s which are present between {@code commitA} and {@code commitB} due
* to other commits in between those two. {@code Edit}s which cannot be clearly attributed to
* those other commits (because they overlap with modifications introduced by {@code commitA} or
* {@code commitB}) are omitted from the result. The {@code Edit}s are expressed as differences
* between {@code treeA} of {@code commitA} and {@code treeB} of {@code commitB}.
* Identifies the edits which are present between {@code commitA} and {@code commitB} due to other
* commits in between those two. Edits which cannot be clearly attributed to those other commits
* (because they overlap with modifications introduced by {@code commitA} or {@code commitB}) are
* omitted from the result. The edits are expressed as differences between {@code treeA} of {@code
* commitA} and {@code treeB} of {@code commitB}.
*
* <p><b>Note:</b> If one of the commits is a merge commit, an empty {@code Multimap} will be
* returned.
@@ -239,10 +242,10 @@ public class PatchListLoader implements Callable<PatchList> {
*
* @param commitA the commit defining {@code treeA}
* @param commitB the commit defining {@code treeB}
* @return the {@code Edit}s per file path they modify in {@code treeB}
* @throws PatchListNotAvailableException if the {@code Edit}s can't be identified
* @return the edits per file path they modify in {@code treeB}
* @throws PatchListNotAvailableException if the edits can't be identified
*/
private Multimap<String, Edit> getEditsDueToRebasePerFilePath(
private Multimap<String, ContextAwareEdit> getEditsDueToRebasePerFilePath(
RevCommit commitA, RevCommit commitB) throws PatchListNotAvailableException {
if (commitA == null
|| isRootOrMergeCommit(commitA)
@@ -280,8 +283,8 @@ public class PatchListLoader implements Callable<PatchList> {
return ObjectId.equals(commitA.getParent(0), commitB.getParent(0));
}
private static Set<Edit> getEditsDueToRebase(
Multimap<String, Edit> editsDueToRebasePerFilePath, DiffEntry diffEntry) {
private static Set<ContextAwareEdit> getEditsDueToRebase(
Multimap<String, ContextAwareEdit> editsDueToRebasePerFilePath, DiffEntry diffEntry) {
if (editsDueToRebasePerFilePath.isEmpty()) {
return ImmutableSet.of();
}
@@ -299,20 +302,30 @@ public class PatchListLoader implements Callable<PatchList> {
DiffEntry diffEntry,
RevTree treeA,
RevTree treeB,
Set<Edit> editsDueToRebase)
Set<ContextAwareEdit> editsDueToRebase)
throws IOException {
FileHeader fileHeader = toFileHeader(key, diffFormatter, diffEntry);
long oldSize = getFileSize(objectReader, diffEntry.getOldMode(), diffEntry.getOldPath(), treeA);
long newSize = getFileSize(objectReader, diffEntry.getNewMode(), diffEntry.getNewPath(), treeB);
Set<Edit> contentEditsDueToRebase = getContentEdits(editsDueToRebase);
PatchListEntry patchListEntry =
newEntry(treeA, fileHeader, editsDueToRebase, newSize, newSize - oldSize);
newEntry(treeA, fileHeader, contentEditsDueToRebase, newSize, newSize - oldSize);
// All edits in a file are due to rebase -> exclude the file from the diff.
if (patchListEntry.getEditsDueToRebase().containsAll(patchListEntry.getEdits())) {
if (EditTransformer.toEdits(patchListEntry).allMatch(editsDueToRebase::contains)) {
return Optional.empty();
}
return Optional.of(patchListEntry);
}
private static Set<Edit> getContentEdits(Set<ContextAwareEdit> editsDueToRebase) {
return editsDueToRebase
.stream()
.map(ContextAwareEdit::toEdit)
.filter(Optional::isPresent)
.map(Optional::get)
.collect(toSet());
}
private ComparisonType getComparisonType(RevObject a, RevCommit b) {
for (int i = 0; i < b.getParentCount(); i++) {
if (b.getParent(i).equals(a)) {