From 6a6697c393ee84114cf106fae572d1dc973411f8 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 21 Jul 2015 18:30:27 -0700 Subject: Add README to list of files in project page if activity feed setting in use Closes #2044 --- app/helpers/projects_helper.rb | 44 ++++++++++++++++++--------------------- app/views/projects/show.html.haml | 15 +++++++------ 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 525a46291f6..ab9b068de05 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -231,37 +231,20 @@ module ProjectsHelper end end + def readme_path(project) + filename_path(project, :readme) + end + def changelog_path(project) - if project && changelog = project.repository.changelog - namespace_project_blob_path( - project.namespace, - project, - tree_join(project.default_branch, - changelog.name) - ) - end + filename_path(project, :changelog) end def license_path(project) - if project && license = project.repository.license - namespace_project_blob_path( - project.namespace, - project, - tree_join(project.default_branch, - license.name) - ) - end + filename_path(project, :license) end def version_path(project) - if project && version = project.repository.version - namespace_project_blob_path( - project.namespace, - project, - tree_join(project.default_branch, - version.name) - ) - end + filename_path(project, :version) end def hidden_pass_url(original_url) @@ -331,4 +314,17 @@ module ProjectsHelper count end end + + private + + def filename_path(project, filename) + if project && blob = project.repository.send(filename) + namespace_project_blob_path( + project.namespace, + project, + tree_join(project.default_branch, + blob.name) + ) + end + end end diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 4577b84ab89..ebbd3e477fc 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -23,18 +23,21 @@ = link_to namespace_project_tags_path(@project.namespace, @project) do = pluralize(number_with_delimiter(@repository.tag_names.count), 'tag') + - if !prefer_readme? && @repository.readme + %li + = link_to 'Readme', readme_path(@project) + - if @repository.changelog %li - = link_to changelog_path(@project) do - Changelog + = link_to 'Changelog', changelog_path(@project) + - if @repository.license %li - = link_to license_path(@project) do - License + = link_to 'License', license_path(@project) + - if @repository.contribution_guide %li - = link_to contribution_guide_path(@project) do - Contribution guide + = link_to 'Contribution guide', contribution_guide_path(@project) - if current_user && can_push_branch?(@project, @project.default_branch) - unless @repository.changelog -- cgit v1.2.1 From b99cbf47af335309dd0832bb772d0527c5e4f060 Mon Sep 17 00:00:00 2001 From: "Sytse Sijbrandij (admin)" Date: Mon, 10 Aug 2015 18:39:50 +0000 Subject: Database size --- doc/install/requirements.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/install/requirements.md b/doc/install/requirements.md index e44ce25b6b4..aa0d03b75bc 100644 --- a/doc/install/requirements.md +++ b/doc/install/requirements.md @@ -93,7 +93,7 @@ To change the Unicorn workers when you have the Omnibus package please see [the ## Database -If you want to run the database separately, the **recommended** database size is **1 MB per user**. +If you want to run the database separately expect a size of about 1 MB per user. ## Redis and Sidekiq @@ -113,4 +113,4 @@ On a very active server (10,000 active users) the Sidekiq process can use 1GB+ o ### Common UI problems with IE -If you experience UI issues with Internet Explorer, please make sure that you have the `Compatibility View` mode disabled. +If you experience UI issues with Internet Explorer, please make sure that you have the `Compatibility View` mode disabled. \ No newline at end of file -- cgit v1.2.1 From e6ef058289f3044964f7538043ba50d9e9fcafe3 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 10 Aug 2015 11:44:39 -0700 Subject: Disable turbolinks when linking to Bitbucket import status Turbolinks was causing issues with the Bitbucket Javascript/display because it was trying to intercept /import/bitbucket/status even when a 302 redirection occured. Closes #2241 --- CHANGELOG | 1 + app/views/import/bitbucket/status.html.haml | 2 +- app/views/projects/new.html.haml | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 74b41106b97..816031ca8ed 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.14.0 (unreleased) + - Disable turbolinks when linking to Bitbucket import status (Stan Hu) - Fix corrupted binary files when using API files endpoint (Stan Hu) - Show incompatible projects in Bitbucket import status (Stan Hu) - Fix coloring of diffs on MR Discussion-tab (Gert Goet) diff --git a/app/views/import/bitbucket/status.html.haml b/app/views/import/bitbucket/status.html.haml index 98ae509096e..777eb482714 100644 --- a/app/views/import/bitbucket/status.html.haml +++ b/app/views/import/bitbucket/status.html.haml @@ -61,7 +61,7 @@ rather than Git. Please convert = link_to "them to Git,", "https://www.atlassian.com/git/tutorials/migrating-overview" and go through the - = link_to "import flow", status_import_bitbucket_path + = link_to "import flow", status_import_bitbucket_path, "data-no-turbolink" => "true" again. diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index d49eacb30dd..d25fe68242b 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -40,7 +40,7 @@ - if bitbucket_import_enabled? - = link_to status_import_bitbucket_path, class: 'btn' do + = link_to status_import_bitbucket_path, class: 'btn', "data-no-turbolink" => "true" do %i.fa.fa-bitbucket Bitbucket - else -- cgit v1.2.1 From 773545d533ae4b9ab2b55d2e4942dc78cbe28f27 Mon Sep 17 00:00:00 2001 From: Jeff Blaine Date: Mon, 10 Aug 2015 14:53:44 -0400 Subject: Fix instructions for empty repo + existing files Fixes #2242 --- app/views/projects/empty.html.haml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/views/projects/empty.html.haml b/app/views/projects/empty.html.haml index dfe45a3802d..e577d35d560 100644 --- a/app/views/projects/empty.html.haml +++ b/app/views/projects/empty.html.haml @@ -43,6 +43,8 @@ cd existing_folder git init git remote add origin #{ content_tag(:span, default_url_to_repo, class: 'clone')} + git add . + git commit git push -u origin master - if can? current_user, :remove_project, @project -- cgit v1.2.1 From 9195d65602c2bb061a576d55949cc65ad755732e Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Mon, 10 Aug 2015 21:14:37 +0000 Subject: Doc: Fix link to gravatar.com --- doc/customization/libravatar.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/customization/libravatar.md b/doc/customization/libravatar.md index ee57fdc6590..54c1780c3ab 100644 --- a/doc/customization/libravatar.md +++ b/doc/customization/libravatar.md @@ -1,6 +1,6 @@ # Use Libravatar service with GitLab -GitLab by default supports [Gravatar](gravatar.com) avatar service. +GitLab by default supports [Gravatar](https://gravatar.com) avatar service. Libravatar is a service which delivers your avatar (profile picture) to other websites and their API is [heavily based on gravatar](http://wiki.libravatar.org/api/). -- cgit v1.2.1 From f1a241e0ed66b8b8d46dec39ff84660d14387cca Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 10 Aug 2015 23:56:01 -0700 Subject: Fix broken code import and display error messages if something went wrong with creating project Clicking "Import" in Bitbucket import status page would result in: ``` Rendered import/base/create.js.haml (38.3ms) Completed 500 Internal Server Error in 1362ms (ActiveRecord: 20.2ms) NoMethodError - undefined method `namespace_projects_path' for #<#:0x007fd6c0cf09d8>: actionpack (4.1.11) lib/action_dispatch/routing/polymorphic_routes.rb:142:in `polymorphic_url' actionpack (4.1.11) lib/action_dispatch/routing/polymorphic_routes.rb:148:in `polymorphic_path' actionview (4.1.11) lib/action_view/routing_url_for.rb:87:in `url_for' turbolinks (2.5.3) lib/turbolinks/xhr_url_for.rb:12:in `url_for_with_xhr_referer' actionview (4.1.11) lib/action_view/helpers/url_helper.rb:181:in `link_to' app/views/import/base/create.js.haml:23:in `_app_views_import_base_create_js_haml__1092746368522631377_70280180507700' actionview (4.1.11) lib/action_view/template.rb:145:in `block in render' activesupport (4.1.11) lib/active_support/notifications.rb:161:in `instrument' actionview (4.1.11) lib/action_view/template.rb:339:in `instrument' actionview (4.1.11) lib/action_view/template.rb:143:in `render' rack-mini-profiler (0.9.0) lib/mini_profiler/profiling_methods.rb:108:in `block in profile_method' actionview (4.1.11) lib/action_view/renderer/template_renderer.rb:55:in `block (2 levels) in render_template' actionview (4.1.11) lib/action_view/renderer/abstract_renderer.rb:38:in `block in instrument' activesupport (4.1.11) lib/active_support/notifications.rb:159:in `block in instrument' activesupport (4.1.11) lib/active_support/notifications/instrumenter.rb:20:in `instrument' activesupport (4.1.11) lib/active_support/notifications.rb:159:in `instrument' ``` Closes #2241 --- CHANGELOG | 1 + app/views/import/base/create.js.haml | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 74b41106b97..abce70d6b49 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 7.14.0 (unreleased) + - Fix broken code import and display error messages if something went wrong with creating project (Stan Hu) - Fix corrupted binary files when using API files endpoint (Stan Hu) - Show incompatible projects in Bitbucket import status (Stan Hu) - Fix coloring of diffs on MR Discussion-tab (Gert Goet) diff --git a/app/views/import/base/create.js.haml b/app/views/import/base/create.js.haml index 90a6f5f9d2d..9529f16c128 100644 --- a/app/views/import/base/create.js.haml +++ b/app/views/import/base/create.js.haml @@ -14,12 +14,16 @@ :plain job = $("tr#repo_#{@repo_id}") job.find(".import-actions").html("

Access denied! Please verify you can add deploy keys to this repository.

") -- else +- elsif @project.persisted? :plain job = $("tr#repo_#{@repo_id}") job.attr("id", "project_#{@project.id}") target_field = job.find(".import-target") target_field.empty() - target_field.append('#{link_to @project.path_with_namespace, [@project.namespace.becomes(Namespace), @project]}') + target_field.append('#{link_to @project.path_with_namespace, namespace_project_path(@project.namespace, @project)}') $("table.import-jobs tbody").prepend(job) job.addClass("active").find(".import-actions").html(" started") +- else + :plain + job = $("tr#repo_#{@repo_id}") + job.find(".import-actions").html("

Error saving project: #{escape_javascript(@project.errors.messages.to_s)}

