Merge branch 'ce-to-ee-2018-07-20' into 'master'

CE upstream - 2018-07-20 05:29 UTC

See merge request gitlab-org/gitlab-ee!6601

changelogs/unreleased/add-total-time-flat-printer-for-profiling.yml

+---
+title: Add a Gitlab::Profiler.print_by_total_time convenience method for profiling
+  from a Rails console
+merge_request:
+author:
+type: other

changelogs/unreleased/gitaly-ff-branch-nil.yml

+---
+title: Add missing Gitaly branch_update nil checks
+merge_request: 20711
+author:
+type: fixed

doc/api/projects.md

@@ -55,6 +55,8 @@ GET /projects
 | `with_custom_attributes` | boolean | no | Include [custom attributes](custom_attributes.md) in response (admins only) |
 | `with_issues_enabled` | boolean | no | Limit by enabled issues feature |
 | `with_merge_requests_enabled` | boolean | no | Limit by enabled merge requests feature |
+| `wiki_checksum_failed` | boolean | no | Limit projects where the wiki checksum calculation has failed _([Introduced][ee-6137] in [GitLab Premium][eep] 11.2)_ |
+| `repository_checksum_failed` | boolean | no | Limit projects where the repository checksum calculation has failed _([Introduced][ee-6137] in [GitLab Premium][eep] 11.2)_ |
 
 When `simple=true` or the user is unauthenticated this returns something like:
 
@@ -1622,3 +1624,6 @@ GET /projects/:id/snapshot
 | --------- | ---- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
 | `wiki`    | boolean | no | Whether to download the wiki, rather than project, repository |
+
+[eep]: https://about.gitlab.com/pricing/ "Available only in GitLab Premium"
+[ee-6137]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6137

doc/development/profiling.md

@@ -42,6 +42,36 @@ Passing a `logger:` keyword argument to `Gitlab::Profiler.profile` will send
 ActiveRecord and ActionController log output to that logger. Further options are
 documented with the method source.
 
+There is also a RubyProf printer available:
+`Gitlab::Profiler::TotalTimeFlatPrinter`. This acts like
+`RubyProf::FlatPrinter`, but its `min_percent` option works on the method's
+total time, not its self time. (This is because we often spend most of our time
+in library code, but this comes from calls in our application.) It also offers a
+`max_percent` option to help filter out outer calls that aren't useful (like
+`ActionDispatch::Integration::Session#process`).
+
+There is a convenience method for using this,
+`Gitlab::Profiler.print_by_total_time`:
+
+```ruby
+result = Gitlab::Profiler.profile('/my-user')
+Gitlab::Profiler.print_by_total_time(result, max_percent: 60, min_percent: 2)
+# Measure Mode: wall_time
+# Thread ID: 70005223698240
+# Fiber ID: 70004894952580
+# Total: 1.768912
+# Sort by: total_time
+#
+#  %self      total      self      wait     child     calls  name
+#   0.00      1.017     0.000     0.000     1.017       14  *ActionView::Helpers::RenderingHelper#render
+#   0.00      1.017     0.000     0.000     1.017       14  *ActionView::Renderer#render_partial
+#   0.00      1.017     0.000     0.000     1.017       14  *ActionView::PartialRenderer#render
+#   0.00      1.007     0.000     0.000     1.007       14  *ActionView::PartialRenderer#render_partial
+#   0.00      0.930     0.000     0.000     0.930       14   Hamlit::TemplateHandler#call
+#   0.00      0.928     0.000     0.000     0.928       14   Temple::Engine#call
+#   0.02      0.865     0.000     0.000     0.864      638  *Enumerable#inject
+```
+
 [GitLab-Profiler](https://gitlab.com/gitlab-com/gitlab-profiler) is a project
 that builds on this to add some additional niceties, such as allowing
 configuration with a single Yaml file for multiple URLs, and uploading of the

lib/api/projects.rb

@@ -8,6 +8,21 @@ module API
 
     before { authenticate_non_get! }
 
+    helpers do
+      params :optional_filter_params_ee do
+        # EE::API::Projects would override this helper
+      end
+
+      # EE::API::Projects would override this method
+      def apply_filters(projects)
+        projects = projects.with_issues_available_for_user(current_user) if params[:with_issues_enabled]
+        projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
+        projects = projects.with_statistics if params[:statistics]
+
+        projects
+      end
+    end
+
     helpers do
       params :statistics_params do
         optional :statistics, type: Boolean, default: false, desc: 'Include project statistics'
@@ -39,6 +54,8 @@ module API
         optional :membership, type: Boolean, default: false, desc: 'Limit by projects that the current user is a member of'
         optional :with_issues_enabled, type: Boolean, default: false, desc: 'Limit by enabled issues feature'
         optional :with_merge_requests_enabled, type: Boolean, default: false, desc: 'Limit by enabled merge requests feature'
+
+        use :optional_filter_params_ee
       end
 
       params :create_params do
@@ -52,9 +69,7 @@ module API
 
       def present_projects(projects, options = {})
         projects = reorder_projects(projects)
-        projects = projects.with_issues_available_for_user(current_user) if params[:with_issues_enabled]
-        projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
-        projects = projects.with_statistics if params[:statistics]
+        projects = apply_filters(projects)
         projects = paginate(projects)
         projects, options = with_custom_attributes(projects, options)
 

lib/gitlab/git/operation_service.rb

@@ -8,6 +8,8 @@ module Gitlab
         alias_method :branch_created?, :branch_created
 
         def self.from_gitaly(branch_update)
+          return if branch_update.nil?
+
           new(
             branch_update.commit_id,
             branch_update.repo_created,

lib/gitlab/gitaly_client/operation_service.rb

@@ -144,13 +144,14 @@ module Gitlab
           branch: encode_binary(target_branch)
         )
 
-        branch_update = GitalyClient.call(
+        response = GitalyClient.call(
           @repository.storage,
           :operation_service,
           :user_ff_branch,
           request
-        ).branch_update
-        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update)
+        )
+
+        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
       rescue GRPC::FailedPrecondition => e
         raise Gitlab::Git::CommitError, e
       end
