Patchwork hook: add more robust python: syntax checking

login
register
mail settings
Submitter Tom Hindle
Date May 24, 2018, 9:35 p.m.
Message ID <a30dbd52f2148af7f2d9.1527197740@hindlet>
Download mbox | patch
Permalink /patch/31846/
State New
Headers show

Comments

Tom Hindle - May 24, 2018, 9:35 p.m.
# HG changeset patch
# User hindlemail <tom_hindle@sil.org>
# Date 1527195876 21600
#      Thu May 24 15:04:36 2018 -0600
# Node ID a30dbd52f2148af7f2d9167b5935367e899b7244
# Parent  bd7a3fa71a72666691b8b77e6bf03be1d2273391
hook: add more robust python: syntax checking

Added checking for missing trailing :hook when python file is a full path and
running on windows. Provides an easier to understand error message.
Yuya Nishihara - May 25, 2018, 12:50 p.m.
On Thu, 24 May 2018 15:35:40 -0600, tom_hindle@sil.org wrote:
> # HG changeset patch
> # User hindlemail <tom_hindle@sil.org>
> # Date 1527195876 21600
> #      Thu May 24 15:04:36 2018 -0600
> # Node ID a30dbd52f2148af7f2d9167b5935367e899b7244
> # Parent  bd7a3fa71a72666691b8b77e6bf03be1d2273391
> hook: add more robust python: syntax checking
> 
> Added checking for missing trailing :hook when python file is a full path and
> running on windows. Provides an easier to understand error message.
> 
> diff -r bd7a3fa71a72 -r a30dbd52f214 mercurial/hook.py
> --- a/mercurial/hook.py	Thu May 24 23:26:28 2018 +0900
> +++ b/mercurial/hook.py	Thu May 24 15:04:36 2018 -0600
> @@ -240,18 +240,24 @@ def runhooks(ui, repo, htype, hooks, thr
>                          hint = _("see 'hg help config.trusted'"))
>                  ui.warn(_('warning: untrusted hook %s not executed\n') % hname)
>                  r = 1
>                  raised = False
>              elif callable(cmd):
>                  r, raised = pythonhook(ui, repo, htype, hname, cmd, args,
>                                          throw)
>              elif cmd.startswith('python:'):
> +                originalcmd = cmd
>                  if cmd.count(':') >= 2:
>                      path, cmd = cmd[7:].rsplit(':', 1)
> +                    if '\\' in cmd or '/' in cmd:
> +                        if throw:
> +                            raise error.HookAbort(
> +                              _("invalid 'python:' syntax: %s") % originalcmd)
> +                        ui.warn(_("invalid 'python:' syntax: %s") % originalcmd)

Perhaps it shouldn't continue even if throw=False. I'm not sure which will be
the correct behavior.

 a) r, raised = 1, False (the same behavior as cmd is untrusted)
 b) raise Abort, HookAbort, or ParseError no matter if throw is False

Patch

diff -r bd7a3fa71a72 -r a30dbd52f214 mercurial/hook.py
--- a/mercurial/hook.py	Thu May 24 23:26:28 2018 +0900
+++ b/mercurial/hook.py	Thu May 24 15:04:36 2018 -0600
@@ -240,18 +240,24 @@  def runhooks(ui, repo, htype, hooks, thr
                         hint = _("see 'hg help config.trusted'"))
                 ui.warn(_('warning: untrusted hook %s not executed\n') % hname)
                 r = 1
                 raised = False
             elif callable(cmd):
                 r, raised = pythonhook(ui, repo, htype, hname, cmd, args,
                                         throw)
             elif cmd.startswith('python:'):
+                originalcmd = cmd
                 if cmd.count(':') >= 2:
                     path, cmd = cmd[7:].rsplit(':', 1)
+                    if '\\' in cmd or '/' in cmd:
+                        if throw:
+                            raise error.HookAbort(
+                              _("invalid 'python:' syntax: %s") % originalcmd)
+                        ui.warn(_("invalid 'python:' syntax: %s") % originalcmd)
                     path = util.expandpath(path)
                     if repo:
                         path = os.path.join(repo.root, path)
                     try:
                         mod = extensions.loadpath(path, 'hghook.%s' % hname)
                     except Exception:
                         ui.write(_("loading %s hook failed:\n") % hname)
                         raise
diff -r bd7a3fa71a72 -r a30dbd52f214 tests/test-hook.t
--- a/tests/test-hook.t	Thu May 24 23:26:28 2018 +0900
+++ b/tests/test-hook.t	Thu May 24 15:04:36 2018 -0600
@@ -695,16 +695,38 @@  test python hook configured with python:
 
   $ hg id
   loading pre-identify.npmd hook failed:
   abort: No module named repo!
   [255]
 
   $ cd ../../b
 
+test python hook with missing hook and file path contains colon
+
+  $ cd ..
+  $ mkdir e
+  $ cd e
+  $ hg init repo
+
+  $ mkdir -p $TESTTMP/some:dir/
+  $ cat > $TESTTMP/some:dir/testhooks.py <<EOF
+  > def testhook(ui, **args):
+  >     ui.write(b'hook works\n')
+  > EOF
+  $ echo '[hooks]' > repo/.hg/hgrc
+  $ echo "pre-commit.test = python:$TESTTMP/some:dir/testhooks.py" >> repo/.hg/hgrc
+
+  $ cd repo
+  $ hg commit -d '0 0'
+  abort: invalid 'python:' syntax: python:$TESTTMP/some:dir/testhooks.py
+  [255]
+
+  $ cd ../../b
+
 make sure --traceback works on hook import failure
 
   $ cat > importfail.py <<EOF
   > import somebogusmodule
   > # dereference something in the module to force demandimport to load it
   > somebogusmodule.whatever
   > EOF