diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 36ca88da1c7de38baeec1aa04f7227827da2389f..00c01c86ae2bf9d14c05746f4475ac2b0afd011c 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -405,6 +405,7 @@ def project_params_attributes ], project_setting_attributes: %i[ show_default_award_emojis + squash_option ] ] end diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index 7940ec1162bf25534c6acc856e423188a9e214bf..caf39741543e535060ec958bfd52606db390cd24 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -118,7 +118,7 @@ def merge_params(merge_request) auto_merge_strategy: AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS, should_remove_source_branch: true, sha: merge_request.diff_head_sha, - squash: merge_request.squash + squash: merge_request.squash_on_merge? } end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 699f82c7e6be71489a32e01dc94a47ba2b956dac..662986cea4e0bd9c7daeed7377c3034cb3e6ee06 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1142,6 +1142,13 @@ def default_squash_commit_message end end + def squash_on_merge? + return true if target_project.squash_always? + return false if target_project.squash_never? + + squash? + end + def has_ci? return false if has_no_commits? diff --git a/app/models/project.rb b/app/models/project.rb index d464c39a43ece5c27ade1df43e060db3295f13c8..9b1e453216b4be9ff9c813b20f04509a1ef8ac55 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -363,6 +363,7 @@ class Project < ApplicationRecord to: :project_setting, allow_nil: true delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?, prefix: :import, to: :import_state, allow_nil: true + delegate :squash_always?, :squash_never?, :squash_enabled_by_default?, :squash_readonly?, to: :project_setting delegate :no_import?, to: :import_state, allow_nil: true delegate :name, to: :owner, allow_nil: true, prefix: true delegate :members, to: :team, prefix: true diff --git a/app/models/project_setting.rb b/app/models/project_setting.rb index 9022d3e879d0c4c96c5688bfc7982fd2fcb10246..aca7eec3382dbd4a7a2e0d3b6a6cf4385d576279 100644 --- a/app/models/project_setting.rb +++ b/app/models/project_setting.rb @@ -3,7 +3,22 @@ class ProjectSetting < ApplicationRecord belongs_to :project, inverse_of: :project_setting + enum squash_option: { + never: 0, + always: 1, + default_on: 2, + default_off: 3 + }, _prefix: 'squash' + self.primary_key = :project_id + + def squash_enabled_by_default? + %w[always default_on].include?(squash_option) + end + + def squash_readonly? + %w[always never].include?(squash_option) + end end ProjectSetting.prepend_if_ee('EE::ProjectSetting') diff --git a/app/serializers/merge_request_poll_cached_widget_entity.rb b/app/serializers/merge_request_poll_cached_widget_entity.rb index 72f629b350753e075ac045b26aca3148bda05ec6..23266bfed7ab04f10238e8ceec5794dcb4c63558 100644 --- a/app/serializers/merge_request_poll_cached_widget_entity.rb +++ b/app/serializers/merge_request_poll_cached_widget_entity.rb @@ -74,6 +74,18 @@ class MergeRequestPollCachedWidgetEntity < IssuableEntity diffs_project_merge_request_path(merge_request.project, merge_request) end + expose :squash_enabled_by_default do |merge_request| + presenter(merge_request).project.squash_enabled_by_default? + end + + expose :squash_readonly do |merge_request| + presenter(merge_request).project.squash_readonly? + end + + expose :squash_on_merge do |merge_request| + presenter(merge_request).squash_on_merge? + end + private delegate :current_user, to: :request diff --git a/app/serializers/merge_request_poll_widget_entity.rb b/app/serializers/merge_request_poll_widget_entity.rb index aad607f358a81bf82c1d3a044b1b259a2113b20e..b0d311681c02d00c91d302d123eea2aca6edf004 100644 --- a/app/serializers/merge_request_poll_widget_entity.rb +++ b/app/serializers/merge_request_poll_widget_entity.rb @@ -145,6 +145,18 @@ class MergeRequestPollWidgetEntity < Grape::Entity presenter(merge_request).revert_in_fork_path end + expose :squash_enabled_by_default do |merge_request| + presenter(merge_request).project.squash_enabled_by_default? + end + + expose :squash_readonly do |merge_request| + presenter(merge_request).project.squash_readonly? + end + + expose :squash_on_merge do |merge_request| + presenter(merge_request).squash_on_merge? + end + private delegate :current_user, to: :request diff --git a/app/services/merge_requests/ff_merge_service.rb b/app/services/merge_requests/ff_merge_service.rb index 6f1fa607ef9ed00c786d150e7d6570a75507de5f..b3896d61a78913108e40933cf8cbd64190ea6220 100644 --- a/app/services/merge_requests/ff_merge_service.rb +++ b/app/services/merge_requests/ff_merge_service.rb @@ -16,7 +16,7 @@ def commit merge_request.target_branch, merge_request: merge_request) - if merge_request.squash + if merge_request.squash_on_merge? merge_request.update_column(:squash_commit_sha, merge_request.in_progress_merge_commit_sha) end diff --git a/app/services/merge_requests/merge_base_service.rb b/app/services/merge_requests/merge_base_service.rb index 27b5e31faab1be4ea7371bac680c047696e57901..fe09c92aab925a81034b64a94198d92159bac5fa 100644 --- a/app/services/merge_requests/merge_base_service.rb +++ b/app/services/merge_requests/merge_base_service.rb @@ -20,7 +20,7 @@ def hooks_validation_error(_merge_request) def source strong_memoize(:source) do - if merge_request.squash + if merge_request.squash_on_merge? squash_sha! else merge_request.diff_head_sha diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 8d57a76f7d027a761a60815c0300fa318c2aadd7..48c1ef3cefe9ab46b49b6de2927ec475b16c3cda 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -56,6 +56,8 @@ def error_check! 'Only fast-forward merge is allowed for your project. Please update your source branch' elsif !@merge_request.mergeable? 'Merge request is not mergeable' + elsif !@merge_request.squash && project.squash_always? + 'This project requires squashing commits when merge requests are accepted.' end raise_error(error) if error diff --git a/app/services/merge_requests/squash_service.rb b/app/services/merge_requests/squash_service.rb index 4b04d42b48ec1aeffc9f333eb81f7a55bd8cb848..faa2e9215812610eea0a60ecc6751f1913e8aeae 100644 --- a/app/services/merge_requests/squash_service.rb +++ b/app/services/merge_requests/squash_service.rb @@ -11,11 +11,14 @@ def execute return success(squash_sha: merge_request.diff_head_sha) end + return error(s_('MergeRequests|This project does not allow squashing commits when merge requests are accepted.')) if squash_forbidden? + if squash_in_progress? return error(s_('MergeRequests|Squash task canceled: another squash is already in progress.')) end squash! || error(s_('MergeRequests|Failed to squash. Should be done manually.')) + rescue SquashInProgressError error(s_('MergeRequests|An error occurred while checking whether another squash is in progress.')) end @@ -40,6 +43,10 @@ def squash_in_progress? raise SquashInProgressError, e.message end + def squash_forbidden? + target_project.squash_never? + end + def repository target_project.repository end diff --git a/changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml b/changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml new file mode 100644 index 0000000000000000000000000000000000000000..bb7749213615fc479edc250f565b12994421d9a9 --- /dev/null +++ b/changelogs/unreleased/17613-configurable-defaults-for-squash-commits-option.yml @@ -0,0 +1,5 @@ +--- +title: Add squash commits options as a project setting. +merge_request: 33099 +author: +type: added diff --git a/db/migrate/20200526193555_add_squash_option_to_project.rb b/db/migrate/20200526193555_add_squash_option_to_project.rb new file mode 100644 index 0000000000000000000000000000000000000000..6dd4e1c7c8e1fa5dbf14d6b5a27a0239927651fb --- /dev/null +++ b/db/migrate/20200526193555_add_squash_option_to_project.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddSquashOptionToProject < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column :project_settings, :squash_option, :integer, default: 3, limit: 2 + end +end diff --git a/db/structure.sql b/db/structure.sql index 4ffd1e379ac55e6101eec33057d3318d33355e5a..0918541fe4aed3925bda817aeb99c8ec94399f37 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14140,6 +14140,7 @@ CREATE TABLE public.project_settings ( push_rule_id bigint, show_default_award_emojis boolean DEFAULT true, allow_merge_on_skipped_pipeline boolean, + squash_option smallint DEFAULT 3, CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL)) ); @@ -23380,6 +23381,7 @@ COPY "schema_migrations" (version) FROM STDIN; 20200526153844 20200526164946 20200526164947 +20200526193555 20200526231421 20200527092027 20200527094322 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index d9e4037ff16bc36aacd5718af0ae9caf0de0c731..97d8acee29336a74ea08f76b944b239b397fd20f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -14084,6 +14084,9 @@ msgstr "" msgid "MergeRequests|Squash task canceled: another squash is already in progress." msgstr "" +msgid "MergeRequests|This project does not allow squashing commits when merge requests are accepted." +msgstr "" + msgid "MergeRequests|Thread stays resolved" msgstr "" diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 382593fd7cb0468b72bbff28ae799e39fbb4cfdf..4327e0bbb7a885b70ecade0bd1ae478e6154af43 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -442,7 +442,7 @@ def merge_with_sha(params = {}) merge_request.update(squash: false) merge_with_sha(squash: '1') - expect(merge_request.reload.squash).to be_truthy + expect(merge_request.reload.squash_on_merge?).to be_truthy end end @@ -451,7 +451,7 @@ def merge_with_sha(params = {}) merge_request.update(squash: true) merge_with_sha(squash: '0') - expect(merge_request.reload.squash).to be_falsey + expect(merge_request.reload.squash_on_merge?).to be_falsey end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 803178d590f5290ff2b7bc06a8c349c05ccd8720..d71cd843a4749b3b40ca11300359c6304b8c18c0 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1930,7 +1930,7 @@ it "updates the MR's squash attribute" do expect do put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: { squash: true } - end.to change { merge_request.reload.squash } + end.to change { merge_request.reload.squash_on_merge? } expect(response).to have_gitlab_http_status(:ok) end diff --git a/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb b/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb index 1357f7cf624276808957194cc1b65110d4519b68..51564de6041e8df68a6e6e0c75b5ea22733996a5 100644 --- a/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_cached_widget_entity_spec.rb @@ -4,6 +4,7 @@ RSpec.describe MergeRequestPollCachedWidgetEntity do include ProjectForksHelper + using RSpec::Parameterized::TableSyntax let(:project) { create :project, :repository } let(:resource) { create(:merge_request, source_project: project, target_project: project) } @@ -181,6 +182,27 @@ def find_matching_commit(short_id) end end + describe 'squash defaults for projects' do + where(:squash_option, :value, :default, :readonly) do + 'always' | true | true | true + 'never' | false | false | true + 'default_on' | false | true | false + 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: squash_option) + end + + it 'the key reflects the correct value' do + expect(subject[:squash_on_merge]).to eq(value) + expect(subject[:squash_enabled_by_default]).to eq(default) + expect(subject[:squash_readonly]).to eq(readonly) + end + end + end + describe 'attributes for squash commit message' do context 'when merge request is mergeable' do before do diff --git a/spec/serializers/merge_request_poll_widget_entity_spec.rb b/spec/serializers/merge_request_poll_widget_entity_spec.rb index 2b6159e883d46280a1839419b7c10e94b47c3380..f0493699209b01d8307b76defdb714aaae4c8c36 100644 --- a/spec/serializers/merge_request_poll_widget_entity_spec.rb +++ b/spec/serializers/merge_request_poll_widget_entity_spec.rb @@ -4,6 +4,7 @@ RSpec.describe MergeRequestPollWidgetEntity do include ProjectForksHelper + using RSpec::Parameterized::TableSyntax let(:project) { create :project, :repository } let(:resource) { create(:merge_request, source_project: project, target_project: project) } @@ -171,6 +172,27 @@ end end + describe 'squash defaults for projects' do + where(:squash_option, :value, :default, :readonly) do + 'always' | true | true | true + 'never' | false | false | true + 'default_on' | false | true | false + 'default_off' | false | false | false + end + + with_them do + before do + project.project_setting.update!(squash_option: squash_option) + end + + it 'the key reflects the correct value' do + expect(subject[:squash_on_merge]).to eq(value) + expect(subject[:squash_enabled_by_default]).to eq(default) + expect(subject[:squash_readonly]).to eq(readonly) + end + end + end + context 'when head pipeline is finished' do before do create(:ci_pipeline, :success, project: project, diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb index 9673a65344d2e1c2805fa1149499ed1b913eaabe..98fa601208957f2289ad5667986d7b06d4b80ac5 100644 --- a/spec/services/auto_merge/base_service_spec.rb +++ b/spec/services/auto_merge/base_service_spec.rb @@ -51,7 +51,7 @@ expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18') expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) - expect(merge_request.squash).to eq(false) + expect(merge_request.squash_on_merge?).to eq(false) expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md') end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 94ecd1830c2d6d1a5b330ebbd0724951edb9056f..11e341994f76dbb02c981f46d5dca57b2b86145b 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -360,6 +360,25 @@ expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end + context 'when squashing is required' do + before do + merge_request.update!(source_branch: 'master', target_branch: 'feature') + merge_request.target_project.project_setting.squash_always! + end + + it 'raises an error if squashing is not done' do + error_message = 'requires squashing commits' + + service.execute(merge_request) + + expect(merge_request).to be_open + + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.merge_error).to include(error_message) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + end + context 'when squashing' do before do merge_request.update!(source_branch: 'master', target_branch: 'feature') diff --git a/spec/services/merge_requests/squash_service_spec.rb b/spec/services/merge_requests/squash_service_spec.rb index b51b7d9fbc6c430e0524380cb88b35c767e17b38..1ec1dc0f6eb434fc2360f94e02680f9fbc2be323 100644 --- a/spec/services/merge_requests/squash_service_spec.rb +++ b/spec/services/merge_requests/squash_service_spec.rb @@ -131,6 +131,42 @@ include_examples 'the squash succeeds' end + context 'when squashing is disabled by default on the project' do + # Squashing is disabled by default, but it should still allow you + # to squash-and-merge if selected through the UI + let(:merge_request) { merge_request_with_only_new_files } + + before do + merge_request.project.project_setting.squash_default_off! + end + + include_examples 'the squash succeeds' + end + + context 'when squashing is forbidden on the project' do + let(:merge_request) { merge_request_with_only_new_files } + + before do + merge_request.project.project_setting.squash_never! + end + + it 'raises a squash error' do + expect(service.execute).to match( + status: :error, + message: a_string_including('does not allow squashing commits when merge requests are accepted')) + end + end + + context 'when squashing is enabled by default on the project' do + let(:merge_request) { merge_request_with_only_new_files } + + before do + merge_request.project.project_setting.squash_always! + end + + include_examples 'the squash succeeds' + end + context 'when squashing with files too large to display' do let(:merge_request) { merge_request_with_large_files }