From 00b27b548835e599b781d47b6b2c784056ba34a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= <rene.pfeuffer@cloudogu.com>
Date: Fri, 29 Mar 2019 11:47:44 +0100
Subject: [PATCH] Close repositories after usage

---
 .../repository/util/SimpleWorkdirFactory.java |  4 +-
 .../scm/repository/util/WorkingCopy.java      |  4 +
 .../util/SimpleWorkdirFactoryTest.java        | 75 +++++++++++++++++++
 .../spi/SimpleGitWorkdirFactory.java          |  6 +-
 4 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100644 scm-core/src/test/java/sonia/scm/repository/util/SimpleWorkdirFactoryTest.java

diff --git a/scm-core/src/main/java/sonia/scm/repository/util/SimpleWorkdirFactory.java b/scm-core/src/main/java/sonia/scm/repository/util/SimpleWorkdirFactory.java
index de4ae6069d..9642f72ae7 100644
--- a/scm-core/src/main/java/sonia/scm/repository/util/SimpleWorkdirFactory.java
+++ b/scm-core/src/main/java/sonia/scm/repository/util/SimpleWorkdirFactory.java
@@ -39,7 +39,9 @@ public abstract class SimpleWorkdirFactory<R, C> implements WorkdirFactory<R, C>
 
   protected abstract Repository getScmRepository(C context);
 
-  protected abstract void closeRepository(R repository);
+  @SuppressWarnings("squid:S00112")
+  // We do allow implementations to throw arbitrary exceptions here, so that we can handle them in close
+  protected abstract void closeRepository(R repository) throws Exception;
 
   protected abstract ParentAndClone<R> cloneRepository(C context, File target) throws IOException;
 
diff --git a/scm-core/src/main/java/sonia/scm/repository/util/WorkingCopy.java b/scm-core/src/main/java/sonia/scm/repository/util/WorkingCopy.java
index 353fca30d2..3c96184142 100644
--- a/scm-core/src/main/java/sonia/scm/repository/util/WorkingCopy.java
+++ b/scm-core/src/main/java/sonia/scm/repository/util/WorkingCopy.java
@@ -15,11 +15,13 @@ public class WorkingCopy<R> implements AutoCloseable {
   private final File directory;
   private final R workingRepository;
   private final R centralRepository;
+  private final Consumer<R> cleanup;
 
   public WorkingCopy(R workingRepository, R centralRepository, Consumer<R> cleanup, File directory) {
     this.directory = directory;
     this.workingRepository = workingRepository;
     this.centralRepository = centralRepository;
+    this.cleanup = cleanup;
   }
 
   public R getWorkingRepository() {
@@ -37,6 +39,8 @@ public class WorkingCopy<R> implements AutoCloseable {
   @Override
   public void close() {
     try {
+      cleanup.accept(workingRepository);
+      cleanup.accept(centralRepository);
       IOUtil.delete(directory);
     } catch (IOException e) {
       LOG.warn("could not delete temporary workdir '{}'", directory, e);
diff --git a/scm-core/src/test/java/sonia/scm/repository/util/SimpleWorkdirFactoryTest.java b/scm-core/src/test/java/sonia/scm/repository/util/SimpleWorkdirFactoryTest.java
new file mode 100644
index 0000000000..2d3c2ed59e
--- /dev/null
+++ b/scm-core/src/test/java/sonia/scm/repository/util/SimpleWorkdirFactoryTest.java
@@ -0,0 +1,75 @@
+package sonia.scm.repository.util;
+
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import sonia.scm.repository.Repository;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+public class SimpleWorkdirFactoryTest {
+
+  private static final Repository REPOSITORY = new Repository("1", "git", "space", "X");
+
+  private final Closeable parent = mock(Closeable.class);
+  private final Closeable clone = mock(Closeable.class);
+
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
+  private SimpleWorkdirFactory<Closeable, Context> simpleWorkdirFactory;
+
+  @Before
+  public void initFactory() throws IOException {
+    simpleWorkdirFactory = new SimpleWorkdirFactory<Closeable, Context>(temporaryFolder.newFolder()) {
+      @Override
+      protected Repository getScmRepository(Context context) {
+        return REPOSITORY;
+      }
+
+      @Override
+      protected void closeRepository(Closeable repository) throws IOException {
+        repository.close();
+      }
+
+      @Override
+      protected ParentAndClone<Closeable> cloneRepository(Context context, File target) {
+        return new ParentAndClone<>(parent, clone);
+      }
+    };
+  }
+
+  @Test
+  public void shouldCreateParentAndClone() {
+    Context context = new Context();
+    try (WorkingCopy<Closeable> workingCopy = simpleWorkdirFactory.createWorkingCopy(context)) {
+      assertThat(workingCopy.getCentralRepository()).isSameAs(parent);
+      assertThat(workingCopy.getWorkingRepository()).isSameAs(clone);
+    }
+  }
+
+  @Test
+  public void shouldCloseParent() throws IOException {
+    Context context = new Context();
+    try (WorkingCopy<Closeable> workingCopy = simpleWorkdirFactory.createWorkingCopy(context)) {}
+
+    verify(parent).close();
+  }
+
+  @Test
+  public void shouldCloseClone() throws IOException {
+    Context context = new Context();
+    try (WorkingCopy<Closeable> workingCopy = simpleWorkdirFactory.createWorkingCopy(context)) {}
+
+    verify(clone).close();
+  }
+
+  private static class Context {}
+}
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
index a4d1ab830e..c30ff59afb 100644
--- 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
@@ -38,7 +38,11 @@ public class SimpleGitWorkdirFactory extends SimpleWorkdirFactory<Repository, Gi
 
   @Override
   protected void closeRepository(Repository repository) {
-    repository.close();
+    // we have to check for null here, because we do not create a repository for
+    // the parent in cloneRepository
+    if (repository != null) {
+      repository.close();
+    }
   }
 
   @Override