opendev/gerrit
Code Issues Proposed changes

Merge changes I6c512a69,I6ad54e54,I1ed59dc2 into stable-2.9

Browse Source 
* changes:
  Limit file list to files that were touched in new patch set
  Remove unused TreeWalk in PatchListLoader
  Add basic test for PatchListCache that checks the returned patches
This commit is contained in:
Shawn Pearce
2014-05-20 21:38:41 +00:00
committed by Gerrit Code Review
parent 4d420fd87c 67a2e7ead8
commit 809835a62b
3 changed files with 269 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -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<PatchListEntry> 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<PatchListEntry> 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<PatchListEntry> 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<PatchListEntry> 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<PatchListEntry> getCurrentPatches(String changeId)
throws PatchListNotAvailableException, OrmException, RestApiException {
return patchListCache.get(getKey(null, getCurrentRevisionId(changeId))).getPatches();
}
private List<PatchListEntry> 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);
}
}

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

@@ -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;

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

@@ -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<PatchListKey, PatchList> {
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<PatchListKey, PatchList> {
}
}
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<PatchListKey, PatchList> {
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<DiffEntry> 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<String> paths = key.getOldId() != null
? FluentIterable.from(patchListCache.get(
new PatchListKey(key.projectKey, null, key.getNewId(),
key.getWhitespace())).getPatches())
.transform(new Function<PatchListEntry, String>() {
@Override
public String apply(PatchListEntry entry) {
return entry.getNewName();
}
})
.toSet()
: null;
int cnt = diffEntries.size();
List<PatchListEntry> 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();
}