Patchwork [RFC] dispatch: print 'abort:' when a pre-command hook fails

login
register
mail settings
Submitter Siddharth Agarwal
Date April 16, 2013, 9:44 p.m.
Message ID <58f26562aa8cc468c651.1366148658@sid0x220>
Download mbox | patch
Permalink /patch/1369/
State Accepted, archived
Headers show

Comments

Siddharth Agarwal - April 16, 2013, 9:44 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1366148377 25200
#      Tue Apr 16 14:39:37 2013 -0700
# Node ID 58f26562aa8cc468c651d65b55c16e2c0f9946df
# Parent  1e9704ba956a08cb298be3afb9a90fcf9d54c142
dispatch: print 'abort:' when a pre-command hook fails

This also changes the exit code from whatever the hook returned to 255. This
brings it in line with all the other hooks that abort.
Siddharth Agarwal - April 16, 2013, 9:45 p.m.
On 04/16/2013 02:44 PM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1366148377 25200
> #      Tue Apr 16 14:39:37 2013 -0700
> # Node ID 58f26562aa8cc468c651d65b55c16e2c0f9946df
> # Parent  1e9704ba956a08cb298be3afb9a90fcf9d54c142
> dispatch: print 'abort:' when a pre-command hook fails
>
> This also changes the exit code from whatever the hook returned to 255. This
> brings it in line with all the other hooks that abort.

RFC because I'm wondering whether this count as breaking BC.
Mads Kiilerich - April 16, 2013, 10:27 p.m.
On 04/16/2013 11:44 PM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1366148377 25200
> #      Tue Apr 16 14:39:37 2013 -0700
> # Node ID 58f26562aa8cc468c651d65b55c16e2c0f9946df
> # Parent  1e9704ba956a08cb298be3afb9a90fcf9d54c142
> dispatch: print 'abort:' when a pre-command hook fails
>
> This also changes the exit code from whatever the hook returned to 255. This
> brings it in line with all the other hooks that abort.

For the record: The core of this change is to set throw=True when 
calling hook() - not completely trivial to spot when looking at the diff.

/Mads

>
> diff -r 1e9704ba956a -r 58f26562aa8c mercurial/dispatch.py
> --- a/mercurial/dispatch.py	Thu Mar 28 00:30:40 2013 -0700
> +++ b/mercurial/dispatch.py	Tue Apr 16 14:39:37 2013 -0700
> @@ -508,10 +508,8 @@ def _earlygetopt(aliases, args):
>   
>   def runcommand(lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions):
>       # run pre-hook, and abort if it fails
> -    ret = hook.hook(lui, repo, "pre-%s" % cmd, False, args=" ".join(fullargs),
> -                    pats=cmdpats, opts=cmdoptions)
> -    if ret:
> -        return ret
> +    hook.hook(lui, repo, "pre-%s" % cmd, True, args=" ".join(fullargs),
> +              pats=cmdpats, opts=cmdoptions)
>       ret = _runcommand(ui, options, cmd, d)
>       # run post-hook, passing command result
>       hook.hook(lui, repo, "post-%s" % cmd, False, args=" ".join(fullargs),
> diff -r 1e9704ba956a -r 58f26562aa8c tests/test-hook.t
> --- a/tests/test-hook.t	Thu Mar 28 00:30:40 2013 -0700
> +++ b/tests/test-hook.t	Tue Apr 16 14:39:37 2013 -0700
> @@ -71,8 +71,8 @@ test generic hooks
>   
>     $ hg id
>     pre-identify hook: HG_ARGS=id HG_OPTS={'bookmarks': None, 'branch': None, 'id': None, 'insecure': None, 'num': None, 'remotecmd': '', 'rev': '', 'ssh': '', 'tags': None} HG_PATS=[]
> -  warning: pre-identify hook exited with status 1
> -  [1]
> +  abort: pre-identify hook exited with status 1
> +  [255]
>     $ hg cat b
>     pre-cat hook: HG_ARGS=cat b HG_OPTS={'decode': None, 'exclude': [], 'include': [], 'output': '', 'rev': ''} HG_PATS=['b']
>     b
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - April 17, 2013, 12:28 a.m.
On Tue, 2013-04-16 at 14:44 -0700, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1366148377 25200
> #      Tue Apr 16 14:39:37 2013 -0700
> # Node ID 58f26562aa8cc468c651d65b55c16e2c0f9946df
> # Parent  1e9704ba956a08cb298be3afb9a90fcf9d54c142
> dispatch: print 'abort:' when a pre-command hook fails

> This also changes the exit code from whatever the hook returned to 255. This
> brings it in line with all the other hooks that abort.

Queued for default with added (BC) tag. I think this is clearly more
correct.

Patch

diff -r 1e9704ba956a -r 58f26562aa8c mercurial/dispatch.py
--- a/mercurial/dispatch.py	Thu Mar 28 00:30:40 2013 -0700
+++ b/mercurial/dispatch.py	Tue Apr 16 14:39:37 2013 -0700
@@ -508,10 +508,8 @@  def _earlygetopt(aliases, args):
 
 def runcommand(lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions):
     # run pre-hook, and abort if it fails
-    ret = hook.hook(lui, repo, "pre-%s" % cmd, False, args=" ".join(fullargs),
-                    pats=cmdpats, opts=cmdoptions)
-    if ret:
-        return ret
+    hook.hook(lui, repo, "pre-%s" % cmd, True, args=" ".join(fullargs),
+              pats=cmdpats, opts=cmdoptions)
     ret = _runcommand(ui, options, cmd, d)
     # run post-hook, passing command result
     hook.hook(lui, repo, "post-%s" % cmd, False, args=" ".join(fullargs),
diff -r 1e9704ba956a -r 58f26562aa8c tests/test-hook.t
--- a/tests/test-hook.t	Thu Mar 28 00:30:40 2013 -0700
+++ b/tests/test-hook.t	Tue Apr 16 14:39:37 2013 -0700
@@ -71,8 +71,8 @@  test generic hooks
 
   $ hg id
   pre-identify hook: HG_ARGS=id HG_OPTS={'bookmarks': None, 'branch': None, 'id': None, 'insecure': None, 'num': None, 'remotecmd': '', 'rev': '', 'ssh': '', 'tags': None} HG_PATS=[]
-  warning: pre-identify hook exited with status 1
-  [1]
+  abort: pre-identify hook exited with status 1
+  [255]
   $ hg cat b
   pre-cat hook: HG_ARGS=cat b HG_OPTS={'decode': None, 'exclude': [], 'include': [], 'output': '', 'rev': ''} HG_PATS=['b']
   b