Major refactor of limit check functionality, better handling of forks, and syntax fixes.

app/controllers/projects/merge_requests_controller.rb

@@ -320,12 +320,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     return access_denied! unless @merge_request.can_be_merged_by?(current_user)
     return render_404 unless @merge_request.approved?
 
-    # user is not able to merge if project is above size limit
-    if @merge_request.target_project.above_size_limit?
-      @status = :size_limit_reached
-      return
-    end
-
     # Disable the CI check if merge_when_build_succeeds is enabled since we have
     # to wait until CI completes to know
     unless @merge_request.mergeable?(skip_ci_check: merge_when_build_succeeds_active?)

app/helpers/groups_helper.rb

@@ -41,6 +41,12 @@ module GroupsHelper
     end
   end
 
+  def size_limit_message_for_group(group)
+    show_lfs = group.lfs_enabled? ? 'and their respective LFS files' : ''
+
+    "Repositories within this group #{show_lfs} will be restricted to this maximum size. Can be overridden inside each project. 0 for unlimited."
+  end
+
   def group_lfs_status(group)
     status = group.lfs_enabled? ? 'enabled' : 'disabled'
 

app/helpers/lfs_helper.rb

 module LfsHelper
+  include Gitlab::Routing.url_helpers
+
   def require_lfs_enabled!
     return if Gitlab.config.lfs.enabled
 
@@ -16,7 +18,7 @@ module LfsHelper
     return if upload_request? && lfs_upload_access?
 
     if project.public? || (user && user.can?(:read_project, project))
-      if project.above_size_limit? || objects_exceeded_limit?
+      if project.above_size_limit? || objects_exceed_repo_limit?
         render_size_error
       else
         render_lfs_forbidden
@@ -41,25 +43,18 @@ module LfsHelper
 
   def objects_exceed_repo_limit?
     return false unless project.size_limit_enabled?
+    return @limit_exceeded if defined?(@limit_exceeded)
 
-    objects_size = 0
-
-    objects.each do |object|
-      objects_size += object[:size]
-    end
-
-    @limit_exceeded = true if (project.aggregated_repository_size + objects_size.to_mb) > project.repo_size_limit
-  end
+    size_of_objects = objects.sum { |o| o[:size] }
 
