Issue 3674 - Add a hg hook that references commits in issues

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 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.
+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.
 """
 
 import posixpath
 import re
 import xmlrpclib
 
-from sitescripts.utils import get_config
-
-def _generate_changeset_url(repository_name, changeset_id):
-  template = get_config().get("hg", "changeset_url_template")
-  return template.format(repo=repository_name, id=changeset_id[:12])
+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",
+                          autoescape=False)
   for issue_id, changes in changes_by_issue.iteritems():
-    multiple_changes = len(changes) > 1
-    if multiple_changes:
-      comment = "Some commits referencing this issue have landed:\n"
-    else:
-      comment = "A commit referencing this issue has landed:\n"
-    for change in changes:
-      if multiple_changes:
-        comment += "- "
-      comment += _generate_changeset_url(repository_name, change.hex())
-      comment += "\n"
-    comments[issue_id] = comment

[Wladimir Palant, 2016/02/22 11:32:33]

I'd rather have this logic in a template. This should also make it easier to add
additional changeset info (author name, description).

[Felix Dahlke, 2016/02/22 20:09:49]

Done.

+    comments[issue_id] = template.render({"repository_name": repository_name,
+                                          "changes": changes})
   return comments
 
 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"})

[Wladimir Palant, 2016/02/22 12:02:51]

The ridiculous part about this is: there is a race condition here, if somebody
updates the issue after we get our _ts attribute but before we send the actual
update request then adding the comment will fail for no good reason. Since the
leave action is available regardless of the issue state we could avoid the
problem by omitting the _ts attribute. This will currently work but the plugin
author wants to deprecate this kind of request. Maybe omit _ts parameter and
communicate to the author why deprecating requests without _ts is a bad idea?

[Wladimir Palant, 2016/02/22 12:06:01]

Sorry about so many mails, I've only noticed some issues while reading
XmlRpcPlugin source code. I just realized that this needs one more parameter:
notify=True. Otherwise no notification mails will be sent which would be wrong
IMHO.

[Felix Dahlke, 2016/02/22 20:09:49]

Done.

[Felix Dahlke, 2016/02/22 20:09:50]

That seems to be quite the hassle for what I think is a fairly improbable edge
case. Note that there will be a warning printed saying that the comment couldn't
be added, and in that case people can still add a comment referencing the
commits manually.

+                       {"_ts": attributes["_ts"], "action": "leave"}, True)
 
 def hook(ui, repo, node, **kwargs):
   first_change = repo[node]
-  issue_number_regex = re.compile(r"Issue (\d+) - ")

[Wladimir Palant, 2016/02/22 11:32:33]

This is too restrictive, see
https://hg.adblockplus.org/adblockplus/rev/637c6ebb90c1 for example. It should
rather be re.compile(r"\bissue\s+(\d+)\b", re.I) IMHO.

[Felix Dahlke, 2016/02/22 20:09:49]

Done.

+  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()
-    match = issue_number_regex.search(description)

[Wladimir Palant, 2016/02/22 11:32:33]

What if there multiple issues referenced in the commit?

Also, we meant to validate commit messages all along - there should be either an
issue reference or Noissue. Maybe now is the right time?

[Felix Dahlke, 2016/02/22 20:09:49]

Never seen that before, but since you've shown a real life example above: Done.
Yeah we should absolutely have that, but so far I've just pointed this out as a
potential follow-up in #3674, because it's a bit complicated when multiple
changesets are being pushed and I'd rather start simple. I've created a
follow-up and added a comment explaining why I think it's complicated:
https://issues.adblockplus.org/ticket/3679

Meanwhile, I've added some code below that warns about an invalid commit message
format. It's a start...

-    if match:
-      issue_number = match.group(1)
-      changes_by_issue.setdefault(issue_number, []).append(change)
+    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("failed to update %s - does it exist?\n" % \

[Wladimir Palant, 2016/02/22 11:32:33]

This backslash is unnecessary.

[Felix Dahlke, 2016/02/22 20:09:49]

Done, also changed the message a bit.

+      ui.warn("warning: failed to update %s\n" %
               issue_url_template.format(id=issue_id))