Patchwork [1,of,3] hook: small refactor in the way we compute the hooks list

login
register
mail settings
Submitter Pierre-Yves David
Date April 15, 2016, 7:50 a.m.
Message ID <063993ec1c63c22ff616.1460706620@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/14636/
State Superseded
Headers show

Comments

Pierre-Yves David - April 15, 2016, 7:50 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1460626126 25200
#      Thu Apr 14 02:28:46 2016 -0700
# Node ID 063993ec1c63c22ff616609001b50292668bd92e
# Parent  8d398155bfda3a98e664af74096f0e405cc7dd09
# EXP-Topic hooks
hook: small refactor in the way we compute the hooks list

We are about to take untrusted hooks into account (to report them as failure)
so we need to rearrange the code a bit to allow config overwriting each other
in a later patch.
Simon Farnsworth - April 15, 2016, 11:26 a.m.
On 15/04/2016 08:50, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1460626126 25200
> #      Thu Apr 14 02:28:46 2016 -0700
> # Node ID 063993ec1c63c22ff616609001b50292668bd92e
> # Parent  8d398155bfda3a98e664af74096f0e405cc7dd09
> # EXP-Topic hooks
> hook: small refactor in the way we compute the hooks list
>
> We are about to take untrusted hooks into account (to report them as failure)
> so we need to rearrange the code a bit to allow config overwriting each other
> in a later patch.
>
> diff -r 8d398155bfda -r 063993ec1c63 mercurial/hook.py
> --- a/mercurial/hook.py	Mon Feb 29 22:58:15 2016 +0900
> +++ b/mercurial/hook.py	Thu Apr 14 02:28:46 2016 -0700
> @@ -162,12 +162,13 @@
>       return r
>
>   def _allhooks(ui):
> -    hooks = []
> +    hooks = {}
>       for name, cmd in ui.configitems('hooks'):
>           if not name.startswith('priority'):
>               priority = ui.configint('hooks', 'priority.%s' % name, 0)
> -            hooks.append((-priority, len(hooks), name, cmd))
> -    return [(k, v) for p, o, k, v in sorted(hooks)]
> +            hooks[name] = (-priority, len(hooks), cmd)
> +    order = sorted(hooks, key=lambda x: hooks[x][:2] + (x,))
> +    return [(k, hooks[k][2]) for k in order]

I'd find this easier to review as:

     hooks[name] = (-priority, len(hooks), name, cmd)
return [(k, v) for p, o, k, v in sorted(hooks.values())]

with similar changes to the other two patches. That way, it's obvious 
that the behaviour isn't changing until patch 3, and there's less 
counting of elements to do.
>
>   _redirect = False
>   def redirect(state):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=1nPp7BXuxnlF6QBfJE7KJ-7-fQ215xgb6pApq6RxM_E&s=cwskb1ilApEaJL4pTniUVp4NIv7KaEiu2u5BpGweq3A&e=
>
timeless - April 15, 2016, 1:15 p.m.
> We are about to take untrusted hooks into account (to report them as
failure)

As failures
Pierre-Yves David - April 15, 2016, 6:46 p.m.
On 04/15/2016 04:26 AM, Simon Farnsworth wrote:
> On 15/04/2016 08:50, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1460626126 25200
>> #      Thu Apr 14 02:28:46 2016 -0700
>> # Node ID 063993ec1c63c22ff616609001b50292668bd92e
>> # Parent  8d398155bfda3a98e664af74096f0e405cc7dd09
>> # EXP-Topic hooks
>> hook: small refactor in the way we compute the hooks list
>>
>> We are about to take untrusted hooks into account (to report them as 
>> failure)
>> so we need to rearrange the code a bit to allow config overwriting 
>> each other
>> in a later patch.
>>
>> diff -r 8d398155bfda -r 063993ec1c63 mercurial/hook.py
>> --- a/mercurial/hook.py    Mon Feb 29 22:58:15 2016 +0900
>> +++ b/mercurial/hook.py    Thu Apr 14 02:28:46 2016 -0700
>> @@ -162,12 +162,13 @@
>>       return r
>>
>>   def _allhooks(ui):
>> -    hooks = []
>> +    hooks = {}
>>       for name, cmd in ui.configitems('hooks'):
>>           if not name.startswith('priority'):
>>               priority = ui.configint('hooks', 'priority.%s' % name, 0)
>> -            hooks.append((-priority, len(hooks), name, cmd))
>> -    return [(k, v) for p, o, k, v in sorted(hooks)]
>> +            hooks[name] = (-priority, len(hooks), cmd)
>> +    order = sorted(hooks, key=lambda x: hooks[x][:2] + (x,))
>> +    return [(k, hooks[k][2]) for k in order]
>
> I'd find this easier to review as:
>
>     hooks[name] = (-priority, len(hooks), name, cmd)
> return [(k, v) for p, o, k, v in sorted(hooks.values())]
>
> with similar changes to the other two patches. That way, it's obvious 
> that the behaviour isn't changing until patch 3, and there's less 
> counting of elements to do.

Yeah, I agree. I'll send a V2.

Patch

diff -r 8d398155bfda -r 063993ec1c63 mercurial/hook.py
--- a/mercurial/hook.py	Mon Feb 29 22:58:15 2016 +0900
+++ b/mercurial/hook.py	Thu Apr 14 02:28:46 2016 -0700
@@ -162,12 +162,13 @@ 
     return r
 
 def _allhooks(ui):
-    hooks = []
+    hooks = {}
     for name, cmd in ui.configitems('hooks'):
         if not name.startswith('priority'):
             priority = ui.configint('hooks', 'priority.%s' % name, 0)
-            hooks.append((-priority, len(hooks), name, cmd))
-    return [(k, v) for p, o, k, v in sorted(hooks)]
+            hooks[name] = (-priority, len(hooks), cmd)
+    order = sorted(hooks, key=lambda x: hooks[x][:2] + (x,))
+    return [(k, hooks[k][2]) for k in order]
 
 _redirect = False
 def redirect(state):