Merge branch 'lint-rugged' into 'master'

Add a lint check to restrict use of Rugged

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

app/models/ci/pipeline.rb

@@ -524,7 +524,7 @@ module Ci
       return unless sha
 
       project.repository.gitlab_ci_yml_for(sha, ci_yaml_file_path)
-    rescue GRPC::NotFound, Rugged::ReferenceError, GRPC::Internal
+    rescue GRPC::NotFound, GRPC::Internal
       nil
     end
 

app/models/deployment.rb

@@ -45,14 +45,7 @@ class Deployment < ActiveRecord::Base
   def includes_commit?(commit)
     return false unless commit
 
-    # Before 8.10, deployments didn't have keep-around refs. Any deployment
-    # created before then could have a `sha` referring to a commit that no
-    # longer exists in the repository, so just ignore those.
-    begin
     project.repository.ancestor?(commit.id, sha)
-    rescue Rugged::OdbError
-      false
-    end
   end
 
   def update_merge_request_metrics!

app/workers/repository_check/single_repository_worker.rb

@@ -20,10 +20,7 @@ module RepositoryCheck
         # Historically some projects never had their wiki repos initialized;
         # this happens on project creation now. Let's initialize an empty repo
         # if it is not already there.
-        begin
         project.create_wiki
-        rescue Rugged::RepositoryError
-        end
 
         git_fsck(project.wiki.repository)
       else

config/initializers/rugged_use_gitlab_git_attributes.rb

-# We don't want to ever call Rugged::Repository#fetch_attributes, because it has
-# a lot of I/O overhead:
-# <https://gitlab.com/gitlab-org/gitlab_git/commit/340e111e040ae847b614d35b4d3173ec48329015>
-#
-# While we don't do this from within the GitLab source itself, the Linguist gem
-# has a dependency on Rugged and uses the gitattributes file when calculating
-# repository-wide language statistics:
-# <https://github.com/github/linguist/blob/v4.7.0/lib/linguist/lazy_blob.rb#L33-L36>
-#
-# The options passed by Linguist are those assumed by Gitlab::Git::InfoAttributes
-# anyway, and there is no great efficiency gain from just fetching the listed
-# attributes with our implementation, so we ignore the additional arguments.
-#
-module Rugged
-  class Repository
-    module UseGitlabGitAttributes
-      def fetch_attributes(name, *)
-        attributes.attributes(name)
-      end
-
-      def attributes
-        @attributes ||= Gitlab::Git::InfoAttributes.new(path)
-      end
-    end
-
-    prepend UseGitlabGitAttributes
-  end
-end

lib/gitlab/bare_repository_import/repository.rb

+# Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/953
+#
 module Gitlab
   module BareRepositoryImport
     class Repository

lib/gitlab/git/repository.rb

@@ -563,6 +563,8 @@ module Gitlab
         return false if ancestor_id.nil? || descendant_id.nil?
 
         merge_base_commit(ancestor_id, descendant_id) == ancestor_id
+      rescue Rugged::OdbError
+        false
       end
 
       # Returns true is +from+ is direct ancestor to +to+, otherwise false

lib/gitlab/gitaly_client/commit_service.rb

@@ -38,20 +38,28 @@ module Gitlab
         from_id = case from
                   when NilClass
                     EMPTY_TREE_ID
-                  when Rugged::Commit
+                  else
+                    if from.respond_to?(:oid)
+                      # This is meant to match a Rugged::Commit. This should be impossible in
+                      # the future.
                       from.oid
                     else
                       from
                     end
+                  end
 
         to_id = case to
                 when NilClass
                   EMPTY_TREE_ID
-                when Rugged::Commit
+                else
+                  if to.respond_to?(:oid)
+                    # This is meant to match a Rugged::Commit. This should be impossible in
+                    # the future.
                     to.oid
                   else
                     to
                   end
+                end
 
         request_params = diff_between_commits_request_params(from_id, to_id, options)
 

lib/gitlab/github_import/importer/pull_requests_importer.rb

@@ -57,10 +57,7 @@ module Gitlab
         end
 
         def commit_exists?(sha)
-          project.repository.lookup(sha)
-          true
-        rescue Rugged::Error
-          false
+          project.repository.commit(sha).present?
         end
 
         def collection_method

lib/tasks/gitlab/cleanup.rake

+# Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/954
+#
 namespace :gitlab do
   namespace :cleanup do
     HASHED_REPOSITORY_NAME = '@hashed'.freeze

scripts/lint-rugged

