opendev/gerrit
Code Issues Proposed changes

Merge "Fix bug in computation of hunks due to rebase"

Browse Source
This commit is contained in:
Alice Kober-Sotzek 2017-06-14 15:59:19 +00:00 committed by Gerrit Code Review
commit 9feae8c650
10 changed files with 151 additions and 84 deletions
Show Stats 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);
}