Issue 7334 - Remove handling of "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
@@ -6,207 +6,96 @@
 # 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/>.
-
-"""Hooks for integrating Mercurial with Trac.
+# along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
 
-Update the issues that are referenced in commit messages when the commits
-are pushed and `master` bookmark is moved. See README.md for more
-information on behavior and configuration.
+"""A changegroup (or pretxnchangegroup) hook for Trac integration.

[mathias, 2019/05/14 09:32:02]

Why do you insist on naming the type(s) of hook?

[Vasily Kuznetsov, 2019/05/14 10:28:59]

This is the intended use and I haven't checked if it can work in other contexts,
so it seems like a useful bit of information here.

+
+Checks commit messages for issue references and posts comments linking to the
+commits into referenced issues.
 """
 
 import collections
-import contextlib
 import posixpath
 import re
 import xmlrpclib
 
 from sitescripts.utils import get_config, get_template
 
 
-_IssueRef = collections.namedtuple('IssueRef', 'id commits is_fixed')
-
-ISSUE_NUMBER_REGEX = re.compile(r'\b(issue|fixes)\s+(\d+)\b', re.I)
+ISSUE_NUMBER_REGEX = re.compile(r'\bissue\s+(\d+)\b', re.I)

[mathias, 2019/05/13 12:23:43]

Shouldn't this one include the separating dash? I.e. `Issue 1337 - Foo bar baz`?
Same goes for the NOISSUE_REGEX below, though that is not part of this patch-set
(yet).

[Vasily Kuznetsov, 2019/05/13 15:14:15]

Yeah I see what you mean, but I'd rather leave it as it was for now because:

- This commit is supposed to undo the change that added "fixes xxx" notation,
making "issue xxx" more precise is a bit out of scope.
- If we add the space and the dash to the regex, it will break the processing of
multiple references: "Issue 1, Issue 2 - foobar". We could fix it, of course, 
but that's extra work that doesn't seem justified at this point.

[mathias, 2019/05/14 09:32:01]

Acknowledged.

 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 _format_description(change):
     lines = change.description().splitlines()
     message = lines[0].rstrip()
     if len(lines) == 1 or lines[1].strip() == '':
         return message
     return message.rstrip('.') + '...'
 
 
-def _post_comments(ui, repo, config, refs):
-    repo_name = posixpath.split(repo.url())[1]
+def _generate_comments(repository_name, changes_by_issue):
+    comments = {}
     template = get_template('hg/template/issue_commit_comment.tmpl',
                             autoescape=False)
-    for ref in refs:
-        comment_text = template.render({
-            'repository_name': repo_name,
-            'changes': ref.commits,
+    for issue_id, changes in changes_by_issue.items():
+        comments[issue_id] = template.render({
+            'repository_name': repository_name,
+            'changes': changes,
             'format_description': _format_description,
         })
-        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)
+    return comments
 
 
-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 _get_module_milestones(ui, config, modules):
-    module_regexps = dict(_compile_module_regexps(ui, config, modules))
-    modules = module_regexps.keys()
-    if not modules:
-        return []
-
-    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
-
-    return milestones_by_module.items()
+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 _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))
+def hook(ui, repo, node, **kwargs):
+    """Post commit references into Trac issues."""
+    changes_by_issue = collections.defaultdict(list)
 
-    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()
-
+    first_rev = repo[node].rev()
+    commits = repo[first_rev:]
     for commit in commits:
-        message = commit.description()
-        if ' - ' not in message:
-            ui.warn("warning: invalid commit message format: '{}'\n"
-                    .format(message))
-            continue
+        description = commit.description()
+        issue_ids = ISSUE_NUMBER_REGEX.findall(description)
+        if issue_ids:
+            for issue_id in issue_ids:
+                changes_by_issue[issue_id].append(commit)
+        elif not NOISSUE_REGEX.search(description):
+            ui.warn('warning: invalid commit message format in changeset {}\n'
+                    .format(commit))
 
-        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)
-
+    repository_name = posixpath.split(repo.url())[1]
+    comments = _generate_comments(repository_name, changes_by_issue)
 
-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
+    issue_url_template = get_config().get('hg', 'issue_url_template')
+    for issue_id, comment in comments.items():
+        issue_url = issue_url_template.format(id=issue_id)
+        try:
+            _post_comment(issue_id, comment)
+            ui.status('updating {}\n'.format(issue_url))

[mathias, 2019/05/14 09:32:02]

This should not be in the `try` block. It should either come before the `try`
and the call to `_post_comment` OR use past tense and be in an `else` block. 

[Vasily Kuznetsov, 2019/05/14 10:28:59]

Makes sense. Done.

+        except Exception as exc:
+            ui.warn('warning: failed to update {}\n'.format(issue_url))
+            ui.warn('error message: {}\n'.format(exc))
 
-    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
+# Alias for smooth migration.
+changegroup_hook = hook

[mathias, 2019/05/14 09:32:01]

This should not be necessary at all:

    hg@hg-2:~$ grep changegroup_hook web/*/.hg/hgrc | wc -l
    0

[Vasily Kuznetsov, 2019/05/14 10:29:00]

👍
Done!