[PATCH 5 of 5 STABLE V2] hook: omit newly added but omitable hook arguments for legacy Python hooks

[FUJIWARA Katsunori]

# HG changeset patch
# User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
# Date 1432064067 -32400
#      Wed May 20 04:34:27 2015 +0900
# Branch stable
# Node ID 47fc7dd58aa1f474e379014145d04904550dddfc
# Parent  8fd66d9066422cc0315a34ffe2e412ec2a02199c
hook: omit newly added but omitable hook arguments for legacy Python hooks

Before this patch, all keyword arguments specified to `hook.hook()`
are passed to Python hooks.

Then, adding hook arguments easily breaks backward compatibility with
existing Python hook implementations, which can accept only arguments
well documented in `hg help config`. For example:

  - `txnid` argument was newly added at 3.4 (or d283517b260b) to
    prechangegroup/changegroup hooks

    BTW, `txnname` shouldn't cause problem, because it is passed only
    to hooks newly added at 3.4 (`pretxnopen`, `pretxnclose`,
    `txnclose` and `txnabort`) and well documented.

  - to make in-memory dirstate changes visible to external process,
    `pending` argument will be added to `precommit`, `preupdate` and
    `update` hooks in the future (see also issue4378)

    In fact, just adding `pending` argument to `preupdate` hook will
    easily cause unexpected failure of current `eol.preupdate`
    implementation.

    BTW, `pretxncommit` and `pretxnchangegroup` hook developers have
    had to take care of undocumented `pending` argument since 1.2 (or
    b8d750daadde).

For backward compatibility with legacy Python hooks, this patch omits
hook arguments, which are newly added but omitable for legacy ones.

Hook arguments listed up in `hook._omitableargs` were extracted from
`grep 'hookargs\[.*\] ='` result and so on.

To reduce problematic Python hooks, this patch also adds the note
recommending to use `**kwargs` in Python hooks into `hooks` section in
`hg help config`.

BTW, wiki page below shows some examples using `**kwargs`, but it has
some warning at the beginning of it and seems not suitable to be
linked from `hg help config`.

  http://mercurial.selenic.com/wiki/HookExamples

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -910,6 +910,11 @@
 environment variables above are passed as keyword arguments, with no
 ``HG_`` prefix, and names in lower case.
 
+.. note::
+
+   For tolerance of additional hook arguments in the future, Python
+   hooks should have ``**kwargs`` in own function signature.
+
 If a Python hook returns a "true" value or raises an exception, this
 is treated as a failure.
 
diff --git a/mercurial/hook.py b/mercurial/hook.py
--- a/mercurial/hook.py
+++ b/mercurial/hook.py
@@ -6,9 +6,54 @@
 # GNU General Public License version 2 or any later version.
 
 from i18n import _
-import os, sys, time
+import os, sys, time, inspect
 import extensions, util, demandimport, error
 
