Patchwork [v2] dispatch: refactor tortured debugger setup logic

login
register
mail settings
Submitter Bryan O'Sullivan
Date Jan. 1, 2016, 10:46 p.m.
Message ID <f019056c88a34d9cf9d0.1451688383@bryano-mbp.netgear.com>
Download mbox | patch
Permalink /patch/12469/
State Changes Requested
Headers show

Comments

Bryan O'Sullivan - Jan. 1, 2016, 10:46 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bos@serpentine.com>
# Date 1451688323 28800
#      Fri Jan 01 14:45:23 2016 -0800
# Node ID f019056c88a34d9cf9d01e10f1354e049ce07521
# Parent  44f717c879033f28d3f7e7dc9f34aa394d2fea3f
dispatch: refactor tortured debugger setup logic

This patch was triggered by observing that the pdb debugger was
always loaded regardless of whether a debugger was to be used.

The logic as it existed was a thicket of accidental complexity.
It's now much easier to follow (and only loads a debugger if needed).
Sean Farley - Jan. 1, 2016, 10:58 p.m.
Bryan O'Sullivan <bos@serpentine.com> writes:

> # HG changeset patch
> # User Bryan O'Sullivan <bos@serpentine.com>
> # Date 1451688323 28800
> #      Fri Jan 01 14:45:23 2016 -0800
> # Node ID f019056c88a34d9cf9d01e10f1354e049ce07521
> # Parent  44f717c879033f28d3f7e7dc9f34aa394d2fea3f
> dispatch: refactor tortured debugger setup logic
>
> This patch was triggered by observing that the pdb debugger was
> always loaded regardless of whether a debugger was to be used.
>
> The logic as it existed was a thicket of accidental complexity.
> It's now much easier to follow (and only loads a debugger if needed).

Long ago, when I wrote some of this code, I wanted the ability to throw
in a:

import ipdb
ipdb.set_trace()

And have that throw me into a debugger. My reasoning was that I didn't
always control the command-line args (such as some test scripts
running).

Anyway, it that seems broken nowadays because of the absolute import
stuff from Greg, so I don't know if what I was trying to achieve is even
worth it now.
Bryan O'Sullivan - Jan. 2, 2016, 12:55 a.m.
On Fri, Jan 1, 2016 at 2:58 PM, Sean Farley <sean@farley.io> wrote:

> Long ago, when I wrote some of this code, I wanted the ability to throw
> in a:
>
> import ipdb
> ipdb.set_trace()
>
> And have that throw me into a debugger. My reasoning was that I didn't
> always control the command-line args (such as some test scripts
> running).
>

That's still quite easy to do. Under this patch, you'd just change the
assignment of debugger to 'ipdb' and make the "if" immediately above it
unconditionally true.
Sean Farley - Jan. 2, 2016, 1:09 a.m.
Bryan O'Sullivan <bos@serpentine.com> writes:

> On Fri, Jan 1, 2016 at 2:58 PM, Sean Farley <sean@farley.io> wrote:
>
>> Long ago, when I wrote some of this code, I wanted the ability to throw
>> in a:
>>
>> import ipdb
>> ipdb.set_trace()
>>
>> And have that throw me into a debugger. My reasoning was that I didn't
>> always control the command-line args (such as some test scripts
>> running).
>>
>
> That's still quite easy to do. Under this patch, you'd just change the
> assignment of debugger to 'ipdb' and make the "if" immediately above it
> unconditionally true.

