From 77b62be68a75c0620adcabe5f3f7e6ca253eab30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 4 May 2021 16:53:43 +0200 Subject: [PATCH] Fix messages for post commit hooks in git (#1647) Fixes the transmission of messages from post commit hooks in Git repositories. We therefore use a new method patched in jGit for SCM-Manager. This simplifies the trigger logic a lot. --- .../post_commit_hook_messages_git.yaml | 2 + scm-plugins/scm-git-plugin/build.gradle | 2 +- .../jgit/transport/ScmTransportProtocol.java | 12 +- .../git/BaseReceivePackFactory.java | 6 +- .../git/GitCommandProtocol.java | 7 +- .../git/ScmReceivePackFactory.java | 4 +- .../scm/web/CollectingPackParserListener.java | 16 +- .../sonia/scm/web/GitHookEventFacade.java | 141 ------------------ .../java/sonia/scm/web/GitReceiveHook.java | 89 +++++------ .../sonia/scm/web/GitReceivePackFactory.java | 3 +- .../java/sonia/scm/web/ScmGitServlet.java | 12 +- .../spi/BindTransportProtocolRule.java | 7 +- .../spi/SimpleGitWorkingCopyFactoryTest.java | 10 +- 13 files changed, 75 insertions(+), 236 deletions(-) create mode 100644 gradle/changelog/post_commit_hook_messages_git.yaml delete mode 100644 scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitHookEventFacade.java diff --git a/gradle/changelog/post_commit_hook_messages_git.yaml b/gradle/changelog/post_commit_hook_messages_git.yaml new file mode 100644 index 0000000000..356760d4f3 --- /dev/null +++ b/gradle/changelog/post_commit_hook_messages_git.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Messages from post commit hooks for git ([#1647](https://github.com/scm-manager/scm-manager/pull/1647)) diff --git a/scm-plugins/scm-git-plugin/build.gradle b/scm-plugins/scm-git-plugin/build.gradle index 262356b7d8..cf9c37a989 100644 --- a/scm-plugins/scm-git-plugin/build.gradle +++ b/scm-plugins/scm-git-plugin/build.gradle @@ -27,7 +27,7 @@ plugins { id 'org.scm-manager.smp' version '0.7.5' } -def jgitVersion = '5.10.0.202012080955-r-scm1' +def jgitVersion = '5.10.0.202012080955-r-scm2' dependencies { // required by scm-it 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 9fcfa69314..049071c538 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 GitHookEventFacade hookEventFacade; + private final HookEventFacade hookEventFacade; public TransportLocalWithHooks( GitChangesetConverterFactory converterFactory, - GitHookEventFacade hookEventFacade, + HookEventFacade hookEventFacade, GitRepositoryHandler handler, Repository local, URIish uri, File gitDir) { super(local, uri, gitDir); @@ -133,7 +133,7 @@ public class ScmTransportProtocol extends TransportProtocol { pack.setPreReceiveHook(hook); pack.setPostReceiveHook(hook); - CollectingPackParserListener.set(pack); + CollectingPackParserListener.set(pack, hook); } return pack; 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 fd6ac85464..9f32812817 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 @@ -33,8 +33,8 @@ import sonia.scm.api.v2.resources.GitRepositoryConfigStoreProvider; import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryConfig; 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 { @@ -45,7 +45,7 @@ public abstract class BaseReceivePackFactory implements ReceivePackFactory protected BaseReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, - GitHookEventFacade hookEventFacade, + HookEventFacade hookEventFacade, GitRepositoryConfigStoreProvider storeProvider) { this.handler = handler; this.storeProvider = storeProvider; @@ -60,7 +60,7 @@ public abstract class BaseReceivePackFactory implements ReceivePackFactory receivePack.setPreReceiveHook(hook); receivePack.setPostReceiveHook(hook); // apply collecting listener, to be able to check which commits are new - CollectingPackParserListener.set(receivePack); + CollectingPackParserListener.set(receivePack, hook); return receivePack; } 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 9dbc7a5a71..900340566f 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 @@ -39,7 +39,6 @@ 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; @@ -51,13 +50,11 @@ public class GitCommandProtocol implements ScmCommandProtocol { private final ScmUploadPackFactory uploadPackFactory; private final ScmReceivePackFactory receivePackFactory; - private final GitHookEventFacade gitHookEventFacade; @Inject - public GitCommandProtocol(ScmUploadPackFactory uploadPackFactory, ScmReceivePackFactory receivePackFactory, GitHookEventFacade gitHookEventFacade) { + public GitCommandProtocol(ScmUploadPackFactory uploadPackFactory, ScmReceivePackFactory receivePackFactory) { this.uploadPackFactory = uploadPackFactory; this.receivePackFactory = receivePackFactory; - this.gitHookEventFacade = gitHookEventFacade; } @Override @@ -81,8 +78,6 @@ 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 357affcc97..4741938b4e 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 @@ -31,14 +31,14 @@ import sonia.scm.api.v2.resources.GitRepositoryConfigStoreProvider; import sonia.scm.protocolcommand.RepositoryContext; import sonia.scm.repository.GitChangesetConverterFactory; import sonia.scm.repository.GitRepositoryHandler; -import sonia.scm.web.GitHookEventFacade; +import sonia.scm.repository.spi.HookEventFacade; public class ScmReceivePackFactory extends BaseReceivePackFactory { @Inject public ScmReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, - GitHookEventFacade hookEventFacade, + HookEventFacade hookEventFacade, GitRepositoryConfigStoreProvider gitRepositoryConfigStoreProvider) { super(converterFactory, handler, hookEventFacade, gitRepositoryConfigStoreProvider); } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java index 0103667759..11dba9ce83 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.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; //~--- non-JDK imports -------------------------------------------------------- @@ -58,6 +58,11 @@ public class CollectingPackParserListener implements PackParserListener */ private static final Logger logger = LoggerFactory.getLogger(CollectingPackParserListener.class); + private final GitReceiveHook hook; + + public CollectingPackParserListener(GitReceiveHook hook) { + this.hook = hook; + } //~--- get methods ---------------------------------------------------------- @@ -94,10 +99,10 @@ public class CollectingPackParserListener implements PackParserListener * * @param pack receive pack */ - public static void set(ReceivePack pack) + public static void set(ReceivePack pack, GitReceiveHook hook) { logger.trace("apply collecting listener to receive pack"); - pack.setPackParserListener(new CollectingPackParserListener()); + pack.setPackParserListener(new CollectingPackParserListener(hook)); } //~--- methods -------------------------------------------------------------- @@ -148,6 +153,11 @@ public class CollectingPackParserListener implements PackParserListener parser.setNeedNewObjectIds(true); } + @Override + public void release() { + hook.afterReceive(); + } + //~--- get methods ---------------------------------------------------------- /** 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 deleted file mode 100644 index 664372c08b..0000000000 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitHookEventFacade.java +++ /dev/null @@ -1,141 +0,0 @@ -/* - * 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 io.micrometer.core.instrument.MeterRegistry; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import sonia.scm.metrics.Metrics; -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, MeterRegistry registry) { - this.hookEventFacade = hookEventFacade; - this.internalThreadHookHandler = createInternalThreadHookHandlerPool(registry); - } - - 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); - Thread.currentThread().interrupt(); - } 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(MeterRegistry registry) { - ExecutorService executorService = Executors.newCachedThreadPool( - new ThreadFactoryBuilder() - .setNameFormat("GitInternalThreadHookHandler-%d") - .build() - ); - Metrics.executor(registry, executorService, "GitInternalHookHandler", "cached"); - return executorService; - } - - @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 9cae0611f8..aa89f1cd87 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,6 +38,7 @@ 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; @@ -49,18 +50,18 @@ import java.util.List; * * @author Sebastian Sdorra */ -public class GitReceiveHook implements PreReceiveHook, PostReceiveHook -{ +public class GitReceiveHook implements PreReceiveHook, PostReceiveHook { - /** the logger for GitReceiveHook */ - private static final Logger logger = - LoggerFactory.getLogger(GitReceiveHook.class); + private static final Logger LOG = LoggerFactory.getLogger(GitReceiveHook.class); private final GitRepositoryHandler handler; private final GitChangesetConverterFactory converterFactory; - private final GitHookEventFacade hookEventFacade; + private final HookEventFacade hookEventFacade; - public GitReceiveHook(GitChangesetConverterFactory converterFactory, GitHookEventFacade hookEventFacade, + private GitHookContextProvider postReceiveContext; + + public GitReceiveHook(GitChangesetConverterFactory converterFactory, + HookEventFacade hookEventFacade, GitRepositoryHandler handler) { this.converterFactory = converterFactory; @@ -69,69 +70,60 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook } @Override - public void onPostReceive(ReceivePack rpack, - Collection receiveCommands) - { + public void onPostReceive(ReceivePack rpack, Collection receiveCommands) { onReceive(rpack, receiveCommands, RepositoryHookType.POST_RECEIVE); } @Override - public void onPreReceive(ReceivePack rpack, - Collection receiveCommands) - { + public void onPreReceive(ReceivePack rpack, Collection receiveCommands) { onReceive(rpack, receiveCommands, RepositoryHookType.PRE_RECEIVE); } - private void handleReceiveCommands(ReceivePack rpack, - List receiveCommands, RepositoryHookType type) - { - try - { + public void afterReceive() { + if (postReceiveContext != null) { + LOG.debug("firing {} hook for repository {}", RepositoryHookType.POST_RECEIVE, postReceiveContext.getRepositoryId()); + try { + hookEventFacade.handle(postReceiveContext.getRepositoryId()).fireHookEvent(RepositoryHookType.POST_RECEIVE, postReceiveContext); + } finally { + postReceiveContext = null; + } + } else { + LOG.debug("No context found for event type {}", RepositoryHookType.POST_RECEIVE); + } + } + + private void handleReceiveCommands(ReceivePack rpack, List receiveCommands, RepositoryHookType type) { + try { Repository repository = rpack.getRepository(); String repositoryId = resolveRepositoryId(repository); - logger.trace("resolved repository to {}", repositoryId); + LOG.trace("resolved repository to {}", repositoryId); GitHookContextProvider context = new GitHookContextProvider(converterFactory, rpack, receiveCommands, repository, repositoryId); - hookEventFacade.fire(type, context); - } - catch (Exception ex) - { - logger.error("could not handle receive commands", ex); + if (type == RepositoryHookType.POST_RECEIVE) { + postReceiveContext = context; + } else { + hookEventFacade.handle(repositoryId).fireHookEvent(type, context); + } + } catch (Exception ex) { + LOG.error("could not handle receive commands", ex); GitHooks.abortIfPossible(type, rpack, receiveCommands, ex.getMessage()); } } - /** - * Method description - * - * - * @param rpack - * @param commands - * @param type - */ - private void onReceive(ReceivePack rpack, - Collection commands, RepositoryHookType type) - { - if (logger.isTraceEnabled()) - { - logger.trace("received git hook, type={}", type); - } + private void onReceive(ReceivePack rpack, Collection commands, RepositoryHookType type) { + LOG.trace("received git hook, type={}", type); - List receiveCommands = GitHooks.filterReceiveable(type, - commands); + List receiveCommands = GitHooks.filterReceiveable(type, commands); GitFileHook.execute(type, rpack, commands); - if (!receiveCommands.isEmpty()) - { + if (!receiveCommands.isEmpty()) { handleReceiveCommands(rpack, receiveCommands, type); - } - else if (logger.isDebugEnabled()) - { - logger.debug("no receive commands found to process"); + } else { + LOG.debug("no receive commands found to process"); } } @@ -145,8 +137,7 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook * * @throws IOException */ - private String resolveRepositoryId(Repository repository) - { + private String resolveRepositoryId(Repository repository) { StoredConfig gitConfig = repository.getConfig(); return handler.getRepositoryId(gitConfig); } 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 9db075bfd5..298df8482f 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 @@ -37,6 +37,7 @@ import sonia.scm.api.v2.resources.GitRepositoryConfigStoreProvider; 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; @@ -56,7 +57,7 @@ public class GitReceivePackFactory extends BaseReceivePackFactory