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
+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')
+
+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('\[\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=''):

[kzar, 2016/05/03 06:47:36]

Nit: Seems weird to have a blank string mean there's no comment. Wouldn't None
make more sense? (To me at least the blank string implies the comment is still
being used even when omitted, but I don't think that's the case.)

[Vasily Kuznetsov, 2016/05/03 10:11:02]

`comment` is passed to `ticket.update` at the end of this function and it seems
that it expects an empty string if there's no comment. If you give it `None`, it
breaks.

[kzar, 2016/05/03 10:13:49]

Ah, missed that. Fair enough.

+    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 {}'.format(changes['milestone']))

[kzar, 2016/05/03 06:47:36]

Nit: Seems inconsistent with above where you just append the two strings instead
of using format?

[Vasily Kuznetsov, 2016/05/03 10:11:02]

Yep. The style guide now recommends using + in this case.

Done.

+    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('^' + 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