Issue 3681 - Add suport for "Fixes XXXX - ..." commit messages

sitescripts/hg/bin/update_issues.py

Index: sitescripts/hg/bin/update_issues.py
===================================================================
--- a/sitescripts/hg/bin/update_issues.py
+++ b/sitescripts/hg/bin/update_issues.py
@@ -8,70 +8,194 @@
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
-"""
-This module implements a changegroup (or pretxnchangegroup) hook that inspects
-all commit messages and checks if issues from the Adblock Plus issue tracker are
-being referenced. If there are, it updates them with the respective changeset
-URLs.
+"""Hooks for integrating Mercurial with Trac.
+
+Update the issues that are referenced in commit messages when the commits
+are pushed and and "master" bookmark is moved. See README.md for more

[Felix Dahlke, 2016/05/17 20:12:06]

Nit: One "and" should do :) Also, "the master bookmark" I guess.

[Vasily Kuznetsov, 2016/05/18 08:30:44]

Done :)

Also changed the quotes around "master" to be consistent with how it's written
in the README.

+information on behavior and configuration.
 """
 
+import collections
+import contextlib
 import posixpath
 import re
 import xmlrpclib
 
 from sitescripts.utils import get_config, get_template
 
 
-def _generate_comments(repository_name, changes_by_issue):
-    comments = {}
-    template = get_template("hg/template/issue_commit_comment.tmpl",
+_IssueRef = collections.namedtuple('IssueRef', 'id commits is_fixed')

[Felix Dahlke, 2016/05/17 20:12:06]

AFAIK we mostly use double quotes (same as in JS where the two are also
functionally equivalent), I think we should stick to that consistently in new
code.

[Sebastian Noack, 2016/05/17 20:39:39]

We use single quotes consistently in new Python code (except when avoiding
escaping of embedded quotes). Frankly I did that ever since except when aiming
to be consistent with surrounding legacy code. But it's in the coding style now.
Also keep in mind that this matches the behaviour of repr() i.e. the natural
representation of literals in Python.

[Felix Dahlke, 2016/05/17 21:24:08]

I see. Makes more sense to me to do this consistently across languages, but it
doesn't matter much. From some quick grepping, it looks like around 60% of our
Python code uses single quotes, so it's the more popular thing already.

Had a quick look at some code in cpython to see what they use. Seems totally
inconsistent. Guess that explains why PEP-8 doesn't specify it :D

[Sebastian Noack, 2016/05/18 06:52:39]

I believe that PEP-8 doesn't dictates which quotes to use simply because it
isn't too important, and enforcing it could alienate some adopters. However,
still using single quotes is more natural in Python as this is what the repl and
built-in APIs do when preparing strings to be displayed. Also, most Python
documentation uses single quotes. While its exactly the other way around in
JavaScript, where browsers use double quotes when displaying strings in the
console, and most JavaScript documentation uses double quotes as well. So there
might actually be a reason to do it differently in different languages, in order
to stick to what's common in the context of those individual languages.

[Vasily Kuznetsov, 2016/05/18 08:30:44]

Fully agree with Sebastian here. Regarding JavaScript, I would also add that
JSON requires double quotes so there's another incentive to use double quotes
everywhere for consistency. So given that there's some, rather remote, but
still, similarity between `JSON.stringify` and `repr`, we're sort of doing the
same thing in our approach to Python strings.

Anyway, I like how our style guide is quite definite about the quotes and
(bonus!) we even have flake8-abp that automatically enforces it.

+
+ISSUE_NUMBER_REGEX = re.compile(r'\b(issue|fixes)\s+(\d+)\b', re.I)
+NOISSUE_REGEX = re.compile(r'^noissue\b', re.I)
+COMMIT_MESSAGE_REGEX = re.compile(r'\[\S+\ ([^\]]+)\]')
+
+
+@contextlib.contextmanager
+def _trac_proxy(ui, config, action_descr):
+    trac_url = config.get('hg', 'trac_xmlrpc_url')
+    try:
+        yield xmlrpclib.ServerProxy(trac_url)
+    except Exception as exc:
+        if getattr(exc, 'faultCode', 0) == 404:
+            ui.warn('warning: 404 (not found) while {}\n'.format(action_descr))
+        else:
+            ui.warn('error: {} while {}\n'.format(exc, action_descr))
+
+
+def _update_issue(ui, config, issue_id, changes, comment=''):
+    issue_url_template = config.get('hg', 'issue_url_template')
+    issue_url = issue_url_template.format(id=issue_id)
+
+    updates = []
+    if comment:
+        for message in COMMIT_MESSAGE_REGEX.findall(comment):
+            updates.append(' - referenced a commit: ' + message)
+    if 'milestone' in changes:
+        updates.append(' - set milestone to ' + changes['milestone'])
+    if changes['action'] == 'resolve':
+        updates.append(' - closed')
+    if not updates:
+        return
+
+    with _trac_proxy(ui, config, 'updating issue {}'.format(issue_id)) as tp:
+        tp.ticket.update(issue_id, comment, changes, True)
+        ui.status('updated {}:\n{}\n'.format(issue_url, '\n'.join(updates)))
+
+
+def _post_comments(ui, repo, config, refs):
+    repo_name = posixpath.split(repo.url())[1]
+    template = get_template('hg/template/issue_commit_comment.tmpl',
                             autoescape=False)
-    for issue_id, changes in changes_by_issue.iteritems():
-        comments[issue_id] = template.render({"repository_name": repository_name,
-                                              "changes": changes})
-    return comments
+    for ref in refs:
+        comment_text = template.render({'repository_name': repo_name,
+                                        'changes': ref.commits})
+        with _trac_proxy(ui, config, 'getting issue {}'.format(ref.id)) as tp:
+            attrs = tp.ticket.get(ref.id)[3]
+            changes = {'_ts': attrs['_ts'], 'action': 'leave'}
+            _update_issue(ui, config, ref.id, changes, comment_text)
 
 
-def _post_comment(issue_id, comment):
-    issue_id = int(issue_id)
-    url = get_config().get("hg", "trac_xmlrpc_url")
-    server = xmlrpclib.ServerProxy(url)
-    attributes = server.ticket.get(issue_id)[3]
-    server.ticket.update(issue_id, comment,
-                         {"_ts": attributes["_ts"], "action": "leave"}, True)
+def _compile_module_regexps(ui, config, modules):
+    for module, regexp in config.items('hg_module_milestones'):
+        try:
+            yield module, re.compile('^{}$'.format(regexp), re.I)
+        except Exception as exc:
+            ui.warn('warning: skipped invalid regexp for module {} in '
+                    "[hg_module_milestones] config: '{}' ({})\n"
+                    .format(module, regexp, exc))
 
 
-def hook(ui, repo, node, **kwargs):
-    first_change = repo[node]
-    issue_number_regex = re.compile(r"\bissue\s+(\d+)\b", re.I)
-    noissue_regex = re.compile(r"^noissue\b", re.I)
-    changes_by_issue = {}
-    for revision in xrange(first_change.rev(), len(repo)):
-        change = repo[revision]
-        description = change.description()
-        issue_ids = issue_number_regex.findall(description)
-        if issue_ids:
-            for issue_id in issue_ids:
-                changes_by_issue.setdefault(issue_id, []).append(change)
-        elif not noissue_regex.search(description):
-            # We should just reject all changes when one of them has an invalid
-            # commit message format, see: https://issues.adblockplus.org/ticket/3679
-            ui.warn("warning: invalid commit message format in changeset %s\n" %
-                    change)
+def _get_module_milestones(ui, config, modules):
+    module_regexps = dict(_compile_module_regexps(ui, config, modules))
+    modules = module_regexps.keys()
+    if not modules:
+        return []
 
-    repository_name = posixpath.split(repo.url())[1]
-    comments = _generate_comments(repository_name, changes_by_issue)
+    milestones_by_module = {}
+    with _trac_proxy(ui, config, 'getting milestones') as tp:
+        milestone_names = [
+            name for name in tp.ticket.milestone.getAll()
+            if any(regexp.search(name) for regexp in module_regexps.values())
+        ]
+        # Using a MultiCall is better because we might have many milestones.
+        get_milestones = xmlrpclib.MultiCall(tp)
+        for name in milestone_names:
+            get_milestones.ticket.milestone.get(name)
+        milestones = [ms for ms in get_milestones() if not ms['completed']]
+        for module in modules:
+            for milestone in milestones:
+                if module_regexps[module].search(milestone['name']):
+                    milestones_by_module[module] = milestone['name']
+                    break
 
-    issue_url_template = get_config().get("hg", "issue_url_template")
-    for issue_id, comment in comments.iteritems():
-        try:
-            _post_comment(issue_id, comment)
-            ui.status("updating %s\n" % issue_url_template.format(id=issue_id))
-        except:
-            ui.warn("warning: failed to update %s\n" %
-                    issue_url_template.format(id=issue_id))
+    return milestones_by_module.items()
+
+
+def _declare_fixed(ui, config, refs):
+    updates = []
+    # Changes that need milestones added to them, indexed by module.
+    need_milestones = collections.defaultdict(list)
+
+    for ref in refs:
+        with _trac_proxy(ui, config, 'getting issue {}'.format(ref.id)) as tp:
+            attrs = tp.ticket.get(ref.id)[3]
+            changes = {
+                '_ts': attrs['_ts'],
+                'action': 'leave'
+            }
+            actions = tp.ticket.getActions(ref.id)
+            if any(action[0] == 'resolve' for action in actions):
+                changes['action'] = 'resolve'
+            if not attrs['milestone']:
+                need_milestones[attrs['component']].append(changes)
+            updates.append((ref.id, changes))
+
+    for module, milestone in _get_module_milestones(ui, config,
+                                                    need_milestones.keys()):
+        for changes in need_milestones[module]:
+            changes['milestone'] = milestone
+
+    for issue_id, changes in updates:
+        _update_issue(ui, config, issue_id, changes)
+
+
+def _collect_references(ui, commits):
+    commits_by_issue = collections.defaultdict(list)
+    fixed_issues = set()
+
+    for commit in commits:
+        message = commit.description()
+        if ' - ' not in message:
+            ui.warn("warning: invalid commit message format: '{}'\n"
+                    .format(message))
+            continue
+
+        refs, rest = message.split(' - ', 1)
+        issue_refs = ISSUE_NUMBER_REGEX.findall(refs)
+        if issue_refs:
+            for ref_type, issue_id in issue_refs:
+                issue_id = int(issue_id)
+                commits_by_issue[issue_id].append(commit)
+                if ref_type.lower() == 'fixes':
+                    fixed_issues.add(issue_id)
+        elif not NOISSUE_REGEX.search(refs):
+            ui.warn("warning: no issue reference in commit message: '{}'\n"
+                    .format(message))
+
+    for issue_id, commits in sorted(commits_by_issue.items()):
+        yield _IssueRef(issue_id, commits, is_fixed=issue_id in fixed_issues)
+
+
+def changegroup_hook(ui, repo, node, **kwargs):
+    config = get_config()
+    first_rev = repo[node].rev()
+    pushed_commits = repo[first_rev:]
+    refs = _collect_references(ui, pushed_commits)
+    _post_comments(ui, repo, config, refs)
+
+
+def pushkey_hook(ui, repo, **kwargs):
+    if (kwargs['namespace'] != 'bookmarks' or  # Not a bookmark move.
+            kwargs['key'] != 'master' or       # Not master bookmark.
+            not kwargs['old']):                # The bookmark is just created.
+        return
+
+    config = get_config()
+    old_master_rev = repo[kwargs['old']].rev()
+    new_master_rev = repo[kwargs['new']].rev()
+    added_revs = repo.changelog.findmissingrevs([old_master_rev],
+                                                [new_master_rev])
+    added_commits = [repo[rev] for rev in added_revs]
+    refs = [ref for ref in _collect_references(ui, added_commits)
+            if ref.is_fixed]
+    _declare_fixed(ui, config, refs)
+
+
+# Alias for backward compatibility.
+hook = changegroup_hook