Refactor web editor

* fix problem with editing non-master branch
* before commit make sure branch exists
* dont allow user change file in one branch and commit to another existing branch
* remove a lot of code duplication
* remove outdated statellite errors

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

Gemfile

@@ -34,7 +34,7 @@ gem "browser"
 # Extracting information from a git repository
 # Provide access to Gitlab::Git library
-gem "gitlab_git", '~> 7.2.2'
+gem "gitlab_git", '~> 7.2.3'
 # 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.2.2)
+    gitlab_git (7.2.3)
       activesupport (~> 4.0)
       charlock_holmes (~> 0.6)
       gitlab-linguist (~> 3.0)
@@ -713,7 +713,7 @@ DEPENDENCIES
   gitlab-grack (~> 2.0.2)
   gitlab-linguist (~> 3.0.1)
   gitlab_emoji (~> 0.1)
-  gitlab_git (~> 7.2.2)
+  gitlab_git (~> 7.2.3)
   gitlab_meta (= 7.0)
   gitlab_omniauth-ldap (= 1.2.1)
   gollum-lib (~> 4.0.2)

app/controllers/projects/blob_controller.rb

@@ -13,27 +13,20 @@ class Projects::BlobController < Projects::ApplicationController
   before_action :commit, except: [:new, :create]
   before_action :blob, except: [:new, :create]
   before_action :from_merge_request, only: [:edit, :update]
-  before_action :after_edit_path, only: [:edit, :update]
   before_action :require_branch_head, only: [:edit, :update]
+  before_action :editor_variables, except: [:show, :preview, :diff]
+  before_action :after_edit_path, only: [:edit, :update]
   def new
     commit unless @repository.empty?
   end
   def create
-    file_path = File.join(@path, File.basename(params[:file_name]))
-    result = Files::CreateService.new(
-      @project,
-      current_user,
-      params.merge(new_branch: sanitized_new_branch_name),
-      @ref,
-      file_path
-    ).execute
+    result = Files::CreateService.new(@project, current_user, @commit_params).execute
     if result[:status] == :success
       flash[:notice] = "Your changes have been successfully committed"
-      ref = sanitized_new_branch_name.presence || @ref
-      redirect_to namespace_project_blob_path(@project.namespace, @project, File.join(ref, file_path))
+      redirect_to namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @file_path))
     else
       flash[:alert] = result[:message]
       render :new
@@ -48,22 +41,10 @@ class Projects::BlobController < Projects::ApplicationController
   end
   def update
-    result = Files::UpdateService.
-      new(
-        @project,
-        current_user,
-        params.merge(new_branch: sanitized_new_branch_name),
-        @ref,
-        @path
-      ).execute
+    result = Files::UpdateService.new(@project, current_user, @commit_params).execute
     if result[:status] == :success
       flash[:notice] = "Your changes have been successfully committed"
-
-      if from_merge_request
-        from_merge_request.reload_code
-      end
-
       redirect_to after_edit_path
     else
       flash[:alert] = result[:message]
@@ -80,12 +61,11 @@ class Projects::BlobController < Projects::ApplicationController
   end
   def destroy
-    result = Files::DeleteService.new(@project, current_user, params, @ref, @path).execute
+    result = Files::DeleteService.new(@project, current_user, @commit_params).execute
     if result[:status] == :success
       flash[:notice] = "Your changes have been successfully committed"
-      redirect_to namespace_project_tree_path(@project.namespace, @project,
-                                              @ref)
+      redirect_to namespace_project_tree_path(@project.namespace, @project, @target_branch)
     else
       flash[:alert] = result[:message]
       render :show
@@ -135,7 +115,6 @@ class Projects::BlobController < Projects::ApplicationController
     @id = params[:id]
     @ref, @path = extract_ref(@id)
-
   rescue InvalidPathError
     not_found!
   end
