Clean up PreviewSubmit and tests * 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
diff --git a/gerrit-acceptance-framework/BUILD b/gerrit-acceptance-framework/BUILD index db5a300..e6f2a79 100644 --- a/gerrit-acceptance-framework/BUILD +++ b/gerrit-acceptance-framework/BUILD
@@ -48,6 +48,7 @@ "//lib/httpcomponents:httpcore", "//lib/jetty:servlet", "//lib/jgit/org.eclipse.jgit.junit:junit", + "//lib:jimfs", "//lib/log:impl_log4j", "//lib/log:log4j", ],
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 2cc64d8..0519862 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -29,8 +29,8 @@ 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.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.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 @@ 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 @@ 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/*");
diff --git a/gerrit-acceptance-tests/BUILD b/gerrit-acceptance-tests/BUILD index 3154b1f..4c1a62d 100644 --- a/gerrit-acceptance-tests/BUILD +++ b/gerrit-acceptance-tests/BUILD
@@ -27,6 +27,7 @@ "//lib:gwtjsonrpc", "//lib:gwtorm", "//lib:h2", + "//lib:jimfs", "//lib:jsch", "//lib:servlet-api-3_1-without-neverlink", "//lib/bouncycastle:bcpg",
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java index cfc5937..5dceff9 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
@@ -22,7 +22,6 @@ 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.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 @@ 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
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 81f136d..6344cbe 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -93,7 +93,6 @@ 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 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 @@ } submit(change.getChangeId()); - assertRevTrees(project, actual); + assertTrees(project, actual); } @Test @@ -205,57 +198,59 @@ 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 @@ 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 @@ // 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 @@ 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")
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java index 96b982d..04151e9 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
@@ -41,8 +41,8 @@ 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 @@ 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 @@ 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());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java index 3fc9789..90c2daf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java
@@ -111,11 +111,13 @@ 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 @@ @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);