Merge branch 'web-editor-rugged' into 'master'

Create and edit files in web editor via rugged

- [x] create file via rugged
- [x] update file via rugged
- [x] remove file via rugged
- [ ] fix tests
- [x] remove satellites code
- [x] create activity event for new/edit file via rugged
- [x] base64 support

Part of https://dev.gitlab.org/gitlab/gitlabhq/issues/2300

See merge request !751

CHANGELOG

@@ -38,6 +38,7 @@ v 7.12.0 (unreleased)
   - Add SAML support as an omniauth provider
   - Allow to configure a URL to show after sign out 
   - Add an option to automatically sign-in with an Omniauth provider
+  - Better performance for web editor (switched from satellites to rugged)
 
 v 7.11.4
   - Fix missing bullets when creating lists

Gemfile

@@ -45,7 +45,7 @@ gem "browser"
 
 # Extracting information from a git repository
 # Provide access to Gitlab::Git library
-gem "gitlab_git", '~> 7.1.13'
+gem "gitlab_git", '~> 7.2.2'
 
 # Ruby/Rack Git Smart-HTTP Server Handler
 # GitLab fork with a lot of changes (improved thread-safety, better memory usage etc)

Gemfile.lock

@@ -225,7 +225,7 @@ GEM
       mime-types (~> 1.19)
     gitlab_emoji (0.1.0)
       gemojione (~> 2.0)
-    gitlab_git (7.1.13)
+    gitlab_git (7.2.2)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.6)
       gitlab-linguist (~> 3.0)
@@ -733,7 +733,7 @@ DEPENDENCIES
   gitlab-grack (~> 2.0.2)
   gitlab-linguist (~> 3.0.1)
   gitlab_emoji (~> 0.1)
-  gitlab_git (~> 7.1.13)
+  gitlab_git (~> 7.2.2)
   gitlab_meta (= 7.0)
   gitlab_omniauth-ldap (= 1.2.1)
   gollum-lib (~> 4.0.2)

app/models/repository.rb

@@ -370,8 +370,55 @@ class Repository
     @root_ref ||= raw_repository.root_ref
   end
 
+  def commit_file(user, path, content, message, ref)
+    path[0] = '' if path[0] == '/'
+
+    committer = user_to_comitter(user)
+    options = {}
+    options[:committer] = committer
+    options[:author] = committer
+    options[:commit] = {
+      message: message,
+      branch: ref
+    }
+
+    options[:file] = {
+      content: content,
+      path: path
+    }
+
+    Gitlab::Git::Blob.commit(raw_repository, options)
+  end
+
+  def remove_file(user, path, message, ref)
+    path[0] = '' if path[0] == '/'
+
+    committer = user_to_comitter(user)
+    options = {}
+    options[:committer] = committer
+    options[:author] = committer
+    options[:commit] = {
+      message: message,
+      branch: ref
+    }
+
+    options[:file] = {
+      path: path
+    }
+
+    Gitlab::Git::Blob.remove(raw_repository, options)
+  end
+
   private
 
+  def user_to_comitter(user)
+    {
+      email: user.email,
+      name: user.name,
+      time: Time.now
+    }
+  end
+
   def cache
     @cache ||= RepositoryCache.new(path_with_namespace)
   end

app/services/files/base_service.rb

@@ -13,5 +13,12 @@ module Files
     def repository
       project.repository
     end
+
+    def after_commit(sha)
+      commit = repository.commit(sha)
+      full_ref = 'refs/heads/' + (params[:new_branch] || ref)
+      old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA
+      GitPushService.new.execute(project, current_user, old_sha, sha, full_ref)
+    end
   end
 end

app/services/files/create_service.rb

 require_relative "base_service"
 
 module Files
-  class CreateService < BaseService
+  class CreateService < Files::BaseService
     def execute
       allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
 
@@ -33,16 +33,24 @@ module Files
         end
       end
 
+      content =
+        if params[:encoding] == 'base64'
+          Base64.decode64(params[:content])
+        else
+          params[:content]
+        end
 
