From d8d2b73b9f17e5af9aeccb1e9ba40045486651b5 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Fri, 18 Aug 2017 17:21:01 +0200
Subject: [PATCH 1/3] Improve bare repository import

- Allow imports into nested groups
- Make sure it sets the correct visibility level when creating new
  groups

While doing this, I moved the import into a testable class, that made
it easier to improve.
---
 .../bvl-improve-bare-project-import.yml       |   6 +
 lib/gitlab/bare_repository_importer.rb        | 127 ++++++++++++++++++
 lib/tasks/gitlab/import.rake                  |  65 +--------
 .../gitlab/bare_repository_importer_spec.rb   |  88 ++++++++++++
 4 files changed, 223 insertions(+), 63 deletions(-)
 create mode 100644 changelogs/unreleased/bvl-improve-bare-project-import.yml
 create mode 100644 lib/gitlab/bare_repository_importer.rb
 create mode 100644 spec/lib/gitlab/bare_repository_importer_spec.rb

diff --git a/changelogs/unreleased/bvl-improve-bare-project-import.yml b/changelogs/unreleased/bvl-improve-bare-project-import.yml
new file mode 100644
index 00000000000..74c1da4ea40
--- /dev/null
+++ b/changelogs/unreleased/bvl-improve-bare-project-import.yml
@@ -0,0 +1,6 @@
+---
+title: 'Improve bare project import: Allow subgroups, take default visibility level
+  into account'
+merge_request: 13670
+author:
+type: fixed
diff --git a/lib/gitlab/bare_repository_importer.rb b/lib/gitlab/bare_repository_importer.rb
new file mode 100644
index 00000000000..4bc3ccc3a1a
--- /dev/null
+++ b/lib/gitlab/bare_repository_importer.rb
@@ -0,0 +1,127 @@
+module Gitlab
+  class BareRepositoryImporter
+    NoAdminError = Class.new(StandardError)
+
+    def self.execute
+      Gitlab.config.repositories.storages.each do |storage_name, repository_storage|
+        git_base_path = repository_storage['path']
+        repos_to_import = Dir.glob(git_base_path + '/**/*.git')
+
+        repos_to_import.each do |repo_path|
+          if repo_path.end_with?('.wiki.git')
+            log " * Skipping wiki repo"
+            next
+          end
+
+          log "Processing #{repo_path}".color(:yellow)
+
+          repo_relative_path = repo_path[repository_storage['path'].length..-1]
+                                 .sub(/^\//, '') # Remove leading `/`
+                                 .sub(/\.git$/, '') # Remove `.git` at the end
+          new(storage_name, repo_relative_path).create_project_if_needed
+        end
+      end
+    end
+
+    attr_reader :storage_name, :full_path, :group_path, :project_path, :user
+    delegate :log, to: :class
+
+    def initialize(storage_name, repo_path)
+      @storage_name = storage_name
+      @full_path = repo_path
+
+      unless @user = User.admins.order_id_asc.first
+        raise NoAdminError.new('No admin user found to import repositories')
+      end
+
+      @group_path, @project_path = File.split(repo_path)
+      @group_path = nil if @group_path == '.'
+    end
+
+    def create_project_if_needed
+      if project = Project.find_by_full_path(full_path)
+        log " * #{project.name} (#{full_path}) exists"
+        return project
+      end
+
+      create_project
+    end
+
+    private
+
+    def create_project
+      group = find_or_create_group
+
+      project_params = {
+        name: project_path,
+        path: project_path,
+        repository_storage: storage_name,
+        namespace_id: group&.id
+      }
+
+      project = Projects::CreateService.new(user, project_params).execute
+
+      if project.persisted?
+        log " * Created #{project.name} (#{full_path})".color(:green)
+        ProjectCacheWorker.perform_async(project.id)
+      else
+        log " * Failed trying to create #{project.name} (#{full_path})".color(:red)
+        log "   Errors: #{project.errors.messages}".color(:red)
+      end
+
+      project
+    end
+
+    def find_or_create_group
+      return nil unless group_path
+
+      if namespace = Namespace.find_by_full_path(group_path)
+        log " * Namespace #{group_path} exists.".color(:green)
+        return namespace
+      end
+
+      create_group_path
+    end
+
+    def create_group_path
+      group_path_segments = group_path.split('/')
+
+      new_group, parent_group = nil
+      partial_path_segments = []
+      while subgroup_name = group_path_segments.shift
+        partial_path_segments << subgroup_name
+        partial_path = partial_path_segments.join('/')
+
+        unless new_group = Group.find_by_full_path(partial_path)
+          log " * Creating group #{partial_path}.".color(:green)
+          params = {
+            path: subgroup_name,
+            name: subgroup_name,
+            parent: parent_group,
+            visibility_level: Gitlab::CurrentSettings.current_application_settings.default_group_visibility
+          }
+          new_group = Groups::CreateService.new(user, params).execute
+        end
+
+        if new_group.persisted?
+          log " * Group #{partial_path} (#{new_group.id}) available".color(:green)
+        else
+          log " * Failed trying to create group #{partial_path}.".color(:red)
+          log " * Errors:  #{new_group.errors.messages}".color(:red)
+        end
+        parent_group = new_group
+      end
+
+      new_group
+    end
+
+    # This is called from within a rake task only used by Admins, so allow writing
+    # to STDOUT
+    #
+    # rubocop:disable Rails/Output
+    def self.log(message)
+      puts message
+    end
+    # rubocop:enable Rails/Output
+  end
+end
diff --git a/lib/tasks/gitlab/import.rake b/lib/tasks/gitlab/import.rake
index 6e10ba374bf..d227a0c8bdb 100644
--- a/lib/tasks/gitlab/import.rake
+++ b/lib/tasks/gitlab/import.rake
@@ -9,6 +9,7 @@ namespace :gitlab do
     #  * The project owner will set to the first administator of the system
     #  * Existing projects will be skipped
     #
+    #
     desc "GitLab | Import bare repositories from repositories -> storages into GitLab project instance"
     task repos: :environment do
       if Project.current_application_settings.hashed_storage_enabled
@@ -17,69 +18,7 @@ namespace :gitlab do
         exit 1
       end
 
-      Gitlab.config.repositories.storages.each_value do |repository_storage|
-        git_base_path = repository_storage['path']
-        repos_to_import = Dir.glob(git_base_path + '/**/*.git')
-
-        repos_to_import.each do |repo_path|
-          # strip repo base path
-          repo_path[0..git_base_path.length] = ''
-
-          path = repo_path.sub(/\.git$/, '')
-          group_name, name = File.split(path)
-          group_name = nil if group_name == '.'
-
-          puts "Processing #{repo_path}".color(:yellow)
-
-          if path.end_with?('.wiki')
-            puts " * Skipping wiki repo"
-            next
-          end
-
-          project = Project.find_by_full_path(path)
-
-          if project
-            puts " * #{project.name} (#{repo_path}) exists"
-          else
-            user = User.admins.reorder("id").first
-
-            project_params = {
-              name: name,
-              path: name
-            }
-
-            # find group namespace
-            if group_name
-              group = Namespace.find_by(path: group_name)
-              # create group namespace
-              unless group
-                group = Group.new(name: group_name)
-                group.path = group_name
-                group.owner = user
-                if group.save
-                  puts " * Created Group #{group.name} (#{group.id})".color(:green)
-                else
-                  puts " * Failed trying to create group #{group.name}".color(:red)
-                end
-              end
-              # set project group
-              project_params[:namespace_id] = group.id
-            end
-
-            project = Projects::CreateService.new(user, project_params).execute
-
-            if project.persisted?
-              puts " * Created #{project.name} (#{repo_path})".color(:green)
-              ProjectCacheWorker.perform_async(project.id)
-            else
-              puts " * Failed trying to create #{project.name} (#{repo_path})".color(:red)
-              puts "   Errors: #{project.errors.messages}".color(:red)
-            end
-          end
-        end
-      end
-
-      puts "Done!".color(:green)
+      Gitlab::BareRepositoryImporter.execute
     end
   end
 end
diff --git a/spec/lib/gitlab/bare_repository_importer_spec.rb b/spec/lib/gitlab/bare_repository_importer_spec.rb
new file mode 100644
index 00000000000..88d30a2e855
--- /dev/null
+++ b/spec/lib/gitlab/bare_repository_importer_spec.rb
@@ -0,0 +1,88 @@
+require 'spec_helper'
+
+describe Gitlab::BareRepositoryImporter, repository: true do
+  subject(:importer) { described_class.new('default', project_path) }
+  let(:project_path) { 'a-group/a-sub-group/a-project' }
+  let!(:admin) { create(:admin) }
+
+  before do
+    allow(described_class).to receive(:log)
+  end
+
+  describe '.execute' do
+    it 'creates a project for a repository in storage' do
+      FileUtils.mkdir_p(File.join(TestEnv.repos_path, "#{project_path}.git"))
+      fake_importer = double
+
+      expect(described_class).to receive(:new).with('default', project_path)
+                                   .and_return(fake_importer)
+      expect(fake_importer).to receive(:create_project_if_needed)
+
+      described_class.execute
+    end
+
+    it 'skips wiki repos' do
+      FileUtils.mkdir_p(File.join(TestEnv.repos_path, 'the-group', 'the-project.wiki.git'))
+
+      expect(described_class).to receive(:log).with(' * Skipping wiki repo')
+      expect(described_class).not_to receive(:new)
+
+      described_class.execute
+    end
+  end
+
+  describe '#initialize' do
+    context 'without admin users' do
+      let(:admin) { nil }
+
+      it 'raises an error' do
+        expect { importer }.to raise_error(Gitlab::BareRepositoryImporter::NoAdminError)
+      end
+    end
+  end
+
+  describe '#create_project_if_needed' do
+    it 'starts an import for a project that did not exist' do
+      expect(importer).to receive(:create_project)
+
+      importer.create_project_if_needed
+    end
+
+    it 'skips importing when the project already exists' do
+      group = create(:group, path: 'a-group')
+      subgroup = create(:group, path: 'a-sub-group', parent: group)
+      project = create(:project, path: 'a-project', namespace: subgroup)
+
+      expect(importer).not_to receive(:create_project)
+      expect(importer).to receive(:log).with(" * #{project.name} (a-group/a-sub-group/a-project) exists")
+
+      importer.create_project_if_needed
+    end
+
+    it 'creates a project with the correct path in the database' do
+      importer.create_project_if_needed
+
+      expect(Project.find_by_full_path(project_path)).not_to be_nil
+    end
+
+    it 'creates group and subgroup in the database' do
+      importer.create_project_if_needed
+
+      parent = Group.find_by_full_path('a-group')
+      child = parent.children.find_by(path: 'a-sub-group')
+
+      expect(parent).not_to be_nil
+      expect(child).not_to be_nil
+    end
+
+    it 'creates the group with correct visibility level' do
+      allow(Gitlab::CurrentSettings.current_application_settings)
+        .to receive(:default_group_visibility) { Gitlab::VisibilityLevel::INTERNAL }
+      project = importer.create_project_if_needed
+
+      group = project.namespace
+
+      expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
+    end
+  end
+end

From 22ef4ba3a4be66296e5ee9231b4eb39e172c0f1f Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Tue, 22 Aug 2017 12:13:25 +0200
Subject: [PATCH 2/3] Migrate creation of nested groups into a service

---
 app/services/groups/nested_create_service.rb  | 45 ++++++++++++++++
 lib/gitlab/bare_repository_importer.rb        | 35 +-----------
 lib/tasks/import.rake                         | 18 +------
 .../gitlab/bare_repository_importer_spec.rb   | 20 -------
 .../groups/nested_create_service_spec.rb      | 53 +++++++++++++++++++
 5 files changed, 101 insertions(+), 70 deletions(-)
 create mode 100644 app/services/groups/nested_create_service.rb
 create mode 100644 spec/services/groups/nested_create_service_spec.rb

diff --git a/app/services/groups/nested_create_service.rb b/app/services/groups/nested_create_service.rb
new file mode 100644
index 00000000000..8d793f5c02e
--- /dev/null
+++ b/app/services/groups/nested_create_service.rb
@@ -0,0 +1,45 @@
+module Groups
+  class NestedCreateService < Groups::BaseService
+    attr_reader :group_path
+
+    def initialize(user, params)
+      @current_user, @params = user, params.dup
+
+      @group_path = @params.delete(:group_path)
+    end
+
+    def execute
+      return nil unless group_path
+
+      if group = Group.find_by_full_path(group_path)
+        return group
+      end
+
+      create_group_path
+    end
+
+    private
+
+    def create_group_path
+      group_path_segments = group_path.split('/')
+
+      last_group = nil
+      partial_path_segments = []
+      while subgroup_name = group_path_segments.shift
+        partial_path_segments << subgroup_name
+        partial_path = partial_path_segments.join('/')
+
+        new_params = params.reverse_merge(
+          path: subgroup_name,
+          name: subgroup_name,
+          parent: last_group
+        )
+        new_params[:visibility_level] ||= Gitlab::CurrentSettings.current_application_settings.default_group_visibility
+
+        last_group = Group.find_by_full_path(partial_path) || Groups::CreateService.new(current_user, new_params).execute
+      end
+
+      last_group
+    end
+  end
+end
diff --git a/lib/gitlab/bare_repository_importer.rb b/lib/gitlab/bare_repository_importer.rb
index 4bc3ccc3a1a..9323bfc7fb2 100644
--- a/lib/gitlab/bare_repository_importer.rb
+++ b/lib/gitlab/bare_repository_importer.rb
@@ -80,39 +80,8 @@ module Gitlab
         return namespace
       end
 
-      create_group_path
-    end
-
-    def create_group_path
-      group_path_segments = group_path.split('/')
-
-      new_group, parent_group = nil
-      partial_path_segments = []
-      while subgroup_name = group_path_segments.shift
-        partial_path_segments << subgroup_name
-        partial_path = partial_path_segments.join('/')
-
-        unless new_group = Group.find_by_full_path(partial_path)
-          log " * Creating group #{partial_path}.".color(:green)
-          params = {
-            path: subgroup_name,
-            name: subgroup_name,
-            parent: parent_group,
-            visibility_level: Gitlab::CurrentSettings.current_application_settings.default_group_visibility
-          }
-          new_group = Groups::CreateService.new(user, params).execute
-        end
-
-        if new_group.persisted?
-          log " * Group #{partial_path} (#{new_group.id}) available".color(:green)
-        else
-          log " * Failed trying to create group #{partial_path}.".color(:red)
-          log " * Errors:  #{new_group.errors.messages}".color(:red)
-        end
-        parent_group = new_group
-      end
-
-      new_group
+      log " * Creating Group: #{group_path}"
+      Groups::NestedCreateService.new(user, group_path: group_path).execute
     end
 
     # This is called from within a rake task only used by Admins, so allow writing
diff --git a/lib/tasks/import.rake b/lib/tasks/import.rake
index 96b8f59242c..1206302cb76 100644
--- a/lib/tasks/import.rake
+++ b/lib/tasks/import.rake
@@ -72,23 +72,7 @@ class GithubImport
     return @current_user.namespace if names == @current_user.namespace_path
     return @current_user.namespace unless @current_user.can_create_group?
 
-    full_path_namespace = Namespace.find_by_full_path(names)
-
-    return full_path_namespace if full_path_namespace
-
-    names.split('/').inject(nil) do |parent, name|
-      begin
-        namespace = Group.create!(name: name,
-                                  path: name,
-                                  owner: @current_user,
-                                  parent: parent)
-        namespace.add_owner(@current_user)
-
-        namespace
-      rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid
-        Namespace.where(parent: parent).find_by_path_or_name(name)
-      end
-    end
+    Groups::NestedCreateService.new(@current_user, group_path: names).execute
   end
 
   def full_path_namespace(names)
diff --git a/spec/lib/gitlab/bare_repository_importer_spec.rb b/spec/lib/gitlab/bare_repository_importer_spec.rb
index 88d30a2e855..892f2dafc96 100644
--- a/spec/lib/gitlab/bare_repository_importer_spec.rb
+++ b/spec/lib/gitlab/bare_repository_importer_spec.rb
@@ -64,25 +64,5 @@ describe Gitlab::BareRepositoryImporter, repository: true do
 
       expect(Project.find_by_full_path(project_path)).not_to be_nil
     end
-
-    it 'creates group and subgroup in the database' do
-      importer.create_project_if_needed
-
-      parent = Group.find_by_full_path('a-group')
-      child = parent.children.find_by(path: 'a-sub-group')
-
-      expect(parent).not_to be_nil
-      expect(child).not_to be_nil
-    end
-
-    it 'creates the group with correct visibility level' do
-      allow(Gitlab::CurrentSettings.current_application_settings)
-        .to receive(:default_group_visibility) { Gitlab::VisibilityLevel::INTERNAL }
-      project = importer.create_project_if_needed
-
-      group = project.namespace
-
-      expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
-    end
   end
 end
diff --git a/spec/services/groups/nested_create_service_spec.rb b/spec/services/groups/nested_create_service_spec.rb
new file mode 100644
index 00000000000..c1526456bac
--- /dev/null
+++ b/spec/services/groups/nested_create_service_spec.rb
@@ -0,0 +1,53 @@
+require 'spec_helper'
+
+describe Groups::NestedCreateService do
+  let(:user) { create(:user) }
+  let(:params) { { group_path: 'a-group/a-sub-group' } }
+
+  subject(:service) { described_class.new(user, params) }
+
+  describe "#execute" do
+    it 'returns the group if it already existed' do
+      parent = create(:group, path: 'a-group', owner: user)
+      child = create(:group, path: 'a-sub-group', parent: parent, owner: user)
+
+      expect(service.execute).to eq(child)
+    end
+
+    it 'reuses a parent if it already existed' do
+      parent = create(:group, path: 'a-group')
+      parent.add_owner(user)
+
+      expect(service.execute.parent).to eq(parent)
+    end
+
+    it 'creates group and subgroup in the database' do
+      service.execute
+
+      parent = Group.find_by_full_path('a-group')
+      child = parent.children.find_by(path: 'a-sub-group')
+
+      expect(parent).not_to be_nil
+      expect(child).not_to be_nil
+    end
+
+    it 'creates the group with correct visibility level' do
+      allow(Gitlab::CurrentSettings.current_application_settings)
+        .to receive(:default_group_visibility) { Gitlab::VisibilityLevel::INTERNAL }
+
+      group = service.execute
+
+      expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
+    end
+
+    context 'adding a visibility level ' do
+      let(:params) { { group_path: 'a-group/a-sub-group', visibility_level: Gitlab::VisibilityLevel::PRIVATE } }
+
+      it 'overwrites the visibility level' do
+        group = service.execute
+
+        expect(group.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE)
+      end
+    end
+  end
+end

From f76d8c902a08040eeaacc8c7a3a9506fb55501e7 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Tue, 22 Aug 2017 14:03:40 +0200
Subject: [PATCH 3/3] Fix error when importing a GitHub-wiki repository

This would occur when Wiki's are enabled on GitHub, but there is no
wiki repository
---
 lib/github/import.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/github/import.rb b/lib/github/import.rb
index 4cc01593ef4..7b848081e85 100644
--- a/lib/github/import.rb
+++ b/lib/github/import.rb
@@ -107,7 +107,7 @@ module Github
       # this means that repo has wiki enabled, but have no pages. So,
       # we can skip the import.
       if e.message !~ /repository not exported/
-        errors(:wiki, wiki_url, e.message)
+        error(:wiki, wiki_url, e.message)
       end
     end