+# Dict of hook arguments, which are newly added but omitable for
+# LEGACY hooks. Each omitable arguments can have white list of hook
+# names to avoid removing compulsory ones.
+_omitableargs = {
+    'bundle2': None,
+    'new_obsmarkers': None,
+    'pending': None,
+    'txnid': set("pretxnopen pretxnclose txnclose txnabort".split()),
+}
+
+def _fixupkwargs(hookname, func, kwargs):
+    '''Fix up hook arguments dict `kwargs` for `func`
+
+    For backward compatibility with legacy Python hooks, newly added
+    hook arguments should be omitted at invocation of them.
+
+    This returns the newly created dict, if `kwargs` should be fixed
+    up for `func`. Otherwise, this returns `kwargs` itself. This can
+    allow to use `kwargs` safely at outer loop while multiple hook
+    invocations.
+
+    `hookname` is used to examine whether an argument is compulsory
+    one or not for the hook.
+    '''
+    args, varargs, keywords, defaults = inspect.getargspec(func)
+    if keywords:
+        # func can always accept arbitrary arguments
+        return kwargs
+
+    unknowns = [n for n in kwargs
+               if n in _omitableargs and n not in args]
+    if not unknowns:
+        # func can accept all omitable arguments in kwargs, even
+        # though acceptance of all compulsory arguments isn't ensured
+        return kwargs
+
+    # remove omitable arguments, which are unknown for func
+    kwargs = kwargs.copy()
+    for unknown in unknowns:
+        whitelist = _omitableargs[unknown]
+        if not whitelist or hookname not in whitelist:
+            del kwargs[unknown]
+
+    return kwargs
+
 def _pythonhook(ui, repo, name, hname, funcname, args, throw):
     '''call python hook. hook is callable object, looked up as
     name in python module. if callable returns "true", hook
@@ -75,6 +120,8 @@
                                '("%s" is not callable)') %
                              (hname, funcname))
 
+    args = _fixupkwargs(name, obj, args)
+
     ui.note(_("calling hook %s: %s\n") % (hname, funcname))
     starttime = time.time()
 
diff --git a/tests/test-hook.t b/tests/test-hook.t
--- a/tests/test-hook.t
+++ b/tests/test-hook.t
@@ -321,6 +321,87 @@
   $ hg -q tip
   3:07f3376c1e65
 
+Test that some newly added (and/or undocumented) hook arguments are
+omitted for backward compatibiity with legacy Python hooks,
+
+  $ cat > $TESTTMP/pretxnchangegroup.checkargs.py <<EOF
+  > def _showargs(ui, hooktype, name, **kwargs):
+  >     ui.write('%s.%s: %s\n' % (hooktype, name, ','.join(sorted(kwargs))))
+  > 
+  > # accepts well documented (at 3.4) arguments only
+  > def minimum(ui, repo, hooktype, node, source, url):
+  >     _showargs(ui, hooktype, 'minimum',
+  >               node=node, source=source, url=url)
+  > 
+  > # accept also part of undocumented arguments
+  > def partial(ui, repo, hooktype, node, source, url, pending):
+  >     _showargs(ui, hooktype, 'partial',
+  >               node=node, source=source, url=url, pending=pending)
+  > 
+  > # accept any arguments by **kwargs
+  > def anyargs(ui, repo, hooktype, **kwargs):
+  >     _showargs(ui, hooktype, 'anyargs', **kwargs)
+  > 
+  > # accept part of well documented arguments (and cause failure)
+  > def invalid(ui, repo, hooktype, node, source, pending):
+  >     _showargs(ui, hooktype, 'invalid',
+  >               node=node, source=source, pending=pending)
+  > EOF
+  $ rm .hg/hgrc
+  $ cat > .hg/hgrc <<EOF
+  > [hooks]
+  > pretxnchangegroup.00 = python:$TESTTMP/pretxnchangegroup.checkargs.py:minimum
+  > pretxnchangegroup.01 = python:$TESTTMP/pretxnchangegroup.checkargs.py:partial
+  > pretxnchangegroup.02 = python:$TESTTMP/pretxnchangegroup.checkargs.py:anyargs
+  > pretxnchangegroup.03 = python:$TESTTMP/pretxnchangegroup.checkargs.py:invalid
+  > EOF
+  $ hg pull ../a 2>&1 | egrep -v '^( +File|    [_a-zA-Z(]|\*\* )'
+  pulling from ../a
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  pretxnchangegroup.minimum: node,source,url
+  pretxnchangegroup.partial: node,pending,source,url
+  pretxnchangegroup.anyargs: node,pending,source,txnid,url
+  error: pretxnchangegroup.03 hook raised an exception: invalid() got an unexpected keyword argument 'url'
+  transaction abort!
+  rollback completed
+  Traceback (most recent call last):
+  TypeError: invalid() got an unexpected keyword argument 'url'
+  $ hg -q tip
+  3:07f3376c1e65
+
+(test that white list of omitable hook arguments works correctly)
+
+  $ cat > $TESTTMP/pretxnopen.checkargs.py <<EOF
+  > def _showargs(ui, hooktype, name, **kwargs):
+  >     ui.write('%s.%s: %s\n' % (hooktype, name, ','.join(sorted(kwargs))))
+  > 
+  > # accept any arguments by **kwargs
+  > def anyargs(ui, repo, hooktype, **kwargs):
+  >     _showargs(ui, hooktype, 'anyargs', **kwargs)
+  > 
+  > # accept part of compulsory arguments (and cause failure)
+  > def invalid(ui, repo, hooktype, txnname):
+  >     _showargs(ui, hooktype, 'invalid',
+  >               txnname=txnname)
+  > EOF
+  $ rm .hg/hgrc
+  $ cat > .hg/hgrc <<EOF
+  > [hooks]
+  > pretxnopen.00 = python:$TESTTMP/pretxnopen.checkargs.py:anyargs
+  > pretxnopen.01 = python:$TESTTMP/pretxnopen.checkargs.py:invalid
+  > EOF
+  $ hg pull ../a 2>&1 | egrep -v '^( +File|    [_a-zA-Z(]|\*\* )'
+  pulling from ../a
+  searching for changes
+  pretxnopen.anyargs: txnid,txnname
+  error: pretxnopen.01 hook raised an exception: invalid() got an unexpected keyword argument 'txnid'
+  Traceback (most recent call last):
+  TypeError: invalid() got an unexpected keyword argument 'txnid'
+
 outgoing hooks can see env vars
 
   $ rm .hg/hgrc