From 045feb08c99fdb96928f14847bdebd5798a4011f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 10 2016 14:44:43 +0000 Subject: [PATCH 1/7] Use the same structure as for the others First check if we do the merge, if not then return the status --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 995764f..bca19a9 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1064,23 +1064,25 @@ def merge_pull_request( session.commit() return 'CONFLICTS' - if not domerge: + if domerge: + head = new_repo.lookup_reference('HEAD').get_object() + user_obj = pagure.lib.__get_user(session, username) + author = pygit2.Signature(user_obj.fullname, user_obj.default_email) + new_repo.create_commit( + 'refs/heads/%s' % request.branch, + author, + author, + 'Merge #%s `%s`' % (request.id, request.title), + tree, + [head.hex, repo_commit.oid.hex]) + PagureRepo.push(ori_remote, refname) + + else: request.merge_status = 'MERGE' session.commit() shutil.rmtree(newpath) return 'MERGE' - head = new_repo.lookup_reference('HEAD').get_object() - user_obj = pagure.lib.__get_user(session, username) - author = pygit2.Signature(user_obj.fullname, user_obj.default_email) - new_repo.create_commit( - 'refs/heads/%s' % request.branch, - author, - author, - 'Merge #%s `%s`' % (request.id, request.title), - tree, - [head.hex, repo_commit.oid.hex]) - PagureRepo.push(ori_remote, refname) # Update status pagure.lib.close_pull_request( From c2dccd37b30dbc4db442ff0baa595093d46fb745 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 10 2016 14:44:43 +0000 Subject: [PATCH 2/7] Add a new settings to project ``always_merge`` --- diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 5b74b49..ab6a98a 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -346,6 +346,7 @@ class Project(BASE): 'Minimum_score_to_merge_pull-request': -1, 'Web-hooks': None, 'Enforce_signed-off_commits_in_pull-request': False, + 'always_merge': False, } if self._settings: From ccc85ec3caff5d97aaf8ba4511c6258f88e4ca90 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 10 2016 14:44:43 +0000 Subject: [PATCH 3/7] If a project's settings always_merge is True, always do a merge commit --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index bca19a9..a2d22cb 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1038,11 +1038,26 @@ def merge_pull_request( mergecode & pygit2.GIT_MERGE_ANALYSIS_FASTFORWARD)): if domerge: - if merge is not None: - # This is depending on the pygit2 version - branch_ref.target = merge.fastforward_oid - elif merge is None and mergecode is not None: - branch_ref.set_target(repo_commit.oid.hex) + if not request.project.settings.get('always_merge', False): + if merge is not None: + # This is depending on the pygit2 version + branch_ref.target = merge.fastforward_oid + elif merge is None and mergecode is not None: + branch_ref.set_target(repo_commit.oid.hex) + else: + tree = new_repo.index.write_tree() + head = new_repo.lookup_reference('HEAD').get_object() + user_obj = pagure.lib.__get_user(session, username) + author = pygit2.Signature( + user_obj.fullname, + user_obj.default_email) + new_repo.create_commit( + 'refs/heads/%s' % request.branch, + author, + author, + 'Merge #%s `%s`' % (request.id, request.title), + tree, + [head.hex, repo_commit.oid.hex]) PagureRepo.push(ori_remote, refname) else: From edfd4c8e11c8ce85256b18ed8ced1240ffcd17fe Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 10 2016 14:44:43 +0000 Subject: [PATCH 4/7] Add unit-tests for the always_merge setting With this change we ensure that w/o always_merge we get the expected commit messages but with always_merge we get the merge commit even on a PR that could have been fast-forwarded --- diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index 8ac8484..8ea6642 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -29,6 +29,25 @@ import tests from pagure.lib.repo import PagureRepo +def _get_commits(output): + ''' Returns the commits message in the output. All commits must have + been made by `Alice Author` or `PY C` to be found. + ''' + commits = [] + save = False + cnt = 0 + for row in output.split('\n'): + if row.strip() in ['Alice Author', 'PY C']: + save = True + if save: + cnt += 1 + if cnt == 7: + commits.append(row.strip()) + save = False + cnt = 0 + return commits + + class PagureFlaskForktests(tests.Modeltests): """ Tests for flask fork controller of pagure """ @@ -375,6 +394,19 @@ class PagureFlaskForktests(tests.Modeltests): self.assertIn( '\n Changes merged!', output.data) + self.assertIn( + 'A commit on branch feature', output.data) + self.assertNotIn( + 'Merge #1 `PR from the feature branch`', output.data) + # Ensure we have the new commit + commits = _get_commits(output.data) + self.assertEqual( + commits, + [ + 'A commit on branch feature', + 'Add sources file for testing' + ] + ) @patch('pagure.lib.notify.send_email') def test_merge_request_pull_merge(self, send_email): @@ -1674,6 +1706,92 @@ index 0000000..2a552bb follow_redirects=True) self.assertEqual(output.status_code, 404) + @patch('pagure.lib.notify.send_email') + def test_merge_request_pull_FF_w_merge_commit(self, send_email): + """ Test the merge_request_pull endpoint with a FF PR but with a + merge commit. + """ + send_email.return_value = True + + self.test_request_pull() + + user = tests.FakeUser() + with tests.user_set(pagure.APP, user): + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + + csrf_token = output.data.split( + 'name="csrf_token" type="hidden" value="')[1].split('">')[0] + + # No CSRF + output = self.app.post( + '/test/pull-request/1/merge', data={}, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#1: PR from the feature branch - test\n - ' + 'Pagure', output.data) + self.assertIn( + '

PR#1\n' + ' PR from the feature branch\n

', output.data) + self.assertIn( + 'title="View file as of 2a552b">View', output.data) + + # Wrong project + data = { + 'csrf_token': csrf_token, + } + output = self.app.post( + '/foobar/pull-request/100/merge', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 404) + + # Wrong project + data = { + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/pull-request/1/merge', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 403) + + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + + # Wrong request id + data = { + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/pull-request/100/merge', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 404) + + # Project requiring a merge commit + repo = pagure.lib.get_project(self.session, 'test') + settings = repo.settings + settings['always_merge'] = True + repo.settings = settings + self.session.add(repo) + self.session.commit() + + # Merge + output = self.app.post( + '/test/pull-request/1/merge', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Overview - test - Pagure', output.data) + self.assertIn( + '\n Changes merged!', + output.data) + self.assertIn( + 'Merge #1 `PR from the feature branch`', output.data) + self.assertIn( + 'A commit on branch feature', output.data) + # Ensure we have the merge commit + commits = _get_commits(output.data) + self.assertEqual(commits, [ + 'Merge #1 `PR from the feature branch`', + 'Add sources file for testing', + 'A commit on branch feature', + ]) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureFlaskForktests) From d4b9dbe587329a511e108a67df471f1a40c749ec Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 10 2016 14:44:43 +0000 Subject: [PATCH 5/7] Fix unit-tests for the new setting: always_merge --- diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index e15e80a..bba3973 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -887,6 +887,7 @@ class PagureLibtests(tests.Modeltests): 'Minimum_score_to_merge_pull-request': -1, 'Web-hooks': None, 'Enforce_signed-off_commits_in_pull-request': False, + 'always_merge': False, }, user='pingou', ) From 7c68f870b3393603ada74a8d40fb5847cf7c332a Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 10 2016 14:53:45 +0000 Subject: [PATCH 6/7] Let's use some non-ascii character in names and email when doing a commit --- diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index 8ea6642..f36b735 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -37,7 +37,7 @@ def _get_commits(output): save = False cnt = 0 for row in output.split('\n'): - if row.strip() in ['Alice Author', 'PY C']: + if row.strip() in ['Alice Author', 'Alice Äuthòr', 'PY C']: save = True if save: cnt += 1 @@ -126,9 +126,9 @@ class PagureFlaskForktests(tests.Modeltests): # Commits the files added tree = clone_repo.index.write_tree() author = pygit2.Signature( - 'Alice Author', 'alice@authors.tld') + 'Alice Äuthòr', 'alice@äuthòrs.tld') committer = pygit2.Signature( - 'Cecil Committer', 'cecil@committers.tld') + 'Cecil Cõmmîttër', 'cecil@cõmmîttërs.tld') clone_repo.create_commit( 'refs/heads/master', author, From 1709b956a626ee47e1620dd2db3e9170776ddb22 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 10 2016 14:54:24 +0000 Subject: [PATCH 7/7] Adjust unit-tests for the always_merge setting change --- diff --git a/tests/test_pagure_lib_git.py b/tests/test_pagure_lib_git.py index 2615f6e..38c026b 100644 --- a/tests/test_pagure_lib_git.py +++ b/tests/test_pagure_lib_git.py @@ -691,7 +691,7 @@ new file mode 100644 index 0000000..60f7480 --- /dev/null +++ b/456 -@@ -0,0 +1,78 @@ +@@ -0,0 +1,80 @@ +{ + "assignee": null, + "branch": "master", @@ -714,6 +714,7 @@ index 0000000..60f7480 + "Minimum_score_to_merge_pull-request": -1, + "Only_assignee_can_merge_pull-request": false, + "Web-hooks": null, ++ "always_merge": false, + "issue_tracker": true, + "project_documentation": true, + "pull_requests": true @@ -741,6 +742,7 @@ index 0000000..60f7480 + "Minimum_score_to_merge_pull-request": -1, + "Only_assignee_can_merge_pull-request": false, + "Web-hooks": null, ++ "always_merge": false, + "issue_tracker": true, + "project_documentation": true, + "pull_requests": true