Merge branch '628-add-username-string-field-to-user-message' into 'master'

Set `GL_USERNAME` env variable for hook excecution

Closes #628

See merge request gitlab-org/gitaly!423

17 files changed, 144 insertions, 50 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 042b0d8b9..4d51e4b60 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Gitaly changelog
 
+UNRELEASED
+
+- Update gitlab_git to b3ba3996e0bd329eaa574ff53c69673efaca6eef and set
+  `GL_USERNAME` env variable for hook excecution
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/423
+
 v0.50.0
 
 - Pass repo git alternate dirs to gitaly-ruby
diff --git a/internal/service/operations/branches_test.go b/internal/service/operations/branches_test.go
index 31fb83799..7a2b1b490 100644
--- a/internal/service/operations/branches_test.go
+++ b/internal/service/operations/branches_test.go
@@ -90,9 +90,10 @@ func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
 
 	branchName := "new-branch"
 	user := &pb.User{
-		Name:  []byte("Alejandro Rodríguez"),
-		Email: []byte("alejandro@gitlab.com"),
-		GlId:  "user-1",
+		Name:       []byte("Alejandro Rodríguez"),
+		Email:      []byte("alejandro@gitlab.com"),
+		GlId:       "user-1",
+		GlUsername: "johndoe",
 	}
 	request := &pb.UserCreateBranchRequest{
 		Repository: testRepo,
@@ -117,6 +118,7 @@ func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
 
 			output := string(testhelper.MustReadFile(t, hookOutputTempPath))
 			require.Contains(t, output, "GL_ID="+user.GlId)
+			require.Contains(t, output, "GL_USERNAME="+user.GlUsername)
 		})
 	}
 }
@@ -129,9 +131,10 @@ func TestFailedUserCreateBranchDueToHooks(t *testing.T) {
 	defer conn.Close()
 
 	user := &pb.User{
-		Name:  []byte("Alejandro Rodríguez"),
-		Email: []byte("alejandro@gitlab.com"),
-		GlId:  "user-1",
+		Name:       []byte("Alejandro Rodríguez"),
+		Email:      []byte("alejandro@gitlab.com"),
+		GlId:       "user-1",
+		GlUsername: "johndoe",
 	}
 	request := &pb.UserCreateBranchRequest{
 		Repository: testRepo,
@@ -154,6 +157,7 @@ func TestFailedUserCreateBranchDueToHooks(t *testing.T) {
 		response, err := client.UserCreateBranch(ctx, request)
 		require.Nil(t, err)
 		require.Contains(t, response.PreReceiveError, "GL_ID="+user.GlId)
+		require.Contains(t, response.PreReceiveError, "GL_USERNAME="+user.GlUsername)
 		require.Contains(t, response.PreReceiveError, "GL_REPOSITORY="+testRepo.GlRepository)
 		require.Contains(t, response.PreReceiveError, "GL_PROTOCOL=web")
 		require.Contains(t, response.PreReceiveError, "PWD="+testRepoPath)
@@ -273,9 +277,10 @@ func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) {
 	defer exec.Command("git", "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
 
 	user := &pb.User{
-		Name:  []byte("Alejandro Rodríguez"),
-		Email: []byte("alejandro@gitlab.com"),
-		GlId:  "user-123",
+		Name:       []byte("Alejandro Rodríguez"),
+		Email:      []byte("alejandro@gitlab.com"),
+		GlId:       "user-123",
+		GlUsername: "johndoe",
 	}
 
 	request := &pb.UserDeleteBranchRequest{
@@ -299,6 +304,7 @@ func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) {
 
 			output := testhelper.MustReadFile(t, hookOutputTempPath)
 			require.Contains(t, string(output), "GL_ID=user-123")
+			require.Contains(t, string(output), "GL_USERNAME=johndoe")
 		})
 	}
 }
diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go
index 5f1de6064..94209ce0f 100644
--- a/internal/service/operations/tags_test.go
+++ b/internal/service/operations/tags_test.go
@@ -63,9 +63,10 @@ func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) {
 	defer exec.Command("git", "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
 
 	user := &pb.User{
-		Name:  []byte("Ahmad Sherif"),
-		Email: []byte("ahmad@gitlab.com"),
-		GlId:  "user-123",
+		Name:       []byte("Ahmad Sherif"),
+		Email:      []byte("ahmad@gitlab.com"),
+		GlId:       "user-123",
+		GlUsername: "johndoe",
 	}
 
 	request := &pb.UserDeleteTagRequest{
@@ -89,6 +90,7 @@ func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) {
 
 			output := testhelper.MustReadFile(t, hookOutputTempPath)
 			require.Contains(t, string(output), "GL_ID=user-123")
+			require.Contains(t, string(output), "GL_USERNAME=johndoe")
 		})
 	}
 }