Yay :-D
Yuya Nishihara - Jan. 5, 2016, 1:53 p.m.
On Fri, 01 Jan 2016 14:46:23 -0800, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bos@serpentine.com>
> # Date 1451688323 28800
> #      Fri Jan 01 14:45:23 2016 -0800
> # Node ID f019056c88a34d9cf9d01e10f1354e049ce07521
> # Parent  44f717c879033f28d3f7e7dc9f34aa394d2fea3f
> dispatch: refactor tortured debugger setup logic
> 
> This patch was triggered by observing that the pdb debugger was
> always loaded regardless of whether a debugger was to be used.
> 
> The logic as it existed was a thicket of accidental complexity.
> It's now much easier to follow (and only loads a debugger if needed).
> 
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -137,14 +137,6 @@ def _runcatch(req):
>  
>      try:
>          try:
> -            debugger = 'pdb'
> -            debugtrace = {
> -                'pdb' : pdb.set_trace
> -            }
> -            debugmortem = {
> -                'pdb' : pdb.post_mortem
> -            }
> -
>              # read --config before doing anything else
>              # (e.g. to change trust settings for reading .hg/hgrc)
>              cfgs = _parseconfig(req.ui, _earlygetopt(['--config'], req.args))
> @@ -155,13 +147,13 @@ def _runcatch(req):
>                  for sec, name, val in cfgs:
>                      req.repo.ui.setconfig(sec, name, val, source='--config')
>  
> -            # developer config: ui.debugger
> -            debugger = ui.config("ui", "debugger")
> -            debugmod = pdb
> -            if not debugger or ui.plain():
> -                # if we are in HGPLAIN mode, then disable custom debugging
> +            if '--debugger' in req.args:
> +                # if we are in HGPLAIN mode, ignore custom debugging
>                  debugger = 'pdb'
> -            elif '--debugger' in req.args:
> +                if not ui.plain():
> +                    # developer config: ui.debugger
> +                    debugger = ui.config('ui', 'debugger', default='pdb')

"--config ui.debugger=" will raise ValueError: Empty module name.

> +
>                  # This import can be slow for fancy debuggers, so only
>                  # do it when absolutely necessary, i.e. when actual
>                  # debugging has been requested
> @@ -169,22 +161,15 @@ def _runcatch(req):
>                      try:
>                          debugmod = __import__(debugger)
>                      except ImportError:
> -                        pass # Leave debugmod = pdb
> +                        ui.warn(_("%s debugger specified "
> +                                  "but its module was not found\n") % debugger)
> +                        debugmod = pdb
>  
> -            debugtrace[debugger] = debugmod.set_trace
> -            debugmortem[debugger] = debugmod.post_mortem
> +                    # enter the debugger before command execution
> +                    ui.warn(_("entering debugger - "
> +                              "type c to continue starting hg or h for help\n"))
>  
> -            # enter the debugger before command execution
> -            if '--debugger' in req.args:
> -                ui.warn(_("entering debugger - "
> -                        "type c to continue starting hg or h for help\n"))
> -
> -                if (debugger != 'pdb' and
> -                    debugtrace[debugger] == debugtrace['pdb']):
> -                    ui.warn(_("%s debugger specified "
> -                              "but its module was not found\n") % debugger)
> -                with demandimport.deactivated():
> -                    debugtrace[debugger]()
> +                    debugmod.set_trace()
>              try:
>                  return _dispatch(req)
>              finally:
> @@ -193,7 +178,7 @@ def _runcatch(req):
>              # enter the debugger when we hit an exception
>              if '--debugger' in req.args:
>                  traceback.print_exc()
> -                debugmortem[debugger](sys.exc_info()[2])
> +                debugmod.post_mortem(sys.exc_info()[2])

And debugmod could be unbound.

% hg --debugger --config foo=bar=baz
Traceback (most recent call last):
  File "mercurial/dispatch.py", line 142, in _runcatch
    cfgs = _parseconfig(req.ui, _earlygetopt(['--config'], req.args))
  File "mercurial/dispatch.py", line 597, in _parseconfig
    '(use --config section.name=value)') % cfg)
Abort: malformed --config option: 'foo=bar=baz' (use --config section.name=value)
** unknown exception encountered, please report by visiting
** https://mercurial-scm.org/wiki/BugTracker
** Python 2.7.11 (default, Dec  9 2015, 00:29:25) [GCC 5.3.1 20151205]
** Mercurial Distributed SCM (version 3.6.3+723-1d7e824ad093)
** Extensions loaded: 
Traceback (most recent call last):
  File "./hg", line 43, in <module>
    mercurial.dispatch.run()
  File "mercurial/dispatch.py", line 54, in run
    sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
  File "mercurial/dispatch.py", line 118, in dispatch
    ret = _runcatch(req)
  File "mercurial/dispatch.py", line 181, in _runcatch
    debugmod.post_mortem(sys.exc_info()[2])
