Merge branch 'bvl-only-use-git-update-ref' into 'master'

Don't use rugged to write refs

Closes #1378

See merge request gitlab-org/gitaly!982

7 files changed, 36 insertions, 58 deletions

diff --git a/changelogs/unreleased/bvl-only-use-git-update-ref.yml b/changelogs/unreleased/bvl-only-use-git-update-ref.yml
new file mode 100644
index 000000000..e30b6f1c7
--- /dev/null
+++ b/changelogs/unreleased/bvl-only-use-git-update-ref.yml
@@ -0,0 +1,5 @@
+---
+title: Don't use rugged when calling write-ref
+merge_request: 982
+author:
+type: other
diff --git a/internal/service/repository/write_ref_test.go b/internal/service/repository/write_ref_test.go
index b8b8601a5..a09fd20be 100644
--- a/internal/service/repository/write_ref_test.go
+++ b/internal/service/repository/write_ref_test.go
@@ -26,30 +26,11 @@ func TestWriteRefSuccessful(t *testing.T) {
 		req  *gitalypb.WriteRefRequest
 	}{
 		{
-			desc: "rugged update refs/heads/master",
-			req: &gitalypb.WriteRefRequest{
-				Repository: testRepo,
-				Ref:        []byte("refs/heads/master"),
-				Revision:   []byte("4a24d82dbca5c11c61556f3b35ca472b7463187e"),
-				Shell:      false,
-			},
-		},
-		{
-			desc: "rugged update refs/keep-around/4a24d82dbca5c11c61556f3b35ca472b7463187e",
-			req: &gitalypb.WriteRefRequest{
-				Repository: testRepo,
-				Ref:        []byte("refs/keep-around/4a24d82dbca5c11c61556f3b35ca472b7463187e"),
-				Revision:   []byte("4a24d82dbca5c11c61556f3b35ca472b7463187e"),
-				Shell:      false,
-			},
-		},
-		{
-			desc: "rugged update HEAD to refs/heads/master",
+			desc: "shell update HEAD to refs/heads/master",
 			req: &gitalypb.WriteRefRequest{
 				Repository: testRepo,
 				Ref:        []byte("HEAD"),
 				Revision:   []byte("refs/heads/master"),
-				Shell:      false,
 			},
 		},
 		{
@@ -58,7 +39,6 @@ func TestWriteRefSuccessful(t *testing.T) {
 				Repository: testRepo,
 				Ref:        []byte("refs/heads/master"),
 				Revision:   []byte("b83d6e391c22777fca1ed3012fce84f633d7fed0"),
-				Shell:      true,
 			},
 		},
 		{
@@ -68,7 +48,6 @@ func TestWriteRefSuccessful(t *testing.T) {
 				Ref:         []byte("refs/heads/master"),
 				Revision:    []byte("498214de67004b1da3d820901307bed2a68a8ef6"),
 				OldRevision: []byte("b83d6e391c22777fca1ed3012fce84f633d7fed0"),
-				Shell:       true,
 			},
 		},
 	}
diff --git a/ruby/Gemfile b/ruby/Gemfile
index b8449d270..6ca1e86b7 100644
--- a/ruby/Gemfile
+++ b/ruby/Gemfile
@@ -6,7 +6,7 @@ gem 'bundler', '>= 1.16.5'
 gem 'rugged', '~> 0.27'
 gem 'github-linguist', '~> 6.1', require: 'linguist'
 gem 'gitlab-markup', '~> 1.6.5'
-gem 'gitaly-proto', '~> 0.123.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 1.1.0'
 gem 'activesupport', '~> 5.0.2'
 gem 'rdoc', '~> 4.2'
 gem 'gitlab-gollum-lib', '~> 4.2', require: false
diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock
index d5868a9e4..53d046698 100644
--- a/ruby/Gemfile.lock
+++ b/ruby/Gemfile.lock
@@ -30,7 +30,7 @@ GEM
       multipart-post (>= 1.2, < 3)
     gemojione (3.3.0)
       json
-    gitaly-proto (0.123.0)
+    gitaly-proto (1.1.0)
       grpc (~> 1.0)
     github-linguist (6.2.0)
       charlock_holmes (~> 0.7.6)
@@ -151,7 +151,7 @@ DEPENDENCIES
   bundler (>= 1.16.5)
   factory_bot
   faraday (~> 0.12)
-  gitaly-proto (~> 0.123.0)
+  gitaly-proto (~> 1.1.0)
   github-linguist (~> 6.1)
   gitlab-gollum-lib (~> 4.2)
   gitlab-gollum-rugged_adapter (~> 0.4.4)
