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 0ae78f693a..48030eb3a2 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 @@ -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 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 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 contentModification = + fileContent -> fileContent.replace("Line 20\n", "Line twenty\n"); + addModifiedPatchSet(changeId, FILE_NAME, contentModification); + + Map changedFiles = + gApi.changes().id(changeId).current().files(previousPatchSetId); + assertThat(changedFiles.keySet()).containsExactly(COMMIT_MSG, FILE_NAME); + } + @Test public void rebaseHunksWhenReversingPatchSetOrderAreIdentified() throws Exception { ObjectId commit2 = diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java index 05acd5bdb7..3964253121 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java @@ -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 { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java index b25b588bb5..6ccd460055 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileInfoJson.java @@ -55,7 +55,7 @@ public class FileInfoJson { Map 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 toFileInfoMap(Change change, RevId revision, int parent) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java index 8ce4dd33fe..0a02e36a78 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java @@ -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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index edff29389a..4a6e01faec 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java @@ -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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java index c99d0d7f46..96f66f6f8a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListEntry.java @@ -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; } 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 6fbdd482c7..abf4d76f74 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,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_TYPES = + ImmutableBiMap.of( + Algorithm.PURE_TREE_DIFF, 'T', + Algorithm.OPTIMIZED_DIFF, 'O'); public static final ImmutableBiMap 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); } } 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 25325b7a31..b178165e75 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 @@ -147,15 +147,6 @@ public class PatchListLoader implements Callable { return save ? repo.newObjectInserter() : new InMemoryInserter(repo); } - /** - * Computes the {@code PatchList} for a given {@code PatchListKey}. - * - *

Warning: 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 { comparisonType)); } Multimap 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 editsDueToRebase = ImmutableSet.copyOf(editsDueToRebasePerFilePath.get(filePath)); + Set editsDueToRebase = getEditsDueToRebase(editsDueToRebasePerFilePath, diffEntry); Optional patchListEntry = getPatchListEntry(reader, df, diffEntry, aTree, bTree, editsDueToRebase); patchListEntry.ifPresent(entries::add); @@ -240,16 +229,7 @@ public class PatchListLoader implements Callable { * *

    *
  • 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)}. - *
  • 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). - *
  • 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. *
  • 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 { * (or just for specific types of merge commits). *
* - * @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 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. - * - *

Warning: 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 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 { 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 getEditsDueToRebase( + Multimap 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 getPatchListEntry( ObjectReader objectReader, DiffFormatter diffFormatter, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListWeigher.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListWeigher.java index f40eac6518..942d0e0903 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListWeigher.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListWeigher.java @@ -23,7 +23,8 @@ public class PatchListWeigher implements Weigher { 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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java index 84a080afea..081ba7aee1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java @@ -235,7 +235,7 @@ public class PatchScriptFactory implements Callable { 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); }