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..70a0a602ba --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/PatchListCacheIT.java @@ -0,0 +1,231 @@ +// 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 (+FILE_B) + List entries = getPatches(a, b); + assertEquals(2, entries.size()); + assertModified(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_B, entries.get(1)); + } + + @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 (+FILE_C) + List entries = getPatches(a, b); + assertEquals(2, entries.size()); + assertModified(Patch.COMMIT_MSG, entries.get(0)); + assertAdded(FILE_C, entries.get(1)); + } + + 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); + } +} 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 2cda3346ca..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; @@ -51,8 +53,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; @@ -60,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); @@ -102,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 { @@ -123,42 +128,42 @@ 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(); - 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); 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(); }