fix redundant git repo closing (#1789)

Both the GitLogCommand and the GitModificationsCommand incorrectly closed the underlying git repository. This caused the git repository to be opened once but closed twice, which in turn flooded the logs with avoidable warnings and affected performance. The redundant closing of the git repo has been removed from both commands.

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

@@ -24,6 +24,7 @@
 
 package sonia.scm.repository.spi;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import sonia.scm.api.v2.resources.GitRepositoryConfigStoreProvider;
@@ -119,6 +120,11 @@ public class GitContext implements Closeable, RepositoryProvider
     storeProvider.get(repository).set(newConfig);
   }
 
+  @VisibleForTesting
+  void setGitRepository(org.eclipse.jgit.lib.Repository gitRepository) {
+    this.gitRepository = gitRepository;
+  }
+
   GitConfig getGlobalConfig() {
     return config;
   }

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

@@ -91,6 +91,7 @@ public class GitLogCommand extends AbstractGitCommand implements LogCommand
    * @return
    */
   @Override
+  @SuppressWarnings("java:S2093")
   public Changeset getChangeset(String revision, LogCommandRequest request)
   {
     if (logger.isDebugEnabled())
@@ -147,7 +148,6 @@ public class GitLogCommand extends AbstractGitCommand implements LogCommand
     {
       IOUtil.close(converter);
       GitUtil.release(revWalk);
-      GitUtil.close(gr);
     }
 
     return changeset;
@@ -176,13 +176,13 @@ public class GitLogCommand extends AbstractGitCommand implements LogCommand
    * @throws IOException
    */
   @Override
-  @SuppressWarnings("unchecked")
+  @SuppressWarnings("java:S2093")
   public ChangesetPagingResult getChangesets(LogCommandRequest request) {
-    try (org.eclipse.jgit.lib.Repository gitRepository = open()) {
+    try {
       if (Strings.isNullOrEmpty(request.getBranch())) {
         request.setBranch(context.getConfig().getDefaultBranch());
       }
-      return new GitLogComputer(this.repository.getId(), gitRepository, converterFactory).compute(request);
+      return new GitLogComputer(this.repository.getId(), open(), converterFactory).compute(request);
     } catch (IOException e) {
       throw new InternalRepositoryException(repository, "could not create change log", e);
     }

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

@@ -62,6 +62,7 @@ public class GitModificationsCommand extends AbstractGitCommand implements Modif
   }
 
   @Override
+  @SuppressWarnings("java:S2093")
   public Modifications getModifications(String baseRevision, String revision) {
     org.eclipse.jgit.lib.Repository gitRepository = null;
     RevWalk revWalk = null;
@@ -84,7 +85,6 @@ public class GitModificationsCommand extends AbstractGitCommand implements Modif
       throw new InternalRepositoryException(entity(repository), "could not open repository", ex);
     } finally {
       GitUtil.release(revWalk);
-      GitUtil.close(gitRepository);
     }
     return null;
   }

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

@@ -50,8 +50,8 @@ import java.io.File;
 import java.io.IOException;
 
 import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 
 //~--- JDK imports ------------------------------------------------------------
 
@@ -83,9 +83,9 @@ public class AbstractRemoteCommandTestBase {
     eventFactory = mock(GitRepositoryHookEventFactory.class);
 
     handler = mock(GitRepositoryHandler.class);
-    when(handler.getDirectory(incomingRepository.getId())).thenReturn(
+    lenient().when(handler.getDirectory(incomingRepository.getId())).thenReturn(
       incomingDirectory);
-    when(handler.getDirectory(outgoingRepository.getId())).thenReturn(
+    lenient().when(handler.getDirectory(outgoingRepository.getId())).thenReturn(
       outgoingDirectory);
   }
 

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

@@ -25,13 +25,14 @@
 package sonia.scm.repository.spi;
 
 import com.google.common.io.Files;
+import org.eclipse.jgit.lib.Repository;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
 import sonia.scm.repository.Changeset;
 import sonia.scm.repository.ChangesetPagingResult;
-import sonia.scm.repository.GitChangesetConverterFactory;
 import sonia.scm.repository.GitRepositoryConfig;
 import sonia.scm.repository.GitTestHelper;
 import sonia.scm.repository.Modifications;
@@ -42,11 +43,13 @@ import java.io.IOException;
 
 import static java.nio.charset.Charset.defaultCharset;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.hamcrest.Matchers.contains;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
@@ -91,6 +94,28 @@ public class GitLogCommandTest extends AbstractGitCommandTestBase
     assertTrue(result.getChangesets().stream().allMatch(r -> r.getBranches().isEmpty()));
   }
 
+  @Test
+  public void shouldNotCloseRepositoryForChangesetCollection() throws IOException {
+    final GitLogCommand logCommand = createCommandWithContextSpy();
+    final Repository repository = Mockito.spy(logCommand.context.open());
+    logCommand.context.setGitRepository(repository);
+    logCommand.getChangesets(new LogCommandRequest());
+
+    verify(repository, never()).close();
+    verify(logCommand.context, times(2)).open();
+  }
+
+  @Test
+  public void shouldNotCloseRepositoryForSingleChangeset() throws IOException {
+    final GitLogCommand logCommand = createCommandWithContextSpy();
+    final Repository repository = Mockito.spy(logCommand.context.open());
+    logCommand.context.setGitRepository(repository);
+    logCommand.getChangeset("435df2f061add3589cb3", null);
+
+    verify(repository, never()).close();
+    verify(logCommand.context, times(2)).open();
+  }
+
   @Test
   public void testGetAll()
   {
@@ -298,4 +323,8 @@ public class GitLogCommandTest extends AbstractGitCommandTestBase
   private GitLogCommand createCommand() {
     return new GitLogCommand(createContext(), GitTestHelper.createConverterFactory());
   }
+
+  private GitLogCommand createCommandWithContextSpy() {
+    return new GitLogCommand(Mockito.spy(createContext()), GitTestHelper.createConverterFactory());
+  }
 }

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

@@ -24,9 +24,14 @@
 
 package sonia.scm.repository.spi;
 
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
 import sonia.scm.repository.GitConfig;
 import sonia.scm.repository.Modifications;
 
@@ -35,7 +40,10 @@ import java.io.IOException;
 import java.util.function.Consumer;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
 
+@RunWith(MockitoJUnitRunner.class)
 public class GitModificationsCommandTest extends AbstractRemoteCommandTestBase {
 
   private GitModificationsCommand incomingModificationsCommand;
@@ -43,8 +51,25 @@ public class GitModificationsCommandTest extends AbstractRemoteCommandTestBase {
 
   @Before
   public void init() {
-    incomingModificationsCommand = new GitModificationsCommand(new GitContext(incomingDirectory, incomingRepository, null, new GitConfig()));
-    outgoingModificationsCommand = new GitModificationsCommand(new GitContext(outgoingDirectory, outgoingRepository, null, new GitConfig()));
+    incomingModificationsCommand = new GitModificationsCommand(Mockito.spy(new GitContext(incomingDirectory, incomingRepository, null, new GitConfig())));
+    outgoingModificationsCommand = new GitModificationsCommand(Mockito.spy(new GitContext(outgoingDirectory, outgoingRepository, null, new GitConfig())));
+  }
+
+  @Test
+  public void shouldNotCloseRepository() throws IOException, GitAPIException {
+    write(outgoing, outgoingDirectory, "a.txt", "bal bla");
+    RevCommit addedFileCommit = commit(outgoing, "add file");
+    String revision = addedFileCommit.getName();
+
+    final GitModificationsCommand command = new GitModificationsCommand(Mockito.spy(new GitContext(outgoingDirectory, outgoingRepository, null, new GitConfig())));
+    final Repository repository = Mockito.spy(command.context.open());
+    command.context.setGitRepository(repository);
+
+    command.getModifications(revision);
+
+    Mockito.verify(command.context, times(3)).open();
+    Mockito.verify(repository, never()).close();
+
   }
 
   @Test