From 5efe838f50f23834220bbd1f502a65fce54c61f3 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 13 2018 09:36:14 +0000 Subject: [PATCH 1/2] Fix seeing the diff on closed PR and deleted forks Basically, we were looking at the fork to generate the diff of the PR, but if the forks has been deleted and later re-created the commits are gone from the forks, so the diff appears empty. If we switch from using the fork to using the target project, since we pulled the object in the target repo when creating the refs refs/pull//head, it will work just fine. This is what this commit does, it moves the logic of the diff from the fork repo to the target repo. Thanks to Slavek Kabrda to pointing this one out and figuring out the fix. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 353b757..9301c83 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -266,7 +266,7 @@ def request_pull(repo, requestid, username=None, namespace=None): if request.status != "Open": commitid = request.commit_stop try: - for commit in repo_obj.walk(commitid, pygit2.GIT_SORT_NONE): + for commit in orig_repo.walk(commitid, pygit2.GIT_SORT_NONE): diff_commits.append(commit) if commit.oid.hex == request.commit_start: break @@ -287,9 +287,9 @@ def request_pull(repo, requestid, username=None, namespace=None): if start == diff_commits[0].oid.hex: diff = diff_commits[0].tree.diff_to_tree(swap=True) else: - diff = repo_obj.diff( - repo_obj.revparse_single(start), - repo_obj.revparse_single(diff_commits[0].oid.hex), + diff = orig_repo.diff( + orig_repo.revparse_single(start), + orig_repo.revparse_single(diff_commits[0].oid.hex), ) else: try: From 3c943f7b0d7ac406ddabb2ed058d6f558e37318b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 13 2018 09:36:14 +0000 Subject: [PATCH 2/2] Try replicating the issue with closed PRs in tests Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/query.py b/pagure/lib/query.py index 8a5c2b2..31e995d 100644 --- a/pagure/lib/query.py +++ b/pagure/lib/query.py @@ -1957,6 +1957,8 @@ def new_pull_request( request.project.user.username if request.project.is_fork else None, request.id, ) + # Update the start and stop commits + pagure.lib.tasks.update_pull_request.delay(request.uid) return request diff --git a/tests/test_pagure_flask_ui_pr_no_sources.py b/tests/test_pagure_flask_ui_pr_no_sources.py index 70a9962..07945ad 100644 --- a/tests/test_pagure_flask_ui_pr_no_sources.py +++ b/tests/test_pagure_flask_ui_pr_no_sources.py @@ -32,7 +32,7 @@ import tests from pagure.lib.repo import PagureRepo -class PagureFlaskPrNoSourcestests(tests.Modeltests): +class BasePrNoSourcestests(tests.Modeltests): """ Tests PR in pagure when the source is gone """ maxDiff = None @@ -41,7 +41,7 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True)) def setUp(self): """ Set up the environnment, ran before every tests. """ - super(PagureFlaskPrNoSourcestests, self).setUp() + super(BasePrNoSourcestests, self).setUp() tests.create_projects(self.session) tests.create_projects_git( @@ -65,6 +65,7 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): project = pagure.lib.query.get_authorized_project(self.session, 'test') fork = pagure.lib.query.get_authorized_project( self.session, 'test', user='foo') + print('fork', fork.fullname) self.set_up_git_repo(repo=project, fork=fork) @@ -76,13 +77,7 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): path = os.path.join( self.path, 'repos', 'test.git', 'refs', 'pull', '1', 'head') - cnt = 0 - while not os.path.exists(path): - time.sleep(0.1) - cnt += 1 - if cnt == 100: - # We're about 10 seconds in, let's bail - raise Exception('Sorry, worker took too long') + self.assertTrue(os.path.exists(path)) def set_up_git_repo(self, repo, fork, branch_from='feature'): """ Set up the git repo and create the corresponding PullRequest @@ -174,6 +169,18 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): shutil.rmtree(newpath) + +class PagureFlaskPrNoSourcestests(BasePrNoSourcestests): + """ Tests PR in pagure when the source is gone """ + + maxDiff = None + + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True)) + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskPrNoSourcestests, self).setUp() + def test_request_pull_reference(self): """ Test if there is a reference created for a new PR. """ @@ -219,6 +226,11 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): # View the pull-request output2 = self.app.get('/test/pull-request/1') self.assertEqual(output2.status_code, 200) + output_text = output2.get_data(as_text=True) + self.assertIn('''Files Changed  + + 1 + ''', output_text) def test_accessing_pr_patch_fork_deleted(self): """ Test accessing the PR's patch if the fork has been deleted. """ @@ -274,6 +286,11 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): # View the pull-request output2 = self.app.get('/test/pull-request/1') self.assertEqual(output2.status_code, 200) + output_text = output2.get_data(as_text=True) + self.assertIn('''Files Changed  + + 1 + ''', output_text) def test_accessing_pr_patch_branch_deleted(self): """ Test accessing the PR's patch if branch it originates from has @@ -307,9 +324,119 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): # View the pull-request output = self.app.get('/test/pull-request/1.patch') self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) self.assertIn( '--- a/sources\n+++ b/sources\n@@ -1,2 +1,4 @@', - output.get_data(as_text=True)) + output_text) + + def test_accessing_pr_patch_fork_deleted_recreated(self): + """ Test accessing the PR's patch if the fork has been deleted + and re-created. """ + + # Delete fork on disk + project = pagure.lib.query.get_authorized_project( + self.session, 'test', user='foo') + repo_path = os.path.join(self.path, 'repos', project.path) + self.assertTrue(os.path.exists(repo_path)) + shutil.rmtree(repo_path) + self.assertFalse(os.path.exists(repo_path)) + + # Delete fork in the DB + self.session.delete(project) + self.session.commit() + + # Re-create foo's fork of pingou's test project + item = pagure.lib.model.Project( + user_id=2, # foo + name='test', + description='test project #1', + hook_token='aaabbb', + is_fork=True, + parent_id=1, + ) + self.session.add(item) + self.session.commit() + # Create the fork's git repo + repo_path = os.path.join(self.path, 'repos', item.path) + pygit2.init_repository(repo_path, bare=True) + + # View the pull-request + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn('''Commits  + + 1 + ''', output_text) + self.assertIn('''Files Changed  + + 1 + ''', output_text) + + +class PagureFlaskClosedPrNoSourcestests(BasePrNoSourcestests): + """ Tests viewing closed PR in pagure when the source is gone """ + + maxDiff = None + + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + @patch('pagure.lib.notify.fedmsg_publish', MagicMock(return_value=True)) + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskClosedPrNoSourcestests, self).setUp() + + # Ensure things got setup straight + project = pagure.lib.query.get_authorized_project(self.session, 'test') + self.assertEqual(len(project.requests), 1) + request = project.requests[0] + self.assertEqual(request.status, "Open") + request.status = "Closed" + self.session.add(request) + self.session.commit() + + def test_accessing_pr_patch_fork_deleted_recreated(self): + """ Test accessing the PR's patch if the fork has been deleted + and re-created. """ + + # Delete fork on disk + project = pagure.lib.query.get_authorized_project( + self.session, 'test', user='foo') + repo_path = os.path.join(self.path, 'repos', project.path) + self.assertTrue(os.path.exists(repo_path)) + shutil.rmtree(repo_path) + self.assertFalse(os.path.exists(repo_path)) + + # Delete fork in the DB + self.session.delete(project) + self.session.commit() + + # Re-create foo's fork of pingou's test project + item = pagure.lib.model.Project( + user_id=2, # foo + name='test', + description='test project #1', + hook_token='aaabbb', + is_fork=True, + parent_id=1, + ) + self.session.add(item) + self.session.commit() + # Create the fork's git repo + fork_path = os.path.join(self.path, 'repos', item.path) + pygit2.clone_repository(repo_path, fork_path, bare=True) + + # View the pull-request + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn('''Commits  + + 1 + ''', output_text) + self.assertIn('''Files Changed  + + 1 + ''', output_text) if __name__ == '__main__':