pull-requests: use close action with proper --close-commit solution.

- Mercurial settings are no long as LABS
- Unified some code between GIT and HG on rebase/close/delete feature branches

docs/api/methods/repo-methods.rst

@@ -534,6 +534,8 b' get_repo_settings'
                "phases_publish": "True",
                "rhodecode_hg_use_rebase_for_merging": true,
                "rhodecode_hg_close_branch_before_merging": false,
+               "rhodecode_git_use_rebase_for_merging": true,
+               "rhodecode_git_close_branch_before_merging": false,
                "rhodecode_pr_merge_enabled": true,
                "rhodecode_use_outdated_comments": true
            }

rhodecode/lib/vcs/backends/hg/repository.py

@@ -558,6 +558,7 b' class MercurialRepository(BaseRepository'
         """
         Update the working copty to the specified revision.
         """
+        log.debug('Doing checkout to commit: `%s` for %s', revision, self)
         self._remote.update(revision, clean=clean)
 
     def _identify(self):
@@ -654,8 +655,8 b' class MercurialRepository(BaseRepository'
         Returns the commit id of the close and a boolean indicating if the
         commit needs to be pushed.
         """
-        self._update(target_ref.commit_id)
-        message = close_message or "Closing branch"
+        self._update(source_ref.commit_id)
+        message = close_message or "Closing branch: `{}`".format(source_ref.name)
         try:
             self._remote.commit(
                 message=safe_str(message),
@@ -732,16 +733,26 b' class MercurialRepository(BaseRepository'
                 False, False, None, MergeFailureReason.MISSING_SOURCE_REF)
 
         merge_ref = None
+        merge_commit_id = None
+        close_commit_id = None
         merge_failure_reason = MergeFailureReason.NONE
 
-        if close_branch and not use_rebase:
+        # enforce that close branch should be used only in case we source from
+        # an actual Branch
+        close_branch = close_branch and source_ref.type == 'branch'
+
+        # don't allow to close branch if source and target are the same
+        close_branch = close_branch and source_ref.name != target_ref.name
+
+        needs_push_on_close = False
+        if close_branch and not use_rebase and not dry_run:
             try:
-                close_commit_id, needs_push = shadow_repo._local_close(
+                close_commit_id, needs_push_on_close = shadow_repo._local_close(
                     target_ref, merger_name, merger_email, source_ref)
-                target_ref.commit_id = close_commit_id
                 merge_possible = True
             except RepositoryError:
-                log.exception('Failure when doing close branch on hg shadow repo')
+                log.exception(
+                    'Failure when doing close branch on hg shadow repo')
                 merge_possible = False
                 merge_failure_reason = MergeFailureReason.MERGE_FAILED
         else:
@@ -754,9 +765,13 b' class MercurialRepository(BaseRepository'
                     source_ref, use_rebase=use_rebase)
                 merge_possible = True
 
-                # Set a bookmark pointing to the merge commit. This bookmark may be
-                # used to easily identify the last successful merge commit in the
-                # shadow repository.
+                # read the state of the close action, if it
+                # maybe required a push
+                needs_push = needs_push or needs_push_on_close
+
+                # Set a bookmark pointing to the merge commit. This bookmark
+                # may be used to easily identify the last successful merge
+                # commit in the shadow repository.
                 shadow_repo.bookmark('pr-merge', revision=merge_commit_id)
                 merge_ref = Reference('book', 'pr-merge', merge_commit_id)
             except SubrepoMergeError:
@@ -776,7 +791,6 b' class MercurialRepository(BaseRepository'
                 if target_ref.type == 'book':
                     shadow_repo.bookmark(
                         target_ref.name, revision=merge_commit_id)
-
                 try:
                     shadow_repo_with_hooks = self._get_shadow_instance(
                         shadow_repository_path,
@@ -788,6 +802,12 b' class MercurialRepository(BaseRepository'
                     shadow_repo_with_hooks._local_push(
                         merge_commit_id, self.path, push_branches=True,
                         enable_hooks=True)
+
+                    # maybe we also need to push the close_commit_id
+                    if close_commit_id:
+                        shadow_repo_with_hooks._local_push(
+                            close_commit_id, self.path, push_branches=True,
+                            enable_hooks=True)
                     merge_succeeded = True
                 except RepositoryError:
                     log.exception(

rhodecode/model/forms.py

@@ -394,6 +394,8 b' class _BaseVcsSettingsForm(formencode.Sc'
 
     # git
     vcs_git_lfs_enabled = v.StringBoolean(if_missing=False)
+    rhodecode_git_use_rebase_for_merging = v.StringBoolean(if_missing=False)
+    rhodecode_git_close_branch_before_merging = v.StringBoolean(if_missing=False)
 
     # svn
     vcs_svn_proxy_http_requests_enabled = v.StringBoolean(if_missing=False)

rhodecode/model/pull_request.py

@@ -1424,12 +1424,26 b' class PullRequestModel(BaseModel):'
             pull_request, 'rhodecode_pr_merge_enabled')
 
     def _use_rebase_for_merging(self, pull_request):
+        repo_type = pull_request.target_repo.repo_type
+        if repo_type == 'hg':
         return self._get_general_setting(
             pull_request, 'rhodecode_hg_use_rebase_for_merging')
+        elif repo_type == 'git':
+            return self._get_general_setting(
+                pull_request, 'rhodecode_git_use_rebase_for_merging')
+
+        return False
 
     def _close_branch_before_merging(self, pull_request):
+        repo_type = pull_request.target_repo.repo_type
+        if repo_type == 'hg':
         return self._get_general_setting(
             pull_request, 'rhodecode_hg_close_branch_before_merging')
+        elif repo_type == 'git':
+            return self._get_general_setting(
+                pull_request, 'rhodecode_git_close_branch_before_merging')
+
+        return False
 
     def _get_general_setting(self, pull_request, settings_key, default=False):
         settings_model = VcsSettingsModel(repo=pull_request.target_repo)

rhodecode/model/settings.py

@@ -409,7 +409,9 b' class VcsSettingsModel(object):'
         'use_outdated_comments',
         'pr_merge_enabled',
         'hg_use_rebase_for_merging',
-        'hg_close_branch_before_merging')
+        'hg_close_branch_before_merging',
+        'git_use_rebase_for_merging',
+        'git_close_branch_before_merging')
 
     HOOKS_SETTINGS = (
         ('hooks', 'changegroup.repo_size'),

rhodecode/templates/base/vcs_settings.mako

@@ -145,34 +145,6 b''
 
             </div>
         </div>
-        ## LABS for HG
-        % if c.labs_active:
-            <div class="panel panel-danger">
-                <div class="panel-heading">
-                    <h3 class="panel-title">${_('Mercurial Labs Settings')} (${_('These features are considered experimental and may not work as expected.')})</h3>
-                </div>
-                <div class="panel-body">
-
-                    <div class="checkbox">
-                        ${h.checkbox('rhodecode_hg_use_rebase_for_merging' + suffix, 'True', **kwargs)}
-                        <label for="rhodecode_hg_use_rebase_for_merging${suffix}">${_('Use rebase as merge strategy')}</label>
-                    </div>
-                    <div class="label">
-                        <span class="help-block">${_('Use rebase instead of creating a merge commit when merging via web interface.')}</span>
-                    </div>
-
-                    <div class="checkbox">
-                        ${h.checkbox('rhodecode_hg_close_branch_before_merging' + suffix, 'True', **kwargs)}
-                        <label for="rhodecode_hg_close_branch_before_merging{suffix}">${_('Close branch before merging it')}</label>
-                    </div>
-                    <div class="label">
-                        <span class="help-block">${_('Close branch before merging it into destination branch. No effect when rebase strategy is use.')}</span>
-                    </div>
-
-                </div>
-            </div>
-        % endif
-
     % endif
 
     % if display_globals or repo_type in ['git']:
@@ -340,4 +312,59 b''
         </div>
     % endif
 
+    % if display_globals or repo_type in ['hg',]:
+        <div class="panel panel-default">
+            <div class="panel-heading" id="vcs-pull-requests-options">
+                <h3 class="panel-title">${_('Mercurial Pull Request Settings')}<a class="permalink" href="#vcs-hg-pull-requests-options"> ¶</a></h3>
+            </div>
+            <div class="panel-body">
+                ## Specific HG settings
+                <div class="checkbox">
+                    ${h.checkbox('rhodecode_hg_use_rebase_for_merging' + suffix, 'True', **kwargs)}
+                    <label for="rhodecode_hg_use_rebase_for_merging${suffix}">${_('Use rebase as merge strategy')}</label>
+                </div>
+                <div class="label">
+                    <span class="help-block">${_('Use rebase instead of creating a merge commit when merging via web interface.')}</span>
+                </div>
+
+                <div class="checkbox">
+                    ${h.checkbox('rhodecode_hg_close_branch_before_merging' + suffix, 'True', **kwargs)}
+                    <label for="rhodecode_hg_close_branch_before_merging{suffix}">${_('Close branch before merging it')}</label>
+                </div>
+                <div class="label">
+                    <span class="help-block">${_('Close branch before merging it into destination branch. No effect when rebase strategy is use.')}</span>
+                </div>
+
+
+            </div>
+        </div>
+    % endif
+
+## DISABLED FOR GIT FOR NOW as the rebase/close is not supported yet
+##     % if display_globals or repo_type in ['git']:
+##         <div class="panel panel-default">
+##             <div class="panel-heading" id="vcs-pull-requests-options">
+##                 <h3 class="panel-title">${_('Git Pull Request Settings')}<a class="permalink" href="#vcs-git-pull-requests-options"> ¶</a></h3>
+##             </div>
+##             <div class="panel-body">
+##                 <div class="checkbox">
+##                     ${h.checkbox('rhodecode_git_use_rebase_for_merging' + suffix, 'True', **kwargs)}
+##                     <label for="rhodecode_git_use_rebase_for_merging${suffix}">${_('Use rebase as merge strategy')}</label>
+##                 </div>
+##                 <div class="label">
+##                     <span class="help-block">${_('Use rebase instead of creating a merge commit when merging via web interface.')}</span>
+##                 </div>
+##
+##                 <div class="checkbox">
+##                     ${h.checkbox('rhodecode_git_close_branch_before_merging' + suffix, 'True', **kwargs)}
+##                     <label for="rhodecode_git_close_branch_before_merging{suffix}">${_('Delete branch after merging it')}</label>
+##                 </div>
+##                 <div class="label">
+##                     <span class="help-block">${_('Delete branch after merging it into destination branch. No effect when rebase strategy is use.')}</span>
+##                 </div>
+##             </div>
+##         </div>
+##     % endif
+
+
 </%def>

rhodecode/tests/functional/test_admin_settings.py

@@ -324,18 +324,6 b' class TestAdminSettingsVcs(object):'
         setting = SettingsModel().get_setting_by_name(setting_key)
         assert setting.app_settings_value is new_value
 
-    def test_has_a_section_for_labs_settings_if_enabled(self, app):
-        with mock.patch.dict(
-                rhodecode.CONFIG, {'labs_settings_active': 'true'}):
-            response = self.app.get(url('admin_settings_vcs'))
-        response.mustcontain('Labs Settings')
-
-    def test_has_not_a_section_for_labs_settings_if_disables(self, app):
-        with mock.patch.dict(
-                rhodecode.CONFIG, {'labs_settings_active': 'false'}):
-            response = self.app.get(url('admin_settings_vcs'))
-        response.mustcontain(no='Labs Settings')
-
     @pytest.mark.parametrize('new_value', [True, False])
     def test_allows_to_change_hg_rebase_merge_strategy(
             self, app, form_defaults, csrf_token, new_value):

rhodecode/tests/models/settings/test_vcs_settings.py

@@ -41,6 +41,9 b' GENERAL_FORM_DATA = {'
     'rhodecode_pr_merge_enabled': True,
     'rhodecode_use_outdated_comments': True,
     'rhodecode_hg_use_rebase_for_merging': True,
+    'rhodecode_hg_close_branch_before_merging': True,
+    'rhodecode_git_use_rebase_for_merging': True,
+    'rhodecode_git_close_branch_before_merging': True,
 }
 
 

rhodecode/tests/models/test_pullrequest.py

@@ -164,7 +164,7 b' class TestPullRequestModel(object):'
             pull_request.target_ref_parts,
             pull_request.source_repo.scm_instance(),
             pull_request.source_ref_parts, self.workspace_id, dry_run=True,
-            use_rebase=False)
+            use_rebase=False, close_branch=False)
 
         assert pull_request._last_merge_source_rev == self.source_commit
         assert pull_request._last_merge_target_rev == self.target_commit
@@ -193,7 +193,7 b' class TestPullRequestModel(object):'
             pull_request.target_ref_parts,
             pull_request.source_repo.scm_instance(),
             pull_request.source_ref_parts, self.workspace_id, dry_run=True,
-            use_rebase=False)
+            use_rebase=False, close_branch=False)
 
         assert pull_request._last_merge_source_rev == self.source_commit
         assert pull_request._last_merge_target_rev == self.target_commit
@@ -225,7 +225,7 b' class TestPullRequestModel(object):'
             pull_request.target_ref_parts,
             pull_request.source_repo.scm_instance(),
             pull_request.source_ref_parts, self.workspace_id, dry_run=True,
-            use_rebase=False)
+            use_rebase=False, close_branch=False)
 
         assert pull_request._last_merge_source_rev is None
         assert pull_request._last_merge_target_rev is None
@@ -299,7 +299,7 b' class TestPullRequestModel(object):'
             pull_request.source_repo.scm_instance(),
             pull_request.source_ref_parts, self.workspace_id,
             user_name=user.username, user_email=user.email, message=message,
-            use_rebase=False
+            use_rebase=False, close_branch=False
         )
         self.invalidation_mock.assert_called_once_with(
             pull_request.target_repo.repo_name)
@@ -338,7 +338,7 b' class TestPullRequestModel(object):'
             pull_request.source_repo.scm_instance(),
             pull_request.source_ref_parts, self.workspace_id,
             user_name=user.username, user_email=user.email, message=message,
-            use_rebase=False
+            use_rebase=False, close_branch=False
         )
 
         pull_request = PullRequest.get(pull_request.pull_request_id)