Implement peer review comments

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

@@ -32,8 +32,6 @@
 
 package sonia.scm.repository.api;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 import sonia.scm.repository.Branch;
 import sonia.scm.repository.spi.BranchCommand;
 
@@ -48,12 +46,24 @@ public final class BranchCommandBuilder {
     this.command = command;
   }
 
+  /**
+   * Specifies the source branch, which the new branch should be based on.
+   *
+   * @param parentBranch The base branch for the new branch.
+   * @return This builder.
+   */
   public BranchCommandBuilder from(String parentBranch) {
     request.setParentBranch(parentBranch);
     return this;
   }
 
-  public Branch branch(String name) throws IOException {
+  /**
+   * Execute the command and create a new branch with the given name.
+   * @param name The name of the new branch.
+   * @return The created branch.
+   * @throws IOException
+   */
+  public Branch branch(String name) {
     request.setNewBranch(name);
     return command.branch(request);
   }

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

@@ -39,6 +39,7 @@ import sonia.scm.repository.Changeset;
 import sonia.scm.repository.Feature;
 import sonia.scm.repository.PreProcessorUtil;
 import sonia.scm.repository.Repository;
+import sonia.scm.repository.RepositoryPermissions;
 import sonia.scm.repository.spi.RepositoryServiceProvider;
 
 import java.io.Closeable;
@@ -82,10 +83,9 @@ import java.util.stream.Stream;
  * @apiviz.uses sonia.scm.repository.api.MergeCommandBuilder
  * @since 1.17
  */
-@Slf4j
 public final class RepositoryService implements Closeable {
 
-  private static final Logger logger = LoggerFactory.getLogger(RepositoryService.class);
+  private static final Logger LOG = LoggerFactory.getLogger(RepositoryService.class);
 
   private final CacheManager cacheManager;
   private final PreProcessorUtil preProcessorUtil;
@@ -131,7 +131,7 @@ public final class RepositoryService implements Closeable {
     try {
       provider.close();
     } catch (IOException ex) {
-      log.error("Could not close repository service provider", ex);
+      LOG.error("Could not close repository service provider", ex);
     }
   }
 
@@ -143,7 +143,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public BlameCommandBuilder getBlameCommand() {
-    logger.debug("create blame command for repository {}",
+    LOG.debug("create blame command for repository {}",
       repository.getNamespaceAndName());
 
     return new BlameCommandBuilder(cacheManager, provider.getBlameCommand(),
@@ -158,7 +158,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public BranchesCommandBuilder getBranchesCommand() {
-    logger.debug("create branches command for repository {}",
+    LOG.debug("create branches command for repository {}",
       repository.getNamespaceAndName());
 
     return new BranchesCommandBuilder(cacheManager,
@@ -173,7 +173,8 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public BranchCommandBuilder getBranchCommand() {
-    logger.debug("create branch command for repository {}",
+    RepositoryPermissions.push(getRepository()).check();
+    LOG.debug("create branch command for repository {}",
       repository.getNamespaceAndName());
 
     return new BranchCommandBuilder(provider.getBranchCommand());
@@ -187,7 +188,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public BrowseCommandBuilder getBrowseCommand() {
-    logger.debug("create browse command for repository {}",
+    LOG.debug("create browse command for repository {}",
       repository.getNamespaceAndName());
 
     return new BrowseCommandBuilder(cacheManager, provider.getBrowseCommand(),
@@ -203,7 +204,7 @@ public final class RepositoryService implements Closeable {
    * @since 1.43
    */
   public BundleCommandBuilder getBundleCommand() {
-    logger.debug("create bundle command for repository {}",
+    LOG.debug("create bundle command for repository {}",
       repository.getNamespaceAndName());
 
     return new BundleCommandBuilder(provider.getBundleCommand(), repository);
@@ -217,7 +218,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public CatCommandBuilder getCatCommand() {
-    logger.debug("create cat command for repository {}",
+    LOG.debug("create cat command for repository {}",
       repository.getNamespaceAndName());
 
     return new CatCommandBuilder(provider.getCatCommand());
@@ -232,7 +233,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public DiffCommandBuilder getDiffCommand() {
-    logger.debug("create diff command for repository {}",
+    LOG.debug("create diff command for repository {}",
       repository.getNamespaceAndName());
 
     return new DiffCommandBuilder(provider.getDiffCommand(), provider.getSupportedFeatures());
@@ -248,7 +249,7 @@ public final class RepositoryService implements Closeable {
    * @since 1.31
    */
   public IncomingCommandBuilder getIncomingCommand() {
-    logger.debug("create incoming command for repository {}",
+    LOG.debug("create incoming command for repository {}",
       repository.getNamespaceAndName());
 
     return new IncomingCommandBuilder(cacheManager,
@@ -263,7 +264,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public LogCommandBuilder getLogCommand() {
-    logger.debug("create log command for repository {}",
+    LOG.debug("create log command for repository {}",
       repository.getNamespaceAndName());
 
     return new LogCommandBuilder(cacheManager, provider.getLogCommand(),
@@ -278,7 +279,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public ModificationsCommandBuilder getModificationsCommand() {
-    logger.debug("create modifications command for repository {}", repository.getNamespaceAndName());
+    LOG.debug("create modifications command for repository {}", repository.getNamespaceAndName());
     return new ModificationsCommandBuilder(provider.getModificationsCommand(),repository, cacheManager.getCache(ModificationsCommandBuilder.CACHE_NAME), preProcessorUtil);
   }
 
@@ -291,7 +292,7 @@ public final class RepositoryService implements Closeable {
    * @since 1.31
    */
   public OutgoingCommandBuilder getOutgoingCommand() {
-    logger.debug("create outgoing command for repository {}",
+    LOG.debug("create outgoing command for repository {}",
       repository.getNamespaceAndName());
 
     return new OutgoingCommandBuilder(cacheManager,
@@ -307,7 +308,7 @@ public final class RepositoryService implements Closeable {
    * @since 1.31
    */
   public PullCommandBuilder getPullCommand() {
-    logger.debug("create pull command for repository {}",
+    LOG.debug("create pull command for repository {}",
       repository.getNamespaceAndName());
 
     return new PullCommandBuilder(provider.getPullCommand(), repository);
@@ -322,7 +323,7 @@ public final class RepositoryService implements Closeable {
    * @since 1.31
    */
   public PushCommandBuilder getPushCommand() {
-    logger.debug("create push command for repository {}",
+    LOG.debug("create push command for repository {}",
       repository.getNamespaceAndName());
 
     return new PushCommandBuilder(provider.getPushCommand());
@@ -345,7 +346,7 @@ public final class RepositoryService implements Closeable {
    *                                      by the implementation of the repository service provider.
    */
   public TagsCommandBuilder getTagsCommand() {
-    logger.debug("create tags command for repository {}",
+    LOG.debug("create tags command for repository {}",
       repository.getNamespaceAndName());
 
     return new TagsCommandBuilder(cacheManager, provider.getTagsCommand(),
@@ -361,7 +362,7 @@ public final class RepositoryService implements Closeable {
    * @since 1.43
    */
   public UnbundleCommandBuilder getUnbundleCommand() {
-    logger.debug("create unbundle command for repository {}",
+    LOG.debug("create unbundle command for repository {}",
       repository.getNamespaceAndName());
 
     return new UnbundleCommandBuilder(provider.getUnbundleCommand(),
@@ -378,7 +379,8 @@ public final class RepositoryService implements Closeable {
    * @since 2.0.0
    */
   public MergeCommandBuilder getMergeCommand() {
-    logger.debug("create merge command for repository {}",
+    RepositoryPermissions.push(getRepository()).check();
+    LOG.debug("create merge command for repository {}",
       repository.getNamespaceAndName());
 
     return new MergeCommandBuilder(provider.getMergeCommand());

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

@@ -36,11 +36,9 @@ package sonia.scm.repository.spi;
 import sonia.scm.repository.Branch;
 import sonia.scm.repository.api.BranchRequest;
 
-import java.io.IOException;
-
 /**
  * @since 2.0
  */
 public interface BranchCommand {
-  Branch branch(BranchRequest name) throws IOException;
+  Branch branch(BranchRequest name);
 }

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

@@ -0,0 +1,23 @@
+package sonia.scm.repository.spi;
+
+import sonia.scm.ContextEntry;
+import sonia.scm.ExceptionWithContext;
+import sonia.scm.repository.Repository;
+
+public class IntegrateChangesFromWorkdirException extends ExceptionWithContext {
+
+  private static final String CODE = "CHRM7IQzo1";
+
+  public IntegrateChangesFromWorkdirException(Repository repository, String message) {
+    super(ContextEntry.ContextBuilder.entity(repository).build(), message);
+  }
+
+  public IntegrateChangesFromWorkdirException(Repository repository, String message, Exception cause) {
+    super(ContextEntry.ContextBuilder.entity(repository).build(), message, cause);
+  }
+
+  @Override
+  public String getCode() {
+    return CODE;
+  }
+}

scm-core/src/main/java/sonia/scm/repository/util/CloseableWrapper.java

@@ -16,10 +16,6 @@ public class CloseableWrapper<T extends AutoCloseable> implements AutoCloseable
 
   @Override
   public void close() {
-    try {
+    cleanup.accept(wrapped);
-      cleanup.accept(wrapped);
-    } catch (Throwable t) {
-      throw new RuntimeException(t);
-    }
   }
 }

scm-core/src/main/java/sonia/scm/repository/util/SimpleWorkdirFactory.java

@@ -16,7 +16,7 @@ public abstract class SimpleWorkdirFactory<R, C> implements WorkdirFactory<R, C>
   private final File poolDirectory;
 
   public SimpleWorkdirFactory() {
-    this(new File(System.getProperty("java.io.tmpdir"), "scmm-work-pool"));
+    this(new File(System.getProperty("scm.workdir" , System.getProperty("java.io.tmpdir")), "scm-work"));
   }
 
   public SimpleWorkdirFactory(File poolDirectory) {

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

@@ -79,11 +79,8 @@ public class GitBranchCommand extends AbstractGitCommand implements BranchComman
   private void handlePushError(RemoteRefUpdate remoteRefUpdate, BranchRequest request, Repository repository) {
     if (remoteRefUpdate.getStatus() != RemoteRefUpdate.Status.OK) {
       // TODO handle failed remote update
-      throw new RuntimeException(
+      throw new IntegrateChangesFromWorkdirException(repository,
-        String.format("Could not pull new branch '%s' into central repository '%s': %s",
+        String.format("Could not push new branch '%s' into central repository", request.getNewBranch()));
-          request.getNewBranch(),
-          repository.getNamespaceAndName(),
-          remoteRefUpdate.getMessage()));
     }
   }
 }

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

@@ -47,8 +47,6 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
 
   @Override
   public MergeCommandResult merge(MergeCommandRequest request) {
-    RepositoryPermissions.push(context.getRepository().getId()).check();
-
     try (WorkingCopy<Repository> workingCopy = workdirFactory.createWorkingCopy(context)) {
       Repository repository = workingCopy.getWorkingRepository();
       logger.debug("cloned repository to folder {}", repository.getWorkTree());

scm-plugins/scm-hg-plugin/pom.xml

@@ -28,12 +28,6 @@
         </exclusion>
       </exclusions>
     </dependency>
-    <dependency>
-      <groupId>io.jsonwebtoken</groupId>
-      <artifactId>jjwt</artifactId>
-      <version>0.4</version>
-    </dependency>
-
   </dependencies>
 
   <build>

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

@@ -34,12 +34,15 @@ import com.aragost.javahg.Changeset;
 import com.aragost.javahg.commands.CommitCommand;
 import com.aragost.javahg.commands.PullCommand;
 import com.aragost.javahg.commands.UpdateCommand;
+import org.apache.shiro.SecurityUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.repository.Branch;
+import sonia.scm.repository.InternalRepositoryException;
 import sonia.scm.repository.Repository;
 import sonia.scm.repository.api.BranchRequest;
 import sonia.scm.repository.util.WorkingCopy;
+import sonia.scm.user.User;
 
 import java.io.IOException;
 
@@ -59,37 +62,53 @@ public class HgBranchCommand extends AbstractCommand implements BranchCommand {
   }
 
   @Override
-  public Branch branch(BranchRequest request) throws IOException {
+  public Branch branch(BranchRequest request) {
     try (WorkingCopy<com.aragost.javahg.Repository> workingCopy = workdirFactory.createWorkingCopy(getContext())) {
       com.aragost.javahg.Repository repository = workingCopy.getWorkingRepository();
-      if (request.getParentBranch() != null) {
-        UpdateCommand.on(repository).rev(request.getParentBranch()).execute();
-      }
-      com.aragost.javahg.commands.BranchCommand.on(repository).set(request.getNewBranch());
 
-      Changeset emptyChangeset = CommitCommand
+      checkoutParentBranchIfSpecified(request, repository);
-        .on(repository)
+
-        .user("SCM-Manager")
+      Changeset emptyChangeset = createNewBranchWithEmptyCommit(request, repository);
-        .message("Create new branch " + request.getNewBranch())
-        .execute();
 
       LOG.debug("Created new branch '{}' in repository {} with changeset {}",
         request.getNewBranch(), getRepository().getNamespaceAndName(), emptyChangeset.getNode());
 
-      try {
+      pullNewBranchIntoCentralRepository(request, workingCopy);
-        com.aragost.javahg.commands.PullCommand pullCommand = PullCommand.on(workingCopy.getCentralRepository());
-        workdirFactory.configure(pullCommand);
-        pullCommand.execute(workingCopy.getDirectory().getAbsolutePath());
-      } catch (Exception e) {
-        // TODO handle failed update
-        throw new RuntimeException(
-          String.format("Could not pull new branch '%s' into central repository '%s'",
-            request.getNewBranch(),
-            getRepository().getNamespaceAndName()),
-          e);
-      }
 
       return Branch.normalBranch(request.getNewBranch(), emptyChangeset.getNode());
     }
   }
+
+  private void checkoutParentBranchIfSpecified(BranchRequest request, com.aragost.javahg.Repository repository) {
+    if (request.getParentBranch() != null) {
+      try {
+        UpdateCommand.on(repository).rev(request.getParentBranch()).execute();
+      } catch (IOException e) {
+        throw new InternalRepositoryException(getRepository(), "Could not check out parent branch " + request.getParentBranch(), e);
+      }
+    }
+  }
+
+  private Changeset createNewBranchWithEmptyCommit(BranchRequest request, com.aragost.javahg.Repository repository) {
+    com.aragost.javahg.commands.BranchCommand.on(repository).set(request.getNewBranch());
+    User currentUser = SecurityUtils.getSubject().getPrincipals().oneByType(User.class);
+    return CommitCommand
+      .on(repository)
+      .user(String.format("%s <%s>", currentUser.getDisplayName(), currentUser.getMail()))
+      .message("Create new branch " + request.getNewBranch())
+      .execute();
+  }
+
+  private void pullNewBranchIntoCentralRepository(BranchRequest request, WorkingCopy<com.aragost.javahg.Repository> workingCopy) {
+    try {
+      PullCommand pullCommand = PullCommand.on(workingCopy.getCentralRepository());
+      workdirFactory.configure(pullCommand);
+      pullCommand.execute(workingCopy.getDirectory().getAbsolutePath());
+    } catch (Exception e) {
+      // TODO handle failed update
+      throw new IntegrateChangesFromWorkdirException(getRepository(),
+        String.format("Could not pull new branch '%s' into central repository", request.getNewBranch()),
+        e);
+    }
+  }
 }

scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/web/HgCGIServlet.java

@@ -203,14 +203,6 @@ public class HgCGIServlet extends HttpServlet implements ScmProviderHttpServlet
     executor.setContentLengthWorkaround(true);
     hgRepositoryEnvironmentBuilder.buildFor(repository, request, executor.getEnvironment().asMutableMap());
 
-    // unused ???
-    HttpSession session = request.getSession(false);
-
-    if (session != null)
-    {
-      passSessionAttributes(executor.getEnvironment(), session);
-    }
-
     String interpreter = getInterpreter();
 
     if (interpreter != null)

scm-webapp/src/main/java/sonia/scm/api/ContextualFallbackExceptionMapper.java

@@ -0,0 +1,32 @@
+package sonia.scm.api;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.MDC;
+import sonia.scm.ExceptionWithContext;
+import sonia.scm.api.v2.resources.ErrorDto;
+import sonia.scm.web.VndMediaType;
+
+import javax.ws.rs.core.Response;
+import javax.ws.rs.ext.ExceptionMapper;
+import javax.ws.rs.ext.Provider;
+
+@Provider
+public class ContextualFallbackExceptionMapper implements ExceptionMapper<ExceptionWithContext> {
+
+  private static final Logger logger = LoggerFactory.getLogger(ContextualFallbackExceptionMapper.class);
+
+  @Override
+  public Response toResponse(ExceptionWithContext exception) {
+    logger.warn("mapping unexpected {} to status code 500", exception.getClass().getName(), exception);
+    ErrorDto errorDto = new ErrorDto();
+    errorDto.setMessage(exception.getMessage());
+    errorDto.setContext(exception.getContext());
+    errorDto.setErrorCode(exception.getCode());
+    errorDto.setTransactionId(MDC.get("transaction_id"));
+    return Response.status(Response.Status.INTERNAL_SERVER_ERROR)
+      .entity(errorDto)
+      .type(VndMediaType.ERROR_TYPE)
+      .build();
+  }
+}

scm-webapp/src/main/resources/locales/de/plugins.json

@@ -147,6 +147,10 @@
     "3zR9vPNIE1": {
       "displayName": "Ungültige Eingabe",
       "description": "Die eingegebenen Daten konnten nicht validiert werden. Bitte korrigieren Sie die Eingaben und senden Sie sie erneut."
+    },
+    "CHRM7IQzo1": {
+      "displayName": "Änderung fehlgeschlagen",
+      "description": "Die Änderung ist fehlgeschlagen. Bitte wenden Sie sich an ihren Administrator für weitere Hinweise."
     }
   },
   "namespaceStrategies": {

scm-webapp/src/main/resources/locales/en/plugins.json

@@ -147,6 +147,10 @@
     "3zR9vPNIE1": {
       "displayName": "Illegal input",
       "description": "The values could not be validated. Please correct your input and try again."
+    },
+    "CHRM7IQzo1": {
+      "displayName": "Change failed",
+      "description": "The change failed. Please contact your administrator for further assistance."
     }
   },
   "namespaceStrategies": {