diff --git a/ruby/lib/gitaly_server/repository_service.rb b/ruby/lib/gitaly_server/repository_service.rb
index 6b34be69c..178c0f4f2 100644
--- a/ruby/lib/gitaly_server/repository_service.rb
+++ b/ruby/lib/gitaly_server/repository_service.rb
@@ -66,17 +66,10 @@ module GitalyServer
 
     def write_ref(request, call)
       bridge_exceptions do
-        repo = Gitlab::Git::Repository.from_gitaly(request.repository, call)
-
-        # We ignore the output since the shell-version returns the output
-        #  while the rugged-version returns true. But both throws expections on errors
-        begin
-          repo.write_ref(request.ref, request.revision, old_ref: request.old_revision, shell: request.shell)
+        Gitlab::Git::Repository.from_gitaly(request.repository, call)
+                               .write_ref(request.ref, request.revision, old_ref: request.old_revision)
 
-          Gitaly::WriteRefResponse.new
-        rescue Rugged::OSError => ex
-          Gitaly::WriteRefResponse.new(error: ex.message.b)
-        end
+        Gitaly::WriteRefResponse.new
       end
     end
 
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index 8d2b8bcbd..e0c3f9b82 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -697,11 +697,16 @@ module Gitlab
         Gitlab::Git.committer_hash(email: user.email, name: user.name)
       end
 
-      def write_ref(ref_path, ref, old_ref: nil, shell: true)
-        if shell
-          shell_write_ref(ref_path, ref, old_ref)
+      def write_ref(ref_path, ref, old_ref: nil)
+        raise ArgumentError, "invalid ref_path #{ref_path.inspect}" if ref_path.include?(' ')
+        raise ArgumentError, "invalid ref #{ref.inspect}" if ref.include?("\x00")
+        raise ArgumentError, "invalid old_ref #{old_ref.inspect}" if !old_ref.nil? && old_ref.include?("\x00")
+
+        if ref_path == 'HEAD'
+          run_git!(%W[symbolic-ref #{ref_path} #{ref}])
         else
-          rugged_write_ref(ref_path, ref)
+          input = "update #{ref_path}\x00#{ref}\x00#{old_ref}\x00"
+          run_git!(%w[update-ref --stdin -z]) { |stdin| stdin.write(input) }
         end
       end
 
@@ -1003,25 +1008,6 @@ module Gitlab
         Rugged::Commit.create(rugged, params)
       end
 
-      def shell_write_ref(ref_path, ref, old_ref)
-        raise ArgumentError, "invalid ref_path #{ref_path.inspect}" if ref_path.include?(' ')
-        raise ArgumentError, "invalid ref #{ref.inspect}" if ref.include?("\x00")
-        raise ArgumentError, "invalid old_ref #{old_ref.inspect}" if !old_ref.nil? && old_ref.include?("\x00")
-
-        input = "update #{ref_path}\x00#{ref}\x00#{old_ref}\x00"
-        run_git!(%w[update-ref --stdin -z]) { |stdin| stdin.write(input) }
-      end
-
-      def rugged_write_ref(ref_path, ref)
-        rugged.references.create(ref_path, ref, force: true)
-      rescue Rugged::ReferenceError => ex
-        Rails.logger.error "Unable to create #{ref_path} reference for repository #{path}: #{ex}"
-      rescue Rugged::OSError => ex
-        raise unless ex.message =~ /Failed to create locked file/ && ex.message =~ /File exists/
-
-        Rails.logger.error "Unable to create #{ref_path} reference for repository #{path}: #{ex}"
-      end
-
       def rugged_head
         rugged.head
       rescue Rugged::ReferenceError
diff --git a/ruby/spec/lib/gitlab/git/repository_spec.rb b/ruby/spec/lib/gitlab/git/repository_spec.rb
index bc0f1bc64..e5cfb7d7b 100644
--- a/ruby/spec/lib/gitlab/git/repository_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_spec.rb
@@ -382,6 +382,8 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
   end
 
   describe '#write_ref' do
+    let(:repository) { mutable_repository }
+
     context 'validations' do
       using RSpec::Parameterized::TableSyntax
 
@@ -396,6 +398,19 @@ describe Gitlab::Git::Repository do # rubocop:disable Metrics/BlockLength
         end
       end
     end
+
+    it 'writes the HEAD' do
+      repository.write_ref('HEAD', 'refs/heads/feature')
+
+      expect(repository.commit('HEAD')).to eq(repository.commit('feature'))
+      expect(repository.root_ref).to eq('feature')
+    end
+
+    it 'writes other refs' do
+      repository.write_ref('refs/heads/feature', SeedRepo::Commit::ID)
+
+      expect(repository.commit('feature').sha).to eq(SeedRepo::Commit::ID)
+    end
   end
 
   describe '#write_config' do