@@ -145,8 +124,8 @@ class Projects::BlobController < Projects::ApplicationController
       if from_merge_request
         diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) +
           "#file-path-#{hexdigest(@path)}"
-      elsif sanitized_new_branch_name.present?
-        namespace_project_blob_path(@project.namespace, @project, File.join(sanitized_new_branch_name, @path))
+      elsif @target_branch.present?
+        namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path))
       else
         namespace_project_blob_path(@project.namespace, @project, @id)
       end
@@ -160,4 +139,25 @@ class Projects::BlobController < Projects::ApplicationController
   def sanitized_new_branch_name
     @new_branch ||= sanitize(strip_tags(params[:new_branch]))
   end
+
+  def editor_variables
+    @current_branch = @ref
+    @target_branch = (sanitized_new_branch_name || @ref)
+
+    @file_path =
+      if action_name.to_s == 'create'
+        File.join(@path, File.basename(params[:file_name]))
+      else
+        @path
+      end
+
+    @commit_params = {
+      file_path: @file_path,
+      current_branch: @current_branch,
+      target_branch: @target_branch,
+      commit_message: params[:commit_message],
+      file_content: params[:content],
+      file_content_encoding: params[:encoding]
+    }
+  end
 end

app/services/files/base_service.rb

 module Files
   class BaseService < ::BaseService
-    attr_reader :ref, :path
-    def initialize(project, user, params, ref, path = nil)
-      @project, @current_user, @params = project, user, params.dup
-      @ref = ref
-      @path = path
+    class ValidationError < StandardError; end
+    def execute
+      @current_branch = params[:current_branch]
+      @target_branch  = params[:target_branch]
+      @commit_message = params[:commit_message]
+      @file_path      = params[:file_path]
+      @file_content   = if params[:file_content_encoding] == 'base64'
+                          Base64.decode64(params[:file_content])
+                        else
+                          params[:file_content]
+                        end
+
+      # Validate parameters
+      validate
+
+      # Create new branch if it different from current_branch
+      if @target_branch != @current_branch
+        create_target_branch
+      end
+
+      if sha = commit
+        after_commit(sha, @target_branch)
+        success
+      else
+        error("Something went wrong. Your changes were not committed")
+      end
+    rescue ValidationError => ex
+      error(ex.message)
     end
     private
@@ -14,11 +37,51 @@ module Files
       project.repository
     end
-    def after_commit(sha)
+    def after_commit(sha, branch)
       commit = repository.commit(sha)
-      full_ref = 'refs/heads/' + (params[:new_branch] || ref)
+      full_ref = 'refs/heads/' + branch
       old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA
       GitPushService.new.execute(project, current_user, old_sha, sha, full_ref)
     end
+
+    def current_branch
+      @current_branch ||= params[:current_branch]
+    end
+
+    def target_branch
+      @target_branch ||= params[:target_branch]
+    end
+
+    def raise_error(message)
+      raise ValidationError.new(message)
+    end
+
+    def validate
+      allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(@target_branch)
+
+      unless allowed
+        raise_error("You are not allowed to push into this branch")
+      end
+
+      unless project.empty_repo?
+        unless repository.branch_names.include?(@current_branch)
+          raise_error("You can only create files if you are on top of a branch")
+        end
+
+        if @current_branch != @target_branch
+          if repository.branch_names.include?(@target_branch)
+            raise_error("Branch with such name already exists. You need to switch to this branch in order to make changes")
+          end
+        end
+      end
+    end
+
+    def create_target_branch
+      result = CreateBranchService.new(project, current_user).execute(@target_branch, @current_branch)
+
+      unless result[:status] == :success
+        raise_error("Something went wrong when we tried to create #{@target_branch} for you")
+      end
+    end
   end
 end

app/services/files/create_service.rb

@@ -2,58 +2,28 @@ require_relative "base_service"
 module Files
   class CreateService < Files::BaseService
