Revert "Revert "Merge branch 'switch-to-embedded-hooks' into 'master'""

This reverts commit 4f376cf16e64a0fa673821c0eb4a56eca9ca9419.

13 files changed, 59 insertions, 49 deletions

diff --git a/changelogs/unreleased/re-apply-hooks-change.yml b/changelogs/unreleased/re-apply-hooks-change.yml
new file mode 100644
index 000000000..99ce156e7
--- /dev/null
+++ b/changelogs/unreleased/re-apply-hooks-change.yml
@@ -0,0 +1,5 @@
+---
+title: Re-apply MR 1088 (Git hooks change)
+merge_request: 1130
+author:
+type: other
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index f02221f4e..6a8bcb5d0 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -416,6 +416,7 @@ func TestConfigureRuby(t *testing.T) {
 		{dir: "", desc: "empty"},
 		{dir: "/does/not/exist", desc: "does not exist"},
 		{dir: tmpFile, desc: "exists but is not a directory"},
+		{dir: ".", ok: true, desc: "relative path"},
 		{dir: tmpDir, ok: true, desc: "ok"},
 	}
 
@@ -424,11 +425,15 @@ func TestConfigureRuby(t *testing.T) {
 			Config.Ruby = Ruby{Dir: tc.dir}
 
 			err := ConfigureRuby()
-			if tc.ok {
-				require.NoError(t, err)
-			} else {
+			if !tc.ok {
 				require.Error(t, err)
+				return
 			}
+
+			require.NoError(t, err)
+
+			dir := Config.Ruby.Dir
+			require.True(t, filepath.IsAbs(dir), "expected %q to be absolute path", dir)
 		})
 	}
 }
diff --git a/internal/config/ruby.go b/internal/config/ruby.go
index 7e73ec0b2..c10cb4e84 100644
--- a/internal/config/ruby.go
+++ b/internal/config/ruby.go
@@ -2,6 +2,7 @@ package config
 
 import (
 	"fmt"
+	"path/filepath"
 	"time"
 )
 
@@ -53,5 +54,11 @@ func ConfigureRuby() error {
 		Config.Ruby.NumWorkers = minWorkers
 	}
 
+	var err error
+	Config.Ruby.Dir, err = filepath.Abs(Config.Ruby.Dir)
+	if err != nil {
+		return err
+	}
+
 	return validateIsDirectory(Config.Ruby.Dir, "gitaly-ruby.dir")
 }
diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go
index 9357f0fe5..c77c3ed1e 100644
--- a/internal/git/hooks/hooks.go
+++ b/internal/git/hooks/hooks.go
@@ -10,18 +10,18 @@ import (
 // Override allows tests to control where the hooks directory is.
 var Override string
 
-// Path returns the path where the global git hooks are located. As a
-// transitional mechanism, GITALY_USE_EMBEDDED_HOOKS=1 will cause
-// Gitaly's embedded hooks to be used instead of the legacy gitlab-shell
-// hooks.
+// Path returns the path where the global git hooks are located. If the
+// environment variable GITALY_TESTING_NO_GIT_HOOKS is set to "1", Path
+// will return an empty directory, which has the effect that no Git hooks
+// will run at all.
 func Path() string {
 	if len(Override) > 0 {
 		return Override
 	}
 
-	if os.Getenv("GITALY_USE_EMBEDDED_HOOKS") == "1" {
-		return path.Join(config.Config.Ruby.Dir, "git-hooks")
+	if os.Getenv("GITALY_TESTING_NO_GIT_HOOKS") == "1" {
+		return "/var/empty"
 	}
 
-	return path.Join(config.Config.GitlabShell.Dir, "hooks")
+	return path.Join(config.Config.Ruby.Dir, "git-hooks")
 }
diff --git a/internal/git/hooks/hooks_test.go b/internal/git/hooks/hooks_test.go
index 655366b7d..6c2657461 100644
--- a/internal/git/hooks/hooks_test.go
+++ b/internal/git/hooks/hooks_test.go
@@ -1,7 +1,6 @@
 package hooks
 
 import (
-	"fmt"
 	"os"
 	"testing"
 
@@ -10,31 +9,29 @@ import (
 )
 
 func TestPath(t *testing.T) {
-	defer func(rubyDir, shellDir string) {
+	defer func(rubyDir string) {
 		config.Config.Ruby.Dir = rubyDir
-		config.Config.GitlabShell.Dir = shellDir
-	}(config.Config.Ruby.Dir, config.Config.GitlabShell.Dir)
+	}(config.Config.Ruby.Dir)
 	config.Config.Ruby.Dir = "/bazqux/gitaly-ruby"
-	config.Config.GitlabShell.Dir = "/foobar/gitlab-shell"
-
-	hooksVar := "GITALY_USE_EMBEDDED_HOOKS"
-	t.Run(fmt.Sprintf("with %s=1", hooksVar), func(t *testing.T) {
-		os.Setenv(hooksVar, "1")
-		defer os.Unsetenv(hooksVar)
 
+	t.Run("default", func(t *testing.T) {
 		require.Equal(t, "/bazqux/gitaly-ruby/git-hooks", Path())
 	})
 
-	t.Run(fmt.Sprintf("without %s=1", hooksVar), func(t *testing.T) {
-		os.Unsetenv(hooksVar)
-
-		require.Equal(t, "/foobar/gitlab-shell/hooks", Path())
-	})
-
 	t.Run("with an override", func(t *testing.T) {
 		Override = "/override/hooks"
 		defer func() { Override = "" }()
 
 		require.Equal(t, "/override/hooks", Path())
 	})
+
+	t.Run("when an env override", func(t *testing.T) {
+		key := "GITALY_TESTING_NO_GIT_HOOKS"
+
+		os.Setenv(key, "1")
+		defer os.Unsetenv(key)
+
+		require.Equal(t, "/var/empty", Path())
+	})
+
 }
diff --git a/internal/service/conflicts/testhelper_test.go b/internal/service/conflicts/testhelper_test.go
index 713a971b8..482fc0631 100644
--- a/internal/service/conflicts/testhelper_test.go
+++ b/internal/service/conflicts/testhelper_test.go
@@ -7,6 +7,7 @@ import (
 
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc"
@@ -24,6 +25,7 @@ func testMain(m *testing.M) int {
 
 	var err error
 
+	hooks.Override = "/does/not/exist"
 	testhelper.ConfigureRuby()
 	RubyServer, err = rubyserver.Start()
 	if err != nil {
diff --git a/internal/service/operations/testhelper_test.go b/internal/service/operations/testhelper_test.go
index 3a5408f6b..5649ed514 100644
--- a/internal/service/operations/testhelper_test.go
+++ b/internal/service/operations/testhelper_test.go
@@ -11,7 +11,6 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -49,17 +48,10 @@ func testMain(m *testing.M) int {
 	if err != nil {
 		log.Fatal(err)
 	}
-
-	defer func(oldDir string) {
-		config.Config.GitlabShell.Dir = oldDir
-	}(config.Config.GitlabShell.Dir)
-	config.Config.GitlabShell.Dir = hookDir
-
-	if err := os.MkdirAll(hooks.Path(), 0755); err != nil {
-		log.Fatal(err)
-	}
 	defer os.RemoveAll(hookDir)
 
+	hooks.Override = hookDir
+
 	testhelper.ConfigureGitalySSH()
 
 	testhelper.ConfigureRuby()
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index ef083b67d..6525aa72e 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -128,10 +128,8 @@ func TestFailedReceivePackRequestDueToHooksFailure(t *testing.T) {
 	require.NoError(t, err)
 	defer os.RemoveAll(hookDir)
 
-	defer func(oldDir string) {
-		config.Config.GitlabShell.Dir = hookDir
-	}(config.Config.GitlabShell.Dir)
-	config.Config.GitlabShell.Dir = hookDir
+	defer func() { hooks.Override = "" }()
+	hooks.Override = hookDir
 
 	require.NoError(t, os.MkdirAll(hooks.Path(), 0755))
 
diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go
index d69bdb002..c42c54c83 100644
--- a/internal/service/ssh/receive_pack_test.go
+++ b/internal/service/ssh/receive_pack_test.go
@@ -148,10 +148,8 @@ func TestReceivePackPushHookFailure(t *testing.T) {
 	require.NoError(t, err)
 	defer os.RemoveAll(hookDir)
 
-	defer func(oldDir string) {
-		config.Config.GitlabShell.Dir = oldDir
-	}(config.Config.GitlabShell.Dir)
-	config.Config.GitlabShell.Dir = hookDir
+	defer func() { hooks.Override = "" }()
+	hooks.Override = hookDir
 
 	require.NoError(t, os.MkdirAll(hooks.Path(), 0755))
 
diff --git a/internal/service/wiki/testhelper_test.go b/internal/service/wiki/testhelper_test.go
index 1637d1ffc..e909db224 100644
--- a/internal/service/wiki/testhelper_test.go
+++ b/internal/service/wiki/testhelper_test.go
@@ -11,6 +11,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
@@ -39,6 +40,8 @@ var rubyServer *rubyserver.Server
 func testMain(m *testing.M) int {
 	defer testhelper.MustHaveNoChildProcess()
 
+	hooks.Override = "/does/not/exist"
+
 	var err error
 	testhelper.ConfigureRuby()
 	rubyServer, err = rubyserver.Start()
diff --git a/ruby/lib/gitlab/git/hook.rb b/ruby/lib/gitlab/git/hook.rb
index 1b2979cc2..222c93d3b 100644
--- a/ruby/lib/gitlab/git/hook.rb
+++ b/ruby/lib/gitlab/git/hook.rb
@@ -7,6 +7,10 @@ module Gitlab
         Gitlab.config.git.hooks_directory
       end
 
+      def self.legacy_hooks_directory
+        File.join(Gitlab.config.gitlab_shell.path, 'hooks')
+      end
+
       GL_PROTOCOL = 'web'
       attr_reader :name, :path, :repository
 
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index 6f4bd9910..1a0b7d044 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -62,10 +62,9 @@ module Gitlab
           repo = Rugged::Repository.init_at(repo_path, true)
           repo.close
 
-          # TODO: remove this when self healing hooks has been merged at least
-          # one release ago: https://gitlab.com/gitlab-org/gitaly/merge_requests/886
-          symlink_hooks_to = Gitlab::Git::Hook.directory
-          create_hooks(repo_path, symlink_hooks_to) if symlink_hooks_to.present?
+          # TODO: stop symlinking to the old hooks location in or after GitLab 12.0.
+          # https://gitlab.com/gitlab-org/gitaly/issues/1392
+          create_hooks(repo_path, Gitlab::Git::Hook.legacy_hooks_directory)
         end
 
         def create_hooks(repo_path, global_hooks_path)
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index 677be7aa9..4364b8818 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -40,7 +40,7 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
   describe '.create_hooks' do
     let(:repo_path) { File.join(storage_path, 'hook-test.git') }
     let(:hooks_dir) { File.join(repo_path, 'hooks') }
-    let(:target_hooks_dir) { Gitlab::Git::Hook.directory }
+    let(:target_hooks_dir) { Gitlab::Git::Hook.legacy_hooks_directory }
     let(:existing_target) { File.join(repo_path, 'foobar') }
 
     before do