opendev/gerrit
Code Issues Proposed changes

Fix bug in computation of hunks due to rebase

Browse Source 
Combining the optimization which omits files from the diff result
containing only hunks due to rebase with the recursive calling of the
diff computation didn't work well. The logic assumed that all
differences between the parent commits were returned. Due to the
optimization, only some of them were returned and hence only some
hunks due to rebase could be marked.

As the optimization was only applied under specific circumstances,
this issue didn't occur for every diff between patch sets. In
particular, it was observable when a change was based on another change.

To resolve this issue, a parameter is introduced which influences
whether optimizations might be applied. All code paths from external
calls continue to apply those optimizations if applicable. When the
differences between parent commits is computed for the hunks due to
rebase, the unmodified differences between their trees is explicitly
requested. As the recursion is thus cut short, the expensive call to
RevWalk#isMergedInto can be removed from the check which determines
whether the edits due to rebase are computed.

Even though the DiffSummaryCache is supposed to be fairly independent
from changes in PatchList and PatchListKey, its implementation is
coupled to them. As the change in the diffing logic shouldn't
influence the DiffSummaryCache, this change tries to work around it.

Change-Id: I9bf31dec18d6dd357c17c640fa0a940808e97269
This commit is contained in:
Alice Kober-Sotzek
2017-06-14 15:01:33 +02:00
parent 40270c4bef
commit a5504b0ca0
10 changed files with 151 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -887,17 +887,28 @@ public class RevisionDiffIT extends AbstractDaemonTest {
assertThat(changedFiles.get(renamedFilePath)).linesDeleted().isEqualTo(1);
}
/*
* change PS B
* |
* change PS A commit4
* | |
* commit2 commit3
* | /
* commit1 --------
*/
@Test
public void rebaseHunksWhenRebasingOnAnotherChangeAreIdentified() throws Exception {
public void rebaseHunksWhenRebasingOnAnotherChangeOrPatchSetAreIdentified() throws Exception {
ObjectId commit2 =
addCommit(commit1, FILE_NAME, FILE_CONTENT.replace("Line 5\n", "Line five\n"));
rebaseChangeOn(changeId, commit2);
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
ObjectId commit3 =
addCommit(commit1, FILE_NAME, FILE_CONTENT.replace("Line 35\n", "Line thirty five\n"));
String commit3FileContent = FILE_CONTENT.replace("Line 35\n", "Line thirty five\n");
ObjectId commit3 = addCommit(commit1, FILE_NAME, commit3FileContent);
ObjectId commit4 =
addCommit(commit3, FILE_NAME, commit3FileContent.replace("Line 60\n", "Line sixty\n"));
rebaseChangeOn(changeId, commit3);
rebaseChangeOn(changeId, commit4);
Function<String, String> contentModification =
fileContent -> fileContent.replace("Line 20\n", "Line twenty\n");
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
@@ -916,7 +927,11 @@ public class RevisionDiffIT extends AbstractDaemonTest {
assertThat(diffInfo).content().element(5).linesOfA().containsExactly("Line 35");
assertThat(diffInfo).content().element(5).linesOfB().containsExactly("Line thirty five");
assertThat(diffInfo).content().element(5).isDueToRebase();
assertThat(diffInfo).content().element(6).commonLines().hasSize(65);
assertThat(diffInfo).content().element(6).commonLines().hasSize(24);
assertThat(diffInfo).content().element(7).linesOfA().containsExactly("Line 60");
assertThat(diffInfo).content().element(7).linesOfB().containsExactly("Line sixty");
assertThat(diffInfo).content().element(7).isDueToRebase();
assertThat(diffInfo).content().element(8).commonLines().hasSize(40);
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
@@ -924,6 +939,37 @@ public class RevisionDiffIT extends AbstractDaemonTest {
assertThat(changedFiles.get(FILE_NAME)).linesDeleted().isEqualTo(1);
}
/*
* change PS B
* |
* change PS A commit4
* | |
* commit2 commit3
* | /
* commit1 --------
*/
@Test
public void unrelatedFileWhenRebasingOnAnotherChangeOrPatchSetIsIgnored() throws Exception {
ObjectId commit2 =
addCommit(commit1, FILE_NAME, FILE_CONTENT.replace("Line 5\n", "Line five\n"));
rebaseChangeOn(changeId, commit2);
String previousPatchSetId = gApi.changes().id(changeId).get().currentRevision;
ObjectId commit3 =
addCommit(commit1, FILE_NAME2, FILE_CONTENT2.replace("2nd line\n", "Second line\n"));
ObjectId commit4 =
addCommit(commit3, FILE_NAME, FILE_CONTENT.replace("Line 60\n", "Line sixty\n"));
rebaseChangeOn(changeId, commit4);
Function<String, String> contentModification =
fileContent -> fileContent.replace("Line 20\n", "Line twenty\n");
addModifiedPatchSet(changeId, FILE_NAME, contentModification);
Map<String, FileInfo> changedFiles =
gApi.changes().id(changeId).current().files(previousPatchSetId);
assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, FILE_NAME);
}
@Test
public void rebaseHunksWhenReversingPatchSetOrderAreIdentified() throws Exception {
ObjectId commit2 =

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

@@ -234,7 +234,7 @@ public class PatchListCacheIT extends AbstractDaemonTest {
}
private PatchListKey getKey(ObjectId revisionIdA, ObjectId revisionIdB) {
return new PatchListKey(revisionIdA, revisionIdB, Whitespace.IGNORE_NONE);
return PatchListKey.againstCommit(revisionIdA, revisionIdB, Whitespace.IGNORE_NONE);
}
private ObjectId getCurrentRevisionId(String changeId) throws Exception {

2
gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java
View File

@@ -55,7 +55,7 @@ public class FileInfoJson {
Map<String, FileInfo> toFileInfoMap(Change change, ObjectId objectId, @Nullable PatchSet base)
throws PatchListNotAvailableException {
ObjectId a = (base == null) ? null : ObjectId.fromString(base.getRevision().get());
return toFileInfoMap(change, new PatchListKey(a, objectId, Whitespace.IGNORE_NONE));
return toFileInfoMap(change, PatchListKey.againstCommit(a, objectId, Whitespace.IGNORE_NONE));
}
Map<String, FileInfo> toFileInfoMap(Change change, RevId revision, int parent)

5
gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java
View File

@@ -19,6 +19,7 @@ import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
import static org.eclipse.jgit.lib.ObjectIdSerialization.writeCanBeNull;
import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
import com.google.common.base.Preconditions;
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import java.io.IOException;
import java.io.ObjectInputStream;
@@ -40,6 +41,7 @@ public class DiffSummaryKey implements Serializable {
private transient Whitespace whitespace;
public static DiffSummaryKey fromPatchListKey(PatchListKey plk) {
Preconditions.checkArgument(plk.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF);
return new DiffSummaryKey(
plk.getOldId(), plk.getParentNum(), plk.getNewId(), plk.getWhitespace());
}
@@ -52,7 +54,8 @@ public class DiffSummaryKey implements Serializable {
}
PatchListKey toPatchListKey() {
return new PatchListKey(oldId, parentNum, newId, whitespace);
return new PatchListKey(
oldId, parentNum, newId, whitespace, PatchListKey.Algorithm.OPTIMIZED_DIFF);
}
@Override

4
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
View File

@@ -101,7 +101,9 @@ public class PatchListCacheImpl implements PatchListCache {
throws PatchListNotAvailableException {
try {
PatchList pl = fileCache.get(key, fileLoaderFactory.create(key, project));
diffSummaryCache.put(DiffSummaryKey.fromPatchListKey(key), toDiffSummary(pl));
if (key.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF) {
diffSummaryCache.put(DiffSummaryKey.fromPatchListKey(key), toDiffSummary(pl));
}
return pl;
} catch (ExecutionException e) {
PatchListLoader.log.warn("Error computing " + key, e);

1
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java
View File

@@ -159,6 +159,7 @@ public class PatchListEntry {
size += stringSize(newName);
size += header.length;
size += (8 + 16 + 4 * 4) * edits.size();
size += (8 + 16 + 4 * 4) * editsDueToRebase.size();
return size;
}

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

@@ -32,7 +32,17 @@ import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
public class PatchListKey implements Serializable {
public static final long serialVersionUID = 25L;
public static final long serialVersionUID = 26L;
public enum Algorithm {
PURE_TREE_DIFF,
OPTIMIZED_DIFF
}
private static final ImmutableBiMap<Algorithm, Character> ALGORITHM_TYPES =
ImmutableBiMap.of(
Algorithm.PURE_TREE_DIFF, 'T',
Algorithm.OPTIMIZED_DIFF, 'O');
public static final ImmutableBiMap<Whitespace, Character> WHITESPACE_TYPES =
ImmutableBiMap.of(
@@ -43,14 +53,25 @@ public class PatchListKey implements Serializable {
static {
checkState(WHITESPACE_TYPES.size() == Whitespace.values().length);
checkState(ALGORITHM_TYPES.size() == Algorithm.values().length);
}
public static PatchListKey againstDefaultBase(AnyObjectId newId, Whitespace ws) {
return new PatchListKey(null, newId, ws);
return new PatchListKey(null, newId, ws, Algorithm.OPTIMIZED_DIFF);
}
public static PatchListKey againstParentNum(int parentNum, AnyObjectId newId, Whitespace ws) {
return new PatchListKey(parentNum, newId, ws);
return new PatchListKey(parentNum, newId, ws, Algorithm.OPTIMIZED_DIFF);
}
public static PatchListKey againstCommit(
AnyObjectId otherCommitId, AnyObjectId newId, Whitespace whitespace) {
return new PatchListKey(otherCommitId, newId, whitespace, Algorithm.OPTIMIZED_DIFF);
}
public static PatchListKey againstCommitWithPureTreeDiff(
AnyObjectId otherCommitId, AnyObjectId newId, Whitespace whitespace) {
return new PatchListKey(otherCommitId, newId, whitespace, Algorithm.PURE_TREE_DIFF);
}
/**
@@ -75,25 +96,34 @@ public class PatchListKey implements Serializable {
private transient ObjectId newId;
private transient Whitespace whitespace;
private transient Algorithm algorithm;
public PatchListKey(AnyObjectId a, AnyObjectId b, Whitespace ws) {
private PatchListKey(AnyObjectId a, AnyObjectId b, Whitespace ws, Algorithm algorithm) {
oldId = a != null ? a.copy() : null;
newId = b.copy();
whitespace = ws;
this.algorithm = algorithm;
}
private PatchListKey(int parentNum, AnyObjectId b, Whitespace ws) {
private PatchListKey(int parentNum, AnyObjectId b, Whitespace ws, Algorithm algorithm) {
this.parentNum = Integer.valueOf(parentNum);
newId = b.copy();
whitespace = ws;
this.algorithm = algorithm;
}
/** For use only by DiffSummaryKey. */
PatchListKey(ObjectId oldId, Integer parentNum, ObjectId newId, Whitespace whitespace) {
PatchListKey(
ObjectId oldId,
Integer parentNum,
ObjectId newId,
Whitespace whitespace,
Algorithm algorithm) {
this.oldId = oldId;
this.parentNum = parentNum;
this.newId = newId;
this.whitespace = whitespace;
this.algorithm = algorithm;
}
/** Old side commit, or null to assume ancestor or combined merge. */
@@ -117,9 +147,13 @@ public class PatchListKey implements Serializable {
return whitespace;
}
public Algorithm getAlgorithm() {
return algorithm;
}
@Override
public int hashCode() {
return Objects.hash(oldId, parentNum, newId, whitespace);
return Objects.hash(oldId, parentNum, newId, whitespace, algorithm);
}
@Override
@@ -129,7 +163,8 @@ public class PatchListKey implements Serializable {
return Objects.equals(oldId, k.oldId)
&& Objects.equals(parentNum, k.parentNum)
&& Objects.equals(newId, k.newId)
&& whitespace == k.whitespace;
&& whitespace == k.whitespace
&& algorithm == k.algorithm;
}
return false;
}
@@ -147,6 +182,8 @@ public class PatchListKey implements Serializable {
n.append(" ");
}
n.append(whitespace.name());
n.append(" ");
n.append(algorithm.name());
n.append("]");
return n.toString();
}
@@ -160,6 +197,7 @@ public class PatchListKey implements Serializable {
throw new IOException("Invalid whitespace type: " + whitespace);
}
out.writeChar(c);
out.writeChar(ALGORITHM_TYPES.get(algorithm));
}
private void readObject(ObjectInputStream in) throws IOException {
@@ -172,5 +210,7 @@ public class PatchListKey implements Serializable {
if (whitespace == null) {
throw new IOException("Invalid whitespace type code: " + t);
}
char algorithmCharacter = in.readChar();
algorithm = ALGORITHM_TYPES.inverse().get(algorithmCharacter);
}
}

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

@@ -147,15 +147,6 @@ public class PatchListLoader implements Callable<PatchList> {
return save ? repo.newObjectInserter() : new InMemoryInserter(repo);
}
/**
* Computes the {@code PatchList} for a given {@code PatchListKey}.
*
* <p><b>Warning:</b> This method may reset the specified {@code RevWalk}. Don't call it in the
* middle of a walk.
*
* @throws IOException if the repository can't be accessed
* @throws PatchListNotAvailableException if the {@code PatchList} can't be computed
*/
private PatchList readPatchList(Repository repo, RevWalk rw, ObjectInserter ins)
throws IOException, PatchListNotAvailableException {
ObjectReader reader = rw.getObjectReader();
@@ -205,13 +196,11 @@ public class PatchListLoader implements Callable<PatchList> {
comparisonType));
}
Multimap<String, Edit> editsDueToRebasePerFilePath =
getEditsDueToRebasePerFilePath(rw, b, aCommit);
key.getAlgorithm() == PatchListKey.Algorithm.OPTIMIZED_DIFF
? getEditsDueToRebasePerFilePath(aCommit, b)
: ImmutableMultimap.of();
for (DiffEntry diffEntry : diffEntries) {
String filePath = diffEntry.getNewPath();
if (diffEntry.getChangeType() == ChangeType.DELETE) {
filePath = diffEntry.getOldPath();
}
Set<Edit> editsDueToRebase = ImmutableSet.copyOf(editsDueToRebasePerFilePath.get(filePath));
Set<Edit> editsDueToRebase = getEditsDueToRebase(editsDueToRebasePerFilePath, diffEntry);
Optional<PatchListEntry> patchListEntry =
getPatchListEntry(reader, df, diffEntry, aTree, bTree, editsDueToRebase);
patchListEntry.ifPresent(entries::add);
@@ -240,16 +229,7 @@ public class PatchListLoader implements Callable<PatchList> {
*
* <ul>
* <li>If {@code commitA} is an ancestor of {@code commitB} (or the other way around), {@code
* commitA} (or {@code commitB}) is used instead of its parent in {@link
* #getEditsDueToRebasePerFilePath(RevCommit, RevCommit)}.
* <li>The method {@link #arePatchSetCommitsWithRebaseInBetween(RevWalk, RevCommit, RevCommit)}
* is adjusted to only short-circuit this method if {@code commitA} is the parent of {@code
* commitB} (or the other way around).
* <li>A flag is added to {@link PatchListKey} indicating whether this method should be called.
* As this method calls {@link PatchListCache#get(PatchListKey, Project.NameKey)} (which
* will end up in this method again), we have to make sure that this method doesn't recurse
* until a parent/child pair of commits (or the root of the history) is reached. Introducing
* a flag would be the simplest approach but there certainly are other ways, too.
* commitA} (or {@code commitB}) is used instead of its parent in this method.
* <li>Special handling for merge commits is added. If only one of them is a merge commit, the
* whole computation has to be done between the single parent and all parents of the merge
* commit. If both of them are merge commits, all combinations of parents have to be
@@ -257,56 +237,24 @@ public class PatchListLoader implements Callable<PatchList> {
* (or just for specific types of merge commits).
* </ul>
*
* @param revWalk a {@code RevWalk} for the repository
* @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 IOException if the repository can't be accessed
* @throws PatchListNotAvailableException if the {@code Edit}s can't be identified
*/
private Multimap<String, Edit> getEditsDueToRebasePerFilePath(
RevWalk revWalk, RevCommit commitB, RevCommit commitA)
throws IOException, PatchListNotAvailableException {
if (!arePatchSetCommitsWithRebaseInBetween(revWalk, commitA, commitB)) {
RevCommit commitA, RevCommit commitB) throws PatchListNotAvailableException {
if (commitA == null
|| isRootOrMergeCommit(commitA)
|| isRootOrMergeCommit(commitB)
|| areParentChild(commitA, commitB)
|| haveCommonParent(commitA, commitB)) {
return ImmutableMultimap.of();
}
return getEditsDueToRebasePerFilePath(commitA, commitB);
}
/**
* Indicates whether {@code commitA} and {@code commitB} represent two patch sets separated by a
* rebase provided the below-mentioned assumption is met.
*
* <p><b>Warning:</b> This method assumes that commitA and commitB are either a parent and child
* commit or represent two patch sets which belong to the same change. No checks are made to
* confirm this assumption!
*
* @throws IOException if the repository can't be accessed
*/
private boolean arePatchSetCommitsWithRebaseInBetween(
RevWalk revWalk, RevCommit commitA, RevCommit commitB) throws IOException {
return key.getOldId() != null
&& commitB.getParentCount() == 1
&& commitA != null
&& commitA.getParentCount() == 1
&& !ObjectId.equals(commitB.getParent(0), commitA.getParent(0))
&& !revWalk.isMergedInto(commitA, commitB)
&& !revWalk.isMergedInto(commitB, commitA);
}
/**
* Determines all {@code Edit}s which were introduced by a rebase. The {@code Edit}s are expressed
* as differences between {@code treeA} of {@code commitA} and {@code treeB} of {@code commitB}.
*
* @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 determined
*/
private Multimap<String, Edit> getEditsDueToRebasePerFilePath(
RevCommit commitA, RevCommit commitB) throws PatchListNotAvailableException {
PatchListKey parentDiffKey =
new PatchListKey(commitA.getParent(0), commitB.getParent(0), key.getWhitespace());
PatchListKey.againstCommitWithPureTreeDiff(
commitA.getParent(0), commitB.getParent(0), key.getWhitespace());
PatchList parentPatchList = patchListCache.get(parentDiffKey, project);
PatchListKey oldKey = PatchListKey.againstDefaultBase(key.getOldId(), key.getWhitespace());
PatchList oldPatchList = patchListCache.get(oldKey, project);
@@ -319,6 +267,32 @@ public class PatchListLoader implements Callable<PatchList> {
return editTransformer.getEditsPerFilePath();
}
private static boolean isRootOrMergeCommit(RevCommit commit) {
return commit.getParentCount() != 1;
}
private static boolean areParentChild(RevCommit commitA, RevCommit commitB) {
return ObjectId.equals(commitA.getParent(0), commitB)
|| ObjectId.equals(commitB.getParent(0), commitA);
}
private static boolean haveCommonParent(RevCommit commitA, RevCommit commitB) {
return ObjectId.equals(commitA.getParent(0), commitB.getParent(0));
}
private static Set<Edit> getEditsDueToRebase(
Multimap<String, Edit> editsDueToRebasePerFilePath, DiffEntry diffEntry) {
if (editsDueToRebasePerFilePath.isEmpty()) {
return ImmutableSet.of();
}
String filePath = diffEntry.getNewPath();
if (diffEntry.getChangeType() == ChangeType.DELETE) {
filePath = diffEntry.getOldPath();
}
return ImmutableSet.copyOf(editsDueToRebasePerFilePath.get(filePath));
}
private Optional<PatchListEntry> getPatchListEntry(
ObjectReader objectReader,
DiffFormatter diffFormatter,

3
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListWeigher.java
View File

@@ -23,7 +23,8 @@ public class PatchListWeigher implements Weigher<PatchListKey, PatchList> {
int size =
16
+ 4 * 8
+ 2 * 36 // Size of PatchListKey, 64 bit JVM
+ 2 * 36
+ 8 // Size of PatchListKey, 64 bit JVM
+ 16
+ 3 * 8
+ 3 * 4

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

@@ -235,7 +235,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
private PatchListKey keyFor(Whitespace whitespace) {
if (parentNum < 0) {
return new PatchListKey(aId, bId, whitespace);
return PatchListKey.againstCommit(aId, bId, whitespace);
}
return PatchListKey.againstParentNum(parentNum + 1, bId, whitespace);
}