summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-04-20 00:37:44 +0000
committerRobert Speicher <robert@gitlab.com>2017-04-20 00:37:44 +0000
commitd170133bded904603833f1b9e2727def3805962d (patch)
treea1c1e1f9973b43a5ff2482d4e1b91b219c5ac3af
parentbfb635a4afea60751a44e6454f9c36850b5342c9 (diff)
downloadgitlab-ce-d170133bded904603833f1b9e2727def3805962d.tar.gz
Refactor changing files in web UI
-rw-r--r--app/assets/javascripts/blob/blob_file_dropzone.js2
-rw-r--r--app/controllers/concerns/creates_commit.rb62
-rw-r--r--app/controllers/projects/application_controller.rb5
-rw-r--r--app/controllers/projects/blob_controller.rb18
-rw-r--r--app/controllers/projects/commit_controller.rb10
-rw-r--r--app/controllers/projects/tree_controller.rb6
-rw-r--r--app/helpers/projects_helper.rb4
-rw-r--r--app/helpers/tree_helper.rb2
-rw-r--r--app/services/commits/change_service.rb52
-rw-r--r--app/services/commits/cherry_pick_service.rb2
-rw-r--r--app/services/commits/create_service.rb74
-rw-r--r--app/services/commits/revert_service.rb2
-rw-r--r--app/services/files/base_service.rb80
-rw-r--r--app/services/files/create_dir_service.rb15
-rw-r--r--app/services/files/create_service.rb36
-rw-r--r--app/services/files/delete_service.rb (renamed from app/services/files/destroy_service.rb)6
-rw-r--r--app/services/files/multi_service.rb125
-rw-r--r--app/services/files/update_service.rb30
-rw-r--r--app/services/validate_new_branch_service.rb5
-rw-r--r--app/views/projects/blob/edit.html.haml2
-rw-r--r--app/views/projects/show.html.haml2
-rw-r--r--app/views/shared/_branch_switcher.html.haml6
-rw-r--r--app/views/shared/_new_commit_form.html.haml6
-rw-r--r--features/project/merge_requests/revert.feature2
-rw-r--r--features/project/source/browse_files.feature2
-rw-r--r--features/steps/project/source/browse_files.rb6
-rw-r--r--lib/api/commits.rb4
-rw-r--r--lib/api/files.rb4
-rw-r--r--lib/api/v3/commits.rb4
-rw-r--r--lib/api/v3/files.rb4
-rw-r--r--lib/gitlab/git/index.rb49
-rw-r--r--lib/gitlab/regex.rb16
-rw-r--r--spec/controllers/projects/blob_controller_spec.rb4
-rw-r--r--spec/controllers/projects/tree_controller_spec.rb10
-rw-r--r--spec/features/projects/blobs/user_create_spec.rb4
-rw-r--r--spec/features/projects/files/creating_a_file_spec.rb6
-rw-r--r--spec/features/projects/files/editing_a_file_spec.rb2
-rw-r--r--spec/features/projects/view_on_env_spec.rb4
-rw-r--r--spec/lib/gitlab/diff/position_tracer_spec.rb8
-rw-r--r--spec/lib/gitlab/git/index_spec.rb20
-rw-r--r--spec/lib/gitlab/git_access_spec.rb2
-rw-r--r--spec/lib/gitlab/regex_spec.rb6
-rw-r--r--spec/requests/api/commits_spec.rb3
-rw-r--r--spec/requests/api/files_spec.rb6
-rw-r--r--spec/requests/api/v3/commits_spec.rb3
-rw-r--r--spec/requests/api/v3/files_spec.rb6
-rw-r--r--spec/services/files/update_service_spec.rb6
47 files changed, 274 insertions, 459 deletions
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/destroy_service.rb b/app/services/files/delete_service.rb
index e294659bc98..7952e5c95d4 100644
--- a/app/services/files/destroy_service.rb
+++ b/app/services/files/delete_service.rb
@@ -1,11 +1,11 @@
module Files
- class DestroyService < Files::BaseService
- def commit
+ class DeleteService < Files::BaseService
+ def create_commit!
repository.delete_file(
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,
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 <strong>new merge request</strong> 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