@@ -306,9 +307,9 @@ module Gitlab
           raise Gitlab::Git::CommitError, response.commit_error
         elsif response.create_tree_error.presence
           raise Gitlab::Git::Repository::CreateTreeError, response.create_tree_error
-        else
-          Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
         end
+
+        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
       end
 
       def user_commit_files_request_header(

lib/gitlab/profiler.rb

@@ -146,5 +146,11 @@ module Gitlab
         logger.info("#{model} total (#{query_count}): #{time.round(2)}ms")
       end
     end
+
+    def self.print_by_total_time(result, options = {})
+      default_options = { sort_method: :total_time }
+
+      Gitlab::Profiler::TotalTimeFlatPrinter.new(result).print(STDOUT, default_options.merge(options))
+    end
   end
 end

lib/gitlab/profiler/total_time_flat_printer.rb

+module Gitlab
+  module Profiler
+    class TotalTimeFlatPrinter < RubyProf::FlatPrinter
+      def max_percent
+        @options[:max_percent] || 100
+      end
+
+      # Copied from:
+      #   <https://github.com/ruby-prof/ruby-prof/blob/master/lib/ruby-prof/printers/flat_printer.rb>
+      #
+      # The changes are just to filter by total time, not self time, and add a
+      # max_percent option as well.
+      def print_methods(thread)
+        total_time = thread.total_time
+        methods = thread.methods.sort_by(&sort_method).reverse
+
+        sum = 0
+        methods.each do |method|
+          total_percent = (method.total_time / total_time) * 100
+          next if total_percent < min_percent
+          next if total_percent > max_percent
+
+          sum += method.self_time
+
+          @output << "%6.2f  %9.3f %9.3f %9.3f %9.3f %8d  %s%s\n" % [
+            method.self_time / total_time * 100, # %self
+            method.total_time,                   # total
+            method.self_time,                    # self
+            method.wait_time,                    # wait
+            method.children_time,                # children
+            method.called,                       # calls
+            method.recursive? ? "*" : " ",       # cycle
+            method_name(method)                  # name
+          ]
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/gitaly_client/operation_service_spec.rb

 require 'spec_helper'
 
 describe Gitlab::GitalyClient::OperationService do
-  let(:project) { create(:project) }
+  set(:project) { create(:project, :repository) }
   let(:repository) { project.repository.raw }
   let(:client) { described_class.new(repository) }
-  let(:user) { create(:user) }
+  set(:user) { create(:user) }
   let(:gitaly_user) { Gitlab::Git::User.from_gitlab(user).to_gitaly }
 
   describe '#user_create_branch' do
@@ -151,18 +151,104 @@ describe Gitlab::GitalyClient::OperationService do
     end
     let(:response) { Gitaly::UserFFBranchResponse.new(branch_update: branch_update) }
 
-    subject { client.user_ff_branch(user, source_sha, target_branch) }
-
-    it 'sends a user_ff_branch message and returns a BranchUpdate object' do
+    before do
       expect_any_instance_of(Gitaly::OperationService::Stub)
         .to receive(:user_ff_branch).with(request, kind_of(Hash))
         .and_return(response)
+    end
 
+    subject { client.user_ff_branch(user, source_sha, target_branch) }
+
+    it 'sends a user_ff_branch message and returns a BranchUpdate object' do
       expect(subject).to be_a(Gitlab::Git::OperationService::BranchUpdate)
       expect(subject.newrev).to eq(source_sha)
       expect(subject.repo_created).to be(false)
       expect(subject.branch_created).to be(false)
     end
+
+    context 'when the response has no branch_update' do
+      let(:response) { Gitaly::UserFFBranchResponse.new }
+
+      it { expect(subject).to be_nil }
+    end
+  end
+
+  shared_examples 'cherry pick and revert errors' do
+    context 'when a pre_receive_error is present' do
+      let(:response) { response_class.new(pre_receive_error: "something failed") }
+
+      it 'raises a PreReceiveError' do
+        expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
+      end
+    end
+
+    context 'when a commit_error is present' do
+      let(:response) { response_class.new(commit_error: "something failed") }
+
+      it 'raises a CommitError' do
+        expect { subject }.to raise_error(Gitlab::Git::CommitError, "something failed")
+      end
+    end
+
+    context 'when a create_tree_error is present' do
+      let(:response) { response_class.new(create_tree_error: "something failed") }
+
+      it 'raises a CreateTreeError' do
+        expect { subject }.to raise_error(Gitlab::Git::Repository::CreateTreeError, "something failed")
+      end
+    end
+
+    context 'when branch_update is nil' do
+      let(:response) { response_class.new }
+
+      it { expect(subject).to be_nil }
+    end
+  end
+
+  describe '#user_cherry_pick' do
+    let(:response_class) { Gitaly::UserCherryPickResponse }
+
+    subject do
+      client.user_cherry_pick(
+        user: user,
+        commit: repository.commit,
+        branch_name: 'master',
+        message: 'Cherry-pick message',
+        start_branch_name: 'master',
+        start_repository: repository
+      )
+    end
+
+    before do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_cherry_pick).with(kind_of(Gitaly::UserCherryPickRequest), kind_of(Hash))
+        .and_return(response)
+    end
+
+    it_behaves_like 'cherry pick and revert errors'
+  end
+
+  describe '#user_revert' do
+    let(:response_class) { Gitaly::UserRevertResponse }
+
+    subject do
+      client.user_revert(
+        user: user,
+        commit: repository.commit,
+        branch_name: 'master',
+        message: 'Revert message',
+        start_branch_name: 'master',
+        start_repository: repository
+      )
+    end
+
+    before do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_revert).with(kind_of(Gitaly::UserRevertRequest), kind_of(Hash))
+        .and_return(response)
+    end
+
+    it_behaves_like 'cherry pick and revert errors'
   end
 
   describe '#user_squash' do
