From 8bced834a0619b2c9be06958645070c229076f5b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:21 +0000 Subject: [PATCH 1/15] When doing temp clone allow specifying a path directly This is required to be able to use this class with remote PRs Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 58b0c95..fcbba20 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -873,7 +873,7 @@ class TemporaryClone(object): repopath = None repo = None - def __init__(self, project, repotype, action): + def __init__(self, project, repotype, action, path=None): """ Initializes a TempoaryClone instance. Args: @@ -889,13 +889,18 @@ class TemporaryClone(object): self._project = project self._repotype = repotype self._action = action + self._path = path def __enter__(self): """ Enter the context manager, creating the clone. """ self.repopath = tempfile.mkdtemp(prefix="pagure-%s-" % self._action) if not self._project.is_on_repospanner: # This is the simple case. Just do a local clone - self._origpath = self._project.repopath(self._repotype) + # use either the specified path or the use the path of the specified + # project + self._origpath = self._path or self._project.repopath( + self._repotype + ) if self._origpath is None: # No repository of this type # 'main' is already caught and returns an error in repopath() @@ -1026,7 +1031,7 @@ class TemporaryClone(object): try: _log.debug( "Running a git push of %s to %s" - % (pushref, self._project.fullname) + % (pushref, self._path or self._project.fullname) ) env = os.environ.copy() env["GL_USER"] = username From da3640fc376bdf08f70cedbd05cbfbc9abafd194 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:21 +0000 Subject: [PATCH 2/15] Support force push for project not on repospanner Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index fcbba20..87d2c8b 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -967,7 +967,7 @@ class TemporaryClone(object): """ Exit the context manager, removing the temorary clone. """ shutil.rmtree(self.repopath) - def push(self, username, sbranch, tbranch=None, **extra): + def push(self, username, sbranch, tbranch=None, force=False, **extra): """ Push the repo back to its origin. Args: @@ -1026,6 +1026,8 @@ class TemporaryClone(object): } else: command = ["git", "push", "origin"] + if force: + command.append("--force") environ = {} try: From 9b2a9685928acb9373ea3e62df7e64de937f547d Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:21 +0000 Subject: [PATCH 3/15] Add a method in pagure.lib.git to rebase pull-requests Basically, we clone the project_from (or remote git), add a new remote pointing to the targeted project, pull --rebase from the targeted project and branch and push --force to the original project_from branch_from. Nothing too strange in this Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 87d2c8b..47ff257 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1752,6 +1752,124 @@ def merge_pull_request(session, request, username, domerge=True): return "Changes merged!" +def rebase_pull_request(request, username): + """ Rebase the specified pull-request. + + Args: + session (sqlalchemy): the session to connect to the database with + request (pagure.lib.model.PullRequest): the database object + corresponding to the pull-request to rebase + username (string): the name of the user asking for the pull-request + to be rebased + + Returns: (string or None): Pull-request rebased + Raises: pagure.exceptions.PagureException + + """ + _log.info("%s asked to rebased the pull-request: %s", username, request) + + if request.remote: + # Get the fork + repopath = pagure.utils.get_remote_repo_path( + request.remote_git, request.branch_from + ) + elif request.project_from: + # Get the fork + repopath = pagure.utils.get_repo_path(request.project_from) + else: + return + + if not request.project or not os.path.exists( + pagure.utils.get_repo_path(request.project) + ): + raise pagure.exceptions.PagureException( + "Could not find the targeted git repository for %s" + % request.project.fullname + ) + + with TemporaryClone( + project=request.project, + repotype="main", + action="rebase_pr", + path=repopath, + ) as tempclone: + new_repo = tempclone.repo + new_repo.checkout("refs/heads/%s" % request.branch_from) + + # Add the upstream repo as remote + upstream = "%s_%s" % (request.user.user, request.uid) + upstream_path = pagure.utils.get_repo_path(request.project) + _log.info( + " Adding remote: %s pointing to: %s", upstream, upstream_path + ) + remote = new_repo.create_remote(upstream, upstream_path) + + # Fetch the commits + remote.fetch() + + def _run_command(command): + try: + out = subprocess.check_output( + command, cwd=tempclone.repopath, stderr=subprocess.STDOUT + ) + _log.debug("Output: %s" % out) + except subprocess.CalledProcessError as err: + _log.debug( + "Rebase FAILED: {cmd} returned code {code} with the " + "following output: {output}".format( + cmd=err.cmd, code=err.returncode, output=err.output + ) + ) + raise pagure.exceptions.PagureException( + "Did not manage to rebase this pull-request" + ) + + # Configure git for that user + command = ["git", "config", "user.name", username] + _run_command(command) + command = ["git", "config", "user.email", "%s@pagure" % username] + _run_command(command) + + # Do the rebase + command = ["git", "pull", "--rebase", upstream, request.branch] + _run_command(command) + + # Retrieve the reference of the branch we're working on + try: + branch_ref = get_branch_ref(new_repo, request.branch_from) + except pagure.exceptions.PagureException: + branch_ref = None + if not branch_ref: + _log.debug(" Target branch could not be found") + raise pagure.exceptions.BranchNotFoundException( + "Branch %s could not be found in the repo %s" + % (request.branch, request.project.fullname) + ) + + # Push the changes + _log.info("Pushing %s to %s", branch_ref.name, request.branch_from) + try: + tempclone.push( + username, + branch_ref.name, + request.branch_from, + pull_request=request, + force=True, + ) + except subprocess.CalledProcessError as err: + _log.debug( + "Rebase FAILED: {cmd} returned code {code} with the " + "following output: {output}".format( + cmd=err.cmd, code=err.returncode, output=err.output + ) + ) + raise pagure.exceptions.PagureException( + "Did not manage to rebase this pull-request" + ) + + return "Pull-request rebased" + + 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. From b9aec24938e1b07ee29d65cd10432a23f71bee01 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:21 +0000 Subject: [PATCH 4/15] Add a dedicated task for rebasing pull-requests Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index feb6b6e..48c51c3 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -703,6 +703,32 @@ def refresh_pr_cache(self, session, name, namespace, user): @conn.task(queue=pagure_config.get("FAST_CELERY_QUEUE", None), bind=True) @pagure_task +def rebase_pull_request( + self, session, name, namespace, user, requestid, user_rebaser +): + """ Rebase a pull-request. + """ + project = pagure.lib.query._get_project( + session, namespace=namespace, name=name, user=user + ) + + with project.lock("WORKER"): + request = pagure.lib.query.search_pull_requests( + session, project_id=project.id, requestid=requestid + ) + _log.debug( + "Rebasing pull-request: %s/#%s", + request.project.fullname, + request.id, + ) + pagure.lib.git.rebase_pull_request(request, user_rebaser) + + # Schedule refresh of all opened PRs + pagure.lib.query.reset_status_pull_request(session, request.project) + + +@conn.task(queue=pagure_config.get("FAST_CELERY_QUEUE", None), bind=True) +@pagure_task def merge_pull_request( self, session, From d9e8b6f3107ec72cc0e0884a285e17e1b7687177 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:21 +0000 Subject: [PATCH 5/15] Introduce a new status: NEEDSREBASE This is returned when the PR doesn't conflict, isn't fast-forwardable (so needs a merge commit) but the project is configured to only allow fast-forward merge. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/utils.py b/pagure/utils.py index 9e2b1dd..a523cda 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -624,8 +624,8 @@ def get_merge_options(request, merge_status): "message": "The pull-request can be merged with a merge commit", }, "MERGE-non-ff-bad": { - "code": "CONFLICTS", - "short_code": "Conflicts", + "code": "NEEDSREBASE", + "short_code": "Needs rebase", "message": "The pull-request must be rebased before merging", }, } diff --git a/tests/test_pagure_flask_internal.py b/tests/test_pagure_flask_internal.py index c9d19d0..29e9224 100644 --- a/tests/test_pagure_flask_internal.py +++ b/tests/test_pagure_flask_internal.py @@ -1132,9 +1132,9 @@ class PagureFlaskInternaltests(tests.Modeltests): output = self.app.post('/pv/pull-request/merge', data=data) self.assertEqual(output.status_code, 200) exp = { - "code": "CONFLICTS", + "code": "NEEDSREBASE", "message": "The pull-request must be rebased before merging", - "short_code": "Conflicts" + "short_code": "Needs rebase" } js_data = json.loads(output.get_data(as_text=True)) @@ -1148,9 +1148,9 @@ class PagureFlaskInternaltests(tests.Modeltests): output = self.app.post('/pv/pull-request/merge', data=data) self.assertEqual(output.status_code, 200) exp = { - "code": "CONFLICTS", + "code": "NEEDSREBASE", "message": "The pull-request must be rebased before merging", - "short_code": "Conflicts" + "short_code": "Needs rebase" } js_data = json.loads(output.get_data(as_text=True)) From 5798ffc5ce1ee17ff408e5bb7fd77e7cb92d9aa4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 6/15] Add a new API endpoint allowing to rebase a given pull-request Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 99cc56d..6a65a53 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -604,6 +604,7 @@ def api(): fork.api_pull_request_by_uid_view ) api_pull_request_merge_doc = load_doc(fork.api_pull_request_merge) + api_pull_request_rebase_doc = load_doc(fork.api_pull_request_rebase) api_pull_request_close_doc = load_doc(fork.api_pull_request_close) api_pull_request_add_comment_doc = load_doc( fork.api_pull_request_add_comment @@ -667,6 +668,7 @@ def api(): api_pull_request_diffstats_doc, api_pull_request_by_uid_view_doc, api_pull_request_merge_doc, + api_pull_request_rebase_doc, api_pull_request_close_doc, api_pull_request_add_comment_doc, api_pull_request_add_flag_doc, diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 6c39dc1..d4466e8 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -505,6 +505,102 @@ def api_pull_request_merge(repo, requestid, username=None, namespace=None): return jsonout +@API.route("//pull-request//rebase", methods=["POST"]) +@API.route( + "///pull-request//rebase", methods=["POST"] +) +@API.route( + "/fork///pull-request//rebase", + methods=["POST"], +) +@API.route( + "/fork////pull-request//rebase", + methods=["POST"], +) +@api_login_required(acls=["pull_request_rebase"]) +@api_method +def api_pull_request_rebase(repo, requestid, username=None, namespace=None): + """ + Rebase a pull-request + -------------------- + Instruct Pagure to rebase a pull request. + + This is an asynchronous call. + + :: + + POST /api/0//pull-request//rebase + POST /api/0///pull-request//rebase + + :: + + POST /api/0/fork///pull-request//rebase + POST /api/0/fork////pull-request//rebase + + Sample response + ^^^^^^^^^^^^^^^ + + :: + + wait=False: + { + "message": "Rebasing queued", + "taskid": "123-abcd" + } + + wait=True: + { + "message": "Pull-request rebased" + } + + """ # noqa + output = {} + + repo = get_authorized_api_project( + flask.g.session, repo, user=username, namespace=namespace + ) + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) + + if not repo.settings.get("pull_requests", True): + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.EPULLREQUESTSDISABLED + ) + + if ( + api_authenticated() and flask.g.token and repo != flask.g.token.project + ) or not authenticated(): + raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) + + request = pagure.lib.query.search_pull_requests( + flask.g.session, project_id=repo.id, requestid=requestid + ) + + if not request: + raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + + if not is_repo_committer(repo): + raise pagure.exceptions.APIError(403, error_code=APIERROR.ENOPRCLOSE) + + task = pagure.lib.tasks.rebase_pull_request.delay( + repo.name, namespace, username, requestid, flask.g.fas_user.username + ) + output = {"message": "Rebasing queued", "taskid": task.id} + + if get_request_data().get("wait", True): + try: + task.get() + output = {"message": "Pull-request rebased"} + except pagure.exceptions.PagureException as err: + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.ENOCODE, error=str(err) + ) + + jsonout = flask.jsonify(output) + return jsonout + + @API.route("//pull-request//close", methods=["POST"]) @API.route( "///pull-request//close", methods=["POST"] diff --git a/pagure/default_config.py b/pagure/default_config.py index 960e509..fde4590 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -345,6 +345,7 @@ ACLS = { "Subscribe the user with this token to a pull-request" ), "update_watch_status": "Update the watch status on a project", + "pull_request_rebase": "Rebase a pull-request", } # List of ACLs which a regular user is allowed to associate to an API token diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 29bf980..0603233 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -5511,6 +5511,7 @@ foo bar 'pull_request_create', 'pull_request_flag', 'pull_request_merge', + 'pull_request_rebase', 'pull_request_subscribe', 'update_watch_status', ] From 2955bbc31b6b475862189338e9b6a14890140a91 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 7/15] Show a rebase button in the UI when needed The rebase button shows in two situations: - the PR can be merged via a merge commit (merge status: MERGE) - the project enforces fast-forward merge and needs to be rebased (merge status: NEEDSREBASE) A lot of the churn after that is to compensate for the use of toggleClass, since it both adds and removes, we need to explicitly remove all the class it could possibly have added and add the ones that were there before on all the different elements touched. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/repo_pull_request.html b/pagure/templates/repo_pull_request.html index f7007e8..38c1b0d 100644 --- a/pagure/templates/repo_pull_request.html +++ b/pagure/templates/repo_pull_request.html @@ -195,6 +195,9 @@ {% endif %} + {% else %} {% endif %} @@ -821,42 +824,51 @@ function showTab(){ function show_merge_status(){ function process_response(res) { $('#spinner').hide(); + $('#merge_dropdown_btn').removeClass("disabled"); + $('#merge_dropdown_btn span.fa').removeClass("fa-spin"); if (res.code == 'FFORWARD'){ $('#merge_dropdown_btn').toggleClass("btn-outline-secondary btn-success"); - $('#merge_dropdown_btn').removeClass("disabled"); $('#merge_dropdown_btn span.fa').toggleClass("fa-circle-o-notch fa-check"); - $('#merge_dropdown_btn span.fa').removeClass("fa-spin"); $('#merge_btn').addClass("btn-success"); $('#merge-alert .alert').addClass("alert-success"); - $('#merge-alert-message').append(res.message); + $('#merge-alert-message').text(res.message); + $('#merge-alert #rebase_btn').hide(); + $('#merge-alert div.small').show(); + $('#merge_btn').show(); } else if (res.code == 'MERGE') { $('#merge_dropdown_btn').toggleClass("btn-outline-secondary btn-warning"); - $('#merge_dropdown_btn').removeClass("disabled"); $('#merge_dropdown_btn span.fa').toggleClass("fa-circle-o-notch fa-check"); - $('#merge_dropdown_btn span.fa').removeClass("fa-spin"); $('#merge_btn').addClass("btn-warning"); $('#merge-alert .alert').addClass("alert-warning"); - $('#merge-alert-message').append(res.message); + $('#merge-alert-message').text(res.message); + $('#merge-alert div.small').show(); + $('#merge_btn').show(); + } + else if (res.code == 'NEEDSREBASE') { + $('#merge_dropdown_btn').toggleClass("btn-outline-secondary btn-warning"); + $('#merge_dropdown_btn span.fa').toggleClass("fa-circle-o-notch fa-times"); + $('#merge_btn').hide(); + $('#merge-alert .alert').addClass("alert-warning"); + $('#merge-alert-message').text(res.message); + $('#merge-alert div.small').hide(); } else if (res.code == 'CONFLICTS') { $('#merge_dropdown_btn').toggleClass("btn-outline-secondary btn-danger"); - $('#merge_dropdown_btn').removeClass("disabled"); $('#merge_dropdown_btn span.fa').toggleClass("fa-circle-o-notch fa-times"); - $('#merge_dropdown_btn span.fa').removeClass("fa-spin"); $('#merge_btn').hide(); $('#merge-alert .alert').addClass("alert-danger"); - $('#merge-alert-message').append(res.message); + $('#merge-alert-message').text(res.message); $('#merge-alert div.small').hide(); + $('#merge-alert #rebase_btn').hide(); } else if (res.code == 'NO_CHANGE') { $('#merge_btn').hide(); - $('#merge_dropdown_btn').removeClass("disabled"); $('#merge_dropdown_btn span.fa').toggleClass("fa-circle-o-notch fa-times"); - $('#merge_dropdown_btn span.fa').removeClass("fa-spin"); $('#merge-alert .alert').addClass("alert-secondary"); - $('#merge-alert-message').append(res.message); + $('#merge-alert-message').text(res.message); $('#merge-alert div.small').hide(); + $('#merge-alert #rebase_btn').hide(); } }; $('#spinner').show(); @@ -887,6 +899,46 @@ function show_merge_status(){ $(document).ready(function() { + $('#rebase_btn').click(function(){ + $('#merge_dropdown_btn span.fa').removeClass( + "fa-circle-o-notch fa-times fa-check").addClass( + "fa-circle-o-notch fa-fw"); + $('#merge_btn').removeClass("btn-success btn-warning btn-danger"); + $('#merge-alert .alert').removeClass("alert-success alert-warning alert-danger"); + $('#merge_dropdown_btn').addClass("disabled"); + $('#merge_dropdown_btn').removeClass( + "btn-outline-secondary btn-danger btn-warning btn-success").addClass( + "btn btn-outline-secondary btn-sm disabled dropdown-toggle"); + $('#merge_dropdown_btn span.fa').addClass("fa-spin"); + $.ajax({ + url: '{{ url_for('api_ns.api_pull_request_rebase', + repo=repo.name, + username=username, + namespace=repo.namespace, + requestid=requestid) + }}' , + type: 'POST', + data: { + csrf_token: "{{ mergeform.csrf_token.current_token }}", + }, + dataType: 'json', + success: function(res) { + show_merge_status() + }, + error: function(res) { + $('#merge_dropdown_btn').removeClass("disabled"); + $('#merge_dropdown_btn span.fa').removeClass("fa-spin"); + $('#merge_dropdown_btn').toggleClass("btn-outline-secondary btn-danger"); + $('#merge_dropdown_btn span.fa').toggleClass("fa-circle-o-notch fa-times"); + $('#merge_btn').hide(); + $('#merge-alert #rebase_btn').hide(); + $('#merge-alert .alert').addClass("alert-danger"); + $('#merge-alert-message').text('Failed to rebase this PR'); + $('#merge-alert div.small').hide(); + } + }); + }); + $( ".commit_msg_txt" ).hide(); $( ".commit_msg_btn" ).click(function() { var msgid = $( this ).attr('data-id'); From 5b6471bef2276cc4caa722ce631ba11e4a499be1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 8/15] Adjust the update_pull_request task so it refreshes all the information Before it was just calling a diff on the PR which led to the refresh of the commit_start and commit_stop but now we're going the entire way meaning we're also refreshing the cached merge_status. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 48c51c3..b26f284 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -878,26 +878,12 @@ def update_pull_request(self, session, pr_uid): request.project.fullname, request.id, ) - if request.remote: - repopath = pagure.utils.get_remote_repo_path( - request.remote_git, request.branch_from - ) - parentpath = pagure.utils.get_repo_path(request.project) - else: - repo_from = request.project_from - parentpath = pagure.utils.get_repo_path(request.project) - repopath = parentpath - if repo_from: - repopath = pagure.utils.get_repo_path(repo_from) - _log.debug( - " working on the repo in: %s and %s", repopath, parentpath - ) - - repo_obj = pygit2.Repository(repopath) - orig_repo = pygit2.Repository(parentpath) - pagure.lib.git.diff_pull_request( - session, request, repo_obj, orig_repo, with_diff=False + merge_status = pagure.lib.git.merge_pull_request( + session=session, + request=request, + username=None, + domerge=False, ) From 9213b3079d72b2d89f183223cbca92bf41d5e15b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 9/15] Small wording improvement in the logs This just to help distinguish when PRs have been merged vs just checked for their merge-ability. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 47ff257..31b5824 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1563,7 +1563,7 @@ def merge_pull_request(session, request, username, domerge=True): return "Changes merged!" else: - _log.info(" PR merged using fast-forward, reporting it") + _log.info(" PR can be merged using fast-forward, reporting it") request.merge_status = "FFORWARD" session.commit() return "FFORWARD" @@ -1674,7 +1674,7 @@ def merge_pull_request(session, request, username, domerge=True): pull_request=request, ) else: - _log.info(" PR merged using fast-forward, reporting it") + _log.info(" PR can be merged using fast-forward, reporting it") request.merge_status = "FFORWARD" session.commit() return "FFORWARD" From ec5e382754d964a230d48757503b907ed10c113e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 10/15] Allow specifying a branch when adding content to git It makes it easier to add content to a different branch than master for the tests. Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/__init__.py b/tests/__init__.py index fe136c3..007a756 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -925,9 +925,11 @@ def add_commit_git_repo(folder, ncommits=10, filename='sources', shutil.rmtree(newfolder) -def add_content_to_git(folder, filename='sources', content='foo'): +def add_content_to_git( + folder, branch='master', filename='sources', content='foo'): """ Create some more commits for the specified git repo. """ - repo, newfolder, parents = _clone_and_top_commits(folder, 'master') + repo, newfolder, branch_ref_obj = _clone_and_top_commits( + folder, branch, branch_ref=True) # Create a file in that git repo with open(os.path.join(newfolder, filename), 'a', encoding="utf-8") as stream: @@ -935,14 +937,27 @@ def add_content_to_git(folder, filename='sources', content='foo'): repo.index.add(filename) repo.index.write() + parents = [] + commit = None + try: + if branch_ref_obj: + commit = repo[branch_ref_obj.get_object().hex] + else: + commit = repo.revparse_single('HEAD') + except (KeyError, AttributeError): + pass + if commit: + parents = [commit.oid.hex] + # Commits the files added tree = repo.index.write_tree() author = pygit2.Signature( 'Alice Author', 'alice@authors.tld') committer = pygit2.Signature( 'Cecil Committer', 'cecil@committers.tld') + branch_ref = "refs/heads/%s" % branch repo.create_commit( - 'refs/heads/master', # the name of the reference to update + branch_ref, # the name of the reference to update author, committer, 'Add content to file %s' % (filename), @@ -954,10 +969,7 @@ def add_content_to_git(folder, filename='sources', content='foo'): # Push to origin ori_remote = repo.remotes[0] - master_ref = repo.lookup_reference('HEAD').resolve() - refname = '%s:%s' % (master_ref.name, master_ref.name) - - PagureRepo.push(ori_remote, refname) + PagureRepo.push(ori_remote, '%s:%s' % (branch_ref, branch_ref)) shutil.rmtree(newfolder) From cf4c77a9fd26f5ecc40ea5bfd661b385468f20e0 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 11/15] Add unit-tests for the PR rebase functionality Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_rebase.py b/tests/test_pagure_flask_rebase.py new file mode 100644 index 0000000..4591b23 --- /dev/null +++ b/tests/test_pagure_flask_rebase.py @@ -0,0 +1,295 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2018 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +from __future__ import unicode_literals + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import datetime +import unittest +import shutil +import sys +import os + +import json +from mock import patch, MagicMock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib.query +import pagure.lib.tasks +import tests + + +class PagureRebasetests(tests.Modeltests): + """ Tests rebasing pull-request in pagure """ + + maxDiff = None + + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureRebasetests, self).setUp() + + pagure.config.config['REQUESTS_FOLDER'] = None + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'repos'), bare=True) + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + tests.add_content_to_git( + os.path.join(self.path, 'repos', 'test.git'), + branch='test', content="foobar") + tests.add_readme_git_repo( + os.path.join(self.path, 'repos', 'test.git')) + + # Create a PR for these changes + project = pagure.lib.query.get_authorized_project( + self.session, 'test') + req = pagure.lib.query.new_pull_request( + session=self.session, + repo_from=project, + branch_from='test', + repo_to=project, + branch_to='master', + title='PR from the test branch', + user='pingou', + ) + self.session.commit() + self.assertEqual(req.id, 1) + self.assertEqual(req.title, 'PR from the test branch') + + self.project = pagure.lib.query.get_authorized_project( + self.session, 'test') + self.assertEqual(len(project.requests), 1) + self.request = self.project.requests[0] + + def test_merge_status_merge(self): + """ Test that the PR can be merged with a merge commit. """ + + user = tests.FakeUser(username='pingou') + with tests.user_set(self.app.application, user): + data = {'requestid': self.request.uid, 'csrf_token': self.get_csrf()} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'MERGE', + u'message': u'The pull-request can be merged with a ' + u'merge commit', + u'short_code': u'With merge' + } + ) + + def test_merge_status_needsrebase(self): + """ Test that the PR is marked as needing a rebase if the project + disables non-fast-forward merges. """ + self.project = pagure.lib.query.get_authorized_project( + self.session, 'test') + settings = self.project.settings + settings['disable_non_fast-forward_merges'] = True + self.project.settings = settings + self.session.add(self.project) + self.session.commit() + + user = tests.FakeUser(username='pingou') + with tests.user_set(self.app.application, user): + data = {'requestid': self.request.uid, 'csrf_token': self.get_csrf()} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'NEEDSREBASE', + u'message': u'The pull-request must be rebased before ' + u'merging', + u'short_code': u'Needs rebase' + } + ) + + def test_rebase_task(self): + """ Test the rebase PR task and its outcome. """ + pagure.lib.tasks.rebase_pull_request( + 'test', namespace=None, user=None, requestid=self.request.id, + user_rebaser='pingou') + + user = tests.FakeUser(username='pingou') + with tests.user_set(self.app.application, user): + data = {'requestid': self.request.uid, 'csrf_token': self.get_csrf()} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'FFORWARD', + u'message': u'The pull-request can be merged and ' + u'fast-forwarded', + u'short_code': u'Ok' + } + ) + + def test_rebase_api_ui_logged_in(self): + """ Test the rebase PR API endpoint when logged in from the UI and + its outcome. """ + + user = tests.FakeUser(username='pingou') + with tests.user_set(self.app.application, user): + output = self.app.post('/api/0/test/pull-request/1/rebase') + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + {u'message': u'Pull-request rebased'} + ) + + data = {'requestid': self.request.uid, 'csrf_token': self.get_csrf()} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'FFORWARD', + u'message': u'The pull-request can be merged and ' + u'fast-forwarded', + u'short_code': u'Ok' + } + ) + + def test_rebase_api_api_logged_in(self): + """ Test the rebase PR API endpoint when using an API token and + its outcome. """ + + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + output = self.app.post('/api/0/test/pull-request/1/rebase', headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + {u'message': u'Pull-request rebased'} + ) + + user = tests.FakeUser(username='pingou') + with tests.user_set(self.app.application, user): + + data = {'requestid': self.request.uid, 'csrf_token': self.get_csrf()} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'FFORWARD', + u'message': u'The pull-request can be merged and ' + u'fast-forwarded', + u'short_code': u'Ok' + } + ) + + def test_rebase_api_conflicts(self): + """ Test the rebase PR API endpoint when logged in from the UI and + its outcome. """ + tests.add_content_to_git( + os.path.join(self.path, 'repos', 'test.git'), + branch='master', content="foobar baz") + + user = tests.FakeUser(username='pingou') + with tests.user_set(self.app.application, user): + output = self.app.post('/api/0/test/pull-request/1/rebase') + self.assertEqual(output.status_code, 400) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'error': u'Did not manage to rebase this pull-request', + u'error_code': u'ENOCODE' + } + ) + + data = {'requestid': self.request.uid, 'csrf_token': self.get_csrf()} + output = self.app.post('/pv/pull-request/merge', data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'code': u'CONFLICTS', + u'message': u'The pull-request cannot be merged due ' + u'to conflicts', + u'short_code': u'Conflicts' + } + ) + + def test_rebase_api_api_logged_in_unknown_project(self): + """ Test the rebase PR API endpoint when the project doesn't exist """ + + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + output = self.app.post('/api/0/unknown/pull-request/1/rebase', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + {u'error': u'Project not found', u'error_code': u'ENOPROJECT'} + ) + + def test_rebase_api_api_logged_in_unknown_pr(self): + """ Test the rebase PR API endpoint when the PR doesn't exist """ + + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token aaabbbcccddd'} + + output = self.app.post('/api/0/test/pull-request/404/rebase', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + {u'error': u'Pull-Request not found', u'error_code': u'ENOREQ'} + ) + + def test_rebase_api_api_logged_in_unknown_token(self): + """ Test the rebase PR API endpoint with an invalid API token """ + + tests.create_tokens(self.session) + tests.create_tokens_acl(self.session) + + headers = {'Authorization': 'token unknown'} + + output = self.app.post('/api/0/test/pull-request/1/rebase', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + u'error': u'Invalid or expired token. Please visit ' + 'http://localhost.localdomain/settings#api-keys to get ' + 'or renew your API token.', + u'error_code': u'EINVALIDTOK' + } + ) + + +if __name__ == '__main__': + unittest.main(verbosity=2) From 12d7b9e9b5c20c54b57ca10feec258fa349da10c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 12/15] Small code style change to improve readability Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/project.py b/pagure/api/project.py index ff52249..1a8de5d 100644 --- a/pagure/api/project.py +++ b/pagure/api/project.py @@ -150,10 +150,10 @@ def api_project_watchers(repo, username=None, namespace=None): if repo is None: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - implicit_watch_users = {repo.user.username} - for access_type in repo.access_users.keys(): - implicit_watch_users = implicit_watch_users | set( - [user.username for user in repo.access_users[access_type]] + implicit_watch_users = set([repo.user.username]) + for access_type in repo.access_users: + implicit_watch_users = implicit_watch_users.union( + set([user.username for user in repo.access_users[access_type]]) ) watching_users_to_watch_level = {} From ca7bbc1a5e95cab2c17867e42668873e475a11b5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 13/15] Fix notifications and refreshing the cached merge status upon updates Basically, before this commit, when we pushed a new commit to the git repo, we would update all the open pull-requests involved with this branch so that we can link them to tickets, then we would refresh all the open pull-requests of the project (ie: clear out their cached merge status). With this commit, before we link PRs to ticket, we refresh them, so it sends notifications about rebase to the configured bus (if any) and comments about it in the UI. This process also refreshes the cached merge status. So we have to be careful not to clear this updated cached merge status later otherwise we updated it for nothing. So this commit ensures we only clear cached merge status of PRs we have not refreshed manually. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/hooks/default.py b/pagure/hooks/default.py index 9ada8cc..fc35c72 100644 --- a/pagure/hooks/default.py +++ b/pagure/hooks/default.py @@ -145,6 +145,8 @@ def inform_pull_request_urls( if project.is_fork: target_repo = project.parent + pr_uids = [] + if ( commits and refname != default_branch @@ -153,13 +155,25 @@ def inform_pull_request_urls( print() prs = pagure.lib.query.search_pull_requests( session, - project_id_from=project.id, + project_id_from=target_repo.id, status="Open", branch_from=refname, ) + if project.id != target_repo.id: + prs.extend( + pagure.lib.query.search_pull_requests( + session, + project_id_from=project.id, + status="Open", + branch_from=refname, + ) + ) # Link to existing PRs if there are any seen = len(prs) != 0 for pr in prs: + # Refresh the PR in the db and everywhere else where needed + pagure.lib.tasks.update_pull_request.delay(pr.uid) + # Link tickets with pull-requests if the commit mentions it pagure.lib.tasks.link_pr_to_ticket.delay(pr.uid) @@ -169,8 +183,7 @@ def inform_pull_request_urls( " %s/%s/pull-request/%s" % (_config["APP_URL"].rstrip("/"), pr.project.url_path, pr.id) ) - # Refresh the PR in the db and everywhere else where needed - pagure.lib.tasks.update_pull_request.delay(pr.uid) + pr_uids.append(pr.uid) # If no existing PRs, provide the link to open one if not seen: @@ -186,6 +199,8 @@ def inform_pull_request_urls( ) print() + return pr_uids + class DefaultRunner(BaseRunner): """ Runner for the default hook.""" @@ -208,6 +223,8 @@ class DefaultRunner(BaseRunner): if not repo_obj.is_empty and not repo_obj.head_is_unborn: default_branch = repo_obj.head.shorthand + pr_uids = [] + for refname in changes: (oldrev, newrev) = changes[refname] @@ -264,17 +281,21 @@ class DefaultRunner(BaseRunner): # Now display to the user if this isn't the default branch links to # open a new pr or review the existing one - inform_pull_request_urls( - session, project, commits, refname, default_branch + pr_uids.extend( + inform_pull_request_urls( + session, project, commits, refname, default_branch + ) ) - # Schedule refresh of all opened PRs + # Refresh of all opened PRs parent = project.parent or project - pagure.lib.tasks.refresh_pr_cache.delay( + pagure.lib.tasks.refresh_pr_cache( parent.name, parent.namespace, parent.user.user if parent.is_fork else None, + but_uids=pr_uids, ) + if not project.is_on_repospanner and \ _config.get("GIT_GARBAGE_COLLECT", False): pagure.lib.tasks.git_garbage_collect.delay( diff --git a/pagure/lib/query.py b/pagure/lib/query.py index 19c1167..03baa60 100644 --- a/pagure/lib/query.py +++ b/pagure/lib/query.py @@ -3275,13 +3275,20 @@ def close_pull_request(session, request, user, merged=True): ) -def reset_status_pull_request(session, project): +def reset_status_pull_request(session, project, but_uids=None): """ Reset the status of all opened Pull-Requests of a project. """ - session.query(model.PullRequest).filter( - model.PullRequest.project_id == project.id - ).filter(model.PullRequest.status == "Open").update( - {model.PullRequest.merge_status: None} + query = ( + session.query(model.PullRequest) + .filter(model.PullRequest.project_id == project.id) + .filter(model.PullRequest.status == "Open") + ) + + if but_uids: + query = query.filter(model.PullRequest.uid.notin_(but_uids)) + + query.update( + {model.PullRequest.merge_status: None}, synchronize_session=False ) session.commit() diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index b26f284..28546b9 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -691,14 +691,16 @@ def move_to_repospanner(self, session, name, namespace, user, region): @conn.task(queue=pagure_config.get("FAST_CELERY_QUEUE", None), bind=True) @pagure_task -def refresh_pr_cache(self, session, name, namespace, user): +def refresh_pr_cache(self, session, name, namespace, user, but_uids=None): """ Refresh the merge status cached of pull-requests. """ project = pagure.lib.query._get_project( session, namespace=namespace, name=name, user=user ) - pagure.lib.query.reset_status_pull_request(session, project) + pagure.lib.query.reset_status_pull_request( + session, project, but_uids=but_uids + ) @conn.task(queue=pagure_config.get("FAST_CELERY_QUEUE", None), bind=True) From a093e7db45f847e43904195ed3fb974ecff437a5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:22 +0000 Subject: [PATCH 14/15] Catch exception thrown by merge_pull_request Let's catch these exception and just ignore them, they will be thrown elsewhere and in the tasks we can't do anything about it anyway Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 28546b9..df5b31e 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -881,12 +881,12 @@ def update_pull_request(self, session, pr_uid): request.id, ) - merge_status = pagure.lib.git.merge_pull_request( - session=session, - request=request, - username=None, - domerge=False, - ) + try: + pagure.lib.git.merge_pull_request( + session=session, request=request, username=None, domerge=False + ) + except pagure.exceptions.PagureException as err: + _log.debug(err) @conn.task(queue=pagure_config.get("MEDIUM_CELERY_QUEUE", None), bind=True) From 97d19d6dae9b3efb5d87bede579255fea157be0b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 23 2018 09:43:55 +0000 Subject: [PATCH 15/15] Project wide black and flake8 fixes --- diff --git a/pagure/hooks/default.py b/pagure/hooks/default.py index fc35c72..5ea0fa9 100644 --- a/pagure/hooks/default.py +++ b/pagure/hooks/default.py @@ -296,8 +296,9 @@ class DefaultRunner(BaseRunner): but_uids=pr_uids, ) - if not project.is_on_repospanner and \ - _config.get("GIT_GARBAGE_COLLECT", False): + if not project.is_on_repospanner and _config.get( + "GIT_GARBAGE_COLLECT", False + ): pagure.lib.tasks.git_garbage_collect.delay( project.repopath("main") ) diff --git a/pagure/lib/git.py b/pagure/lib/git.py index 31b5824..d477666 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -896,8 +896,8 @@ class TemporaryClone(object): self.repopath = tempfile.mkdtemp(prefix="pagure-%s-" % self._action) if not self._project.is_on_repospanner: # This is the simple case. Just do a local clone - # use either the specified path or the use the path of the specified - # project + # use either the specified path or the use the path of the + # specified project self._origpath = self._path or self._project.repopath( self._repotype ) @@ -1563,7 +1563,9 @@ def merge_pull_request(session, request, username, domerge=True): return "Changes merged!" else: - _log.info(" PR can be merged using fast-forward, reporting it") + _log.info( + " PR can be merged using fast-forward, reporting it" + ) request.merge_status = "FFORWARD" session.commit() return "FFORWARD" @@ -1674,7 +1676,9 @@ def merge_pull_request(session, request, username, domerge=True): pull_request=request, ) else: - _log.info(" PR can be merged using fast-forward, reporting it") + _log.info( + " PR can be merged using fast-forward, reporting it" + ) request.merge_status = "FFORWARD" session.commit() return "FFORWARD" @@ -2111,8 +2115,9 @@ def diff_pull_request( topic="pull-request.%s" % pr_action, msg=dict( pullrequest=request.to_json( - with_comments=False, public=True), - agent='pagure', + with_comments=False, public=True + ), + agent="pagure", ), ) pagure.lib.query.add_pull_request_comment( diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index df5b31e..eb44613 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -1173,7 +1173,4 @@ def git_garbage_collect(self, session, repopath): # libgit2 doesn't support "git gc" and probably never will: # https://github.com/libgit2/libgit2/issues/3247 _log.info("Running 'git gc --auto' for repo %s", repopath) - subprocess.check_output( - ["git", "gc", "--auto", "-q"], - cwd=repopath, - ) + subprocess.check_output(["git", "gc", "--auto", "-q"], cwd=repopath) diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index 9bc7d78..121bb7d 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -877,7 +877,7 @@ def new_issue(repo, username=None, namespace=None): open_access = repo.settings.get("open_metadata_access_to_all", False) milestones = [] - for m in (repo.milestones_keys or repo.milestones): + for m in repo.milestones_keys or repo.milestones: if m in repo.milestones and repo.milestones[m]["active"]: milestones.append(m) @@ -1080,7 +1080,7 @@ def view_issue(repo, issueid, username=None, namespace=None): status = pagure.lib.query.get_issue_statuses(flask.g.session) milestones = [] - for m in (repo.milestones_keys or repo.milestones): + for m in repo.milestones_keys or repo.milestones: if m in repo.milestones and repo.milestones[m]["active"]: milestones.append(m) @@ -1470,7 +1470,7 @@ def view_issue_raw_file(repo, filename=None, username=None, namespace=None): select="issues", repo=repo, username=username, - diff=data.decode('utf-8'), + diff=data.decode("utf-8"), patchfile=orig_filename, )