opendev/gerrit
Code Issues Proposed changes

Fix bug excluding some renamed files from patch set diff

Browse Source 
Files which were renamed between two patch sets but contained only hunks
due to rebase were erroneously excluded from the set of modified files.
The tricky part is to identify which renames truly happen between
patch sets.

To be able to distinguish different types of renames, we use a flag to
mark all renames which are introduced when transforming the diff hunks
between the parents to ones between the patch sets. This flag is only
relevant when deciding which files should be excluded from the diff.

The current fix might not be the most elegant solution and might not
be perfect, but it seems to result in the correct behavior.

Change-Id: I7dfd81961803110fd65932e0bcba7f79dd462079
This commit is contained in:
Alice Kober-Sotzek 2017-07-13 15:12:30 +02:00
parent 704ba94fae
commit a29041ce24
4 changed files with 150 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -249,6 +249,61 @@ public class RevisionDiffIT extends AbstractDaemonTest {
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, FILE_NAME);
}
@Test
public void renamedUnrelatedFileIsIgnored_ForPatchSetDiffWithRebase_WhenModifiedDuringRebase()
throws Exception {
String renamedFilePath = "renamed_some_file.txt";
ObjectId commit2 =
addCommit(commit1, FILE_NAME, FILE_CONTENT.replace("Line 5\n", "Line five\n"));
ObjectId commit3 = addCommitRenamingFile(commit2, FILE_NAME, renamedFilePath);
rebaseChangeOn(changeId, commit3);
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(initialPatchSetId);
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG);
}
@Test
public void fileRenamedDuringRebaseSameAsInPatchSetIsIgnored() throws Exception {
String renamedFileName = "renamed_file.txt";
gApi.changes().id(changeId).edit().renameFile(FILE_NAME, renamedFileName);
gApi.changes().id(changeId).edit().publish();
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
// Revert the renaming to be able to rebase.
gApi.changes().id(changeId).edit().renameFile(renamedFileName, FILE_NAME);
gApi.changes().id(changeId).edit().publish();
ObjectId commit2 = addCommitRenamingFile(commit1, FILE_NAME, renamedFileName);
rebaseChangeOn(changeId, commit2);
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG);
}
@Test
public void fileWithRebaseHunksRenamedDuringRebaseSameAsInPatchSetIsIgnored() throws Exception {
String renamedFileName = "renamed_file.txt";
gApi.changes().id(changeId).edit().renameFile(FILE_NAME, renamedFileName);
gApi.changes().id(changeId).edit().publish();
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
// Revert the renaming to be able to rebase.
gApi.changes().id(changeId).edit().renameFile(renamedFileName, FILE_NAME);
gApi.changes().id(changeId).edit().publish();
ObjectId commit2 =
addCommit(commit1, FILE_NAME, FILE_CONTENT.replace("Line 10\n", "Line ten\n"));
ObjectId commit3 = addCommitRenamingFile(commit2, FILE_NAME, renamedFileName);
rebaseChangeOn(changeId, commit3);
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG);
}
@Test
public void filesNotTouchedByPatchSetsAndContainingOnlyRebaseHunksAreIgnored() throws Exception {
String newFileContent = FILE_CONTENT.replace("Line 10\n", "Line ten\n");
@ -912,6 +967,78 @@ public class RevisionDiffIT extends AbstractDaemonTest {
assertThat(changedFiles.get(renamedFilePath)).linesDeleted().isEqualTo(1);
}
@Test
public void renamedFileWithOnlyRebaseHunksIsIdentified_WhenRenamedBetweenPatchSets()
throws Exception {
String newFilePath1 = "renamed_some_file.txt";
gApi.changes().id(changeId).edit().renameFile(FILE_NAME, newFilePath1);
gApi.changes().id(changeId).edit().publish();
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
// Revert the renaming to be able to rebase.
gApi.changes().id(changeId).edit().renameFile(newFilePath1, FILE_NAME);
gApi.changes().id(changeId).edit().publish();
ObjectId commit2 =
addCommit(commit1, FILE_NAME, FILE_CONTENT.replace("Line 5\n", "Line five\n"));
rebaseChangeOn(changeId, commit2);
String newFilePath2 = "renamed_some_file_to_something_else.txt";
gApi.changes().id(changeId).edit().renameFile(FILE_NAME, newFilePath2);
gApi.changes().id(changeId).edit().publish();
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, newFilePath2);
assertThat(changedFiles.get(newFilePath2)).linesInserted().isNull();
assertThat(changedFiles.get(newFilePath2)).linesDeleted().isNull();
DiffInfo diffInfo =
getDiffRequest(changeId, CURRENT, newFilePath2).withBase(previousPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(4);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 5");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line five");
assertThat(diffInfo).content().element(1).isDueToRebase();
assertThat(diffInfo).content().element(2).commonLines().hasSize(95);
}
@Test
public void renamedFileWithOnlyRebaseHunksIsIdentified_WhenRenamedForRebaseAndForPatchSets()
throws Exception {
String newFilePath1 = "renamed_some_file.txt";
gApi.changes().id(changeId).edit().renameFile(FILE_NAME, newFilePath1);
gApi.changes().id(changeId).edit().publish();
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
// Revert the renaming to be able to rebase.
gApi.changes().id(changeId).edit().renameFile(newFilePath1, FILE_NAME);
gApi.changes().id(changeId).edit().publish();
ObjectId commit2 =
addCommit(commit1, FILE_NAME, FILE_CONTENT.replace("Line 5\n", "Line five\n"));
String newFilePath2 = "renamed_some_file_during_rebase.txt";
ObjectId commit3 = addCommitRenamingFile(commit2, FILE_NAME, newFilePath2);
rebaseChangeOn(changeId, commit3);
String newFilePath3 = "renamed_some_file_to_something_else.txt";
gApi.changes().id(changeId).edit().renameFile(newFilePath2, newFilePath3);
gApi.changes().id(changeId).edit().publish();
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, newFilePath3);
assertThat(changedFiles.get(newFilePath3)).linesInserted().isNull();
assertThat(changedFiles.get(newFilePath3)).linesDeleted().isNull();
DiffInfo diffInfo =
getDiffRequest(changeId, CURRENT, newFilePath3).withBase(previousPatchSetId).get();
assertThat(diffInfo).content().element(0).commonLines().hasSize(4);
assertThat(diffInfo).content().element(1).linesOfA().containsExactly("Line 5");
assertThat(diffInfo).content().element(1).linesOfB().containsExactly("Line five");
assertThat(diffInfo).content().element(1).isDueToRebase();
assertThat(diffInfo).content().element(2).commonLines().hasSize(95);
}
/*
* change PS B
* |

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

@ -29,6 +29,7 @@ import com.google.common.collect.Multimap;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Stream;
@ -186,18 +187,27 @@ class EditTransformer {
edit.getBeginA(),
edit.getEndA(),
edit.getBeginB(),
edit.getEndB());
edit.getEndB(),
false);
}
static ContextAwareEdit createForNoContentEdit(PatchListEntry patchListEntry) {
return create(patchListEntry.getOldName(), patchListEntry.getNewName(), -1, -1, -1, -1);
return create(
patchListEntry.getOldName(), patchListEntry.getNewName(), -1, -1, -1, -1, false);
}
static ContextAwareEdit create(
String oldFilePath, String newFilePath, int beginA, int endA, int beginB, int endB) {
String oldFilePath,
String newFilePath,
int beginA,
int endA,
int beginB,
int endB,
boolean filePathAdjusted) {
String adjustedOldFilePath = MoreObjects.firstNonNull(oldFilePath, newFilePath);
boolean implicitRename = !Objects.equals(oldFilePath, newFilePath) && filePathAdjusted;
return new AutoValue_EditTransformer_ContextAwareEdit(
adjustedOldFilePath, newFilePath, beginA, endA, beginB, endB);
adjustedOldFilePath, newFilePath, beginA, endA, beginB, endB, implicitRename);
}
public abstract String getOldFilePath();
@ -212,6 +222,9 @@ class EditTransformer {
public abstract int getEndB();
// Used for equals(), for which this value is important.
public abstract boolean isImplicitRename();
public Optional<Edit> toEdit() {
if (getBeginA() < 0) {
return Optional.empty();
@ -258,7 +271,8 @@ class EditTransformer {
edit.getBeginA() + shiftedAmount,
edit.getEndA() + shiftedAmount,
edit.getBeginB(),
edit.getEndB());
edit.getEndB(),
!Objects.equals(edit.getOldFilePath(), adjustedFilePath));
}
}
@ -289,7 +303,8 @@ class EditTransformer {
edit.getBeginA(),
edit.getEndA(),
edit.getBeginB() + shiftedAmount,
edit.getEndB() + shiftedAmount);
edit.getEndB() + shiftedAmount,
!Objects.equals(edit.getNewFilePath(), adjustedFilePath));
}
}
}

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