UnboundLocalError: local variable 'debugmod' referenced before assignment
Bryan O'Sullivan - Jan. 5, 2016, 7:25 p.m.
On Tue, Jan 5, 2016 at 5:53 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> "--config ui.debugger=" will raise ValueError: Empty module name.
>

I don't think we should care about that.


> And debugmod could be unbound.
>

This I just fixed, thanks.
Bryan O'Sullivan - Jan. 5, 2016, 7:30 p.m.
On Tue, Jan 5, 2016 at 11:25 AM, Bryan O'Sullivan <bos@serpentine.com>
wrote:

>
> On Tue, Jan 5, 2016 at 5:53 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>
>> "--config ui.debugger=" will raise ValueError: Empty module name.
>>
>
> I don't think we should care about that.
>

Never mind, I fixed this too.

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -137,14 +137,6 @@  def _runcatch(req):
 
     try:
         try:
-            debugger = 'pdb'
-            debugtrace = {
-                'pdb' : pdb.set_trace
-            }
-            debugmortem = {
-                'pdb' : pdb.post_mortem
-            }
-
             # read --config before doing anything else
             # (e.g. to change trust settings for reading .hg/hgrc)
             cfgs = _parseconfig(req.ui, _earlygetopt(['--config'], req.args))
@@ -155,13 +147,13 @@  def _runcatch(req):
                 for sec, name, val in cfgs:
                     req.repo.ui.setconfig(sec, name, val, source='--config')
 
-            # developer config: ui.debugger
-            debugger = ui.config("ui", "debugger")
-            debugmod = pdb
-            if not debugger or ui.plain():
-                # if we are in HGPLAIN mode, then disable custom debugging
+            if '--debugger' in req.args:
+                # if we are in HGPLAIN mode, ignore custom debugging
                 debugger = 'pdb'
-            elif '--debugger' in req.args:
+                if not ui.plain():
+                    # developer config: ui.debugger
+                    debugger = ui.config('ui', 'debugger', default='pdb')
+
                 # This import can be slow for fancy debuggers, so only
                 # do it when absolutely necessary, i.e. when actual
                 # debugging has been requested
@@ -169,22 +161,15 @@  def _runcatch(req):
                     try:
                         debugmod = __import__(debugger)
                     except ImportError:
-                        pass # Leave debugmod = pdb
+                        ui.warn(_("%s debugger specified "
+                                  "but its module was not found\n") % debugger)
+                        debugmod = pdb
 
-            debugtrace[debugger] = debugmod.set_trace
-            debugmortem[debugger] = debugmod.post_mortem
+                    # enter the debugger before command execution
+                    ui.warn(_("entering debugger - "
+                              "type c to continue starting hg or h for help\n"))
 
-            # enter the debugger before command execution
-            if '--debugger' in req.args:
-                ui.warn(_("entering debugger - "
-                        "type c to continue starting hg or h for help\n"))
-
-                if (debugger != 'pdb' and
-                    debugtrace[debugger] == debugtrace['pdb']):
-                    ui.warn(_("%s debugger specified "
-                              "but its module was not found\n") % debugger)
-                with demandimport.deactivated():
-                    debugtrace[debugger]()
+                    debugmod.set_trace()
             try:
                 return _dispatch(req)
             finally:
@@ -193,7 +178,7 @@  def _runcatch(req):
             # enter the debugger when we hit an exception
             if '--debugger' in req.args:
                 traceback.print_exc()
-                debugmortem[debugger](sys.exc_info()[2])
+                debugmod.post_mortem(sys.exc_info()[2])
             ui.traceback()
             raise