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)

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();
   }
 }

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));
+  }
 }

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));