Repository: smarkets/marge-bot

Issue:
Add reference to original merge request in commit message
Marge could put a link to original merge request in the commit message, presumably for every commit in a MR.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6627c28..9002695 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,5 @@
+  * 0.3.2: Fix support for branches with "/" in their names #50.
+  * 0.3.1: Fix start-up error when running as non-admin user #49.
   * 0.3.0:
     - Display better messages when GitLab refuses to merge #32, #33.
     - Handle auto-squash being selected #14.
diff --git a/README.md b/README.md
index 02f2f47..751b94a 100644
--- a/README.md
+++ b/README.md
@@ -139,9 +139,10 @@ ktmpl ./deploy.yml \
    commits in the merge request as described in the next section.
 
 
-## Adding Reviewed-by: and Tested: messages to commits
+## Adding Reviewed-by:, Tested: and Part-of: to commit messages
+
 Marge-bot supports automated addition of the following
-two [standardized git commit headers](https://www.kernel.org/doc/html/v4.11/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes):
+two [standardized git commit trailers](https://www.kernel.org/doc/html/v4.11/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes):
 `Reviewed-by` and `Tested-by`. For the latter it uses `Marge Bot
 <$MERGE_REQUEST_URL>` as a slight abuse of the convention (here `Marge Bot` is
 the name of the `marge-bot` user in GitLab).
@@ -155,20 +156,27 @@ target branch into your PR branch:
 Reviewed-by: A. Reviewer <a.reviewer@example.com>
 ```
 
-All existing `Reviewed-by:` tags on commits in the branch will be stripped. This
+All existing `Reviewed-by:` trailers on commits in the branch will be stripped. This
 feature requires marge to run with admin privileges due to a peculiarity of the
 GitLab API: only admin users can obtain email addresses of other users, even
 ones explicitly declared as public (strangely this limitation is particular to
 email, Skype handles etc. are visible to everyone).
 
-If you pass `--add-tested` the final commit in a PR will be tagged with
-`Tested-by: marge-bot <$MERGE_REQUEST_URL>`. This can be very useful for two
-reasons:
+If you pass `--add-tested` the final commit message in a PR will be tagged with
+`Tested-by: marge-bot <$MERGE_REQUEST_URL>` trailer. This can be very useful for
+two reasons:
 
 1. Seeing where stuff "came from" in a rebase-based workflow
 2. Knowing that a commit has been tested, which is e.g. important for bisection
    so you can easily and automatically `git bisect --skip` untested commits.
 
+Additionally, by using `--add-part-of`, all commit messages will be tagged with
+a `Part-of: <$MERGE_REQUEST_URL>` trailer to the merge request on which they
+were merged. This is useful, for example, to go from a commit shown in `git
+blame` to the merge request on which it was introduced or to easily revert a all
+commits introduced by a single Merge Request when using a fast-forward/rebase
+based merge workflow.
+
 ## Impersonating approvers
 If you want a full audit trail, you will configure Gitlab
 [require approvals](https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html#approvals-required)
diff --git a/deploy.yml b/deploy.yml
index 7f295c3..852e8de 100644
--- a/deploy.yml
+++ b/deploy.yml
@@ -36,7 +36,11 @@ objects:
             - name: app
               image: "$(APP_IMAGE)"
               imagePullPolicy: Always
-              args: ["--gitlab-url=$(MARGE_GITLAB_URL)", "--impersonate-approvers", "--add-tested"]
+              args: ["--gitlab-url=$(MARGE_GITLAB_URL)",
+                     "--impersonate-approvers",
+                     "--add-tested",
+                     "--add-reviewed-by",
+                     "--add-part-of"]
               env:
                 - name: MARGE_AUTH_TOKEN
                   valueFrom:
diff --git a/marge/app.py b/marge/app.py
index eda524f..847cffe 100644
--- a/marge/app.py
+++ b/marge/app.py
@@ -68,14 +68,19 @@ def _parse_args(args):
         help='Time(s) during which no merging is to take place, e.g. "Friday 1pm - Monday 9am".',
     )
     arg(
-        '--add-reviewers',
+        '--add-tested',
         action='store_true',
-        help='add Reviewed-by: $approver for each approver of PR to each commit in PR'
+        help='add Tested: marge-bot <$PR_URL> for the final commit on branch after it passed CI',
     )
     arg(
-        '--add-tested',
+        '--add-part-of',
         action='store_true',
-        help='add Tested: marge-bot <$PR_URL> for the final commit on branch after it passed CI',
+        help='add Part-of: <$PR_URL> to each commit in PR (except top commit when --add-tested is used)',
+    )
+    arg(
+        '--add-reviewers',
+        action='store_true',
+        help='add Reviewed-by: $approver for each approver of PR to each commit in PR',
     )
     arg(
         '--impersonate-approvers',
@@ -140,6 +145,7 @@ def main(args=sys.argv[1:]):
             project_regexp=options.project_regexp,
             merge_opts=bot.MergeJobOptions.default(
                 add_tested=options.add_tested,
+                add_part_of=options.add_part_of,
                 add_reviewers=options.add_reviewers,
                 reapprove=options.impersonate_approvers,
                 embargo=options.embargo,
diff --git a/marge/bot.py b/marge/bot.py
index ad92db5..2d9cd43 100644
--- a/marge/bot.py
+++ b/marge/bot.py
@@ -21,7 +21,7 @@ class Bot(object):
         opts = config.merge_opts
 
         if not user.is_admin:
-            assert not opts.impersonate_approvers, (
+            assert not opts.reapprove, (
                 "{0.username} is not an admin, can't impersonate!".format(user)
             )
             assert not opts.add_reviewers, (
diff --git a/marge/commit.py b/marge/commit.py
index b6df891..91a4316 100644
--- a/marge/commit.py
+++ b/marge/commit.py
@@ -1,5 +1,7 @@
 import re
 
+from requests.utils import quote
+
 from . import gitlab
 
 
@@ -23,7 +25,7 @@ class Commit(gitlab.Resource):
         info = api.call(GET(
             '/projects/{project_id}/repository/branches/{branch}'.format(
                 project_id=project_id,
-                branch=branch,
+                branch=quote(branch, safe=''),
             ),
         ))['commit']
         return cls(api, info)
diff --git a/marge/job.py b/marge/job.py
index b2078d8..5287bc4 100644
--- a/marge/job.py
+++ b/marge/job.py
@@ -10,7 +10,7 @@ from .project import Project
 from .user import User
 
 class MergeJob(object):
-    def __init__(self, api, user, project, merge_request, repo, options):
+    def __init__(self, *, api, user, project, merge_request, repo, options):
         self._api = api
         self._user = user
         self._project = project
@@ -100,6 +100,10 @@ class MergeJob(object):
                 _get_reviewer_names_and_emails(approvals=approvals, api=api) if self.opts.add_reviewers
                 else None
             )
+            part_of = (
+                '<{0.web_url}>'.format(merge_request) if self.opts.add_part_of
+                else None
+            )
             source_repo_url = None if source_project is self._project else source_project.ssh_url_to_repo
             # NB. this will be a no-op if there is nothing to rebase/rewrite
             target_sha, _rebased_sha, actual_sha = push_rebased_and_rewritten_version(
@@ -109,6 +113,7 @@ class MergeJob(object):
                 source_repo_url=source_repo_url,
                 reviewers=reviewers,
                 tested_by=tested_by,
+                part_of=part_of,
             )
             log.info('Commit id to merge %r (into: %r)', actual_sha, target_sha)
             time.sleep(5)
@@ -142,7 +147,7 @@ class MergeJob(object):
                 if new_target_sha != target_sha:
                     log.info('Someone was naughty and by-passed marge')
                     merge_request.comment(
-                        "My job would be easier if people didn't jump the queue and pushed directly... *sigh*"
+                        "My job would be easier if people didn't jump the queue and push directly... *sigh*"
                     )
                     continue
                 # otherwise the source branch has been pushed to or something
@@ -247,12 +252,14 @@ class MergeJob(object):
         return self.opts.embargo.covers(now)
 
 def push_rebased_and_rewritten_version(
+        *,
         repo,
         source_branch,
         target_branch,
         source_repo_url=None,
         reviewers=None,
         tested_by=None,
+        part_of=None,
 ):
     """Rebase `target_branch` into `source_branch`, optionally add trailers and push.
 
@@ -273,6 +280,8 @@ def push_rebased_and_rewritten_version(
     tested_by
        A list like ``["User Name <u.name@invalid.com>", ...]`` or `None`. ``None`` means
        existing Tested-by lines will be left alone, otherwise they will be replaced.
+    part_of
+       A string with likely a link to the merge request this commit is part-of, or ``None``.
 
     Returns
     -------
@@ -302,7 +311,14 @@ def push_rebased_and_rewritten_version(
                 trailer_name='Tested-by',
                 trailer_values=tested_by,
                 branch=source_branch,
-                start_commit=source_branch+'^'
+                start_commit=source_branch + '^'
+            )
+        if part_of is not None:
+            rewritten_sha = repo.tag_with_trailer(
+                trailer_name='Part-of',
+                trailer_values=[part_of],
+                branch=source_branch,
+                start_commit='origin/' + target_branch,
             )
         branch_rewritten = True
         repo.push_force(source_branch, source_repo_url)
@@ -336,7 +352,12 @@ def _get_reviewer_names_and_emails(approvals, api):
 
 
 _job_options = [
-    'add_tested', 'add_reviewers', 'reapprove', 'embargo', 'max_ci_waiting_time',
+    'add_tested',
+    'add_part_of',
+    'add_reviewers',
+    'reapprove',
+    'embargo',
+    'max_ci_waiting_time',
 ]
 
 class MergeJobOptions(namedtuple('MergeJobOptions', _job_options)):
@@ -344,18 +365,19 @@ class MergeJobOptions(namedtuple('MergeJobOptions', _job_options)):
 
     @property
     def requests_commit_tagging(self):
-        return self.add_tested or self.add_reviewers
+        return self.add_tested or self.add_part_of or self.add_reviewers
 
     @classmethod
     def default(
-            cls,
-            add_tested=False, add_reviewers=False, reapprove=False,
+            cls, *,
+            add_tested=False, add_part_of=False, add_reviewers=False, reapprove=False,
             embargo=None, max_ci_waiting_time=None,
     ):
         embargo = embargo or IntervalUnion.empty()
         max_ci_waiting_time = max_ci_waiting_time or timedelta(minutes=15)
         return cls(
             add_tested=add_tested,
+            add_part_of=add_part_of,
             add_reviewers=add_reviewers,
             reapprove=reapprove,
             embargo=embargo,
diff --git a/pinnedNixpkgs.nix b/pinnedNixpkgs.nix
index e8fe663..5ce5fce 100644
--- a/pinnedNixpkgs.nix
+++ b/pinnedNixpkgs.nix
@@ -1,1 +1,9 @@
-import (fetchTarball "https://github.com/NixOS/nixpkgs/archive/90afb0c10fe6f437fca498298747b2bcb6a77d39.tar.gz") { }
+let
+  fetchFromGitHub = (import <nixpkgs> {}).fetchFromGitHub;
+  pkgs = import (fetchFromGitHub {
+                   owner  = "NixOS";
+                   repo   = "nixpkgs";
+                   rev    = "90afb0c10fe6f437fca498298747b2bcb6a77d39";
+                   sha256 = "0mvzdw5aygi1vjnvm0bc8bp7iwb9rypiqg749m6a6km84m7srm0w";
+                 }) {};
+in pkgs
diff --git a/version b/version
index 0d91a54..d15723f 100644
--- a/version
+++ b/version
@@ -1,1 +1,1 @@
-0.3.0
+0.3.2


Test patch:
diff --git a/tests/test_commit.py b/tests/test_commit.py
index 60de697..789d0f9 100644
--- a/tests/test_commit.py
+++ b/tests/test_commit.py
@@ -48,6 +48,11 @@ class TestProject(object):
         Commit.last_on_branch(project_id=1234, branch='foobar-branch', api=self.api)
         self.api.call.assert_called_once_with(GET('/projects/1234/repository/branches/foobar-branch'))
 
+    def test_last_on_branch_encoding(self):
+        self.api.call.side_effect = lambda *_, **__: {'commit': INFO}
+        Commit.last_on_branch(project_id=1234, branch='foo/bar', api=self.api)
+        self.api.call.assert_called_once_with(GET('/projects/1234/repository/branches/foo%2Fbar'))
+
     def test_properties(self):
         commit = Commit(api=self.api, info=INFO)
         assert commit.id == "6104942438c14ec7bd21c6cd5bd995272b3faff6"
diff --git a/tests/test_job.py b/tests/test_job.py
index 984c9c4..fa16c39 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -121,7 +121,7 @@ class MockLab(object):
         )
         api.expected_note(
             self.merge_request_info,
-            "My job would be easier if people didn't jump the queue and pushed directly... *sigh*",
+            "My job would be easier if people didn't jump the queue and push directly... *sigh*",
             from_state=['pushed_but_master_moved', 'merge_rejected'],
         )
         api.expected_note(
@@ -244,7 +244,7 @@ class TestRebaseAndAccept(object):
             job.execute()
 
         assert api.state == 'merged'
-        assert api.notes == ["My job would be easier if people didn't jump the queue and pushed directly... *sigh*"]
+        assert api.notes == ["My job would be easier if people didn't jump the queue and push directly... *sigh*"]
 
     def test_handles_races_for_merging(self, time_sleep):
         api, mocklab = self.api, self.mocklab
@@ -397,6 +397,7 @@ class TestMergeJobOptions(object):
     def test_default(self):
         assert MergeJobOptions.default() == MergeJobOptions(
             add_tested=False,
+            add_part_of=False,
             add_reviewers=False,
             reapprove=False,
             embargo=marge.interval.IntervalUnion.empty(),