@@ -181,9 +183,10 @@ func TestSuccessfulGitHooksForUserCreateTagRequest(t *testing.T) {
 
 	tagName := "new-tag"
 	user := &pb.User{
-		Name:  []byte("Ahmad Sherif"),
-		Email: []byte("ahmad@gitlab.com"),
-		GlId:  "user-123",
+		Name:       []byte("Ahmad Sherif"),
+		Email:      []byte("ahmad@gitlab.com"),
+		GlId:       "user-123",
+		GlUsername: "johndoe",
 	}
 	request := &pb.UserCreateTagRequest{
 		Repository:     testRepo,
@@ -208,6 +211,7 @@ func TestSuccessfulGitHooksForUserCreateTagRequest(t *testing.T) {
 
 			output := string(testhelper.MustReadFile(t, hookOutputTempPath))
 			require.Contains(t, output, "GL_ID="+user.GlId)
+			require.Contains(t, output, "GL_USERNAME="+user.GlUsername)
 		})
 	}
 }
@@ -320,9 +324,10 @@ func TestFailedUserCreateTagDueToHooks(t *testing.T) {
 	defer conn.Close()
 
 	user := &pb.User{
-		Name:  []byte("Ahmad Sherif"),
-		Email: []byte("ahmad@gitlab.com"),
-		GlId:  "user-123",
+		Name:       []byte("Ahmad Sherif"),
+		Email:      []byte("ahmad@gitlab.com"),
+		GlId:       "user-123",
+		GlUsername: "johndoe",
 	}
 	request := &pb.UserCreateTagRequest{
 		Repository:     testRepo,
diff --git a/ruby/Gemfile b/ruby/Gemfile
index 8e5b66a56..13a922ef7 100644
--- a/ruby/Gemfile
+++ b/ruby/Gemfile
@@ -1,7 +1,7 @@
 source 'https://rubygems.org'
 
 gem 'github-linguist', '~> 4.7.0', require: 'linguist'
-gem 'gitaly-proto', '~> 0.45.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.48.0', require: 'gitaly'
 gem 'activesupport'
 gem 'gollum-lib', '~> 4.2', require: false
 gem 'gollum-rugged_adapter', '~> 0.4.4', require: false
diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock
index 9a360eb3f..1849665fe 100644
--- a/ruby/Gemfile.lock
+++ b/ruby/Gemfile.lock
@@ -17,7 +17,7 @@ GEM
       multipart-post (>= 1.2, < 3)
     gemojione (3.3.0)
       json
-    gitaly-proto (0.45.0)
+    gitaly-proto (0.48.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (4.7.6)
@@ -119,7 +119,7 @@ PLATFORMS
 
 DEPENDENCIES
   activesupport
-  gitaly-proto (~> 0.45.0)
+  gitaly-proto (~> 0.48.0)
   github-linguist (~> 4.7.0)
   gitlab-styles (~> 2.0.0)
   gollum-lib (~> 4.2)
diff --git a/ruby/vendor/gitlab_git/REVISION b/ruby/vendor/gitlab_git/REVISION
index acf51f1fc..80906ffd1 100644
--- a/ruby/vendor/gitlab_git/REVISION
+++ b/ruby/vendor/gitlab_git/REVISION
@@ -1 +1 @@
-c23c09366db610c1994fa592961ae838001ed5b0
+b3ba3996e0bd329eaa574ff53c69673efaca6eef
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/encoding_helper.rb b/ruby/vendor/gitlab_git/lib/gitlab/encoding_helper.rb
index 7b3483a7f..99dfee3dd 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/encoding_helper.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/encoding_helper.rb
@@ -14,9 +14,9 @@ module Gitlab
     ENCODING_CONFIDENCE_THRESHOLD = 50
 
     def encode!(message)
-      return nil unless message.respond_to? :force_encoding
+      return nil unless message.respond_to?(:force_encoding)
+      return message if message.encoding == Encoding::UTF_8 && message.valid_encoding?
 
-      # if message is utf-8 encoding, just return it
       message.force_encoding("UTF-8")
       return message if message.valid_encoding?
 
@@ -50,6 +50,9 @@ module Gitlab
     end
 
     def encode_utf8(message)
+      return nil unless message.is_a?(String)
+      return message if message.encoding == Encoding::UTF_8 && message.valid_encoding?
+
       detect = CharlockHolmes::EncodingDetector.detect(message)
       if detect && detect[:encoding]
         begin
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/hooks_service.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/hooks_service.rb
index c327e9b16..f302b852b 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/hooks_service.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/hooks_service.rb
@@ -8,7 +8,7 @@ module Gitlab
       def execute(pusher, repository, oldrev, newrev, ref)
         @repository  = repository
         @gl_id       = pusher.gl_id
-        @gl_username = pusher.name
+        @gl_username = pusher.username
         @oldrev      = oldrev
         @newrev      = newrev
         @ref         = ref
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb
index 59a54b48e..408616d17 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb
@@ -166,7 +166,7 @@ module Gitlab
       end
 
       def local_branches(sort_by: nil)
-        gitaly_migrate(:local_branches) do |is_enabled|
+        gitaly_migrate(:local_branches, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
           if is_enabled
             gitaly_ref_client.local_branches(sort_by: sort_by)
           else
@@ -745,6 +745,16 @@ module Gitlab
         nil
       end
 
+      def ff_merge(user, source_sha, target_branch)
+        OperationService.new(user, self).with_branch(target_branch) do |our_commit|
+          raise ArgumentError, 'Invalid merge target' unless our_commit
+
+          source_sha
+        end
+      rescue Rugged::ReferenceError
+        raise ArgumentError, 'Invalid merge source'
+      end
+
       def revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
         OperationService.new(user, self).with_branch(
           branch_name,
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/storage.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/storage.rb
index 08e6c29ab..99518c9b1 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/storage.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/storage.rb
@@ -12,6 +12,7 @@ module Gitlab
 
       CircuitOpen = Class.new(Inaccessible)
       Misconfiguration = Class.new(Inaccessible)
+      Failing = Class.new(Inaccessible)
 
       REDIS_KEY_PREFIX = 'storage_accessible:'.freeze
 
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker.rb
index 0456ad9a1..be7598ef0 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker.rb
@@ -54,7 +54,7 @@ module Gitlab
         end
 
         def perform
-          return yield unless Feature.enabled?('git_storage_circuit_breaker')
+          return yield unless enabled?
 
           check_storage_accessible!
 
@@ -64,10 +64,27 @@ module Gitlab
         def circuit_broken?
           return false if no_failures?
 
+          failure_count > failure_count_threshold
+        end
+
+        def backing_off?
+          return false if no_failures?
+
           recent_failure = last_failure > failure_wait_time.seconds.ago
-          too_many_failures = failure_count > failure_count_threshold
+          too_many_failures = failure_count > backoff_threshold
 
-          recent_failure || too_many_failures
+          recent_failure && too_many_failures
+        end
+
+        private
+
+        # The circuitbreaker can be enabled for the entire fleet using a Feature
+        # flag.
+        #
+        # Enabling it for a single host can be done setting the
+        # `GIT_STORAGE_CIRCUIT_BREAKER` environment variable.
+        def enabled?
+          ENV['GIT_STORAGE_CIRCUIT_BREAKER'].present? || Feature.enabled?('git_storage_circuit_breaker')
         end
 
         def failure_info
@@ -83,7 +100,7 @@ module Gitlab
           return @storage_available if @storage_available
 
           if @storage_available = Gitlab::Git::Storage::ForkedStorageCheck
-                                    .storage_available?(storage_path, storage_timeout)
+                                    .storage_available?(storage_path, storage_timeout, access_retries)
             track_storage_accessible
           else
             track_storage_inaccessible
@@ -94,7 +111,11 @@ module Gitlab
 
         def check_storage_accessible!
           if circuit_broken?
-            raise Gitlab::Git::Storage::CircuitOpen.new("Circuit for #{storage} is broken", failure_wait_time)
+            raise Gitlab::Git::Storage::CircuitOpen.new("Circuit for #{storage} is broken", failure_reset_time)
+          end
+
+          if backing_off?
+            raise Gitlab::Git::Storage::Failing.new("Backing off access to #{storage}", failure_wait_time)
           end
 
           unless storage_available?
@@ -131,12 +152,6 @@ module Gitlab
           end
         end
 
-        def cache_key
-          @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}"
-        end
-
-        private
-
         def get_failure_info
           last_failure, failure_count = Gitlab::Git::Storage.redis.with do |redis|
             redis.hmget(cache_key, :last_failure, :failure_count)
@@ -146,6 +161,10 @@ module Gitlab
 
           FailureInfo.new(last_failure, failure_count.to_i)
         end
+
+        def cache_key
+          @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}"
+        end
       end
     end
   end
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker_settings.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker_settings.rb
index d2313fe7c..257fe8cd8 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker_settings.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/circuit_breaker_settings.rb
@@ -18,6 +18,14 @@ module Gitlab
           application_settings.circuitbreaker_storage_timeout
         end
 
+        def access_retries
+          application_settings.circuitbreaker_access_retries
+        end
+
+        def backoff_threshold
+          application_settings.circuitbreaker_backoff_threshold
+        end
+
         private
 
         def application_settings
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/forked_storage_check.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/forked_storage_check.rb
index 91d8241f1..1307f4007 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/forked_storage_check.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/forked_storage_check.rb
@@ -4,8 +4,17 @@ module Gitlab
       module ForkedStorageCheck
         extend self
 
-        def storage_available?(path, timeout_seconds = 5)
-          status = timeout_check(path, timeout_seconds)
+        def storage_available?(path, timeout_seconds = 5, retries = 1)
+          partial_timeout = timeout_seconds / retries
+          status = timeout_check(path, partial_timeout)
+
+          # If the status check did not succeed the first time, we retry a few
+          # more times to avoid one-off failures
+          current_attempts = 1
+          while current_attempts < retries && !status.success?
+            status = timeout_check(path, partial_timeout)
+            current_attempts += 1
+          end
 
           status.success?
         end
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/null_circuit_breaker.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/null_circuit_breaker.rb
index 60c6791a7..a12d52d29 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/storage/null_circuit_breaker.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/storage/null_circuit_breaker.rb
@@ -25,6 +25,10 @@ module Gitlab
           !!@error
         end
 
+        def backing_off?
+          false
+        end
+
         def last_failure
           circuit_broken? ? Time.now : nil
         end
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/user.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/user.rb
index da74719ae..e6b61417d 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/user.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/user.rb
@@ -7,9 +7,8 @@ module Gitlab
         new(gitlab_user.username, gitlab_user.name, gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user))
       end
 
-      # TODO support the username field in Gitaly https://gitlab.com/gitlab-org/gitaly/issues/628
       def self.from_gitaly(gitaly_user)
-        new('', gitaly_user.name, gitaly_user.email, gitaly_user.gl_id)
+        new(gitaly_user.gl_username, gitaly_user.name, gitaly_user.email, gitaly_user.gl_id)
       end
 
       def initialize(username, name, email, gl_id)
@@ -22,6 +21,10 @@ module Gitlab
       def ==(other)
         [username, name, email, gl_id] == [other.username, other.name, other.email, other.gl_id]
       end
+
+      def to_gitaly
+        Gitaly::User.new(gl_username: username, gl_id: gl_id, name: name, email: email)
+      end
     end
   end
 end
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/wiki.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/wiki.rb
index d651c931a..e7b2f52a5 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/wiki.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/wiki.rb
@@ -23,14 +23,14 @@ module Gitlab
       end
 
       def write_page(name, format, content, commit_details)
-        assert_type!(format, Symbol)
-        assert_type!(commit_details, CommitDetails)
-
-        gollum_wiki.write_page(name, format, content, commit_details.to_h)
-
-        nil
-      rescue Gollum::DuplicatePageError => e
-        raise Gitlab::Git::Wiki::DuplicatePageError, e.message
+        @repository.gitaly_migrate(:wiki_write_page) do |is_enabled|
+          if is_enabled
+            gitaly_write_page(name, format, content, commit_details)
+            gollum_wiki.clear_cache
+          else
+            gollum_write_page(name, format, content, commit_details)
+          end
+        end
       end
 
       def delete_page(page_path, commit_details)
@@ -110,6 +110,25 @@ module Gitlab
           raise ArgumentError, "expected a #{klass}, got #{object.inspect}"
         end
       end
+
+      def gitaly_wiki_client
+        @gitaly_wiki_client ||= Gitlab::GitalyClient::WikiService.new(@repository)
+      end
+
+      def gollum_write_page(name, format, content, commit_details)
+        assert_type!(format, Symbol)
+        assert_type!(commit_details, CommitDetails)
+
+        gollum_wiki.write_page(name, format, content, commit_details.to_h)
+
+        nil
+      rescue Gollum::DuplicatePageError => e
+        raise Gitlab::Git::Wiki::DuplicatePageError, e.message
+      end
+
+      def gitaly_write_page(name, format, content, commit_details)
+        gitaly_wiki_client.write_page(name, format, content, commit_details)
+      end
     end
   end
 end
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/wiki_file.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/wiki_file.rb
index 527f2a44d..84335aca4 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/wiki_file.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/wiki_file.rb
@@ -1,7 +1,7 @@
 module Gitlab
   module Git
     class WikiFile
-      attr_reader :mime_type, :raw_data, :name
+      attr_reader :mime_type, :raw_data, :name, :path
 
       # This class is meant to be serializable so that it can be constructed
       # by Gitaly and sent over the network to GitLab.
@@ -13,6 +13,7 @@ module Gitlab
         @mime_type = gollum_file.mime_type
         @raw_data = gollum_file.raw_data
         @name = gollum_file.name
+        @path = gollum_file.path
       end
     end
   end