-      new_file_action = Gitlab::Satellite::NewFileAction.new(current_user, project, ref, file_path)
-      created_successfully = new_file_action.commit!(
-        params[:content],
+      sha = repository.commit_file(
+        current_user,
+        file_path,
+        content,
         params[:commit_message],
-        params[:encoding],
-        params[:new_branch]
+        params[:new_branch] || ref
       )
 
-      if created_successfully
+
+      if sha
+        after_commit(sha)
         success
       else
         error("Your changes could not be committed, because the file has been changed")

app/services/files/delete_service.rb

 require_relative "base_service"
 
 module Files
-  class DeleteService < BaseService
+  class DeleteService < Files::BaseService
     def execute
       allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
 
@@ -19,14 +19,15 @@ module Files
         return error("You can only edit text files")
       end
 
-      delete_file_action = Gitlab::Satellite::DeleteFileAction.new(current_user, project, ref, path)
-
-      deleted_successfully = delete_file_action.commit!(
-        nil,
-        params[:commit_message]
+      sha = repository.remove_file(
+        current_user,
+        path,
+        params[:commit_message],
+        ref
       )
 
-      if deleted_successfully
+      if sha
+        after_commit(sha)
         success
       else
         error("Your changes could not be committed, because the file has been changed")

app/services/files/update_service.rb

 require_relative "base_service"
 
 module Files
-  class UpdateService < BaseService
+  class UpdateService < Files::BaseService
     def execute
       allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
 
@@ -19,14 +19,22 @@ module Files
         return error("You can only edit text files")
       end
 
-      edit_file_action = Gitlab::Satellite::EditFileAction.new(current_user, project, ref, path)
-      edit_file_action.commit!(
-        params[:content],
+      content =
+        if params[:encoding] == 'base64'
+          Base64.decode64(params[:content])
+        else
+          params[:content]
+        end
+
+      sha = repository.commit_file(
+        current_user,
+        path,
+        content,
         params[:commit_message],
-        params[:encoding],
-        params[:new_branch]
+        params[:new_branch] || ref
       )
 
+      after_commit(sha)
       success
     rescue Gitlab::Satellite::CheckoutFailed => ex
       error("Your changes could not be committed because ref '#{ref}' could not be checked out", 400)

app/services/git_push_service.rb

@@ -127,7 +127,8 @@ class GitPushService
   end
 
   def is_default_branch?(ref)
-    Gitlab::Git.branch_ref?(ref) && Gitlab::Git.ref_name(ref) == project.default_branch
+    Gitlab::Git.branch_ref?(ref) &&
+      (Gitlab::Git.ref_name(ref) == project.default_branch || project.default_branch.nil?)
   end
 
   def commit_user(commit)

app/views/projects/blob/_editor.html.haml

@@ -12,8 +12,8 @@
         \/
         = text_field_tag 'file_name', params[:file_name], placeholder: "File name",
           required: true, class: 'form-control new-file-name'
-        .pull-right
-          = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'form-control'
+      .pull-right
+        = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'form-control'
 
   .file-content.code
     %pre.js-edit-mode-pane#editor

lib/gitlab/satellite/files/delete_file_action.rb

-require_relative 'file_action'
-
-module Gitlab
-  module Satellite
-    class DeleteFileAction < FileAction
-      # Deletes file and creates a new commit for it
-      #
-      # Returns false if committing the change fails
-      # Returns false if pushing from the satellite to bare repo failed or was rejected
-      # Returns true otherwise
-      def commit!(content, commit_message)
-        in_locked_and_timed_satellite do |repo|
-          prepare_satellite!(repo)
-
-          # create target branch in satellite at the corresponding commit from bare repo
-          repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}")
-
-          # update the file in the satellite's working dir
-          file_path_in_satellite = File.join(repo.working_dir, file_path)
-
-          # Prevent relative links
-          unless safe_path?(file_path_in_satellite)
-            Gitlab::GitLogger.error("FileAction: Relative path not allowed")
-            return false
-          end
-
-          File.delete(file_path_in_satellite)
-
-          # add removed file
-          repo.remove(file_path_in_satellite)
-
-          # commit the changes
-          # will raise CommandFailed when commit fails
-          repo.git.commit(raise: true, timeout: true, a: true, m: commit_message)
-
-
-          # push commit back to bare repo
-          # will raise CommandFailed when push fails
-          repo.git.push({ raise: true, timeout: true }, :origin, ref)
-
-          # everything worked
-          true
-        end
-      rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
-      end
-    end
-  end
-end

lib/gitlab/satellite/files/edit_file_action.rb

-require_relative 'file_action'
-
-module Gitlab
-  module Satellite
-    # GitLab server-side file update and commit
-    class EditFileAction < FileAction
-      # Updates the files content and creates a new commit for it
-      #
-      # Returns false if the ref has been updated while editing the file
-      # Returns false if committing the change fails
-      # Returns false if pushing from the satellite to bare repo failed or was rejected
-      # Returns true otherwise
-      def commit!(content, commit_message, encoding, new_branch = nil)
-        in_locked_and_timed_satellite do |repo|
-          prepare_satellite!(repo)
-
-          # create target branch in satellite at the corresponding commit from bare repo
-          begin
-            repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}")
-          rescue Grit::Git::CommandFailed => ex
-            log_and_raise(CheckoutFailed, ex.message)
-          end
-
-          # update the file in the satellite's working dir
-          file_path_in_satellite = File.join(repo.working_dir, file_path)
-
-          # Prevent relative links
-          unless safe_path?(file_path_in_satellite)
-            Gitlab::GitLogger.error("FileAction: Relative path not allowed")
-            return false
-          end
-
-          # Write file
-          write_file(file_path_in_satellite, content, encoding)
-
-          # commit the changes
-          # will raise CommandFailed when commit fails
-          begin
-            repo.git.commit(raise: true, timeout: true, a: true, m: commit_message)
-          rescue Grit::Git::CommandFailed => ex
-            log_and_raise(CommitFailed, ex.message)
-          end
-
-
-          target_branch = new_branch.present? ? "#{ref}:#{new_branch}" : ref
-
-          # push commit back to bare repo
-          # will raise CommandFailed when push fails
-          begin
-            repo.git.push({ raise: true, timeout: true }, :origin, target_branch)
-          rescue Grit::Git::CommandFailed => ex
-            log_and_raise(PushFailed, ex.message)
-          end
-
-          # everything worked
-          true
-        end
-      end
-
-      private
-
-      def log_and_raise(errorClass, message)
-        Gitlab::GitLogger.error(message)
-        raise(errorClass, message)
-      end
-    end
-  end
-end

lib/gitlab/satellite/files/file_action.rb

-module Gitlab
-  module Satellite
-    class FileAction < Action
-      attr_accessor :file_path, :ref
-
-      def initialize(user, project, ref, file_path)
-        super user, project
-        @file_path = file_path
-        @ref = ref
-      end
-
-      def safe_path?(path)
-        File.absolute_path(path) == path
-      end
-
-      def write_file(abs_file_path, content, file_encoding = 'text')
-        if file_encoding == 'base64'
-          File.open(abs_file_path, 'wb') { |f| f.write(Base64.decode64(content)) }
-        else
-          File.open(abs_file_path, 'w') { |f| f.write(content) }
-        end
-      end
-    end
-  end
-end

lib/gitlab/satellite/files/new_file_action.rb

-require_relative 'file_action'
-
-module Gitlab
-  module Satellite
-    class NewFileAction < FileAction
-      # Updates the files content and creates a new commit for it
-      #
-      # Returns false if the ref has been updated while editing the file
-      # Returns false if committing the change fails
-      # Returns false if pushing from the satellite to bare repo failed or was rejected
-      # Returns true otherwise
-      def commit!(content, commit_message, encoding, new_branch = nil)
-        in_locked_and_timed_satellite do |repo|
-          prepare_satellite!(repo)
-
-          # create target branch in satellite at the corresponding commit from bare repo
-          current_ref =
-            if @project.empty_repo?
-              # skip this step if we want to add first file to empty repo
-              Satellite::PARKING_BRANCH
-            else
-              repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}")
-              ref
-            end
-
-          file_path_in_satellite = File.join(repo.working_dir, file_path)
-          dir_name_in_satellite = File.dirname(file_path_in_satellite)
-
-          # Prevent relative links
-          unless safe_path?(file_path_in_satellite)
-            Gitlab::GitLogger.error("FileAction: Relative path not allowed")
-            return false
-          end
-
-          # Create dir if not exists
-          FileUtils.mkdir_p(dir_name_in_satellite)
-
-          # Write file
-          write_file(file_path_in_satellite, content, encoding)
-
-          # add new file
-          repo.add(file_path_in_satellite)
-
-          # commit the changes
-          # will raise CommandFailed when commit fails
-          repo.git.commit(raise: true, timeout: true, a: true, m: commit_message)
-
-          target_branch = if new_branch.present? && !@project.empty_repo?
-                            "#{ref}:#{new_branch}"
-                          else
-                            "#{current_ref}:#{ref}"
-                          end
-
-          # push commit back to bare repo
-          # will raise CommandFailed when push fails
-          repo.git.push({ raise: true, timeout: true }, :origin, target_branch)
-
-          # everything worked
-          true
-        end
-      rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
-      end
-    end
-  end
-end

