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

sitescripts/hg/bin/update_issues.py

-#!/usr/bin/env python
-
 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # 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.
+"""Hook for integrating Mercurial with Trac.

[kzar, 2016/04/18 14:48:30]

Mind putting this documentation in a README (sitescripts/hg/README.md) instead?
That's what we've been doing for some of the other parts of sitescripts, for
example content_blocker_lists.

[Vasily Kuznetsov, 2016/04/24 21:49:50]

Done.

+
+The function called `hook` in this module should be installed as `changegroup`
+or `pretxnchangegroup` in target Mercurial repositories. It will recognise
+issue references in commit messages and update referenced issues in Adblock
+Plus issue tracker.
+
+The canonical format of the commit messages is "ISSUE-REFERENCE - MESSAGE"
+where ISSUE-REFERENCE is one of "Noissue", "Issue NUMBER" or "Fixes NUMBER".
+Several "Issue" and "Fixes" references can be present in the same commit
+message. Such commit will affect all the referenced issues.
+
+For "Issue" references a comment is posted into the referenced issue informing
+that a related commit has landed and providing a link to the commit.
+
+For "Fixes" references, in addition to posting the comment, the issue will be
+closed (unless it's already closed) and a module-dependent milestone will be
+assigned to it if configured and unless the issue already has a milestone.

[kzar, 2016/04/18 14:48:30]

We should also avoid assigning milestones and closing issues if the commit was
pushed to any branch other than master. Mind adding a note about that here?

[Vasily Kuznetsov, 2016/04/24 21:49:50]

Oh damn! I missed this in the original code. Now the issue is updated to make
this more clear and the code is separated into two hooks (it seems that's the
only way to track the movement of the `master` bookmark properly).

+
+# Configuration
+
+This hook is configured via `sitescripts.ini` using [hg] and
+[hg_module_milestones] sections.
+
+`track_xmlrpc_url` key from [hg] is used to determine the address of XMLRPC
+interface of Trac and `issue_url_template` as a template for producing links to
+the referenced issues that are displayed in the log.
+
+The keys of [hg_module_milestones] section are module names and the values are
+corresponding milestone regular expressions. The first open milestone that
+matches the regular expression of issue's module will be assigned to the issue
+when a commit fixing it arrives.
 """
 
+import collections
 import posixpath
 import re
 import xmlrpclib
 
 from sitescripts.utils import get_config, get_template
 
-def _generate_comments(repository_name, changes_by_issue):
-  comments = {}
+class _Update(object):

[kzar, 2016/04/18 14:48:30]

I guess I don't understand the need for this class, we could just use a
dictionary right?

[Vasily Kuznetsov, 2016/04/24 21:49:50]

Tried to make the code below prettier/more readable. But you're right, it's not
worth it, and `_Update` is redundant. I will remove it.

Done

+  def __init__(self, issue_id, commits, is_fixed):
+    self.issue_id = issue_id
+    self.commits = commits
+    self.is_fixed = is_fixed
+
+def _create_issue_updates(ui, repo, node):
+  issue_number_regex = re.compile(r"\b(issue|fixes)\s+(\d+)\b", re.I)
+  noissue_regex = re.compile(r"^noissue\b", re.I)
+  commits_by_issue = collections.defaultdict(list)
+  fixed_issues = set()
+  first_rev = repo[node].rev()
+  for change in repo[first_rev:]:
+    message = change.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(change)
+        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 _Update(issue_id, commits, is_fixed=issue_id in fixed_issues)
+
+def _prepare_changes(ui, trac, updates):
+  for update in updates:
+    try:
+      update.issue_attrs = trac.ticket.get(update.issue_id)[3]
+      update.changes = {"_ts": update.issue_attrs["_ts"], "action": "leave"}
+      if update.is_fixed:
+        actions = trac.ticket.getActions(update.issue_id)
+        if any(action[0] == "resolve" for action in actions):
+          update.changes["action"] = "resolve"
+      yield update
+    except Exception as exc:
+      if getattr(exc, 'faultCode', 0) == 404:  # Not found
+        ui.warn("warning: reference to a non-existent issue: {}\n"
+                .format(update.issue_id))
+      else:
+        ui.warn("warning: error while getting issue {}: {}\n"
+                .format(update.issue_id, exc))
+
+def _compile_module_regexps(ui, config, modules):
+  for module, regexp in config.items("hg_module_milestones"):
+    try:
+      yield module, re.compile("^" + regexp + "$", re.I)

[kzar, 2016/04/18 14:48:30]

I don't think this regexp should be case-insensitive.

[Vasily Kuznetsov, 2016/04/24 21:49:50]

My reasoning was that we want to be more forgiving to people who configure the
hook and create milestones, so that if they mess up the case, the hook won't
misbehave. The errors could easily go undetected, so we better try to make it
work in the hook.

