Fix bug excluding renamed or binary files from diff
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
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 48030eb..8c0b927 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
@@ -31,6 +31,9 @@
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.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,12 +88,62 @@
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");
@@ -152,6 +206,29 @@
}
@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");
ObjectId commit2 = addCommit(commit1, FILE_NAME, newFileContent);
@@ -1070,6 +1147,18 @@
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 @@
}
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();
+ }
}
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 8c103b7..376dc51 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
@@ -24,10 +24,12 @@
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 @@
}
/**
- * 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 @@
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 @@
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()));
}
}
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 ea5acef..b78d519 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 @@
@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);
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 abf4d76..e7b78b7 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.ObjectId;
public class PatchListKey implements Serializable {
- public static final long serialVersionUID = 26L;
+ public static final long serialVersionUID = 27L;
public enum Algorithm {
PURE_TREE_DIFF,
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 b178165..e6ababd 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
@@ -17,6 +17,7 @@
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.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 @@
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 @@
}
/**
- * 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 @@
*
* @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 @@
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 @@
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)) {
