Checkout target branch while cloning repository

This will prevent the checkout of a wrong initial branch and therefore
safe some unnecessary io

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

@@ -140,8 +140,8 @@ class AbstractGitCommand
     }
   }
 
-  <R, W extends GitCloneWorker<R>> R inClone(Function<Git, W> workerSupplier, GitWorkdirFactory workdirFactory) {
-    try (WorkingCopy<Repository> workingCopy = workdirFactory.createWorkingCopy(context)) {
+  <R, W extends GitCloneWorker<R>> R inClone(Function<Git, W> workerSupplier, GitWorkdirFactory workdirFactory, String initialBranch) {
+    try (WorkingCopy<Repository> workingCopy = workdirFactory.createWorkingCopy(context, initialBranch)) {
       Repository repository = workingCopy.getWorkingRepository();
       logger.debug("cloned repository to folder {}", repository.getWorkTree());
       return workerSupplier.apply(new Git(repository)).run();

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

@@ -58,11 +58,8 @@ public class GitBranchCommand extends AbstractGitCommand implements BranchComman
 
   @Override
   public Branch branch(BranchRequest request) {
-    try (WorkingCopy<org.eclipse.jgit.lib.Repository> workingCopy = workdirFactory.createWorkingCopy(context)) {
+    try (WorkingCopy<org.eclipse.jgit.lib.Repository> workingCopy = workdirFactory.createWorkingCopy(context, request.getParentBranch())) {
       Git clone = new Git(workingCopy.getWorkingRepository());
-      if (request.getParentBranch() != null) {
-        clone.checkout().setName("origin/" + request.getParentBranch()).call();
-      }
       Ref ref = clone.branchCreate().setName(request.getNewBranch()).call();
       Iterable<PushResult> call = clone.push().add(request.getNewBranch()).call();
       StreamSupport.stream(call.spliterator(), false)

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

@@ -38,7 +38,7 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
 
   @Override
   public MergeCommandResult merge(MergeCommandRequest request) {
-    return inClone(clone -> new MergeWorker(clone, request), workdirFactory);
+    return inClone(clone -> new MergeWorker(clone, request), workdirFactory, request.getTargetBranch());
   }
 
   @Override
@@ -72,7 +72,6 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
 
     @Override
     MergeCommandResult run() throws IOException {
-      checkOutTargetBranch();
       MergeResult result = doMergeInClone();
       if (result.getMergeStatus().isSuccessful()) {
         doCommit();
@@ -83,10 +82,6 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand
       }
     }
 
-    private void checkOutTargetBranch() throws IOException {
-      checkOutBranch(target);
-    }
-
     private MergeResult doMergeInClone() throws IOException {
       MergeResult result;
       try {

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

@@ -3,8 +3,6 @@ package sonia.scm.repository.spi;
 import org.apache.commons.lang.StringUtils;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.GitAPIException;
-import org.eclipse.jgit.lib.Constants;
-import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.revwalk.RevCommit;
 import sonia.scm.BadRequestException;
 import sonia.scm.ConcurrentModificationException;
@@ -25,7 +23,6 @@ import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
 import static sonia.scm.AlreadyExistsException.alreadyExists;
 import static sonia.scm.ContextEntry.ContextBuilder.entity;
 import static sonia.scm.NotFoundException.notFound;
-import static sonia.scm.ScmConstraintViolationException.Builder.doThrow;
 
 public class GitModifyCommand extends AbstractGitCommand implements ModifyCommand {
 
@@ -38,7 +35,7 @@ public class GitModifyCommand extends AbstractGitCommand implements ModifyComman
 
   @Override
   public String execute(ModifyCommandRequest request) {
-    return inClone(clone -> new ModifyWorker(clone, request), workdirFactory);
+    return inClone(clone -> new ModifyWorker(clone, request), workdirFactory, request.getBranch());
   }
 
   private class ModifyWorker extends GitCloneWorker<String> implements Worker {
@@ -54,11 +51,6 @@ public class GitModifyCommand extends AbstractGitCommand implements ModifyComman
 
     @Override
     String run() throws IOException {
-      if (!StringUtils.isEmpty(request.getBranch())) {
-        checkOutBranch(request.getBranch());
-      }
-      Ref head = getClone().getRepository().exactRef(Constants.HEAD);
-      doThrow().violation("branch has to be a valid branch, no revision", "branch", request.getBranch()).when(head == null || !head.isSymbolic());
       getClone().getRepository().getFullBranch();
       if (!StringUtils.isEmpty(request.getExpectedRevision())) {
         if (!request.getExpectedRevision().equals(getCurrentRevision().getName())) {

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

@@ -2,6 +2,8 @@ package sonia.scm.repository.spi;
 
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.ScmTransportProtocol;
 import sonia.scm.repository.GitWorkdirFactory;
@@ -11,6 +13,10 @@ import sonia.scm.repository.util.WorkdirProvider;
 
 import javax.inject.Inject;
 import java.io.File;
+import java.io.IOException;
+
+import static sonia.scm.ContextEntry.ContextBuilder.entity;
+import static sonia.scm.NotFoundException.notFound;
 
 public class SimpleGitWorkdirFactory extends SimpleWorkdirFactory<Repository, GitContext> implements GitWorkdirFactory {
 
@@ -20,14 +26,23 @@ public class SimpleGitWorkdirFactory extends SimpleWorkdirFactory<Repository, Gi
   }
 
   @Override
-  public ParentAndClone<Repository> cloneRepository(GitContext context, File target) {
+  public ParentAndClone<Repository> cloneRepository(GitContext context, File target, String initialBranch) {
     try {
-      return new ParentAndClone<>(null, Git.cloneRepository()
+      Repository clone = Git.cloneRepository()
         .setURI(createScmTransportProtocolUri(context.getDirectory()))
         .setDirectory(target)
+        .setBranch(initialBranch)
         .call()
-        .getRepository());
-    } catch (GitAPIException e) {
+        .getRepository();
+
+      Ref head = clone.exactRef(Constants.HEAD);
+
+      if (head == null || !head.isSymbolic() || (initialBranch != null && !head.getTarget().getName().endsWith(initialBranch))) {
+        throw notFound(entity("Branch", initialBranch).in(context.getRepository()));
+      }
+
+      return new ParentAndClone<>(null, clone);
+    } catch (GitAPIException | IOException e) {
       throw new InternalRepositoryException(context.getRepository(), "could not clone working copy of repository", e);
     }
   }

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

@@ -263,8 +263,8 @@ public class GitModifyCommandTest extends AbstractGitCommandTestBase {
     command.execute(request);
   }
 
-  @Test(expected = ScmConstraintViolationException.class)
-  public void shouldFailWithConstraintViolationIfBranchIsNoBranch() throws IOException {
+  @Test(expected = NotFoundException.class)
+  public void shouldFailWithNotFoundExceptionIfBranchIsNoBranch() throws IOException {
     File newFile = Files.write(temporaryFolder.newFile().toPath(), "irrelevant\n".getBytes()).toFile();
 
     GitModifyCommand command = createCommand();

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

@@ -0,0 +1,84 @@
+package sonia.scm.repository.spi;
+
+import com.github.sdorra.shiro.ShiroRule;
+import com.github.sdorra.shiro.SubjectAware;
+import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.errors.CorruptObjectException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.CanonicalTreeParser;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import sonia.scm.ScmConstraintViolationException;
+import sonia.scm.repository.Person;
+import sonia.scm.repository.util.WorkdirProvider;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+@SubjectAware(configuration = "classpath:sonia/scm/configuration/shiro.ini", username = "admin", password = "secret")
+public class GitModifyCommand_withEmptyRepositoryTest extends AbstractGitCommandTestBase {
+
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
+  @Rule
+  public BindTransportProtocolRule transportProtocolRule = new BindTransportProtocolRule();
+  @Rule
+  public ShiroRule shiro = new ShiroRule();
+
+  @Test
+  public void shouldCreateNewFileInEmptyRepository() throws IOException, GitAPIException {
+    File newFile = Files.write(temporaryFolder.newFile().toPath(), "new content".getBytes()).toFile();
+
+    GitModifyCommand command = createCommand();
+
+    ModifyCommandRequest request = new ModifyCommandRequest();
+    request.setCommitMessage("test commit");
+    request.addRequest(new ModifyCommandRequest.CreateFileRequest("new_file", newFile, false));
+    request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det"));
+
+    command.execute(request);
+
+    TreeAssertions assertions = canonicalTreeParser -> assertThat(canonicalTreeParser.findFile("new_file")).isTrue();
+
+    assertInTree(assertions);
+  }
+
+  @Override
+  protected String getZippedRepositoryResource() {
+    return "sonia/scm/repository/spi/scm-git-empty-repo.zip";
+  }
+
+  private void assertInTree(TreeAssertions assertions) throws IOException, GitAPIException {
+    try (Git git = new Git(createContext().open())) {
+      RevCommit lastCommit = getLastCommit(git);
+      try (RevWalk walk = new RevWalk(git.getRepository())) {
+        RevCommit commit = walk.parseCommit(lastCommit);
+        ObjectId treeId = commit.getTree().getId();
+        try (ObjectReader reader = git.getRepository().newObjectReader()) {
+          assertions.checkAssertions(new CanonicalTreeParser(null, reader, treeId));
+        }
+      }
+    }
+  }
+
+  private RevCommit getLastCommit(Git git) throws GitAPIException {
+    return git.log().setMaxCount(1).call().iterator().next();
+  }
+
+  private GitModifyCommand createCommand() {
+    return new GitModifyCommand(createContext(), repository, new SimpleGitWorkdirFactory(new WorkdirProvider()));
+  }
+
+  @FunctionalInterface
+  private interface TreeAssertions {
+    void checkAssertions(CanonicalTreeParser treeParser) throws CorruptObjectException;
+  }
+}

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

@@ -20,8 +20,6 @@ import java.io.IOException;
 import static com.google.inject.util.Providers.of;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.verify;
 
 public class SimpleGitWorkdirFactoryTest extends AbstractGitCommandTestBase {
 
@@ -43,11 +41,11 @@ public class SimpleGitWorkdirFactoryTest extends AbstractGitCommandTestBase {
   }
 
   @Test
-  public void emptyPoolShouldCreateNewWorkdir() throws IOException {
+  public void emptyPoolShouldCreateNewWorkdir() {
     SimpleGitWorkdirFactory factory = new SimpleGitWorkdirFactory(workdirProvider);
     File masterRepo = createRepositoryDirectory();
 
-    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext())) {
+    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext(), null)) {
 
       assertThat(workingCopy.getDirectory())
         .exists()
@@ -61,25 +59,37 @@ public class SimpleGitWorkdirFactoryTest extends AbstractGitCommandTestBase {
   }
 
   @Test
-  public void cloneFromPoolShouldNotBeReused() throws IOException {
+  public void shouldCheckoutInitialBranch() {
+    SimpleGitWorkdirFactory factory = new SimpleGitWorkdirFactory(workdirProvider);
+
+    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext(), "test-branch")) {
+      assertThat(new File(workingCopy.getWorkingRepository().getWorkTree(), "a.txt"))
+        .exists()
+        .isFile()
+        .hasContent("a and b");
+    }
+  }
+
+  @Test
+  public void cloneFromPoolShouldNotBeReused() {
     SimpleGitWorkdirFactory factory = new SimpleGitWorkdirFactory(workdirProvider);
 
     File firstDirectory;
-    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext())) {
+    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext(), null)) {
       firstDirectory = workingCopy.getDirectory();
     }
-    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext())) {
+    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext(), null)) {
       File secondDirectory = workingCopy.getDirectory();
       assertThat(secondDirectory).isNotEqualTo(firstDirectory);
     }
   }
 
   @Test
-  public void cloneFromPoolShouldBeDeletedOnClose() throws IOException {
+  public void cloneFromPoolShouldBeDeletedOnClose() {
     SimpleGitWorkdirFactory factory = new SimpleGitWorkdirFactory(workdirProvider);
 
     File directory;
-    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext())) {
+    try (WorkingCopy<Repository> workingCopy = factory.createWorkingCopy(createContext(), null)) {
       directory = workingCopy.getWorkingRepository().getWorkTree();
     }
     assertThat(directory).doesNotExist();