") -- cgit v1.2.1 From f293d991a4bd55df10ff8dc9201b6b45377a6681 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 10:26:12 +0200 Subject: Revert "Merge branch 'mr-widget-text' into 'master' " This reverts commit 151d9fb35fd66a161cf0e1f95d7b0f0448ca8034, reversing changes made to 819d110cabfc2791e0e84bd89b9f87aef570c1db. --- CHANGELOG | 2 - app/assets/stylesheets/pages/merge_requests.scss | 4 -- .../merge_requests/show/_how_to_merge.html.haml | 55 ++++++++++------------ .../merge_requests/widget/_closed.html.haml | 5 +- .../merge_requests/widget/_heading.html.haml | 10 ++-- .../merge_requests/widget/_locked.html.haml | 5 +- .../merge_requests/widget/_merged.html.haml | 24 ++++------ .../projects/merge_requests/widget/_open.html.haml | 2 +- .../merge_requests/widget/open/_accept.html.haml | 6 +-- .../merge_requests/widget/open/_archived.html.haml | 4 +- .../merge_requests/widget/open/_check.html.haml | 4 +- .../widget/open/_conflicts.html.haml | 17 +++---- .../widget/open/_missing_branch.html.haml | 32 ++++++------- .../widget/open/_not_allowed.html.haml | 6 +-- .../merge_requests/widget/open/_nothing.html.haml | 14 +++--- .../merge_requests/widget/open/_reload.html.haml | 7 +-- .../merge_requests/widget/open/_wip.html.haml | 16 +++++-- app/views/shared/issuable/_form.html.haml | 4 +- 18 files changed, 98 insertions(+), 119 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index d228ef616d4..ebffc0d90ab 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -44,8 +44,6 @@ v 7.14.0 (unreleased) - Fetch code from forks to refs/merge-requests/:id/head when merge request created - Remove satellites - Remove comments and email addresses when publicly exposing ssh keys (Zeger-Jan van de Weg) - - Improve MR merge widget text and UI consistency. - - Improve text in MR "How To Merge" modal. - Cache all events v 7.13.3 diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 30bbec7b795..bb61a51029b 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -182,7 +182,3 @@ .merge-request-form .select2-container { width: 250px !important; } - -#modal_merge_info .modal-dialog { - width: 600px; -} diff --git a/app/views/projects/merge_requests/show/_how_to_merge.html.haml b/app/views/projects/merge_requests/show/_how_to_merge.html.haml index db1575f899a..22f601ac99e 100644 --- a/app/views/projects/merge_requests/show/_how_to_merge.html.haml +++ b/app/views/projects/merge_requests/show/_how_to_merge.html.haml @@ -3,45 +3,42 @@ .modal-content .modal-header %a.close{href: "#", "data-dismiss" => "modal"} × - %h3 Check out, review and merge locally + %h3 How to merge .modal-body - %p - %strong Step 1. - Fetch and check out the branch for this merge request - %pre.dark - - if @merge_request.for_fork? + - if @merge_request.for_fork? + - source_remote = @merge_request.source_project.namespace.nil? ? "source" :@merge_request.source_project.namespace.path + - target_remote = @merge_request.target_project.namespace.nil? ? "target" :@merge_request.target_project.namespace.path + %p + %strong Step 1. + Fetch the code and create a new branch pointing to it + %pre.dark :preserve git fetch #{@merge_request.source_project.http_url_to_repo} #{@merge_request.source_branch} git checkout -b #{@merge_request.source_project_path}-#{@merge_request.source_branch} FETCH_HEAD - - else - :preserve - git fetch origin - git checkout -b #{@merge_request.source_branch} origin/#{@merge_request.source_branch} - %p - %strong Step 2. - Review the changes locally - - %p - %strong Step 3. - Merge the branch and fix any conflicts that come up - %pre.dark - - if @merge_request.for_fork? + %p + %strong Step 2. + Merge the branch and push the changes to GitLab + %pre.dark :preserve git checkout #{@merge_request.target_branch} git merge --no-ff #{@merge_request.source_project_path}-#{@merge_request.source_branch} - - else + git push origin #{@merge_request.target_branch} + - else + %p + %strong Step 1. + Update the repo and checkout the branch we are going to merge + %pre.dark + :preserve + git fetch origin + git checkout -b #{@merge_request.source_branch} origin/#{@merge_request.source_branch} + %p + %strong Step 2. + Merge the branch and push the changes to GitLab + %pre.dark :preserve git checkout #{@merge_request.target_branch} git merge --no-ff #{@merge_request.source_branch} - %p - %strong Step 4. - Push the result of the merge to GitLab - %pre.dark - :preserve - git push origin #{@merge_request.target_branch} - - unless @merge_request.can_be_merged_by?(current_user) - %p - Note that pushing to GitLab requires write access to this repository. + git push origin #{@merge_request.target_branch} :javascript $(function(){ diff --git a/app/views/projects/merge_requests/widget/_closed.html.haml b/app/views/projects/merge_requests/widget/_closed.html.haml index f3cc0e7e8a1..b5704c502c8 100644 --- a/app/views/projects/merge_requests/widget/_closed.html.haml +++ b/app/views/projects/merge_requests/widget/_closed.html.haml @@ -6,7 +6,4 @@ - if @merge_request.closed_event by #{link_to_member(@project, @merge_request.closed_event.author, avatar: true)} #{time_ago_with_tooltip(@merge_request.closed_event.created_at)} - %p - = succeed '.' do - The changes were not merged into - %span.label-branch= @merge_request.target_branch + %p Changes were not merged into target branch diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 4d4e2f68f61..17d529766e6 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -3,26 +3,26 @@ - [:success, :skipped, :canceled, :failed, :running, :pending].each do |status| .ci_widget{class: "ci-#{status}", style: "display:none"} - if status == :success - - status = "passed" = icon("check-circle") - else = icon("circle") %span CI build #{status} for #{@merge_request.last_commit_short_sha}. %span.ci-coverage - = link_to "View build details", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" + = link_to "View build page", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" .ci_widget = icon("spinner spin") - Checking CI status for #{@merge_request.last_commit_short_sha}… + Checking for CI status for #{@merge_request.last_commit_short_sha} .ci_widget.ci-not_found{style: "display:none"} = icon("times-circle") - Could not find CI status for #{@merge_request.last_commit_short_sha}. + %span Can not find commit in the CI server + for #{@merge_request.last_commit_short_sha}. .ci_widget.ci-error{style: "display:none"} = icon("times-circle") - Could not connect to the CI server. Please check your settings and try again. + %span Cannot connect to the CI server. Please check your settings and try again. :coffeescript $ -> diff --git a/app/views/projects/merge_requests/widget/_locked.html.haml b/app/views/projects/merge_requests/widget/_locked.html.haml index 78d0783cba0..13ec278847b 100644 --- a/app/views/projects/merge_requests/widget/_locked.html.haml +++ b/app/views/projects/merge_requests/widget/_locked.html.haml @@ -2,8 +2,7 @@ = render 'projects/merge_requests/widget/heading' .mr-widget-body %h4 - = icon("spinner spin") - Merge in progress… + Merge in progress... %p - This merge request is in the process of being merged, during which time it is locked and cannot be closed. + Merging is in progress. While merging this request is locked and cannot be closed. diff --git a/app/views/projects/merge_requests/widget/_merged.html.haml b/app/views/projects/merge_requests/widget/_merged.html.haml index d22dfa085b8..a3b13140810 100644 --- a/app/views/projects/merge_requests/widget/_merged.html.haml +++ b/app/views/projects/merge_requests/widget/_merged.html.haml @@ -7,31 +7,23 @@ by #{link_to_member(@project, @merge_request.merge_event.author, avatar: true)} #{time_ago_with_tooltip(@merge_request.merge_event.created_at)} %div - - if !@merge_request.source_branch_exists? - = succeed '.' do - The changes were merged into - %span.label-branch= @merge_request.target_branch - The source branch has been removed. + - if @source_branch.blank? + Source branch has been removed - - elsif can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) + - elsif can_remove_branch?(@merge_request.source_project, @merge_request.source_branch) && @merge_request.merged? .remove_source_branch_widget - %p - = succeed '.' do - The changes were merged into - %span.label-branch= @merge_request.target_branch - You can remove the source branch now. + %p Changes merged into #{@merge_request.target_branch}. You can remove source branch now = link_to namespace_project_branch_path(@merge_request.source_project.namespace, @merge_request.source_project, @source_branch), remote: true, method: :delete, class: "btn btn-primary btn-sm remove_source_branch" do %i.fa.fa-times Remove Source Branch .remove_source_branch_widget.failed.hide - %p - Failed to remove source branch '#{@merge_request.source_branch}'. + Failed to remove source branch '#{@merge_request.source_branch}' .remove_source_branch_in_progress.hide - %p - = icon('spinner spin') - Removing source branch '#{@merge_request.source_branch}'. Please wait. This page will be automatically reload. + %i.fa.fa-spinner.fa-spin +   + Removing source branch '#{@merge_request.source_branch}'. Please wait. Page will be automatically reloaded.   :coffeescript $('.remove_source_branch').on 'click', -> diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index 0aad9bb3e88..f420cdcab49 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -22,6 +22,6 @@ .mr-widget-footer %span %i.fa.fa-check - Accepting this merge request will close #{"issue".pluralize(@closes_issues.size)} + Accepting this merge request will close #{@closes_issues.size == 1 ? 'issue' : 'issues'} = succeed '.' do != gfm(issues_sentence(@closes_issues)) diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index e1525f6aeb7..1be98cbe8de 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -8,10 +8,10 @@ .accept-control.checkbox = label_tag :should_remove_source_branch, class: "remove_source_checkbox" do = check_box_tag :should_remove_source_branch - Remove source branch + Remove source-branch .accept-control - = link_to "#", class: "modify-merge-commit-link js-toggle-button" do - = icon('edit') + = link_to "#", class: "modify-merge-commit-link js-toggle-button", title: "Modify merge commit message" do + %i.fa.fa-edit Modify commit message .js-toggle-content.hide.prepend-top-20 = render 'shared/commit_message_container', params: params, diff --git a/app/views/projects/merge_requests/widget/open/_archived.html.haml b/app/views/projects/merge_requests/widget/open/_archived.html.haml index ab30fa6b243..eaf113ee568 100644 --- a/app/views/projects/merge_requests/widget/open/_archived.html.haml +++ b/app/views/projects/merge_requests/widget/open/_archived.html.haml @@ -1,4 +1,2 @@ -%h4 - Project is archived %p - This merge request cannot be merged because archived projects cannot be written to. + %strong Archived projects do not provide commit access. diff --git a/app/views/projects/merge_requests/widget/open/_check.html.haml b/app/views/projects/merge_requests/widget/open/_check.html.haml index b6b8974297e..e775447cb75 100644 --- a/app/views/projects/merge_requests/widget/open/_check.html.haml +++ b/app/views/projects/merge_requests/widget/open/_check.html.haml @@ -1,6 +1,6 @@ %strong - = icon("spinner spin") - Checking ability to merge automatically… + %i.fa.fa-spinner.fa-spin + Checking automatic merge… :coffeescript $ -> diff --git a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml index e6c089fefb2..440a7aa1c61 100644 --- a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml +++ b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml @@ -1,10 +1,11 @@ %h4 - = icon("exclamation-triangle") - This merge request contains merge conflicts + This merge request contains merge conflicts that must be resolved. -%p - Please resolve these conflicts or - - if @merge_request.can_be_merged_by?(current_user) - #{link_to "merge this request manually", "#modal_merge_info", class: "how_to_merge_link vlink", "data-toggle" => "modal"}. - - else - ask someone with write access to this repository to merge this request manually. +- if @merge_request.can_be_merged_by?(current_user) + %p + You can merge it manually using the + %strong + = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal" +- else + %p + Only those with write access to this repository can merge merge requests. diff --git a/app/views/projects/merge_requests/widget/open/_missing_branch.html.haml b/app/views/projects/merge_requests/widget/open/_missing_branch.html.haml index c9f07629493..1c565bae80a 100644 --- a/app/views/projects/merge_requests/widget/open/_missing_branch.html.haml +++ b/app/views/projects/merge_requests/widget/open/_missing_branch.html.haml @@ -1,16 +1,16 @@ -- unless @merge_request.source_branch_exists? - %h4 - = icon("exclamation-triangle") - Source branch - %span.label-branch= source_branch_with_namespace(@merge_request) - does not exist - %p - Please restore the source branch or close this merge request and open a new merge request with a different source branch. -- else - %h4 - = icon("exclamation-triangle") - Target branch - %span.label-branch= @merge_request.target_branch - does not exist - %p - Please restore the target branch or use a different target branch. +%h4 + Can't be merged +%p + This merge request can not be accepted because branch + - unless @merge_request.source_branch_exists? + %span.label.label-inverse= @merge_request.source_branch + does not exist in + %span.label.label-info= @merge_request.source_project_path + %br + %strong Please close this merge request and open a new merge request to change source branches. + - else + %span.label.label-inverse= @merge_request.target_branch + does not exist in + %span.label.label-info= @merge_request.target_project_path + %br + %strong Please close this merge request or change to another target branch. diff --git a/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml b/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml index a8145558ca8..82f6ffd8fcb 100644 --- a/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml +++ b/app/views/projects/merge_requests/widget/open/_not_allowed.html.haml @@ -1,4 +1,2 @@ -%h4 - Ready to be merged automatically -%p - Ask someone with write access to this repository to merge this request. +%strong This request can be merged automatically. +Only those with write access to this repository can merge merge requests. diff --git a/app/views/projects/merge_requests/widget/open/_nothing.html.haml b/app/views/projects/merge_requests/widget/open/_nothing.html.haml index 35626b624b7..4d526576bc2 100644 --- a/app/views/projects/merge_requests/widget/open/_nothing.html.haml +++ b/app/views/projects/merge_requests/widget/open/_nothing.html.haml @@ -1,8 +1,8 @@ -%h4 - = icon("exclamation-triangle") - Nothing to merge from - %span.label-branch= source_branch_with_namespace(@merge_request) - into - %span.label-branch= @merge_request.target_branch +%h4 Nothing to merge %p - Please push new commits to the source branch or use a different target branch. + Nothing to merge from + %span.label-branch #{@merge_request.source_branch} + to + %span.label-branch #{@merge_request.target_branch} + %br + Try to use different branches or push new code. diff --git a/app/views/projects/merge_requests/widget/open/_reload.html.haml b/app/views/projects/merge_requests/widget/open/_reload.html.haml index acfc31725eb..5787f6efea4 100644 --- a/app/views/projects/merge_requests/widget/open/_reload.html.haml +++ b/app/views/projects/merge_requests/widget/open/_reload.html.haml @@ -1,6 +1 @@ -%h4 - = icon("exclamation-triangle") - This merge request failed to be merged automatically - -%p - Please reload the page to find out the reason. +This merge request cannot be merged. Try to reload the page. diff --git a/app/views/projects/merge_requests/widget/open/_wip.html.haml b/app/views/projects/merge_requests/widget/open/_wip.html.haml index 0cf16542cc1..4ce3ab31278 100644 --- a/app/views/projects/merge_requests/widget/open/_wip.html.haml +++ b/app/views/projects/merge_requests/widget/open/_wip.html.haml @@ -1,5 +1,13 @@ -%h4 - This merge request is currently a Work In Progress +- if @merge_request.can_be_merged_by?(current_user) + %h4 + This merge request cannot be accepted because it is marked as Work In Progress. -%p - When this merge request is ready, remove the "WIP" prefix from the title to allow it to be merged. + %p + %button.btn.disabled{:type => 'button'} + %i.fa.fa-warning + Accept Merge Request +   + When the merge request is ready, remove the "WIP" prefix from the title to allow it to be accepted. +- else + %strong This merge request is marked as Work In Progress. + Only those with write access to this repository can merge merge requests. diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 3489bf3f191..ac8c1936c9e 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -16,10 +16,10 @@ %p.help-block - if issuable.work_in_progress? Remove the WIP prefix from the title to allow this - Work In Progress merge request to be merged when it's ready. + Work In Progress merge request to be accepted when it's ready. - else Start the title with [WIP] or WIP: to prevent a - Work In Progress merge request from being merged before it's ready. + Work In Progress merge request from being accepted before it's ready. .form-group.issuable-description = f.label :description, 'Description', class: 'control-label' .col-sm-10 -- cgit v1.2.1 From 84727fba96c6794874e1f94d581408b70e1842a7 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 10:26:19 +0200 Subject: Revert "Merge branch 'improve-merge-requests' into 'master' " This reverts commit 4773f38e28c91dbbb6e5e385e0c403877298bfed, reversing changes made to 0d5d80b735eb18ae79eb2bfe26c08896d53db414. --- .../javascripts/merge_request_widget.js.coffee | 6 +- app/assets/stylesheets/pages/merge_requests.scss | 144 +++++++++++---------- app/helpers/merge_requests_helper.rb | 10 -- app/models/merge_request.rb | 2 +- app/models/merge_request_diff.rb | 4 +- app/views/projects/merge_requests/_show.html.haml | 51 +++++--- .../merge_requests/show/_commits.html.haml | 2 +- .../projects/merge_requests/show/_diffs.html.haml | 2 +- .../merge_requests/widget/_heading.html.haml | 37 ++++-- .../merge_requests/widget/open/_accept.html.haml | 6 + .../widget/open/_conflicts.html.haml | 9 +- doc/workflow/README.md | 1 - doc/workflow/merge_requests.md | 40 ------ 13 files changed, 160 insertions(+), 154 deletions(-) delete mode 100644 doc/workflow/merge_requests.md diff --git a/app/assets/javascripts/merge_request_widget.js.coffee b/app/assets/javascripts/merge_request_widget.js.coffee index 995a2f24093..c68a2a9d047 100644 --- a/app/assets/javascripts/merge_request_widget.js.coffee +++ b/app/assets/javascripts/merge_request_widget.js.coffee @@ -49,8 +49,10 @@ class @MergeRequestWidget @setMergeButtonClass('btn-danger') showCiCoverage: (coverage) -> - text = 'Coverage ' + coverage + '%' - $('.ci_widget:visible .ci-coverage').text(text) + cov_html = $('') + cov_html.addClass('ci-coverage') + cov_html.text('Coverage ' + coverage + '%') + $('.ci_widget:visible').append(cov_html) setMergeButtonClass: (css_class) -> $('.accept_merge_request').removeClass("btn-create").addClass(css_class) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index bb61a51029b..baba6f32129 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -1,15 +1,9 @@ -/** - * MR -> show: Automerge widget + + /** + * MR -> show: Automerge widget * */ .mr-state-widget { - background: #FAFAFA; - margin-bottom: 20px; - color: #666; - border: 1px solid #e5e5e5; - @include box-shadow(0 1px 1px rgba(0, 0, 0, 0.05)); - @include border-radius(3px); - form { margin-bottom: 0; .clearfix { @@ -26,67 +20,16 @@ display: inline-block; margin: 0; margin-left: 20px; - padding: 5px; + padding: 10px 0; line-height: 20px; + font-weight: bold; .remove_source_checkbox { margin: 0; + font-weight: bold; } } } - - .ci_widget { - border-bottom: 1px solid #EEE; - - i { - margin-right: 4px; - } - - &.ci-success { - color: $gl-success; - } - - &.ci-skipped { - background-color: #eee; - color: #888; - } - - &.ci-pending, - &.ci-running { - color: $gl-warning; - } - - &.ci-failed, - &.ci-canceled, - &.ci-error { - color: $gl-danger; - } - } - - .mr-widget-body, - .ci_widget, - .mr-widget-footer { - padding: 15px; - } - - .mr-widget-body { - h4 { - font-weight: bold; - margin: 5px 0; - } - - p:last-child { - margin-bottom: 0; - } - } - - .mr-widget-footer { - border-top: 1px solid #EEE; - } - - .ci-coverage { - float: right; - } } @media(min-width: $screen-sm-max) { @@ -118,10 +61,23 @@ } .label-branch { - color: #222; + @include border-radius(4px); + padding: 3px 4px; + border: none; + background: $hover; + color: #333; font-family: $monospace_font; - font-weight: bold; + font-weight: normal; overflow: hidden; + + .label-project { + @include border-radius-left(4px); + padding: 3px 4px; + background: #279; + position: relative; + left: -4px; + letter-spacing: -1px; + } } .mr-list { @@ -168,6 +124,64 @@ display: none; } +.mr-state-widget { + font-size: 13px; + background: #FAFAFA; + margin-bottom: 20px; + color: #666; + border: 1px solid #e5e5e5; + @include box-shadow(0 1px 1px rgba(0, 0, 0, 0.05)); + @include border-radius(3px); + + .ci_widget { + padding: 10px 15px; + font-size: 15px; + border-bottom: 1px solid #EEE; + + &.ci-success { + color: $gl-success; + } + + &.ci-skipped { + background-color: #eee; + color: #888; + } + + &.ci-pending, + &.ci-running { + color: $gl-warning; + } + + &.ci-failed, + &.ci-canceled, + &.ci-error { + color: $gl-danger; + } + } + + .mr-widget-body { + padding: 10px 15px; + + h4 { + font-weight: bold; + margin: 5px 0; + } + + p:last-child { + margin-bottom: 0; + } + } + + .mr-widget-footer { + padding: 10px 15px; + border-top: 1px solid #EEE; + } + + .ci-coverage { + float: right; + } +} + .merge-request-show-labels { a { margin-right: 5px; diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index f8169b4f288..45ee4fe4135 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -61,14 +61,4 @@ module MergeRequestsHelper } ) end - - def source_branch_with_namespace(merge_request) - if merge_request.for_fork? - namespace = link_to(merge_request.source_project_namespace, - project_path(merge_request.source_project)) - namespace + ":#{merge_request.source_branch}" - else - merge_request.source_branch - end - end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 467b90861f9..631a2d887cc 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -432,7 +432,7 @@ class MergeRequest < ActiveRecord::Base target_project.repository.fetch_ref( source_project.repository.path_to_repo, "refs/heads/#{source_branch}", - "refs/merge-requests/#{iid}/head" + "refs/merge-requests/#{id}/head" ) end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index e317c8eac4d..2177f972ca3 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -161,8 +161,8 @@ class MergeRequestDiff < ActiveRecord::Base def compare_result @compare_result ||= begin - # Update ref for merge request - merge_request.fetch_ref + # Update ref if merge request is from fork + merge_request.fetch_ref if merge_request.for_fork? # Get latest sha of branch from source project source_sha = merge_request.source_project.commit(source_branch).sha diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 2662e3aff6b..c57eee14143 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -6,25 +6,40 @@ = render "projects/merge_requests/show/mr_box" %hr .append-bottom-20 - - if @merge_request.open? - .btn-group.btn-group-sm.pull-right - %a.btn.btn-sm.dropdown-toggle{ data: {toggle: :dropdown} } - = icon('download') - Download as - %span.caret - %ul.dropdown-menu - %li= link_to "Email Patches", merge_request_path(@merge_request, format: :patch) - %li= link_to "Plain Diff", merge_request_path(@merge_request, format: :diff) - .light - %div - %span From - %span.label-branch #{source_branch_with_namespace(@merge_request)} + .slead + %span From + - if @merge_request.for_fork? + %strong.label-branch< + - if @merge_request.source_project + = link_to @merge_request.source_project_namespace, namespace_project_path(@merge_request.source_project.namespace, @merge_request.source_project) + - else + \ #{@merge_request.source_project_namespace} + \:#{@merge_request.source_branch} %span into - %span.label-branch #{@merge_request.target_branch} - - if @merge_request.open? && !@merge_request.branch_missing? - %div - If you want to try or merge this request manually, you can use the - = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal" + %strong.label-branch #{@merge_request.target_project_namespace}:#{@merge_request.target_branch} + - else + %strong.label-branch #{@merge_request.source_branch} + %span into + %strong.label-branch #{@merge_request.target_branch} + - if @merge_request.open? + .btn-group.btn-group-sm.pull-right + %a.btn.btn-sm.dropdown-toggle{ data: {toggle: :dropdown} } + = icon('download') + Download as + %span.caret + %ul.dropdown-menu + %li= link_to "Email Patches", merge_request_path(@merge_request, format: :patch) + %li= link_to "Plain Diff", merge_request_path(@merge_request, format: :diff) + + - if @merge_request.open? and @merge_request.source_branch_exists? + .append-bottom-20 + .slead + %span + Fetch the branch with + %strong.label-branch< + git fetch + \ #{@merge_request.source_project.http_url_to_repo} + \ #{@merge_request.source_branch} = render "projects/merge_requests/show/how_to_merge" = render "projects/merge_requests/widget/show.html.haml" diff --git a/app/views/projects/merge_requests/show/_commits.html.haml b/app/views/projects/merge_requests/show/_commits.html.haml index a71b181a6a5..3b7f283daf0 100644 --- a/app/views/projects/merge_requests/show/_commits.html.haml +++ b/app/views/projects/merge_requests/show/_commits.html.haml @@ -1 +1 @@ -= render "projects/commits/commits", project: @merge_request.project += render "projects/commits/commits", project: @merge_request.source_project diff --git a/app/views/projects/merge_requests/show/_diffs.html.haml b/app/views/projects/merge_requests/show/_diffs.html.haml index 626970f39be..786b5f39063 100644 --- a/app/views/projects/merge_requests/show/_diffs.html.haml +++ b/app/views/projects/merge_requests/show/_diffs.html.haml @@ -1,5 +1,5 @@ - if @merge_request_diff.collected? - = render "projects/diffs/diffs", diffs: @merge_request.diffs, project: @merge_request.project + = render "projects/diffs/diffs", diffs: @merge_request.diffs, project: @merge_request.source_project - elsif @merge_request_diff.empty? .nothing-here-block Nothing to merge from #{@merge_request.source_branch} into #{@merge_request.target_branch} - else diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 17d529766e6..f04eac0e3bb 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -1,14 +1,28 @@ - if @merge_request.has_ci? .mr-widget-heading - - [:success, :skipped, :canceled, :failed, :running, :pending].each do |status| + .ci_widget.ci-success{style: "display:none"} + = icon("check") + %span CI build passed + for #{@merge_request.last_commit_short_sha}. + = link_to "View build page", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" + + .ci_widget.ci-skipped{style: "display:none"} + = icon("check") + %span CI build skipped + for #{@merge_request.last_commit_short_sha}. + = link_to "View build page", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" + + .ci_widget.ci-failed{style: "display:none"} + = icon("times") + %span CI build failed + for #{@merge_request.last_commit_short_sha}. + = link_to "View build page", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" + + - [:running, :pending].each do |status| .ci_widget{class: "ci-#{status}", style: "display:none"} - - if status == :success - = icon("check-circle") - - else - = icon("circle") + = icon("clock-o") %span CI build #{status} for #{@merge_request.last_commit_short_sha}. - %span.ci-coverage = link_to "View build page", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" .ci_widget @@ -16,12 +30,19 @@ Checking for CI status for #{@merge_request.last_commit_short_sha} .ci_widget.ci-not_found{style: "display:none"} - = icon("times-circle") + = icon("times") %span Can not find commit in the CI server for #{@merge_request.last_commit_short_sha}. + + + .ci_widget.ci-canceled{style: "display:none"} + = icon("times") + %span CI build canceled + for #{@merge_request.last_commit_short_sha}. + = link_to "View build page", ci_build_details_path(@merge_request), :"data-no-turbolink" => "data-no-turbolink" .ci_widget.ci-error{style: "display:none"} - = icon("times-circle") + = icon("times") %span Cannot connect to the CI server. Please check your settings and try again. :coffeescript diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index 1be98cbe8de..3c0cd25ba07 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -18,6 +18,12 @@ text: @merge_request.merge_commit_message, rows: 14, hint: true + %br + .light + If you want to merge this request manually, you can use the + %strong + = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal" + :coffeescript $('.accept-mr-form').on 'ajax:before', -> btn = $('.accept_merge_request') diff --git a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml index 440a7aa1c61..7dc3b4eb2cc 100644 --- a/app/views/projects/merge_requests/widget/open/_conflicts.html.haml +++ b/app/views/projects/merge_requests/widget/open/_conflicts.html.haml @@ -1,11 +1,10 @@ -%h4 - This merge request contains merge conflicts that must be resolved. - - if @merge_request.can_be_merged_by?(current_user) + %h4 + This merge request contains merge conflicts that must be resolved. %p You can merge it manually using the %strong = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal" - else - %p - Only those with write access to this repository can merge merge requests. + %strong This merge request contains merge conflicts that must be resolved. + Only those with write access to this repository can merge merge requests. diff --git a/doc/workflow/README.md b/doc/workflow/README.md index 5b8d72dfd34..3915198ad2a 100644 --- a/doc/workflow/README.md +++ b/doc/workflow/README.md @@ -13,5 +13,4 @@ - [Project users](add-user/add-user.md) - [Protected branches](protected_branches.md) - [Web Editor](web_editor.md) -- [Merge Requests](merge_requests.md) - ["Work In Progress" Merge Requests](wip_merge_requests.md) diff --git a/doc/workflow/merge_requests.md b/doc/workflow/merge_requests.md deleted file mode 100644 index 751e19da7f1..00000000000 --- a/doc/workflow/merge_requests.md +++ /dev/null @@ -1,40 +0,0 @@ -# Merge Requests - -Merge requests allow you to exchange changes you made to source code - -## Checkout merge requests locally - -Locate the section for your GitLab remote in the `.git/config` file. It looks like this: - -``` -[remote "origin"] - url = https://gitlab.com/gitlab-org/gitlab-ce.git - fetch = +refs/heads/*:refs/remotes/origin/* -``` - -Now add the line `fetch = +refs/merge-requests/*/head:refs/remotes/origin/merge-requests/*` to this section. - -It should looks like this: - -``` -[remote "origin"] - url = https://gitlab.com/gitlab-org/gitlab-ce.git - fetch = +refs/heads/*:refs/remotes/origin/* - fetch = +refs/merge-requests/*/head:refs/remotes/origin/merge-requests/* -``` - -Now you can fetch all the merge requests requests: - -``` -$ git fetch origin -From https://gitlab.com/gitlab-org/gitlab-ce.git - * [new ref] refs/merge-requests/1/head -> origin/merge-requests/1 - * [new ref] refs/merge-requests/2/head -> origin/merge-requests/2 -... -``` - -To check out a particular merge request: - -``` -$ git checkout origin/merge-requests/1 -``` -- cgit v1.2.1 From 9f10943c1a76576ac40d96189a28a4d6123a75d8 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 10:28:42 +0200 Subject: Revert "Merge branch 'drop-satellites'" This reverts commit 957e849f41d96fa9778fcdd06792d2f0274b29ab, reversing changes made to 6b9dbe9f5a175a8162abf296367f561bab3eea1a. Signed-off-by: Dmitriy Zaporozhets --- Gemfile.lock | 2 +- .../javascripts/merge_request_widget.js.coffee | 2 +- app/controllers/projects/compare_controller.rb | 9 +- .../projects/merge_requests_controller.rb | 12 +- app/models/merge_request.rb | 56 ++++---- app/models/merge_request_diff.rb | 32 ++--- app/models/namespace.rb | 5 +- app/models/project.rb | 14 +- app/models/project_services/gitlab_ci_service.rb | 4 - app/models/repository.rb | 30 +---- app/services/base_service.rb | 4 - app/services/compare_service.rb | 41 +++--- app/services/files/base_service.rb | 9 +- app/services/git_push_service.rb | 10 +- app/services/merge_requests/auto_merge_service.rb | 75 +++++++++++ app/services/merge_requests/base_merge_service.rb | 10 ++ app/services/merge_requests/build_service.rb | 14 +- app/services/merge_requests/merge_service.rb | 61 ++------- app/services/merge_requests/post_merge_service.rb | 22 --- app/services/merge_requests/refresh_service.rb | 4 +- app/services/post_commit_service.rb | 71 ---------- app/services/projects/destroy_service.rb | 1 + app/services/projects/transfer_service.rb | 6 + .../projects/merge_requests/_new_compare.html.haml | 2 +- .../projects/merge_requests/_new_submit.html.haml | 4 +- .../projects/merge_requests/automerge.js.haml | 6 + app/views/projects/merge_requests/merge.js.haml | 6 - .../projects/merge_requests/widget/_open.html.haml | 2 + .../projects/merge_requests/widget/_show.html.haml | 4 +- .../merge_requests/widget/open/_accept.html.haml | 2 +- .../widget/open/_no_satellite.html.haml | 3 + app/workers/auto_merge_worker.rb | 13 ++ app/workers/merge_worker.rb | 19 --- app/workers/repository_import_worker.rb | 1 + config/routes.rb | 4 +- doc/development/architecture.md | 6 +- doc/install/installation.md | 4 + doc/install/requirements.md | 2 +- doc/install/structure.md | 4 +- doc/logs/logs.md | 12 +- doc/raketasks/maintenance.md | 13 ++ features/steps/dashboard/help.rb | 2 +- features/steps/dashboard/merge_requests.rb | 8 +- features/steps/project/forked_merge_requests.rb | 1 + features/steps/project/merge_requests.rb | 5 + lib/api/merge_requests.rb | 6 +- lib/gitlab.rb | 1 + lib/gitlab/backend/shell.rb | 14 ++ lib/gitlab/satellite/action.rb | 58 ++++++++ lib/gitlab/satellite/compare_action.rb | 44 ++++++ lib/gitlab/satellite/logger.rb | 13 ++ lib/gitlab/satellite/merge_action.rb | 146 ++++++++++++++++++++ lib/gitlab/satellite/satellite.rb | 148 +++++++++++++++++++++ lib/tasks/gitlab/check.rake | 56 ++++++++ lib/tasks/gitlab/enable_automerge.rake | 39 ++++++ spec/lib/gitlab/reference_extractor_spec.rb | 8 +- spec/lib/gitlab/satellite/action_spec.rb | 116 ++++++++++++++++ spec/lib/gitlab/satellite/merge_action_spec.rb | 104 +++++++++++++++ spec/models/merge_request_spec.rb | 2 +- spec/models/project_services/slack_service_spec.rb | 2 +- spec/models/project_spec.rb | 1 + spec/models/repository_spec.rb | 4 +- spec/requests/api/files_spec.rb | 2 +- spec/requests/api/merge_requests_spec.rb | 26 ++-- spec/routing/project_routing_spec.rb | 14 +- .../services/merge_requests/create_service_spec.rb | 2 +- spec/services/merge_requests/merge_service_spec.rb | 5 + spec/support/mentionable_shared_examples.rb | 2 +- 68 files changed, 1075 insertions(+), 355 deletions(-) create mode 100644 app/services/merge_requests/auto_merge_service.rb create mode 100644 app/services/merge_requests/base_merge_service.rb delete mode 100644 app/services/merge_requests/post_merge_service.rb delete mode 100644 app/services/post_commit_service.rb create mode 100644 app/views/projects/merge_requests/automerge.js.haml delete mode 100644 app/views/projects/merge_requests/merge.js.haml create mode 100644 app/views/projects/merge_requests/widget/open/_no_satellite.html.haml create mode 100644 app/workers/auto_merge_worker.rb delete mode 100644 app/workers/merge_worker.rb create mode 100644 lib/gitlab/satellite/action.rb create mode 100644 lib/gitlab/satellite/compare_action.rb create mode 100644 lib/gitlab/satellite/logger.rb create mode 100644 lib/gitlab/satellite/merge_action.rb create mode 100644 lib/gitlab/satellite/satellite.rb create mode 100644 lib/tasks/gitlab/enable_automerge.rake create mode 100644 spec/lib/gitlab/satellite/action_spec.rb create mode 100644 spec/lib/gitlab/satellite/merge_action_spec.rb diff --git a/Gemfile.lock b/Gemfile.lock index 643c513161f..53d21b26e97 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -875,4 +875,4 @@ DEPENDENCIES wikicloth (= 0.8.1) BUNDLED WITH - 1.10.4 + 1.10.5 diff --git a/app/assets/javascripts/merge_request_widget.js.coffee b/app/assets/javascripts/merge_request_widget.js.coffee index c68a2a9d047..762eec63dee 100644 --- a/app/assets/javascripts/merge_request_widget.js.coffee +++ b/app/assets/javascripts/merge_request_widget.js.coffee @@ -19,7 +19,7 @@ class @MergeRequestWidget when 'merged' location.reload() else - setTimeout(merge_request_widget.mergeInProgress, 2000) + setTimeout(merge_request_widget.mergeInProgress, 3000) dataType: 'json' getMergeStatus: -> diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index d9b3adae95b..c5f085c236f 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -13,8 +13,13 @@ class Projects::CompareController < Projects::ApplicationController base_ref = Addressable::URI.unescape(params[:from]) @ref = head_ref = Addressable::URI.unescape(params[:to]) - compare_result = CompareService.new. - execute(@project, head_ref, @project, base_ref) + compare_result = CompareService.new.execute( + current_user, + @project, + head_ref, + @project, + base_ref + ) @commits = compare_result.commits @diffs = compare_result.diffs diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index f3054881daf..d1265198318 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -1,7 +1,9 @@ +require 'gitlab/satellite/satellite' + class Projects::MergeRequestsController < Projects::ApplicationController before_action :module_enabled before_action :merge_request, only: [ - :edit, :update, :show, :diffs, :commits, :merge, :merge_check, + :edit, :update, :show, :diffs, :commits, :automerge, :automerge_check, :ci_status, :toggle_subscription ] before_action :closes_issues, only: [:edit, :update, :show, :diffs, :commits] @@ -135,7 +137,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController end end - def merge_check + def automerge_check if @merge_request.unchecked? @merge_request.check_if_can_be_merged end @@ -145,11 +147,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController render partial: "projects/merge_requests/widget/show.html.haml", layout: false end - def merge + def automerge return access_denied! unless @merge_request.can_be_merged_by?(current_user) - if @merge_request.mergeable? - MergeWorker.perform_async(@merge_request.id, current_user.id, params) + if @merge_request.automergeable? + AutoMergeWorker.perform_async(@merge_request.id, current_user.id, params) @status = true else @status = false diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 631a2d887cc..1ef76d16700 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -41,6 +41,8 @@ class MergeRequest < ActiveRecord::Base delegate :commits, :diffs, :last_commit, :last_commit_short_sha, to: :merge_request_diff, prefix: nil + attr_accessor :should_remove_source_branch + # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests attr_accessor :allow_broken @@ -55,7 +57,7 @@ class MergeRequest < ActiveRecord::Base transition [:reopened, :opened] => :closed end - event :mark_as_merged do + event :merge do transition [:reopened, :opened, :locked] => :merged end @@ -204,7 +206,11 @@ class MergeRequest < ActiveRecord::Base def check_if_can_be_merged can_be_merged = - project.repository.can_be_merged?(source_sha, target_branch) + if for_fork? + Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged? + else + project.repository.can_be_merged?(source_branch, target_branch) + end if can_be_merged mark_as_mergeable @@ -221,6 +227,18 @@ class MergeRequest < ActiveRecord::Base self.target_project.events.where(target_id: self.id, target_type: "MergeRequest", action: Event::CLOSED).last end + def automerge!(current_user, commit_message = nil) + return unless automergeable? + + MergeRequests::AutoMergeService. + new(target_project, current_user). + execute(self, commit_message) + end + + def remove_source_branch? + self.should_remove_source_branch && !self.source_project.root_ref?(self.source_branch) && !self.for_fork? + end + def open? opened? || reopened? end @@ -229,11 +247,11 @@ class MergeRequest < ActiveRecord::Base title =~ /\A\[?WIP\]?:? /i end - def mergeable? + def automergeable? open? && !work_in_progress? && can_be_merged? end - def gitlab_merge_status + def automerge_status if work_in_progress? "work_in_progress" else @@ -260,14 +278,14 @@ class MergeRequest < ActiveRecord::Base # # see "git diff" def to_diff(current_user) - target_project.repository.diff_text(target_branch, source_sha) + Gitlab::Satellite::MergeAction.new(current_user, self).diff_in_satellite end # Returns the commit as a series of email patches. # # see "git format-patch" def to_patch(current_user) - target_project.repository.format_patch(target_branch, source_sha) + Gitlab::Satellite::MergeAction.new(current_user, self).format_patch end def hook_attrs @@ -418,30 +436,4 @@ class MergeRequest < ActiveRecord::Base "Open" end end - - def target_sha - @target_sha ||= target_project. - repository.commit(target_branch).sha - end - - def source_sha - commits.first.sha - end - - def fetch_ref - target_project.repository.fetch_ref( - source_project.repository.path_to_repo, - "refs/heads/#{source_branch}", - "refs/merge-requests/#{id}/head" - ) - end - - def in_locked_state - begin - lock_mr - yield - ensure - unlock_mr if locked? - end - end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 2177f972ca3..df1c2b78758 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -16,8 +16,9 @@ require Rails.root.join("app/models/commit") class MergeRequestDiff < ActiveRecord::Base include Sortable - # Prevent store of diff if commits amount more then 500 - COMMITS_SAFE_SIZE = 500 + # Prevent store of diff + # if commits amount more then 200 + COMMITS_SAFE_SIZE = 200 attr_reader :commits, :diffs @@ -123,12 +124,12 @@ class MergeRequestDiff < ActiveRecord::Base if new_diffs.any? if new_diffs.size > Commit::DIFF_HARD_LIMIT_FILES self.state = :overflow_diff_files_limit - new_diffs = new_diffs.first[Commit::DIFF_HARD_LIMIT_LINES] + new_diffs = [] end if new_diffs.sum { |diff| diff.diff.lines.count } > Commit::DIFF_HARD_LIMIT_LINES self.state = :overflow_diff_lines_limit - new_diffs = new_diffs.first[Commit::DIFF_HARD_LIMIT_LINES] + new_diffs = [] end end @@ -159,21 +160,12 @@ class MergeRequestDiff < ActiveRecord::Base private def compare_result - @compare_result ||= - begin - # Update ref if merge request is from fork - merge_request.fetch_ref if merge_request.for_fork? - - # Get latest sha of branch from source project - source_sha = merge_request.source_project.commit(source_branch).sha - - Gitlab::CompareResult.new( - Gitlab::Git::Compare.new( - merge_request.target_project.repository.raw_repository, - merge_request.target_branch, - source_sha, - ) - ) - end + @compare_result ||= CompareService.new.execute( + merge_request.author, + merge_request.source_project, + merge_request.source_branch, + merge_request.target_project, + merge_request.target_branch, + ) end end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 161a16ca61c..30ffacadded 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -118,11 +118,12 @@ class Namespace < ActiveRecord::Base gitlab_shell.add_namespace(path_was) if gitlab_shell.mv_namespace(path_was, path) - # If repositories moved successfully we need to - # send update instructions to users. + # If repositories moved successfully we need to remove old satellites + # and send update instructions to users. # However we cannot allow rollback since we moved namespace dir # So we basically we mute exceptions in next actions begin + gitlab_shell.rm_satellites(path_was) send_update_instructions rescue # Returning false does not rollback after_* transaction but gives diff --git a/app/models/project.rb b/app/models/project.rb index 4628f478ca6..3dc1729e812 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -520,6 +520,14 @@ class Project < ActiveRecord::Base !repository.exists? || repository.empty? end + def ensure_satellite_exists + self.satellite.create unless self.satellite.exists? + end + + def satellite + @satellite ||= Gitlab::Satellite::Satellite.new(self) + end + def repo repository.raw end @@ -589,11 +597,14 @@ class Project < ActiveRecord::Base new_path_with_namespace = File.join(namespace_dir, path) if gitlab_shell.mv_repository(old_path_with_namespace, new_path_with_namespace) - # If repository moved successfully we need to send update instructions to users. + # If repository moved successfully we need to remove old satellite + # and send update instructions to users. # However we cannot allow rollback since we moved repository # So we basically we mute exceptions in next actions begin gitlab_shell.mv_repository("#{old_path_with_namespace}.wiki", "#{new_path_with_namespace}.wiki") + gitlab_shell.rm_satellites(old_path_with_namespace) + ensure_satellite_exists send_move_instructions reset_events_cache rescue @@ -691,6 +702,7 @@ class Project < ActiveRecord::Base def create_repository if forked? if gitlab_shell.fork_repository(forked_from_project.path_with_namespace, self.namespace.path) + ensure_satellite_exists true else errors.add(:base, 'Failed to fork repository via gitlab-shell') diff --git a/app/models/project_services/gitlab_ci_service.rb b/app/models/project_services/gitlab_ci_service.rb index ecdcd48ae60..5aaa4e85cbc 100644 --- a/app/models/project_services/gitlab_ci_service.rb +++ b/app/models/project_services/gitlab_ci_service.rb @@ -74,8 +74,6 @@ class GitlabCiService < CiService else :error end - rescue Errno::ECONNREFUSED - :error end def fork_registration(new_project, private_token) @@ -105,8 +103,6 @@ class GitlabCiService < CiService if response.code == 200 and response["coverage"] response["coverage"] end - rescue Errno::ECONNREFUSED - nil end def build_page(sha, ref) diff --git a/app/models/repository.rb b/app/models/repository.rb index 46efbede2a2..807b33b2a3e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -411,36 +411,15 @@ class Repository } end - def can_be_merged?(source_sha, target_branch) + def can_be_merged?(source_branch, target_branch) our_commit = rugged.branches[target_branch].target - their_commit = rugged.lookup(source_sha) + their_commit = rugged.branches[source_branch].target if our_commit && their_commit !rugged.merge_commits(our_commit, their_commit).conflicts? - else - false end end - def merge(source_sha, target_branch, options = {}) - our_commit = rugged.branches[target_branch].target - their_commit = rugged.lookup(source_sha) - - raise "Invalid merge target" if our_commit.nil? - raise "Invalid merge source" if their_commit.nil? - - merge_index = rugged.merge_commits(our_commit, their_commit) - return false if merge_index.conflicts? - - actual_options = options.merge( - parents: [our_commit, their_commit], - tree: merge_index.write_tree(rugged), - update_ref: "refs/heads/#{target_branch}" - ) - - Rugged::Commit.create(rugged, actual_options) - end - def search_files(query, ref) offset = 2 args = %W(git grep -i -n --before-context #{offset} --after-context #{offset} #{query} #{ref || root_ref}) @@ -474,11 +453,6 @@ class Repository ) end - def fetch_ref(source_path, source_ref, target_ref) - args = %W(git fetch #{source_path} #{source_ref}:#{target_ref}) - Gitlab::Popen.popen(args, path_to_repo) - end - private def cache diff --git a/app/services/base_service.rb b/app/services/base_service.rb index f00ec7408b6..6d9ed345914 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -31,10 +31,6 @@ class BaseService SystemHooksService.new end - def repository - project.repository - end - # Add an error to the specified model for restricted visibility levels def deny_visibility_level(model, denied_visibility_level = nil) denied_visibility_level ||= model.visibility_level diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 70f642baaaa..6aa9df4b194 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -1,28 +1,27 @@ -require 'securerandom' - # Compare 2 branches for one repo or between repositories # and return Gitlab::CompareResult object that responds to commits and diffs class CompareService - def execute(source_project, source_branch, target_project, target_branch) - source_sha = source_project.commit(source_branch).sha - - # If compare with other project we need to fetch ref first - unless target_project == source_project - random_string = SecureRandom.hex - - target_project.repository.fetch_ref( - source_project.repository.path_to_repo, - "refs/heads/#{source_branch}", - "refs/tmp/#{random_string}/head" + def execute(current_user, source_project, source_branch, target_project, target_branch) + # Try to compare branches to get commits list and diffs + # + # Note: Use satellite only when need to compare between two repos + # because satellites are slower than operations on bare repo + if target_project == source_project + Gitlab::CompareResult.new( + Gitlab::Git::Compare.new( + target_project.repository.raw_repository, + target_branch, + source_branch, + ) ) - end - - Gitlab::CompareResult.new( - Gitlab::Git::Compare.new( - target_project.repository.raw_repository, + else + Gitlab::Satellite::CompareAction.new( + current_user, + target_project, target_branch, - source_sha, - ) - ) + source_project, + source_branch + ).result + end end end diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index d7b40ee8906..646784f2d9d 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -33,8 +33,15 @@ module Files private + def repository + project.repository + end + def after_commit(sha, branch) - PostCommitService.new(project, current_user).execute(sha, branch) + commit = repository.commit(sha) + full_ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch}" + old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA + GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) end def current_branch diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 81535450ac1..3511392d1d8 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -10,14 +10,16 @@ class GitPushService # # Next, this method: # 1. Creates the push event - # 2. Updates merge requests - # 3. Recognizes cross-references from commit messages - # 4. Executes the project's web hooks - # 5. Executes the project's services + # 2. Ensures that the project satellite exists + # 3. Updates merge requests + # 4. Recognizes cross-references from commit messages + # 5. Executes the project's web hooks + # 6. Executes the project's services # def execute(project, user, oldrev, newrev, ref) @project, @user = project, user + project.ensure_satellite_exists project.repository.expire_cache if push_remove_branch?(ref, newrev) diff --git a/app/services/merge_requests/auto_merge_service.rb b/app/services/merge_requests/auto_merge_service.rb new file mode 100644 index 00000000000..db824d452d0 --- /dev/null +++ b/app/services/merge_requests/auto_merge_service.rb @@ -0,0 +1,75 @@ +module MergeRequests + # AutoMergeService class + # + # Do git merge in satellite and in case of success + # mark merge request as merged and execute all hooks and notifications + # Called when you do merge via GitLab UI + class AutoMergeService < BaseMergeService + attr_reader :merge_request, :commit_message + + def execute(merge_request, commit_message) + @commit_message = commit_message + @merge_request = merge_request + + merge_request.lock_mr + + if merge! + merge_request.merge + create_merge_event(merge_request, current_user) + create_note(merge_request) + notification_service.merge_mr(merge_request, current_user) + execute_hooks(merge_request, 'merge') + true + else + merge_request.unlock_mr + false + end + rescue + merge_request.unlock_mr if merge_request.locked? + merge_request.mark_as_unmergeable + false + end + + def merge! + if merge_request.for_fork? + Gitlab::Satellite::MergeAction.new(current_user, merge_request).merge!(commit_message) + else + # Merge local branches using rugged instead of satellites + if sha = commit + after_commit(sha, merge_request.target_branch) + + if merge_request.remove_source_branch? + DeleteBranchService.new(merge_request.source_project, current_user).execute(merge_request.source_branch) + end + + true + else + false + end + end + end + + def commit + committer = repository.user_to_comitter(current_user) + + options = { + message: commit_message, + author: committer, + committer: committer + } + + repository.merge(merge_request.source_branch, merge_request.target_branch, options) + end + + def after_commit(sha, branch) + commit = repository.commit(sha) + full_ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch}" + old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA + GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) + end + + def repository + project.repository + end + end +end diff --git a/app/services/merge_requests/base_merge_service.rb b/app/services/merge_requests/base_merge_service.rb new file mode 100644 index 00000000000..9579573adf9 --- /dev/null +++ b/app/services/merge_requests/base_merge_service.rb @@ -0,0 +1,10 @@ +module MergeRequests + class BaseMergeService < MergeRequests::BaseService + + private + + def create_merge_event(merge_request, current_user) + EventCreateService.new.merge_mr(merge_request, current_user) + end + end +end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index a9b29f9654d..956480938c3 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -12,16 +12,12 @@ module MergeRequests merge_request.target_project ||= (project.forked_from_project || project) merge_request.target_branch ||= merge_request.target_project.default_branch - if merge_request.target_branch.blank? || merge_request.source_branch.blank? - message = - if params[:source_branch] || params[:target_branch] - "You must select source and target branch" - end - - return build_failed(merge_request, message) + unless merge_request.target_branch && merge_request.source_branch + return build_failed(merge_request, nil) end compare_result = CompareService.new.execute( + current_user, merge_request.source_project, merge_request.source_branch, merge_request.target_project, @@ -44,6 +40,7 @@ module MergeRequests merge_request.compare_diffs = diffs elsif diffs == false + # satellite timeout return false merge_request.can_be_created = false merge_request.compare_failed = true end @@ -62,6 +59,9 @@ module MergeRequests end merge_request + + rescue Gitlab::Satellite::BranchesWithoutParent + return build_failed(merge_request, "Selected branches have no common commit so they cannot be merged.") end def build_failed(merge_request, message) diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 2107529a21a..327ead4ff3f 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -1,57 +1,22 @@ module MergeRequests # MergeService class # - # Do git merge and in case of success - # mark merge request as merged and execute all hooks and notifications - # Executed when you do merge via GitLab UI - # - class MergeService < MergeRequests::BaseService - attr_reader :merge_request, :commit_message - + # Mark existing merge request as merged + # and execute all hooks and notifications + # Called when you do merge via command line and push code + # to target branch + class MergeService < BaseMergeService def execute(merge_request, commit_message) - @commit_message = commit_message - @merge_request = merge_request + merge_request.merge - unless @merge_request.mergeable? - return error('Merge request is not mergeable') - end - - merge_request.in_locked_state do - if merge_changes - after_merge - success - else - error('Can not merge changes') - end - end - end - - private - - def merge_changes - if sha = commit - after_commit(sha, merge_request.target_branch) - end - end - - def commit - committer = repository.user_to_comitter(current_user) - - options = { - message: commit_message, - author: committer, - committer: committer - } - - repository.merge(merge_request.source_sha, merge_request.target_branch, options) - end - - def after_commit(sha, branch) - PostCommitService.new(project, current_user).execute(sha, branch) - end + create_merge_event(merge_request, current_user) + create_note(merge_request) + notification_service.merge_mr(merge_request, current_user) + execute_hooks(merge_request, 'merge') - def after_merge - MergeRequests::PostMergeService.new(project, current_user).execute(merge_request) + true + rescue + false end end end diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb deleted file mode 100644 index aceb8cb9021..00000000000 --- a/app/services/merge_requests/post_merge_service.rb +++ /dev/null @@ -1,22 +0,0 @@ -module MergeRequests - # PostMergeService class - # - # Mark existing merge request as merged - # and execute all hooks and notifications - # - class PostMergeService < MergeRequests::BaseService - def execute(merge_request) - merge_request.mark_as_merged - create_merge_event(merge_request, current_user) - create_note(merge_request) - notification_service.merge_mr(merge_request, current_user) - execute_hooks(merge_request, 'merge') - end - - private - - def create_merge_event(merge_request, current_user) - EventCreateService.new.merge_mr(merge_request, current_user) - end - end -end diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index e903e48e3cd..d0648da049b 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -33,9 +33,9 @@ module MergeRequests merge_requests.uniq.select(&:source_project).each do |merge_request| - MergeRequests::PostMergeService. + MergeRequests::MergeService. new(merge_request.target_project, @current_user). - execute(merge_request) + execute(merge_request, nil) end end diff --git a/app/services/post_commit_service.rb b/app/services/post_commit_service.rb deleted file mode 100644 index 8592c8d238b..00000000000 --- a/app/services/post_commit_service.rb +++ /dev/null @@ -1,71 +0,0 @@ -class PostCommitService < BaseService - include Gitlab::Popen - - attr_reader :changes, :repo_path - - def execute(sha, branch) - commit = repository.commit(sha) - full_ref = Gitlab::Git::BRANCH_REF_PREFIX + branch - old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA - @changes = "#{old_sha} #{sha} #{full_ref}" - @repo_path = repository.path_to_repo - - post_receive - end - - private - - def post_receive - hook = hook_file('post-receive', repo_path) - return true if hook.nil? - call_receive_hook(hook) - end - - def call_receive_hook(hook) - # function will return true if succesful - exit_status = false - - vars = { - 'GL_ID' => Gitlab::ShellEnv.gl_id(current_user), - 'PWD' => repo_path - } - - options = { - chdir: repo_path - } - - # we combine both stdout and stderr as we don't know what stream - # will be used by the custom hook - Open3.popen2e(vars, hook, options) do |stdin, stdout_stderr, wait_thr| - exit_status = true - stdin.sync = true - - # in git, pre- and post- receive hooks may just exit without - # reading stdin. We catch the exception to avoid a broken pipe - # warning - begin - # inject all the changes as stdin to the hook - changes.lines do |line| - stdin.puts line - end - rescue Errno::EPIPE - end - - # need to close stdin before reading stdout - stdin.close - - # only output stdut_stderr if scripts doesn't return 0 - unless wait_thr.value == 0 - exit_status = false - end - end - - exit_status - end - - def hook_file(hook_type, repo_path) - hook_path = File.join(repo_path.strip, 'hooks') - hook_file = "#{hook_path}/#{hook_type}" - hook_file if File.exist?(hook_file) - end -end diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 28872c89259..403f419ec50 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -27,6 +27,7 @@ module Projects end end + project.satellite.destroy log_info("Project \"#{project.name}\" was removed") system_hook_service.execute_hooks_for(project, :destroy) true diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index 550ed6897dd..f43c0ef70e9 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -33,6 +33,9 @@ module Projects raise TransferError.new("Project with same path in target namespace already exists") end + # Remove old satellite + project.satellite.destroy + # Apply new namespace id project.namespace = new_namespace project.save! @@ -48,6 +51,9 @@ module Projects # Move wiki repo also if present gitlab_shell.mv_repository("#{old_path}.wiki", "#{new_path}.wiki") + # Create a new satellite (reload project from DB) + Project.find(project.id).ensure_satellite_exists + # clear project cached events project.reset_events_cache diff --git a/app/views/projects/merge_requests/_new_compare.html.haml b/app/views/projects/merge_requests/_new_compare.html.haml index 7709330611a..ff9c0cdb283 100644 --- a/app/views/projects/merge_requests/_new_compare.html.haml +++ b/app/views/projects/merge_requests/_new_compare.html.haml @@ -35,7 +35,7 @@ - if @merge_request.compare_failed .alert.alert-danger %h4 Compare failed - %p We can't compare selected branches. It may be because of huge diff. Please try again or select different branches. + %p We can't compare selected branches. It may be because of huge diff or satellite timeout. Please try again or select different branches. - else .light-well .center diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml index 76f44211dac..633a54f3620 100644 --- a/app/views/projects/merge_requests/_new_submit.html.haml +++ b/app/views/projects/merge_requests/_new_submit.html.haml @@ -24,7 +24,7 @@ = icon('history') Commits %span.badge= @commits.size - %li.diffs-tab.active + %li.diffs-tab = link_to url_for(params), data: {target: '#diffs', action: 'diffs', toggle: 'tab'} do = icon('list-alt') Changes @@ -33,7 +33,7 @@ .tab-content #commits.commits.tab-pane = render "projects/commits/commits", project: @project - #diffs.diffs.tab-pane.active + #diffs.diffs.tab-pane - if @diffs.present? = render "projects/diffs/diffs", diffs: @diffs, project: @project - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE diff --git a/app/views/projects/merge_requests/automerge.js.haml b/app/views/projects/merge_requests/automerge.js.haml new file mode 100644 index 00000000000..33321651e32 --- /dev/null +++ b/app/views/projects/merge_requests/automerge.js.haml @@ -0,0 +1,6 @@ +- if @status + :plain + merge_request_widget.mergeInProgress(); +- else + :plain + $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}"); diff --git a/app/views/projects/merge_requests/merge.js.haml b/app/views/projects/merge_requests/merge.js.haml deleted file mode 100644 index 33321651e32..00000000000 --- a/app/views/projects/merge_requests/merge.js.haml +++ /dev/null @@ -1,6 +0,0 @@ -- if @status - :plain - merge_request_widget.mergeInProgress(); -- else - :plain - $('.mr-widget-body').html("#{escape_javascript(render('projects/merge_requests/widget/open/reload'))}"); diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index f420cdcab49..bb794912f8f 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -3,6 +3,8 @@ .mr-widget-body - if @project.archived? = render 'projects/merge_requests/widget/open/archived' + - elsif !@project.satellite.exists? + = render 'projects/merge_requests/widget/open/no_satellite' - elsif @merge_request.commits.blank? = render 'projects/merge_requests/widget/open/nothing' - elsif @merge_request.branch_missing? diff --git a/app/views/projects/merge_requests/widget/_show.html.haml b/app/views/projects/merge_requests/widget/_show.html.haml index a489d4f9b24..263cab7a9e8 100644 --- a/app/views/projects/merge_requests/widget/_show.html.haml +++ b/app/views/projects/merge_requests/widget/_show.html.haml @@ -11,10 +11,10 @@ var merge_request_widget; merge_request_widget = new MergeRequestWidget({ - url_to_automerge_check: "#{merge_check_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}", + url_to_automerge_check: "#{automerge_check_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}", check_enable: #{@merge_request.unchecked? ? "true" : "false"}, url_to_ci_check: "#{ci_status_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}", ci_enable: #{@project.ci_service ? "true" : "false"}, - current_status: "#{@merge_request.gitlab_merge_status}", + current_status: "#{@merge_request.automerge_status}", }); diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml index 3c0cd25ba07..f5bacaf280a 100644 --- a/app/views/projects/merge_requests/widget/open/_accept.html.haml +++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml @@ -1,4 +1,4 @@ -= form_for [:merge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f| += form_for [:automerge, @project.namespace.becomes(Namespace), @project, @merge_request], remote: true, method: :post, html: { class: 'accept-mr-form js-requires-input' } do |f| = hidden_field_tag :authenticity_token, form_authenticity_token .accept-merge-holder.clearfix.js-toggle-container .accept-action diff --git a/app/views/projects/merge_requests/widget/open/_no_satellite.html.haml b/app/views/projects/merge_requests/widget/open/_no_satellite.html.haml new file mode 100644 index 00000000000..3718cfd8333 --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_no_satellite.html.haml @@ -0,0 +1,3 @@ +%p + %span + %strong This repository does not have a satellite. Please ask an administrator to fix this issue! diff --git a/app/workers/auto_merge_worker.rb b/app/workers/auto_merge_worker.rb new file mode 100644 index 00000000000..a6dd73eee5f --- /dev/null +++ b/app/workers/auto_merge_worker.rb @@ -0,0 +1,13 @@ +class AutoMergeWorker + include Sidekiq::Worker + + sidekiq_options queue: :default + + def perform(merge_request_id, current_user_id, params) + params = params.with_indifferent_access + current_user = User.find(current_user_id) + merge_request = MergeRequest.find(merge_request_id) + merge_request.should_remove_source_branch = params[:should_remove_source_branch] + merge_request.automerge!(current_user, params[:commit_message]) + end +end diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb deleted file mode 100644 index 6a8665c179a..00000000000 --- a/app/workers/merge_worker.rb +++ /dev/null @@ -1,19 +0,0 @@ -class MergeWorker - include Sidekiq::Worker - - sidekiq_options queue: :default - - def perform(merge_request_id, current_user_id, params) - params = params.with_indifferent_access - current_user = User.find(current_user_id) - merge_request = MergeRequest.find(merge_request_id) - - result = MergeRequests::MergeService.new(merge_request.target_project, current_user). - execute(merge_request, params[:commit_message]) - - if result[:status] == :success && params[:should_remove_source_branch].present? - DeleteBranchService.new(merge_request.source_project, current_user). - execute(merge_request.source_branch) - end - end -end diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index b546f8777e1..94832872d13 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -27,6 +27,7 @@ class RepositoryImportWorker project.import_finish project.save + project.satellite.create unless project.satellite.exists? ProjectCacheWorker.perform_async(project.id) Gitlab::BitbucketImport::KeyDeleter.new(project).execute if project.import_type == 'bitbucket' end diff --git a/config/routes.rb b/config/routes.rb index d7307a61ede..1166a4b3eba 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -463,8 +463,8 @@ Gitlab::Application.routes.draw do member do get :diffs get :commits - post :merge - get :merge_check + post :automerge + get :automerge_check get :ci_status post :toggle_subscription end diff --git a/doc/development/architecture.md b/doc/development/architecture.md index c00d290371e..541af487bb1 100644 --- a/doc/development/architecture.md +++ b/doc/development/architecture.md @@ -56,9 +56,9 @@ To serve repositories over SSH there's an add-on application called gitlab-shell A typical install of GitLab will be on GNU/Linux. It uses Nginx or Apache as a web front end to proxypass the Unicorn web server. By default, communication between Unicorn and the front end is via a Unix domain socket but forwarding requests via TCP is also supported. The web front end accesses `/home/git/gitlab/public` bypassing the Unicorn server to serve static pages, uploads (e.g. avatar images or attachments), and precompiled assets. GitLab serves web pages and a [GitLab API](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/doc/api) using the Unicorn web server. It uses Sidekiq as a job queue which, in turn, uses redis as a non-persistent database backend for job information, meta data, and incoming jobs. -The GitLab web app uses MySQL or PostgreSQL for persistent database information (e.g. users, permissions, issues, other meta data). GitLab stores the bare git repositories it serves in `/home/git/repositories` by default. It also keeps default branch and hook information with the bare repository. +The GitLab web app uses MySQL or PostgreSQL for persistent database information (e.g. users, permissions, issues, other meta data). GitLab stores the bare git repositories it serves in `/home/git/repositories` by default. It also keeps default branch and hook information with the bare repository. `/home/git/gitlab-satellites` keeps checked out repositories when performing actions such as a merge request, editing files in the web interface, etc. -When serving repositories over HTTP/HTTPS GitLab utilizes the GitLab API to resolve authorization and access as well as serving git objects. +The satellite repository is used by the web interface for editing repositories and the wiki which is also a git repository. When serving repositories over HTTP/HTTPS GitLab utilizes the GitLab API to resolve authorization and access as well as serving git objects. The add-on component gitlab-shell serves repositories over SSH. It manages the SSH keys within `/home/git/.ssh/authorized_keys` which should not be manually edited. gitlab-shell accesses the bare repositories directly to serve git objects and communicates with redis to submit jobs to Sidekiq for GitLab to process. gitlab-shell queries the GitLab API to determine authorization and access. @@ -129,7 +129,7 @@ Note: `/home/git/` is shorthand for `/home/git`. gitlabhq (includes Unicorn and Sidekiq logs) -- `/home/git/gitlab/log/` contains `application.log`, `production.log`, `sidekiq.log`, `unicorn.stdout.log`, `githost.log` and `unicorn.stderr.log` normally. +- `/home/git/gitlab/log/` contains `application.log`, `production.log`, `sidekiq.log`, `unicorn.stdout.log`, `githost.log`, `satellites.log`, and `unicorn.stderr.log` normally. gitlab-shell diff --git a/doc/install/installation.md b/doc/install/installation.md index 55b6f216dde..6e2bb4191ca 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -216,6 +216,10 @@ We recommend using a PostgreSQL database. For MySQL check [MySQL setup guide](da sudo chmod -R u+rwX,go-w log/ sudo chmod -R u+rwX tmp/ + # Create directory for satellites + sudo -u git -H mkdir /home/git/gitlab-satellites + sudo chmod u+rwx,g=rx,o-rwx /home/git/gitlab-satellites + # Make sure GitLab can write to the tmp/pids/ and tmp/sockets/ directories sudo chmod -R u+rwX tmp/pids/ sudo chmod -R u+rwX tmp/sockets/ diff --git a/doc/install/requirements.md b/doc/install/requirements.md index e44ce25b6b4..605977097b4 100644 --- a/doc/install/requirements.md +++ b/doc/install/requirements.md @@ -40,7 +40,7 @@ We love [JRuby](http://jruby.org/) and [Rubinius](http://rubini.us/) but GitLab ### Storage -The necessary hard drive space largely depends on the size of the repos you want to store in GitLab but as a *rule of thumb* you should have at least as much free space as all your repos combined take up. +The necessary hard drive space largely depends on the size of the repos you want to store in GitLab but as a *rule of thumb* you should have at least twice as much free space as all your repos combined take up. You need twice the storage because [GitLab satellites](structure.md) contain an extra copy of each repo. If you want to be flexible about growing your hard drive space in the future consider mounting it using LVM so you can add more hard drives when you need them. diff --git a/doc/install/structure.md b/doc/install/structure.md index d58b0040eef..5c03f073c18 100644 --- a/doc/install/structure.md +++ b/doc/install/structure.md @@ -6,14 +6,16 @@ This is the directory structure you will end up with following the instructions | |-- git | |-- .ssh | |-- gitlab + | |-- gitlab-satellites | |-- gitlab-shell | |-- repositories * `/home/git/.ssh` - contains openssh settings. Specifically the `authorized_keys` file managed by gitlab-shell. * `/home/git/gitlab` - GitLab core software. +* `/home/git/gitlab-satellites` - checked out repositories for merge requests and file editing from web UI. This can be treated as a temporary files directory. * `/home/git/gitlab-shell` - Core add-on component of GitLab. Maintains SSH cloning and other functionality. * `/home/git/repositories` - bare repositories for all projects organized by namespace. This is where the git repositories which are pushed/pulled are maintained for all projects. **This area is critical data for projects. [Keep a backup](../raketasks/backup_restore.md)** -*Note: the default locations for repositories can be configured in `config/gitlab.yml` of GitLab and `config.yml` of gitlab-shell.* +*Note: the default locations for gitlab-satellites and repositories can be configured in `config/gitlab.yml` of GitLab and `config.yml` of gitlab-shell.* To see a more in-depth overview see the [GitLab architecture doc](../development/architecture.md). diff --git a/doc/logs/logs.md b/doc/logs/logs.md index 27937e51764..83c32b09253 100644 --- a/doc/logs/logs.md +++ b/doc/logs/logs.md @@ -51,6 +51,16 @@ December 03, 2014 13:20 -> ERROR -> Command failed [1]: /usr/bin/git --git-dir=/ error: failed to push some refs to '/Users/vsizov/gitlab-development-kit/repositories/gitlabhq/gitlab_git.git' ``` +#### satellites.log +This file lives in `/var/log/gitlab/gitlab-rails/satellites.log` for omnibus package or in `/home/git/gitlab/log/satellites.log` for installations from the source. + +In some cases GitLab should perform write actions to git repository, for example when it is needed to merge the merge request or edit a file with online editor. If something went wrong you can look into this file to find out what exactly happened. +``` +October 07, 2014 11:36: Failed to create satellite for Chesley Weimann III / project1817 +October 07, 2014 11:36: PID: 1872: git clone /Users/vsizov/gitlab-development-kit/gitlab/tmp/tests/repositories/conrad6841/gitlabhq.git /Users/vsizov/gitlab-development-kit/gitlab/tmp/tests/gitlab-satellites/conrad6841/gitlabhq +October 07, 2014 11:36: PID: 1872: -> fatal: repository '/Users/vsizov/gitlab-development-kit/gitlab/tmp/tests/repositories/conrad6841/gitlabhq.git' does not exist +``` + #### sidekiq.log This file lives in `/var/log/gitlab/gitlab-rails/sidekiq.log` for omnibus package or in `/home/git/gitlab/log/sidekiq.log` for installations from the source. @@ -89,4 +99,4 @@ W, [2015-02-13T07:16:01.313000 #9094] WARN -- : Unicorn::WorkerKiller send SIGQ I, [2015-02-13T07:16:01.530733 #9047] INFO -- : reaped # worker=1 I, [2015-02-13T07:16:01.534501 #13379] INFO -- : worker=1 spawned pid=13379 I, [2015-02-13T07:16:01.534848 #13379] INFO -- : worker=1 ready -``` +``` \ No newline at end of file diff --git a/doc/raketasks/maintenance.md b/doc/raketasks/maintenance.md index d9dce2af480..69171cd1765 100644 --- a/doc/raketasks/maintenance.md +++ b/doc/raketasks/maintenance.md @@ -105,11 +105,24 @@ Log directory writable? ... yes Tmp directory writable? ... yes Init script exists? ... yes Init script up-to-date? ... yes +Projects have satellites? ... yes Redis version >= 2.0.0? ... yes Checking GitLab ... Finished ``` +## (Re-)Create satellite repositories + +This will create satellite repositories for all your projects. + +If necessary, remove the `repo_satellites` directory and rerun the commands below. + +``` +sudo -u git -H mkdir -p /home/git/gitlab-satellites +sudo -u git -H bundle exec rake gitlab:satellites:create RAILS_ENV=production +sudo chmod u+rwx,g=rx,o-rwx /home/git/gitlab-satellites +``` + ## Rebuild authorized_keys file In some case it is necessary to rebuild the `authorized_keys` file. diff --git a/features/steps/dashboard/help.rb b/features/steps/dashboard/help.rb index 800e869533e..86ab31a58ab 100644 --- a/features/steps/dashboard/help.rb +++ b/features/steps/dashboard/help.rb @@ -16,6 +16,6 @@ class Spinach::Features::DashboardHelp < Spinach::FeatureSteps end step 'Header "Rebuild project satellites" should have correct ids and links' do - header_should_have_correct_id_and_link(2, 'Check GitLab configuration', 'check-gitlab-configuration', '.documentation') + header_should_have_correct_id_and_link(2, '(Re-)Create satellite repositories', 're-create-satellite-repositories', '.documentation') end end diff --git a/features/steps/dashboard/merge_requests.rb b/features/steps/dashboard/merge_requests.rb index 28c8c6b6015..cec8d06adee 100644 --- a/features/steps/dashboard/merge_requests.rb +++ b/features/steps/dashboard/merge_requests.rb @@ -66,7 +66,7 @@ class Spinach::Features::DashboardMergeRequests < Spinach::FeatureSteps def authored_merge_request @authored_merge_request ||= create :merge_request, - source_branch: 'markdown', + source_branch: 'simple_merge_request', author: current_user, target_project: project, source_project: project @@ -74,14 +74,14 @@ class Spinach::Features::DashboardMergeRequests < Spinach::FeatureSteps def other_merge_request @other_merge_request ||= create :merge_request, - source_branch: 'fix', + source_branch: '2_3_notes_fix', target_project: project, source_project: project end def authored_merge_request_from_fork @authored_merge_request_from_fork ||= create :merge_request, - source_branch: 'feature_conflict', + source_branch: 'basic_page', author: current_user, target_project: public_project, source_project: forked_project @@ -89,7 +89,7 @@ class Spinach::Features::DashboardMergeRequests < Spinach::FeatureSteps def assigned_merge_request_from_fork @assigned_merge_request_from_fork ||= create :merge_request, - source_branch: 'markdown', + source_branch: 'basic_page_fix', assignee: current_user, target_project: public_project, source_project: forked_project diff --git a/features/steps/project/forked_merge_requests.rb b/features/steps/project/forked_merge_requests.rb index 2a333222fb2..3e97e84d116 100644 --- a/features/steps/project/forked_merge_requests.rb +++ b/features/steps/project/forked_merge_requests.rb @@ -9,6 +9,7 @@ class Spinach::Features::ProjectForkedMergeRequests < Spinach::FeatureSteps @project = Project.find_by(name: "Shop") @project ||= create(:project, name: "Shop") @project.team << [@user, :reporter] + @project.ensure_satellite_exists end step 'I have a project forked off of "Shop" called "Forked Shop"' do diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index f2198f58c13..a1a26abd8ca 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -198,10 +198,15 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps end step 'merge request "Bug NS-05" is mergeable' do + merge_request.project.satellite.create merge_request.mark_as_mergeable end step 'I accept this merge request' do + Gitlab::Satellite::MergeAction.any_instance.stub( + merge!: true, + ) + page.within '.mr-state-widget' do click_button "Accept Merge Request" end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 7412274b045..ce21c699e8f 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -198,11 +198,7 @@ module API if merge_request.open? && !merge_request.work_in_progress? if merge_request.can_be_merged? - commit_message = params[:merge_commit_message] || merge_request.merge_commit_message - - ::MergeRequests::MergeService.new(merge_request.target_project, current_user). - execute(merge_request, commit_message) - + merge_request.automerge!(current_user, params[:merge_commit_message] || merge_request.merge_commit_message) present merge_request, with: Entities::MergeRequest else render_api_error!('Branch cannot be merged', 405) diff --git a/lib/gitlab.rb b/lib/gitlab.rb index 6108697bc20..5fc1862c3e9 100644 --- a/lib/gitlab.rb +++ b/lib/gitlab.rb @@ -1,4 +1,5 @@ require 'gitlab/git' module Gitlab + autoload :Satellite, 'gitlab/satellite/satellite' end diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb index 14ee4701e7b..172d4902add 100644 --- a/lib/gitlab/backend/shell.rb +++ b/lib/gitlab/backend/shell.rb @@ -217,6 +217,20 @@ module Gitlab FileUtils.mv(full_path(old_name), full_path(new_name)) end + # Remove GitLab Satellites for provided path (namespace or repo dir) + # + # Ex. + # rm_satellites("gitlab") + # + # rm_satellites("gitlab/gitlab-ci.git") + # + def rm_satellites(path) + raise ArgumentError.new("Path can't be blank") if path.blank? + + satellites_path = File.join(Gitlab.config.satellites.path, path) + FileUtils.rm_r(satellites_path, force: true) + end + def url_to_repo(path) Gitlab.config.gitlab_shell.ssh_path_prefix + "#{path}.git" end diff --git a/lib/gitlab/satellite/action.rb b/lib/gitlab/satellite/action.rb new file mode 100644 index 00000000000..489070f1a3f --- /dev/null +++ b/lib/gitlab/satellite/action.rb @@ -0,0 +1,58 @@ +module Gitlab + module Satellite + class Action + DEFAULT_OPTIONS = { git_timeout: Gitlab.config.satellites.timeout.seconds } + + attr_accessor :options, :project, :user + + def initialize(user, project, options = {}) + @options = DEFAULT_OPTIONS.merge(options) + @project = project + @user = user + end + + protected + + # * Sets a 30s timeout for Git + # * Locks the satellite repo + # * Yields the prepared satellite repo + def in_locked_and_timed_satellite + Gitlab::ShellEnv.set_env(user) + + Grit::Git.with_timeout(options[:git_timeout]) do + project.satellite.lock do + return yield project.satellite.repo + end + end + rescue Errno::ENOMEM => ex + return handle_exception(ex) + rescue Grit::Git::GitTimeout => ex + return handle_exception(ex) + ensure + Gitlab::ShellEnv.reset_env + end + + # * Recreates the satellite + # * Sets up Git variables for the user + # + # Note: use this within #in_locked_and_timed_satellite + def prepare_satellite!(repo) + project.satellite.clear_and_update! + + if user + repo.config['user.name'] = user.name + repo.config['user.email'] = user.email + end + end + + def default_options(options = {}) + { raise: true, timeout: true }.merge(options) + end + + def handle_exception(exception) + Gitlab::GitLogger.error(exception.message) + false + end + end + end +end diff --git a/lib/gitlab/satellite/compare_action.rb b/lib/gitlab/satellite/compare_action.rb new file mode 100644 index 00000000000..46c98a8f4ca --- /dev/null +++ b/lib/gitlab/satellite/compare_action.rb @@ -0,0 +1,44 @@ +module Gitlab + module Satellite + class BranchesWithoutParent < StandardError; end + + class CompareAction < Action + def initialize(user, target_project, target_branch, source_project, source_branch) + super user, target_project + + @target_project, @target_branch = target_project, target_branch + @source_project, @source_branch = source_project, source_branch + end + + # Compare 2 repositories and return Gitlab::CompareResult object + def result + in_locked_and_timed_satellite do |target_repo| + prepare_satellite!(target_repo) + update_satellite_source_and_target!(target_repo) + + Gitlab::CompareResult.new(compare(target_repo)) + end + rescue Grit::Git::CommandFailed => ex + raise BranchesWithoutParent + end + + private + + # Assumes a satellite exists that is a fresh clone of the projects repo, prepares satellite for diffs + def update_satellite_source_and_target!(target_repo) + target_repo.remote_add('source', @source_project.repository.path_to_repo) + target_repo.remote_fetch('source') + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + + def compare(repo) + @compare ||= Gitlab::Git::Compare.new( + Gitlab::Git::Repository.new(repo.path), + "origin/#{@target_branch}", + "source/#{@source_branch}" + ) + end + end + end +end diff --git a/lib/gitlab/satellite/logger.rb b/lib/gitlab/satellite/logger.rb new file mode 100644 index 00000000000..6f3f8255aca --- /dev/null +++ b/lib/gitlab/satellite/logger.rb @@ -0,0 +1,13 @@ +module Gitlab + module Satellite + class Logger < Gitlab::Logger + def self.file_name + 'satellites.log' + end + + def format_message(severity, timestamp, progname, msg) + "#{timestamp.to_s(:long)}: #{msg}\n" + end + end + end +end diff --git a/lib/gitlab/satellite/merge_action.rb b/lib/gitlab/satellite/merge_action.rb new file mode 100644 index 00000000000..f9bf286697e --- /dev/null +++ b/lib/gitlab/satellite/merge_action.rb @@ -0,0 +1,146 @@ +module Gitlab + module Satellite + # GitLab server-side merge + class MergeAction < Action + attr_accessor :merge_request + + def initialize(user, merge_request) + super user, merge_request.target_project + @merge_request = merge_request + end + + # Checks if a merge request can be executed without user interaction + def can_be_merged? + in_locked_and_timed_satellite do |merge_repo| + prepare_satellite!(merge_repo) + merge_in_satellite!(merge_repo) + end + end + + # Merges the source branch into the target branch in the satellite and + # pushes it back to the repository. + # It also removes the source branch if requested in the merge request (and this is permitted by the merge request). + # + # Returns false if the merge produced conflicts + # Returns false if pushing from the satellite to the repository failed or was rejected + # Returns true otherwise + def merge!(merge_commit_message = nil) + in_locked_and_timed_satellite do |merge_repo| + prepare_satellite!(merge_repo) + if merge_in_satellite!(merge_repo, merge_commit_message) + # push merge back to bare repo + # will raise CommandFailed when push fails + merge_repo.git.push(default_options, :origin, merge_request.target_branch) + + # remove source branch + if merge_request.remove_source_branch? + # will raise CommandFailed when push fails + merge_repo.git.push(default_options, :origin, ":#{merge_request.source_branch}") + end + # merge, push and branch removal successful + true + end + end + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + + def diff_in_satellite + in_locked_and_timed_satellite do |merge_repo| + prepare_satellite!(merge_repo) + update_satellite_source_and_target!(merge_repo) + + # Only show what is new in the source branch compared to the target branch, not the other way around. + # The line below with merge_base is equivalent to diff with three dots (git diff branch1...branch2) + # From the git documentation: "git diff A...B" is equivalent to "git diff $(git-merge-base A B) B" + common_commit = merge_repo.git.native(:merge_base, default_options, ["origin/#{merge_request.target_branch}", "source/#{merge_request.source_branch}"]).strip + merge_repo.git.native(:diff, default_options, common_commit, "source/#{merge_request.source_branch}") + end + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + + def diffs_between_satellite + in_locked_and_timed_satellite do |merge_repo| + prepare_satellite!(merge_repo) + update_satellite_source_and_target!(merge_repo) + if merge_request.for_fork? + repository = Gitlab::Git::Repository.new(merge_repo.path) + diffs = Gitlab::Git::Diff.between( + repository, + "source/#{merge_request.source_branch}", + "origin/#{merge_request.target_branch}" + ) + else + raise "Attempt to determine diffs between for a non forked merge request in satellite MergeRequest.id:[#{merge_request.id}]" + end + + return diffs + end + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + + # Get commit as an email patch + def format_patch + in_locked_and_timed_satellite do |merge_repo| + prepare_satellite!(merge_repo) + update_satellite_source_and_target!(merge_repo) + patch = merge_repo.git.format_patch(default_options({ stdout: true }), "origin/#{merge_request.target_branch}..source/#{merge_request.source_branch}") + end + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + + # Retrieve an array of commits between the source and the target + def commits_between + in_locked_and_timed_satellite do |merge_repo| + prepare_satellite!(merge_repo) + update_satellite_source_and_target!(merge_repo) + if merge_request.for_fork? + repository = Gitlab::Git::Repository.new(merge_repo.path) + commits = Gitlab::Git::Commit.between( + repository, + "origin/#{merge_request.target_branch}", + "source/#{merge_request.source_branch}" + ) + else + raise "Attempt to determine commits between for a non forked merge request in satellite MergeRequest.id:[#{merge_request.id}]" + end + + return commits + end + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + + private + # Merges the source_branch into the target_branch in the satellite. + # + # Note: it will clear out the satellite before doing anything + # + # Returns false if the merge produced conflicts + # Returns true otherwise + def merge_in_satellite!(repo, message = nil) + update_satellite_source_and_target!(repo) + + message ||= "Merge branch '#{merge_request.source_branch}' into '#{merge_request.target_branch}'" + + # merge the source branch into the satellite + # will raise CommandFailed when merge fails + repo.git.merge(default_options({ no_ff: true }), "-m#{message}", "source/#{merge_request.source_branch}") + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + + # Assumes a satellite exists that is a fresh clone of the projects repo, prepares satellite for merges, diffs etc + def update_satellite_source_and_target!(repo) + repo.remote_add('source', merge_request.source_project.repository.path_to_repo) + repo.remote_fetch('source') + repo.git.checkout(default_options({ b: true }), merge_request.target_branch, "origin/#{merge_request.target_branch}") + rescue Grit::Git::CommandFailed => ex + handle_exception(ex) + end + end + end +end diff --git a/lib/gitlab/satellite/satellite.rb b/lib/gitlab/satellite/satellite.rb new file mode 100644 index 00000000000..398643d68de --- /dev/null +++ b/lib/gitlab/satellite/satellite.rb @@ -0,0 +1,148 @@ +module Gitlab + module Satellite + autoload :DeleteFileAction, 'gitlab/satellite/files/delete_file_action' + autoload :EditFileAction, 'gitlab/satellite/files/edit_file_action' + autoload :FileAction, 'gitlab/satellite/files/file_action' + autoload :NewFileAction, 'gitlab/satellite/files/new_file_action' + + class CheckoutFailed < StandardError; end + class CommitFailed < StandardError; end + class PushFailed < StandardError; end + + class Satellite + include Gitlab::Popen + + PARKING_BRANCH = "__parking_branch" + + attr_accessor :project + + def initialize(project) + @project = project + end + + def log(message) + Gitlab::Satellite::Logger.error(message) + end + + def clear_and_update! + project.ensure_satellite_exists + + @repo = nil + clear_working_dir! + delete_heads! + remove_remotes! + update_from_source! + end + + def create + output, status = popen(%W(git clone -- #{project.repository.path_to_repo} #{path}), + Gitlab.config.satellites.path) + + log("PID: #{project.id}: git clone #{project.repository.path_to_repo} #{path}") + log("PID: #{project.id}: -> #{output}") + + if status.zero? + true + else + log("Failed to create satellite for #{project.name_with_namespace}") + false + end + end + + def exists? + File.exists? path + end + + # * Locks the satellite + # * Changes the current directory to the satellite's working dir + # * Yields + def lock + project.ensure_satellite_exists + + File.open(lock_file, "w+") do |f| + begin + f.flock File::LOCK_EX + yield + ensure + f.flock File::LOCK_UN + end + end + end + + def lock_file + create_locks_dir unless File.exists?(lock_files_dir) + File.join(lock_files_dir, "satellite_#{project.id}.lock") + end + + def path + File.join(Gitlab.config.satellites.path, project.path_with_namespace) + end + + def repo + project.ensure_satellite_exists + + @repo ||= Grit::Repo.new(path) + end + + def destroy + FileUtils.rm_rf(path) + end + + private + + # Clear the working directory + def clear_working_dir! + repo.git.reset(hard: true) + repo.git.clean(f: true, d: true, x: true) + end + + # Deletes all branches except the parking branch + # + # This ensures we have no name clashes or issues updating branches when + # working with the satellite. + def delete_heads! + heads = repo.heads.map(&:name) + + # update or create the parking branch + repo.git.checkout(default_options({ B: true }), PARKING_BRANCH) + + # remove the parking branch from the list of heads ... + heads.delete(PARKING_BRANCH) + # ... and delete all others + heads.each { |head| repo.git.branch(default_options({ D: true }), head) } + end + + # Deletes all remotes except origin + # + # This ensures we have no remote name clashes or issues updating branches when + # working with the satellite. + def remove_remotes! + remotes = repo.git.remote.split(' ') + remotes.delete('origin') + remotes.each { |name| repo.git.remote(default_options,'rm', name)} + end + + # Updates the satellite from bare repo + # + # Note: this will only update remote branches (i.e. origin/*) + def update_from_source! + repo.git.remote(default_options, 'set-url', :origin, project.repository.path_to_repo) + repo.git.fetch(default_options, :origin) + end + + def default_options(options = {}) + { raise: true, timeout: true }.merge(options) + end + + # Create directory for storing + # satellites lock files + def create_locks_dir + FileUtils.mkdir_p(lock_files_dir) + end + + def lock_files_dir + @lock_files_dir ||= File.join(Gitlab.config.satellites.path, "tmp") + end + end + end +end diff --git a/lib/tasks/gitlab/check.rake b/lib/tasks/gitlab/check.rake index 8acb6a7fd19..badb47c6779 100644 --- a/lib/tasks/gitlab/check.rake +++ b/lib/tasks/gitlab/check.rake @@ -25,6 +25,7 @@ namespace :gitlab do check_init_script_exists check_init_script_up_to_date check_projects_have_namespace + check_satellites_exist check_redis_version check_ruby_version check_git_version @@ -237,6 +238,37 @@ namespace :gitlab do end end + def check_satellites_exist + print "Projects have satellites? ... " + + unless Project.count > 0 + puts "can't check, you have no projects".magenta + return + end + puts "" + + Project.find_each(batch_size: 100) do |project| + print sanitized_message(project) + + if project.satellite.exists? + puts "yes".green + elsif project.empty_repo? + puts "can't create, repository is empty".magenta + else + puts "no".red + try_fixing_it( + sudo_gitlab("bundle exec rake gitlab:satellites:create RAILS_ENV=production"), + "If necessary, remove the tmp/repo_satellites directory ...", + "... and rerun the above command" + ) + for_more_information( + "doc/raketasks/maintenance.md " + ) + fix_and_rerun + end + end + end + def check_log_writable print "Log directory writable? ... " @@ -307,6 +339,7 @@ namespace :gitlab do check_repo_base_is_not_symlink check_repo_base_user_and_group check_repo_base_permissions + check_satellites_permissions check_repos_hooks_directory_is_link check_gitlab_shell_self_test @@ -384,6 +417,29 @@ namespace :gitlab do end end + def check_satellites_permissions + print "Satellites access is drwxr-x---? ... " + + satellites_path = Gitlab.config.satellites.path + unless File.exists?(satellites_path) + puts "can't check because of previous errors".magenta + return + end + + if File.stat(satellites_path).mode.to_s(8).ends_with?("0750") + puts "yes".green + else + puts "no".red + try_fixing_it( + "sudo chmod u+rwx,g=rx,o-rwx #{satellites_path}", + ) + for_more_information( + see_installation_guide_section "GitLab" + ) + fix_and_rerun + end + end + def check_repo_base_user_and_group gitlab_shell_ssh_user = Gitlab.config.gitlab_shell.ssh_user gitlab_shell_owner_group = Gitlab.config.gitlab_shell.owner_group diff --git a/lib/tasks/gitlab/enable_automerge.rake b/lib/tasks/gitlab/enable_automerge.rake new file mode 100644 index 00000000000..3dade9d75b8 --- /dev/null +++ b/lib/tasks/gitlab/enable_automerge.rake @@ -0,0 +1,39 @@ +namespace :gitlab do + namespace :satellites do + desc "GitLab | Create satellite repos" + task create: :environment do + create_satellites + end + end + + def create_satellites + warn_user_is_not_gitlab + + print "Creating satellites for ..." + unless Project.count > 0 + puts "skipping, because you have no projects".magenta + return + end + puts "" + + Project.find_each(batch_size: 100) do |project| + print "#{project.name_with_namespace.yellow} ... " + + unless project.repo_exists? + puts "skipping, because the repo is empty".magenta + next + end + + if project.satellite.exists? + puts "exists already".green + else + print "\n... " + if project.satellite.create + puts "created".green + else + puts "error".red + end + end + end + end +end diff --git a/spec/lib/gitlab/reference_extractor_spec.rb b/spec/lib/gitlab/reference_extractor_spec.rb index 088e34f050c..f921dd9cc09 100644 --- a/spec/lib/gitlab/reference_extractor_spec.rb +++ b/spec/lib/gitlab/reference_extractor_spec.rb @@ -25,7 +25,7 @@ describe Gitlab::ReferenceExtractor do project.team << [@u_bar, :guest] subject.analyze(%Q{ - Inline code: `@foo` + Inline code: `@foo` Code block: @@ -33,7 +33,7 @@ describe Gitlab::ReferenceExtractor do @bar ``` - Quote: + Quote: > @offteam }) @@ -49,8 +49,8 @@ describe Gitlab::ReferenceExtractor do end it 'accesses valid merge requests' do - @m0 = create(:merge_request, source_project: project, target_project: project, source_branch: 'markdown') - @m1 = create(:merge_request, source_project: project, target_project: project, source_branch: 'feature_conflict') + @m0 = create(:merge_request, source_project: project, target_project: project, source_branch: 'aaa') + @m1 = create(:merge_request, source_project: project, target_project: project, source_branch: 'bbb') subject.analyze("!999, !#{@m1.iid}, and !#{@m0.iid}.") expect(subject.merge_requests).to eq([@m1, @m0]) diff --git a/spec/lib/gitlab/satellite/action_spec.rb b/spec/lib/gitlab/satellite/action_spec.rb new file mode 100644 index 00000000000..0a93676edc3 --- /dev/null +++ b/spec/lib/gitlab/satellite/action_spec.rb @@ -0,0 +1,116 @@ +require 'spec_helper' + +describe 'Gitlab::Satellite::Action' do + let(:project) { create(:project) } + let(:user) { create(:user) } + + describe '#prepare_satellite!' do + it 'should be able to fetch timeout from conf' do + expect(Gitlab::Satellite::Action::DEFAULT_OPTIONS[:git_timeout]).to eq(30.seconds) + end + + it 'create a repository with a parking branch and one remote: origin' do + repo = project.satellite.repo + + #now lets dirty it up + + starting_remote_count = repo.git.list_remotes.size + expect(starting_remote_count).to be >= 1 + #kind of hookey way to add a second remote + origin_uri = repo.git.remote({ v: true }).split(" ")[1] + + repo.git.remote({ raise: true }, 'add', 'another-remote', origin_uri) + repo.git.branch({ raise: true }, 'a-new-branch') + + expect(repo.heads.size).to be > (starting_remote_count) + expect(repo.git.remote().split(" ").size).to be > (starting_remote_count) + + repo.git.config({}, "user.name", "#{user.name} -- foo") + repo.git.config({}, "user.email", "#{user.email} -- foo") + expect(repo.config['user.name']).to eq("#{user.name} -- foo") + expect(repo.config['user.email']).to eq("#{user.email} -- foo") + + + #These must happen in the context of the satellite directory... + satellite_action = Gitlab::Satellite::Action.new(user, project) + project.satellite.lock do + #Now clean it up, use send to get around prepare_satellite! being protected + satellite_action.send(:prepare_satellite!, repo) + end + + #verify it's clean + heads = repo.heads.map(&:name) + expect(heads.size).to eq(1) + expect(heads.include?(Gitlab::Satellite::Satellite::PARKING_BRANCH)).to eq(true) + remotes = repo.git.remote().split(' ') + expect(remotes.size).to eq(1) + expect(remotes.include?('origin')).to eq(true) + expect(repo.config['user.name']).to eq(user.name) + expect(repo.config['user.email']).to eq(user.email) + end + end + + describe '#in_locked_and_timed_satellite' do + + it 'should make use of a lockfile' do + repo = project.satellite.repo + called = false + + #set assumptions + FileUtils.rm_f(project.satellite.lock_file) + + expect(File.exists?(project.satellite.lock_file)).to be_falsey + + satellite_action = Gitlab::Satellite::Action.new(user, project) + satellite_action.send(:in_locked_and_timed_satellite) do |sat_repo| + expect(repo).to eq(sat_repo) + expect(File.exists? project.satellite.lock_file).to be_truthy + called = true + end + + expect(called).to be_truthy + + end + + it 'should be able to use the satellite after locking' do + repo = project.satellite.repo + called = false + + # Set base assumptions + if File.exists? project.satellite.lock_file + expect(FileLockStatusChecker.new(project.satellite.lock_file).flocked?).to be_falsey + end + + satellite_action = Gitlab::Satellite::Action.new(user, project) + satellite_action.send(:in_locked_and_timed_satellite) do |sat_repo| + called = true + expect(repo).to eq(sat_repo) + expect(File.exists? project.satellite.lock_file).to be_truthy + expect(FileLockStatusChecker.new(project.satellite.lock_file).flocked?).to be_truthy + end + + expect(called).to be_truthy + expect(FileLockStatusChecker.new(project.satellite.lock_file).flocked?).to be_falsey + + end + + class FileLockStatusChecker < File + def flocked?(&block) + status = flock LOCK_EX|LOCK_NB + case status + when false + return true + when 0 + begin + block ? block.call : false + ensure + flock LOCK_UN + end + else + raise SystemCallError, status + end + end + end + + end +end diff --git a/spec/lib/gitlab/satellite/merge_action_spec.rb b/spec/lib/gitlab/satellite/merge_action_spec.rb new file mode 100644 index 00000000000..9b1c9a34e29 --- /dev/null +++ b/spec/lib/gitlab/satellite/merge_action_spec.rb @@ -0,0 +1,104 @@ +require 'spec_helper' + +describe 'Gitlab::Satellite::MergeAction' do + include RepoHelpers + + let(:project) { create(:project, namespace: create(:group)) } + let(:fork_project) { create(:project, namespace: create(:group), forked_from_project: project) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:merge_request_fork) { create(:merge_request, source_project: fork_project, target_project: project) } + + let(:merge_request_with_conflict) { create(:merge_request, :conflict, source_project: project, target_project: project) } + let(:merge_request_fork_with_conflict) { create(:merge_request, :conflict, source_project: project, target_project: project) } + + describe '#commits_between' do + def verify_commits(commits, first_commit_sha, last_commit_sha) + commits.each { |commit| expect(commit.class).to eq(Gitlab::Git::Commit) } + expect(commits.first.id).to eq(first_commit_sha) + expect(commits.last.id).to eq(last_commit_sha) + end + + context 'on fork' do + it 'should get proper commits between' do + commits = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).commits_between + verify_commits(commits, sample_compare.commits.first, sample_compare.commits.last) + end + end + + context 'between branches' do + it 'should raise exception -- not expected to be used by non forks' do + expect { Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).commits_between }.to raise_error(RuntimeError) + end + end + end + + describe '#format_patch' do + def verify_content(patch) + sample_compare.commits.each do |commit| + expect(patch.include?(commit)).to be_truthy + end + end + + context 'on fork' do + it 'should build a format patch' do + patch = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).format_patch + verify_content(patch) + end + end + + context 'between branches' do + it 'should build a format patch' do + patch = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request).format_patch + verify_content(patch) + end + end + end + + describe '#diffs_between_satellite tested against diff_in_satellite' do + def is_a_matching_diff(diff, diffs) + diff_count = diff.scan('diff --git').size + expect(diff_count).to be >= 1 + expect(diffs.size).to eq(diff_count) + diffs.each do |a_diff| + expect(a_diff.class).to eq(Gitlab::Git::Diff) + expect(diff.include? a_diff.diff).to be_truthy + end + end + + context 'on fork' do + it 'should get proper diffs' do + diffs = Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).diffs_between_satellite + diff = Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request_fork).diff_in_satellite + is_a_matching_diff(diff, diffs) + end + end + + context 'between branches' do + it 'should get proper diffs' do + expect{ Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).diffs_between_satellite }.to raise_error(RuntimeError) + end + end + end + + describe '#can_be_merged?' do + context 'on fork' do + it do + expect(Gitlab::Satellite::MergeAction.new(merge_request_fork.author, merge_request_fork).can_be_merged?).to be_truthy + end + + it do + expect(Gitlab::Satellite::MergeAction.new(merge_request_fork_with_conflict.author, merge_request_fork_with_conflict).can_be_merged?).to be_falsey + end + end + + context 'between branches' do + it do + expect(Gitlab::Satellite::MergeAction.new(merge_request.author, merge_request).can_be_merged?).to be_truthy + end + + it do + expect(Gitlab::Satellite::MergeAction.new(merge_request_with_conflict.author, merge_request_with_conflict).can_be_merged?).to be_falsey + end + end + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index b91687bc09f..76f6d8c54c4 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -165,7 +165,7 @@ describe MergeRequest do end it_behaves_like 'an editable mentionable' do - subject { create(:merge_request) } + subject { create(:merge_request, source_project: project) } let(:backref_text) { "merge request #{subject.to_reference}" } let(:set_mentionable_text) { ->(txt){ subject.description = txt } } diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/slack_service_spec.rb index 97b60e19e40..69466b11f09 100644 --- a/spec/models/project_services/slack_service_spec.rb +++ b/spec/models/project_services/slack_service_spec.rb @@ -67,7 +67,7 @@ describe SlackService do opts = { title: 'Awesome merge_request', description: 'please fix', - source_branch: 'feature', + source_branch: 'stable', target_branch: 'master' } merge_service = MergeRequests::CreateService.new(project, diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2fcbd5ae108..5d40754d59d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -91,6 +91,7 @@ describe Project do describe 'Respond to' do it { is_expected.to respond_to(:url_to_repo) } it { is_expected.to respond_to(:repo_exists?) } + it { is_expected.to respond_to(:satellite) } it { is_expected.to respond_to(:update_merge_requests) } it { is_expected.to respond_to(:execute_hooks) } it { is_expected.to respond_to(:name_with_namespace) } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index d96244f23e0..d25351b0f0e 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -36,13 +36,13 @@ describe Repository do describe :can_be_merged? do context 'mergeable branches' do - subject { repository.can_be_merged?('0b4bc9a49b562e85de7cc9e834518ea6828729b9', 'master') } + subject { repository.can_be_merged?('feature', 'master') } it { is_expected.to be_truthy } end context 'non-mergeable branches' do - subject { repository.can_be_merged?('bb5206fee213d983da88c47f9cf4cc6caf9c66dc', 'feature') } + subject { repository.can_be_merged?('feature_conflict', 'feature') } it { is_expected.to be_falsey } end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 8cb8790c339..6c7860511e8 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -110,7 +110,7 @@ describe API::API, api: true do expect(response.status).to eq(400) end - it "should return a 400 if fails to create file" do + it "should return a 400 if satellite fails to create file" do allow_any_instance_of(Repository).to receive(:remove_file).and_return(false) delete api("/projects/#{project.id}/repository/files", user), valid_params diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 942768fa254..29db035b2de 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -148,7 +148,7 @@ describe API::API, api: true do it "should return merge_request" do post api("/projects/#{project.id}/merge_requests", user), title: 'Test merge_request', - source_branch: 'feature_conflict', + source_branch: 'stable', target_branch: 'master', author: user, labels: 'label, label2' @@ -171,20 +171,20 @@ describe API::API, api: true do it "should return 400 when target_branch is missing" do post api("/projects/#{project.id}/merge_requests", user), - title: "Test merge_request", source_branch: "markdown", author: user + title: "Test merge_request", source_branch: "stable", author: user expect(response.status).to eq(400) end it "should return 400 when title is missing" do post api("/projects/#{project.id}/merge_requests", user), - target_branch: 'master', source_branch: 'markdown' + target_branch: 'master', source_branch: 'stable' expect(response.status).to eq(400) end it 'should return 400 on invalid label names' do post api("/projects/#{project.id}/merge_requests", user), title: 'Test merge_request', - source_branch: 'markdown', + source_branch: 'stable', target_branch: 'master', author: user, labels: 'label, ?' @@ -198,7 +198,7 @@ describe API::API, api: true do before do post api("/projects/#{project.id}/merge_requests", user), title: 'Test merge_request', - source_branch: 'feature_conflict', + source_branch: 'stable', target_branch: 'master', author: user @mr = MergeRequest.all.last @@ -208,7 +208,7 @@ describe API::API, api: true do expect do post api("/projects/#{project.id}/merge_requests", user), title: 'New test merge_request', - source_branch: 'feature_conflict', + source_branch: 'stable', target_branch: 'master', author: user end.to change { MergeRequest.count }.by(0) @@ -228,8 +228,7 @@ describe API::API, api: true do it "should return merge_request" do post api("/projects/#{fork_project.id}/merge_requests", user2), - title: 'Test merge_request', source_branch: "feature_conflict", target_branch: "master", - author: user2, target_project_id: project.id, description: 'Test description for Test merge_request' + title: 'Test merge_request', source_branch: "stable", target_branch: "master", author: user2, target_project_id: project.id, description: 'Test description for Test merge_request' expect(response.status).to eq(201) expect(json_response['title']).to eq('Test merge_request') expect(json_response['description']).to eq('Test description for Test merge_request') @@ -259,7 +258,7 @@ describe API::API, api: true do it "should return 400 when title is missing" do post api("/projects/#{fork_project.id}/merge_requests", user2), - target_branch: 'master', source_branch: 'markdown', author: user2, target_project_id: project.id + target_branch: 'master', source_branch: 'stable', author: user2, target_project_id: project.id expect(response.status).to eq(400) end @@ -268,7 +267,7 @@ describe API::API, api: true do post api("/projects/#{project.id}/merge_requests", user), title: 'Test merge_request', target_branch: 'master', - source_branch: 'markdown', + source_branch: 'stable', author: user, target_project_id: fork_project.id expect(response.status).to eq(422) @@ -278,7 +277,7 @@ describe API::API, api: true do post api("/projects/#{fork_project.id}/merge_requests", user2), title: 'Test merge_request', target_branch: 'master', - source_branch: 'markdown', + source_branch: 'stable', author: user2, target_project_id: unrelated_project.id expect(response.status).to eq(422) @@ -287,7 +286,7 @@ describe API::API, api: true do it "should return 201 when target_branch is specified and for the same project" do post api("/projects/#{fork_project.id}/merge_requests", user2), - title: 'Test merge_request', target_branch: 'master', source_branch: 'markdown', author: user2, target_project_id: fork_project.id + title: 'Test merge_request', target_branch: 'master', source_branch: 'stable', author: user2, target_project_id: fork_project.id expect(response.status).to eq(201) end end @@ -303,6 +302,9 @@ describe API::API, api: true do describe "PUT /projects/:id/merge_request/:merge_request_id/merge" do it "should return merge_request in case of success" do + allow_any_instance_of(MergeRequest). + to receive_messages(can_be_merged?: true, automerge!: true) + put api("/projects/#{project.id}/merge_request/#{merge_request.id}/merge", user) expect(response.status).to eq(200) diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index 82f62a8709c..0040718d9be 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -210,8 +210,8 @@ end # diffs_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/diffs(.:format) projects/merge_requests#diffs # commits_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/commits(.:format) projects/merge_requests#commits -# merge_namespace_project_merge_request POST /:namespace_id/:project_id/merge_requests/:id/merge(.:format) projects/merge_requests#merge -# merge_check_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/merge_check(.:format) projects/merge_requests#merge_check +# automerge_namespace_project_merge_request POST /:namespace_id/:project_id/merge_requests/:id/automerge(.:format) projects/merge_requests#automerge +# automerge_check_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/automerge_check(.:format) projects/merge_requests#automerge_check # ci_status_namespace_project_merge_request GET /:namespace_id/:project_id/merge_requests/:id/ci_status(.:format) projects/merge_requests#ci_status # toggle_subscription_namespace_project_merge_request POST /:namespace_id/:project_id/merge_requests/:id/toggle_subscription(.:format) projects/merge_requests#toggle_subscription # branch_from_namespace_project_merge_requests GET /:namespace_id/:project_id/merge_requests/branch_from(.:format) projects/merge_requests#branch_from @@ -233,15 +233,15 @@ describe Projects::MergeRequestsController, 'routing' do expect(get('/gitlab/gitlabhq/merge_requests/1/commits')).to route_to('projects/merge_requests#commits', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') end - it 'to #merge' do - expect(post('/gitlab/gitlabhq/merge_requests/1/merge')).to route_to( - 'projects/merge_requests#merge', + it 'to #automerge' do + expect(post('/gitlab/gitlabhq/merge_requests/1/automerge')).to route_to( + 'projects/merge_requests#automerge', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1' ) end - it 'to #merge_check' do - expect(get('/gitlab/gitlabhq/merge_requests/1/merge_check')).to route_to('projects/merge_requests#merge_check', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') + it 'to #automerge_check' do + expect(get('/gitlab/gitlabhq/merge_requests/1/automerge_check')).to route_to('projects/merge_requests#automerge_check', namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') end it 'to #branch_from' do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index cc64d69361e..d9bfdf64308 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -10,7 +10,7 @@ describe MergeRequests::CreateService do { title: 'Awesome merge_request', description: 'please fix', - source_branch: 'feature', + source_branch: 'stable', target_branch: 'master' } end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 7b564d34d7b..0a25fb12f4e 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -24,6 +24,11 @@ describe MergeRequests::MergeService do it { expect(merge_request).to be_valid } it { expect(merge_request).to be_merged } + it 'should execute hooks with merge action' do + expect(service).to have_received(:execute_hooks). + with(merge_request, 'merge') + end + it 'should send email to user2 about merge of new merge_request' do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) diff --git a/spec/support/mentionable_shared_examples.rb b/spec/support/mentionable_shared_examples.rb index e3de0afb448..f0717e61781 100644 --- a/spec/support/mentionable_shared_examples.rb +++ b/spec/support/mentionable_shared_examples.rb @@ -9,7 +9,7 @@ def common_mentionable_setup let(:author) { subject.author } let(:mentioned_issue) { create(:issue, project: project) } - let!(:mentioned_mr) { create(:merge_request, :simple, source_project: project) } + let(:mentioned_mr) { create(:merge_request, :simple, source_project: project) } let(:mentioned_commit) { project.commit } let(:ext_proj) { create(:project, :public) } -- cgit v1.2.1 From 32046983ad0a702b9c091d052859a13660497dcd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 10:48:02 +0200 Subject: Revert "Merge branch 'refactor-can-be-merge' into 'master'" This reverts commit 459e6d346768d9d8fceaee00bf0870b8e7c4ed9a, reversing changes made to 804168e1def1204af712febb229f41a3865f085f. Signed-off-by: Dmitriy Zaporozhets --- app/models/merge_request.rb | 8 +++++++- app/models/repository.rb | 9 --------- spec/models/repository_spec.rb | 14 -------------- 3 files changed, 7 insertions(+), 24 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1ef76d16700..5a48fe66281 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -209,7 +209,13 @@ class MergeRequest < ActiveRecord::Base if for_fork? Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged? else - project.repository.can_be_merged?(source_branch, target_branch) + rugged = project.repository.rugged + our_commit = rugged.branches[target_branch].target + their_commit = rugged.branches[source_branch].target + + if our_commit && their_commit + !rugged.merge_commits(our_commit, their_commit).conflicts? + end end if can_be_merged diff --git a/app/models/repository.rb b/app/models/repository.rb index 807b33b2a3e..638e278b49e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -411,15 +411,6 @@ class Repository } end - def can_be_merged?(source_branch, target_branch) - our_commit = rugged.branches[target_branch].target - their_commit = rugged.branches[source_branch].target - - if our_commit && their_commit - !rugged.merge_commits(our_commit, their_commit).conflicts? - end - end - def search_files(query, ref) offset = 2 args = %W(git grep -i -n --before-context #{offset} --after-context #{offset} #{query} #{ref || root_ref}) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index d25351b0f0e..0927cde61a6 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -34,20 +34,6 @@ describe Repository do end end - describe :can_be_merged? do - context 'mergeable branches' do - subject { repository.can_be_merged?('feature', 'master') } - - it { is_expected.to be_truthy } - end - - context 'non-mergeable branches' do - subject { repository.can_be_merged?('feature_conflict', 'feature') } - - it { is_expected.to be_falsey } - end - end - describe "search_files" do let(:results) { repository.search_files('feature', 'master') } subject { results } -- cgit v1.2.1 From 3b02ad846723888a8468f5be88bcbb5955028b6d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 10:50:39 +0200 Subject: Revert "Merge branches inside one repository using rugged instead of satellites" This reverts commit d24c40ec219d76e01e2fab5f6ebf431adae91bdd. --- app/models/merge_request.rb | 15 +------ app/models/repository.rb | 2 + app/services/merge_requests/auto_merge_service.rb | 51 ++--------------------- 3 files changed, 6 insertions(+), 62 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5a48fe66281..324d1795ab4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -205,20 +205,7 @@ class MergeRequest < ActiveRecord::Base end def check_if_can_be_merged - can_be_merged = - if for_fork? - Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged? - else - rugged = project.repository.rugged - our_commit = rugged.branches[target_branch].target - their_commit = rugged.branches[source_branch].target - - if our_commit && their_commit - !rugged.merge_commits(our_commit, their_commit).conflicts? - end - end - - if can_be_merged + if Gitlab::Satellite::MergeAction.new(self.author, self).can_be_merged? mark_as_mergeable else mark_as_unmergeable diff --git a/app/models/repository.rb b/app/models/repository.rb index 638e278b49e..4394f948e12 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -403,6 +403,8 @@ class Repository Gitlab::Git::Blob.remove(raw_repository, options) end + private + def user_to_comitter(user) { email: user.email, diff --git a/app/services/merge_requests/auto_merge_service.rb b/app/services/merge_requests/auto_merge_service.rb index db824d452d0..cdedf48b0c0 100644 --- a/app/services/merge_requests/auto_merge_service.rb +++ b/app/services/merge_requests/auto_merge_service.rb @@ -5,20 +5,17 @@ module MergeRequests # mark merge request as merged and execute all hooks and notifications # Called when you do merge via GitLab UI class AutoMergeService < BaseMergeService - attr_reader :merge_request, :commit_message - def execute(merge_request, commit_message) - @commit_message = commit_message - @merge_request = merge_request - merge_request.lock_mr - if merge! + if Gitlab::Satellite::MergeAction.new(current_user, merge_request).merge!(commit_message) merge_request.merge + create_merge_event(merge_request, current_user) create_note(merge_request) notification_service.merge_mr(merge_request, current_user) execute_hooks(merge_request, 'merge') + true else merge_request.unlock_mr @@ -29,47 +26,5 @@ module MergeRequests merge_request.mark_as_unmergeable false end - - def merge! - if merge_request.for_fork? - Gitlab::Satellite::MergeAction.new(current_user, merge_request).merge!(commit_message) - else - # Merge local branches using rugged instead of satellites - if sha = commit - after_commit(sha, merge_request.target_branch) - - if merge_request.remove_source_branch? - DeleteBranchService.new(merge_request.source_project, current_user).execute(merge_request.source_branch) - end - - true - else - false - end - end - end - - def commit - committer = repository.user_to_comitter(current_user) - - options = { - message: commit_message, - author: committer, - committer: committer - } - - repository.merge(merge_request.source_branch, merge_request.target_branch, options) - end - - def after_commit(sha, branch) - commit = repository.commit(sha) - full_ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch}" - old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA - GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) - end - - def repository - project.repository - end end end -- cgit v1.2.1 From 1891cab44ce63e63c6606675021f8eaf81444aee Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:12:35 +0200 Subject: Fix conflict issue Signed-off-by: Dmitriy Zaporozhets --- app/models/repository.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index 4394f948e12..638e278b49e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -403,8 +403,6 @@ class Repository Gitlab::Git::Blob.remove(raw_repository, options) end - private - def user_to_comitter(user) { email: user.email, -- cgit v1.2.1 From 79b294267aba9e8c5e305620f0f00e4a9b581c23 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:40:39 +0200 Subject: Remove changelog items Signed-off-by: Dmitriy Zaporozhets --- CHANGELOG | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ebffc0d90ab..60848dcf57e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -42,7 +42,6 @@ v 7.14.0 (unreleased) - Remove redis-store TTL monkey patch - Add support for CI skipped status - Fetch code from forks to refs/merge-requests/:id/head when merge request created - - Remove satellites - Remove comments and email addresses when publicly exposing ssh keys (Zeger-Jan van de Weg) - Cache all events @@ -190,7 +189,6 @@ v 7.12.0 - Add SAML support as an omniauth provider - Allow to configure a URL to show after sign out - Add an option to automatically sign-in with an Omniauth provider - - Better performance for web editor (switched from satellites to rugged) - GitLab CI service sends .gitlab-ci.yml in each push call - When remove project - move repository and schedule it removal - Improve group removing logic -- cgit v1.2.1 From d4cfa0bf766bb99cb6b8e600faee1e90678816b4 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:49:09 +0200 Subject: Revert "Refactor web editor" This reverts commit dfccb06dda344819989fa8d6a9a3c56c5ca0b65f. Signed-off-by: Dmitriy Zaporozhets --- Gemfile.lock | 4 ++ app/controllers/projects/blob_controller.rb | 60 +++++++++++----------- app/services/files/base_service.rb | 77 +++-------------------------- app/services/files/create_service.rb | 50 +++++++++++++++---- app/services/files/delete_service.rb | 32 +++++++++++- app/services/files/update_service.rb | 42 +++++++++++++++- app/views/projects/blob/new.html.haml | 11 ++--- 7 files changed, 156 insertions(+), 120 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 53d21b26e97..7f8db0606e5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -783,7 +783,11 @@ DEPENDENCIES gitlab-grack (~> 2.0.2) gitlab-linguist (~> 3.0.1) gitlab_emoji (~> 0.1) +<<<<<<< HEAD gitlab_git (~> 7.2.6) +======= + gitlab_git (~> 7.2.2) +>>>>>>> parent of dfccb06... Refactor web editor gitlab_meta (= 7.0) gitlab_omniauth-ldap (= 1.2.1) gollum-lib (~> 4.0.2) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 100d3d3b317..b762518d377 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -13,20 +13,27 @@ class Projects::BlobController < Projects::ApplicationController before_action :commit, except: [:new, :create] before_action :blob, except: [:new, :create] before_action :from_merge_request, only: [:edit, :update] - before_action :require_branch_head, only: [:edit, :update] - before_action :editor_variables, except: [:show, :preview, :diff] before_action :after_edit_path, only: [:edit, :update] + before_action :require_branch_head, only: [:edit, :update] def new commit unless @repository.empty? end def create - result = Files::CreateService.new(@project, current_user, @commit_params).execute + file_path = File.join(@path, File.basename(params[:file_name])) + result = Files::CreateService.new( + @project, + current_user, + params.merge(new_branch: sanitized_new_branch_name), + @ref, + file_path + ).execute if result[:status] == :success flash[:notice] = "Your changes have been successfully committed" - redirect_to namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @file_path)) + ref = sanitized_new_branch_name.presence || @ref + redirect_to namespace_project_blob_path(@project.namespace, @project, File.join(ref, file_path)) else flash[:alert] = result[:message] render :new @@ -41,10 +48,22 @@ class Projects::BlobController < Projects::ApplicationController end def update - result = Files::UpdateService.new(@project, current_user, @commit_params).execute + result = Files::UpdateService. + new( + @project, + current_user, + params.merge(new_branch: sanitized_new_branch_name), + @ref, + @path + ).execute if result[:status] == :success flash[:notice] = "Your changes have been successfully committed" + + if from_merge_request + from_merge_request.reload_code + end + redirect_to after_edit_path else flash[:alert] = result[:message] @@ -61,11 +80,12 @@ class Projects::BlobController < Projects::ApplicationController end def destroy - result = Files::DeleteService.new(@project, current_user, @commit_params).execute + result = Files::DeleteService.new(@project, current_user, params, @ref, @path).execute if result[:status] == :success flash[:notice] = "Your changes have been successfully committed" - redirect_to namespace_project_tree_path(@project.namespace, @project, @target_branch) + redirect_to namespace_project_tree_path(@project.namespace, @project, + @ref) else flash[:alert] = result[:message] render :show @@ -115,6 +135,7 @@ class Projects::BlobController < Projects::ApplicationController @id = params[:id] @ref, @path = extract_ref(@id) + rescue InvalidPathError not_found! end @@ -124,8 +145,8 @@ class Projects::BlobController < Projects::ApplicationController if from_merge_request diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) + "#file-path-#{hexdigest(@path)}" - elsif @target_branch.present? - namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path)) + elsif sanitized_new_branch_name.present? + namespace_project_blob_path(@project.namespace, @project, File.join(sanitized_new_branch_name, @path)) else namespace_project_blob_path(@project.namespace, @project, @id) end @@ -139,25 +160,4 @@ class Projects::BlobController < Projects::ApplicationController def sanitized_new_branch_name @new_branch ||= sanitize(strip_tags(params[:new_branch])) end - - def editor_variables - @current_branch = @ref - @target_branch = (sanitized_new_branch_name || @ref) - - @file_path = - if action_name.to_s == 'create' - File.join(@path, File.basename(params[:file_name])) - else - @path - end - - @commit_params = { - file_path: @file_path, - current_branch: @current_branch, - target_branch: @target_branch, - commit_message: params[:commit_message], - file_content: params[:content], - file_content_encoding: params[:encoding] - } - end end diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 646784f2d9d..4d02752454e 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -1,34 +1,11 @@ module Files class BaseService < ::BaseService - class ValidationError < StandardError; end + attr_reader :ref, :path - def execute - @current_branch = params[:current_branch] - @target_branch = params[:target_branch] - @commit_message = params[:commit_message] - @file_path = params[:file_path] - @file_content = if params[:file_content_encoding] == 'base64' - Base64.decode64(params[:file_content]) - else - params[:file_content] - end - - # Validate parameters - validate - - # Create new branch if it different from current_branch - if @target_branch != @current_branch - create_target_branch - end - - if sha = commit - after_commit(sha, @target_branch) - success - else - error("Something went wrong. Your changes were not committed") - end - rescue ValidationError => ex - error(ex.message) + def initialize(project, user, params, ref, path = nil) + @project, @current_user, @params = project, user, params.dup + @ref = ref + @path = path end private @@ -37,51 +14,11 @@ module Files project.repository end - def after_commit(sha, branch) + def after_commit(sha) commit = repository.commit(sha) - full_ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch}" + full_ref = 'refs/heads/' + (params[:new_branch] || ref) old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) end - - def current_branch - @current_branch ||= params[:current_branch] - end - - def target_branch - @target_branch ||= params[:target_branch] - end - - def raise_error(message) - raise ValidationError.new(message) - end - - def validate - allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(@target_branch) - - unless allowed - raise_error("You are not allowed to push into this branch") - end - - unless project.empty_repo? - unless repository.branch_names.include?(@current_branch) - raise_error("You can only create files if you are on top of a branch") - end - - if @current_branch != @target_branch - if repository.branch_names.include?(@target_branch) - raise_error("Branch with such name already exists. You need to switch to this branch in order to make changes") - end - end - end - end - - def create_target_branch - result = CreateBranchService.new(project, current_user).execute(@target_branch, @current_branch) - - unless result[:status] == :success - raise_error("Something went wrong when we tried to create #{@target_branch} for you") - end - end end end diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index 91d715b2d63..0a80455bc6b 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -2,28 +2,58 @@ require_relative "base_service" module Files class CreateService < Files::BaseService - def commit - repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch) - end + def execute + allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) - def validate - super + unless allowed + return error("You are not allowed to create file in this branch") + end - file_name = File.basename(@file_path) + file_name = File.basename(path) + file_path = path unless file_name =~ Gitlab::Regex.file_name_regex - raise_error( + return error( 'Your changes could not be committed, because the file name ' + Gitlab::Regex.file_name_regex_message ) end - unless project.empty_repo? - blob = repository.blob_at_branch(@current_branch, @file_path) + if project.empty_repo? + # everything is ok because repo does not have a commits yet + else + unless repository.branch_names.include?(ref) + return error("You can only create files if you are on top of a branch") + end + + blob = repository.blob_at_branch(ref, file_path) if blob - raise_error("Your changes could not be committed, because file with such name exists") + return error("Your changes could not be committed, because file with such name exists") + end + end + + content = + if params[:encoding] == 'base64' + Base64.decode64(params[:content]) + else + params[:content] end + + sha = repository.commit_file( + current_user, + file_path, + content, + params[:commit_message], + params[:new_branch] || ref + ) + + + if sha + after_commit(sha) + success + else + error("Your changes could not be committed, because the file has been changed") end end end diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index 27c881c3430..2281777604c 100644 --- a/app/services/files/delete_service.rb +++ b/app/services/files/delete_service.rb @@ -2,8 +2,36 @@ require_relative "base_service" module Files class DeleteService < Files::BaseService - def commit - repository.remove_file(current_user, @file_path, @commit_message, @target_branch) + def execute + allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) + + unless allowed + return error("You are not allowed to push into this branch") + end + + unless repository.branch_names.include?(ref) + return error("You can only create files if you are on top of a branch") + end + + blob = repository.blob_at_branch(ref, path) + + unless blob + return error("You can only edit text files") + end + + sha = repository.remove_file( + current_user, + path, + params[:commit_message], + ref + ) + + if sha + after_commit(sha) + success + else + error("Your changes could not be committed, because the file has been changed") + end end end end diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index a20903c6f02..013cc1ee322 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -2,8 +2,46 @@ require_relative "base_service" module Files class UpdateService < Files::BaseService - def commit - repository.commit_file(current_user, @file_path, @file_content, @commit_message, @target_branch) + def execute + allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) + + unless allowed + return error("You are not allowed to push into this branch") + end + + unless repository.branch_names.include?(ref) + return error("You can only create files if you are on top of a branch") + end + + blob = repository.blob_at_branch(ref, path) + + unless blob + return error("You can only edit text files") + end + + content = + if params[:encoding] == 'base64' + Base64.decode64(params[:content]) + else + params[:content] + end + + sha = repository.commit_file( + current_user, + path, + content, + params[:commit_message], + params[:new_branch] || ref + ) + + after_commit(sha) + success + rescue Gitlab::Satellite::CheckoutFailed => ex + error("Your changes could not be committed because ref '#{ref}' could not be checked out", 400) + rescue Gitlab::Satellite::CommitFailed => ex + error("Your changes could not be committed. Maybe there was nothing to commit?", 409) + rescue Gitlab::Satellite::PushFailed => ex + error("Your changes could not be committed. Maybe the file was changed by another process?", 409) end end end diff --git a/app/views/projects/blob/new.html.haml b/app/views/projects/blob/new.html.haml index 7c2a4fece94..dac984f8c31 100644 --- a/app/views/projects/blob/new.html.haml +++ b/app/views/projects/blob/new.html.haml @@ -6,12 +6,11 @@ = render 'shared/commit_message_container', params: params, placeholder: 'Add new file' - - unless @project.empty_repo? - .form-group.branch - = label_tag 'branch', class: 'control-label' do - Branch - .col-sm-10 - = text_field_tag 'new_branch', @ref, class: "form-control" + .form-group.branch + = label_tag 'branch', class: 'control-label' do + Branch + .col-sm-10 + = text_field_tag 'new_branch', @ref, class: "form-control" = hidden_field_tag 'content', '', id: 'file-content' = render 'projects/commit_button', ref: @ref, -- cgit v1.2.1 From 24332b7b99e63083a4914f149c43bf8cce5e7a92 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:49:29 +0200 Subject: Revert "Make web editor work correctly after switch from satellites" This reverts commit 435f680b897b892103fa157d4699dbb6d9ecf758. --- app/services/files/base_service.rb | 3 +-- app/services/files/create_service.rb | 2 +- app/services/files/delete_service.rb | 2 +- app/services/files/update_service.rb | 2 +- app/services/git_push_service.rb | 3 +-- 5 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 4d02752454e..29013be0f97 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -17,8 +17,7 @@ module Files def after_commit(sha) commit = repository.commit(sha) full_ref = 'refs/heads/' + (params[:new_branch] || ref) - old_sha = commit.parent_id || Gitlab::Git::BLANK_SHA - GitPushService.new.execute(project, current_user, old_sha, sha, full_ref) + GitPushService.new.execute(project, current_user, commit.parent_id, sha, full_ref) end end end diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index 0a80455bc6b..bafc3565da1 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -1,7 +1,7 @@ require_relative "base_service" module Files - class CreateService < Files::BaseService + class CreateService < BaseService def execute allowed = Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index 2281777604c..fabcdc19648 100644 --- a/app/services/files/delete_service.rb +++ b/app/services/files/delete_service.rb @@ -1,7 +1,7 @@ require_relative "base_service" module Files - class DeleteService < Files::BaseService + class DeleteService < BaseService def execute allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 013cc1ee322..c972f8322bb 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -1,7 +1,7 @@ require_relative "base_service" module Files - class UpdateService < Files::BaseService + class UpdateService < BaseService def execute allowed = ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref) diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index 3511392d1d8..5a2c97b08af 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -133,8 +133,7 @@ class GitPushService end def is_default_branch?(ref) - Gitlab::Git.branch_ref?(ref) && - (Gitlab::Git.ref_name(ref) == project.default_branch || project.default_branch.nil?) + Gitlab::Git.branch_ref?(ref) && Gitlab::Git.ref_name(ref) == project.default_branch end def commit_user(commit) -- cgit v1.2.1 From 34975f0180b209cf3ac7cbe5955f83c10aa4c210 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:50:43 +0200 Subject: Revert "Create activity event and execute hooks on web editor commit" This reverts commit 3d416f1682c5e6a6ac1ea7013f66bbd0d23b452c. --- app/services/files/base_service.rb | 6 ------ app/services/files/create_service.rb | 5 ++--- app/services/files/update_service.rb | 3 +-- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index 29013be0f97..bd245100955 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -13,11 +13,5 @@ module Files def repository project.repository end - - def after_commit(sha) - commit = repository.commit(sha) - full_ref = 'refs/heads/' + (params[:new_branch] || ref) - GitPushService.new.execute(project, current_user, commit.parent_id, sha, full_ref) - end end end diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index bafc3565da1..3516cf30dbc 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -40,7 +40,7 @@ module Files params[:content] end - sha = repository.commit_file( + created_successfully = repository.commit_file( current_user, file_path, content, @@ -49,8 +49,7 @@ module Files ) - if sha - after_commit(sha) + if created_successfully success else error("Your changes could not be committed, because the file has been changed") diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index c972f8322bb..4d7ac3b7504 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -26,7 +26,7 @@ module Files params[:content] end - sha = repository.commit_file( + repository.commit_file( current_user, path, content, @@ -34,7 +34,6 @@ module Files params[:new_branch] || ref ) - after_commit(sha) success rescue Gitlab::Satellite::CheckoutFailed => ex error("Your changes could not be committed because ref '#{ref}' could not be checked out", 400) -- cgit v1.2.1 From 815e678c4e331df49ee080af149253be008faf76 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:50:48 +0200 Subject: Revert "Use rugged in web editor for base64 encoding" This reverts commit 541133197be098732cdc14a12aa059e21cac3d71. --- app/services/files/create_service.rb | 31 +++++++++++++++++-------------- app/services/files/update_service.rb | 31 +++++++++++++++++-------------- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index 3516cf30dbc..21065f71510 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -33,20 +33,23 @@ module Files end end - content = - if params[:encoding] == 'base64' - Base64.decode64(params[:content]) - else - params[:content] - end - - created_successfully = repository.commit_file( - current_user, - file_path, - content, - params[:commit_message], - params[:new_branch] || ref - ) + if params[:encoding] == 'base64' + new_file_action = Gitlab::Satellite::NewFileAction.new(current_user, project, ref, file_path) + created_successfully = new_file_action.commit!( + params[:content], + params[:commit_message], + params[:encoding], + params[:new_branch] + ) + else + created_successfully = repository.commit_file( + current_user, + file_path, + params[:content], + params[:commit_message], + params[:new_branch] || ref + ) + end if created_successfully diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 4d7ac3b7504..5efd43d16ce 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -19,20 +19,23 @@ module Files return error("You can only edit text files") end - content = - if params[:encoding] == 'base64' - Base64.decode64(params[:content]) - else - params[:content] - end - - repository.commit_file( - current_user, - path, - content, - params[:commit_message], - params[:new_branch] || ref - ) + if params[:encoding] == 'base64' + edit_file_action = Gitlab::Satellite::EditFileAction.new(current_user, project, ref, path) + edit_file_action.commit!( + params[:content], + params[:commit_message], + params[:encoding], + params[:new_branch] + ) + else + repository.commit_file( + current_user, + path, + params[:content], + params[:commit_message], + params[:new_branch] || ref + ) + end success rescue Gitlab::Satellite::CheckoutFailed => ex -- cgit v1.2.1 From 08066059623184dbd55f0591fa05adc18ed80580 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:51:26 +0200 Subject: Revert "Fix adding new file to empty repo" This reverts commit 27a158506e033acd7195acf91995c1574e122832. Signed-off-by: Dmitriy Zaporozhets --- Gemfile.lock | 4 ++++ app/services/files/create_service.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 7f8db0606e5..51d590778b3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -783,11 +783,15 @@ DEPENDENCIES gitlab-grack (~> 2.0.2) gitlab-linguist (~> 3.0.1) gitlab_emoji (~> 0.1) +<<<<<<< HEAD <<<<<<< HEAD gitlab_git (~> 7.2.6) ======= gitlab_git (~> 7.2.2) >>>>>>> parent of dfccb06... Refactor web editor +======= + gitlab_git (~> 7.2.0) +>>>>>>> parent of 27a1585... Fix adding new file to empty repo gitlab_meta (= 7.0) gitlab_omniauth-ldap (= 1.2.1) gollum-lib (~> 4.0.2) diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index 21065f71510..c0cf5956326 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -42,7 +42,7 @@ module Files params[:new_branch] ) else - created_successfully = repository.commit_file( + created_successfull = repository.commit_file( current_user, file_path, params[:content], -- cgit v1.2.1 From 9bc512314a6a11e075539ada1d1d032b6992d039 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:52:33 +0200 Subject: Revert "Create and edit files in web editor via rugged" This reverts commit 734a4ba87de7bc8cf152c5bc7f93ba04210b282d. Signed-off-by: Dmitriy Zaporozhets --- Gemfile.lock | 4 +++ app/models/repository.rb | 47 ------------------------------------ app/services/files/create_service.rb | 24 ++++++------------ app/services/files/update_service.rb | 24 ++++++------------ 4 files changed, 18 insertions(+), 81 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 51d590778b3..a8d86c88ba6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -784,6 +784,7 @@ DEPENDENCIES gitlab-linguist (~> 3.0.1) gitlab_emoji (~> 0.1) <<<<<<< HEAD +<<<<<<< HEAD <<<<<<< HEAD gitlab_git (~> 7.2.6) ======= @@ -792,6 +793,9 @@ DEPENDENCIES ======= gitlab_git (~> 7.2.0) >>>>>>> parent of 27a1585... Fix adding new file to empty repo +======= + gitlab_git (~> 7.1.13) +>>>>>>> parent of 734a4ba... Create and edit files in web editor via rugged gitlab_meta (= 7.0) gitlab_omniauth-ldap (= 1.2.1) gollum-lib (~> 4.0.2) diff --git a/app/models/repository.rb b/app/models/repository.rb index 638e278b49e..0a62980f93a 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -364,53 +364,6 @@ class Repository @root_ref ||= raw_repository.root_ref end - def commit_file(user, path, content, message, ref) - path[0] = '' if path[0] == '/' - - committer = user_to_comitter(user) - options = {} - options[:committer] = committer - options[:author] = committer - options[:commit] = { - message: message, - branch: ref - } - - options[:file] = { - content: content, - path: path - } - - Gitlab::Git::Blob.commit(raw_repository, options) - end - - def remove_file(user, path, message, ref) - path[0] = '' if path[0] == '/' - - committer = user_to_comitter(user) - options = {} - options[:committer] = committer - options[:author] = committer - options[:commit] = { - message: message, - branch: ref - } - - options[:file] = { - path: path - } - - Gitlab::Git::Blob.remove(raw_repository, options) - end - - def user_to_comitter(user) - { - email: user.email, - name: user.name, - time: Time.now - } - end - def search_files(query, ref) offset = 2 args = %W(git grep -i -n --before-context #{offset} --after-context #{offset} #{query} #{ref || root_ref}) diff --git a/app/services/files/create_service.rb b/app/services/files/create_service.rb index c0cf5956326..23833aa78ec 100644 --- a/app/services/files/create_service.rb +++ b/app/services/files/create_service.rb @@ -33,24 +33,14 @@ module Files end end - if params[:encoding] == 'base64' - new_file_action = Gitlab::Satellite::NewFileAction.new(current_user, project, ref, file_path) - created_successfully = new_file_action.commit!( - params[:content], - params[:commit_message], - params[:encoding], - params[:new_branch] - ) - else - created_successfull = repository.commit_file( - current_user, - file_path, - params[:content], - params[:commit_message], - params[:new_branch] || ref - ) - end + new_file_action = Gitlab::Satellite::NewFileAction.new(current_user, project, ref, file_path) + created_successfully = new_file_action.commit!( + params[:content], + params[:commit_message], + params[:encoding], + params[:new_branch] + ) if created_successfully success diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index 5efd43d16ce..0724d3ae634 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -19,23 +19,13 @@ module Files return error("You can only edit text files") end - if params[:encoding] == 'base64' - edit_file_action = Gitlab::Satellite::EditFileAction.new(current_user, project, ref, path) - edit_file_action.commit!( - params[:content], - params[:commit_message], - params[:encoding], - params[:new_branch] - ) - else - repository.commit_file( - current_user, - path, - params[:content], - params[:commit_message], - params[:new_branch] || ref - ) - end + edit_file_action = Gitlab::Satellite::EditFileAction.new(current_user, project, ref, path) + edit_file_action.commit!( + params[:content], + params[:commit_message], + params[:encoding], + params[:new_branch] + ) success rescue Gitlab::Satellite::CheckoutFailed => ex -- cgit v1.2.1 From 40b1703263bbeb35e54d21c8f0c2a575d6965a9f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 11:53:49 +0200 Subject: Fix gemfile Signed-off-by: Dmitriy Zaporozhets --- Gemfile.lock | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index a8d86c88ba6..53d21b26e97 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -783,19 +783,7 @@ DEPENDENCIES gitlab-grack (~> 2.0.2) gitlab-linguist (~> 3.0.1) gitlab_emoji (~> 0.1) -<<<<<<< HEAD -<<<<<<< HEAD -<<<<<<< HEAD gitlab_git (~> 7.2.6) -======= - gitlab_git (~> 7.2.2) ->>>>>>> parent of dfccb06... Refactor web editor -======= - gitlab_git (~> 7.2.0) ->>>>>>> parent of 27a1585... Fix adding new file to empty repo -======= - gitlab_git (~> 7.1.13) ->>>>>>> parent of 734a4ba... Create and edit files in web editor via rugged gitlab_meta (= 7.0) gitlab_omniauth-ldap (= 1.2.1) gollum-lib (~> 4.0.2) -- cgit v1.2.1 From 9d029a05f1a6e35228bfb094ad2bd8369b7ee45f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 13:04:38 +0200 Subject: Revert "Merge branch 'web-editor-rugged' into 'master'" This reverts commit 5a1aa49b5533593dc4c6de82279fe44f5f15616c, reversing changes made to a675bea2c1c1d5d6923cb97b8714eb72d4e4ff9b. Signed-off-by: Dmitriy Zaporozhets --- app/services/files/delete_service.rb | 13 +++-- app/views/projects/blob/_editor.html.haml | 4 +- lib/gitlab/satellite/files/delete_file_action.rb | 50 +++++++++++++++++ lib/gitlab/satellite/files/edit_file_action.rb | 68 ++++++++++++++++++++++++ lib/gitlab/satellite/files/file_action.rb | 25 +++++++++ lib/gitlab/satellite/files/new_file_action.rb | 67 +++++++++++++++++++++++ spec/requests/api/files_spec.rb | 52 ++++++++++++++++-- 7 files changed, 266 insertions(+), 13 deletions(-) create mode 100644 lib/gitlab/satellite/files/delete_file_action.rb create mode 100644 lib/gitlab/satellite/files/edit_file_action.rb create mode 100644 lib/gitlab/satellite/files/file_action.rb create mode 100644 lib/gitlab/satellite/files/new_file_action.rb diff --git a/app/services/files/delete_service.rb b/app/services/files/delete_service.rb index fabcdc19648..1497a0f883b 100644 --- a/app/services/files/delete_service.rb +++ b/app/services/files/delete_service.rb @@ -19,15 +19,14 @@ module Files return error("You can only edit text files") end - sha = repository.remove_file( - current_user, - path, - params[:commit_message], - ref + delete_file_action = Gitlab::Satellite::DeleteFileAction.new(current_user, project, ref, path) + + deleted_successfully = delete_file_action.commit!( + nil, + params[:commit_message] ) - if sha - after_commit(sha) + if deleted_successfully success else error("Your changes could not be committed, because the file has been changed") diff --git a/app/views/projects/blob/_editor.html.haml b/app/views/projects/blob/_editor.html.haml index 9c3e1703c89..96f188e4aa7 100644 --- a/app/views/projects/blob/_editor.html.haml +++ b/app/views/projects/blob/_editor.html.haml @@ -12,8 +12,8 @@ \/ = text_field_tag 'file_name', params[:file_name], placeholder: "File name", required: true, class: 'form-control new-file-name' - .pull-right - = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'form-control' + .pull-right + = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'form-control' .file-content.code %pre.js-edit-mode-pane#editor diff --git a/lib/gitlab/satellite/files/delete_file_action.rb b/lib/gitlab/satellite/files/delete_file_action.rb new file mode 100644 index 00000000000..0d37b9dea85 --- /dev/null +++ b/lib/gitlab/satellite/files/delete_file_action.rb @@ -0,0 +1,50 @@ +require_relative 'file_action' + +module Gitlab + module Satellite + class DeleteFileAction < FileAction + # Deletes file and creates a new commit for it + # + # Returns false if committing the change fails + # Returns false if pushing from the satellite to bare repo failed or was rejected + # Returns true otherwise + def commit!(content, commit_message) + in_locked_and_timed_satellite do |repo| + prepare_satellite!(repo) + + # create target branch in satellite at the corresponding commit from bare repo + repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + + # update the file in the satellite's working dir + file_path_in_satellite = File.join(repo.working_dir, file_path) + + # Prevent relative links + unless safe_path?(file_path_in_satellite) + Gitlab::GitLogger.error("FileAction: Relative path not allowed") + return false + end + + File.delete(file_path_in_satellite) + + # add removed file + repo.remove(file_path_in_satellite) + + # commit the changes + # will raise CommandFailed when commit fails + repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + + + # push commit back to bare repo + # will raise CommandFailed when push fails + repo.git.push({ raise: true, timeout: true }, :origin, ref) + + # everything worked + true + end + rescue Grit::Git::CommandFailed => ex + Gitlab::GitLogger.error(ex.message) + false + end + end + end +end diff --git a/lib/gitlab/satellite/files/edit_file_action.rb b/lib/gitlab/satellite/files/edit_file_action.rb new file mode 100644 index 00000000000..3cb9c0b5ecb --- /dev/null +++ b/lib/gitlab/satellite/files/edit_file_action.rb @@ -0,0 +1,68 @@ +require_relative 'file_action' + +module Gitlab + module Satellite + # GitLab server-side file update and commit + class EditFileAction < FileAction + # Updates the files content and creates a new commit for it + # + # Returns false if the ref has been updated while editing the file + # Returns false if committing the change fails + # Returns false if pushing from the satellite to bare repo failed or was rejected + # Returns true otherwise + def commit!(content, commit_message, encoding, new_branch = nil) + in_locked_and_timed_satellite do |repo| + prepare_satellite!(repo) + + # create target branch in satellite at the corresponding commit from bare repo + begin + repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + rescue Grit::Git::CommandFailed => ex + log_and_raise(CheckoutFailed, ex.message) + end + + # update the file in the satellite's working dir + file_path_in_satellite = File.join(repo.working_dir, file_path) + + # Prevent relative links + unless safe_path?(file_path_in_satellite) + Gitlab::GitLogger.error("FileAction: Relative path not allowed") + return false + end + + # Write file + write_file(file_path_in_satellite, content, encoding) + + # commit the changes + # will raise CommandFailed when commit fails + begin + repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + rescue Grit::Git::CommandFailed => ex + log_and_raise(CommitFailed, ex.message) + end + + + target_branch = new_branch.present? ? "#{ref}:#{new_branch}" : ref + + # push commit back to bare repo + # will raise CommandFailed when push fails + begin + repo.git.push({ raise: true, timeout: true }, :origin, target_branch) + rescue Grit::Git::CommandFailed => ex + log_and_raise(PushFailed, ex.message) + end + + # everything worked + true + end + end + + private + + def log_and_raise(errorClass, message) + Gitlab::GitLogger.error(message) + raise(errorClass, message) + end + end + end +end diff --git a/lib/gitlab/satellite/files/file_action.rb b/lib/gitlab/satellite/files/file_action.rb new file mode 100644 index 00000000000..6446b14568a --- /dev/null +++ b/lib/gitlab/satellite/files/file_action.rb @@ -0,0 +1,25 @@ +module Gitlab + module Satellite + class FileAction < Action + attr_accessor :file_path, :ref + + def initialize(user, project, ref, file_path) + super user, project + @file_path = file_path + @ref = ref + end + + def safe_path?(path) + File.absolute_path(path) == path + end + + def write_file(abs_file_path, content, file_encoding = 'text') + if file_encoding == 'base64' + File.open(abs_file_path, 'wb') { |f| f.write(Base64.decode64(content)) } + else + File.open(abs_file_path, 'w') { |f| f.write(content) } + end + end + end + end +end diff --git a/lib/gitlab/satellite/files/new_file_action.rb b/lib/gitlab/satellite/files/new_file_action.rb new file mode 100644 index 00000000000..724dfa0d042 --- /dev/null +++ b/lib/gitlab/satellite/files/new_file_action.rb @@ -0,0 +1,67 @@ +require_relative 'file_action' + +module Gitlab + module Satellite + class NewFileAction < FileAction + # Updates the files content and creates a new commit for it + # + # Returns false if the ref has been updated while editing the file + # Returns false if committing the change fails + # Returns false if pushing from the satellite to bare repo failed or was rejected + # Returns true otherwise + def commit!(content, commit_message, encoding, new_branch = nil) + in_locked_and_timed_satellite do |repo| + prepare_satellite!(repo) + + # create target branch in satellite at the corresponding commit from bare repo + current_ref = + if @project.empty_repo? + # skip this step if we want to add first file to empty repo + Satellite::PARKING_BRANCH + else + repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + ref + end + + file_path_in_satellite = File.join(repo.working_dir, file_path) + dir_name_in_satellite = File.dirname(file_path_in_satellite) + + # Prevent relative links + unless safe_path?(file_path_in_satellite) + Gitlab::GitLogger.error("FileAction: Relative path not allowed") + return false + end + + # Create dir if not exists + FileUtils.mkdir_p(dir_name_in_satellite) + + # Write file + write_file(file_path_in_satellite, content, encoding) + + # add new file + repo.add(file_path_in_satellite) + + # commit the changes + # will raise CommandFailed when commit fails + repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + + target_branch = if new_branch.present? && !@project.empty_repo? + "#{ref}:#{new_branch}" + else + "#{current_ref}:#{ref}" + end + + # push commit back to bare repo + # will raise CommandFailed when push fails + repo.git.push({ raise: true, timeout: true }, :origin, target_branch) + + # everything worked + true + end + rescue Grit::Git::CommandFailed => ex + Gitlab::GitLogger.error(ex.message) + false + end + end + end +end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 6c7860511e8..346f1e29d48 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -49,6 +49,10 @@ describe API::API, api: true do end it "should create a new file in project repo" do + Gitlab::Satellite::NewFileAction.any_instance.stub( + commit!: true, + ) + post api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(201) expect(json_response['file_path']).to eq('newfile.rb') @@ -59,9 +63,10 @@ describe API::API, api: true do expect(response.status).to eq(400) end - it "should return a 400 if editor fails to create file" do - allow_any_instance_of(Repository).to receive(:commit_file). - and_return(false) + it "should return a 400 if satellite fails to create file" do + Gitlab::Satellite::NewFileAction.any_instance.stub( + commit!: false, + ) post api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) @@ -79,6 +84,10 @@ describe API::API, api: true do end it "should update existing file in project repo" do + Gitlab::Satellite::EditFileAction.any_instance.stub( + commit!: true, + ) + put api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(200) expect(json_response['file_path']).to eq(file_path) @@ -88,6 +97,35 @@ describe API::API, api: true do put api("/projects/#{project.id}/repository/files", user) expect(response.status).to eq(400) end + + it 'should return a 400 if the checkout fails' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::CheckoutFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(400) + + ref = valid_params[:branch_name] + expect(response.body).to match("ref '#{ref}' could not be checked out") + end + + it 'should return a 409 if the file was not modified' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::CommitFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(409) + expect(response.body).to match("Maybe there was nothing to commit?") + end + + it 'should return a 409 if the push fails' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::PushFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(409) + expect(response.body).to match("Maybe the file was changed by another process?") + end end describe "DELETE /projects/:id/repository/files" do @@ -100,6 +138,10 @@ describe API::API, api: true do end it "should delete existing file in project repo" do + Gitlab::Satellite::DeleteFileAction.any_instance.stub( + commit!: true, + ) + delete api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(200) expect(json_response['file_path']).to eq(file_path) @@ -111,7 +153,9 @@ describe API::API, api: true do end it "should return a 400 if satellite fails to create file" do - allow_any_instance_of(Repository).to receive(:remove_file).and_return(false) + Gitlab::Satellite::DeleteFileAction.any_instance.stub( + commit!: false, + ) delete api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) -- cgit v1.2.1 From b9b9fe70b8fd2deac5dd795e3f220ea90171a75f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 13:29:35 +0200 Subject: Revert "Fix editing files via API" This reverts commit 7bde6ae540bab5c93a83bfe26102674adba0eab5. --- lib/api/files.rb | 50 ++++++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/lib/api/files.rb b/lib/api/files.rb index c7b30cf2f07..e0ea6d7dd1d 100644 --- a/lib/api/files.rb +++ b/lib/api/files.rb @@ -3,26 +3,6 @@ module API class Files < Grape::API before { authenticate! } - helpers do - def commit_params(attrs) - { - file_path: attrs[:file_path], - current_branch: attrs[:branch_name], - target_branch: attrs[:branch_name], - commit_message: attrs[:commit_message], - file_content: attrs[:content], - file_content_encoding: attrs[:encoding] - } - end - - def commit_response(attrs) - { - file_path: attrs[:file_path], - branch_name: attrs[:branch_name], - } - end - end - resource :projects do # Get file from repository # File content is Base64 encoded @@ -93,11 +73,17 @@ module API required_attributes! [:file_path, :branch_name, :content, :commit_message] attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding] - result = ::Files::CreateService.new(user_project, current_user, commit_params(attrs)).execute + branch_name = attrs.delete(:branch_name) + file_path = attrs.delete(:file_path) + result = ::Files::CreateService.new(user_project, current_user, attrs, branch_name, file_path).execute if result[:status] == :success status(201) - commit_response(attrs) + + { + file_path: file_path, + branch_name: branch_name + } else render_api_error!(result[:message], 400) end @@ -119,11 +105,17 @@ module API required_attributes! [:file_path, :branch_name, :content, :commit_message] attrs = attributes_for_keys [:file_path, :branch_name, :content, :commit_message, :encoding] - result = ::Files::UpdateService.new(user_project, current_user, commit_params(attrs)).execute + branch_name = attrs.delete(:branch_name) + file_path = attrs.delete(:file_path) + result = ::Files::UpdateService.new(user_project, current_user, attrs, branch_name, file_path).execute if result[:status] == :success status(200) - commit_response(attrs) + + { + file_path: file_path, + branch_name: branch_name + } else http_status = result[:http_status] || 400 render_api_error!(result[:message], http_status) @@ -146,11 +138,17 @@ module API required_attributes! [:file_path, :branch_name, :commit_message] attrs = attributes_for_keys [:file_path, :branch_name, :commit_message] - result = ::Files::DeleteService.new(user_project, current_user, commit_params(attrs)).execute + branch_name = attrs.delete(:branch_name) + file_path = attrs.delete(:file_path) + result = ::Files::DeleteService.new(user_project, current_user, attrs, branch_name, file_path).execute if result[:status] == :success status(200) - commit_response(attrs) + + { + file_path: file_path, + branch_name: branch_name + } else render_api_error!(result[:message], 400) end -- cgit v1.2.1 From 3a63c00505307a1d1e8196c0eae72a79b2a6885f Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 11 Aug 2015 13:47:37 +0200 Subject: Fix file api tests Signed-off-by: Dmitriy Zaporozhets --- spec/requests/api/files_spec.rb | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 346f1e29d48..1d14a1729c6 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -49,9 +49,7 @@ describe API::API, api: true do end it "should create a new file in project repo" do - Gitlab::Satellite::NewFileAction.any_instance.stub( - commit!: true, - ) + expect_any_instance_of(Gitlab::Satellite::NewFileAction).to receive(:commit!).and_return(true) post api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(201) @@ -64,9 +62,7 @@ describe API::API, api: true do end it "should return a 400 if satellite fails to create file" do - Gitlab::Satellite::NewFileAction.any_instance.stub( - commit!: false, - ) + expect_any_instance_of(Gitlab::Satellite::NewFileAction).to receive(:commit!).and_return(false) post api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) @@ -84,9 +80,7 @@ describe API::API, api: true do end it "should update existing file in project repo" do - Gitlab::Satellite::EditFileAction.any_instance.stub( - commit!: true, - ) + expect_any_instance_of(Gitlab::Satellite::EditFileAction).to receive(:commit!).and_return(true) put api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(200) @@ -99,8 +93,7 @@ describe API::API, api: true do end it 'should return a 400 if the checkout fails' do - Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) - .and_raise(Gitlab::Satellite::CheckoutFailed) + expect_any_instance_of(Gitlab::Satellite::EditFileAction).to receive(:commit!).and_raise(Gitlab::Satellite::CheckoutFailed) put api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) @@ -110,8 +103,7 @@ describe API::API, api: true do end it 'should return a 409 if the file was not modified' do - Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) - .and_raise(Gitlab::Satellite::CommitFailed) + expect_any_instance_of(Gitlab::Satellite::EditFileAction).to receive(:commit!).and_raise(Gitlab::Satellite::CommitFailed) put api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(409) @@ -119,8 +111,7 @@ describe API::API, api: true do end it 'should return a 409 if the push fails' do - Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) - .and_raise(Gitlab::Satellite::PushFailed) + expect_any_instance_of(Gitlab::Satellite::EditFileAction).to receive(:commit!).and_raise(Gitlab::Satellite::PushFailed) put api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(409) @@ -138,10 +129,7 @@ describe API::API, api: true do end it "should delete existing file in project repo" do - Gitlab::Satellite::DeleteFileAction.any_instance.stub( - commit!: true, - ) - + expect_any_instance_of(Gitlab::Satellite::DeleteFileAction).to receive(:commit!).and_return(true) delete api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(200) expect(json_response['file_path']).to eq(file_path) @@ -153,9 +141,7 @@ describe API::API, api: true do end it "should return a 400 if satellite fails to create file" do - Gitlab::Satellite::DeleteFileAction.any_instance.stub( - commit!: false, - ) + expect_any_instance_of(Gitlab::Satellite::DeleteFileAction).to receive(:commit!).and_return(false) delete api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) -- cgit v1.2.1