From ff9832170c80bc788b632221bfe74333c8856a1c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Sep 12 2017 11:08:33 +0000 Subject: 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)