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
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:]: >
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:]: >> >
--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:]: