From d170133bded904603833f1b9e2727def3805962d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 20 Apr 2017 00:37:44 +0000 Subject: Refactor changing files in web UI --- app/assets/javascripts/blob/blob_file_dropzone.js | 2 +- app/controllers/concerns/creates_commit.rb | 62 +++++----- app/controllers/projects/application_controller.rb | 5 - app/controllers/projects/blob_controller.rb | 18 +-- app/controllers/projects/commit_controller.rb | 10 +- app/controllers/projects/tree_controller.rb | 6 +- app/helpers/projects_helper.rb | 4 +- app/helpers/tree_helper.rb | 2 +- app/services/commits/change_service.rb | 52 +-------- app/services/commits/cherry_pick_service.rb | 2 +- app/services/commits/create_service.rb | 74 ++++++++++++ app/services/commits/revert_service.rb | 2 +- app/services/files/base_service.rb | 80 ++----------- app/services/files/create_dir_service.rb | 15 +-- app/services/files/create_service.rb | 36 +----- app/services/files/delete_service.rb | 15 +++ app/services/files/destroy_service.rb | 15 --- app/services/files/multi_service.rb | 125 ++------------------- app/services/files/update_service.rb | 30 +++-- app/services/validate_new_branch_service.rb | 5 +- app/views/projects/blob/edit.html.haml | 2 +- app/views/projects/show.html.haml | 2 +- app/views/shared/_branch_switcher.html.haml | 6 +- app/views/shared/_new_commit_form.html.haml | 6 +- features/project/merge_requests/revert.feature | 2 - features/project/source/browse_files.feature | 2 +- features/steps/project/source/browse_files.rb | 6 +- lib/api/commits.rb | 4 +- lib/api/files.rb | 4 +- lib/api/v3/commits.rb | 4 +- lib/api/v3/files.rb | 4 +- lib/gitlab/git/index.rb | 49 +++++--- lib/gitlab/regex.rb | 16 --- spec/controllers/projects/blob_controller_spec.rb | 4 +- spec/controllers/projects/tree_controller_spec.rb | 10 +- spec/features/projects/blobs/user_create_spec.rb | 4 +- .../projects/files/creating_a_file_spec.rb | 6 +- .../features/projects/files/editing_a_file_spec.rb | 2 +- spec/features/projects/view_on_env_spec.rb | 4 +- spec/lib/gitlab/diff/position_tracer_spec.rb | 8 +- spec/lib/gitlab/git/index_spec.rb | 20 +++- spec/lib/gitlab/git_access_spec.rb | 2 +- spec/lib/gitlab/regex_spec.rb | 6 - spec/requests/api/commits_spec.rb | 3 +- spec/requests/api/files_spec.rb | 6 +- spec/requests/api/v3/commits_spec.rb | 3 +- spec/requests/api/v3/files_spec.rb | 6 +- spec/services/files/update_service_spec.rb | 6 +- 48 files changed, 286 insertions(+), 471 deletions(-) create mode 100644 app/services/commits/create_service.rb create mode 100644 app/services/files/delete_service.rb delete mode 100644 app/services/files/destroy_service.rb diff --git a/app/assets/javascripts/blob/blob_file_dropzone.js b/app/assets/javascripts/blob/blob_file_dropzone.js index c9fe23aec75..4568b86f298 100644 --- a/app/assets/javascripts/blob/blob_file_dropzone.js +++ b/app/assets/javascripts/blob/blob_file_dropzone.js @@ -35,7 +35,7 @@ export default class BlobFileDropzone { this.removeFile(file); }); this.on('sending', function (file, xhr, formData) { - formData.append('target_branch', form.find('input[name="target_branch"]').val()); + formData.append('branch_name', form.find('input[name="branch_name"]').val()); formData.append('create_merge_request', form.find('.js-create-merge-request').val()); formData.append('commit_message', form.find('.js-commit-message').val()); }); diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 9ac8197e45a..183eb00ef67 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -1,17 +1,29 @@ module CreatesCommit extend ActiveSupport::Concern + def set_start_branch_to_branch_name + branch_exists = @repository.find_branch(@branch_name) + @start_branch = @branch_name if branch_exists + end + def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) - set_commit_variables + if can?(current_user, :push_code, @project) + @project_to_commit_into = @project + @branch_name ||= @ref + else + @project_to_commit_into = current_user.fork_of(@project) + @branch_name ||= @project_to_commit_into.repository.next_branch('patch') + end + + @start_branch ||= @ref || @branch_name commit_params = @commit_params.merge( - start_project: @mr_target_project, - start_branch: @mr_target_branch, - target_branch: @mr_source_branch + start_project: @project, + start_branch: @start_branch, + branch_name: @branch_name ) - result = service.new( - @mr_source_project, current_user, commit_params).execute + result = service.new(@project_to_commit_into, current_user, commit_params).execute if result[:status] == :success update_flash_notice(success_notice) @@ -72,30 +84,30 @@ module CreatesCommit def new_merge_request_path new_namespace_project_merge_request_path( - @mr_source_project.namespace, - @mr_source_project, + @project_to_commit_into.namespace, + @project_to_commit_into, merge_request: { - source_project_id: @mr_source_project.id, - target_project_id: @mr_target_project.id, - source_branch: @mr_source_branch, - target_branch: @mr_target_branch + source_project_id: @project_to_commit_into.id, + target_project_id: @project.id, + source_branch: @branch_name, + target_branch: @start_branch } ) end def existing_merge_request_path - namespace_project_merge_request_path(@mr_target_project.namespace, @mr_target_project, @merge_request) + namespace_project_merge_request_path(@project.namespace, @project, @merge_request) end def merge_request_exists? return @merge_request if defined?(@merge_request) - @merge_request = MergeRequestsFinder.new(current_user, project_id: @mr_target_project.id).execute.opened. - find_by(source_branch: @mr_source_branch, target_branch: @mr_target_branch, source_project_id: @mr_source_project) + @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened. + find_by(source_project_id: @project_to_commit_into, source_branch: @branch_name, target_branch: @start_branch) end def different_project? - @mr_source_project != @mr_target_project + @project_to_commit_into != @project end def create_merge_request? @@ -103,22 +115,6 @@ module CreatesCommit # as the target branch in the same project, # we don't want to create a merge request. params[:create_merge_request].present? && - (different_project? || @mr_target_branch != @mr_source_branch) - end - - def set_commit_variables - if can?(current_user, :push_code, @project) - @mr_source_project = @project - @target_branch ||= @ref - else - @mr_source_project = current_user.fork_of(@project) - @target_branch ||= @mr_source_project.repository.next_branch('patch') - end - - # Merge request to this project - @mr_target_project = @project - @mr_target_branch ||= @ref || @target_branch - - @mr_source_branch = @target_branch + (different_project? || @start_branch != @branch_name) end end diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index f1a93ccb3ad..e2f81b09adc 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -89,9 +89,4 @@ class Projects::ApplicationController < ApplicationController def builds_enabled return render_404 unless @project.feature_available?(:builds, current_user) end - - def update_ref - branch_exists = @repository.find_branch(@target_branch) - @ref = @target_branch if branch_exists - end end diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 73706bf8dae..9fce1db6742 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -25,10 +25,10 @@ class Projects::BlobController < Projects::ApplicationController end def create - update_ref + set_start_branch_to_branch_name create_commit(Files::CreateService, success_notice: "The file has been successfully created.", - success_path: -> { namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @file_path)) }, + success_path: -> { namespace_project_blob_path(@project.namespace, @project, File.join(@branch_name, @file_path)) }, failure_view: :new, failure_path: namespace_project_new_blob_path(@project.namespace, @project, @ref)) end @@ -69,10 +69,10 @@ class Projects::BlobController < Projects::ApplicationController end def destroy - create_commit(Files::DestroyService, success_notice: "The file has been successfully deleted.", - success_path: -> { namespace_project_tree_path(@project.namespace, @project, @target_branch) }, - failure_view: :show, - failure_path: namespace_project_blob_path(@project.namespace, @project, @id)) + create_commit(Files::DeleteService, success_notice: "The file has been successfully deleted.", + success_path: -> { namespace_project_tree_path(@project.namespace, @project, @branch_name) }, + failure_view: :show, + failure_path: namespace_project_blob_path(@project.namespace, @project, @id)) end def diff @@ -127,16 +127,16 @@ class Projects::BlobController < Projects::ApplicationController def after_edit_path from_merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.find_by(iid: params[:from_merge_request_iid]) - if from_merge_request && @target_branch == @ref + if from_merge_request && @branch_name == @ref diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) + "##{hexdigest(@path)}" else - namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path)) + namespace_project_blob_path(@project.namespace, @project, File.join(@branch_name, @path)) end end def editor_variables - @target_branch = params[:target_branch] + @branch_name = params[:branch_name] @file_path = if action_name.to_s == 'create' diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index d25bbddd1bb..2b5f0383ac1 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -56,9 +56,7 @@ class Projects::CommitController < Projects::ApplicationController return render_404 if @start_branch.blank? - @target_branch = create_new_branch? ? @commit.revert_branch_name : @start_branch - - @mr_target_branch = @start_branch + @branch_name = create_new_branch? ? @commit.revert_branch_name : @start_branch create_commit(Commits::RevertService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully reverted.", success_path: -> { successful_change_path }, failure_path: failed_change_path) @@ -69,9 +67,7 @@ class Projects::CommitController < Projects::ApplicationController return render_404 if @start_branch.blank? - @target_branch = create_new_branch? ? @commit.cherry_pick_branch_name : @start_branch - - @mr_target_branch = @start_branch + @branch_name = create_new_branch? ? @commit.cherry_pick_branch_name : @start_branch create_commit(Commits::CherryPickService, success_notice: "The #{@commit.change_type_title(current_user)} has been successfully cherry-picked.", success_path: -> { successful_change_path }, failure_path: failed_change_path) @@ -84,7 +80,7 @@ class Projects::CommitController < Projects::ApplicationController end def successful_change_path - referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @target_branch) + referenced_merge_request_url || namespace_project_commits_url(@project.namespace, @project, @branch_name) end def failed_change_path diff --git a/app/controllers/projects/tree_controller.rb b/app/controllers/projects/tree_controller.rb index 637b61504d8..5e2182c883e 100644 --- a/app/controllers/projects/tree_controller.rb +++ b/app/controllers/projects/tree_controller.rb @@ -34,16 +34,16 @@ class Projects::TreeController < Projects::ApplicationController def create_dir return render_404 unless @commit_params.values.all? - update_ref + set_start_branch_to_branch_name create_commit(Files::CreateDirService, success_notice: "The directory has been successfully created.", - success_path: namespace_project_tree_path(@project.namespace, @project, File.join(@target_branch, @dir_name)), + success_path: namespace_project_tree_path(@project.namespace, @project, File.join(@branch_name, @dir_name)), failure_path: namespace_project_tree_path(@project.namespace, @project, @ref)) end private def assign_dir_vars - @target_branch = params[:target_branch] + @branch_name = params[:branch_name] @dir_name = File.join(@path, params[:dir_name]) @commit_params = { diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 43669b6f356..5f97e6114ea 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -272,14 +272,14 @@ module ProjectsHelper end end - def add_special_file_path(project, file_name:, commit_message: nil, target_branch: nil, context: nil) + def add_special_file_path(project, file_name:, commit_message: nil, branch_name: nil, context: nil) namespace_project_new_blob_path( project.namespace, project, project.default_branch || 'master', file_name: file_name, commit_message: commit_message || "Add #{file_name.downcase}", - target_branch: target_branch, + branch_name: branch_name, context: context ) end diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index 4a76c679bad..f1dab60524e 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -35,7 +35,7 @@ module TreeHelper end def on_top_of_branch?(project = @project, ref = @ref) - project.repository.branch_names.include?(ref) + project.repository.branch_exists?(ref) end def can_edit_tree?(project = nil, ref = nil) diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb index 1297a792259..a48d6a976f0 100644 --- a/app/services/commits/change_service.rb +++ b/app/services/commits/change_service.rb @@ -1,69 +1,27 @@ module Commits - class ChangeService < ::BaseService - ValidationError = Class.new(StandardError) - ChangeError = Class.new(StandardError) + class ChangeService < Commits::CreateService + def initialize(*args) + super - def execute - @start_project = params[:start_project] || @project - @start_branch = params[:start_branch] - @target_branch = params[:target_branch] @commit = params[:commit] - - check_push_permissions - - commit - rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError, - ValidationError, ChangeError => ex - error(ex.message) end private - def commit - raise NotImplementedError - end - def commit_change(action) raise NotImplementedError unless repository.respond_to?(action) - validate_target_branch if different_branch? - repository.public_send( action, current_user, @commit, - @target_branch, + @branch_name, start_project: @start_project, start_branch_name: @start_branch) - - success rescue Repository::CreateTreeError error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title(current_user)} automatically. - A #{action.to_s.dasherize} may have already been performed with this #{@commit.change_type_title(current_user)}, or a more recent commit may have updated some of its content." + This #{@commit.change_type_title(current_user)} may already have been #{action.to_s.dasherize}ed, or a more recent commit may have updated some of its content." raise ChangeError, error_msg end - - def check_push_permissions - allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) - - unless allowed - raise ValidationError.new('You are not allowed to push into this branch') - end - - true - end - - def validate_target_branch - result = ValidateNewBranchService.new(@project, current_user) - .execute(@target_branch) - - if result[:status] == :error - raise ChangeError, "There was an error creating the source branch: #{result[:message]}" - end - end - - def different_branch? - @start_branch != @target_branch || @start_project != @project - end end end diff --git a/app/services/commits/cherry_pick_service.rb b/app/services/commits/cherry_pick_service.rb index 605cca36f9c..320e229560d 100644 --- a/app/services/commits/cherry_pick_service.rb +++ b/app/services/commits/cherry_pick_service.rb @@ -1,6 +1,6 @@ module Commits class CherryPickService < ChangeService - def commit + def create_commit! commit_change(:cherry_pick) end end diff --git a/app/services/commits/create_service.rb b/app/services/commits/create_service.rb new file mode 100644 index 00000000000..c58f04a252b --- /dev/null +++ b/app/services/commits/create_service.rb @@ -0,0 +1,74 @@ +module Commits + class CreateService < ::BaseService + ValidationError = Class.new(StandardError) + ChangeError = Class.new(StandardError) + + def initialize(*args) + super + + @start_project = params[:start_project] || @project + @start_branch = params[:start_branch] + @branch_name = params[:branch_name] + end + + def execute + validate! + + new_commit = create_commit! + + success(result: new_commit) + rescue ValidationError, ChangeError, Gitlab::Git::Index::IndexError, Repository::CommitError, GitHooksService::PreReceiveError => ex + error(ex.message) + end + + private + + def create_commit! + raise NotImplementedError + end + + def raise_error(message) + raise ValidationError, message + end + + def different_branch? + @start_branch != @branch_name || @start_project != @project + end + + def validate! + validate_permissions! + validate_on_branch! + validate_branch_existance! + + validate_new_branch_name! if different_branch? + end + + def validate_permissions! + allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@branch_name) + + unless allowed + raise_error("You are not allowed to push into this branch") + end + end + + def validate_on_branch! + if !@start_project.empty_repo? && !@start_project.repository.branch_exists?(@start_branch) + raise_error('You can only create or edit files when you are on a branch') + end + end + + def validate_branch_existance! + if !project.empty_repo? && different_branch? && repository.branch_exists?(@branch_name) + raise_error("A branch called '#{@branch_name}' already exists. Switch to that branch in order to make changes") + end + end + + def validate_new_branch_name! + result = ValidateNewBranchService.new(project, current_user).execute(@branch_name) + + if result[:status] == :error + raise_error("Something went wrong when we tried to create '#{@branch_name}' for you: #{result[:message]}") + end + end + end +end diff --git a/app/services/commits/revert_service.rb b/app/services/commits/revert_service.rb index addd55cb32f..dc27399e047 100644 --- a/app/services/commits/revert_service.rb +++ b/app/services/commits/revert_service.rb @@ -1,6 +1,6 @@ module Commits class RevertService < ChangeService - def commit + def create_commit! commit_change(:revert) end end diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index c8a60422bf4..38231f66009 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -1,79 +1,17 @@ module Files - class BaseService < ::BaseService - ValidationError = Class.new(StandardError) - - def execute - @start_project = params[:start_project] || @project - @start_branch = params[:start_branch] - @target_branch = params[:target_branch] + class BaseService < Commits::CreateService + def initialize(*args) + super + @author_email = params[:author_email] + @author_name = params[:author_name] @commit_message = params[:commit_message] - @file_path = params[:file_path] - @previous_path = params[:previous_path] - @file_content = if params[:file_content_encoding] == 'base64' - Base64.decode64(params[:file_content]) - else - params[:file_content] - end - @last_commit_sha = params[:last_commit_sha] - @author_email = params[:author_email] - @author_name = params[:author_name] - - # Validate parameters - validate - - # Create new branch if it different from start_branch - validate_target_branch if different_branch? - - result = commit - if result - success(result: result) - else - error('Something went wrong. Your changes were not committed') - end - rescue Repository::CommitError, Gitlab::Git::Repository::InvalidBlobName, GitHooksService::PreReceiveError, ValidationError => ex - error(ex.message) - end - - private - - def different_branch? - @start_branch != @target_branch || @start_project != @project - end - - def file_has_changed? - return false unless @last_commit_sha && last_commit - - @last_commit_sha != last_commit.sha - end - - def raise_error(message) - raise ValidationError.new(message) - end - - def validate - allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch) - - unless allowed - raise_error("You are not allowed to push into this branch") - end - - if !@start_project.empty_repo? && !@start_project.repository.branch_exists?(@start_branch) - raise ValidationError, 'You can only create or edit files when you are on a branch' - end - - if !project.empty_repo? && different_branch? && repository.branch_exists?(@branch_name) - raise ValidationError, "A branch called #{@branch_name} already exists. Switch to that branch in order to make changes" - end - end - def validate_target_branch - result = ValidateNewBranchService.new(project, current_user). - execute(@target_branch) + @file_path = params[:file_path] + @previous_path = params[:previous_path] - if result[:status] == :error - raise_error("Something went wrong when we tried to create #{@target_branch} for you: #{result[:message]}") - end + @file_content = params[:file_content] + @file_content = Base64.decode64(@file_content) if params[:file_content_encoding] == 'base64' end end end diff --git a/app/services/files/create_dir_service.rb b/app/services/files/create_dir_service.rb index 083ffdc634c..8ecac6115bd 100644 --- a/app/services/files/create_dir_service.rb +++ b/app/services/files/create_dir_service.rb @@ -1,26 +1,15 @@ module Files class CreateDirService < Files::BaseService - def commit + def create_commit! repository.create_dir( current_user, @file_path, message: @commit_message, - branch_name: @target_branch, + branch_name: @branch_name, author_email: @author_email, author_name: @author_name, start_project: @start_project, start_branch_name: @start_branch) end - - def validate - super - - unless @file_path =~ Gitlab::Regex.file_path_regex - raise_error( - 'Your changes could not be committed, because the file path ' + - Gitlab::Regex.file_path_regex_message - ) - end - end end end diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index 65b5537fb68..00a8dcf0934 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -1,48 +1,16 @@ module Files class CreateService < Files::BaseService - def commit + def create_commit! repository.create_file( current_user, @file_path, @file_content, message: @commit_message, - branch_name: @target_branch, + branch_name: @branch_name, author_email: @author_email, author_name: @author_name, start_project: @start_project, start_branch_name: @start_branch) end - - def validate - super - - if @file_content.nil? - raise_error("You must provide content.") - end - - if @file_path =~ Gitlab::Regex.directory_traversal_regex - raise_error( - 'Your changes could not be committed, because the file name ' + - Gitlab::Regex.directory_traversal_regex_message - ) - end - - unless @file_path =~ Gitlab::Regex.file_path_regex - raise_error( - 'Your changes could not be committed, because the file name ' + - Gitlab::Regex.file_path_regex_message - ) - end - - unless project.empty_repo? - @file_path.slice!(0) if @file_path.start_with?('/') - - blob = repository.blob_at_branch(@start_branch, @file_path) - - if blob - raise_error('Your changes could not be committed because a file with the same name already exists') - end - end - end end end diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb new file mode 100644 index 00000000000..7952e5c95d4 --- /dev/null +++ b/app/services/files/delete_service.rb @@ -0,0 +1,15 @@ +module Files + class DeleteService < Files::BaseService + def create_commit! + repository.delete_file( + current_user, + @file_path, + message: @commit_message, + branch_name: @branch_name, + author_email: @author_email, + author_name: @author_name, + start_project: @start_project, + start_branch_name: @start_branch) + end + end +end diff --git a/app/services/files/destroy_service.rb b/app/services/files/destroy_service.rb deleted file mode 100644 index e294659bc98..00000000000 --- a/app/services/files/destroy_service.rb +++ /dev/null @@ -1,15 +0,0 @@ -module Files - class DestroyService < Files::BaseService - def commit - repository.delete_file( - current_user, - @file_path, - message: @commit_message, - branch_name: @target_branch, - author_email: @author_email, - author_name: @author_name, - start_project: @start_project, - start_branch_name: @start_branch) - end - end -end diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb index 700f9f4f6f0..bfacc462847 100644 --- a/app/services/files/multi_service.rb +++ b/app/services/files/multi_service.rb @@ -1,14 +1,10 @@ module Files class MultiService < Files::BaseService - FileChangedError = Class.new(StandardError) - - ACTIONS = %w[create update delete move].freeze - - def commit + def create_commit! repository.multi_action( user: current_user, message: @commit_message, - branch_name: @target_branch, + branch_name: @branch_name, actions: params[:actions], author_email: @author_email, author_name: @author_name, @@ -19,122 +15,17 @@ module Files private - def validate + def validate! super - params[:actions].each_with_index do |action, index| - if ACTIONS.include?(action[:action].to_s) - action[:action] = action[:action].to_sym - else - raise_error("Unknown action type `#{action[:action]}`.") - end - - unless action[:file_path].present? - raise_error("You must specify a file_path.") - end - - action[:file_path].slice!(0) if action[:file_path] && action[:file_path].start_with?('/') - action[:previous_path].slice!(0) if action[:previous_path] && action[:previous_path].start_with?('/') - - regex_check(action[:file_path]) - regex_check(action[:previous_path]) if action[:previous_path] - - if project.empty_repo? && action[:action] != :create - raise_error("No files to #{action[:action]}.") - end - - validate_file_exists(action) - - case action[:action] - when :create - validate_create(action) - when :update - validate_update(action) - when :delete - validate_delete(action) - when :move - validate_move(action, index) - end - end - end - - def validate_file_exists(action) - return if action[:action] == :create - - file_path = action[:file_path] - file_path = action[:previous_path] if action[:action] == :move - - blob = repository.blob_at_branch(params[:branch], file_path) - - unless blob - raise_error("File to be #{action[:action]}d `#{file_path}` does not exist.") + params[:actions].each do |action| + validate_action!(action) end end - def last_commit - Gitlab::Git::Commit.last_for_path(repository, @start_branch, @file_path) - end - - def regex_check(file) - if file =~ Gitlab::Regex.directory_traversal_regex - raise_error( - 'Your changes could not be committed, because the file name, `' + - file + - '` ' + - Gitlab::Regex.directory_traversal_regex_message - ) - end - - unless file =~ Gitlab::Regex.file_path_regex - raise_error( - 'Your changes could not be committed, because the file name, `' + - file + - '` ' + - Gitlab::Regex.file_path_regex_message - ) - end - end - - def validate_create(action) - return if project.empty_repo? - - if repository.blob_at_branch(params[:branch], action[:file_path]) - raise_error("Your changes could not be committed because a file with the name `#{action[:file_path]}` already exists.") - end - - if action[:content].nil? - raise_error("You must provide content.") - end - end - - def validate_update(action) - if action[:content].nil? - raise_error("You must provide content.") - end - - if file_has_changed? - raise FileChangedError.new("You are attempting to update a file `#{action[:file_path]}` that has changed since you started editing it.") - end - end - - def validate_delete(action) - end - - def validate_move(action, index) - if action[:previous_path].nil? - raise_error("You must supply the original file path when moving file `#{action[:file_path]}`.") - end - - blob = repository.blob_at_branch(params[:branch], action[:file_path]) - - if blob - raise_error("Move destination `#{action[:file_path]}` already exists.") - end - - if action[:content].nil? - blob = repository.blob_at_branch(params[:branch], action[:previous_path]) - blob.load_all_data!(repository) if blob.truncated? - params[:actions][index][:content] = blob.data + def validate_action!(action) + unless Gitlab::Git::Index::ACTIONS.include?(action[:action].to_s) + raise_error("Unknown action '#{action[:action]}'") end end end diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index fbbab97632e..f23a9f6d57c 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -2,10 +2,16 @@ module Files class UpdateService < Files::BaseService FileChangedError = Class.new(StandardError) - def commit + def initialize(*args) + super + + @last_commit_sha = params[:last_commit_sha] + end + + def create_commit! repository.update_file(current_user, @file_path, @file_content, message: @commit_message, - branch_name: @target_branch, + branch_name: @branch_name, previous_path: @previous_path, author_email: @author_email, author_name: @author_name, @@ -15,21 +21,23 @@ module Files private - def validate - super - - if @file_content.nil? - raise_error("You must provide content.") - end + def file_has_changed? + return false unless @last_commit_sha && last_commit - if file_has_changed? - raise FileChangedError.new("You are attempting to update a file that has changed since you started editing it.") - end + @last_commit_sha != last_commit.sha end def last_commit @last_commit ||= Gitlab::Git::Commit. last_for_path(@start_project.repository, @start_branch, @file_path) end + + def validate! + super + + if file_has_changed? + raise FileChangedError, "You are attempting to update a file that has changed since you started editing it." + end + end end end diff --git a/app/services/validate_new_branch_service.rb b/app/services/validate_new_branch_service.rb index 2f61be184ce..d232e85cd33 100644 --- a/app/services/validate_new_branch_service.rb +++ b/app/services/validate_new_branch_service.rb @@ -8,10 +8,7 @@ class ValidateNewBranchService < BaseService return error('Branch name is invalid') end - repository = project.repository - existing_branch = repository.find_branch(branch_name) - - if existing_branch + if project.repository.branch_exists?(branch_name) return error('Branch already exists') end diff --git a/app/views/projects/blob/edit.html.haml b/app/views/projects/blob/edit.html.haml index 4b26f944733..4af62461151 100644 --- a/app/views/projects/blob/edit.html.haml +++ b/app/views/projects/blob/edit.html.haml @@ -9,7 +9,7 @@ - if @conflict .alert.alert-danger Someone edited the file the same time you did. Please check out - = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@target_branch, @file_path)), target: "_blank", rel: 'noopener noreferrer' + = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@branch_name, @file_path)), target: "_blank", rel: 'noopener noreferrer' and make sure your changes will not unintentionally remove theirs. .editor-title-row %h3.page-title.blob-edit-page-title diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index fd7bd21677c..d6c4195e2d0 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -70,7 +70,7 @@ = link_to 'Set up Koding', add_koding_stack_path(@project) - if @repository.gitlab_ci_yml.blank? && @project.deployment_service.present? %li.missing - = link_to add_special_file_path(@project, file_name: '.gitlab-ci.yml', commit_message: 'Set up auto deploy', target_branch: 'auto-deploy', context: 'autodeploy') do + = link_to add_special_file_path(@project, file_name: '.gitlab-ci.yml', commit_message: 'Set up auto deploy', branch_name: 'auto-deploy', context: 'autodeploy') do Set up auto deploy - if @repository.commit diff --git a/app/views/shared/_branch_switcher.html.haml b/app/views/shared/_branch_switcher.html.haml index 7799aff6b5b..69e3f3042a9 100644 --- a/app/views/shared/_branch_switcher.html.haml +++ b/app/views/shared/_branch_switcher.html.haml @@ -1,8 +1,8 @@ -- dropdown_toggle_text = @target_branch || tree_edit_branch -= hidden_field_tag 'target_branch', dropdown_toggle_text +- dropdown_toggle_text = @branch_name || tree_edit_branch += hidden_field_tag 'branch_name', dropdown_toggle_text .dropdown - = dropdown_toggle dropdown_toggle_text, { toggle: 'dropdown', selected: dropdown_toggle_text, field_name: 'target_branch', form_id: '.js-edit-blob-form', refs_url: namespace_project_branches_path(@project.namespace, @project) }, { toggle_class: 'js-project-branches-dropdown js-target-branch' } + = dropdown_toggle dropdown_toggle_text, { toggle: 'dropdown', selected: dropdown_toggle_text, field_name: 'branch_name', form_id: '.js-edit-blob-form', refs_url: namespace_project_branches_path(@project.namespace, @project) }, { toggle_class: 'js-project-branches-dropdown js-target-branch' } .dropdown-menu.dropdown-menu-selectable.dropdown-menu-paging.dropdown-menu-branches = render partial: 'shared/projects/blob/branch_page_default' = render partial: 'shared/projects/blob/branch_page_create' diff --git a/app/views/shared/_new_commit_form.html.haml b/app/views/shared/_new_commit_form.html.haml index 3ac5e15d1c4..0b37fe3013b 100644 --- a/app/views/shared/_new_commit_form.html.haml +++ b/app/views/shared/_new_commit_form.html.haml @@ -1,11 +1,11 @@ = render 'shared/commit_message_container', placeholder: placeholder - if @project.empty_repo? - = hidden_field_tag 'target_branch', @ref + = hidden_field_tag 'branch_name', @ref - else - if can?(current_user, :push_code, @project) .form-group.branch - = label_tag 'target_branch', 'Target branch', class: 'control-label' + = label_tag 'branch_name', 'Target branch', class: 'control-label' .col-sm-10 = render 'shared/branch_switcher' @@ -16,7 +16,7 @@ = check_box_tag 'create_merge_request', 1, true, class: 'js-create-merge-request', id: "create_merge_request-#{nonce}" Start a new merge request with these changes - else - = hidden_field_tag 'target_branch', @target_branch || tree_edit_branch + = hidden_field_tag 'branch_name', @branch_name || tree_edit_branch = hidden_field_tag 'create_merge_request', 1 = hidden_field_tag 'original_branch', @ref, class: 'js-original-branch' diff --git a/features/project/merge_requests/revert.feature b/features/project/merge_requests/revert.feature index ec6666f227f..aaac5fd7209 100644 --- a/features/project/merge_requests/revert.feature +++ b/features/project/merge_requests/revert.feature @@ -25,7 +25,5 @@ Feature: Revert Merge Requests @javascript Scenario: I revert a merge request in a new merge request Given I click on the revert button - And I am on the Merge Request detail page - And I click on the revert button And I revert the changes in a new merge request Then I should see the new merge request notice diff --git a/features/project/source/browse_files.feature b/features/project/source/browse_files.feature index 894c4a96bb8..536c24b6882 100644 --- a/features/project/source/browse_files.feature +++ b/features/project/source/browse_files.feature @@ -135,7 +135,7 @@ Feature: Project Source Browse Files And I fill the commit message And I click on "Commit changes" Then I am on the new file page - And I see a commit error message + And I see "Path can contain only..." @javascript Scenario: I can create file with a directory name diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb index 5bd3c1a1246..d52fa10c337 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -284,7 +284,11 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps end step 'I see "Unable to create directory"' do - expect(page).to have_content('Directory already exists') + expect(page).to have_content('A directory with this name already exists') + end + + step 'I see "Path can contain only..."' do + expect(page).to have_content('Path can contain only') end step 'I see a commit error message' do diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 66b37fd2bcc..621b9dcecd9 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -62,7 +62,7 @@ module API post ":id/repository/commits" do authorize! :push_code, user_project - attrs = declared_params.merge(start_branch: declared_params[:branch], target_branch: declared_params[:branch]) + attrs = declared_params.merge(start_branch: declared_params[:branch], branch_name: declared_params[:branch]) result = ::Files::MultiService.new(user_project, current_user, attrs).execute @@ -140,7 +140,7 @@ module API commit_params = { commit: commit, start_branch: params[:branch], - target_branch: params[:branch] + branch_name: params[:branch] } result = ::Commits::CherryPickService.new(user_project, current_user, commit_params).execute diff --git a/lib/api/files.rb b/lib/api/files.rb index 33fc970dc09..e6ea12c5ab7 100644 --- a/lib/api/files.rb +++ b/lib/api/files.rb @@ -5,7 +5,7 @@ module API { file_path: attrs[:file_path], start_branch: attrs[:branch], - target_branch: attrs[:branch], + branch_name: attrs[:branch], commit_message: attrs[:commit_message], file_content: attrs[:content], file_content_encoding: attrs[:encoding], @@ -130,7 +130,7 @@ module API authorize! :push_code, user_project file_params = declared_params(include_missing: false) - result = ::Files::DestroyService.new(user_project, current_user, commit_params(file_params)).execute + result = ::Files::DeleteService.new(user_project, current_user, commit_params(file_params)).execute if result[:status] != :success render_api_error!(result[:message], 400) diff --git a/lib/api/v3/commits.rb b/lib/api/v3/commits.rb index 3414a2883e5..674de592f0a 100644 --- a/lib/api/v3/commits.rb +++ b/lib/api/v3/commits.rb @@ -53,7 +53,7 @@ module API attrs = declared_params.dup branch = attrs.delete(:branch_name) - attrs.merge!(branch: branch, start_branch: branch, target_branch: branch) + attrs.merge!(start_branch: branch, branch_name: branch) result = ::Files::MultiService.new(user_project, current_user, attrs).execute @@ -131,7 +131,7 @@ module API commit_params = { commit: commit, start_branch: params[:branch], - target_branch: params[:branch] + branch_name: params[:branch] } result = ::Commits::CherryPickService.new(user_project, current_user, commit_params).execute diff --git a/lib/api/v3/files.rb b/lib/api/v3/files.rb index 13542b0c71c..c76acc86504 100644 --- a/lib/api/v3/files.rb +++ b/lib/api/v3/files.rb @@ -6,7 +6,7 @@ module API { file_path: attrs[:file_path], start_branch: attrs[:branch], - target_branch: attrs[:branch], + branch_name: attrs[:branch], commit_message: attrs[:commit_message], file_content: attrs[:content], file_content_encoding: attrs[:encoding], @@ -123,7 +123,7 @@ module API file_params = declared_params(include_missing: false) file_params[:branch] = file_params.delete(:branch_name) - result = ::Files::DestroyService.new(user_project, current_user, commit_params(file_params)).execute + result = ::Files::DeleteService.new(user_project, current_user, commit_params(file_params)).execute if result[:status] == :success status(200) diff --git a/lib/gitlab/git/index.rb b/lib/gitlab/git/index.rb index af1744c9c46..1add037fa5f 100644 --- a/lib/gitlab/git/index.rb +++ b/lib/gitlab/git/index.rb @@ -1,8 +1,12 @@ module Gitlab module Git class Index + IndexError = Class.new(StandardError) + DEFAULT_MODE = 0o100644 + ACTIONS = %w(create create_dir update move delete).freeze + attr_reader :repository, :raw_index def initialize(repository) @@ -23,9 +27,8 @@ module Gitlab def create(options) options = normalize_options(options) - file_entry = get(options[:file_path]) - if file_entry - raise Gitlab::Git::Repository::InvalidBlobName.new("Filename already exists") + if get(options[:file_path]) + raise IndexError, "A file with this name already exists" end add_blob(options) @@ -34,13 +37,12 @@ module Gitlab def create_dir(options) options = normalize_options(options) - file_entry = get(options[:file_path]) - if file_entry - raise Gitlab::Git::Repository::InvalidBlobName.new("Directory already exists as a file") + if get(options[:file_path]) + raise IndexError, "A file with this name already exists" end if dir_exists?(options[:file_path]) - raise Gitlab::Git::Repository::InvalidBlobName.new("Directory already exists") + raise IndexError, "A directory with this name already exists" end options = options.dup @@ -55,7 +57,7 @@ module Gitlab file_entry = get(options[:file_path]) unless file_entry - raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist") + raise IndexError, "A file with this name doesn't exist" end add_blob(options, mode: file_entry[:mode]) @@ -66,7 +68,11 @@ module Gitlab file_entry = get(options[:previous_path]) unless file_entry - raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist") + raise IndexError, "A file with this name doesn't exist" + end + + if get(options[:file_path]) + raise IndexError, "A file with this name already exists" end raw_index.remove(options[:previous_path]) @@ -77,9 +83,8 @@ module Gitlab def delete(options) options = normalize_options(options) - file_entry = get(options[:file_path]) - unless file_entry - raise Gitlab::Git::Repository::InvalidBlobName.new("File doesn't exist") + unless get(options[:file_path]) + raise IndexError, "A file with this name doesn't exist" end raw_index.remove(options[:file_path]) @@ -95,10 +100,20 @@ module Gitlab end def normalize_path(path) + unless path + raise IndexError, "You must provide a file path" + end + pathname = Gitlab::Git::PathHelper.normalize_path(path.dup) - if pathname.each_filename.include?('..') - raise Gitlab::Git::Repository::InvalidBlobName.new('Invalid path') + pathname.each_filename do |segment| + if segment == '..' + raise IndexError, 'Path cannot include directory traversal' + end + + unless segment =~ Gitlab::Regex.file_name_regex + raise IndexError, "Path #{Gitlab::Regex.file_name_regex_message}" + end end pathname.to_s @@ -106,6 +121,10 @@ module Gitlab def add_blob(options, mode: nil) content = options[:content] + unless content + raise IndexError, "You must provide content" + end + content = Base64.decode64(content) if options[:encoding] == 'base64' detect = CharlockHolmes::EncodingDetector.new.detect(content) @@ -119,7 +138,7 @@ module Gitlab raw_index.add(path: options[:file_path], oid: oid, mode: mode || DEFAULT_MODE) rescue Rugged::IndexError => e - raise Gitlab::Git::Repository::InvalidBlobName.new(e.message) + raise IndexError, e.message end end end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index e599dd4a656..08b061d5e31 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -73,22 +73,6 @@ module Gitlab "can contain only letters, digits, '_', '-', '@', '+' and '.'." end - def file_path_regex - @file_path_regex ||= /\A[[[:alnum:]]_\-\.\/\@]*\z/.freeze - end - - def file_path_regex_message - "can contain only letters, digits, '_', '-', '@' and '.'. Separate directories with a '/'." - end - - def directory_traversal_regex - @directory_traversal_regex ||= /\.{2}/.freeze - end - - def directory_traversal_regex_message - "cannot include directory traversal." - end - def archive_formats_regex # |zip|tar| tar.gz | tar.bz2 | @archive_formats_regex ||= /(zip|tar|tar\.gz|tgz|gz|tar\.bz2|tbz|tbz2|tb2|bz2)/.freeze diff --git a/spec/controllers/projects/blob_controller_spec.rb b/spec/controllers/projects/blob_controller_spec.rb index 3e9f272a0d8..0fd09d156c4 100644 --- a/spec/controllers/projects/blob_controller_spec.rb +++ b/spec/controllers/projects/blob_controller_spec.rb @@ -106,7 +106,7 @@ describe Projects::BlobController do namespace_id: project.namespace, project_id: project, id: 'master/CHANGELOG', - target_branch: 'master', + branch_name: 'master', content: 'Added changes', commit_message: 'Update CHANGELOG' } @@ -178,7 +178,7 @@ describe Projects::BlobController do context 'when editing on the original repository' do it "redirects to forked project new merge request" do - default_params[:target_branch] = "fork-test-1" + default_params[:branch_name] = "fork-test-1" default_params[:create_merge_request] = 1 put :update, default_params diff --git a/spec/controllers/projects/tree_controller_spec.rb b/spec/controllers/projects/tree_controller_spec.rb index ab94e292e48..a43dad5756d 100644 --- a/spec/controllers/projects/tree_controller_spec.rb +++ b/spec/controllers/projects/tree_controller_spec.rb @@ -97,29 +97,29 @@ describe Projects::TreeController do project_id: project, id: 'master', dir_name: path, - target_branch: target_branch, + branch_name: branch_name, commit_message: 'Test commit message') end context 'successful creation' do let(:path) { 'files/new_dir'} - let(:target_branch) { 'master-test'} + let(:branch_name) { 'master-test'} it 'redirects to the new directory' do expect(subject). - to redirect_to("/#{project.path_with_namespace}/tree/#{target_branch}/#{path}") + to redirect_to("/#{project.path_with_namespace}/tree/#{branch_name}/#{path}") expect(flash[:notice]).to eq('The directory has been successfully created.') end end context 'unsuccessful creation' do let(:path) { 'README.md' } - let(:target_branch) { 'master'} + let(:branch_name) { 'master'} it 'does not allow overwriting of existing files' do expect(subject). to redirect_to("/#{project.path_with_namespace}/tree/master") - expect(flash[:alert]).to eq('Directory already exists as a file') + expect(flash[:alert]).to eq('A file with this name already exists') end end end diff --git a/spec/features/projects/blobs/user_create_spec.rb b/spec/features/projects/blobs/user_create_spec.rb index fa1a753afcb..6ea149956fe 100644 --- a/spec/features/projects/blobs/user_create_spec.rb +++ b/spec/features/projects/blobs/user_create_spec.rb @@ -77,7 +77,7 @@ feature 'New blob creation', feature: true, js: true do project, user, start_branch: 'master', - target_branch: 'master', + branch_name: 'master', commit_message: 'Create file', file_path: 'feature.rb', file_content: content @@ -87,7 +87,7 @@ feature 'New blob creation', feature: true, js: true do end scenario 'shows error message' do - expect(page).to have_content('Your changes could not be committed because a file with the same name already exists') + expect(page).to have_content('A file with this name already exists') expect(page).to have_content('New file') expect(page).to have_content('NextFeature') end diff --git a/spec/features/projects/files/creating_a_file_spec.rb b/spec/features/projects/files/creating_a_file_spec.rb index 5d7bd3dc4ce..de6905f2b58 100644 --- a/spec/features/projects/files/creating_a_file_spec.rb +++ b/spec/features/projects/files/creating_a_file_spec.rb @@ -29,16 +29,16 @@ feature 'User wants to create a file', feature: true do scenario 'directory name contains Chinese characters' do submit_new_file(file_name: '中文/测试.md') - expect(page).to have_content 'The file has been successfully created.' + expect(page).to have_content 'The file has been successfully created' end scenario 'file name contains invalid characters' do submit_new_file(file_name: '\\') - expect(page).to have_content 'Your changes could not be committed, because the file name can contain only' + expect(page).to have_content 'Path can contain only' end scenario 'file name contains directory traversal' do submit_new_file(file_name: '../README.md') - expect(page).to have_content 'Your changes could not be committed, because the file name cannot include directory traversal.' + expect(page).to have_content 'Path cannot include directory traversal' end end diff --git a/spec/features/projects/files/editing_a_file_spec.rb b/spec/features/projects/files/editing_a_file_spec.rb index 3e544316f28..4da34108b46 100644 --- a/spec/features/projects/files/editing_a_file_spec.rb +++ b/spec/features/projects/files/editing_a_file_spec.rb @@ -8,7 +8,7 @@ feature 'User wants to edit a file', feature: true do let(:commit_params) do { start_branch: project.default_branch, - target_branch: project.default_branch, + branch_name: project.default_branch, commit_message: "Committing First Update", file_path: ".gitignore", file_content: "First Update", diff --git a/spec/features/projects/view_on_env_spec.rb b/spec/features/projects/view_on_env_spec.rb index ce5c5f21167..34c6a10950f 100644 --- a/spec/features/projects/view_on_env_spec.rb +++ b/spec/features/projects/view_on_env_spec.rb @@ -25,7 +25,7 @@ describe 'View on environment', js: true do project, user, start_branch: branch_name, - target_branch: branch_name, + branch_name: branch_name, commit_message: "Add .gitlab/route-map.yml", file_path: '.gitlab/route-map.yml', file_content: route_map @@ -36,7 +36,7 @@ describe 'View on environment', js: true do project, user, start_branch: branch_name, - target_branch: branch_name, + branch_name: branch_name, commit_message: "Update feature", file_path: file_path, file_content: "# Noop" diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb index 994995b57b8..c166f83664a 100644 --- a/spec/lib/gitlab/diff/position_tracer_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer_spec.rb @@ -100,7 +100,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do project, current_user, start_branch: branch_name, - target_branch: branch_name, + branch_name: branch_name, commit_message: "Create file", file_path: file_name, file_content: content @@ -113,7 +113,7 @@ describe Gitlab::Diff::PositionTracer, lib: true do project, current_user, start_branch: branch_name, - target_branch: branch_name, + branch_name: branch_name, commit_message: "Update file", file_path: file_name, file_content: content @@ -122,11 +122,11 @@ describe Gitlab::Diff::PositionTracer, lib: true do end def delete_file(branch_name, file_name) - Files::DestroyService.new( + Files::DeleteService.new( project, current_user, start_branch: branch_name, - target_branch: branch_name, + branch_name: branch_name, commit_message: "Delete file", file_path: file_name ).execute diff --git a/spec/lib/gitlab/git/index_spec.rb b/spec/lib/gitlab/git/index_spec.rb index 07d71f6777d..21b71654251 100644 --- a/spec/lib/gitlab/git/index_spec.rb +++ b/spec/lib/gitlab/git/index_spec.rb @@ -33,7 +33,7 @@ describe Gitlab::Git::Index, seed_helper: true do end it 'raises an error' do - expect { index.create(options) }.to raise_error('Filename already exists') + expect { index.create(options) }.to raise_error('A file with this name already exists') end end @@ -89,7 +89,7 @@ describe Gitlab::Git::Index, seed_helper: true do end it 'raises an error' do - expect { index.create_dir(options) }.to raise_error('Directory already exists as a file') + expect { index.create_dir(options) }.to raise_error('A file with this name already exists') end end @@ -99,7 +99,7 @@ describe Gitlab::Git::Index, seed_helper: true do end it 'raises an error' do - expect { index.create_dir(options) }.to raise_error('Directory already exists') + expect { index.create_dir(options) }.to raise_error('A directory with this name already exists') end end end @@ -118,7 +118,7 @@ describe Gitlab::Git::Index, seed_helper: true do end it 'raises an error' do - expect { index.update(options) }.to raise_error("File doesn't exist") + expect { index.update(options) }.to raise_error("A file with this name doesn't exist") end end @@ -156,7 +156,15 @@ describe Gitlab::Git::Index, seed_helper: true do it 'raises an error' do options[:previous_path] = 'documents/story.txt' - expect { index.move(options) }.to raise_error("File doesn't exist") + expect { index.move(options) }.to raise_error("A file with this name doesn't exist") + end + end + + context 'when a file at the new path already exists' do + it 'raises an error' do + options[:file_path] = 'CHANGELOG' + + expect { index.move(options) }.to raise_error("A file with this name already exists") end end @@ -203,7 +211,7 @@ describe Gitlab::Git::Index, seed_helper: true do end it 'raises an error' do - expect { index.delete(options) }.to raise_error("File doesn't exist") + expect { index.delete(options) }.to raise_error("A file with this name doesn't exist") end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 703b41f95ac..d8b72615fab 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -211,7 +211,7 @@ describe Gitlab::GitAccess, lib: true do target_branch = project.repository.lookup('feature') source_branch = project.repository.create_file( user, - 'John Doe', + 'filename', 'This is the file content', message: 'This is a good commit message', branch_name: unprotected_branch) diff --git a/spec/lib/gitlab/regex_spec.rb b/spec/lib/gitlab/regex_spec.rb index ba45e2d758c..127cd8c78d8 100644 --- a/spec/lib/gitlab/regex_spec.rb +++ b/spec/lib/gitlab/regex_spec.rb @@ -32,12 +32,6 @@ describe Gitlab::Regex, lib: true do it { is_expected.to match('foo@bar') } end - describe '.file_path_regex' do - subject { described_class.file_path_regex } - - it { is_expected.to match('foo@/bar') } - end - describe '.environment_slug_regex' do subject { described_class.environment_slug_regex } diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index a10d876ffad..42dbab586cd 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -599,8 +599,7 @@ describe API::Commits, api: true do post api("/projects/#{project.id}/repository/commits/#{master_pickable_commit.id}/cherry_pick", user), branch: 'markdown' expect(response).to have_http_status(400) - expect(json_response['message']).to eq('Sorry, we cannot cherry-pick this commit automatically. - A cherry-pick may have already been performed with this commit, or a more recent commit may have updated some of its content.') + expect(json_response['message']).to include('Sorry, we cannot cherry-pick this commit automatically.') end it 'returns 400 if you are not allowed to push to the target branch' do diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 8012530f139..6db2faed76b 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -205,7 +205,7 @@ describe API::Files, api: true do it "returns a 400 if editor fails to create file" do allow_any_instance_of(Repository).to receive(:create_file). - and_return(false) + and_raise(Repository::CommitError, 'Cannot create file') post api(route("any%2Etxt"), user), valid_params @@ -299,8 +299,8 @@ describe API::Files, api: true do expect(response).to have_http_status(400) end - it "returns a 400 if fails to create file" do - allow_any_instance_of(Repository).to receive(:delete_file).and_return(false) + it "returns a 400 if fails to delete file" do + allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Repository::CommitError, 'Cannot delete file') delete api(route(file_path), user), valid_params diff --git a/spec/requests/api/v3/commits_spec.rb b/spec/requests/api/v3/commits_spec.rb index adba3a787aa..0a28cb9bddb 100644 --- a/spec/requests/api/v3/commits_spec.rb +++ b/spec/requests/api/v3/commits_spec.rb @@ -485,8 +485,7 @@ describe API::V3::Commits, api: true do post v3_api("/projects/#{project.id}/repository/commits/#{master_pickable_commit.id}/cherry_pick", user), branch: 'markdown' expect(response).to have_http_status(400) - expect(json_response['message']).to eq('Sorry, we cannot cherry-pick this commit automatically. - A cherry-pick may have already been performed with this commit, or a more recent commit may have updated some of its content.') + expect(json_response['message']).to include('Sorry, we cannot cherry-pick this commit automatically.') end it 'returns 400 if you are not allowed to push to the target branch' do diff --git a/spec/requests/api/v3/files_spec.rb b/spec/requests/api/v3/files_spec.rb index 349fd6b3415..c45e2028e1d 100644 --- a/spec/requests/api/v3/files_spec.rb +++ b/spec/requests/api/v3/files_spec.rb @@ -129,7 +129,7 @@ describe API::V3::Files, api: true do it "returns a 400 if editor fails to create file" do allow_any_instance_of(Repository).to receive(:create_file). - and_return(false) + and_raise(Repository::CommitError, 'Cannot create file') post v3_api("/projects/#{project.id}/repository/files", user), valid_params @@ -229,8 +229,8 @@ describe API::V3::Files, api: true do expect(response).to have_http_status(400) end - it "returns a 400 if fails to create file" do - allow_any_instance_of(Repository).to receive(:delete_file).and_return(false) + it "returns a 400 if fails to delete file" do + allow_any_instance_of(Repository).to receive(:delete_file).and_raise(Repository::CommitError, 'Cannot delete file') delete v3_api("/projects/#{project.id}/repository/files", user), valid_params diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb index 26aa5b432d4..16bca66766a 100644 --- a/spec/services/files/update_service_spec.rb +++ b/spec/services/files/update_service_spec.rb @@ -7,7 +7,7 @@ describe Files::UpdateService do let(:user) { create(:user) } let(:file_path) { 'files/ruby/popen.rb' } let(:new_contents) { 'New Content' } - let(:target_branch) { project.default_branch } + let(:branch_name) { project.default_branch } let(:last_commit_sha) { nil } let(:commit_params) do @@ -19,7 +19,7 @@ describe Files::UpdateService do last_commit_sha: last_commit_sha, start_project: project, start_branch: project.default_branch, - target_branch: target_branch + branch_name: branch_name } end @@ -73,7 +73,7 @@ describe Files::UpdateService do end context 'when target branch is different than source branch' do - let(:target_branch) { "#{project.default_branch}-new" } + let(:branch_name) { "#{project.default_branch}-new" } it 'fires hooks only once' do expect(GitHooksService).to receive(:new).once.and_call_original -- cgit v1.2.1