diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitRepositoryHandler.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitRepositoryHandler.java index 00add44466..faf2b23a27 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitRepositoryHandler.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitRepositoryHandler.java @@ -91,7 +91,7 @@ public class GitRepositoryHandler private final Scheduler scheduler; - private final GitWorkdirPool workdirPool; + private final GitWorkdirFactory workdirFactory; private Task task; @@ -106,11 +106,11 @@ public class GitRepositoryHandler * @param scheduler */ @Inject - public GitRepositoryHandler(ConfigurationStoreFactory storeFactory, FileSystem fileSystem, Scheduler scheduler, GitWorkdirPool workdirPool) + public GitRepositoryHandler(ConfigurationStoreFactory storeFactory, FileSystem fileSystem, Scheduler scheduler, GitWorkdirFactory workdirFactory) { super(storeFactory, fileSystem); this.scheduler = scheduler; - this.workdirPool = workdirPool; + this.workdirFactory = workdirFactory; } //~--- get methods ---------------------------------------------------------- @@ -239,7 +239,7 @@ public class GitRepositoryHandler return new File(directory, DIRECTORY_REFS).exists(); } - public GitWorkdirPool getWorkdirPool() { - return workdirPool; + public GitWorkdirFactory getWorkdirFactory() { + return workdirFactory; } } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitWorkdirFactory.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitWorkdirFactory.java new file mode 100644 index 0000000000..f93713a221 --- /dev/null +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitWorkdirFactory.java @@ -0,0 +1,8 @@ +package sonia.scm.repository; + +import sonia.scm.repository.spi.GitContext; +import sonia.scm.repository.spi.WorkingCopy; + +public interface GitWorkdirFactory { + WorkingCopy createWorkingCopy(GitContext gitContext); +} diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitWorkdirPool.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitWorkdirPool.java deleted file mode 100644 index 2c94a75be5..0000000000 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitWorkdirPool.java +++ /dev/null @@ -1,46 +0,0 @@ -package sonia.scm.repository; - -import org.eclipse.jgit.api.Git; -import org.eclipse.jgit.api.errors.GitAPIException; -import org.eclipse.jgit.lib.Repository; - -import java.io.File; -import java.util.Random; - - -/** - * Config: - * - * 1. Overall and absolute maximum of temp work directories - * 2. Maximum number of temp work directories pooled overall - * 3. Maximum number of temp work directories pooled for one master repository - */ -public class GitWorkdirPool { - - private final Random random = new Random(); - private final File poolDirectory; - - public GitWorkdirPool() { - this(new File(System.getProperty("java.io.tmpdir"))); - } - - public GitWorkdirPool(File poolDirectory) { - this.poolDirectory = poolDirectory; - } - - public CloseableWrapper getWorkingCopy(File bareRepository) { - try { - Git clone = cloneRepository(bareRepository, new File(poolDirectory, Long.toString(random.nextLong()))); - return new CloseableWrapper<>(clone.getRepository(), r -> clone.close()); - } catch (GitAPIException e) { - throw new InternalRepositoryException("could not clone working copy of repository", e); - } - } - - protected Git cloneRepository(File bareRepository, File target) throws GitAPIException { - return Git.cloneRepository() - .setURI(bareRepository.getAbsolutePath()) - .setDirectory(target) - .call(); - } -} diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitContext.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitContext.java index 2175846d5a..a10e16e200 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitContext.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitContext.java @@ -106,6 +106,10 @@ public class GitContext implements Closeable return repository; } + File getDirectory() { + return directory; + } + //~--- fields --------------------------------------------------------------- /** Field description */ diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeCommand.java index ba1cc0e496..6e69a2c5ba 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitMergeCommand.java @@ -2,10 +2,9 @@ package sonia.scm.repository.spi; import org.eclipse.jgit.merge.MergeStrategy; import org.eclipse.jgit.merge.ResolveMerger; -import sonia.scm.repository.CloseableWrapper; -import sonia.scm.repository.GitWorkdirPool; +import org.eclipse.jgit.lib.Repository; +import sonia.scm.repository.GitWorkdirFactory; import sonia.scm.repository.InternalRepositoryException; -import sonia.scm.repository.Repository; import sonia.scm.repository.api.MergeCommandResult; import sonia.scm.repository.api.MergeDryRunCommandResult; @@ -13,17 +12,17 @@ import java.io.IOException; public class GitMergeCommand extends AbstractGitCommand implements MergeCommand { - private final GitWorkdirPool workdirPool; + private final GitWorkdirFactory workdirPool; - GitMergeCommand(GitContext context, Repository repository, GitWorkdirPool workdirPool) { + GitMergeCommand(GitContext context, sonia.scm.repository.Repository repository, GitWorkdirFactory workdirPool) { super(context, repository); this.workdirPool = workdirPool; } @Override public MergeCommandResult merge(MergeCommandRequest request) { - try (CloseableWrapper workingCopy = workdirPool.getWorkingCopy(context.open().getDirectory())) { - org.eclipse.jgit.lib.Repository repository = workingCopy.get(); + try (WorkingCopy workingCopy = workdirPool.createWorkingCopy(context)) { + Repository repository = workingCopy.get(); ResolveMerger merger = (ResolveMerger) MergeStrategy.RECURSIVE.newMerger(repository); boolean mergeResult = merger.merge(repository.resolve(request.getBranchToMerge()), repository.resolve(request.getTargetBranch())); if (mergeResult) { @@ -38,7 +37,7 @@ public class GitMergeCommand extends AbstractGitCommand implements MergeCommand @Override public MergeDryRunCommandResult dryRun(MergeCommandRequest request) { try { - org.eclipse.jgit.lib.Repository repository = context.open(); + Repository repository = context.open(); ResolveMerger merger = (ResolveMerger) MergeStrategy.RECURSIVE.newMerger(repository, true); return new MergeDryRunCommandResult(merger.merge(repository.resolve(request.getBranchToMerge()), repository.resolve(request.getTargetBranch()))); } catch (IOException e) { diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java index c29da1029f..846c127c4c 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java @@ -243,7 +243,7 @@ public class GitRepositoryServiceProvider extends RepositoryServiceProvider @Override public MergeCommand getMergeCommand() { - return new GitMergeCommand(context, repository, handler.getWorkdirPool()); + return new GitMergeCommand(context, repository, handler.getWorkdirFactory()); } //~--- fields --------------------------------------------------------------- diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/SimpleGitWorkdirFactory.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/SimpleGitWorkdirFactory.java new file mode 100644 index 0000000000..c69acf5c6e --- /dev/null +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/SimpleGitWorkdirFactory.java @@ -0,0 +1,56 @@ +package sonia.scm.repository.spi; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.util.FileUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import sonia.scm.repository.GitWorkdirFactory; +import sonia.scm.repository.InternalRepositoryException; + +import java.io.File; +import java.io.IOException; +import java.util.Random; + +public class SimpleGitWorkdirFactory implements GitWorkdirFactory { + + private static final Logger logger = LoggerFactory.getLogger(SimpleGitWorkdirFactory.class); + + private final Random random = new Random(); + private final File poolDirectory; + + public SimpleGitWorkdirFactory() { + this(new File(System.getProperty("java.io.tmpdir"), "scmm-git-pool")); + } + + public SimpleGitWorkdirFactory(File poolDirectory) { + this.poolDirectory = poolDirectory; + } + + public WorkingCopy createWorkingCopy(GitContext gitContext) { + try { + Repository clone = cloneRepository(gitContext.getDirectory(), new File(poolDirectory, Long.toString(random.nextLong()))); + return new WorkingCopy(clone, this::close); + } catch (GitAPIException e) { + throw new InternalRepositoryException("could not clone working copy of repository", e); + } + } + + protected Repository cloneRepository(File bareRepository, File target) throws GitAPIException { + return Git.cloneRepository() + .setURI(bareRepository.getAbsolutePath()) + .setDirectory(target) + .call() + .getRepository(); + } + + private void close(Repository repository) { + repository.close(); + try { + FileUtils.delete(repository.getDirectory(), FileUtils.RECURSIVE); + } catch (IOException e) { + logger.warn("could not delete temporary git workdir '{}'", repository.getDirectory(), e); + } + } +} diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/WorkingCopy.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/WorkingCopy.java new file mode 100644 index 0000000000..fd0cba510b --- /dev/null +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/WorkingCopy.java @@ -0,0 +1,12 @@ +package sonia.scm.repository.spi; + +import org.eclipse.jgit.lib.Repository; +import sonia.scm.repository.CloseableWrapper; + +import java.util.function.Consumer; + +public class WorkingCopy extends CloseableWrapper { + WorkingCopy(Repository wrapped, Consumer cleanup) { + super(wrapped, cleanup); + } +} diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/GitRepositoryHandlerTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/GitRepositoryHandlerTest.java index 26c449835b..ad731280c5 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/GitRepositoryHandlerTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/GitRepositoryHandlerTest.java @@ -62,7 +62,7 @@ public class GitRepositoryHandlerTest extends SimpleRepositoryHandlerTestBase { private ConfigurationStoreFactory factory; @Mock - private GitWorkdirPool gitWorkdirPool; + private GitWorkdirFactory gitWorkdirFactory; @Override protected void checkDirectory(File directory) { @@ -87,7 +87,7 @@ public class GitRepositoryHandlerTest extends SimpleRepositoryHandlerTestBase { protected RepositoryHandler createRepositoryHandler(ConfigurationStoreFactory factory, File directory) { GitRepositoryHandler repositoryHandler = new GitRepositoryHandler(factory, - new DefaultFileSystem(), scheduler, gitWorkdirPool); + new DefaultFileSystem(), scheduler, gitWorkdirFactory); repositoryHandler.init(contextProvider); @@ -103,7 +103,7 @@ public class GitRepositoryHandlerTest extends SimpleRepositoryHandlerTestBase { @Test public void getDirectory() { GitRepositoryHandler repositoryHandler = new GitRepositoryHandler(factory, - new DefaultFileSystem(), scheduler, gitWorkdirPool); + new DefaultFileSystem(), scheduler, gitWorkdirFactory); GitConfig gitConfig = new GitConfig(); gitConfig.setRepositoryDirectory(new File("/path")); diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/GitWorkdirPoolTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/GitWorkdirPoolTest.java deleted file mode 100644 index 133df75ea7..0000000000 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/GitWorkdirPoolTest.java +++ /dev/null @@ -1,59 +0,0 @@ -package sonia.scm.repository; - -import org.eclipse.jgit.api.Git; -import org.eclipse.jgit.api.errors.GitAPIException; -import org.eclipse.jgit.lib.Repository; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; -import sonia.scm.repository.spi.AbstractGitCommandTestBase; - -import java.io.File; -import java.io.IOException; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; - -public class GitWorkdirPoolTest extends AbstractGitCommandTestBase { - - @Rule - public TemporaryFolder temporaryFolder = new TemporaryFolder(); - - @Test - public void emptyPoolShouldCreateNewWorkdir() throws IOException { - GitWorkdirPool pool = new GitWorkdirPool(temporaryFolder.newFolder()); - File masterRepo = createRepositoryDirectory(); - - CloseableWrapper workingCopy = pool.getWorkingCopy(masterRepo); - - assertThat(workingCopy) - .isNotNull() - .extracting(w -> w.get().getDirectory()) - .isNotEqualTo(masterRepo); - } - - @Test - public void cloneFromPoolShouldBeClosed() throws IOException { - PoolWithSpy pool = new PoolWithSpy(temporaryFolder.newFolder()); - File masterRepo = createRepositoryDirectory(); - - try (CloseableWrapper workingCopy = pool.getWorkingCopy(masterRepo)) { - assertThat(workingCopy).isNotNull(); - } - verify(pool.createdClone).close(); - } - - private static class PoolWithSpy extends GitWorkdirPool { - PoolWithSpy(File poolDirectory) { - super(poolDirectory); - } - - Git createdClone; - @Override - protected Git cloneRepository(File bareRepository, File destination) throws GitAPIException { - createdClone = spy(super.cloneRepository(bareRepository, destination)); - return createdClone; - } - } -} diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/SimpleGitWorkdirFactoryTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/SimpleGitWorkdirFactoryTest.java new file mode 100644 index 0000000000..d9c41f85b5 --- /dev/null +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/SimpleGitWorkdirFactoryTest.java @@ -0,0 +1,87 @@ +package sonia.scm.repository.spi; + +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.lib.Repository; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; + +public class SimpleGitWorkdirFactoryTest extends AbstractGitCommandTestBase { + + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); + + @Test + public void emptyPoolShouldCreateNewWorkdir() throws IOException { + SimpleGitWorkdirFactory factory = new SimpleGitWorkdirFactory(temporaryFolder.newFolder()); + File masterRepo = createRepositoryDirectory(); + + try (WorkingCopy workingCopy = factory.createWorkingCopy(createContext())) { + + assertThat(workingCopy.get().getDirectory()) + .exists() + .isNotEqualTo(masterRepo) + .isDirectory(); + assertThat(new File(workingCopy.get().getWorkTree(), "a.txt")) + .exists() + .isFile() + .hasContent("a\nline for blame"); + } + } + + @Test + public void cloneFromPoolShouldBeClosed() throws IOException { + PoolWithSpy factory = new PoolWithSpy(temporaryFolder.newFolder()); + + try (WorkingCopy workingCopy = factory.createWorkingCopy(createContext())) { + assertThat(workingCopy).isNotNull(); + } + verify(factory.createdClone).close(); + } + + @Test + public void cloneFromPoolShouldNotBeReused() throws IOException { + SimpleGitWorkdirFactory factory = new SimpleGitWorkdirFactory(temporaryFolder.newFolder()); + + File firstDirectory; + try (WorkingCopy workingCopy = factory.createWorkingCopy(createContext())) { + firstDirectory = workingCopy.get().getDirectory(); + } + try (WorkingCopy workingCopy = factory.createWorkingCopy(createContext())) { + File secondDirectory = workingCopy.get().getDirectory(); + assertThat(secondDirectory).isNotEqualTo(firstDirectory); + } + } + + @Test + public void cloneFromPoolShouldBeDeletedOnClose() throws IOException { + SimpleGitWorkdirFactory factory = new SimpleGitWorkdirFactory(temporaryFolder.newFolder()); + + File directory; + try (WorkingCopy workingCopy = factory.createWorkingCopy(createContext())) { + directory = workingCopy.get().getDirectory(); + } + assertThat(directory).doesNotExist(); + } + + private static class PoolWithSpy extends SimpleGitWorkdirFactory { + PoolWithSpy(File poolDirectory) { + super(poolDirectory); + } + + Repository createdClone; + + @Override + protected Repository cloneRepository(File bareRepository, File destination) throws GitAPIException { + createdClone = spy(super.cloneRepository(bareRepository, destination)); + return createdClone; + } + } +}