Merge branch 'ce-to-ee-2018-01-31' into 'master'

CE upstream - 2018-01-31 15:29 UTC

Closes gitaly#979 et gitaly#967

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

.codeclimate.yml

 ---
 engines:
-  brakeman:
-    enabled: true
   bundler-audit:
     enabled: true
   duplication:

app/assets/javascripts/api.js

@@ -26,7 +26,11 @@ const Api = {
     const url = Api.buildUrl(Api.groupPath)
       .replace(':id', groupId);
     return axios.get(url)
-      .then(({ data }) => callback(data));
+      .then(({ data }) => {
+        callback(data);
+
+        return data;
+      });
   },
 
   // Return groups list. Filtered by query

app/assets/javascripts/lib/utils/common_utils.js

 import { getLocationHash } from './url_utility';
+import axios from './axios_utils';
 
 export const getPagePath = (index = 0) => $('body').attr('data-page').split(':')[index];
 
@@ -382,22 +383,16 @@ export const resetFavicon = () => {
   }
 };
 
-export const setCiStatusFavicon = (pageUrl) => {
-  $.ajax({
-    url: pageUrl,
-    dataType: 'json',
-    success: (data) => {
+export const setCiStatusFavicon = pageUrl =>
+  axios.get(pageUrl)
+    .then(({ data }) => {
       if (data && data.favicon) {
         setFavicon(data.favicon);
       } else {
         resetFavicon();
       }
-    },
-    error: () => {
-      resetFavicon();
-    },
-  });
-};
+    })
+    .catch(resetFavicon);
 
 export const spriteIcon = (icon, className = '') => {
   const classAttribute = className.length > 0 ? `class="${className}"` : '';

app/controllers/groups_controller.rb

@@ -157,7 +157,6 @@ class GroupsController < Groups::ApplicationController
     @projects = GroupProjectsFinder.new(params: params, group: group, options: options, current_user: current_user)
                   .execute
                   .includes(:namespace)
-                  .page(params[:page])
 
     @events = EventCollection
       .new(@projects, offset: params[:offset].to_i, filter: event_filter)

app/models/repository.rb

@@ -901,7 +901,7 @@ class Repository
     @root_ref_sha ||= commit(root_ref).sha
   end
 
-  delegate :merged_branch_names, to: :raw_repository
+  delegate :merged_branch_names, :can_be_merged?, to: :raw_repository
 
   def merge_base(first_commit_id, second_commit_id)
     first_commit_id = commit(first_commit_id).try(:id) || first_commit_id
@@ -1020,7 +1020,7 @@ class Repository
           end
 
         instance_variable_set(ivar, value)
-      rescue Rugged::ReferenceError, Gitlab::Git::Repository::NoRepository
+      rescue Gitlab::Git::Repository::NoRepository
         # Even if the above `#exists?` check passes these errors might still
         # occur (for example because of a non-existing HEAD). We want to
         # gracefully handle this and not cache anything

app/views/admin/dashboard/index.html.haml

@@ -158,7 +158,6 @@
                 GitLab Pages
                 %span.pull-right
                   = Gitlab::Pages::VERSION
-
             - if Gitlab::Geo.enabled?
               %p
                 = link_to 'Geo', admin_geo_nodes_path
@@ -167,7 +166,6 @@
                     = Gitlab::Geo.current_node.primary ? 'Primary node' : 'Secondary node'
                   - else
                     Undefined
-
             %p
               Ruby
               %span.pull-right

changelogs/unreleased/sh-fix-events-collection.yml

+---
+title: Fix not all events being shown in group dashboard
+merge_request:
+author:
+type: fixed

changelogs/unreleased/sh-fix-project-members-api-perf.yml

+---
+title: Remove N+1 queries with /projects/:project_id/{access_requests,members} API
+  endpoints
+merge_request:
+author:
+type: performance

config/initializers/ar5_pg_10_support.rb

 raise "Vendored ActiveRecord 5 code! Delete #{__FILE__}!" if ActiveRecord::VERSION::MAJOR >= 5
 
-require 'active_record/connection_adapters/postgresql_adapter'
-require 'active_record/connection_adapters/postgresql/schema_statements'
-
-#
-# Monkey-patch the refused Rails 4.2 patch at https://github.com/rails/rails/pull/31330
-#
-# Updates sequence logic to support PostgreSQL 10.
-#
-# rubocop:disable all
-module ActiveRecord
+if Gitlab::Database.postgresql?
+  require 'active_record/connection_adapters/postgresql_adapter'
+  require 'active_record/connection_adapters/postgresql/schema_statements'
+
+  #
+  # Monkey-patch the refused Rails 4.2 patch at https://github.com/rails/rails/pull/31330
+  #
+  # Updates sequence logic to support PostgreSQL 10.
+  #
+  # rubocop:disable all
+  module ActiveRecord
     module ConnectionAdapters
 
       # We need #postgresql_version to be public as in ActiveRecord 5 for seed_fu
@@ -53,5 +54,6 @@ module ActiveRecord
         end
       end
     end
+  end
+  # rubocop:enable all
 end
-# rubocop:enable all

config/initializers/peek.rb

@@ -7,10 +7,12 @@ if Gitlab::Database.mysql?
   require 'peek-mysql2'
   PEEK_DB_CLIENT = ::Mysql2::Client
   PEEK_DB_VIEW = Peek::Views::Mysql2
-else
+elsif Gitlab::Database.postgresql?
   require 'peek-pg'
   PEEK_DB_CLIENT = ::PG::Connection
   PEEK_DB_VIEW = Peek::Views::PG
+else
+  raise "Unsupported database adapter for peek!"
 end
 
 Peek.into PEEK_DB_VIEW

doc/api/repositories.md

@@ -114,6 +114,7 @@ Parameters:
 
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
 - `sha` (optional) - The commit SHA to download. A tag, branch reference or sha can be used. This defaults to the tip of the default branch if not specified
+- `format` (optional) - The archive format. Default is `tar.gz`. Options are `tar.gz`, `tar.bz2`, `tbz`, `tbz2`, `tb2`, `bz2`, `tar`, `zip`
 
 ## Compare branches, tags or commits
 

lib/api/access_requests.rb

@@ -24,7 +24,7 @@ module API
           access_requesters = AccessRequestsFinder.new(source).execute!(current_user)
           access_requesters = paginate(access_requesters.includes(:user))
 
-          present access_requesters.map(&:user), with: Entities::AccessRequester, source: source
+          present access_requesters, with: Entities::AccessRequester
         end
 
         desc "Requests access for the authenticated user to a #{source_type}." do
@@ -36,7 +36,7 @@ module API
           access_requester = source.request_access(current_user)
 
           if access_requester.persisted?
-            present access_requester.user, with: Entities::AccessRequester, access_requester: access_requester
+            present access_requester, with: Entities::AccessRequester
           else
             render_validation_error!(access_requester)
           end
@@ -56,7 +56,7 @@ module API
           member = ::Members::ApproveAccessRequestService.new(source, current_user, declared_params).execute
 
           status :created
-          present member.user, with: Entities::Member, member: member
+          present member, with: Entities::Member
         end
 
         desc 'Denies an access request for the given user.' do

lib/api/entities.rb

@@ -219,22 +219,15 @@ module API
       expose :build_artifacts_size, as: :job_artifacts_size
     end
 
-    class Member < UserBasic
-      expose :access_level do |user, options|
-        member = options[:member] || options[:source].members.find_by(user_id: user.id)
-        member.access_level
-      end
-      expose :expires_at do |user, options|
-        member = options[:member] || options[:source].members.find_by(user_id: user.id)
-        member.expires_at
-      end
+    class Member < Grape::Entity
+      expose :user, merge: true, using: UserBasic
+      expose :access_level
+      expose :expires_at
     end
 
-    class AccessRequester < UserBasic
-      expose :requested_at do |user, options|
-        access_requester = options[:access_requester] || options[:source].requesters.find_by(user_id: user.id)
-        access_requester.requested_at
-      end
+    class AccessRequester < Grape::Entity
+      expose :user, merge: true, using: UserBasic
+      expose :requested_at
     end
 
     class LdapGroupLink < Grape::Entity

lib/api/members.rb

@@ -21,10 +21,11 @@ module API
         get ":id/members" do
           source = find_source(source_type, params[:id])
 
-          users = source.users
-          users = users.merge(User.search(params[:query])) if params[:query].present?
+          members = source.members.where.not(user_id: nil).includes(:user)
+          members = members.joins(:user).merge(User.search(params[:query])) if params[:query].present?
+          members = paginate(members)
 
-          present paginate(users), with: Entities::Member, source: source
+          present members, with: Entities::Member
         end
 
         desc 'Gets a member of a group or project.' do
@@ -39,7 +40,7 @@ module API
           members = source.members
           member = members.find_by!(user_id: params[:user_id])
 
-          present member.user, with: Entities::Member, member: member
+          present member, with: Entities::Member
         end
 
         desc 'Adds a member to a group or project.' do
@@ -62,7 +63,7 @@ module API
           if !member
             not_allowed! # This currently can only be reached in EE
           elsif member.persisted? && member.valid?
-            present member.user, with: Entities::Member, member: member
+            present member, with: Entities::Member
           else
             render_validation_error!(member)
           end
@@ -83,7 +84,7 @@ module API
           member = source.members.find_by!(user_id: params.delete(:user_id))
 
           if member.update_attributes(declared_params(include_missing: false))
-            present member.user, with: Entities::Member, member: member
+            present member, with: Entities::Member
           else
             render_validation_error!(member)
           end

lib/api/v3/members.rb

@@ -22,10 +22,11 @@ module API
           get ":id/members" do
             source = find_source(source_type, params[:id])
 
-            users = source.users
-            users = users.merge(User.search(params[:query])) if params[:query].present?
+            members = source.members.where.not(user_id: nil).includes(:user)
+            members = members.joins(:user).merge(User.search(params[:query])) if params[:query].present?
+            members = paginate(members)
 
-            present paginate(users), with: ::API::Entities::Member, source: source
+            present members, with: ::API::Entities::Member
           end
 
           desc 'Gets a member of a group or project.' do
@@ -40,7 +41,7 @@ module API
             members = source.members
             member = members.find_by!(user_id: params[:user_id])
 
-            present member.user, with: ::API::Entities::Member, member: member
+            present member, with: ::API::Entities::Member
           end
 
           desc 'Adds a member to a group or project.' do
@@ -69,7 +70,7 @@ module API
             end
 
             if member.persisted? && member.valid?
-              present member.user, with: ::API::Entities::Member, member: member
+              present member, with: ::API::Entities::Member
             else
               # This is to ensure back-compatibility but 400 behavior should be used
               # for all validation errors in 9.0!
@@ -93,7 +94,7 @@ module API
             member = source.members.find_by!(user_id: params.delete(:user_id))
 
             if member.update_attributes(declared_params(include_missing: false))
-              present member.user, with: ::API::Entities::Member, member: member
+              present member, with: ::API::Entities::Member
             else
               # This is to ensure back-compatibility but 400 behavior should be used
               # for all validation errors in 9.0!
@@ -125,7 +126,7 @@ module API
             else
               ::Members::DestroyService.new(source, current_user, declared_params).execute
 
-              present member.user, with: ::API::Entities::Member, member: member
+              present member, with: ::API::Entities::Member
             end
           end
         end

lib/gitlab/git/repository.rb

@@ -888,16 +888,12 @@ module Gitlab
       end
 
       def delete_refs(*ref_names)
-        instructions = ref_names.map do |ref|
-          "delete #{ref}\x00\x00"
-        end
-
-        message, status = run_git(%w[update-ref --stdin -z]) do |stdin|
-          stdin.write(instructions.join)
+        gitaly_migrate(:delete_refs) do |is_enabled|
+          if is_enabled
+            gitaly_delete_refs(*ref_names)
+          else
+            git_delete_refs(*ref_names)
           end
-
-        unless status.zero?
-          raise GitError.new("Could not delete refs #{ref_names}: #{message}")
         end
       end
 
@@ -1390,6 +1386,16 @@ module Gitlab
         run_git(args).first.scrub.split(/^--$/)
       end
 
+      def can_be_merged?(source_sha, target_branch)
+        gitaly_migrate(:can_be_merged) do |is_enabled|
+          if is_enabled
+            gitaly_can_be_merged?(source_sha, find_branch(target_branch, true).target)
+          else
+            rugged_can_be_merged?(source_sha, target_branch)
+          end
+        end
+      end
+
       def search_files_by_name(query, ref)
         safe_query = Regexp.escape(query.sub(/^\/*/, ""))
 
@@ -2204,6 +2210,24 @@ module Gitlab
         remote_update(remote_name, url: url)
       end
 
+      def git_delete_refs(*ref_names)
+        instructions = ref_names.map do |ref|
+          "delete #{ref}\x00\x00"
+        end
+
+        message, status = run_git(%w[update-ref --stdin -z]) do |stdin|
+          stdin.write(instructions.join)
+        end
+
+        unless status.zero?
+          raise GitError.new("Could not delete refs #{ref_names}: #{message}")
+        end
+      end
+
+      def gitaly_delete_refs(*ref_names)
+        gitaly_ref_client.delete_refs(refs: ref_names)
+      end
+
       def rugged_remove_remote(remote_name)
         # When a remote is deleted all its remote refs are deleted too, but in
         # the case of mirrors we map its refs (that would usualy go under
@@ -2266,6 +2290,14 @@ module Gitlab
         run_git(['fetch', remote_name], env: env).last.zero?
       end
 
+      def gitaly_can_be_merged?(their_commit, our_commit)
+        !gitaly_conflicts_client(our_commit, their_commit).conflicts?
+      end
+
+      def rugged_can_be_merged?(their_commit, our_commit)
+        !rugged.merge_commits(our_commit, their_commit).conflicts?
+      end
+
       def gitlab_projects_error
         raise CommandError, @gitlab_projects.output
       end

lib/gitlab/git/tree.rb

@@ -83,6 +83,8 @@ module Gitlab
               commit_id: sha
             )
           end
+        rescue Rugged::ReferenceError
+          []
         end
       end
 

lib/gitlab/gitaly_client/operation_service.rb

@@ -103,7 +103,13 @@ module Gitlab
 
         request_enum.push(Gitaly::UserMergeBranchRequest.new(apply: true))
 
-        branch_update = response_enum.next.branch_update
+        second_response = response_enum.next
+
+        if second_response.pre_receive_error.present?
+          raise Gitlab::Git::HooksService::PreReceiveError, second_response.pre_receive_error
+        end
+
+        branch_update = second_response.branch_update
         return if branch_update.nil?
         raise Gitlab::Git::CommitError.new('failed to apply merge to branch') unless branch_update.commit_id.present?
 

lib/gitlab/gitaly_client/ref_service.rb

@@ -133,13 +133,16 @@ module Gitlab
         GitalyClient.call(@repository.storage, :ref_service, :delete_branch, request)
       end
 
-      def delete_refs(except_with_prefixes:)
+      def delete_refs(refs: [], except_with_prefixes: [])
         request = Gitaly::DeleteRefsRequest.new(
           repository: @gitaly_repo,
-          except_with_prefix: except_with_prefixes
+          refs: refs.map { |r| encode_binary(r) },
+          except_with_prefix: except_with_prefixes.map { |r| encode_binary(r) }
         )
 
-        GitalyClient.call(@repository.storage, :ref_service, :delete_refs, request)
+        response = GitalyClient.call(@repository.storage, :ref_service, :delete_refs, request)
+
+        raise Gitlab::Git::Repository::GitError, response.git_error if response.git_error.present?
       end
 
       private

locale/gitlab.pot

@@ -8,8 +8,8 @@ msgid ""
 msgstr ""
 "Project-Id-Version: gitlab 1.0.0\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2018-01-31 09:31+0100\n"
-"PO-Revision-Date: 2018-01-31 09:31+0100\n"
+"POT-Creation-Date: 2018-01-31 17:04+0100\n"
+"PO-Revision-Date: 2018-01-31 17:04+0100\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
 "Language: \n"

qa/README.md

@@ -34,6 +34,9 @@ You can use GitLab QA to exercise tests on any live instance! For example, the
 following call would login to a local [GDK] instance and run all specs in
 `qa/specs/features`:
 
+First, `cd` into the `$gdk/gitlab/qa` directory.
+The `bin/qa` script expects you to be in the `qa` folder of the app.
+
 ```
 bin/qa Test::Instance http://localhost:3000
 ```

qa/qa.rb

@@ -155,6 +155,13 @@ module QA
       autoload :Main, 'qa/page/mattermost/main'
       autoload :Login, 'qa/page/mattermost/login'
     end
+
+    ##
+    # Classes describing components that are used by several pages.
+    #
+    module Component
+      autoload :Dropzone, 'qa/page/component/dropzone'
+    end
   end
 
   ##

qa/qa/page/base.rb

@@ -97,21 +97,6 @@ module QA
         views.map(&:errors).flatten
       end
 
-      # Not tested and not expected to work with multiple dropzones
-      # instantiated on one page because there is no distinguishing
-      # attribute per dropzone file field.
-      def attach_file_to_dropzone(attachment, dropzone_form_container)
-        filename = File.basename(attachment)
-
-        field_style = { visibility: 'visible', height: '', width: '' }
-        attach_file(attachment, class: 'dz-hidden-input', make_visible: field_style)
-
-        # Wait for link to be appended to dropzone text
-        wait(reload: false) do
-          find("#{dropzone_form_container} textarea").value.match(filename)
-        end
-      end
-
       class DSL
         attr_reader :views
 

qa/qa/page/component/dropzone.rb

+module QA
+  module Page
+    module Component
+      class Dropzone
+        attr_reader :page, :container
+
+        def initialize(page, container)
+          @page = page
+          @container = container
+        end
+
+        # Not tested and not expected to work with multiple dropzones
+        # instantiated on one page because there is no distinguishing
+        # attribute per dropzone file field.
+        def attach_file(attachment)
+          filename = File.basename(attachment)
+
+          field_style = { visibility: 'visible', height: '', width: '' }
+          page.attach_file(attachment, class: 'dz-hidden-input', make_visible: field_style)
+
+          # Wait for link to be appended to dropzone text
+          page.wait(reload: false) do
+            page.find("#{container} textarea").value.match(filename)
+          end
+        end
+      end
+    end
+  end
+end

qa/qa/page/project/issue/show.rb

@@ -23,10 +23,13 @@ module QA
 
           # Adds a comment to an issue
           # attachment option should be an absolute path
-          def comment(text, attachment:)
+          def comment(text, attachment: nil)
             fill_in(with: text, name: 'note[note]')
 
-            attach_file_to_dropzone(attachment, '.new-note') if attachment
+            unless attachment.nil?
+              QA::Page::Component::Dropzone.new(page, '.new-note')
+                .attach_file(attachment)
+            end
 
             click_on 'Comment'
           end

spec/controllers/groups_controller_spec.rb

@@ -85,6 +85,30 @@ describe GroupsController do
     end
   end
 
+  describe 'GET #activity' do
+    render_views
+
+    before do
+      sign_in(user)
+      project
+    end
+
+    context 'as json' do
+      it 'includes all projects in event feed' do
+        3.times do
+          project = create(:project, group: group)
+          create(:event, project: project)
+        end
+
+        get :activity, id: group.to_param, format: :json
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['count']).to eq(3)
+        expect(assigns(:projects).limit_value).to be_nil
+      end
+    end
+  end
+
   describe 'POST #create' do
     it 'allows creating a group' do
       sign_in(user)

spec/javascripts/job_spec.js

@@ -58,8 +58,7 @@ describe('Job', () => {
       it('updates the build trace on an interval', function () {
         const deferred1 = $.Deferred();
         const deferred2 = $.Deferred();
-        const deferred3 = $.Deferred();
-        spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise(), deferred3.promise());
+        spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise());
         spyOn(urlUtils, 'visitUrl');
 
         deferred1.resolve({
@@ -70,9 +69,7 @@ describe('Job', () => {
           complete: false,
         });
 
-        deferred2.resolve();
-
-        deferred3.resolve({
+        deferred2.resolve({
           html: '<span>More</span>',
           status: 'running',
           state: 'finalstate',
@@ -94,9 +91,8 @@ describe('Job', () => {
       it('replaces the entire build trace', () => {
         const deferred1 = $.Deferred();
         const deferred2 = $.Deferred();
-        const deferred3 = $.Deferred();
 
-        spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise(), deferred3.promise());
+        spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise());
 
         spyOn(urlUtils, 'visitUrl');
 
@@ -107,9 +103,7 @@ describe('Job', () => {
           complete: false,
         });
 
-        deferred2.resolve();
-
-        deferred3.resolve({
+        deferred2.resolve({
           html: '<span>Different</span>',
           status: 'running',
           append: false,
@@ -170,9 +164,8 @@ describe('Job', () => {
         it('shows incremented size', () => {
           const deferred1 = $.Deferred();
           const deferred2 = $.Deferred();
-          const deferred3 = $.Deferred();
 
-          spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise(), deferred3.promise());
+          spyOn($, 'ajax').and.returnValues(deferred1.promise(), deferred2.promise());
 
           spyOn(urlUtils, 'visitUrl');
 
@@ -184,8 +177,6 @@ describe('Job', () => {
             total: 100,
           });
 
-          deferred2.resolve();
-
           this.job = new Job();
 
           expect(
@@ -194,7 +185,7 @@ describe('Job', () => {
 
           jasmine.clock().tick(4001);
 
-          deferred3.resolve({
+          deferred2.resolve({
             html: '<span>Update</span>',
             status: 'success',
             append: true,

spec/javascripts/lib/utils/common_utils_spec.js

 /* eslint-disable promise/catch-or-return */
 
 import * as commonUtils from '~/lib/utils/common_utils';
+import axios from '~/lib/utils/axios_utils';
+import MockAdapter from 'axios-mock-adapter';
 
 describe('common_utils', () => {
   describe('parseUrl', () => {
@@ -413,37 +415,48 @@ describe('common_utils', () => {
 
   describe('setCiStatusFavicon', () => {
     const BUILD_URL = `${gl.TEST_HOST}/frontend-fixtures/builds-project/-/jobs/1/status.json`;
+    let mock;
 
     beforeEach(() => {
       const favicon = document.createElement('link');
       favicon.setAttribute('id', 'favicon');
       document.body.appendChild(favicon);
+      mock = new MockAdapter(axios);
     });
 
     afterEach(() => {
+      mock.restore();
       document.body.removeChild(document.getElementById('favicon'));
     });
 
-    it('should reset favicon in case of error', () => {
+    it('should reset favicon in case of error', (done) => {
+      mock.onGet(BUILD_URL).networkError();
+
+      commonUtils.setCiStatusFavicon(BUILD_URL)
+        .then(() => {
           const favicon = document.getElementById('favicon');
-      spyOn($, 'ajax').and.callFake(function (options) {
-        options.error();
           expect(favicon.getAttribute('href')).toEqual('null');
+          done();
+        })
+        // Error is already caught in catch() block of setCiStatusFavicon,
+        // It won't throw another error for us to catch
+        .catch(done.fail);
     });
 
-      commonUtils.setCiStatusFavicon(BUILD_URL);
-    });
-
-    it('should set page favicon to CI status favicon based on provided status', () => {
+    it('should set page favicon to CI status favicon based on provided status', (done) => {
       const FAVICON_PATH = '//icon_status_success';
-      const favicon = document.getElementById('favicon');
 
-      spyOn($, 'ajax').and.callFake(function (options) {
-        options.success({ favicon: FAVICON_PATH });
-        expect(favicon.getAttribute('href')).toEqual(FAVICON_PATH);
+      mock.onGet(BUILD_URL).reply(200, {
+        favicon: FAVICON_PATH,
       });
 
-      commonUtils.setCiStatusFavicon(BUILD_URL);
+      commonUtils.setCiStatusFavicon(BUILD_URL)
+        .then(() => {
+          const favicon = document.getElementById('favicon');
+          expect(favicon.getAttribute('href')).toEqual(FAVICON_PATH);
+          done();
+        })
+        .catch(done.fail);
     });
   });
 

spec/lib/gitlab/git/repository_spec.rb

@@ -562,35 +562,39 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#delete_refs' do
-    before(:all) do
-      @repo = Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '')
+    shared_examples 'deleting refs' do
+      let(:repo) { Gitlab::Git::Repository.new('default', TEST_MUTABLE_REPO_PATH, '') }
+
+      after do
+        ensure_seeds
       end
 
       it 'deletes the ref' do
-      @repo.delete_refs('refs/heads/feature')
+        repo.delete_refs('refs/heads/feature')
 
-      expect(@repo.rugged.references['refs/heads/feature']).to be_nil
+        expect(repo.rugged.references['refs/heads/feature']).to be_nil
       end
 
       it 'deletes all refs' do
         refs = %w[refs/heads/wip refs/tags/v1.1.0]
-      @repo.delete_refs(*refs)
+        repo.delete_refs(*refs)
 
         refs.each do |ref|
-        expect(@repo.rugged.references[ref]).to be_nil
+          expect(repo.rugged.references[ref]).to be_nil
         end
       end
 
       it 'raises an error if it failed' do
-      expect(@repo).to receive(:popen).and_return(['Error', 1])
+        expect { repo.delete_refs('refs\heads\fix') }.to raise_error(Gitlab::Git::Repository::GitError)
+      end
+    end
 
-      expect do
-        @repo.delete_refs('refs/heads/fix')
-      end.to raise_error(Gitlab::Git::Repository::GitError)
+    context 'when Gitaly delete_refs feature is enabled' do
+      it_behaves_like 'deleting refs'
     end
 
-    after(:all) do
-      ensure_seeds
+    context 'when Gitaly delete_refs feature is disabled', :disable_gitaly do
+      it_behaves_like 'deleting refs'
     end
   end
 

spec/lib/gitlab/git/tree_spec.rb

@@ -80,22 +80,18 @@ describe Gitlab::Git::Tree, seed_helper: true do
   end
 
   describe '#where' do
-    context 'with gitaly disabled' do
-      before do
-        allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_return(false)
-      end
-
-      it 'calls #tree_entries_from_rugged' do
-        expect(described_class).to receive(:tree_entries_from_rugged)
-
-        described_class.where(repository, SeedRepo::Commit::ID, '/')
+    shared_examples '#where' do
+      it 'returns an empty array when called with an invalid ref' do
+        expect(described_class.where(repository, 'foobar-does-not-exist')).to eq([])
       end
     end
 
-    it 'gets the tree entries from GitalyClient' do
-      expect_any_instance_of(Gitlab::GitalyClient::CommitService).to receive(:tree_entries)
+    context 'with gitaly' do
+      it_behaves_like '#where'
+    end
 
-      described_class.where(repository, SeedRepo::Commit::ID, '/')
+    context 'without gitaly', :skip_gitaly_mock do
+      it_behaves_like '#where'
     end
   end
 end

spec/lib/gitlab/gitaly_client/ref_service_spec.rb

@@ -112,7 +112,7 @@ describe Gitlab::GitalyClient::RefService do
       expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:delete_refs)
         .with(gitaly_request_with_params(except_with_prefix: prefixes), kind_of(Hash))
-        .and_return(double('delete_refs_response'))
+        .and_return(double('delete_refs_response', git_error: ""))
 
       client.delete_refs(except_with_prefixes: prefixes)
     end

spec/models/repository_spec.rb

@@ -804,8 +804,7 @@ describe Repository do
         user, 'LICENSE', 'Copyright!',
         message: 'Add LICENSE', branch_name: 'master')
 
-      allow(repository).to receive(:file_on_head)
-        .and_raise(Rugged::ReferenceError)
+      allow(repository).to receive(:root_ref).and_raise(Gitlab::Git::Repository::NoRepository)
 
       expect(repository.license_blob).to be_nil
     end
@@ -917,7 +916,7 @@ describe Repository do
     end
 
     it 'returns nil for empty repository' do
-      allow(repository).to receive(:file_on_head).and_raise(Rugged::ReferenceError)
+      allow(repository).to receive(:root_ref).and_raise(Gitlab::Git::Repository::NoRepository)
       expect(repository.gitlab_ci_yml).to be_nil
     end
   end
@@ -2011,8 +2010,7 @@ describe Repository do
 
   describe '#avatar' do
     it 'returns nil if repo does not exist' do
-      expect(repository).to receive(:file_on_head)
-        .and_raise(Rugged::ReferenceError)
+      allow(repository).to receive(:root_ref).and_raise(Gitlab::Git::Repository::NoRepository)
 
       expect(repository.avatar).to eq(nil)
     end

spec/requests/api/members_spec.rb

@@ -44,6 +44,21 @@ describe API::Members do
         end
       end
 
+      it 'avoids N+1 queries' do
+        # Establish baseline
+        get api("/#{source_type.pluralize}/#{source.id}/members", master)
+
+        control = ActiveRecord::QueryRecorder.new do
+          get api("/#{source_type.pluralize}/#{source.id}/members", master)
+        end
+
+        project.add_developer(create(:user))
+
+        expect do
+          get api("/#{source_type.pluralize}/#{source.id}/members", master)
+        end.not_to exceed_query_limit(control)
+      end
+
       it 'does not return invitees' do
         create(:"#{source_type}_member", invite_token: '123', invite_email: 'test@abc.com', source: source, user: nil)