[FIX] runbot_merge: avoid double closes message

If the message already contains the pr number, no need to add it again

runbot_merge/models/pull_requests.py

@@ -922,6 +922,15 @@ class Batch(models.Model):
 
         :return: () or Batch object (if all prs successfully staged)
         """
+
+        def build_message(message, pr):
+            m = re.search(r'( |{repository})#{pr.number}\b'.format(
+                    pr=pr, repository=pr.repository.name.replace('/', '\\/')),
+                message)
+            if m:
+                return message
+            return message + '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr)
+
         new_heads = {}
         for pr in prs:
             gh = meta[pr.repository]['gh']
@@ -932,17 +941,17 @@ class Batch(models.Model):
             )
 
             target = 'tmp.{}'.format(pr.target.name)
-            suffix = '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr)
             try:
                 # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1]
                 pr_commits = gh.commits(pr.number)
                 rebase_and_merge = pr.rebase
                 squash = rebase_and_merge and len(pr_commits) == 1
                 if squash:
-                    pr_commits[0]['commit']['message'] += suffix
+                    msg = build_message(pr_commits[0]['commit']['message'], pr)
+                    pr_commits[0]['commit']['message'] = msg
                     new_heads[pr] = gh.rebase(pr.number, target, commits=pr_commits)
                 elif rebase_and_merge:
-                    msg = pr.message + suffix
+                    msg = build_message(pr.message, pr)
                     h = gh.rebase(pr.number, target, reset=True, commits=pr_commits)
                     new_heads[pr] = gh.merge(h, target, msg)['sha']
                 else:
@@ -964,8 +973,9 @@ class Batch(models.Model):
                         original_head = gh.head(target)
                         merge_tree = gh.merge(pr_head['sha'], target, 'temp merge')['tree']['sha']
                         new_parents = [original_head] + list(head_parents - {base_commit})
+                        msg = build_message(pr_head['commit']['message'], pr)
                         copy = gh('post', 'git/commits', json={
-                            'message': pr_head['commit']['message'] + suffix,
+                            'message': msg,
                             'tree': merge_tree,
                             'author': pr_head['commit']['author'],
                             'committer': pr_head['commit']['committer'],
@@ -975,7 +985,7 @@ class Batch(models.Model):
                         new_heads[pr] = copy['sha']
                     else:
                         # otherwise do a regular merge
-                        msg = pr.message + suffix
+                        msg = build_message(pr.message, pr)
                         new_heads[pr] = gh.merge(pr.head, target, msg)['sha']
             except (exceptions.MergeError, AssertionError) as e:
                 _logger.exception("Failed to merge %s:%s into staging branch (error: %s)", pr.repository.name, pr.number, e)

runbot_merge/tests/test_basic.py

@@ -62,7 +62,96 @@ def test_trivial_flow(env, repo):
         'a': b'some other content',
         'b': b'a second file',
     }
-    assert master.message, "gibberish\n\nblahblah\n\ncloses odoo/odoo#1"
+    assert master.message == "gibberish\n\nblahblah\n\ncloses {repo.name}#1".format(repo=repo)
+
+class TestCommitMessage:
+    def test_commit_simple(self, env, repo, users):
+        """ verify 'closes ...' is correctly added in the commit message
+        """
+        c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
+        repo.make_ref('heads/master', c1)
+        c2 = repo.make_commit(c1, 'simple commit message', None, tree={'f': 'm2'})
+
+        prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        prx.post_comment('hansen r+', "reviewer")
+
+        env['runbot_merge.project']._check_progress()
+
+        repo.post_status('heads/staging.master', 'success', 'ci/runbot')
+        repo.post_status('heads/staging.master', 'success', 'legal/cla')
+        env['runbot_merge.project']._check_progress()
+
+        master = repo.commit('heads/master')
+        assert master.message == "simple commit message\n\ncloses {repo.name}#1".format(repo=repo)
+
+    def test_commit_existing(self, env, repo, users):
+        """ verify do not duplicate 'closes' instruction
+        """
+        c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
+        repo.make_ref('heads/master', c1)
+        c2 = repo.make_commit(c1, 'simple commit message that closes #1', None, tree={'f': 'm2'})
+
+        prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        prx.post_comment('hansen r+', "reviewer")
+
+        env['runbot_merge.project']._check_progress()
+
+        repo.post_status('heads/staging.master', 'success', 'ci/runbot')
+        repo.post_status('heads/staging.master', 'success', 'legal/cla')
+        env['runbot_merge.project']._check_progress()
+
+        master = repo.commit('heads/master')
+        # closes #1 is already present, should not modify message
+        assert master.message == "simple commit message that closes #1"
+
+    def test_commit_other(self, env, repo, users):
+        """ verify do not duplicate 'closes' instruction
+        """
+        c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
+        repo.make_ref('heads/master', c1)
+        c2 = repo.make_commit(c1, 'simple commit message that closes odoo/enterprise#1', None, tree={'f': 'm2'})
+
+        prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        prx.post_comment('hansen r+', "reviewer")
+
+        env['runbot_merge.project']._check_progress()
+
+        repo.post_status('heads/staging.master', 'success', 'ci/runbot')
+        repo.post_status('heads/staging.master', 'success', 'legal/cla')
+        env['runbot_merge.project']._check_progress()
+
+        master = repo.commit('heads/master')
+        # closes on another repositoy, should modify the commit message
+        assert master.message == "simple commit message that closes odoo/enterprise#1\n\ncloses {repo.name}#1".format(repo=repo)
+
+    def test_commit_wrong_number(self, env, repo, users):
+        """ verify do not match on a wrong number
+        """
+        c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
+        repo.make_ref('heads/master', c1)
+        c2 = repo.make_commit(c1, 'simple commit message that closes #11', None, tree={'f': 'm2'})
+
+        prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        prx.post_comment('hansen r+', "reviewer")
+
+        env['runbot_merge.project']._check_progress()
+
+        repo.post_status('heads/staging.master', 'success', 'ci/runbot')
+        repo.post_status('heads/staging.master', 'success', 'legal/cla')
+        env['runbot_merge.project']._check_progress()
+
+        master = repo.commit('heads/master')
+        # closes on another repositoy, should modify the commit message
+        assert master.message == "simple commit message that closes #11\n\ncloses {repo.name}#1".format(repo=repo)
+
 
 class TestWebhookSecurity:
     def test_no_secret(self, env, project, repo):