From dfccb06dda344819989fa8d6a9a3c56c5ca0b65f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 5 Jun 2015 15:01:38 +0200 Subject: 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 --- Gemfile | 2 +- Gemfile.lock | 4 +- app/controllers/projects/blob_controller.rb | 60 +++++++++++----------- app/services/files/base_service.rb | 77 ++++++++++++++++++++++++++--- app/services/files/create_service.rb | 50 ++++--------------- app/services/files/delete_service.rb | 32 +----------- app/services/files/update_service.rb | 42 +--------------- app/views/projects/blob/new.html.haml | 11 +++-- 8 files changed, 123 insertions(+), 155 deletions(-) diff --git a/Gemfile b/Gemfile index 0009a8affba..0c1fff0bc14 100644 --- a/Gemfile +++ b/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) diff --git a/Gemfile.lock b/Gemfile.lock index a341a5df409..9d87de7d4e0 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index b762518d377..100d3d3b317 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/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 diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 4d02752454e..f587ee266da 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -1,11 +1,34 @@ module Files class BaseService < ::BaseService - attr_reader :ref, :path + class ValidationError < StandardError; end - def initialize(project, user, params, ref, path = nil) - @project, @current_user, @params = project, user, params.dup - @ref = ref - @path = path + 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 diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index 0a80455bc6b..91d715b2d63 100644 --- a/app/services/files/create_service.rb +++ b/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) + def commit + repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch) + end - unless allowed - return error("You are not allowed to create file in this branch") - end + def validate + super - file_name = File.basename(path) - file_path = path + 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 diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index 2281777604c..27c881c3430 100644 --- a/app/services/files/delete_service.rb +++ b/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 diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 013cc1ee322..a20903c6f02 100644 --- a/app/services/files/update_service.rb +++ b/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 diff --git a/app/views/projects/blob/new.html.haml b/app/views/projects/blob/new.html.haml index 9b1d03b820e..f7ddf74b4fc 100644 --- a/app/views/projects/blob/new.html.haml +++ b/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, -- cgit v1.2.1