From 790b2870508e52fe5052b230c9ffc3c59ef03629 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 13 Mar 2019 16:00:07 +0100
Subject: [PATCH 1/5] Revert "Revert "Merge branch 'switch-to-embedded-hooks'
 into 'master'""

This reverts commit 4f376cf16e64a0fa673821c0eb4a56eca9ca9419.
---
 internal/config/config_test.go                | 11 +++++++---
 internal/config/ruby.go                       |  7 +++++++
 internal/git/hooks/hooks.go                   | 12 ++---------
 internal/git/hooks/hooks_test.go              | 20 +++----------------
 internal/service/conflicts/testhelper_test.go |  2 ++
 .../service/operations/testhelper_test.go     | 12 ++---------
 .../service/smarthttp/receive_pack_test.go    |  6 ++----
 internal/service/ssh/receive_pack_test.go     |  6 ++----
 internal/service/wiki/testhelper_test.go      |  3 +++
 ruby/lib/gitlab/git/hook.rb                   |  4 ++++
 ruby/lib/gitlab/git/repository.rb             |  7 +++----
 ruby/spec/lib/gitlab/git/repository_spec.rb   |  2 +-
 12 files changed, 39 insertions(+), 53 deletions(-)

diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index f02221f4e90..6a8bcb5d05f 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 7e73ec0b259..c10cb4e8428 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 9357f0fe53b..d7df23a8acc 100644
--- a/internal/git/hooks/hooks.go
+++ b/internal/git/hooks/hooks.go
@@ -1,7 +1,6 @@
 package hooks
 
 import (
-	"os"
 	"path"
 
 	"gitlab.com/gitlab-org/gitaly/internal/config"
@@ -10,18 +9,11 @@ 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.
 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")
-	}
-
-	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 655366b7d2f..ce69896360f 100644
--- a/internal/git/hooks/hooks_test.go
+++ b/internal/git/hooks/hooks_test.go
@@ -1,8 +1,6 @@
 package hooks
 
 import (
-	"fmt"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -10,27 +8,15 @@ 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 = "" }()
diff --git a/internal/service/conflicts/testhelper_test.go b/internal/service/conflicts/testhelper_test.go
index 713a971b889..482fc0631e3 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 3a5408f6b76..5649ed514d5 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 ef083b67de6..6525aa72e3e 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 d69bdb002b0..c42c54c8365 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 1637d1ffc96..e909db22400 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 1b2979cc276..222c93d3bb3 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 6f4bd99106a..1a0b7d04410 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 677be7aa960..4364b8818aa 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
-- 
GitLab


From 8f3f00a1c61cb60e47af2feff66fecb1f50f1c8f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Wed, 13 Mar 2019 16:04:46 +0100
Subject: [PATCH 2/5] Add hook override via env for tests

---
 internal/git/hooks/hooks.go      |  9 ++++++++-
 internal/git/hooks/hooks_test.go | 12 ++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go
index d7df23a8acc..f110af9519e 100644
--- a/internal/git/hooks/hooks.go
+++ b/internal/git/hooks/hooks.go
@@ -1,12 +1,15 @@
 package hooks
 
 import (
+	"os"
 	"path"
 
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
-// Override allows tests to control where the hooks directory is.
+// Override allows tests to control where the hooks directory is. Outside
+// consumers can also use the GITALY_TESTING_HOOKS_OVERRIDE environment
+// variable.
 var Override string
 
 // Path returns the path where the global git hooks are located.
@@ -15,5 +18,9 @@ func Path() string {
 		return Override
 	}
 
+	if dir := os.Getenv("GITALY_TESTING_HOOKS_DIRECTORY"); len(dir) > 0 {
+		return dir
+	}
+
 	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 ce69896360f..4424c921c08 100644
--- a/internal/git/hooks/hooks_test.go
+++ b/internal/git/hooks/hooks_test.go
@@ -1,6 +1,7 @@
 package hooks
 
 import (
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -23,4 +24,15 @@ func TestPath(t *testing.T) {
 
 		require.Equal(t, "/override/hooks", Path())
 	})
+
+	t.Run("when an env override", func(t *testing.T) {
+		key := "GITALY_TESTING_HOOKS_DIRECTORY"
+		dir := "/override/hooks-from-env"
+
+		os.Setenv(key, dir)
+		defer os.Unsetenv(key)
+
+		require.Equal(t, dir, Path())
+	})
+
 }