-  def objects_exceeded_limit?
-    @limit_exceeded ||= false
+    @limit_exceeded = (project.repository_and_lfs_size + size_of_objects.to_mb) > project.actual_size_limit
   end
 
   def render_lfs_forbidden
     render(
       json: {
         message: 'Access forbidden. Check your access level.',
-        documentation_url: "#{Gitlab.config.gitlab.url}/help",
+        documentation_url: help_url,
       },
       content_type: "application/vnd.git-lfs+json",
       status: 403
@@ -70,7 +65,7 @@ module LfsHelper
     render(
       json: {
         message: 'Not found.',
-        documentation_url: "#{Gitlab.config.gitlab.url}/help",
+        documentation_url: help_url,
       },
       content_type: "application/vnd.git-lfs+json",
       status: 404
@@ -80,8 +75,8 @@ module LfsHelper
   def render_size_error
     render(
       json: {
-        message: 'This repository has exceeded its storage limit. Please contact your GitLab admin.',
-        documentation_url: "#{Gitlab.config.gitlab.url}/help",
+        message: Gitlab::RepositorySizeError.new(project).push_error,
+        documentation_url: help_url,
       },
       content_type: "application/vnd.git-lfs+json",
       status: 406

app/helpers/projects_helper.rb

@@ -214,6 +214,12 @@ module ProjectsHelper
     end
   end
 
+  def size_limit_message(project)
+    show_lfs = project.lfs_enabled? ? 'including files in LFS' : ''
+
+    "The total size of this project's repository #{show_lfs} will be limited to this size. 0 for unlimited."
+  end
+
   def git_user_name
     if current_user
       current_user.name
@@ -231,12 +237,12 @@ module ProjectsHelper
   end
 
   def repository_size(project = @project)
-    size_in_bytes = project.aggregated_repository_size * 1.megabyte
-    limit_in_bytes = project.repo_size_limit * 1.megabyte
+    size_in_bytes = project.repository_and_lfs_size * 1.megabyte
+    limit_in_bytes = project.actual_size_limit * 1.megabyte
 
-    limit_text = limit_in_bytes.zero? ? 'Unlimited' : number_to_human_size(limit_in_bytes, delimiter: ',', precision: 2)
+    limit_text = limit_in_bytes.zero? ? '' : "/#{number_to_human_size(limit_in_bytes, delimiter: ',', precision: 2)}"
 
-    "#{number_to_human_size(size_in_bytes, delimiter: ',', precision: 2)}/#{limit_text}"
+    "#{number_to_human_size(size_in_bytes, delimiter: ',', precision: 2)}#{limit_text}"
   end
 
   def default_url_to_repo(project = @project)

app/models/group.rb

@@ -202,7 +202,7 @@ class Group < Namespace
     system_hook_service.execute_hooks_for(self, :destroy)
   end
 
-  def repo_size_limit
+  def actual_size_limit
     return current_application_settings.repository_size_limit if repository_size_limit.nil?
 
     repository_size_limit

app/models/merge_request.rb

@@ -289,10 +289,6 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  def is_valid?
-    !(target_project.above_size_limit? || target_project.blank? || source_branch.blank?)
-  end
-
   def branch_merge_base_sha
     branch_merge_base_commit.try(:sha)
   end

app/models/namespace.rb

@@ -147,7 +147,7 @@ class Namespace < ActiveRecord::Base
     Gitlab.config.lfs.enabled
   end
 
-  def repo_size_limit
+  def actual_size_limit
     current_application_settings.repository_size_limit
   end
 

app/models/project.rb

@@ -1540,28 +1540,32 @@ class Project < ActiveRecord::Base
     Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) }
   end
 
-  def aggregated_repository_size
+  def repository_and_lfs_size
     repository_size + lfs_objects.sum(:size).to_i.to_mb
   end
 
   def above_size_limit?
-    return false if repo_size_limit == 0
+    return false unless size_limit_enabled?
 
-    aggregated_repository_size > repo_size_limit
+    repository_and_lfs_size > actual_size_limit
   end
 
   def size_to_remove
-    aggregated_repository_size - repo_size_limit
+    repository_and_lfs_size - actual_size_limit
   end
 
-  def repo_size_limit
-    return namespace.repo_size_limit if repository_size_limit.nil?
+  def actual_size_limit
+    return namespace.actual_size_limit if repository_size_limit.nil?
 
     repository_size_limit
   end
 
   def size_limit_enabled?
-    repo_size_limit != 0
+    actual_size_limit != 0
+  end
+
+  def changes_will_exceed_size_limit?(size_mb)
+    size_limit_enabled? && (size_mb > actual_size_limit || size_mb + repository_and_lfs_size > actual_size_limit)
   end
 
   private

app/services/files/base_service.rb

@@ -34,10 +34,6 @@ module Files
       error(ex.message)
     end
 
-    def size_limit_error_message
-      "Your changes could not be committed, because this repository has exceeded its size limit of #{project.repo_size_limit}MB by #{project.size_to_remove}MB"
-    end
-
     private
 
     def different_branch?
@@ -51,6 +47,10 @@ module Files
     def validate
       allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
 
+      if project.above_size_limit?
+        raise_error(Gitlab::RepositorySizeError.new(project).commit_error)
+      end
+
       unless allowed
         raise_error("You are not allowed to push into this branch")
       end

app/services/files/create_dir_service.rb

@@ -9,10 +9,6 @@ module Files
     def validate
       super
 
-      if project.above_size_limit?
-        raise_error(size_limit_error_message)
-      end
-
       unless @file_path =~ Gitlab::Regex.file_path_regex
         raise_error(
           'Your changes could not be committed, because the file path ' +

app/services/files/create_service.rb

@@ -9,10 +9,6 @@ module Files
     def validate
       super
 
-      if project.above_size_limit?
-        raise_error(size_limit_error_message)
-      end
-
       if @file_path =~ Gitlab::Regex.directory_traversal_regex
         raise_error(
           'Your changes could not be committed, because the file name ' +

app/services/files/update_service.rb

@@ -16,10 +16,6 @@ module Files
     def validate
       super
 
-      if project.above_size_limit?
-        raise_error(size_limit_error_message)
-      end
-
       if file_has_changed?
         raise FileChangedError.new("You are attempting to update a file that has changed since you started editing it.")
       end

app/services/merge_requests/base_service.rb

@@ -35,10 +35,6 @@ module MergeRequests
       end
     end
 
-    def render_size_limit_message(project)
-      "The target's repository size (#{project.aggregated_repository_size}MB) exceeds the limit of #{project.repo_size_limit}MB by #{project.size_to_remove}MB"
-    end
-
     private
 
     def filter_params

app/services/merge_requests/build_service.rb

@@ -13,23 +13,15 @@ module MergeRequests
       merge_request.target_project ||= (project.forked_from_project || project)
       merge_request.target_branch ||= merge_request.target_project.default_branch
 
-      if merge_request.target_project.above_size_limit?
-        message = render_size_limit_message(merge_request.target_project)
-
-        merge_request.errors.add(:base, message)
-      end
-
       if merge_request.target_branch.blank? || merge_request.source_branch.blank?
         message =
           if params[:source_branch] || params[:target_branch]
             "You must select source and target branch"
           end
 
-        merge_request.errors.add(:base, message) unless message.nil?
+        return build_failed(merge_request, message)
       end
 
-      return build_failed(merge_request) unless merge_request.is_valid?
-
       compare = CompareService.new.execute(
         merge_request.source_project,
         merge_request.source_branch,
@@ -105,7 +97,8 @@ module MergeRequests
       merge_request
     end
 
-    def build_failed(merge_request)
+    def build_failed(merge_request, message)
+      merge_request.errors.add(:base, message) unless message.nil?
       merge_request.compare_commits = []
       merge_request.can_be_created = false
       merge_request

app/services/merge_requests/merge_service.rb

@@ -18,6 +18,12 @@ module MergeRequests
 
       return error('Merge request is not mergeable') unless @merge_request.mergeable?
 
+      if @merge_request.target_project.above_size_limit?
+        message = Gitlab::RepositorySizeError.new(@merge_request.target_project).merge_error
+        @merge_request.update(merge_error: message)
+        return error(message)
+      end
+
       merge_request.in_locked_state do
         if commit
           after_merge

app/views/groups/_repository_size_limit_setting.html.haml

@@ -5,4 +5,4 @@
     .col-sm-10
       = f.number_field :repository_size_limit, class: 'form-control', min: 0
       %span.help-block#repository_size_limit_help_block
-        Repositories within this group will be restricted to this maximum size (includes LFS objects). Can be overridden inside each project. 0 for unlimited.
+        = size_limit_message_for_group(@group)

app/views/projects/edit.html.haml

@@ -40,7 +40,7 @@
               Repository size limit (MB)
             = f.number_field :repository_size_limit, class: 'form-control', min: 0
             %span.help-block#repository_size_limit_help_block
-              This project's repository will be restricted to this maximum size (includes LFS objects). 0 for unlimited.
+              = size_limit_message(@project)
 
         .form-group
           = render 'shared/allow_request_access', form: f

app/views/projects/merge_requests/merge.js.haml

@@ -11,9 +11,6 @@
 - when :sha_mismatch
   :plain
     $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/sha_mismatch'))}");
-- when :size_limit_reached
-  :plain
-    $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/size_limit_reached'))}");
 - else
   :plain
     $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}");

app/views/projects/merge_requests/widget/_open.html.haml

@@ -11,6 +11,8 @@
       = render 'projects/merge_requests/widget/open/geo'
     - if @project.archived?
       = render 'projects/merge_requests/widget/open/archived'
+    - elsif @project.above_size_limit?
+      = render 'projects/merge_requests/widget/open/size_limit_reached'
     - elsif @merge_request.commits.blank?
       = render 'projects/merge_requests/widget/open/nothing'
     - elsif @merge_request.branch_missing?

app/views/projects/merge_requests/widget/open/_size_limit_reached.html.haml

+- error_messages = Gitlab::RepositorySizeError.new(@project)
+
 %h4.size-limit-reached
   = icon("exclamation-triangle")
-  This repository has reached its size limit.
+  = error_messages.merge_error
 
 %p
-  Please contact your GitLab administrator for more information.
+  = error_messages.more_info_message

lib/gitlab/git_access.rb

@@ -70,8 +70,6 @@ module Gitlab
       return build_status_object(true) if git_annex_branch_sync?(changes)
 
       if user
-        return build_status_object(false, above_size_limit_message) if project.above_size_limit?
-
         user_push_access_check(changes)
       elsif deploy_key
         build_status_object(false, "Deploy keys are not allowed to push code.")
@@ -89,13 +87,11 @@ module Gitlab
     end
 
     def user_push_access_check(changes)
-      if changes.blank?
-        return build_status_object(true)
-      end
+      return build_status_object(true) if changes.blank?
 
-      unless project.repository.exists?
-        return build_status_object(false, "A repository for this project does not exist yet.")
-      end
+      return build_status_object(false, "A repository for this project does not exist yet.") unless project.repository.exists?
+
+      return build_status_object(false, Gitlab::RepositorySizeError.new(project).push_error) if project.above_size_limit?
 
       if ::License.block_changes?
         message = ::LicenseHelper.license_message(signed_in: true, is_admin: (user && user.is_admin?))
@@ -119,20 +115,19 @@ module Gitlab
         end
       end
 
-      if project.size_limit_enabled? && changes_above_limit(push_size_in_bytes.to_mb)
-        return build_status_object(false, will_go_over_limit_message)
+      if project.changes_will_exceed_size_limit?(push_size_in_bytes.to_mb)
+        return build_status_object(false, Gitlab::RepositorySizeError.new(project).new_changes_error)
       end
 
       build_status_object(true)
     end
 
     def delta_size_check(change, repo)
-      oldrev, newrev = change.values_at(:oldrev, :newrev)
       size_of_deltas = 0
 
       begin
-        tree_a = repo.lookup(oldrev)
-        tree_b = repo.lookup(newrev)
+        tree_a = repo.lookup(change[:oldrev])
+        tree_b = repo.lookup(change[:newrev])
         diff = tree_a.diff(tree_b)
 
         diff.each_delta do |d|
@@ -147,10 +142,6 @@ module Gitlab
       end
     end
 
-    def changes_above_limit(size_mb)
-      size_mb > project.repo_size_limit || size_mb + project.aggregated_repository_size > project.repo_size_limit
-    end
-
     def change_access_check(change)
       Checks::ChangeAccess.new(change, user_access: user_access, project: project).exec
     end
@@ -159,21 +150,6 @@ module Gitlab
       Gitlab::ProtocolAccess.allowed?(protocol)
     end
 
-    def above_size_limit_message
-      [
-        "This repository's size (#{project.aggregated_repository_size}MB) exceeds the limit of #{project.repo_size_limit}MB",
-        "GitLab: by #{project.size_to_remove}MB and as a result you are unable to push to it.",
-        "GitLab: Please contact your GitLab administrator for more information.",
-      ].join("\n") + "\n"
-    end
-
-    def will_go_over_limit_message
-      [
-        "Your push to this repository would cause it to exceed the limit of #{project.repo_size_limit}MB.",
-        "GitLab: As a result it has been rejected. Please contact your GitLab administrator for more information.",
-      ].join("\n") + "\n"
-    end
-
     def matching_merge_request?(newrev, branch_name)
       Checks::MatchingMergeRequest.new(newrev, branch_name, project).match?
     end

lib/gitlab/repository_size_error.rb

+module Gitlab
+  class RepositorySizeError < StandardError
+    include ActionView::Helpers
+
+    attr_reader :project
+
+    def initialize(project)
+      @project = project
+    end
+
+    def to_s
+      "The size of this repository (#{current_size}) exceeds the limit of #{limit} by #{size_to_remove}."
+    end
+
+    def commit_error
+      "Your changes could not be committed, #{base_message}"
+    end
+
+    def merge_error
+      "This merge request cannot be merged, #{base_message}"
+    end
+
+    def push_error
+      "Your push has been rejected, #{base_message}. #{more_info_message}"
+    end
+
+    def new_changes_error
+      "Your push to this repository would cause it to exceed the size limit of #{limit} so it has been rejected. #{more_info_message}"
+    end
+
+    def more_info_message
+      'Please contact your GitLab administrator for more information.'
+    end
+
+    private
+
+    def base_message
+      "because this repository has exceeded its size limit of #{limit} by #{size_to_remove}"
+    end
+
+    def current_size
+      format_number(project.repository_and_lfs_size)
+    end
+
+    def limit
+      format_number(project.actual_size_limit)
+    end
+
+    def size_to_remove
+      format_number(project.size_to_remove)
+    end
+
+    def format_number(number)
+      number_to_human_size(number * 1.megabyte, delimiter: ',', precision: 2)
+    end
+  end
+end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -355,18 +355,6 @@ describe Projects::MergeRequestsController do
       end
     end
 
-    context 'when the repository is above size limit' do
-      before do
-        allow_any_instance_of(Project).to receive(:above_size_limit?).and_return(true)
-
-        post :merge, base_params.merge(sha: merge_request.diff_head_sha)
-      end
-
-      it 'returns :size_limit_reached' do
-        expect(assigns(:status)).to eq(:size_limit_reached)
-      end
-    end
-
     context 'when the merge request is not mergeable' do
       before do
         merge_request.update_attributes(title: "WIP: #{merge_request.title}")

spec/models/ee/group_spec.rb

@@ -86,7 +86,7 @@ describe Group, models: true do
     end
   end
 
-  describe '#repo_size_limit' do
+  describe '#actual_size_limit' do
     let(:group) { build(:group) }
 
     before do
@@ -94,13 +94,13 @@ describe Group, models: true do
     end
 
     it 'returns the value set globally' do
-      expect(group.repo_size_limit).to eq(50)
+      expect(group.actual_size_limit).to eq(50)
     end
 
     it 'returns the value set locally' do
       group.update_attribute(:repository_size_limit, 75)
 
-      expect(group.repo_size_limit).to eq(75)
+      expect(group.actual_size_limit).to eq(75)
     end
   end
 end

spec/models/project_spec.rb

@@ -486,22 +486,22 @@ describe Project, models: true do
       allow_any_instance_of(ApplicationSetting).to receive(:repository_size_limit).and_return(50)
     end
 
-    describe '#repo_size_limit' do
+    describe '#actual_size_limit' do
       it 'returns the limit set in the application settings' do
-        expect(project.repo_size_limit).to eq(50)
+        expect(project.actual_size_limit).to eq(50)
       end
 
       it 'returns the value set in the group' do
         group = create(:group, repository_size_limit: 100)
         project.update_attribute(:namespace_id, group.id)
 
-        expect(project.repo_size_limit).to eq(100)
+        expect(project.actual_size_limit).to eq(100)
       end
 
       it 'returns the value set locally' do
         project.update_attribute(:repository_size_limit, 75)
 
-        expect(project.repo_size_limit).to eq(75)
+        expect(project.actual_size_limit).to eq(75)
       end
     end
 
@@ -521,7 +521,7 @@ describe Project, models: true do
 
     describe '#above_size_limit?' do
       it 'returns true when above the limit' do
-        allow(project).to receive(:aggregated_repository_size).and_return(100)
+        allow(project).to receive(:repository_and_lfs_size).and_return(100)
 
         expect(project.above_size_limit?).to be_truthy
       end
@@ -533,7 +533,7 @@ describe Project, models: true do
 
     describe '#size_to_remove' do
       it 'returns the correct value' do
-        allow(project).to receive(:aggregated_repository_size).and_return(100)
+        allow(project).to receive(:repository_and_lfs_size).and_return(100)
 
         expect(project.size_to_remove).to eq(50)
       end

spec/requests/lfs_http_spec.rb

@@ -549,11 +549,12 @@ describe 'Git LFS API and storage' do
 
             context 'and project will go over the limit' do
               let(:update_lfs_permissions) do
-                allow_any_instance_of(Project).to receive_messages(repo_size_limit: 145, size_limit_enabled?: true)
+                allow_any_instance_of(Project).to receive_messages(actual_size_limit: 145, size_limit_enabled?: true)
               end
 
               it 'responds with status 406' do
                 expect(response).to have_http_status(406)
+                expect(json_response['documentation_url']).to include('/help')
               end
             end
           end