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

Brodie Rao brodie at sf.io
Tue Feb 11 14:08:10 CST 2014


On Mon, Feb 10, 2014 at 3:51 PM, Simon Heimberg <Simohe at besonet.ch> wrote:
> Am 11.02.2014 00:00, schrieb Brodie Rao:
>
>> # HG changeset patch
>> # User Brodie Rao <brodie at 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:]:
>>
>


More information about the Mercurial-devel mailing list