opendev/gerrit
Code Issues Proposed changes

Clean up PreviewSubmit and tests

Browse Source 
* Fix Eclipse warnings about unclosed resources and missing case blocks.
* Use Jimfs instead of a real file on the filesystem for storing
  temporary zip files. This is a moderate rewrite to use NIO, because
  ZipFile doesn't support Paths.
* Return ObjectId in the fetchFromBundles map. Using RevObjects after
  the lifetime of their RevWalks can lead to difficult-to-debug errors,
  and in this case we don't need anything but the ID.
* Rename assertRevTrees to assertTrees, since it no longer refers to
  RevTrees.
* Add convenience method to run SubmitPreview in a try-with-resources
  block and return the Map result.
* Run google-java-format over all affected files.

Change-Id: I251f390f81cb3372e5b3243d6fbe7e2365bd2dcb
This commit is contained in:
Dave Borowitz 2017-02-22 11:27:49 -05:00
parent 0fed7e848b
commit 262857f297
7 changed files with 103 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
gerrit-acceptance-framework/BUILD
View File

@ -48,6 +48,7 @@ java_library2(
"//lib/httpcomponents:httpcore",
"//lib/jetty:servlet",
"//lib/jgit/org.eclipse.jgit.junit:junit",
"//lib:jimfs",
"//lib/log:impl_log4j",
"//lib/log:log4j",
],

