From ff9832170c80bc788b632221bfe74333c8856a1c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 11:08:33 +0000 Subject: [PATCH 1/9] Allow viewing a PR when its origin (fork or branch) is gone This will ensure a pull-request remains accessible despite its source being gone by relying on the newly introduced reference for each PR. Since that reference is in the main repo and is not going away when the fork or branch gets deleted, it is just as easy to rely on it when the source of the PR disappears. Fixes https://pagure.io/pagure/issue/2492 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index e107d37..0935626 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1322,7 +1322,7 @@ def merge_pull_request( return 'Changes merged!' -def get_diff_info(repo_obj, orig_repo, branch_from, branch_to): +def get_diff_info(repo_obj, orig_repo, branch_from, branch_to, prid=None): ''' Return the info needed to see a diff or make a Pull-Request between the two specified repo. @@ -1332,10 +1332,11 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to): first git repo : arg branch_to: the name of the branch in which we want to merge the changes in the second git repo + :kwarg prid: the identifier of the pull-request to ''' frombranch = repo_obj.lookup_branch(branch_from) - if not frombranch and not repo_obj.is_empty: + if not frombranch and not repo_obj.is_empty and prid is None: raise pagure.exceptions.BranchNotFoundException( 'Branch %s does not exist' % branch_from ) @@ -1351,10 +1352,24 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to): commitid = None if frombranch: commitid = frombranch.get_object().hex + elif prid is not None: + # If there is not branch found but there is a PR open, use the ref + # of that PR in the main repo + try: + ref = orig_repo.lookup_reference("refs/pull/%s/head" % prid) + commitid = ref.target.hex + except KeyError as err: + print err + pass diff_commits = [] diff = None orig_commit = None + + # If the fork is empty but there is a PR open, use the main repo + if repo_obj.is_empty and prid is not None: + repo_obj = orig_repo + if not repo_obj.is_empty and not orig_repo.is_empty: if branch: orig_commit = orig_repo[branch.get_object().hex] @@ -1450,7 +1465,8 @@ def diff_pull_request( diff = None diff_commits = [] diff, diff_commits, _ = get_diff_info( - repo_obj, orig_repo, request.branch_from, request.branch) + repo_obj, orig_repo, request.branch_from, request.branch, + prid=request.id) if request.status and diff_commits: first_commit = repo_obj[diff_commits[-1].oid.hex] diff --git a/pagure/lib/model.py b/pagure/lib/model.py index c6eb116..fe1955e 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1599,7 +1599,7 @@ class PullRequest(BASE): project_id_from = sa.Column( sa.Integer, sa.ForeignKey( - 'projects.id', ondelete='CASCADE', onupdate='CASCADE', + 'projects.id', ondelete='SET NULL', onupdate='CASCADE', ), nullable=True) remote_git = sa.Column( @@ -1666,12 +1666,6 @@ class PullRequest(BASE): default=datetime.datetime.utcnow, onupdate=datetime.datetime.utcnow) - __table_args__ = ( - sa.CheckConstraint( - 'NOT(project_id_from IS NULL AND remote_git IS NULL)', - ), - ) - project = relation( 'Project', foreign_keys=[project_id], remote_side=[Project.id], backref=backref( diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 35cc51f..e5a85f0 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -193,8 +193,11 @@ def request_pull(repo, requestid, username=None, namespace=None): parentpath = pagure.get_repo_path(request.project) else: repo_from = request.project_from - repopath = pagure.get_repo_path(repo_from) - parentpath = _get_parent_repo_path(repo_from) + parentpath = pagure.get_repo_path(request.project) + if repo_from: + repopath = pagure.get_repo_path(repo_from) + else: + repopath = pagure.get_repo_path(request.project) repo_obj = pygit2.Repository(repopath) orig_repo = pygit2.Repository(parentpath) diff --git a/tests/test_pagure_flask_ui_pr_no_sources.py b/tests/test_pagure_flask_ui_pr_no_sources.py new file mode 100644 index 0000000..df9df96 --- /dev/null +++ b/tests/test_pagure_flask_ui_pr_no_sources.py @@ -0,0 +1,263 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import json +import unittest +import shutil +import sys +import tempfile +import time +import os + +import pygit2 +from mock import patch, MagicMock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure +import pagure.lib +import tests +from pagure.lib.repo import PagureRepo + + +class PagureFlaskPrNoSourcestests(tests.Modeltests): + """ 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() + + pagure.APP.config['TESTING'] = True + pagure.SESSION = self.session + pagure.lib.SESSION = self.session + pagure.ui.SESSION = self.session + pagure.ui.app.SESSION = self.session + pagure.ui.filters.SESSION = self.session + pagure.ui.fork.SESSION = self.session + pagure.ui.repo.SESSION = self.session + pagure.ui.issues.SESSION = self.session + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'repos'), bare=True) + + # 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) + + project = pagure.get_authorized_project(self.session, 'test') + fork = pagure.get_authorized_project( + self.session, 'test', user='foo') + + self.set_up_git_repo(repo=project, fork=fork) + + # Ensure things got setup straight + project = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(len(project.requests), 1) + + def set_up_git_repo(self, repo, fork, branch_from='feature'): + """ Set up the git repo and create the corresponding PullRequest + object. + """ + + # Clone the main repo + gitrepo = os.path.join(self.path, 'repos', repo.path) + newpath = tempfile.mkdtemp(prefix='pagure-fork-test') + repopath = os.path.join(newpath, 'test') + clone_repo = pygit2.clone_repository(gitrepo, repopath) + + # Create a file in that git repo + with open(os.path.join(repopath, 'sources'), 'w') as stream: + stream.write('foo\n bar') + clone_repo.index.add('sources') + clone_repo.index.write() + + # Commits the files added + tree = clone_repo.index.write_tree() + author = pygit2.Signature( + 'Alice Author', 'alice@authors.tld') + committer = pygit2.Signature( + 'Cecil Committer', 'cecil@committers.tld') + clone_repo.create_commit( + 'refs/heads/master', # the name of the reference to update + author, + committer, + 'Add sources file for testing', + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + [] + ) + refname = 'refs/heads/master:refs/heads/master' + ori_remote = clone_repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + first_commit = clone_repo.revparse_single('HEAD') + + # Set the second repo + repopath = os.path.join(self.path, 'repos', fork.path) + new_gitrepo = os.path.join(newpath, 'fork_test') + clone_repo = pygit2.clone_repository(repopath, new_gitrepo) + + # Add the main project as remote repo + upstream_path = os.path.join(self.path, 'repos', repo.path) + remote = clone_repo.create_remote('upstream', upstream_path) + remote.fetch() + + # Edit the sources file again + with open(os.path.join(new_gitrepo, 'sources'), 'w') as stream: + stream.write('foo\n bar\nbaz\n boose') + clone_repo.index.add('sources') + clone_repo.index.write() + + # Commits the files added + tree = clone_repo.index.write_tree() + author = pygit2.Signature( + 'Alice Author', 'alice@authors.tld') + committer = pygit2.Signature( + 'Cecil Committer', 'cecil@committers.tld') + clone_repo.create_commit( + 'refs/heads/%s' % branch_from, + author, + committer, + 'A commit on branch %s' % branch_from, + tree, + [first_commit.oid.hex] + ) + refname = 'refs/heads/%s' % (branch_from) + ori_remote = clone_repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + # Create a PR for these changes + project = pagure.get_authorized_project(self.session, 'test') + req = pagure.lib.new_pull_request( + session=self.session, + repo_from=fork, + branch_from=branch_from, + repo_to=project, + branch_to='master', + title='PR from the %s branch' % branch_from, + user='pingou', + requestfolder=None, + ) + self.session.commit() + self.assertEqual(req.id, 1) + self.assertEqual(req.title, 'PR from the %s branch' % branch_from) + + shutil.rmtree(newpath) + + def test_request_pull_reference(self): + """ Test if there is a reference created for a new PR. """ + # Give time to the worker to process the task + time.sleep(1) + + project = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(len(project.requests), 1) + + gitrepo = os.path.join(self.path, 'repos', 'test.git') + repo = pygit2.Repository(gitrepo) + self.assertEqual( + list(repo.listall_references()), + ['refs/heads/master', 'refs/pull/1/head'] + ) + + def test_request_pull_fork_reference(self): + """ Test if there the references created on the fork. """ + + project = pagure.get_authorized_project( + self.session, 'test', user='foo') + self.assertEqual(len(project.requests), 0) + + gitrepo = os.path.join(self.path, 'repos', project.path) + repo = pygit2.Repository(gitrepo) + self.assertEqual( + list(repo.listall_references()), + ['refs/heads/feature'] + ) + + def test_accessing_pr_fork_deleted(self): + """ Test accessing the PR if the fork has been deleted. """ + # Give time to the worker to process the task + time.sleep(1) + + # Delete fork on disk + project = pagure.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() + + # View the pull-request + output2 = self.app.get('/test/pull-request/1') + self.assertEqual(output2.status_code, 200) + + def test_accessing_pr_branch_deleted(self): + """ Test accessing the PR if branch it originates from has been + deleted. """ + # Give time to the worker to process the task + time.sleep(1) + + project = pagure.get_authorized_project( + self.session, 'test', user='foo') + + # Check the branches before + gitrepo = os.path.join(self.path, 'repos', project.path) + repo = pygit2.Repository(gitrepo) + self.assertEqual( + list(repo.listall_references()), + ['refs/heads/feature'] + ) + + # Delete branch of the fork + user = tests.FakeUser(username='foo') + with tests.user_set(pagure.APP, user): + output = self.app.post( + '/fork/foo/test/b/feature/delete', follow_redirects=True) + self.assertEqual(output.status_code, 200) + + # Check the branches after + gitrepo = os.path.join(self.path, 'repos', project.path) + repo = pygit2.Repository(gitrepo) + self.assertEqual( + list(repo.listall_references()), + [] + ) + + # View the pull-request + output2 = self.app.get('/test/pull-request/1') + self.assertEqual(output2.status_code, 200) + + +if __name__ == '__main__': + unittest.main(verbosity=2) From 0f5f4fa7ef6078854fa185e77cd873097b46acc1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 11:11:03 +0000 Subject: [PATCH 2/9] Drop doubled call to tasks.sync_pull_ref.delay() Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 0935626..de15f0e 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1497,12 +1497,6 @@ def diff_pull_request( request.commit_stop = diff_commits[0].oid.hex session.add(request) session.commit() - tasks.sync_pull_ref.delay( - request.project.name, - request.project.namespace, - request.project.user.username if request.project.is_fork else None, - request.id - ) tasks.sync_pull_ref.delay( request.project.name, From 41dbac8a7d5005e9bd46e44987e831164cf9653b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 11:12:14 +0000 Subject: [PATCH 3/9] Couple of fixes in the in-code documentation Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index de15f0e..46afacb 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1330,7 +1330,7 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to, prid=None): :arg orig_repo: The pygit2.Repository object of the second git repo :arg branch_from: the name of the branch having the changes, in the first git repo - : arg branch_to: the name of the branch in which we want to merge the + :arg branch_to: the name of the branch in which we want to merge the changes in the second git repo :kwarg prid: the identifier of the pull-request to @@ -1538,7 +1538,7 @@ def update_pull_ref(request, repo): remote = rc.create(reponame, repopath) try: _log.info( - ' Pushing refs/heads/%s to refs/pull/%s', + ' Pushing refs/heads/%s to refs/pull/%s/head', request.branch_from, request.id) refname = '+refs/heads/%s:refs/pull/%s/head' % ( request.branch_from, request.id) From 3c8b0959ad97900dacc12eaa3cb290e1f4b51311 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 11:12:42 +0000 Subject: [PATCH 4/9] Create the reference directly when the PR is created Instead of waiting for the first view of the PR, just create the reference when creating the PR. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 0014a6d..f9d5b3e 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1565,6 +1565,14 @@ def new_pull_request(session, branch_from, 'pr': request.to_json(public=True, with_comments=False) })) + # Create the ref from the start + tasks.sync_pull_ref.delay( + request.project.name, + request.project.namespace, + request.project.user.username if request.project.is_fork else None, + request.id + ) + return request From ca658dd4332e1bdda023d86a90f952941faa015e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 11:17:55 +0000 Subject: [PATCH 5/9] Fix the check, requests always have a status but it's not always open Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 46afacb..ddd6164 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1468,7 +1468,7 @@ def diff_pull_request( repo_obj, orig_repo, request.branch_from, request.branch, prid=request.id) - if request.status and diff_commits: + if request.status == 'Open' and diff_commits: first_commit = repo_obj[diff_commits[-1].oid.hex] # Check if we can still rely on the merge_status commenttext = None From 0f71015fda17bdc38bdb1e24f9bb0899bf290561 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 13:03:33 +0000 Subject: [PATCH 6/9] Drop un-necessary print statement Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index ddd6164..9337b06 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1358,8 +1358,7 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to, prid=None): try: ref = orig_repo.lookup_reference("refs/pull/%s/head" % prid) commitid = ref.target.hex - except KeyError as err: - print err + except KeyError: pass diff_commits = [] From 8ffbe4b03d862bbcaa7bfddce7a3d13ee19f0341 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 13:03:43 +0000 Subject: [PATCH 7/9] Raise an exception if no commit hash was found Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 9337b06..6eda762 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1361,6 +1361,11 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to, prid=None): except KeyError: pass + if not commitid: + raise pagure.exceptions.PagureException( + 'No branch from which to pull or local PR reference were found' + ) + diff_commits = [] diff = None orig_commit = None From 0fe9c9bc4ab749596d7bbc80e2cc8004be25d62d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 13:58:21 +0000 Subject: [PATCH 8/9] Make the tests more robust using a while loop Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_pr_no_sources.py b/tests/test_pagure_flask_ui_pr_no_sources.py index df9df96..f283191 100644 --- a/tests/test_pagure_flask_ui_pr_no_sources.py +++ b/tests/test_pagure_flask_ui_pr_no_sources.py @@ -81,6 +81,18 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): project = pagure.get_authorized_project(self.session, 'test') self.assertEqual(len(project.requests), 1) + # wait for the worker to process the task + 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') + def set_up_git_repo(self, repo, fork, branch_from='feature'): """ Set up the git repo and create the corresponding PullRequest object. @@ -174,8 +186,6 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): def test_request_pull_reference(self): """ Test if there is a reference created for a new PR. """ - # Give time to the worker to process the task - time.sleep(1) project = pagure.get_authorized_project(self.session, 'test') self.assertEqual(len(project.requests), 1) @@ -203,8 +213,6 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): def test_accessing_pr_fork_deleted(self): """ Test accessing the PR if the fork has been deleted. """ - # Give time to the worker to process the task - time.sleep(1) # Delete fork on disk project = pagure.get_authorized_project( @@ -225,9 +233,6 @@ class PagureFlaskPrNoSourcestests(tests.Modeltests): def test_accessing_pr_branch_deleted(self): """ Test accessing the PR if branch it originates from has been deleted. """ - # Give time to the worker to process the task - time.sleep(1) - project = pagure.get_authorized_project( self.session, 'test', user='foo') From fc8c36dfb8498035e53f6c35d54c26041b277bcf Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 14:43:51 +0000 Subject: [PATCH 9/9] Fix the test suite Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 6eda762..c7e8547 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1336,7 +1336,7 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to, prid=None): ''' frombranch = repo_obj.lookup_branch(branch_from) - if not frombranch and not repo_obj.is_empty and prid is None: + if not frombranch and not repo_obj.is_empty and prid is None: raise pagure.exceptions.BranchNotFoundException( 'Branch %s does not exist' % branch_from ) diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index 66ce171..ffd9cf4 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -723,8 +723,8 @@ class PagureFlaskForktests(tests.Modeltests): self.assertIn( 'Overview - test - Pagure', output.data) self.assertIn( - '\n Fork is empty, there are no ' - 'commits to create a pull request with', output.data) + '\n No branch from which to pull ' + 'or local PR reference were found', output.data) shutil.rmtree(newpath) @@ -1095,8 +1095,8 @@ index 0000000..2a552bb self.assertIn( 'Overview - test - Pagure', output.data) self.assertIn( - '\n Fork is empty, there are no ' - 'commits to create a pull request with', output.data) + '\n No branch from which to pull ' + 'or local PR reference were found', output.data) shutil.rmtree(newpath) @@ -1537,8 +1537,8 @@ index 0000000..2a552bb follow_redirects=True) self.assertEqual(output.status_code, 400) self.assertIn( - '

Fork is empty, there are no commits to create a ' - 'pull request with

', output.data) + '

No branch from which to pull or local PR reference ' + 'were found

', output.data) output = self.app.get('/test/new_issue') csrf_token = self.get_csrf(output=output) @@ -1552,8 +1552,8 @@ index 0000000..2a552bb '/test/diff/master..feature', data=data, follow_redirects=True) self.assertEqual(output.status_code, 400) self.assertIn( - '

Fork is empty, there are no commits to create a ' - 'pull request with

', output.data) + '

No branch from which to pull or local PR reference ' + 'were found

', output.data) shutil.rmtree(newpath) @@ -1587,8 +1587,8 @@ index 0000000..2a552bb '/fork/foo/test/diff/master..master', follow_redirects=True) self.assertEqual(output.status_code, 400) self.assertIn( - '

Fork is empty, there are no commits to create a ' - 'pull request with

', output.data) + '

No branch from which to pull or local PR reference ' + 'were found

', output.data) shutil.rmtree(newpath) diff --git a/tests/test_pagure_lib_notify.py b/tests/test_pagure_lib_notify.py index b8b302d..e917098 100644 --- a/tests/test_pagure_lib_notify.py +++ b/tests/test_pagure_lib_notify.py @@ -342,7 +342,7 @@ class PagureLibNotifytests(tests.Modeltests): 'Email “Subject“', 'foo@bar.com,zöé@foo.net', mail_id='test-pull-request-2edbf96ebe644f4bb31b94605e-1', - in_reply_to='test-pull-request-2edbf96ebe644f4bb31b94605e@pagure', + in_reply_to='test-pull-request-2edbf96ebe644f4bb31b94605e', project_name='namespace/project', user_from='Zöé', ) @@ -353,7 +353,7 @@ Subject: =?utf-8?b?W25hbWVzcGFjZS9wcm9qZWN0XSBFbWFpbCDigJxTdWJqZWN04oCc?= From: =?utf-8?b?WsO2w6k=?= mail-id: test-pull-request-2edbf96ebe644f4bb31b94605e-1@pagure.org Message-Id: -In-Reply-To: +In-Reply-To: X-Auto-Response-Suppress: All X-pagure: https://pagure.org/ X-pagure-project: namespace/project @@ -372,7 +372,7 @@ RW1haWwgY29udGVudA== 'Email “Subject“', u'foo@bar.com,zöé@foo.net', mail_id='test-pull-request-2edbf96ebe644f4bb31b94605e-1', - in_reply_to='test-pull-request-2edbf96ebe644f4bb31b94605e@pagure', + in_reply_to='test-pull-request-2edbf96ebe644f4bb31b94605e', project_name='namespace/project', user_from='Zöé', )