spec/requests/api/files_spec.rb

@@ -49,10 +49,6 @@ describe API::API, api: true  do
     }
 
     it "should create a new file in project repo" do
-      Gitlab::Satellite::NewFileAction.any_instance.stub(
-        commit!: true,
-      )
-
       post api("/projects/#{project.id}/repository/files", user), valid_params
       expect(response.status).to eq(201)
       expect(json_response['file_path']).to eq('newfile.rb')
@@ -63,9 +59,9 @@ describe API::API, api: true  do
       expect(response.status).to eq(400)
     end
 
-    it "should return a 400 if satellite fails to create file" do
-      Gitlab::Satellite::NewFileAction.any_instance.stub(
-        commit!: false,
+    it "should return a 400 if editor fails to create file" do
+      Repository.any_instance.stub(
+        commit_file: false,
       )
 
       post api("/projects/#{project.id}/repository/files", user), valid_params
@@ -84,10 +80,6 @@ describe API::API, api: true  do
     }
 
     it "should update existing file in project repo" do
-      Gitlab::Satellite::EditFileAction.any_instance.stub(
-        commit!: true,
-      )
-
       put api("/projects/#{project.id}/repository/files", user), valid_params
       expect(response.status).to eq(200)
       expect(json_response['file_path']).to eq(file_path)
