Merged in bugfix/postpone_writing_to_stream (pull request #322)

Postpone writing to output stream in diff command

scm-core/src/main/java/sonia/scm/repository/api/DiffCommandBuilder.java

@@ -40,7 +40,6 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.repository.Feature;
 import sonia.scm.repository.spi.DiffCommand;
-import sonia.scm.util.IOUtil;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -103,16 +102,12 @@ public final class DiffCommandBuilder extends AbstractDiffCommandBuilder<DiffCom
    * Passes the difference of the given parameter to the outputstream.
    *
    *
-   * @param outputStream outputstream for the difference
+   * @return A consumer that expects the output stream for the difference
-   *
-   * @return {@code this}
    *
    * @throws IOException
    */
-  public DiffCommandBuilder retrieveContent(OutputStream outputStream) throws IOException {
+  public OutputStreamConsumer retrieveContent() throws IOException {
-    getDiffResult(outputStream);
+    return getDiffResult();
-
-    return this;
   }
 
   //~--- get methods ----------------------------------------------------------
@@ -125,21 +120,10 @@ public final class DiffCommandBuilder extends AbstractDiffCommandBuilder<DiffCom
    * @throws IOException
    */
   public String getContent() throws IOException {
-    String content = null;
+    try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
-    ByteArrayOutputStream baos = null;
+      getDiffResult();
-
+      return baos.toString();
-    try
-    {
-      baos = new ByteArrayOutputStream();
-      getDiffResult(baos);
-      content = baos.toString();
     }
-    finally
-    {
-      IOUtil.close(baos);
-    }
-
-    return content;
   }
 
   //~--- set methods ----------------------------------------------------------
@@ -169,25 +153,25 @@ public final class DiffCommandBuilder extends AbstractDiffCommandBuilder<DiffCom
    * Method description
    *
    *
-   * @param outputStream
-   *
    * @throws IOException
+   * @return
    */
-  private void getDiffResult(OutputStream outputStream) throws IOException {
+  private OutputStreamConsumer getDiffResult() throws IOException {
-    Preconditions.checkNotNull(outputStream, "OutputStream is required");
     Preconditions.checkArgument(request.isValid(),
       "path and/or revision is required");
 
-    if (logger.isDebugEnabled())
-    {
     logger.debug("create diff for {}", request);
-    }
 
-    diffCommand.getDiffResult(request, outputStream);
+    return diffCommand.getDiffResult(request);
   }
 
   @Override
   DiffCommandBuilder self() {
     return this;
   }
+
+  @FunctionalInterface
+  public interface OutputStreamConsumer {
+    void accept(OutputStream outputStream) throws IOException;
+  }
 }

scm-core/src/main/java/sonia/scm/repository/spi/DiffCommand.java

@@ -33,8 +33,9 @@
 
 package sonia.scm.repository.spi;
 
+import sonia.scm.repository.api.DiffCommandBuilder;
+
 import java.io.IOException;
-import java.io.OutputStream;
 
 /**
  *
@@ -49,10 +50,9 @@ public interface DiffCommand
    *
    *
    * @param request
-   * @param output
-   *
    * @throws IOException
    * @throws RuntimeException
+   * @return
    */
-  public void getDiffResult(DiffCommandRequest request, OutputStream output) throws IOException;
+  DiffCommandBuilder.OutputStreamConsumer getDiffResult(DiffCommandRequest request) throws IOException;
 }

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitUtil.java

@@ -62,6 +62,7 @@ import org.eclipse.jgit.util.LfsFactory;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.ContextEntry;
+import sonia.scm.ScmConstraintViolationException;
 import sonia.scm.util.HttpUtil;
 import sonia.scm.util.Util;
 import sonia.scm.web.GitUserAgentProvider;
@@ -76,6 +77,7 @@ import java.util.concurrent.TimeUnit;
 
 import static java.util.Optional.empty;
 import static java.util.Optional.of;
+import static sonia.scm.ScmConstraintViolationException.Builder.doThrow;
 
 //~--- JDK imports ------------------------------------------------------------
 
@@ -733,7 +735,11 @@ public final class GitUtil
       mergeBaseWalk.setRevFilter(RevFilter.MERGE_BASE);
       mergeBaseWalk.markStart(mergeBaseWalk.lookupCommit(revision1));
       mergeBaseWalk.markStart(mergeBaseWalk.parseCommit(revision2));
-      return mergeBaseWalk.next().getId();
+      RevCommit ancestor = mergeBaseWalk.next();
+      doThrow()
+        .violation("revisions " + revision1.name() + " and " + revision2.name() + " are not related and therefore do not have a common ancestor", "revisions")
+        .when(ancestor == null);
+      return ancestor.getId();
     }
   }
 

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitDiffCommand.java

@@ -31,15 +31,12 @@
 
 package sonia.scm.repository.spi;
 
-//~--- non-JDK imports --------------------------------------------------------
-
 import org.eclipse.jgit.diff.DiffEntry;
 import org.eclipse.jgit.diff.DiffFormatter;
 import sonia.scm.repository.Repository;
+import sonia.scm.repository.api.DiffCommandBuilder;
 
-import java.io.BufferedOutputStream;
 import java.io.IOException;
-import java.io.OutputStream;
 
 /**
  *
@@ -52,14 +49,16 @@ public class GitDiffCommand extends AbstractGitCommand implements DiffCommand {
   }
 
   @Override
-  public void getDiffResult(DiffCommandRequest request, OutputStream output) throws IOException {
+  public DiffCommandBuilder.OutputStreamConsumer getDiffResult(DiffCommandRequest request) throws IOException {
     @SuppressWarnings("squid:S2095") // repository will be closed with the RepositoryService
     org.eclipse.jgit.lib.Repository repository = open();
-    try (DiffFormatter formatter = new DiffFormatter(new BufferedOutputStream(output))) {
-      formatter.setRepository(repository);
 
     Differ.Diff diff = Differ.diff(repository, request);
 
+    return output -> {
+      try (DiffFormatter formatter = new DiffFormatter(output)) {
+        formatter.setRepository(repository);
+
         for (DiffEntry e : diff.getEntries()) {
           if (!e.getOldId().equals(e.getNewId())) {
             formatter.format(e);
@@ -68,6 +67,7 @@ public class GitDiffCommand extends AbstractGitCommand implements DiffCommand {
 
         formatter.flush();
       }
+    };
   }
 
 }

scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitDiffCommandTest.java

@@ -44,7 +44,7 @@ public class GitDiffCommandTest extends AbstractGitCommandTestBase {
     DiffCommandRequest diffCommandRequest = new DiffCommandRequest();
     diffCommandRequest.setRevision("3f76a12f08a6ba0dc988c68b7f0b2cd190efc3c4");
     ByteArrayOutputStream output = new ByteArrayOutputStream();
-    gitDiffCommand.getDiffResult(diffCommandRequest, output);
+    gitDiffCommand.getDiffResult(diffCommandRequest).accept(output);
     assertEquals(DIFF_FILE_A + DIFF_FILE_B, output.toString());
   }
 
@@ -54,7 +54,7 @@ public class GitDiffCommandTest extends AbstractGitCommandTestBase {
     DiffCommandRequest diffCommandRequest = new DiffCommandRequest();
     diffCommandRequest.setRevision("test-branch");
     ByteArrayOutputStream output = new ByteArrayOutputStream();
-    gitDiffCommand.getDiffResult(diffCommandRequest, output);
+    gitDiffCommand.getDiffResult(diffCommandRequest).accept(output);
     assertEquals(DIFF_FILE_A + DIFF_FILE_B, output.toString());
   }
 
@@ -65,7 +65,7 @@ public class GitDiffCommandTest extends AbstractGitCommandTestBase {
     diffCommandRequest.setRevision("test-branch");
     diffCommandRequest.setPath("a.txt");
     ByteArrayOutputStream output = new ByteArrayOutputStream();
-    gitDiffCommand.getDiffResult(diffCommandRequest, output);
+    gitDiffCommand.getDiffResult(diffCommandRequest).accept(output);
     assertEquals(DIFF_FILE_A, output.toString());
   }
 
@@ -76,7 +76,7 @@ public class GitDiffCommandTest extends AbstractGitCommandTestBase {
     diffCommandRequest.setRevision("master");
     diffCommandRequest.setAncestorChangeset("test-branch");
     ByteArrayOutputStream output = new ByteArrayOutputStream();
-    gitDiffCommand.getDiffResult(diffCommandRequest, output);
+    gitDiffCommand.getDiffResult(diffCommandRequest).accept(output);
     assertEquals(DIFF_FILE_A_MULTIPLE_REVISIONS + DIFF_FILE_F_MULTIPLE_REVISIONS, output.toString());
   }
 
@@ -88,7 +88,7 @@ public class GitDiffCommandTest extends AbstractGitCommandTestBase {
     diffCommandRequest.setAncestorChangeset("test-branch");
     diffCommandRequest.setPath("a.txt");
     ByteArrayOutputStream output = new ByteArrayOutputStream();
-    gitDiffCommand.getDiffResult(diffCommandRequest, output);
+    gitDiffCommand.getDiffResult(diffCommandRequest).accept(output);
     assertEquals(DIFF_FILE_A_MULTIPLE_REVISIONS, output.toString());
   }
 }

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/HgDiffCommand.java

@@ -39,13 +39,12 @@ import com.google.common.base.Strings;
 import com.google.common.io.ByteStreams;
 import com.google.common.io.Closeables;
 import sonia.scm.repository.Repository;
+import sonia.scm.repository.api.DiffCommandBuilder;
 import sonia.scm.repository.api.DiffFormat;
 import sonia.scm.repository.spi.javahg.HgDiffInternalCommand;
 import sonia.scm.web.HgUtil;
 
-import java.io.IOException;
 import java.io.InputStream;
-import java.io.OutputStream;
 
 //~--- JDK imports ------------------------------------------------------------
 
@@ -71,9 +70,9 @@ public class HgDiffCommand extends AbstractCommand implements DiffCommand
   //~--- get methods ----------------------------------------------------------
 
   @Override
-  public void getDiffResult(DiffCommandRequest request, OutputStream output)
+  public DiffCommandBuilder.OutputStreamConsumer getDiffResult(DiffCommandRequest request)
-    throws IOException
   {
+    return output -> {
       com.aragost.javahg.Repository hgRepo = open();
 
       HgDiffInternalCommand cmd = HgDiffInternalCommand.on(hgRepo);
@@ -88,24 +87,19 @@ public class HgDiffCommand extends AbstractCommand implements DiffCommand
 
       InputStream inputStream = null;
 
-    try
+      try {
-    {
 
-      if (!Strings.isNullOrEmpty(request.getPath()))
+        if (!Strings.isNullOrEmpty(request.getPath())) {
-      {
           inputStream = cmd.stream(hgRepo.file(request.getPath()));
-      }
+        } else {
-      else
-      {
           inputStream = cmd.stream();
         }
 
         ByteStreams.copy(inputStream, output);
 
-    }
+      } finally {
-    finally
-    {
         Closeables.close(inputStream, true);
       }
+    };
   }
 }

scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnDiffCommand.java

@@ -46,11 +46,10 @@ import org.tmatesoft.svn.core.wc.SVNRevision;
 import sonia.scm.repository.InternalRepositoryException;
 import sonia.scm.repository.Repository;
 import sonia.scm.repository.SvnUtil;
+import sonia.scm.repository.api.DiffCommandBuilder;
 import sonia.scm.repository.api.DiffFormat;
 import sonia.scm.util.Util;
 
-import java.io.OutputStream;
-
 //~--- JDK imports ------------------------------------------------------------
 
 /**
@@ -70,12 +69,12 @@ public class SvnDiffCommand extends AbstractSvnCommand implements DiffCommand {
   }
 
   @Override
-  public void getDiffResult(DiffCommandRequest request, OutputStream output) {
+  public DiffCommandBuilder.OutputStreamConsumer getDiffResult(DiffCommandRequest request) {
     logger.debug("create diff for {}", request);
     Preconditions.checkNotNull(request, "request is required");
-    Preconditions.checkNotNull(output, "outputstream is required");
 
     String path = request.getPath();
+    return output -> {
       SVNClientManager clientManager = null;
       try {
         SVNURL svnurl = context.createUrl();
@@ -98,5 +97,6 @@ public class SvnDiffCommand extends AbstractSvnCommand implements DiffCommand {
       } finally {
         SvnUtil.dispose(clientManager);
       }
+    };
   }
 }

scm-webapp/src/main/java/sonia/scm/api/v2/resources/DiffRootResource.java

@@ -4,6 +4,7 @@ import com.webcohesion.enunciate.metadata.rs.ResponseCode;
 import com.webcohesion.enunciate.metadata.rs.StatusCodes;
 import sonia.scm.NotFoundException;
 import sonia.scm.repository.NamespaceAndName;
+import sonia.scm.repository.api.DiffCommandBuilder;
 import sonia.scm.repository.api.DiffFormat;
 import sonia.scm.repository.api.RepositoryService;
 import sonia.scm.repository.api.RepositoryServiceFactory;
@@ -20,6 +21,7 @@ import javax.ws.rs.Produces;
 import javax.ws.rs.QueryParam;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.StreamingOutput;
+import java.io.IOException;
 
 public class DiffRootResource {
 
@@ -55,20 +57,17 @@ public class DiffRootResource {
     @ResponseCode(code = 404, condition = "not found, no revision with the specified param for the repository available or repository not found"),
     @ResponseCode(code = 500, condition = "internal server error")
   })
-  public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("revision") String revision , @Pattern(regexp = DIFF_FORMAT_VALUES_REGEX) @DefaultValue("NATIVE") @QueryParam("format") String format ){
+  public Response get(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("revision") String revision , @Pattern(regexp = DIFF_FORMAT_VALUES_REGEX) @DefaultValue("NATIVE") @QueryParam("format") String format ) throws IOException {
     HttpUtil.checkForCRLFInjection(revision);
     DiffFormat diffFormat = DiffFormat.valueOf(format);
     try (RepositoryService repositoryService = serviceFactory.create(new NamespaceAndName(namespace, name))) {
-      StreamingOutput responseEntry = output -> {
+      DiffCommandBuilder.OutputStreamConsumer outputStreamConsumer = repositoryService.getDiffCommand()
-        repositoryService.getDiffCommand()
         .setRevision(revision)
         .setFormat(diffFormat)
-          .retrieveContent(output);
+        .retrieveContent();
-      };
+      return Response.ok((StreamingOutput) outputStreamConsumer::accept)
-      return Response.ok(responseEntry)
         .header(HEADER_CONTENT_DISPOSITION, HttpUtil.createContentDispositionAttachmentHeader(String.format("%s-%s.diff", name, revision)))
         .build();
     }
   }
-
 }

scm-webapp/src/main/java/sonia/scm/api/v2/resources/IncomingRootResource.java

@@ -10,6 +10,7 @@ import sonia.scm.repository.ChangesetPagingResult;
 import sonia.scm.repository.NamespaceAndName;
 import sonia.scm.repository.Repository;
 import sonia.scm.repository.RepositoryPermissions;
+import sonia.scm.repository.api.DiffCommandBuilder;
 import sonia.scm.repository.api.DiffFormat;
 import sonia.scm.repository.api.RepositoryService;
 import sonia.scm.repository.api.RepositoryServiceFactory;
@@ -138,14 +139,13 @@ public class IncomingRootResource {
     HttpUtil.checkForCRLFInjection(target);
     DiffFormat diffFormat = DiffFormat.valueOf(format);
     try (RepositoryService repositoryService = serviceFactory.create(new NamespaceAndName(namespace, name))) {
-      StreamingOutput responseEntry = output ->
+      DiffCommandBuilder.OutputStreamConsumer outputStreamConsumer = repositoryService.getDiffCommand()
-        repositoryService.getDiffCommand()
         .setRevision(source)
         .setAncestorChangeset(target)
         .setFormat(diffFormat)
-          .retrieveContent(output);
+        .retrieveContent();
 
-      return Response.ok(responseEntry)
+      return Response.ok((StreamingOutput) outputStreamConsumer::accept)
         .header(HEADER_CONTENT_DISPOSITION, HttpUtil.createContentDispositionAttachmentHeader(String.format("%s-%s.diff", name, source)))
         .build();
     }

scm-webapp/src/test/java/sonia/scm/api/v2/resources/DiffResourceTest.java

@@ -91,7 +91,7 @@ public class DiffResourceTest extends RepositoryTestBase {
   public void shouldGetDiffs() throws Exception {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenReturn(diffCommandBuilder);
+    when(diffCommandBuilder.retrieveContent()).thenReturn(output -> {});
     MockHttpRequest request = MockHttpRequest
       .get(DIFF_URL + "revision")
       .accept(VndMediaType.DIFF);
@@ -123,7 +123,7 @@ public class DiffResourceTest extends RepositoryTestBase {
   public void shouldGet404OnMissingRevision() throws Exception {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenThrow(new NotFoundException("Text", "x"));
+    when(diffCommandBuilder.retrieveContent()).thenThrow(new NotFoundException("Text", "x"));
 
     MockHttpRequest request = MockHttpRequest
       .get(DIFF_URL + "revision")
@@ -139,7 +139,7 @@ public class DiffResourceTest extends RepositoryTestBase {
   public void shouldGet400OnCrlfInjection() throws Exception {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenThrow(new NotFoundException("Text", "x"));
+    when(diffCommandBuilder.retrieveContent()).thenThrow(new NotFoundException("Text", "x"));
 
     MockHttpRequest request = MockHttpRequest
       .get(DIFF_URL + "ny%0D%0ASet-cookie:%20Tamper=3079675143472450634")
@@ -153,7 +153,7 @@ public class DiffResourceTest extends RepositoryTestBase {
   public void shouldGet400OnUnknownFormat() throws Exception {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenThrow(new NotFoundException("Test", "test"));
+    when(diffCommandBuilder.retrieveContent()).thenThrow(new NotFoundException("Test", "test"));
 
     MockHttpRequest request = MockHttpRequest
       .get(DIFF_URL + "revision?format=Unknown")
@@ -167,7 +167,7 @@ public class DiffResourceTest extends RepositoryTestBase {
   public void shouldAcceptDiffFormats() throws Exception {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenReturn(diffCommandBuilder);
+    when(diffCommandBuilder.retrieveContent()).thenReturn(output -> {});
 
     Arrays.stream(DiffFormat.values()).map(DiffFormat::name).forEach(
       this::assertRequestOk

scm-webapp/src/test/java/sonia/scm/api/v2/resources/IncomingRootResourceTest.java

@@ -171,7 +171,7 @@ public class IncomingRootResourceTest extends RepositoryTestBase {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setAncestorChangeset(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenReturn(diffCommandBuilder);
+    when(diffCommandBuilder.retrieveContent()).thenReturn(output -> {});
     MockHttpRequest request = MockHttpRequest
       .get(INCOMING_DIFF_URL + "src_changeset_id/target_changeset_id/diff")
       .accept(VndMediaType.DIFF);
@@ -206,7 +206,7 @@ public class IncomingRootResourceTest extends RepositoryTestBase {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setAncestorChangeset(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenThrow(new NotFoundException("Text", "x"));
+    when(diffCommandBuilder.retrieveContent()).thenThrow(new NotFoundException("Text", "x"));
 
     MockHttpRequest request = MockHttpRequest
       .get(INCOMING_DIFF_URL + "src_changeset_id/target_changeset_id/diff")
@@ -223,7 +223,7 @@ public class IncomingRootResourceTest extends RepositoryTestBase {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setAncestorChangeset(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenThrow(new NotFoundException("Text", "x"));
+    when(diffCommandBuilder.retrieveContent()).thenThrow(new NotFoundException("Text", "x"));
     MockHttpRequest request = MockHttpRequest
       .get(INCOMING_DIFF_URL + "ny%0D%0ASet-cookie:%20Tamper=3079675143472450634/ny%0D%0ASet-cookie:%20Tamper=3079675143472450634/diff")
       .accept(VndMediaType.DIFF);
@@ -240,7 +240,7 @@ public class IncomingRootResourceTest extends RepositoryTestBase {
     when(diffCommandBuilder.setRevision(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setAncestorChangeset(anyString())).thenReturn(diffCommandBuilder);
     when(diffCommandBuilder.setFormat(any())).thenReturn(diffCommandBuilder);
-    when(diffCommandBuilder.retrieveContent(any())).thenThrow(new NotFoundException("Test", "test"));
+    when(diffCommandBuilder.retrieveContent()).thenThrow(new NotFoundException("Test", "test"));
     MockHttpRequest request = MockHttpRequest
       .get(INCOMING_DIFF_URL + "src_changeset_id/target_changeset_id/diff?format=Unknown")
       .accept(VndMediaType.DIFF);