@@ -203,7 +289,7 @@ describe Gitlab::GitalyClient::OperationService do
         Gitaly::UserSquashResponse.new(git_error: "something failed")
       end
 
-      it "throws a PreReceive exception" do
+      it "raises a GitError exception" do
         expect_any_instance_of(Gitaly::OperationService::Stub)
           .to receive(:user_squash).with(request, kind_of(Hash))
           .and_return(response)
@@ -212,5 +298,41 @@ describe Gitlab::GitalyClient::OperationService do
           Gitlab::Git::Repository::GitError, "something failed")
       end
     end
+
+    describe '#user_commit_files' do
+      subject do
+        client.user_commit_files(
+          gitaly_user, 'my-branch', 'Commit files message', [], 'janedoe@example.com', 'Jane Doe',
+          'master', repository)
+      end
+
+      before do
+        expect_any_instance_of(Gitaly::OperationService::Stub)
+          .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash))
+          .and_return(response)
+      end
+
+      context 'when a pre_receive_error is present' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "something failed") }
+
+        it 'raises a PreReceiveError' do
+          expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
+        end
+      end
+
+      context 'when an index_error is present' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new(index_error: "something failed") }
+
+        it 'raises a PreReceiveError' do
+          expect { subject }.to raise_error(Gitlab::Git::Index::IndexError, "something failed")
+        end
+      end
+
+      context 'when branch_update is nil' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new }
+
+        it { expect(subject).to be_nil }
+      end
+    end
   end
 end