From 7a7ac9a3f2578aad4b6f405cff64ab9ed4dc27f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 3 Feb 2021 11:08:13 +0100 Subject: [PATCH] Bugfix: Conflict in pack parser (#1518) The change tackles a sporadic error in integration tests, where during or after a change in a git repository a pack file could not be read correctly due to a "short compressed stream". The exception could look like this: ERROR org.eclipse.jgit.internal.storage.file.ObjectDirectory - Exception caught while accessing pack file scm-home/repositories/8gSNr4ogc5X/data/objects/pack/pack-51a3500283ece83ab8efa7edfb9370e6f97311b5.pack, the pack file might be corrupt. Caught 1 consecutive errors while trying to read this pack. java.io.EOFException: Short compressed stream at 197 at org.eclipse.jgit.internal.storage.file.PackFile.decompress(PackFile.java:381) at org.eclipse.jgit.internal.storage.file.PackFile.load(PackFile.java:815) at org.eclipse.jgit.internal.storage.file.PackFile.get(PackFile.java:284) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openPackedObject(ObjectDirectory.java:455) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openPackedFromSelfOrAlternate(ObjectDirectory.java:413) at org.eclipse.jgit.internal.storage.file.ObjectDirectory.openObject(ObjectDirectory.java:404) at org.eclipse.jgit.internal.storage.file.WindowCursor.open(WindowCursor.java:132) at org.eclipse.jgit.treewalk.CanonicalTreeParser.reset(CanonicalTreeParser.java:191) at org.eclipse.jgit.treewalk.TreeWalk.parserFor(TreeWalk.java:1344) at org.eclipse.jgit.treewalk.TreeWalk.addTree(TreeWalk.java:732) at sonia.scm.repository.spi.Differ.create(Differ.java:102) at sonia.scm.repository.spi.Differ.diff(Differ.java:63) We found out that this seems to be linked to the asynchronous execution of post commit hooks, that originally are triggered in the midst of the processing of a receive pack. With this change the fireing of these triggers is delayed until the end of the internal git processing. The central class for this is the GitHookEventFacade, where hook events are either - put into a ThreadLocal, so that they can be triggered later, or - triggered in a thread that is joined with an internal JGit thread for internal pushes (eg. for modify command or merge command) --- gradle/changelog/fix_pack_parser_error | 2 + .../jgit/transport/ScmTransportProtocol.java | 10 +- .../git/BaseReceivePackFactory.java | 4 +- .../git/GitCommandProtocol.java | 14 +- .../git/ScmReceivePackFactory.java | 4 +- .../spi/GitHookContextProvider.java | 4 + .../sonia/scm/web/GitHookEventFacade.java | 136 ++++++++++++++++++ .../java/sonia/scm/web/GitReceiveHook.java | 52 +------ .../sonia/scm/web/GitReceivePackFactory.java | 3 +- .../java/sonia/scm/web/ScmGitServlet.java | 35 +++-- .../spi/BindTransportProtocolRule.java | 11 +- .../repository/spi/GitModifyCommandTest.java | 20 +++ .../spi/SimpleGitWorkingCopyFactoryTest.java | 10 +- 13 files changed, 218 insertions(+), 87 deletions(-) create mode 100644 gradle/changelog/fix_pack_parser_error create mode 100644 scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitHookEventFacade.java diff --git a/gradle/changelog/fix_pack_parser_error b/gradle/changelog/fix_pack_parser_error new file mode 100644 index 0000000000..92496aa311 --- /dev/null +++ b/gradle/changelog/fix_pack_parser_error @@ -0,0 +1,2 @@ +- type: bugfix + description: Sporadic error in reading git pack files ([#1518](https://github.com/scm-manager/scm-manager/issues/1518)) diff --git a/scm-plugins/scm-git-plugin/src/main/java/org/eclipse/jgit/transport/ScmTransportProtocol.java b/scm-plugins/scm-git-plugin/src/main/java/org/eclipse/jgit/transport/ScmTransportProtocol.java index 61c3013f36..9fcfa69314 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/org/eclipse/jgit/transport/ScmTransportProtocol.java +++ b/scm-plugins/scm-git-plugin/src/main/java/org/eclipse/jgit/transport/ScmTransportProtocol.java @@ -36,8 +36,8 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.RepositoryCache; import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryHandler; -import sonia.scm.repository.spi.HookEventFacade; import sonia.scm.web.CollectingPackParserListener; +import sonia.scm.web.GitHookEventFacade; import sonia.scm.web.GitReceiveHook; import java.io.File; @@ -54,7 +54,7 @@ public class ScmTransportProtocol extends TransportProtocol { private static final Set SCHEMES = ImmutableSet.of(NAME); private Provider converterFactory; - private Provider hookEventFacadeProvider; + private Provider hookEventFacadeProvider; private Provider repositoryHandlerProvider; public ScmTransportProtocol() { @@ -63,7 +63,7 @@ public class ScmTransportProtocol extends TransportProtocol { @Inject public ScmTransportProtocol( Provider converterFactory, - Provider hookEventFacadeProvider, + Provider hookEventFacadeProvider, Provider repositoryHandlerProvider) { this.converterFactory = converterFactory; this.hookEventFacadeProvider = hookEventFacadeProvider; @@ -110,11 +110,11 @@ public class ScmTransportProtocol extends TransportProtocol { private final GitChangesetConverterFactory converterFactory; private final GitRepositoryHandler handler; - private final HookEventFacade hookEventFacade; + private final GitHookEventFacade hookEventFacade; public TransportLocalWithHooks( GitChangesetConverterFactory converterFactory, - HookEventFacade hookEventFacade, + GitHookEventFacade hookEventFacade, GitRepositoryHandler handler, Repository local, URIish uri, File gitDir) { super(local, uri, gitDir); diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java index 028e91009d..18840670c6 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java @@ -31,8 +31,8 @@ import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException; import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryHandler; -import sonia.scm.repository.spi.HookEventFacade; import sonia.scm.web.CollectingPackParserListener; +import sonia.scm.web.GitHookEventFacade; import sonia.scm.web.GitReceiveHook; public abstract class BaseReceivePackFactory implements ReceivePackFactory { @@ -40,7 +40,7 @@ public abstract class BaseReceivePackFactory implements ReceivePackFactory private final GitRepositoryHandler handler; private final GitReceiveHook hook; - protected BaseReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, HookEventFacade hookEventFacade) { + protected BaseReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, GitHookEventFacade hookEventFacade) { this.handler = handler; this.hook = new GitReceiveHook(converterFactory, hookEventFacade, handler); } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/GitCommandProtocol.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/GitCommandProtocol.java index 7902c1cef1..9dbc7a5a71 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/GitCommandProtocol.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/GitCommandProtocol.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.protocolcommand.git; import org.eclipse.jgit.lib.Repository; @@ -39,6 +39,7 @@ import sonia.scm.protocolcommand.CommandContext; import sonia.scm.protocolcommand.RepositoryContext; import sonia.scm.protocolcommand.ScmCommandProtocol; import sonia.scm.repository.RepositoryPermissions; +import sonia.scm.web.GitHookEventFacade; import javax.inject.Inject; import java.io.IOException; @@ -48,19 +49,20 @@ public class GitCommandProtocol implements ScmCommandProtocol { private static final Logger LOG = LoggerFactory.getLogger(GitCommandProtocol.class); - private ScmUploadPackFactory uploadPackFactory; - private ScmReceivePackFactory receivePackFactory; + private final ScmUploadPackFactory uploadPackFactory; + private final ScmReceivePackFactory receivePackFactory; + private final GitHookEventFacade gitHookEventFacade; @Inject - public GitCommandProtocol(ScmUploadPackFactory uploadPackFactory, ScmReceivePackFactory receivePackFactory) { + public GitCommandProtocol(ScmUploadPackFactory uploadPackFactory, ScmReceivePackFactory receivePackFactory, GitHookEventFacade gitHookEventFacade) { this.uploadPackFactory = uploadPackFactory; this.receivePackFactory = receivePackFactory; + this.gitHookEventFacade = gitHookEventFacade; } @Override public void handle(CommandContext commandContext, RepositoryContext repositoryContext) throws IOException { String subCommand = commandContext.getArgs()[0]; - if (RemoteConfig.DEFAULT_UPLOAD_PACK.equals(subCommand)) { LOG.trace("got upload pack"); upload(commandContext, repositoryContext); @@ -79,6 +81,8 @@ public class GitCommandProtocol implements ScmCommandProtocol { receivePack.receive(commandContext.getInputStream(), commandContext.getOutputStream(), commandContext.getErrorStream()); } catch (ServiceNotEnabledException | ServiceNotAuthorizedException e) { throw new IOException("error creating receive pack for ssh", e); + } finally { + gitHookEventFacade.firePending(); } } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/ScmReceivePackFactory.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/ScmReceivePackFactory.java index a999a57d4d..8fe5e5ce3b 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/ScmReceivePackFactory.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/ScmReceivePackFactory.java @@ -30,12 +30,12 @@ import org.eclipse.jgit.transport.ReceivePack; import sonia.scm.protocolcommand.RepositoryContext; import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryHandler; -import sonia.scm.repository.spi.HookEventFacade; +import sonia.scm.web.GitHookEventFacade; public class ScmReceivePackFactory extends BaseReceivePackFactory { @Inject - public ScmReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, HookEventFacade hookEventFacade) { + public ScmReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, GitHookEventFacade hookEventFacade) { super(converterFactory, handler, hookEventFacade); } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitHookContextProvider.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitHookContextProvider.java index ce61b85a57..1b03a87406 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitHookContextProvider.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitHookContextProvider.java @@ -123,6 +123,10 @@ public class GitHookContextProvider extends HookContextProvider return SUPPORTED_FEATURES; } + public String getRepositoryId() { + return repositoryId; + } + private final GitHookChangesetProvider changesetProvider; private final List receiveCommands; private final ReceivePack receivePack; diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitHookEventFacade.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitHookEventFacade.java new file mode 100644 index 0000000000..1c86ce1072 --- /dev/null +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitHookEventFacade.java @@ -0,0 +1,136 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.web; + +import com.google.common.util.concurrent.ThreadFactoryBuilder; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import sonia.scm.repository.RepositoryHookType; +import sonia.scm.repository.spi.GitHookContextProvider; +import sonia.scm.repository.spi.HookEventFacade; + +import javax.annotation.Nonnull; +import javax.inject.Inject; +import java.io.Closeable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +/** + * This class is used to delay the firing of post commit hooks, so that they are not + * executed while the internal git processing has not finished. Without this, this can + * lead to conflicting access to pack files which results in 'Short compressed stream' + * errors (see https://github.com/scm-manager/scm-manager/pull/1518). + *
+ * The delay is handled either by "caching" the hook data in a thread local, where it + * is fetched from when {@link #firePending()} is called, or in case the trigger is fired + * due to changes made with internal git file push (from a workdir to the central + * repository) by detecting the internal thread used by JGit and joining another thread + * where the pending push is triggered. + */ +public class GitHookEventFacade implements Closeable { + + private static final Logger LOG = LoggerFactory.getLogger(GitHookEventFacade.class); + + private static final ThreadLocal PENDING_POST_HOOK = new ThreadLocal<>(); + + private final HookEventFacade hookEventFacade; + private final ExecutorService internalThreadHookHandler; + + @Inject + public GitHookEventFacade(HookEventFacade hookEventFacade) { + this.hookEventFacade = hookEventFacade; + this.internalThreadHookHandler = createInternalThreadHookHandlerPool(); + } + + public void fire(RepositoryHookType type, GitHookContextProvider context) { + switch (type) { + case PRE_RECEIVE: + doFire(type, context); + break; + case POST_RECEIVE: + Thread thread = Thread.currentThread(); + if ("JGit-Receive-Pack".equals(thread.getName())) { + // this thread name is used in the JGit class org.eclipse.jgit.transport.InternalPushConnection + LOG.debug("handling internal git thread for post receive hook"); + handleGitInternalThread(context, thread); + } else { + LOG.debug("register post receive hook for repository id {} in thread {}", context.getRepositoryId(), thread); + PENDING_POST_HOOK.set(context); + } + break; + default: + throw new IllegalArgumentException("unknown hook type: " + type); + } + } + + public void firePending() { + try { + LOG.debug("fire pending post receive hooks in thread {}", Thread.currentThread()); + doFire(RepositoryHookType.POST_RECEIVE, PENDING_POST_HOOK.get()); + } finally { + clean(); + } + } + + private void handleGitInternalThread(GitHookContextProvider context, Thread internalJGitThread) { + internalThreadHookHandler.submit(() -> { + try { + internalJGitThread.join(); + } catch (InterruptedException e) { + LOG.debug("got interrupted in internal git thread for repository id {}", context.getRepositoryId(), e); + } finally { + LOG.debug("internal git thread ended for repository id {}", context.getRepositoryId()); + doFire(RepositoryHookType.POST_RECEIVE, context); + } + }); + } + + public void clean() { + PENDING_POST_HOOK.remove(); + } + + private void doFire(RepositoryHookType type, GitHookContextProvider context) { + if (context != null) { + LOG.debug("firing {} hook for repository {} in Thread {}", type, context.getRepositoryId(), Thread.currentThread()); + hookEventFacade.handle(context.getRepositoryId()).fireHookEvent(type, context); + } else { + LOG.debug("No context found for event type {} in Thread {}", type, Thread.currentThread()); + } + } + + @Nonnull + private ExecutorService createInternalThreadHookHandlerPool() { + return Executors.newCachedThreadPool( + new ThreadFactoryBuilder() + .setNameFormat("GitInternalThreadHookHandler-%d") + .build() + ); + } + + @Override + public void close() { + internalThreadHookHandler.shutdown(); + } +} diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java index a093b5cda3..9cae0611f8 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java @@ -38,7 +38,6 @@ import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryHandler; import sonia.scm.repository.RepositoryHookType; import sonia.scm.repository.spi.GitHookContextProvider; -import sonia.scm.repository.spi.HookEventFacade; import java.io.IOException; import java.util.Collection; @@ -57,17 +56,11 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook private static final Logger logger = LoggerFactory.getLogger(GitReceiveHook.class); - //~--- constructors --------------------------------------------------------- + private final GitRepositoryHandler handler; + private final GitChangesetConverterFactory converterFactory; + private final GitHookEventFacade hookEventFacade; - /** - * Constructs ... - * - * - * - * @param hookEventFacade - * @param handler - */ - public GitReceiveHook(GitChangesetConverterFactory converterFactory, HookEventFacade hookEventFacade, + public GitReceiveHook(GitChangesetConverterFactory converterFactory, GitHookEventFacade hookEventFacade, GitRepositoryHandler handler) { this.converterFactory = converterFactory; @@ -75,15 +68,6 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook this.handler = handler; } - //~--- methods -------------------------------------------------------------- - - /** - * Method description - * - * - * @param rpack - * @param receiveCommands - */ @Override public void onPostReceive(ReceivePack rpack, Collection receiveCommands) @@ -91,14 +75,6 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook onReceive(rpack, receiveCommands, RepositoryHookType.POST_RECEIVE); } - /** - * Method description - * - * - * - * @param rpack - * @param receiveCommands - */ @Override public void onPreReceive(ReceivePack rpack, Collection receiveCommands) @@ -106,14 +82,6 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook onReceive(rpack, receiveCommands, RepositoryHookType.PRE_RECEIVE); } - /** - * Method description - * - * - * @param rpack - * @param receiveCommands - * @param type - */ private void handleReceiveCommands(ReceivePack rpack, List receiveCommands, RepositoryHookType type) { @@ -126,8 +94,7 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook GitHookContextProvider context = new GitHookContextProvider(converterFactory, rpack, receiveCommands, repository, repositoryId); - hookEventFacade.handle(repositoryId).fireHookEvent(type, context); - + hookEventFacade.fire(type, context); } catch (Exception ex) { @@ -183,13 +150,4 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook StoredConfig gitConfig = repository.getConfig(); return handler.getRepositoryId(gitConfig); } - - //~--- fields --------------------------------------------------------------- - - /** Field description */ - private GitRepositoryHandler handler; - - private final GitChangesetConverterFactory converterFactory; - /** Field description */ - private HookEventFacade hookEventFacade; } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceivePackFactory.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceivePackFactory.java index dc18189da9..49cfed592d 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceivePackFactory.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceivePackFactory.java @@ -36,7 +36,6 @@ import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; import sonia.scm.protocolcommand.git.BaseReceivePackFactory; import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryHandler; -import sonia.scm.repository.spi.HookEventFacade; import javax.servlet.http.HttpServletRequest; @@ -54,7 +53,7 @@ public class GitReceivePackFactory extends BaseReceivePackFactory wrapped; @Inject - public GitReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, HookEventFacade hookEventFacade) { + public GitReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, GitHookEventFacade hookEventFacade) { super(converterFactory, handler, hookEventFacade); this.wrapped = new DefaultReceivePackFactory(); } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/ScmGitServlet.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/ScmGitServlet.java index a063127b77..d4236e45b8 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/ScmGitServlet.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/ScmGitServlet.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.web; import com.google.common.annotations.VisibleForTesting; @@ -68,27 +68,17 @@ public class ScmGitServlet extends GitServlet implements ScmProviderHttpServlet //~--- constructors --------------------------------------------------------- - /** - * Constructs ... - * - * - * - * @param repositoryResolver - * @param receivePackFactory - * @param repositoryViewer - * @param repositoryRequestListenerUtil - * @param lfsServletFactory - */ @Inject public ScmGitServlet(GitRepositoryResolver repositoryResolver, GitReceivePackFactory receivePackFactory, GitRepositoryViewer repositoryViewer, RepositoryRequestListenerUtil repositoryRequestListenerUtil, - LfsServletFactory lfsServletFactory) + LfsServletFactory lfsServletFactory, GitHookEventFacade gitHookEventFacade) { this.repositoryViewer = repositoryViewer; this.repositoryRequestListenerUtil = repositoryRequestListenerUtil; this.lfsServletFactory = lfsServletFactory; + this.gitHookEventFacade = gitHookEventFacade; setRepositoryResolver(repositoryResolver); setReceivePackFactory(receivePackFactory); @@ -109,7 +99,7 @@ public class ScmGitServlet extends GitServlet implements ScmProviderHttpServlet @Override public void service(HttpServletRequest request, HttpServletResponse response, Repository repository) throws ServletException, IOException - { + { String repoPath = repository.getNamespace() + "/" + repository.getName(); logger.trace("handle git repository at {}", repoPath); if (isLfsBatchApiRequest(request, repoPath)) { @@ -123,17 +113,22 @@ public class ScmGitServlet extends GitServlet implements ScmProviderHttpServlet } else if (isRegularGitAPIRequest(request)) { logger.trace("handle regular git request"); // continue with the regular git Backend - handleRegularGitRequest(request, response, repository); + try { + handleRegularGitRequest(request, response, repository); + gitHookEventFacade.firePending(); + } finally { + gitHookEventFacade.clean(); + } } else { logger.trace("handle browser request"); handleBrowserRequest(request, response, repository); } } - + private boolean isRegularGitAPIRequest(HttpServletRequest request) { return REGEX_GITHTTPBACKEND.matcher(HttpUtil.getStrippedURI(request)).matches(); } - + private void handleGitLfsRequest(HttpServlet servlet, HttpServletRequest request, HttpServletResponse response, Repository repository) throws ServletException, IOException { if (repositoryRequestListenerUtil.callListeners(request, response, repository)) { servlet.service(request, response); @@ -141,7 +136,7 @@ public class ScmGitServlet extends GitServlet implements ScmProviderHttpServlet logger.debug("request aborted by repository request listener"); } } - + private void handleRegularGitRequest(HttpServletRequest request, HttpServletResponse response, Repository repository) throws ServletException, IOException { if (repositoryRequestListenerUtil.callListeners(request, response, repository)) { super.service(request, response); @@ -149,7 +144,7 @@ public class ScmGitServlet extends GitServlet implements ScmProviderHttpServlet logger.debug("request aborted by repository request listener"); } } - + /** * This method renders basic information about the repository into the response. The result is meant to be viewed by @@ -245,4 +240,6 @@ public class ScmGitServlet extends GitServlet implements ScmProviderHttpServlet private final GitRepositoryViewer repositoryViewer; private final LfsServletFactory lfsServletFactory; + + private final GitHookEventFacade gitHookEventFacade; } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/BindTransportProtocolRule.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/BindTransportProtocolRule.java index f859efecae..d949267e69 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/BindTransportProtocolRule.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/BindTransportProtocolRule.java @@ -27,27 +27,29 @@ package sonia.scm.repository.spi; import org.eclipse.jgit.transport.ScmTransportProtocol; import org.eclipse.jgit.transport.Transport; import org.junit.rules.ExternalResource; -import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryHandler; import sonia.scm.repository.GitTestHelper; import sonia.scm.repository.PreProcessorUtil; import sonia.scm.repository.RepositoryManager; import sonia.scm.repository.api.HookContextFactory; +import sonia.scm.web.GitHookEventFacade; import static com.google.inject.util.Providers.of; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -public class BindTransportProtocolRule extends ExternalResource { +class BindTransportProtocolRule extends ExternalResource { private ScmTransportProtocol scmTransportProtocol; + RepositoryManager repositoryManager = mock(RepositoryManager.class); + GitHookEventFacade hookEventFacade; + @Override protected void before() { HookContextFactory hookContextFactory = new HookContextFactory(mock(PreProcessorUtil.class)); - RepositoryManager repositoryManager = mock(RepositoryManager.class); - HookEventFacade hookEventFacade = new HookEventFacade(of(repositoryManager), hookContextFactory); + hookEventFacade = new GitHookEventFacade(new HookEventFacade(of(repositoryManager), hookContextFactory)); GitRepositoryHandler gitRepositoryHandler = mock(GitRepositoryHandler.class); scmTransportProtocol = new ScmTransportProtocol(of(GitTestHelper.createConverterFactory()), of(hookEventFacade), of(gitRepositoryHandler)); @@ -60,5 +62,6 @@ public class BindTransportProtocolRule extends ExternalResource { @Override protected void after() { Transport.unregister(scmTransportProtocol); + hookEventFacade.close(); } } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java index e4e09c5bd5..8048308783 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitModifyCommandTest.java @@ -36,12 +36,15 @@ import sonia.scm.ConcurrentModificationException; import sonia.scm.NotFoundException; import sonia.scm.repository.GitTestHelper; import sonia.scm.repository.Person; +import sonia.scm.repository.RepositoryHookType; import java.io.File; import java.io.IOException; import java.nio.file.Files; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.Mockito.verify; public class GitModifyCommandTest extends GitModifyCommandTestBase { @@ -327,4 +330,21 @@ public class GitModifyCommandTest extends GitModifyCommandTestBase { assertThat(lastCommit.getRawGpgSignature()).isNullOrEmpty(); } } + + @Test + public void shouldTriggerPostCommitHook() throws IOException { + File newFile = Files.write(temporaryFolder.newFile().toPath(), "new content".getBytes()).toFile(); + + GitModifyCommand command = createCommand(); + + ModifyCommandRequest request = new ModifyCommandRequest(); + request.setCommitMessage("test commit"); + request.addRequest(new ModifyCommandRequest.CreateFileRequest("new_file", newFile, false)); + request.setAuthor(new Person("Dirk Gently", "dirk@holistic.det")); + + command.execute(request); + + verify(transportProtocolRule.repositoryManager).fireHookEvent(argThat(argument -> argument.getType() == RepositoryHookType.PRE_RECEIVE)); + verify(transportProtocolRule.repositoryManager).fireHookEvent(argThat(argument -> argument.getType() == RepositoryHookType.POST_RECEIVE)); + } } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/SimpleGitWorkingCopyFactoryTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/SimpleGitWorkingCopyFactoryTest.java index 93c47a7474..8f2a47796f 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/SimpleGitWorkingCopyFactoryTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/SimpleGitWorkingCopyFactoryTest.java @@ -30,6 +30,7 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.ScmTransportProtocol; import org.eclipse.jgit.transport.Transport; import org.eclipse.jgit.transport.URIish; +import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -42,6 +43,7 @@ import sonia.scm.repository.api.HookContextFactory; import sonia.scm.repository.work.NoneCachingWorkingCopyPool; import sonia.scm.repository.work.WorkdirProvider; import sonia.scm.repository.work.WorkingCopy; +import sonia.scm.web.GitHookEventFacade; import java.io.File; import java.io.IOException; @@ -60,17 +62,23 @@ public class SimpleGitWorkingCopyFactoryTest extends AbstractGitCommandTestBase // keep this so that it will not be garbage collected (Transport keeps this in a week reference) private ScmTransportProtocol proto; private WorkdirProvider workdirProvider; + GitHookEventFacade hookEventFacade; @Before public void bindScmProtocol() throws IOException { HookContextFactory hookContextFactory = new HookContextFactory(mock(PreProcessorUtil.class)); - HookEventFacade hookEventFacade = new HookEventFacade(of(mock(RepositoryManager.class)), hookContextFactory); + hookEventFacade = new GitHookEventFacade(new HookEventFacade(of(mock(RepositoryManager.class)), hookContextFactory)); GitRepositoryHandler gitRepositoryHandler = mock(GitRepositoryHandler.class); proto = new ScmTransportProtocol(of(GitTestHelper.createConverterFactory()), of(hookEventFacade), of(gitRepositoryHandler)); Transport.register(proto); workdirProvider = new WorkdirProvider(temporaryFolder.newFolder(), repositoryLocationResolver, false); } + @After + public void close() { + hookEventFacade.close(); + } + @Test public void emptyPoolShouldCreateNewWorkdir() { SimpleGitWorkingCopyFactory factory = new SimpleGitWorkingCopyFactory(new NoneCachingWorkingCopyPool(workdirProvider));