-    def execute
-      allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
-      unless allowed
-        return error("You are not allowed to create file in this branch")
-      end
-      file_name = File.basename(path)
-      file_path = path
+    def commit
+      repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch)
+    end
+    def validate
+      super
+      file_name = File.basename(@file_path)
       unless file_name =~ Gitlab::Regex.file_name_regex
-        return error(
+        raise_error(
           'Your changes could not be committed, because the file name ' +
           Gitlab::Regex.file_name_regex_message
         )
       end
-      if project.empty_repo?
-        # everything is ok because repo does not have a commits yet
-      else
-        unless repository.branch_names.include?(ref)
-          return error("You can only create files if you are on top of a branch")
-        end
-
-        blob = repository.blob_at_branch(ref, file_path)
+      unless project.empty_repo?
+        blob = repository.blob_at_branch(@current_branch, @file_path)
         if blob
-          return error("Your changes could not be committed, because file with such name exists")
-        end
-      end
-
-      content =
-        if params[:encoding] == 'base64'
-          Base64.decode64(params[:content])
-        else
-          params[:content]
+          raise_error("Your changes could not be committed, because file with such name exists")
         end
-
-      sha = repository.commit_file(
-        current_user,
-        file_path,
-        content,
-        params[:commit_message],
-        params[:new_branch] || ref
-      )
-
-
-      if sha
-        after_commit(sha)
-        success
-      else
-        error("Your changes could not be committed, because the file has been changed")
       end
     end
   end

app/services/files/delete_service.rb

@@ -2,36 +2,8 @@ require_relative "base_service"
 module Files
   class DeleteService < Files::BaseService
-    def execute
-      allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
-
-      unless allowed
-        return error("You are not allowed to push into this branch")
-      end
-
-      unless repository.branch_names.include?(ref)
-        return error("You can only create files if you are on top of a branch")
-      end
-
-      blob = repository.blob_at_branch(ref, path)
-
-      unless blob
-        return error("You can only edit text files")
-      end
-
-      sha = repository.remove_file(
-        current_user,
-        path,
-        params[:commit_message],
-        ref
-      )
-
-      if sha
-        after_commit(sha)
-        success
-      else
-        error("Your changes could not be committed, because the file has been changed")
-      end
+    def commit
+      repository.remove_file(current_user, @file_path, @commit_message, @target_branch)
     end
   end
 end

app/services/files/update_service.rb

@@ -2,46 +2,8 @@ require_relative "base_service"
 module Files
   class UpdateService < Files::BaseService
-    def execute
-      allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
-
-      unless allowed
-        return error("You are not allowed to push into this branch")
-      end
-
-      unless repository.branch_names.include?(ref)
-        return error("You can only create files if you are on top of a branch")
-      end
-
-      blob = repository.blob_at_branch(ref, path)
-
-      unless blob
-        return error("You can only edit text files")
-      end
-
-      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[: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)
-    rescue Gitlab::Satellite::CommitFailed => ex
-      error("Your changes could not be committed. Maybe there was nothing to commit?", 409)
-    rescue Gitlab::Satellite::PushFailed => ex
-      error("Your changes could not be committed. Maybe the file was changed by another process?", 409)
+    def commit
+      repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch)
     end
   end
 end

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

@@ -6,11 +6,12 @@
     = render 'shared/commit_message_container', params: params,
              placeholder: 'Add new file'
-    .form-group.branch
-      = label_tag 'branch', class: 'control-label' do
-        Branch
-      .col-sm-10
-        = text_field_tag 'new_branch', @ref, class: "form-control"
+    - unless @project.empty_repo?
+      .form-group.branch
+        = label_tag 'branch', class: 'control-label' do
+          Branch
+        .col-sm-10
+          = text_field_tag 'new_branch', @ref, class: "form-control"
     = hidden_field_tag 'content', '', id: 'file-content'
     = render 'projects/commit_button', ref: @ref,