+#!/usr/bin/env ruby
+
+ALLOWED = [
+  # Can be deleted (?) once rugged is no longer used in production. Doesn't make Rugged calls.
+  'config/initializers/8_metrics.rb',
+
+  # Can be deleted once wiki's are fully (mandatory) migrated
+  'config/initializers/gollum.rb',
+
+  # Needs to be migrated, https://gitlab.com/gitlab-org/gitaly/issues/953
+  'lib/gitlab/bare_repository_import/repository.rb',
+
+  # Needs to be migrated, https://gitlab.com/gitlab-org/gitaly/issues/954
+  'lib/tasks/gitlab/cleanup.rake',
+
+  # https://gitlab.com/gitlab-org/gitaly/issues/961
+  'app/models/repository.rb',
+
+  # The only place where Rugged code is still allowed in production
+  'lib/gitlab/git/'
+].freeze
+
+rugged_lines = IO.popen(%w[git grep -i -n rugged -- app config lib], &:read).lines
+rugged_lines = rugged_lines.reject { |l| l.start_with?(*ALLOWED) }
+rugged_lines = rugged_lines.reject do |line|
+  code, _comment = line.split('# ', 2)
+  code !~ /rugged/i
+end
+
+exit if rugged_lines.empty?
+
+puts "Using Rugged is only allowed in test and #{ALLOWED}\n\n"
+
+puts rugged_lines
+
+exit(false)

scripts/static-analysis

@@ -13,7 +13,8 @@ tasks = [
   %w[bundle exec rake gettext:lint],
   %w[bundle exec rake lint:static_verification],
   %w[scripts/lint-changelog-yaml],
-  %w[scripts/lint-conflicts.sh]
+  %w[scripts/lint-conflicts.sh],
+  %w[scripts/lint-rugged]
 ]
 
 failed_tasks = tasks.reduce({}) do |failures, task|

spec/lib/gitlab/github_import/importer/pull_requests_importer_spec.rb

@@ -244,7 +244,7 @@ describe Gitlab::GithubImport::Importer::PullRequestsImporter do
 
     it 'returns true when a commit exists' do
       expect(project.repository)
-        .to receive(:lookup)
+        .to receive(:commit)
         .with('123')
         .and_return(double(:commit))
 
@@ -253,9 +253,9 @@ describe Gitlab::GithubImport::Importer::PullRequestsImporter do
 
     it 'returns false when a commit does not exist' do
       expect(project.repository)
-        .to receive(:lookup)
+        .to receive(:commit)
         .with('123')
-        .and_raise(Rugged::OdbError)
+        .and_return(nil)
 
       expect(importer.commit_exists?('123')).to eq(false)
     end

spec/models/repository_spec.rb

@@ -2356,7 +2356,7 @@ describe Repository do
     let(:commit) { repository.commit }
     let(:ancestor) { commit.parents.first }
 
-    context 'with Gitaly enabled' do
+    shared_examples '#ancestor?' do
       it 'it is an ancestor' do
         expect(repository.ancestor?(ancestor.id, commit.id)).to eq(true)
       end
@@ -2370,27 +2370,19 @@ describe Repository do
         expect(repository.ancestor?(ancestor.id, nil)).to eq(false)
         expect(repository.ancestor?(nil, nil)).to eq(false)
       end
-    end
 
-    context 'with Gitaly disabled' do
-      before do
-        allow(Gitlab::GitalyClient).to receive(:enabled?).and_return(false)
-        allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:is_ancestor).and_return(false)
+      it 'returns false for invalid commit IDs' do
+        expect(repository.ancestor?(commit.id, Gitlab::Git::BLANK_SHA)).to eq(false)
+        expect(repository.ancestor?( Gitlab::Git::BLANK_SHA, commit.id)).to eq(false)
       end
-
-      it 'it is an ancestor' do
-        expect(repository.ancestor?(ancestor.id, commit.id)).to eq(true)
     end
 
-      it 'it is not an ancestor' do
-        expect(repository.ancestor?(commit.id, ancestor.id)).to eq(false)
+    context 'with Gitaly enabled' do
+      it_behaves_like('#ancestor?')
     end
 
-      it 'returns false on nil-values' do
-        expect(repository.ancestor?(nil, commit.id)).to eq(false)
-        expect(repository.ancestor?(ancestor.id, nil)).to eq(false)
-        expect(repository.ancestor?(nil, nil)).to eq(false)
-      end
+    context 'with Gitaly disabled', :skip_gitaly_mock do
+      it_behaves_like('#ancestor?')
     end
   end