65
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@ -29,8 +29,8 @@ import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Iterators;
import com.google.common.collect.Sets;
import com.google.common.jimfs.Jimfs;
import com.google.common.primitives.Chars;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
import com.google.gerrit.common.Nullable;
@ -107,10 +107,14 @@ import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.DirectoryStream;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@ -121,8 +125,6 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;
import org.eclipse.jgit.api.Git;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@ -1033,33 +1035,51 @@ public abstract class AbstractDaemonTest {
return ca;
}
protected BinaryResult submitPreview(String changeId) throws Exception {
return gApi.changes().id(changeId).current().submitPreview();
}
protected BinaryResult submitPreview(String changeId, String format) throws Exception {
return gApi.changes().id(changeId).current().submitPreview(format);
}
protected Map<Branch.NameKey, ObjectId> fetchFromSubmitPreview(String changeId) throws Exception {
try (BinaryResult result = submitPreview(changeId)) {
return fetchFromBundles(result);
}
}
/**
* Fetches each bundle into a newly cloned repository, then it applies the bundle, and returns the
* resulting tree id.
*/
protected Map<Branch.NameKey, RevTree> fetchFromBundles(BinaryResult bundles) throws Exception {
protected Map<Branch.NameKey, ObjectId> fetchFromBundles(BinaryResult bundles) throws Exception {
assertThat(bundles.getContentType()).isEqualTo("application/x-zip");
File tempfile = File.createTempFile("test", null);
bundles.writeTo(new FileOutputStream(tempfile));
Map<Branch.NameKey, RevTree> ret = new HashMap<>();
try (ZipFile readback = new ZipFile(tempfile); ) {
for (ZipEntry entry : ImmutableList.copyOf(Iterators.forEnumeration(readback.entries()))) {
String bundleName = entry.getName();
InputStream bundleStream = readback.getInputStream(entry);
FileSystem fs = Jimfs.newFileSystem();
Path previewPath = fs.getPath("preview.zip");
try (OutputStream out = Files.newOutputStream(previewPath)) {
bundles.writeTo(out);
}
Map<Branch.NameKey, ObjectId> ret = new HashMap<>();
try (FileSystem zipFs = FileSystems.newFileSystem(previewPath, null);
DirectoryStream<Path> dirStream =
Files.newDirectoryStream(Iterables.getOnlyElement(zipFs.getRootDirectories()))) {
for (Path p : dirStream) {
if (!Files.isRegularFile(p)) {
continue;
}
String bundleName = p.getFileName().toString();
int len = bundleName.length();
assertThat(bundleName).endsWith(".git");
String repoName = bundleName.substring(0, len - 4);
Project.NameKey proj = new Project.NameKey(repoName);
TestRepository<?> localRepo = cloneProject(proj);
try (TransportBundleStream tbs =
new TransportBundleStream(
localRepo.getRepository(), new URIish(bundleName), bundleStream); ) {
try (InputStream bundleStream = Files.newInputStream(p);
TransportBundleStream tbs =
new TransportBundleStream(
localRepo.getRepository(), new URIish(bundleName), bundleStream)) {
FetchResult fr =
tbs.fetch(
NullProgressMonitor.INSTANCE,
@ -1069,16 +1089,17 @@ public abstract class AbstractDaemonTest {
Branch.NameKey n = new Branch.NameKey(proj, branchName);
RevCommit c = localRepo.getRevWalk().parseCommit(r.getObjectId());
ret.put(n, c.getTree());
ret.put(n, c.getTree().copy());
}
}
}
}
assertThat(ret).isNotEmpty();
return ret;
}
/** Assert that the given branches have the given tree ids. */
protected void assertRevTrees(Project.NameKey proj, Map<Branch.NameKey, RevTree> trees)
protected void assertTrees(Project.NameKey proj, Map<Branch.NameKey, ObjectId> trees)
throws Exception {
TestRepository<?> localRepo = cloneProject(proj);
GitUtil.fetch(localRepo, "refs/*:refs/*");

1
gerrit-acceptance-tests/BUILD
View File

@ -27,6 +27,7 @@ java_library(
"//lib:gwtjsonrpc",
"//lib:gwtorm",
"//lib:h2",
"//lib:jimfs",
"//lib:jsch",
"//lib:servlet-api-3_1-without-neverlink",
"//lib/bouncycastle:bcpg",

7
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
View File

@ -22,7 +22,6 @@ import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.testutil.ConfigSuite;
@ -31,7 +30,6 @@ import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.transport.RefSpec;
import org.junit.Test;
@ -136,10 +134,7 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT extends AbstractSubmoduleSu
gApi.changes().id(id2).current().review(ReviewInput.approve());
gApi.changes().id(id3).current().review(ReviewInput.approve());
Map<Branch.NameKey, RevTree> preview;
try (BinaryResult request = gApi.changes().id(id1).current().submitPreview()) {
preview = fetchFromBundles(request);
}
Map<Branch.NameKey, ObjectId> preview = fetchFromSubmitPreview(id1);
gApi.changes().id(id1).current().submit();
ObjectId subRepoId =
subRepo

110
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@ -93,7 +93,6 @@ import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.After;
import org.junit.Before;
@ -150,27 +149,21 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
public void submitToEmptyRepo() throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change = createChange();
Map<Branch.NameKey, RevTree> actual;
try (BinaryResult request = submitPreview(change.getChangeId())) {
actual = fetchFromBundles(request);
}
Map<Branch.NameKey, ObjectId> actual = fetchFromSubmitPreview(change.getChangeId());
RevCommit headAfterSubmitPreview = getRemoteHead();
assertThat(headAfterSubmitPreview).isEqualTo(initialHead);
assertThat(actual).hasSize(1);
submit(change.getChangeId());
assertThat(getRemoteHead().getId()).isEqualTo(change.getCommit());
assertRevTrees(project, actual);
assertTrees(project, actual);
}
@Test
public void submitSingleChange() throws Exception {
RevCommit initialHead = getRemoteHead();
PushOneCommit.Result change = createChange();
Map<Branch.NameKey, RevTree> actual;
try (BinaryResult request = submitPreview(change.getChangeId())) {
actual = fetchFromBundles(request);
}
Map<Branch.NameKey, ObjectId> actual = fetchFromSubmitPreview(change.getChangeId());
RevCommit headAfterSubmit = getRemoteHead();
assertThat(headAfterSubmit).isEqualTo(initialHead);
assertRefUpdatedEvents();
@ -185,7 +178,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
}
submit(change.getChangeId());
assertRevTrees(project, actual);
assertTrees(project, actual);
}
@Test
@ -205,57 +198,59 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
try (BinaryResult request = submitPreview(change4.getChangeId())) {
assertThat(getSubmitType()).isEqualTo(SubmitType.CHERRY_PICK);
Map<Branch.NameKey, RevTree> s = fetchFromBundles(request);
submit(change4.getChangeId());
assertRevTrees(project, s);
} catch (RestApiException e) {
switch (getSubmitType()) {
case FAST_FORWARD_ONLY:
assertThat(e.getMessage()).isEqualTo(
"Failed to submit 3 changes due to the following problems:\n"
+ "Change "
+ change2.getChange().getId()
+ ": internal error: "
+ "change not processed by merge strategy\n"
+ "Change "
+ change3.getChange().getId()
+ ": internal error: "
+ "change not processed by merge strategy\n"
+ "Change "
+ change4.getChange().getId()
+ ": Project policy "
+ "requires all submissions to be a fast-forward. Please "
+ "rebase the change locally and upload again for review.");
assertThat(e.getMessage())
.isEqualTo(
"Failed to submit 3 changes due to the following problems:\n"
+ "Change "
+ change2.getChange().getId()
+ ": internal error: "
+ "change not processed by merge strategy\n"
+ "Change "
+ change3.getChange().getId()
+ ": internal error: "
+ "change not processed by merge strategy\n"
+ "Change "
+ change4.getChange().getId()
+ ": Project policy "
+ "requires all submissions to be a fast-forward. Please "
+ "rebase the change locally and upload again for review.");
break;
case REBASE_IF_NECESSARY:
case REBASE_ALWAYS:
String change2hash = change2.getChange().currentPatchSet().getRevision().get();
assertThat(e.getMessage()).isEqualTo(
"Cannot rebase "
+ change2hash
+ ": The change could "
+ "not be rebased due to a conflict during merge.");
assertThat(e.getMessage())
.isEqualTo(
"Cannot rebase "
+ change2hash
+ ": The change could "
+ "not be rebased due to a conflict during merge.");
break;
case MERGE_ALWAYS:
case MERGE_IF_NECESSARY:
assertThat(e.getMessage()).isEqualTo(
"Failed to submit 3 changes due to the following problems:\n"
+ "Change "
+ change2.getChange().getId()
+ ": Change could not be "
+ "merged due to a path conflict. Please rebase the change "
+ "locally and upload the rebased commit for review.\n"
+ "Change "
+ change3.getChange().getId()
+ ": Change could not be "
+ "merged due to a path conflict. Please rebase the change "
+ "locally and upload the rebased commit for review.\n"
+ "Change "
+ change4.getChange().getId()
+ ": Change could not be "
+ "merged due to a path conflict. Please rebase the change "
+ "locally and upload the rebased commit for review.");
assertThat(e.getMessage())
.isEqualTo(
"Failed to submit 3 changes due to the following problems:\n"
+ "Change "
+ change2.getChange().getId()
+ ": Change could not be "
+ "merged due to a path conflict. Please rebase the change "
+ "locally and upload the rebased commit for review.\n"
+ "Change "
+ change3.getChange().getId()
+ ": Change could not be "
+ "merged due to a path conflict. Please rebase the change "
+ "locally and upload the rebased commit for review.\n"
+ "Change "
+ change4.getChange().getId()
+ ": Change could not be "
+ "merged due to a path conflict. Please rebase the change "
+ "locally and upload the rebased commit for review.");
break;
case CHERRY_PICK:
default:
fail("Should not reach here.");
break;
@ -276,10 +271,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
PushOneCommit.Result change4 = createChange("Change 4", "e", "e");
// change 2 is not approved, but we ignore labels
approve(change3.getChangeId());
Map<Branch.NameKey, RevTree> actual;
try (BinaryResult request = submitPreview(change4.getChangeId())) {
actual = fetchFromBundles(request);
}
Map<Branch.NameKey, ObjectId> actual = fetchFromSubmitPreview(change4.getChangeId());
Map<String, Map<String, Integer>> expected = new HashMap<>();
expected.put(project.get(), new HashMap<>());
expected.get(project.get()).put("refs/heads/master", 3);
@ -306,7 +298,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
// now check we actually have the same content:
approve(change2.getChangeId());
submit(change4.getChangeId());
assertRevTrees(project, actual);
assertTrees(project, actual);
}
@Test
@ -842,14 +834,6 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
assertMerged(change.changeId);
}
protected BinaryResult submitPreview(String changeId) throws Exception {
return gApi.changes().id(changeId).current().submitPreview();
}
protected BinaryResult submitPreview(String changeId, String format) throws Exception {
return gApi.changes().id(changeId).current().submitPreview(format);
}
protected void assertSubmittable(String changeId) throws Exception {
assertThat(get(changeId, SUBMITTABLE).submittable)
.named("submit bit on ChangeInfo")

13
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
View File

@ -41,8 +41,8 @@ import org.apache.commons.compress.archivers.ArchiveStreamFactory;
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.transport.RefSpec;
import org.junit.Test;
@ -171,10 +171,7 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
approve(change3.getChangeId());
// get a preview before submitting:
Map<Branch.NameKey, RevTree> preview;
try (BinaryResult request = submitPreview(change1b.getChangeId())) {
preview = fetchFromBundles(request);
}
Map<Branch.NameKey, ObjectId> preview = fetchFromSubmitPreview(change1b.getChangeId());
submit(change1b.getChangeId());
RevCommit tip1 = getRemoteLog(p1, "master").get(0);
@ -191,13 +188,13 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
assertThat(preview).hasSize(3);
assertThat(preview).containsKey(new Branch.NameKey(p1, "refs/heads/master"));
assertRevTrees(p1, preview);
assertTrees(p1, preview);
assertThat(preview).containsKey(new Branch.NameKey(p2, "refs/heads/master"));
assertRevTrees(p2, preview);
assertTrees(p2, preview);
assertThat(preview).containsKey(new Branch.NameKey(p3, "refs/heads/master"));
assertRevTrees(p3, preview);
assertTrees(p3, preview);
} else {
assertThat(tip2.getShortMessage()).isEqualTo(initialHead2.getShortMessage());
assertThat(tip3.getShortMessage()).isEqualTo(initialHead3.getShortMessage());

9
gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java
View File

@ -111,11 +111,13 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
IdentifiedUser caller = control.getUser().asIdentifiedUser();
Change change = rsrc.getChange();
final MergeOp op = mergeOpProvider.get();
@SuppressWarnings("resource") // Returned BinaryResult takes ownership and handles closing.
MergeOp op = mergeOpProvider.get();
try {
op.merge(db, change, caller, false, new SubmitInput(), true);
BinaryResult bin = new SubmitPreviewResult(op, f, maxBundleSize);
bin.disableGzip().setContentType(f.getMimeType())
bin.disableGzip()
.setContentType(f.getMimeType())
.setAttachmentName("submit-preview-" + change.getChangeId() + "." + format);
return bin;
} catch (OrmException | RestApiException | RuntimeException e) {
@ -138,8 +140,7 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
@Override
public void writeTo(OutputStream out) throws IOException {
try (ArchiveOutputStream aos = archiveFormat
.createArchiveOutputStream(out)) {
try (ArchiveOutputStream aos = archiveFormat.createArchiveOutputStream(out)) {
MergeOpRepoManager orm = mergeOp.getMergeOpRepoManager();
for (Project.NameKey p : mergeOp.getAllProjects()) {
OpenRepo or = orm.getRepo(p);