Incorporate peer review

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

@@ -31,6 +31,7 @@
 
 package sonia.scm.repository.api;
 
+import lombok.extern.slf4j.Slf4j;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.cache.CacheManager;
@@ -80,6 +81,7 @@ import java.util.stream.Stream;
  * @apiviz.uses sonia.scm.repository.api.UnbundleCommandBuilder
  * @since 1.17
  */
+@Slf4j
 public final class RepositoryService implements Closeable {
 
   private static final Logger logger = LoggerFactory.getLogger(RepositoryService.class);
@@ -128,7 +130,7 @@ public final class RepositoryService implements Closeable {
     try {
       provider.close();
     } catch (IOException ex) {
-      logger.error("Could not close repository service provider", ex);
+      log.error("Could not close repository service provider", ex);
     }
   }
 
@@ -141,7 +143,7 @@ public final class RepositoryService implements Closeable {
    */
   public BlameCommandBuilder getBlameCommand() {
     logger.debug("create blame command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new BlameCommandBuilder(cacheManager, provider.getBlameCommand(),
       repository, preProcessorUtil);
@@ -156,7 +158,7 @@ public final class RepositoryService implements Closeable {
    */
   public BranchesCommandBuilder getBranchesCommand() {
     logger.debug("create branches command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new BranchesCommandBuilder(cacheManager,
       provider.getBranchesCommand(), repository);
@@ -171,7 +173,7 @@ public final class RepositoryService implements Closeable {
    */
   public BrowseCommandBuilder getBrowseCommand() {
     logger.debug("create browse command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new BrowseCommandBuilder(cacheManager, provider.getBrowseCommand(),
       repository, preProcessorUtil);
@@ -187,7 +189,7 @@ public final class RepositoryService implements Closeable {
    */
   public BundleCommandBuilder getBundleCommand() {
     logger.debug("create bundle command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new BundleCommandBuilder(provider.getBundleCommand(), repository);
   }
@@ -201,7 +203,7 @@ public final class RepositoryService implements Closeable {
    */
   public CatCommandBuilder getCatCommand() {
     logger.debug("create cat command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new CatCommandBuilder(provider.getCatCommand());
   }
@@ -216,7 +218,7 @@ public final class RepositoryService implements Closeable {
    */
   public DiffCommandBuilder getDiffCommand() {
     logger.debug("create diff command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new DiffCommandBuilder(provider.getDiffCommand());
   }
@@ -232,7 +234,7 @@ public final class RepositoryService implements Closeable {
    */
   public IncomingCommandBuilder getIncomingCommand() {
     logger.debug("create incoming command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new IncomingCommandBuilder(cacheManager,
       provider.getIncomingCommand(), repository, preProcessorUtil);
@@ -247,7 +249,7 @@ public final class RepositoryService implements Closeable {
    */
   public LogCommandBuilder getLogCommand() {
     logger.debug("create log command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new LogCommandBuilder(cacheManager, provider.getLogCommand(),
       repository, preProcessorUtil);
@@ -261,7 +263,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());
+    logger.debug("create modifications command for repository {}", repository.getNamespaceAndName());
     return new ModificationsCommandBuilder(provider.getModificationsCommand(),repository, cacheManager.getCache(ModificationsCommandBuilder.CACHE_NAME), preProcessorUtil);
   }
 
@@ -275,7 +277,7 @@ public final class RepositoryService implements Closeable {
    */
   public OutgoingCommandBuilder getOutgoingCommand() {
     logger.debug("create outgoing command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new OutgoingCommandBuilder(cacheManager,
       provider.getOutgoingCommand(), repository, preProcessorUtil);
@@ -291,7 +293,7 @@ public final class RepositoryService implements Closeable {
    */
   public PullCommandBuilder getPullCommand() {
     logger.debug("create pull command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new PullCommandBuilder(provider.getPullCommand(), repository);
   }
@@ -306,7 +308,7 @@ public final class RepositoryService implements Closeable {
    */
   public PushCommandBuilder getPushCommand() {
     logger.debug("create push command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new PushCommandBuilder(provider.getPushCommand());
   }
@@ -329,7 +331,7 @@ public final class RepositoryService implements Closeable {
    */
   public TagsCommandBuilder getTagsCommand() {
     logger.debug("create tags command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new TagsCommandBuilder(cacheManager, provider.getTagsCommand(),
       repository);
@@ -345,7 +347,7 @@ public final class RepositoryService implements Closeable {
    */
   public UnbundleCommandBuilder getUnbundleCommand() {
     logger.debug("create unbundle command for repository {}",
-      repository.getName());
+      repository.getNamespaceAndName());
 
     return new UnbundleCommandBuilder(provider.getUnbundleCommand(),
       repository);
@@ -372,21 +374,20 @@ public final class RepositoryService implements Closeable {
     return provider.getSupportedFeatures().contains(feature);
   }
 
-
-  public Stream<ScmProtocol> getSupportedProtocols() {
+  public <T extends ScmProtocol> Stream<T> getSupportedProtocols() {
     return protocolProviders.stream()
       .filter(protocolProvider -> protocolProvider.getType().equals(getRepository().getType()))
-      .map(this::createProviderInstanceForRepository);
+      .map(this::<T>createProviderInstanceForRepository);
   }
 
-  private ScmProtocol createProviderInstanceForRepository(ScmProtocolProvider protocolProvider) {
+  private <T extends ScmProtocol> T createProviderInstanceForRepository(ScmProtocolProvider<T> protocolProvider) {
     return protocolProvider.get(repository);
   }
 
   public <T extends ScmProtocol> T getProtocol(Class<T> clazz) {
-    return (T) getSupportedProtocols()
+    return this.<T>getSupportedProtocols()
       .filter(scmProtocol -> clazz.isAssignableFrom(scmProtocol.getClass()))
       .findFirst()
-      .orElseThrow(() -> new IllegalArgumentException("no implementation for " + clazz.getName() + " and repository type " + getRepository().getType()));
+      .orElseThrow(() -> new IllegalArgumentException(String.format("no implementation for %s and repository type %s", clazz.getName(),getRepository().getType())));
   }
 }

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

@@ -4,9 +4,9 @@ import sonia.scm.plugin.ExtensionPoint;
 import sonia.scm.repository.Repository;
 
 @ExtensionPoint(multi = true)
-public interface ScmProtocolProvider {
+public interface ScmProtocolProvider<T extends ScmProtocol> {
 
   String getType();
 
-  ScmProtocol get(Repository repository);
+  T get(Repository repository);
 }

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

@@ -27,7 +27,7 @@ public abstract class HttpScmProtocol implements ScmProtocol {
 
   @Override
   public String getUrl() {
-      return URI.create(basePath + "/").resolve("repo/" + repository.getNamespace() + "/" + repository.getName()).toASCIIString();
+      return URI.create(basePath + "/").resolve(String.format("repo/%s/%s", repository.getNamespace(), repository.getName())).toASCIIString();
   }
 
   public final void serve(HttpServletRequest request, HttpServletResponse response, ServletConfig config) throws ServletException, IOException {

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

@@ -1,7 +1,6 @@
 package sonia.scm.repository.spi;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import lombok.extern.slf4j.Slf4j;
 import sonia.scm.api.v2.resources.ScmPathInfoStore;
 import sonia.scm.config.ScmConfiguration;
 import sonia.scm.repository.Repository;
@@ -18,20 +17,19 @@ import java.util.Optional;
 import static java.util.Optional.empty;
 import static java.util.Optional.of;
 
-public abstract class InitializingHttpScmProtocolWrapper implements ScmProtocolProvider {
-
-  private static final Logger logger = LoggerFactory.getLogger(InitializingHttpScmProtocolWrapper.class);
+@Slf4j
+public abstract class InitializingHttpScmProtocolWrapper implements ScmProtocolProvider<HttpScmProtocol> {
 
   private final Provider<? extends ScmProviderHttpServlet> delegateProvider;
-  private final Provider<ScmPathInfoStore> uriInfoStore;
+  private final Provider<ScmPathInfoStore> pathInfoStore;
   private final ScmConfiguration scmConfiguration;
 
   private volatile boolean isInitialized = false;
 
 
-  protected InitializingHttpScmProtocolWrapper(Provider<? extends ScmProviderHttpServlet> delegateProvider, Provider<ScmPathInfoStore> uriInfoStore, ScmConfiguration scmConfiguration) {
+  protected InitializingHttpScmProtocolWrapper(Provider<? extends ScmProviderHttpServlet> delegateProvider, Provider<ScmPathInfoStore> pathInfoStore, ScmConfiguration scmConfiguration) {
     this.delegateProvider = delegateProvider;
-    this.uriInfoStore = uriInfoStore;
+    this.pathInfoStore = pathInfoStore;
     this.scmConfiguration = scmConfiguration;
   }
 
@@ -42,7 +40,7 @@ public abstract class InitializingHttpScmProtocolWrapper implements ScmProtocolP
   @Override
   public HttpScmProtocol get(Repository repository) {
     if (!repository.getType().equals(getType())) {
-      throw new IllegalArgumentException("cannot handle repository with type " + repository.getType() + " with protocol for type " + getType());
+      throw new IllegalArgumentException(String.format("cannot handle repository with type %s with protocol for type %s", repository.getType(), getType()));
     }
     return new ProtocolWrapper(repository, computeBasePath());
   }
@@ -53,18 +51,18 @@ public abstract class InitializingHttpScmProtocolWrapper implements ScmProtocolP
 
   private Optional<String> getPathFromScmPathInfoIfAvailable() {
     try {
-      ScmPathInfoStore scmPathInfoStore = uriInfoStore.get();
+      ScmPathInfoStore scmPathInfoStore = pathInfoStore.get();
       if (scmPathInfoStore != null && scmPathInfoStore.get() != null) {
         return of(scmPathInfoStore.get().getRootUri().toASCIIString());
       }
     } catch (Exception e) {
-      logger.debug("could not get ScmPathInfoStore from context", e);
+      log.debug("could not get ScmPathInfoStore from context", e);
     }
     return empty();
   }
 
   private String getPathFromConfiguration() {
-    logger.debug("using base path from configuration: {}", scmConfiguration.getBaseUrl());
+    log.debug("using base path from configuration: {}", scmConfiguration.getBaseUrl());
     return scmConfiguration.getBaseUrl();
   }
 

scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/web/SvnScmProtocolProviderWrapper.java

@@ -56,6 +56,9 @@ public class SvnScmProtocolProviderWrapper extends InitializingHttpScmProtocolWr
     }
 
     @Override
+    /**
+     * Overridden to return the systems temp directory for the key {@link PARAMETER_SVN_PARENTPATH}.
+     */
     public String getInitParameter(String key) {
       if (PARAMETER_SVN_PARENTPATH.equals(key)) {
         return System.getProperty("java.io.tmpdir");

scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java

@@ -38,15 +38,13 @@ public class HttpProtocolServlet extends HttpServlet {
   private final PushStateDispatcher dispatcher;
   private final UserAgentParser userAgentParser;
 
-  private final NamespaceAndNameFromPathExtractor namespaceAndNameFromPathExtractor;
 
   @Inject
-  public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, Provider<HttpServletRequest> requestProvider, PushStateDispatcher dispatcher, UserAgentParser userAgentParser, NamespaceAndNameFromPathExtractor namespaceAndNameFromPathExtractor) {
+  public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, Provider<HttpServletRequest> requestProvider, PushStateDispatcher dispatcher, UserAgentParser userAgentParser) {
     this.serviceFactory = serviceFactory;
     this.requestProvider = requestProvider;
     this.dispatcher = dispatcher;
     this.userAgentParser = userAgentParser;
-    this.namespaceAndNameFromPathExtractor = namespaceAndNameFromPathExtractor;
   }
 
   @Override
@@ -58,7 +56,7 @@ public class HttpProtocolServlet extends HttpServlet {
     } else {
 
       String pathInfo = request.getPathInfo();
-      Optional<NamespaceAndName> namespaceAndName = namespaceAndNameFromPathExtractor.fromUri(pathInfo);
+      Optional<NamespaceAndName> namespaceAndName = NamespaceAndNameFromPathExtractor.fromUri(pathInfo);
       if (namespaceAndName.isPresent()) {
         service(request, response, namespaceAndName.get());
       } else {

scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java

@@ -8,8 +8,11 @@ import java.util.Optional;
 import static java.util.Optional.empty;
 import static java.util.Optional.of;
 
-class NamespaceAndNameFromPathExtractor {
-  Optional<NamespaceAndName> fromUri(String uri) {
+final class NamespaceAndNameFromPathExtractor {
+
+  private NamespaceAndNameFromPathExtractor() {}
+
+  static Optional<NamespaceAndName> fromUri(String uri) {
     if (uri.startsWith(HttpUtil.SEPARATOR_PATH)) {
       uri = uri.substring(1);
     }

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

@@ -42,7 +42,7 @@ public class RepositoryToRepositoryDtoMapperTest {
   private final URI baseUri = URI.create("http://example.com/base/");
   @SuppressWarnings("unused") // Is injected
   private final ResourceLinks resourceLinks = ResourceLinksMock.createMock(baseUri);
-  @Mock//(answer = Answers.RETURNS_DEEP_STUBS)
+  @Mock
   private RepositoryServiceFactory serviceFactory;
   @Mock
   private RepositoryService repositoryService;

scm-webapp/src/test/java/sonia/scm/it/GitLfsITCase.java

@@ -251,8 +251,7 @@ public class GitLfsITCase {
   }
 
   private String createBatchUrl() {
-    String url = BASE_URL + "repo/" + repository.getNamespace() + "/" + repository.getName();
-    return url + "/info/lfs/objects/batch";
+    return String.format("%srepo/%s/%s/info/lfs/objects/batch", BASE_URL, repository.getNamespace(), repository.getName());
   }
 
   private byte[] download(ScmClient client, LfsObject lfsObject) throws IOException {

scm-webapp/src/test/java/sonia/scm/it/RepositoryHookITCase.java

@@ -172,7 +172,7 @@ public class RepositoryHookITCase extends AbstractAdminITCaseBase
     Thread.sleep(WAIT_TIME);
     
     // check debug servlet that only one commit is present
-    WebResource.Builder wr = createResource(client, "../debug/" + repository.getNamespace() + "/" + repository.getName() + "/post-receive/last");
+    WebResource.Builder wr = createResource(client, String.format("../debug/%s/%s/post-receive/last", repository.getNamespace(), repository.getName()));
     DebugHookData data = wr.get(DebugHookData.class);
     assertNotNull(data);
     assertThat(data.getChangesets(), allOf(
@@ -195,8 +195,8 @@ public class RepositoryHookITCase extends AbstractAdminITCaseBase
   
   private RepositoryClient createRepositoryClient() throws IOException 
   {
-    return REPOSITORY_CLIENT_FACTORY.create(repositoryType, 
-      IntegrationTestUtil.BASE_URL + "repo/" + repository.getNamespace() + "/" + repository.getName(),
+    return REPOSITORY_CLIENT_FACTORY.create(repositoryType,
+      String.format("%srepo/%s/%s", IntegrationTestUtil.BASE_URL, repository.getNamespace(), repository.getName()),
       IntegrationTestUtil.ADMIN_USERNAME, IntegrationTestUtil.ADMIN_PASSWORD, workingCopy
     );
   }

scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java

@@ -20,20 +20,16 @@ import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
-import java.util.Optional;
 
-import static java.util.Optional.of;
 import static org.mockito.AdditionalMatchers.not;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.mockito.MockitoAnnotations.initMocks;
 
 public class HttpProtocolServletTest {
 
-  private static final NamespaceAndName EXISTING_REPO = new NamespaceAndName("space", "repo");
 
   @Mock
   private RepositoryServiceFactory serviceFactory;
@@ -44,8 +40,6 @@ public class HttpProtocolServletTest {
   @Mock
   private UserAgentParser userAgentParser;
   @Mock
-  private NamespaceAndNameFromPathExtractor namespaceAndNameFromPathExtractor;
-  @Mock
   private Provider<HttpServletRequest> requestProvider;
 
   @InjectMocks
@@ -68,8 +62,9 @@ public class HttpProtocolServletTest {
     initMocks(this);
     when(userAgentParser.parse(request)).thenReturn(userAgent);
     when(userAgent.isBrowser()).thenReturn(false);
-    when(serviceFactory.create(not(eq(EXISTING_REPO)))).thenThrow(RepositoryNotFoundException.class);
-    when(serviceFactory.create(EXISTING_REPO)).thenReturn(repositoryService);
+    NamespaceAndName existingRepo = new NamespaceAndName("space", "repo");
+    when(serviceFactory.create(not(eq(existingRepo)))).thenThrow(RepositoryNotFoundException.class);
+    when(serviceFactory.create(existingRepo)).thenReturn(repositoryService);
     when(requestProvider.get()).thenReturn(httpServletRequest);
   }
 
@@ -85,8 +80,7 @@ public class HttpProtocolServletTest {
 
   @Test
   public void shouldHandleBadPaths() throws IOException, ServletException {
-    when(request.getPathInfo()).thenReturn("/space/name");
-    when(namespaceAndNameFromPathExtractor.fromUri("/space/name")).thenReturn(Optional.empty());
+    when(request.getPathInfo()).thenReturn("/illegal");
 
     servlet.service(request, response);
 
@@ -94,11 +88,8 @@ public class HttpProtocolServletTest {
   }
 
   @Test
-  public void shouldHandleNotExistingRepository() throws RepositoryNotFoundException, IOException, ServletException {
-    when(request.getPathInfo()).thenReturn("/space/name");
-    NamespaceAndName namespaceAndName = new NamespaceAndName("space", "name");
-    when(namespaceAndNameFromPathExtractor.fromUri("/space/name")).thenReturn(of(namespaceAndName));
-    doThrow(new RepositoryNotFoundException(namespaceAndName)).when(serviceFactory).create(namespaceAndName);
+  public void shouldHandleNotExistingRepository() throws IOException, ServletException {
+    when(request.getPathInfo()).thenReturn("/not/exists");
 
     servlet.service(request, response);
 
@@ -109,7 +100,6 @@ public class HttpProtocolServletTest {
   public void shouldDelegateToProvider() throws RepositoryNotFoundException, IOException, ServletException {
     when(request.getPathInfo()).thenReturn("/space/name");
     NamespaceAndName namespaceAndName = new NamespaceAndName("space", "name");
-    when(namespaceAndNameFromPathExtractor.fromUri("/space/name")).thenReturn(of(namespaceAndName));
     doReturn(repositoryService).when(serviceFactory).create(namespaceAndName);
     Repository repository = new Repository();
     when(repositoryService.getRepository()).thenReturn(repository);

scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java

@@ -38,7 +38,7 @@ public class NamespaceAndNameFromPathExtractorTest {
     return dynamicTest(
       "should extract correct namespace and name for path " + path,
       () -> {
-        Optional<NamespaceAndName> namespaceAndName = new NamespaceAndNameFromPathExtractor().fromUri(path);
+        Optional<NamespaceAndName> namespaceAndName = NamespaceAndNameFromPathExtractor.fromUri(path);
 
         assertThat(namespaceAndName.get()).isEqualTo(new NamespaceAndName("space", "repo"));
       }
@@ -59,7 +59,7 @@ public class NamespaceAndNameFromPathExtractorTest {
     return dynamicTest(
       "should not fail for wrong path " + path,
       () -> {
-        Optional<NamespaceAndName> namespaceAndName = new NamespaceAndNameFromPathExtractor().fromUri(path);
+        Optional<NamespaceAndName> namespaceAndName = NamespaceAndNameFromPathExtractor.fromUri(path);
 
         assertThat(namespaceAndName.isPresent()).isFalse();
       }