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

This reverts commit 5a1aa49b, reversing
changes made to a675bea2.
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/services/files/delete_service.rb

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

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,6 +49,10 @@ describe API::API, api: true  do
     end
 
     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')
@@ -59,9 +63,10 @@ describe API::API, api: true  do
       expect(response.status).to eq(400)
     end
 
-    it "should return a 400 if editor fails to create file" do
-      allow_any_instance_of(Repository).to receive(:commit_file).
-        and_return(false)
+    it "should return a 400 if satellite fails to create file" do
+      Gitlab::Satellite::NewFileAction.any_instance.stub(
+        commit!: false,
+      )
 
       post api("/projects/#{project.id}/repository/files", user), valid_params
       expect(response.status).to eq(400)
@@ -79,6 +84,10 @@ describe API::API, api: true  do
     end
 
     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)
@@ -88,6 +97,35 @@ 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
@@ -100,6 +138,10 @@ describe API::API, api: true  do
     end
 
     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)
@@ -111,7 +153,9 @@ describe API::API, api: true  do
     end
 
     it "should return a 400 if satellite fails to create file" do
-      allow_any_instance_of(Repository).to receive(:remove_file).and_return(false)
+      Gitlab::Satellite::DeleteFileAction.any_instance.stub(
+        commit!: false,
+      )
 
       delete api("/projects/#{project.id}/repository/files", user), valid_params
       expect(response.status).to eq(400)