From 6aecd8efdef42b6592b01e6a51c0b084e03fba6a Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 16 May 2014 15:52:23 +0200 Subject: [PATCH 1/3] Add basic test for PatchListCache that checks the returned patches The tests cover the current behaviour. At the moment when two patch sets are compared the returned file list may be incorrect and contain too many files. This is the case if the old patch set contains modifications that were reverted in the new patch set or if there was a rebase between the two patch sets that touched files that are not contained in the new patch set. As commented in the tests they check the current behaviour which is incorrect. The problems will be fixed in a follow-up change that then adapts these tests to check the correct behaviour. Change-Id: I1ed59dc25a40a8de4b28f824187e37983fac1072 Signed-off-by: Edwin Kempin --- .../server/change/PatchListCacheIT.java | 242 ++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java 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 new file mode 100644 index 0000000000..4088cd1284 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java @@ -0,0 +1,242 @@ +// Copyright (C) 2014 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.server.change; + +import static com.google.gerrit.acceptance.GitUtil.add; +import static com.google.gerrit.acceptance.GitUtil.amendCommit; +import static com.google.gerrit.acceptance.GitUtil.createCommit; +import static com.google.gerrit.acceptance.GitUtil.pushHead; +import static com.google.gerrit.acceptance.GitUtil.rm; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GitUtil.Commit; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; +import com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.Patch.ChangeType; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListEntry; +import com.google.gerrit.server.patch.PatchListKey; +import com.google.gerrit.server.patch.PatchListNotAvailableException; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; + +import org.eclipse.jgit.api.ResetCommand.ResetType; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.lib.ObjectId; +import org.junit.Test; + +import java.io.IOException; +import java.util.List; + +public class PatchListCacheIT extends AbstractDaemonTest { + private static String SUBJECT_1 = "subject 1"; + private static String SUBJECT_2 = "subject 2"; + private static String SUBJECT_3 = "subject 3"; + private static String FILE_A = "a.txt"; + private static String FILE_B = "b.txt"; + private static String FILE_C = "c.txt"; + private static String FILE_D = "d.txt"; + + @Inject + private PatchListCache patchListCache; + + @Test + public void listPatchesAgainstBase() throws GitAPIException, IOException, + PatchListNotAvailableException, OrmException, RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1, 1 (+FILE_A, -FILE_D) + add(git, FILE_A, "1"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + + // Compare Change 1,1 with Base (+FILE_A, -FILE_D) + List entries = getCurrentPatches(c.getChangeId()); + assertEquals(3, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertDeleted(FILE_D, entries.get(2)); + + // Change 1,2 (+FILE_A, +FILE_B, -FILE_D) + add(git, FILE_B, "2"); + c = amendCommit(git, admin.getIdent(), SUBJECT_2, c.getChangeId()); + pushHead(git, "refs/for/master", false); + entries = getCurrentPatches(c.getChangeId()); + + // Compare Change 1,2 with Base (+FILE_A, +FILE_B, -FILE_D) + assertEquals(4, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertAdded(FILE_B, entries.get(2)); + assertDeleted(FILE_D, entries.get(3)); + } + + @Test + public void listPatchesAgainstBaseWithRebase() throws GitAPIException, + IOException, PatchListNotAvailableException, OrmException, + RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1,1 (+FILE_A, -FILE_D) + add(git, FILE_A, "1"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + List entries = getCurrentPatches(c.getChangeId()); + assertEquals(3, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertDeleted(FILE_D, entries.get(2)); + + // Change 2,1 (+FILE_B) + git.reset().setMode(ResetType.HARD).setRef("HEAD~1").call(); + add(git, FILE_B, "2"); + createCommit(git, admin.getIdent(), SUBJECT_3); + pushHead(git, "refs/for/master", false); + + // Change 1,2 (+FILE_A, -FILE_D)) + git.cherryPick().include(c.getCommit()).call(); + pushHead(git, "refs/for/master", false); + + // Compare Change 1,2 with Base (+FILE_A, -FILE_D)) + entries = getCurrentPatches(c.getChangeId()); + assertEquals(3, entries.size()); + assertAdded(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_A, entries.get(1)); + assertDeleted(FILE_D, entries.get(2)); + } + + @Test + public void listPatchesAgainstOtherPatchSet() throws GitAPIException, + IOException, PatchListNotAvailableException, OrmException, + RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1,1 (+FILE_A, +FILE_C, -FILE_D) + add(git, FILE_A, "1"); + add(git, FILE_C, "3"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + ObjectId a = getCurrentRevisionId(c.getChangeId()); + + // Change 1,2 (+FILE_A, +FILE_B, -FILE_D) + add(git, FILE_B, "2"); + rm(git, FILE_C); + c = amendCommit(git, admin.getIdent(), SUBJECT_2, c.getChangeId()); + pushHead(git, "refs/for/master", false); + ObjectId b = getCurrentRevisionId(c.getChangeId()); + + // Compare Change 1,1 with Change 1,2 + // expected: +FILE_B + // actual: +FILE_B, -FILE_C + // -FILE_C is wrongly returned, it is not contained in Change 1,2 + // but was only added in Change 1,1 + List entries = getPatches(a, b); + assertEquals(3, entries.size()); + assertModified(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_B, entries.get(1)); + assertDeleted(FILE_C, entries.get(2)); + } + + @Test + public void listPatchesAgainstOtherPatchSetWithRebase() + throws GitAPIException, IOException, PatchListNotAvailableException, + OrmException, RestApiException { + add(git, FILE_D, "4"); + createCommit(git, admin.getIdent(), SUBJECT_1); + pushHead(git, "refs/heads/master", false); + + // Change 1,1 (+FILE_A, -FILE_D) + add(git, FILE_A, "1"); + rm(git, FILE_D); + Commit c = createCommit(git, admin.getIdent(), SUBJECT_2); + pushHead(git, "refs/for/master", false); + ObjectId a = getCurrentRevisionId(c.getChangeId()); + + // Change 2,1 (+FILE_B) + git.reset().setMode(ResetType.HARD).setRef("HEAD~1").call(); + add(git, FILE_B, "2"); + createCommit(git, admin.getIdent(), SUBJECT_3); + pushHead(git, "refs/for/master", false); + + // Change 1,2 (+FILE_A, +FILE_C, -FILE_D) + git.cherryPick().include(c.getCommit()).call(); + add(git, FILE_C, "2"); + c = amendCommit(git, admin.getIdent(), SUBJECT_2, c.getChangeId()); + pushHead(git, "refs/for/master", false); + ObjectId b = getCurrentRevisionId(c.getChangeId()); + + // Compare Change 1,1 with Change 1,2 + // expected: +FILE_C + // actual: +FILE_B, +FILE_C + // +FILE_B is wrongly returned, it is neither contained in Change 1,1 + // nor in Change 1,2, but was only changed due to the rebase + // on Change 2,1 + List entries = getPatches(a, b); + assertEquals(3, entries.size()); + assertModified(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_B, entries.get(1)); + assertAdded(FILE_C, entries.get(2)); + } + + private static void assertAdded(String expectedNewName, PatchListEntry e) { + assertName(expectedNewName, e); + assertEquals(ChangeType.ADDED, e.getChangeType()); + } + + private static void assertModified(String expectedNewName, PatchListEntry e) { + assertName(expectedNewName, e); + assertEquals(ChangeType.MODIFIED, e.getChangeType()); + } + + private static void assertDeleted(String expectedNewName, PatchListEntry e) { + assertName(expectedNewName, e); + assertEquals(ChangeType.DELETED, e.getChangeType()); + } + + private static void assertName(String expectedNewName, PatchListEntry e) { + assertEquals(expectedNewName, e.getNewName()); + assertNull(e.getOldName()); + } + + private List getCurrentPatches(String changeId) + throws PatchListNotAvailableException, OrmException, RestApiException { + return patchListCache.get(getKey(null, getCurrentRevisionId(changeId))).getPatches(); + } + + private List getPatches(ObjectId revisionIdA, ObjectId revisionIdB) + throws PatchListNotAvailableException, OrmException { + return patchListCache.get(getKey(revisionIdA, revisionIdB)).getPatches(); + } + + private PatchListKey getKey(ObjectId revisionIdA, ObjectId revisionIdB) throws OrmException { + return new PatchListKey(project, revisionIdA, revisionIdB, Whitespace.IGNORE_NONE); + } + + private ObjectId getCurrentRevisionId(String changeId) throws RestApiException { + return ObjectId.fromString(gApi.changes().id(changeId).get().currentRevision); + } +} From eb85036e0a2889ebd5c104ee65422cc0e196efdf Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 16 May 2014 10:56:38 +0200 Subject: [PATCH 2/3] Remove unused TreeWalk in PatchListLoader Change-Id: I6ad54e54fceb2d80fb4be70ea31cddf942f6aefd Signed-off-by: Edwin Kempin --- .../com/google/gerrit/server/patch/PatchListLoader.java | 9 --------- 1 file changed, 9 deletions(-) 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 2cda3346ca..8a73503ae0 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 @@ -51,8 +51,6 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTree; import org.eclipse.jgit.revwalk.RevWalk; -import org.eclipse.jgit.treewalk.TreeWalk; -import org.eclipse.jgit.treewalk.filter.TreeFilter; import org.eclipse.jgit.util.TemporaryBuffer; import org.eclipse.jgit.util.io.DisabledOutputStream; import org.slf4j.Logger; @@ -137,13 +135,6 @@ public class PatchListLoader extends CacheLoader { RevTree bTree = b.getTree(); - final TreeWalk walk = new TreeWalk(reader); - walk.reset(); - walk.setRecursive(true); - walk.addTree(aTree); - walk.addTree(bTree); - walk.setFilter(TreeFilter.ANY_DIFF); - DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE); df.setRepository(repo); df.setDiffComparator(cmp); From 67a2e7ead8c1d8e1ee69be89de36c5db37d5629a Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 12 May 2014 10:43:20 +0200 Subject: [PATCH 3/3] Limit file list to files that were touched in new patch set When comparing two patch sets only files that were touched in the new patch set should be listed. At the moment the file list contains all files that were changed between the commits of the compared patch sets. E.g. if a rebase was done which touched many files this made the patch set comparison pretty useless since the file list grew way too big. Change-Id: I6c512a69d8632a4ce57ef23928411bf0829aeb70 Signed-off-by: Edwin Kempin --- .../server/change/PatchListCacheIT.java | 21 ++----- .../gerrit/server/patch/PatchListKey.java | 2 +- .../gerrit/server/patch/PatchListLoader.java | 60 ++++++++++++------- 3 files changed, 43 insertions(+), 40 deletions(-) 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 4088cd1284..70a0a602ba 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 @@ -149,16 +149,11 @@ public class PatchListCacheIT extends AbstractDaemonTest { pushHead(git, "refs/for/master", false); ObjectId b = getCurrentRevisionId(c.getChangeId()); - // Compare Change 1,1 with Change 1,2 - // expected: +FILE_B - // actual: +FILE_B, -FILE_C - // -FILE_C is wrongly returned, it is not contained in Change 1,2 - // but was only added in Change 1,1 + // Compare Change 1,1 with Change 1,2 (+FILE_B) List entries = getPatches(a, b); - assertEquals(3, entries.size()); + assertEquals(2, entries.size()); assertModified(Patch.COMMIT_MSG, entries.get(0)); assertAdded(FILE_B, entries.get(1)); - assertDeleted(FILE_C, entries.get(2)); } @Test @@ -189,17 +184,11 @@ public class PatchListCacheIT extends AbstractDaemonTest { pushHead(git, "refs/for/master", false); ObjectId b = getCurrentRevisionId(c.getChangeId()); - // Compare Change 1,1 with Change 1,2 - // expected: +FILE_C - // actual: +FILE_B, +FILE_C - // +FILE_B is wrongly returned, it is neither contained in Change 1,1 - // nor in Change 1,2, but was only changed due to the rebase - // on Change 2,1 + // Compare Change 1,1 with Change 1,2 (+FILE_C) List entries = getPatches(a, b); - assertEquals(3, entries.size()); + assertEquals(2, entries.size()); assertModified(Patch.COMMIT_MSG, entries.get(0)); - assertAdded(FILE_B, entries.get(1)); - assertAdded(FILE_C, entries.get(2)); + assertAdded(FILE_C, entries.get(1)); } private static void assertAdded(String expectedNewName, PatchListEntry e) { 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 c36fbc0096..ff038403cc 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 @@ -34,7 +34,7 @@ import java.io.ObjectOutputStream; import java.io.Serializable; public class PatchListKey implements Serializable { - static final long serialVersionUID = 16L; + static final long serialVersionUID = 17L; private transient ObjectId oldId; private transient ObjectId newId; 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 8a73503ae0..744c379402 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 @@ -15,7 +15,9 @@ package com.google.gerrit.server.patch; +import com.google.common.base.Function; import com.google.common.cache.CacheLoader; +import com.google.common.collect.FluentIterable; import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.RefNames; @@ -58,23 +60,28 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.io.InputStream; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; public class PatchListLoader extends CacheLoader { static final Logger log = LoggerFactory.getLogger(PatchListLoader.class); private final GitRepositoryManager repoManager; + private final PatchListCache patchListCache; @Inject - PatchListLoader(GitRepositoryManager mgr) { + PatchListLoader(GitRepositoryManager mgr, PatchListCache plc) { repoManager = mgr; + patchListCache = plc; } @Override - public PatchList load(final PatchListKey key) throws Exception { + public PatchList load(final PatchListKey key) throws IOException, + PatchListNotAvailableException { final Repository repo = repoManager.openRepository(key.projectKey); try { return readPatchList(key, repo); @@ -100,8 +107,8 @@ public class PatchListLoader extends CacheLoader { } } - private PatchList readPatchList(final PatchListKey key, - final Repository repo) throws IOException { + private PatchList readPatchList(final PatchListKey key, final Repository repo) + throws IOException, PatchListNotAvailableException { final RawTextComparator cmp = comparatorFor(key.getWhitespace()); final ObjectReader reader = repo.newObjectReader(); try { @@ -121,18 +128,8 @@ public class PatchListLoader extends CacheLoader { final boolean againstParent = b.getParentCount() > 0 && b.getParent(0) == a; - RevCommit aCommit; - RevTree aTree; - if (a instanceof RevCommit) { - aCommit = (RevCommit) a; - aTree = aCommit.getTree(); - } else if (a instanceof RevTree) { - aCommit = null; - aTree = (RevTree) a; - } else { - throw new IOException("Unexpected type: " + a.getClass()); - } - + RevCommit aCommit = a instanceof RevCommit ? (RevCommit) a : null; + RevTree aTree = rw.parseTree(a); RevTree bTree = b.getTree(); DiffFormatter df = new DiffFormatter(DisabledOutputStream.INSTANCE); @@ -141,15 +138,32 @@ public class PatchListLoader extends CacheLoader { df.setDetectRenames(true); List diffEntries = df.scan(aTree, bTree); - final int cnt = diffEntries.size(); - final PatchListEntry[] entries = new PatchListEntry[1 + cnt]; - entries[0] = newCommitMessage(cmp, repo, reader, // - againstParent ? null : aCommit, b); + Set paths = key.getOldId() != null + ? FluentIterable.from(patchListCache.get( + new PatchListKey(key.projectKey, null, key.getNewId(), + key.getWhitespace())).getPatches()) + .transform(new Function() { + @Override + public String apply(PatchListEntry entry) { + return entry.getNewName(); + } + }) + .toSet() + : null; + int cnt = diffEntries.size(); + List entries = new ArrayList<>(); + entries.add(newCommitMessage(cmp, repo, reader, // + againstParent ? null : aCommit, b)); for (int i = 0; i < cnt; i++) { - FileHeader fh = df.toFileHeader(diffEntries.get(i)); - entries[1 + i] = newEntry(aTree, fh); + DiffEntry diffEntry = diffEntries.get(i); + if (paths == null || paths.contains(diffEntry.getNewPath()) + || paths.contains(diffEntry.getOldPath())) { + FileHeader fh = df.toFileHeader(diffEntry); + entries.add(newEntry(aTree, fh)); + } } - return new PatchList(a, b, againstParent, entries); + return new PatchList(a, b, againstParent, + entries.toArray(new PatchListEntry[entries.size()])); } finally { reader.release(); }