Merge branch 'bvl-use-shell-writeref' into 'master'

Don't use rugged write-ref anymore

See merge request gitlab-org/gitlab-ce!23286

11 files changed, 31 insertions, 24 deletions

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 3eefcb9dd5b..9084fa2f716 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-1.0.0
+1.1.0
diff --git a/Gemfile b/Gemfile
index 3fa1989afb9..1e20c8e7cd7 100644
--- a/Gemfile
+++ b/Gemfile
@@ -425,7 +425,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.123.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 1.1.0', require: 'gitaly'
 gem 'grpc', '~> 1.15.0'
 
 gem 'google-protobuf', '~> 3.6'
diff --git a/Gemfile.lock b/Gemfile.lock
index 20231696795..fdcac3b3952 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -273,7 +273,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.123.0)
+    gitaly-proto (1.1.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-default_value_for (3.1.1)
@@ -1006,7 +1006,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.123.0)
+  gitaly-proto (~> 1.1.0)
   github-markup (~> 1.7.0)
   gitlab-default_value_for (~> 3.1.1)
   gitlab-markup (~> 1.6.5)
diff --git a/Gemfile.rails4.lock b/Gemfile.rails4.lock
index caaa886987b..57f126ddfee 100644
--- a/Gemfile.rails4.lock
+++ b/Gemfile.rails4.lock
@@ -272,7 +272,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (0.123.0)
+    gitaly-proto (1.1.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-markup (1.6.5)
@@ -998,7 +998,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 0.123.0)
+  gitaly-proto (~> 1.1.0)
   github-markup (~> 1.7.0)
   gitlab-markup (~> 1.6.5)
   gitlab-sidekiq-fetcher
diff --git a/app/models/project.rb b/app/models/project.rb
index 4d1917b9ab2..b85ec90f3ca 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1394,7 +1394,7 @@ class Project < ActiveRecord::Base
   def change_head(branch)
     if repository.branch_exists?(branch)
       repository.before_change_head
-      repository.raw_repository.write_ref('HEAD', "refs/heads/#{branch}", shell: false)
+      repository.raw_repository.write_ref('HEAD', "refs/heads/#{branch}")
       repository.copy_gitattributes(branch)
       repository.after_change_head
       reload_default_branch
diff --git a/app/models/repository.rb b/app/models/repository.rb
index a77fa8f2ce7..427dac99b79 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -259,7 +259,7 @@ class Repository
         next if kept_around?(sha)
 
         # This will still fail if the file is corrupted (e.g. 0 bytes)
-        raw_repository.write_ref(keep_around_ref_name(sha), sha, shell: false)
+        raw_repository.write_ref(keep_around_ref_name(sha), sha)
       rescue Gitlab::Git::CommandError => ex
         Rails.logger.error "Unable to create keep-around reference for repository #{disk_path}: #{ex}"
       end
diff --git a/changelogs/unreleased/bvl-use-shell-writeref.yml b/changelogs/unreleased/bvl-use-shell-writeref.yml
new file mode 100644
index 00000000000..682d428e8c5
--- /dev/null
+++ b/changelogs/unreleased/bvl-use-shell-writeref.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid creating invalid refs using rugged, shelling out for writing refs
+merge_request: 23286
+author:
+type: fixed
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index 9b48031b2d3..0a541031884 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -723,11 +723,11 @@ module Gitlab
         delete_refs(tmp_ref)
       end
 
-      def write_ref(ref_path, ref, old_ref: nil, shell: true)
+      def write_ref(ref_path, ref, old_ref: nil)
         ref_path = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{ref_path}" unless ref_path.start_with?("refs/") || ref_path == "HEAD"
 
         wrapped_gitaly_errors do
-          gitaly_repository_client.write_ref(ref_path, ref, old_ref, shell)
+          gitaly_repository_client.write_ref(ref_path, ref, old_ref)
         end
       end
 
diff --git a/lib/gitlab/gitaly_client/repository_service.rb b/lib/gitlab/gitaly_client/repository_service.rb
index 12a0ee16649..8a1abfbf874 100644
--- a/lib/gitlab/gitaly_client/repository_service.rb
+++ b/lib/gitlab/gitaly_client/repository_service.rb
@@ -251,20 +251,15 @@ module Gitlab
         )
       end
 
-      def write_ref(ref_path, ref, old_ref, shell)
+      def write_ref(ref_path, ref, old_ref)
         request = Gitaly::WriteRefRequest.new(
           repository: @gitaly_repo,
           ref: ref_path.b,
-          revision: ref.b,
-          shell: shell
+          revision: ref.b
         )
         request.old_revision = old_ref.b unless old_ref.nil?
 
-        response = GitalyClient.call(@storage, :repository_service, :write_ref, request, timeout: GitalyClient.fast_timeout)
-
-        raise Gitlab::Git::CommandError, encode!(response.error) if response.error.present?
-
-        true
+        GitalyClient.call(@storage, :repository_service, :write_ref, request, timeout: GitalyClient.fast_timeout)
       end
 
       def set_config(entries)
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 1fe73c12fc0..852ee9c96af 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1469,6 +1469,19 @@ describe Gitlab::Git::Repository, :seed_helper do
         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
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 51278836604..d23bdbc2c30 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2203,12 +2203,6 @@ describe Project do
       project.change_head(project.default_branch)
     end
 
-    it 'creates the new reference with rugged' do
-      expect(project.repository.raw_repository).to receive(:write_ref).with('HEAD', "refs/heads/#{project.default_branch}", shell: false)
-
-      project.change_head(project.default_branch)
-    end
-
     it 'copies the gitattributes' do
       expect(project.repository).to receive(:copy_gitattributes).with(project.default_branch)
       project.change_head(project.default_branch)