From 02517d1a7b527efe6b5a418971f3e0e1a1fde077 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 5 Feb 2019 10:16:06 +0100 Subject: [PATCH] Stop symlinking hooks on repository creation In an earlier MR[1] hooks are executed not through the symlink, but leveraging the `-c` flag on the `git` binary. This works well, but when `#run_git` was executed in Gitaly-Ruby the hooks weren't executed the new way. Luckily this was covered by tests, and the symlinking strategy remained employed. This commit fixes the bug where the hooks weren't executed and now allows the removal of the code that set the hooks. Part of: #1226 [1]: !886 --- .../unreleased/zj-stop-symlinking-hooks.yml | 6 +++ .../repository/create_from_snapshot_test.go | 4 -- internal/service/repository/create_test.go | 11 ----- ruby/lib/gitlab/git/repository.rb | 31 ------------ ruby/spec/lib/gitlab/git/repository_spec.rb | 47 ------------------- 5 files changed, 6 insertions(+), 93 deletions(-) create mode 100644 changelogs/unreleased/zj-stop-symlinking-hooks.yml diff --git a/changelogs/unreleased/zj-stop-symlinking-hooks.yml b/changelogs/unreleased/zj-stop-symlinking-hooks.yml new file mode 100644 index 0000000000..b366b813e1 --- /dev/null +++ b/changelogs/unreleased/zj-stop-symlinking-hooks.yml @@ -0,0 +1,6 @@ +--- +title: Stop symlinking hooks on repository creation +merge_request: 1052 +author: +type: added + diff --git a/internal/service/repository/create_from_snapshot_test.go b/internal/service/repository/create_from_snapshot_test.go index e252ce9e24..b092479d07 100644 --- a/internal/service/repository/create_from_snapshot_test.go +++ b/internal/service/repository/create_from_snapshot_test.go @@ -106,10 +106,6 @@ func TestCreateRepositoryFromSnapshotSuccess(t *testing.T) { // hooks/ and config were excluded, but the RPC should create them require.FileExists(t, filepath.Join(repoPath, "config"), "Config file not created") - - fi, err := os.Lstat(filepath.Join(repoPath, "hooks")) - require.NoError(t, err) - require.Equal(t, os.ModeSymlink, fi.Mode()&os.ModeSymlink, "Symlink to global hooks not created") } func TestCreateRepositoryFromSnapshotFailsIfRepositoryExists(t *testing.T) { diff --git a/internal/service/repository/create_test.go b/internal/service/repository/create_test.go index 7a36c40f7f..e238f8e186 100644 --- a/internal/service/repository/create_test.go +++ b/internal/service/repository/create_test.go @@ -8,7 +8,6 @@ import ( "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/helper" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "google.golang.org/grpc/codes" @@ -40,16 +39,6 @@ func TestCreateRepositorySuccess(t *testing.T) { require.NoError(t, err) require.True(t, fi.IsDir(), "%q must be a directory", fi.Name()) } - - hooksDir := path.Join(repoDir, "hooks") - - fi, err := os.Lstat(hooksDir) - require.NoError(t, err) - require.True(t, fi.Mode()&os.ModeSymlink > 0, "expected %q to be a symlink, got mode %v", hooksDir, fi.Mode()) - - hooksTarget, err := os.Readlink(hooksDir) - require.NoError(t, err) - require.Equal(t, path.Join(config.Config.GitlabShell.Dir, "hooks"), hooksTarget) } func TestCreateRepositoryFailure(t *testing.T) { diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb index 73373a0a9f..f511b3ef69 100644 --- a/ruby/lib/gitlab/git/repository.rb +++ b/ruby/lib/gitlab/git/repository.rb @@ -61,37 +61,6 @@ module Gitlab # Equivalent to `git --git-path=#{repo_path} init [--bare]` repo = Rugged::Repository.init_at(repo_path, true) repo.close - - # 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) - local_hooks_path = File.join(repo_path, 'hooks') - real_local_hooks_path = :not_found - - begin - real_local_hooks_path = File.realpath(local_hooks_path) - rescue Errno::ENOENT - # real_local_hooks_path == :not_found - end - - # Do nothing if hooks already exist - unless real_local_hooks_path == File.realpath(global_hooks_path) - if File.exist?(local_hooks_path) - # Move the existing hooks somewhere safe - FileUtils.mv( - local_hooks_path, - "#{local_hooks_path}.old.#{Time.now.to_i}" - ) - end - - # Create the hooks symlink - FileUtils.ln_sf(global_hooks_path, local_hooks_path) - end - - true end end diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb index 962be8fc6a..57b88ef597 100644 --- a/ruby/spec/lib/gitlab/git/repository_spec.rb +++ b/ruby/spec/lib/gitlab/git/repository_spec.rb @@ -37,53 +37,6 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength end end - 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.legacy_hooks_directory } - let(:existing_target) { File.join(repo_path, 'foobar') } - - before do - GitlabShellHelper.setup_gitlab_shell - - FileUtils.rm_rf(repo_path) - FileUtils.mkdir_p(repo_path) - end - - context 'hooks is a directory' do - let(:existing_file) { File.join(hooks_dir, 'my-file') } - - before do - FileUtils.mkdir_p(hooks_dir) - FileUtils.touch(existing_file) - described_class.create_hooks(repo_path, target_hooks_dir) - end - - it { expect(File.readlink(hooks_dir)).to eq(target_hooks_dir) } - it { expect(Dir[File.join(repo_path, "hooks.old.*/my-file")].count).to eq(1) } - end - - context 'hooks is a valid symlink' do - before do - FileUtils.mkdir_p existing_target - File.symlink(existing_target, hooks_dir) - described_class.create_hooks(repo_path, target_hooks_dir) - end - - it { expect(File.readlink(hooks_dir)).to eq(target_hooks_dir) } - end - - context 'hooks is a broken symlink' do - before do - FileUtils.rm_f(existing_target) - File.symlink(existing_target, hooks_dir) - described_class.create_hooks(repo_path, target_hooks_dir) - end - - it { expect(File.readlink(hooks_dir)).to eq(target_hooks_dir) } - end - end - describe "Respond to" do subject { repository } -- GitLab