[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