From 9ad501b4c674b9d31f7e190ca3101edddd895099 Mon Sep 17 00:00:00 2001 From: Konstantin Schaper Date: Wed, 1 Sep 2021 09:44:11 +0200 Subject: [PATCH] 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. --- .../changelog/redundant_git_repo_closing.yaml | 2 ++ .../sonia/scm/repository/spi/GitContext.java | 6 ++++ .../scm/repository/spi/GitLogCommand.java | 8 ++--- .../spi/GitModificationsCommand.java | 2 +- .../spi/AbstractRemoteCommandTestBase.java | 6 ++-- .../scm/repository/spi/GitLogCommandTest.java | 33 +++++++++++++++++-- .../spi/GitModificationsCommandTest.java | 29 ++++++++++++++-- 7 files changed, 74 insertions(+), 12 deletions(-) create mode 100644 gradle/changelog/redundant_git_repo_closing.yaml 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