@@ -97,35 +89,6 @@ describe API::API, api: true  do
       put api("/projects/#{project.id}/repository/files", user)
       expect(response.status).to eq(400)
     end
-
-    it 'should return a 400 if the checkout fails' do
-      Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!)
-        .and_raise(Gitlab::Satellite::CheckoutFailed)
-
-      put api("/projects/#{project.id}/repository/files", user), valid_params
-      expect(response.status).to eq(400)
-
-      ref = valid_params[:branch_name]
-      expect(response.body).to match("ref '#{ref}' could not be checked out")
-    end
-
-    it 'should return a 409 if the file was not modified' do
-      Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!)
-        .and_raise(Gitlab::Satellite::CommitFailed)
-
-      put api("/projects/#{project.id}/repository/files", user), valid_params
-      expect(response.status).to eq(409)
-      expect(response.body).to match("Maybe there was nothing to commit?")
-    end
-
-    it 'should return a 409 if the push fails' do
-      Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!)
-        .and_raise(Gitlab::Satellite::PushFailed)
-
-      put api("/projects/#{project.id}/repository/files", user), valid_params
-      expect(response.status).to eq(409)
-      expect(response.body).to match("Maybe the file was changed by another process?")
-    end
   end
 
   describe "DELETE /projects/:id/repository/files" do
@@ -138,10 +101,6 @@ describe API::API, api: true  do
     }
 
     it "should delete existing file in project repo" do
-      Gitlab::Satellite::DeleteFileAction.any_instance.stub(
-        commit!: true,
-      )
-
       delete api("/projects/#{project.id}/repository/files", user), valid_params
       expect(response.status).to eq(200)
       expect(json_response['file_path']).to eq(file_path)
@@ -153,8 +112,8 @@ describe API::API, api: true  do
     end
 
     it "should return a 400 if satellite fails to create file" do
-      Gitlab::Satellite::DeleteFileAction.any_instance.stub(
-        commit!: false,
+      Repository.any_instance.stub(
+        remove_file: false,
       )
 
       delete api("/projects/#{project.id}/repository/files", user), valid_params