[PATCH] chgserver: wrap ui without calling its constructor

Yuya Nishihara yuya at tcha.org
Sat Mar 12 00:04:15 EST 2016


On Fri, 11 Mar 2016 22:19:29 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark at fb.com>
> # Date 1457734734 0
> #      Fri Mar 11 22:18:54 2016 +0000
> # Node ID 3bd824c35cd84e5259e65be611a40ad2f8080922
> # Parent  e94645d05c6a53f08fe81d5209f17d2209894a6f
> chgserver: wrap ui without calling its constructor
> 
> Calling __init__ could be dangerous if another extension overrides it has
> a bug. For example:
> 
>   Traceback (most recent call last):
>     ...
>     File "$HGREPO/hgext/chgserver.py", line 535, in handle
>       self.server.hashstate, self.server.baseaddress)
>     File "$HGREPO/hgext/chgserver.py", line 346, in __init__
>       _newchgui(ui, channeledsystem(fin, fout, 'S')), repo, fin, fout)
>     File "$HGREPO/hgext/chgserver.py", line 263, in _newchgui
>       return chgui(srcui)
>     File "$HGREPO/hgext/chgserver.py", line 234, in __init__
>       super(chgui, self).__init__(src)
>     File "$HGREPO/hgext/blackbox.py", line 86, in __init__
>       self._bbvfs = src._bbvfs
>   AttributeError: 'blackboxui' object has no attribute '_bbvfs'
> 
> It's pretty bad since the user will get a chg server accepting connections
> without being able to serve them. The server cannot even tell the user the
> error message since attachio is not executed at this point.
> 
> This patch solves the issue by only replacing the system method without
> constructing a new ui object.

I really don't like this sort of hacks that just hide bugs. If it is a bug
of blackboxui, it must be fixed in blackbox.

> -def _newchgui(srcui, csystem):
> +def _wrapchgui(srcui, defaultcsystem):
>      class chgui(srcui.__class__):
> -        def __init__(self, src=None):
> -            super(chgui, self).__init__(src)
> -            if src:
> -                self._csystem = getattr(src, '_csystem', csystem)
> -            else:
> -                self._csystem = csystem
> -
>          def system(self, cmd, environ=None, cwd=None, onerr=None,
>                     errprefix=None):
>              # copied from mercurial/util.py:system()
> @@ -246,7 +239,11 @@
>              if environ:
>                  env.update((k, py2shell(v)) for k, v in environ.iteritems())
>              env['HG'] = util.hgexecutable()
> -            rc = self._csystem(cmd, env, cwd)
> +            if util.safehasattr(self, '_csystem'):
> +                csystem = self._csystem
> +            else:
> +                csystem = defaultcsystem

It complicates the scope of csystem variable more. I'd rather remove csystem
from __init__.


More information about the Mercurial-devel mailing list