From fbb6847985cd61b0d96c8695734e7c1d8ea53b47 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 29 2017 11:24:10 +0000 Subject: [PATCH 1/2] Fix generating the diff of two branches Until now the algorithm was looking at the main branch and the feature branch and for each step in both branches checking if there was a shared commit. Otherwise it would keep the list of the commits seen. The first time it finds a shared commit, it stops. Then we clear from the list of diff commits all the ones found in master. Except that we may end up in a situation where we have in the diff list a number of commits that are already in master but after the first one shared. Say: a - b - c - d - e - f - g \ h The diff list would be [h, b, a ...] since it will take some iteration to reach the b commit on both branches. With this commit, instead of removing all the commits that are in master it iterates through the list of diff commits and only keep the commit that are present before the first shared commit. --- diff --git a/pagure/internal/__init__.py b/pagure/internal/__init__.py index 0a3bca2..d76b7b8 100644 --- a/pagure/internal/__init__.py +++ b/pagure/internal/__init__.py @@ -296,7 +296,7 @@ def get_pull_request_ready_branch(): if compare_branch: try: com = main_walker.next() - main_commits.add(com.hex) + main_commits.add(com.oid.hex) except StopIteration: com = None try: @@ -309,22 +309,17 @@ def get_pull_request_ready_branch(): break if branch_commit: - tmp = set(branch_commits + [branch_commit.hex]) - else: - tmp = set(branch_commits) - if main_commits.intersection(tmp): - break - - if branch_commit: branch_commits.append(branch_commit.hex) + if main_commits.intersection(set(branch_commits)): + break # If master is ahead of branch, we need to remove the commits - # that are already in master - branch_commits = [ - com - for com in branch_commits - if com not in main_commits - ] + # that are after the first one found in master + i = 0 + for i in range(len(branch_commits)): + if branch_commits[i] in main_commits: + break + branch_commits = branch_commits[:i] if branch_commits: branches[branchname] = branch_commits diff --git a/pagure/lib/git.py b/pagure/lib/git.py index e177772..8a9b118 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1377,7 +1377,7 @@ def diff_pull_request( while 1: try: com = main_walker.next() - main_commits.add(com.hex) + main_commits.add(com.oid.hex) except StopIteration: com = None @@ -1392,19 +1392,17 @@ def diff_pull_request( if branch_commit: branch_commits.add(branch_commit.oid.hex) + diff_commits.append(branch_commit) if main_commits.intersection(branch_commits): break - if branch_commit: - diff_commits.append(branch_commit) - # If master is ahead of branch, we need to remove the commits - # that are already in master - diff_commits = [ - com - for com in diff_commits - if com.oid.hex not in main_commits - ] + # that are after the first one found in master + i = 0 + for i in range(len(diff_commits)): + if diff_commits[i].oid.hex in main_commits: + break + diff_commits = diff_commits[:i] if request.status and diff_commits: first_commit = repo_obj[diff_commits[-1].oid.hex] diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 8338e93..75d4818 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -96,7 +96,7 @@ def _get_pr_info(repo_obj, orig_repo, branch_from, branch_to): while 1: try: com = main_walker.next() - main_commits.add(com.hex) + main_commits.add(com.oid.hex) except StopIteration: com = None @@ -111,19 +111,17 @@ def _get_pr_info(repo_obj, orig_repo, branch_from, branch_to): if branch_commit: branch_commits.add(branch_commit.oid.hex) + diff_commits.append(branch_commit) if main_commits.intersection(branch_commits): break - if branch_commit: - diff_commits.append(branch_commit) - # If master is ahead of branch, we need to remove the commits - # that are already in master - diff_commits = [ - com - for com in diff_commits - if com.oid.hex not in main_commits - ] + # that are after the first one found in master + i = 0 + for i in range(len(diff_commits)): + if diff_commits[i].oid.hex in main_commits: + break + diff_commits = diff_commits[:i] if diff_commits: first_commit = repo_obj[diff_commits[-1].oid.hex] From 839f14c080bc5e0aa488b7e52c47b2448852d7af Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 29 2017 13:04:38 +0000 Subject: [PATCH 2/2] Add unit-test checking the behavior of the diff algorithm --- diff --git a/tests/test_pagure_flask_ui_fork_pr.py b/tests/test_pagure_flask_ui_fork_pr.py new file mode 100644 index 0000000..cc30493 --- /dev/null +++ b/tests/test_pagure_flask_ui_fork_pr.py @@ -0,0 +1,188 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2017 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources # noqa + +import json # noqa +import unittest # noqa +import shutil # noqa +import sys # noqa +import tempfile # noqa +import os # noqa + +import pygit2 # noqa +from mock import patch # noqa + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib # noqa +import tests # noqa +from pagure.lib.repo import PagureRepo # noqa + + +class PagureFlaskForkPrtests(tests.Modeltests): + """ Tests for flask fork controller of pagure regarding diffing PRs """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskForkPrtests, self).setUp() + + pagure.APP.config['GIT_FOLDER'] = os.path.join(self.path, 'repos') + pagure.APP.config['TICKETS_FOLDER'] = os.path.join( + self.path, 'tickets') + pagure.APP.config['DOCS_FOLDER'] = os.path.join( + self.path, 'docs') + pagure.APP.config['REQUESTS_FOLDER'] = os.path.join( + self.path, 'requests') + + # Create two git repos, one has 6 commits, the other 4 of which only + # 1 isn't present in the first repo + gitrepo = os.path.join(self.path, 'repos', 'test.git') + pygit2.init_repository(gitrepo, bare=True) + + gitrepo2 = os.path.join( + self.path, 'repos', 'forks', 'pingou', 'test.git') + pygit2.init_repository(gitrepo2, bare=True) + + newpath = tempfile.mkdtemp(prefix='pagure-fork-test') + repopath = os.path.join(newpath, 'test') + clone_repo = pygit2.clone_repository(gitrepo, repopath) + + # Do 3 commits to the main repo + for i in range(3): + with open(os.path.join(repopath, 'sources'), 'w') as stream: + stream.write('foo%s\n bar%s\n' % (i, i)) + clone_repo.index.add('sources') + clone_repo.index.write() + + parents = [] + try: + last_commit = clone_repo.revparse_single('HEAD') + parents = [last_commit.oid.hex] + except KeyError: + pass + + # 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, + 'Editing the file sources for testing #%s' % i, + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + parents + ) + + # Push to the main repo + refname = 'refs/heads/master:refs/heads/master' + ori_remote = clone_repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + # Push to the fork repo + remote = clone_repo.create_remote('pingou_fork', gitrepo2) + PagureRepo.push(remote, refname) + + # Do another 3 commits to the main repo + for i in range(3, 6): + with open(os.path.join(repopath, 'sources'), 'w') as stream: + stream.write('foo%s\n bar%s\n' % (i, i)) + clone_repo.index.add('sources') + clone_repo.index.write() + + last_commit = clone_repo.revparse_single('HEAD') + + # 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, + 'Editing the file sources for testing #%s' % i, + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + [last_commit.oid.hex] + ) + # Push to the main repo + refname = 'refs/heads/master:refs/heads/master' + ori_remote = clone_repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + # Add one commit to the fork repo + repopath = os.path.join(newpath, 'pingou_test') + clone_repo = pygit2.clone_repository(gitrepo2, repopath) + + with open(os.path.join(repopath, 'sources'), 'w') as stream: + stream.write('foo\n bar\n') + clone_repo.index.add('sources') + clone_repo.index.write() + + last_commit = clone_repo.revparse_single('HEAD') + + # 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/feature_foo', # the name of the reference to update + author, + committer, + 'New edition on side branch of the file sources for testing', + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + [last_commit.oid.hex] + ) + + # Push to the fork repo + ori_remote = clone_repo.remotes[0] + refname = 'refs/heads/feature_foo:refs/heads/feature_foo' + PagureRepo.push(ori_remote, refname) + + def test_get_pr_info(self): + """ Test pagure.ui.fork._get_pr_info """ + + gitrepo = os.path.join(self.path, 'repos', 'test.git') + gitrepo2 = os.path.join( + self.path, 'repos', 'forks', 'pingou', 'test.git') + + diff, diff_commits, orig_commit = pagure.ui.fork._get_pr_info( + repo_obj=PagureRepo(gitrepo2), + orig_repo=PagureRepo(gitrepo), + branch_from='feature_foo', + branch_to='master' + ) + self.assertEqual(len(diff_commits), 1) + self.assertEqual( + diff_commits[0].message, + 'New edition on side branch of the file sources for testing' + ) + self.assertEqual( + orig_commit.message, + 'Editing the file sources for testing #5' + ) + + +if __name__ == '__main__': + unittest.main(verbosity=2)