-- 
GitLab


From 854287d853d7d8805492e2a0ecfc3d604b8a5af0 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Thu, 14 Mar 2019 11:30:38 +0100
Subject: [PATCH 3/5] Make it an on/off switch

---
 internal/git/hooks/hooks.go      | 6 +++---
 internal/git/hooks/hooks_test.go | 7 +++----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go
index f110af9519e..43c549fef96 100644
--- a/internal/git/hooks/hooks.go
+++ b/internal/git/hooks/hooks.go
@@ -8,7 +8,7 @@ import (
 )
 
 // Override allows tests to control where the hooks directory is. Outside
-// consumers can also use the GITALY_TESTING_HOOKS_OVERRIDE environment
+// consumers can also use the GITALY_TESTING_NO_GIT_HOOKS environment
 // variable.
 var Override string
 
@@ -18,8 +18,8 @@ func Path() string {
 		return Override
 	}
 
-	if dir := os.Getenv("GITALY_TESTING_HOOKS_DIRECTORY"); len(dir) > 0 {
-		return dir
+	if os.Getenv("GITALY_TESTING_NO_GIT_HOOKS") == "1" {
+		return "/var/empty"
 	}
 
 	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 4424c921c08..6c26574616b 100644
--- a/internal/git/hooks/hooks_test.go
+++ b/internal/git/hooks/hooks_test.go
@@ -26,13 +26,12 @@ func TestPath(t *testing.T) {
 	})
 
 	t.Run("when an env override", func(t *testing.T) {
-		key := "GITALY_TESTING_HOOKS_DIRECTORY"
-		dir := "/override/hooks-from-env"
+		key := "GITALY_TESTING_NO_GIT_HOOKS"
 
-		os.Setenv(key, dir)
+		os.Setenv(key, "1")
 		defer os.Unsetenv(key)
 
-		require.Equal(t, dir, Path())
+		require.Equal(t, "/var/empty", Path())
 	})
 
 }
-- 
GitLab


From 9ebd04f63ab3a49ecbf8ef2ab6e6babb65561f5b Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Thu, 14 Mar 2019 11:31:52 +0100
Subject: [PATCH 4/5] changelog

---
 changelogs/unreleased/re-apply-hooks-change.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/re-apply-hooks-change.yml

diff --git a/changelogs/unreleased/re-apply-hooks-change.yml b/changelogs/unreleased/re-apply-hooks-change.yml
new file mode 100644
index 00000000000..99ce156e750
--- /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
-- 
GitLab


From 035e4c2ee2556b0865f93eb9d6e98755f4e3aa70 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <jacob@gitlab.com>
Date: Thu, 14 Mar 2019 11:46:43 +0100
Subject: [PATCH 5/5] update comment

---
 internal/git/hooks/hooks.go | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/internal/git/hooks/hooks.go b/internal/git/hooks/hooks.go
index 43c549fef96..c77c3ed1e37 100644
--- a/internal/git/hooks/hooks.go
+++ b/internal/git/hooks/hooks.go
@@ -7,12 +7,13 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
-// Override allows tests to control where the hooks directory is. Outside
-// consumers can also use the GITALY_TESTING_NO_GIT_HOOKS environment
-// variable.
+// Override allows tests to control where the hooks directory is.
 var Override string
 
-// Path returns the path where the global git hooks are located.
+// 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
-- 
GitLab