diff --git a/gradle/changelog/redundant_git_repo_closing.yaml b/gradle/changelog/redundant_git_repo_closing.yaml new file mode 100644 index 0000000000..8650cfc926 --- /dev/null +++ b/gradle/changelog/redundant_git_repo_closing.yaml @@ -0,0 +1,2 @@ +- type: Fixed + description: redundant git repo closing in some commands ([#1789](https://github.com/scm-manager/scm-manager/pull/1789)) 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 4246dd4ff3..175ed878b3 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 @@ -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; } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitLogCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitLogCommand.java index e0f0b2868b..e9786a7bce 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitLogCommand.java +++ b/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); } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitModificationsCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitModificationsCommand.java index cbe17a0e46..f972172d22 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitModificationsCommand.java +++ b/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; } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/AbstractRemoteCommandTestBase.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/AbstractRemoteCommandTestBase.java index 5da5c8aaf4..d2abb6318e 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/AbstractRemoteCommandTestBase.java +++ b/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); } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitLogCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitLogCommandTest.java index b15c6ecebd..a0df19f6cb 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitLogCommandTest.java +++ b/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()); + } } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModificationsCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModificationsCommandTest.java index 114fd2e82b..efe3888f2f 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModificationsCommandTest.java +++ b/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