Patchwork [stable] hooks: only disable/re-enable demandimport when it's already enabled

login
register
mail settings
Submitter Brodie Rao
Date Feb. 10, 2014, 11 p.m.
Message ID <aac87f70f38e1561c980.1392073237@kapp.staff.sf.atlassian.com>
Download mbox | patch
Permalink /patch/3545/
State Accepted
Commit aac87f70f38e1561c98057fc845f333b185b6d5d
Headers show

Comments

Brodie Rao - Feb. 10, 2014, 11 p.m.
# HG changeset patch
# User Brodie Rao <brodie@sf.io>
# Date 1392072666 28800
#      Mon Feb 10 14:51:06 2014 -0800
# Branch stable
# Node ID aac87f70f38e1561c98057fc845f333b185b6d5d
# Parent  e4d7cbc94219e54f5e73df9c2f88eca3d46d7f90
hooks: only disable/re-enable demandimport when it's already enabled

This fixes an issue introduced in d7c28954d901 where, when disabling
demandimport while running hooks, it's inadvertently re-enabled even when
it was never enabled in the first place.

This doesn't affect normal command line usage of Mercurial; it only matters
when Mercurial is run with demandimport intentionally disabled.
Simon Heimberg - Feb. 10, 2014, 11:51 p.m.
Am 11.02.2014 00:00, schrieb Brodie Rao:
> # HG changeset patch
> # User Brodie Rao <brodie@sf.io>
> # Date 1392072666 28800
> #      Mon Feb 10 14:51:06 2014 -0800
> # Branch stable
> # Node ID aac87f70f38e1561c98057fc845f333b185b6d5d
> # Parent  e4d7cbc94219e54f5e73df9c2f88eca3d46d7f90
> hooks: only disable/re-enable demandimport when it's already enabled
>
> This fixes an issue introduced in d7c28954d901 where, when disabling
> demandimport while running hooks, it's inadvertently re-enabled even when
> it was never enabled in the first place.
>
> This doesn't affect normal command line usage of Mercurial; it only matters
> when Mercurial is run with demandimport intentionally disabled.
>
> diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
> --- a/mercurial/demandimport.py
> +++ b/mercurial/demandimport.py
> @@ -162,6 +162,9 @@ ignore = [
>       'mimetools',
>       ]
>
> +def isenabled():
> +    return __builtin__.__import__ == _demandimport
> +
>   def enable():
>       "enable global demand-loading of modules"
>       __builtin__.__import__ = _demandimport
> diff --git a/mercurial/hook.py b/mercurial/hook.py
> --- a/mercurial/hook.py
> +++ b/mercurial/hook.py
> @@ -36,28 +36,33 @@ def _pythonhook(ui, repo, name, hname, f
>               if modpath and modfile:
>                   sys.path = sys.path[:] + [modpath]
>                   modname = modfile
> +        demandimportenabled = demandimport.isenabled()
> +        if demandimportenabled:
> +            demandimport.disable()
>           try:
> -            demandimport.disable()
> -            obj = __import__(modname)

Why did we disable demandimport here? __import__ is never delayed. (See 
comment at start of mercurial/demandimport.py) Let's only remove disable 
and enable.

> -            demandimport.enable()
> -        except ImportError:
> -            e1 = sys.exc_type, sys.exc_value, sys.exc_traceback
>               try:
> -                # extensions are loaded with hgext_ prefix
> -                obj = __import__("hgext_%s" % modname)
> +                obj = __import__(modname)
> +            except ImportError:
> +                e1 = sys.exc_type, sys.exc_value, sys.exc_traceback
> +                try:
> +                    # extensions are loaded with hgext_ prefix
> +                    obj = __import__("hgext_%s" % modname)
> +                except ImportError:
> +                    e2 = sys.exc_type, sys.exc_value, sys.exc_traceback
> +                    if ui.tracebackflag:
> +                        ui.warn(_('exception from first failed import '
> +                                  'attempt:\n'))
> +                    ui.traceback(e1)
> +                    if ui.tracebackflag:
> +                        ui.warn(_('exception from second failed import '
> +                                  'attempt:\n'))
> +                    ui.traceback(e2)
> +                    raise util.Abort(_('%s hook is invalid '
> +                                       '(import of "%s" failed)') %
> +                                     (hname, modname))
> +        finally:
> +            if demandimportenabled:
>                   demandimport.enable()
> -            except ImportError:
> -                demandimport.enable()
> -                e2 = sys.exc_type, sys.exc_value, sys.exc_traceback
> -                if ui.tracebackflag:
> -                    ui.warn(_('exception from first failed import attempt:\n'))
> -                ui.traceback(e1)
> -                if ui.tracebackflag:
> -                    ui.warn(_('exception from second failed import attempt:\n'))
> -                ui.traceback(e2)
> -                raise util.Abort(_('%s hook is invalid '
> -                                   '(import of "%s" failed)') %
> -                                 (hname, modname))
>           sys.path = oldpaths
>           try:
>               for p in funcname.split('.')[1:]:
>
Brodie Rao - Feb. 11, 2014, 8:08 p.m.
On Mon, Feb 10, 2014 at 3:51 PM, Simon Heimberg <Simohe@besonet.ch> wrote:
> Am 11.02.2014 00:00, schrieb Brodie Rao:
>
>> # HG changeset patch
>> # User Brodie Rao <brodie@sf.io>
>> # Date 1392072666 28800
>> #      Mon Feb 10 14:51:06 2014 -0800
>> # Branch stable
>> # Node ID aac87f70f38e1561c98057fc845f333b185b6d5d
>> # Parent  e4d7cbc94219e54f5e73df9c2f88eca3d46d7f90
>> hooks: only disable/re-enable demandimport when it's already enabled
>>
>> This fixes an issue introduced in d7c28954d901 where, when disabling
>> demandimport while running hooks, it's inadvertently re-enabled even when
>> it was never enabled in the first place.
>>
>> This doesn't affect normal command line usage of Mercurial; it only
>> matters
>> when Mercurial is run with demandimport intentionally disabled.
>>
>> diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
>> --- a/mercurial/demandimport.py
>> +++ b/mercurial/demandimport.py
>> @@ -162,6 +162,9 @@ ignore = [
>>       'mimetools',
>>       ]
>>
>> +def isenabled():
>> +    return __builtin__.__import__ == _demandimport
>> +
>>   def enable():
>>       "enable global demand-loading of modules"
>>       __builtin__.__import__ = _demandimport
>> diff --git a/mercurial/hook.py b/mercurial/hook.py
>> --- a/mercurial/hook.py
>> +++ b/mercurial/hook.py
>> @@ -36,28 +36,33 @@ def _pythonhook(ui, repo, name, hname, f
>>               if modpath and modfile:
>>                   sys.path = sys.path[:] + [modpath]
>>                   modname = modfile
>> +        demandimportenabled = demandimport.isenabled()
>> +        if demandimportenabled:
>> +            demandimport.disable()
>>           try:
>> -            demandimport.disable()
>> -            obj = __import__(modname)
>
>
> Why did we disable demandimport here? __import__ is never delayed. (See
> comment at start of mercurial/demandimport.py) Let's only remove disable and
> enable.

I believe that change was made to disable demandimport for import
statements made inside the extension. Using __import__() here only
bypasses demandimport when importing the extension itself;
demandimport will still be enabled for the extension's own imports.

So I don't think removing disable/enable is the right thing to do. I
think my patch as it stands is OK.

>
>
>> -            demandimport.enable()
>> -        except ImportError:
>> -            e1 = sys.exc_type, sys.exc_value, sys.exc_traceback
>>               try:
>> -                # extensions are loaded with hgext_ prefix
>> -                obj = __import__("hgext_%s" % modname)
>> +                obj = __import__(modname)
>> +            except ImportError:
>> +                e1 = sys.exc_type, sys.exc_value, sys.exc_traceback
>> +                try:
>> +                    # extensions are loaded with hgext_ prefix
>> +                    obj = __import__("hgext_%s" % modname)
>> +                except ImportError:
>> +                    e2 = sys.exc_type, sys.exc_value, sys.exc_traceback
>> +                    if ui.tracebackflag:
>> +                        ui.warn(_('exception from first failed import '
>> +                                  'attempt:\n'))
>> +                    ui.traceback(e1)
>> +                    if ui.tracebackflag:
>> +                        ui.warn(_('exception from second failed import '
>> +                                  'attempt:\n'))
>> +                    ui.traceback(e2)
>> +                    raise util.Abort(_('%s hook is invalid '
>> +                                       '(import of "%s" failed)') %
>> +                                     (hname, modname))
>> +        finally:
>> +            if demandimportenabled:
>>                   demandimport.enable()
>> -            except ImportError:
>> -                demandimport.enable()
>> -                e2 = sys.exc_type, sys.exc_value, sys.exc_traceback
>> -                if ui.tracebackflag:
>> -                    ui.warn(_('exception from first failed import
>> attempt:\n'))
>> -                ui.traceback(e1)
>> -                if ui.tracebackflag:
>> -                    ui.warn(_('exception from second failed import
>> attempt:\n'))
>> -                ui.traceback(e2)
>> -                raise util.Abort(_('%s hook is invalid '
>> -                                   '(import of "%s" failed)') %
>> -                                 (hname, modname))
>>           sys.path = oldpaths
>>           try:
>>               for p in funcname.split('.')[1:]:
>>
>
Simon Heimberg - Feb. 11, 2014, 8:33 p.m.
--On 2014-02-11 12:08 -0800 Brodie Rao <brodie@sf.io> wrote:
> On Mon, Feb 10, 2014 at 3:51 PM, Simon Heimberg <Simohe@besonet.ch> wrote:
>> Am 11.02.2014 00:00, schrieb Brodie Rao:
>>
>>> # HG changeset patch
>>> # User Brodie Rao <brodie@sf.io>
>>> # Date 1392072666 28800
>>> #      Mon Feb 10 14:51:06 2014 -0800
>>> # Branch stable
>>> # Node ID aac87f70f38e1561c98057fc845f333b185b6d5d
>>> # Parent  e4d7cbc94219e54f5e73df9c2f88eca3d46d7f90
>>> hooks: only disable/re-enable demandimport when it's already enabled
>>>
>>> diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
<snip>
>>> -            obj = __import__(modname)
>>
>>
>> Why did we disable demandimport here? __import__ is never delayed. (See
>> comment at start of mercurial/demandimport.py) Let's only remove disable
>> and enable.
>
> I believe that change was made to disable demandimport for import
> statements made inside the extension. Using __import__() here only
> bypasses demandimport when importing the extension itself;
> demandimport will still be enabled for the extension's own imports.
>
> So I don't think removing disable/enable is the right thing to do. I
> think my patch as it stands is OK.
>

Absolutely right. Enabling was introduced in d7c28954d901, the commit 
message tells exactly this. Maybe you should add a comment to prevent me 
from removing it.  (-:
The patch is already pushed to stable, so it looked good for somebody 
responsible, and you convinced me too.

Greetings,
Simon

>>
>>
>>> -            demandimport.enable()
>>> -        except ImportError:
<snip>

Patch

diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
--- a/mercurial/demandimport.py
+++ b/mercurial/demandimport.py
@@ -162,6 +162,9 @@  ignore = [
     'mimetools',
     ]
 
+def isenabled():
+    return __builtin__.__import__ == _demandimport
+
 def enable():
     "enable global demand-loading of modules"
     __builtin__.__import__ = _demandimport
diff --git a/mercurial/hook.py b/mercurial/hook.py
--- a/mercurial/hook.py
+++ b/mercurial/hook.py
@@ -36,28 +36,33 @@  def _pythonhook(ui, repo, name, hname, f
             if modpath and modfile:
                 sys.path = sys.path[:] + [modpath]
                 modname = modfile
+        demandimportenabled = demandimport.isenabled()
+        if demandimportenabled:
+            demandimport.disable()
         try:
-            demandimport.disable()
-            obj = __import__(modname)
-            demandimport.enable()
-        except ImportError:
-            e1 = sys.exc_type, sys.exc_value, sys.exc_traceback
             try:
-                # extensions are loaded with hgext_ prefix
-                obj = __import__("hgext_%s" % modname)
+                obj = __import__(modname)
+            except ImportError:
+                e1 = sys.exc_type, sys.exc_value, sys.exc_traceback
+                try:
+                    # extensions are loaded with hgext_ prefix
+                    obj = __import__("hgext_%s" % modname)
+                except ImportError:
+                    e2 = sys.exc_type, sys.exc_value, sys.exc_traceback
+                    if ui.tracebackflag:
+                        ui.warn(_('exception from first failed import '
+                                  'attempt:\n'))
+                    ui.traceback(e1)
+                    if ui.tracebackflag:
+                        ui.warn(_('exception from second failed import '
+                                  'attempt:\n'))
+                    ui.traceback(e2)
+                    raise util.Abort(_('%s hook is invalid '
+                                       '(import of "%s" failed)') %
+                                     (hname, modname))
+        finally:
+            if demandimportenabled:
                 demandimport.enable()
-            except ImportError:
-                demandimport.enable()
-                e2 = sys.exc_type, sys.exc_value, sys.exc_traceback
-                if ui.tracebackflag:
-                    ui.warn(_('exception from first failed import attempt:\n'))
-                ui.traceback(e1)
-                if ui.tracebackflag:
-                    ui.warn(_('exception from second failed import attempt:\n'))
-                ui.traceback(e2)
-                raise util.Abort(_('%s hook is invalid '
-                                   '(import of "%s" failed)') %
-                                 (hname, modname))
         sys.path = oldpaths
         try:
             for p in funcname.split('.')[1:]: