Patchwork [3,of,3] chgserver: print traceback if --traceback flag is passed to validate

login
register
mail settings
Submitter Yuya Nishihara
Date March 21, 2016, 1:27 a.m.
Message ID <a63445cc0d56f1af9e29.1458523621@mimosa>
Download mbox | patch
Permalink /patch/14007/
State Changes Requested
Headers show

Comments

Yuya Nishihara - March 21, 2016, 1:27 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1458516023 25200
#      Sun Mar 20 16:20:23 2016 -0700
# Node ID a63445cc0d56f1af9e29a1eb3afa75623330f709
# Parent  306bcd1d5f7655ea1ab3f1bc87a48ce9d7a6c150
chgserver: print traceback if --traceback flag is passed to validate

This makes test-globalopts.t pass with chg. We cannot trust ui.tracebackflag
at this point because we haven't run dispatch() with the new args yet and
ui.tracebackflag may be set globally by "serve --cmdserver chgunix --traceback".
Sean Farley - March 21, 2016, 5:50 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1458516023 25200
> #      Sun Mar 20 16:20:23 2016 -0700
> # Node ID a63445cc0d56f1af9e29a1eb3afa75623330f709
> # Parent  306bcd1d5f7655ea1ab3f1bc87a48ce9d7a6c150
> chgserver: print traceback if --traceback flag is passed to validate
>
> This makes test-globalopts.t pass with chg. We cannot trust ui.tracebackflag
> at this point because we haven't run dispatch() with the new args yet and
> ui.tracebackflag may be set globally by "serve --cmdserver chgunix --traceback".

These patches look good to me but see below.

> diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> --- a/hgext/chgserver.py
> +++ b/hgext/chgserver.py
> @@ -448,7 +448,14 @@ class chgcmdserver(commandserver.server)
>          """
>          args = self._readlist()
>          try:
> -            self.ui, lui = _loadnewui(self.ui, args)
> +            try:
> +                self.ui, lui = _loadnewui(self.ui, args)
> +            except: # re-raises
> +                # simulate early traceback output in dispatch.dispatch(),
> +                # where args aren't fully parsed
> +                if '--traceback' in args:
> +                    self.ui.traceback(force=True)
> +                raise

This seems like a bit of a hack. Is there no better way to do it? Or is
it something that will be refactored later?
Jun Wu - March 21, 2016, 6:32 p.m.
On 03/20/2016 06:27 PM, Yuya Nishihara wrote:
>           try:
> -            self.ui, lui = _loadnewui(self.ui, args)
> +            try:
> +                self.ui, lui = _loadnewui(self.ui, args)

The nested try catch is not necessary, since dispatch.py won't print traceback
even with --traceback when it encounters ParseError.

Other than that, the series look good to me.

smf: My ui refactoring plan includes replacing "debugflag" etc. with @property
and killing fixconfig(), which may make this cleaner. But the refactoring won't
happen soon.
Sean Farley - March 21, 2016, 6:52 p.m.
Jun Wu <quark@fb.com> writes:

> On 03/20/2016 06:27 PM, Yuya Nishihara wrote:
>>           try:
>> -            self.ui, lui = _loadnewui(self.ui, args)
>> +            try:
>> +                self.ui, lui = _loadnewui(self.ui, args)
>
> The nested try catch is not necessary, since dispatch.py won't print traceback
> even with --traceback when it encounters ParseError.
>
> Other than that, the series look good to me.
>
> smf: My ui refactoring plan includes replacing "debugflag" etc. with @property
> and killing fixconfig(), which may make this cleaner. But the refactoring won't
> happen soon.

Ok, I see. This seems good to me, then.
Yuya Nishihara - March 22, 2016, 3:31 p.m.
On Mon, 21 Mar 2016 10:50:36 -0700, Sean Farley wrote:
> > diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> > --- a/hgext/chgserver.py
> > +++ b/hgext/chgserver.py
> > @@ -448,7 +448,14 @@ class chgcmdserver(commandserver.server)
> >          """
> >          args = self._readlist()
> >          try:
> > -            self.ui, lui = _loadnewui(self.ui, args)
> > +            try:
> > +                self.ui, lui = _loadnewui(self.ui, args)
> > +            except: # re-raises
> > +                # simulate early traceback output in dispatch.dispatch(),
> > +                # where args aren't fully parsed
> > +                if '--traceback' in args:
> > +                    self.ui.traceback(force=True)
> > +                raise
> 
> This seems like a bit of a hack. Is there no better way to do it? Or is
> it something that will be refactored later?

It is a hack of the same weirdness as dispatch.dispatch(). There's a
chicken-and-egg issue that is the option parser needs a ui but creating a ui
can raise an exception.
Yuya Nishihara - March 22, 2016, 3:42 p.m.
On Mon, 21 Mar 2016 11:32:59 -0700, Jun Wu wrote:
> On 03/20/2016 06:27 PM, Yuya Nishihara wrote:
> >           try:
> > -            self.ui, lui = _loadnewui(self.ui, args)
> > +            try:
> > +                self.ui, lui = _loadnewui(self.ui, args)
> 
> The nested try catch is not necessary, since dispatch.py won't print traceback
> even with --traceback when it encounters ParseError.

It sounds like a bug (or a misfeature) of dispatch(). That said, I need to
rethink about the patch 3, so please disregard it. The other patches can be
applied without it.

junw: there's a bug. --traceback flag persists because it isn't unset by
dispatch.

  start server with --traceback
  $ chg verify --traceback
  run command without --traceback
  $ chg verify
  ^C
  Traceback (most recent call last):
    ...
Jun Wu - March 22, 2016, 3:54 p.m.
On 03/22/2016 08:42 AM, Yuya Nishihara wrote:
> junw: there's a bug. --traceback flag persists because it isn't unset by
> dispatch.

I think it's related to fixconfig(). I have a plan to replace ui.debugflag,
ui.verbose, ui.quiet, ui..tracebackflag with @property. And eventually kill
ui.fixconfig().
Yuya Nishihara - March 23, 2016, 2:47 p.m.
On Tue, 22 Mar 2016 08:54:49 -0700, Jun Wu wrote:
> On 03/22/2016 08:42 AM, Yuya Nishihara wrote:
> > junw: there's a bug. --traceback flag persists because it isn't unset by
> > dispatch.
> 
> I think it's related to fixconfig(). I have a plan to replace ui.debugflag,
> ui.verbose, ui.quiet, ui..tracebackflag with @property. And eventually kill
> ui.fixconfig().

Yeah, that will solve the issue, but I think we'll need a temporary fix.
Because chg.c sends --traceback to the main server, it is different from the
other verbosity flags.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -448,7 +448,14 @@  class chgcmdserver(commandserver.server)
         """
         args = self._readlist()
         try:
-            self.ui, lui = _loadnewui(self.ui, args)
+            try:
+                self.ui, lui = _loadnewui(self.ui, args)
+            except: # re-raises
+                # simulate early traceback output in dispatch.dispatch(),
+                # where args aren't fully parsed
+                if '--traceback' in args:
+                    self.ui.traceback(force=True)
+                raise
         except error.ParseError as inst:
             dispatch._formatparse(self.ui.warn, inst)
             self.ui.flush()