Another reason is that the regexps for the commit message are case insensitive
(even though the errors there are more likely to be seen), so I thought that
would be consistent with this approach of being not completely strict.

Do you think there's a chance that the case-insensitive regexp will match
something it shouldn't match? If not, what arguments do you see for regexp being
case-sensitive?

[kzar, 2016/04/29 14:05:42]

Well the issue doesn't say the regexp here is matched case insensitively. Also
the examples that contain both upper and lower-case imply that case sensitive
matching will be used. Finally, unless I missed something, the documentation
doesn't mention that the regexp here will be matched case insensitively either.

I don't think it's too important which way we go but the documentation, examples
and issue should reflect reality at least.

[Vasily Kuznetsov, 2016/05/02 16:54:02]

I changed the README and the issue to reflect the fact that those regexps are
matched case-insensitively.

+    except Exception as exc:
+      ui.warn("warning: invalid module milestone regexp in config: '{}' ({})\n"
+              .format(regexp, exc))
+
+def _get_milestone_by_module(ui, config, trac, updates):
+  modules = {update.issue_attrs["component"] for update in updates
+             if update.is_fixed and not update.issue_attrs["milestone"]}
+  module_regexps = list(_compile_module_regexps(ui, config, modules))
+
+  def get_milestone_module(milestone_name):
+    for module, regex in module_regexps:
+      if regex.search(milestone_name):
+        return module
+    return None
+
+  milestones_by_module = collections.defaultdict(str)
+  if modules & {module for module, regexp in module_regexps}:
+    try:
+      milestone_names = filter(get_milestone_module,
+                               trac.ticket.milestone.getAll())
+      # Using a MultiCall is better because we might have many milestones.
+      multicall = xmlrpclib.MultiCall(trac)
+      for name in milestone_names:
+        multicall.ticket.milestone.get(name)
+      milestones = multicall()
+    except Exception as exc:
+      ui.warn("warning: unable to get milestones from trac: {}\n".format(exc))
+    else:
+      for milestone in milestones:
+        if not milestone["completed"]:
+          module = get_milestone_module(milestone["name"])
+          if module not in milestones_by_module:
+            milestones_by_module[module] = milestone["name"]
+  return milestones_by_module
+
+def _assign_milestones(ui, config, trac, updates):
+  updates = list(updates)
+  milestones_by_module = _get_milestone_by_module(ui, config, trac, updates)
+  for update in updates:
+    if update.is_fixed and not update.issue_attrs["milestone"]:
+      component = update.issue_attrs["component"]
+      if milestones_by_module[component]:
+        update.changes["milestone"] = milestones_by_module[component]
+    yield update
+
+def _format_comments(repo, updates):
+  repository_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 update in updates:
+    update.comment = template.render({"repository_name": repository_name,
+                                      "changes": update.commits})
+    yield update
 
-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 _apply_updates(ui, config, trac, updates):
+  issue_url_template = config.get("hg", "issue_url_template")
+  for update in updates:
+    issue_url = issue_url_template.format(id=update.issue_id)
+    try:
+      trac.ticket.update(update.issue_id, update.comment, update.changes, True)
+      updates = ["posted comment"]
+      if "milestone" in update.changes:
+        updates.append("set milestone: {}".format(update.changes["milestone"]))
+      if update.changes["action"] == "resolve":
+        updates.append("closed")
+      ui.status("updated {} ({})\n".format(issue_url, ', '.join(updates)))
+    except Exception as exc:
+      ui.warn("warning: failed to update {} ({})\n".format(issue_url, 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)
-
-  repository_name = posixpath.split(repo.url())[1]
-  comments = _generate_comments(repository_name, changes_by_issue)
-
-  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))
+  config = get_config()
+  trac_url = config.get("hg", "trac_xmlrpc_url")
+  trac = xmlrpclib.ServerProxy(trac_url)
+  updates = _create_issue_updates(ui, repo, node)

[kzar, 2016/04/18 14:48:30]

Couldn't we instead iterate through the updates once, doing the various
processing tasks as we go? (A loop in one place, calling various processing
functions for each update.) It seems kind of weird to me how we're creating the
`updates` generator(?) over and over, and iterating through it each time in the
various processing functions.

[Vasily Kuznetsov, 2016/04/24 21:49:50]

We're basically creating a stream of updates and send it through a bunch of
handlers that filter/change the updates. I agree, the naming looks a bit weird
in these `updates = foo(updates)`, `updates = bar(updates)` lines, but the code
is simpler this way.

After separating the logic into two hooks (that was necessary to react to the
movements of `master` bookmark) the logic of each of the hooks became simple
enough that it was possible to change it from this streaming pattern to more
imperative style. So, Done.

+  updates = _prepare_changes(ui, trac, updates)
+  updates = _assign_milestones(ui, config, trac, updates)
+  updates = _format_comments(repo, updates)
+  _apply_updates(ui, config, trac, updates)