[PATCH] chgserver: wrap ui without calling its constructor

Yuya Nishihara yuya at tcha.org
Sun Mar 13 08:12:04 EDT 2016


On Sun, 13 Mar 2016 11:11:07 +0000, Jun Wu wrote:
> On 03/13/2016 04:21 AM, Yuya Nishihara wrote:
> > Well, I designed it to create a new ui so that chgcmdserver can be
> > instantiated without forking. If the server isn't fork-per-request
> 
> But fork won't go away soon right? I think it cannot happen before some ui
> object refactoring, which is not happening soon either.

I don't think ui is the blocker. Instead, we'll have to eliminate globals that
are modified while running command, which is also hard and won't happen soon.

> There are 45 "self.ui = ui" and it's causing trouble because they may keep
> outdated ui objects. By trouble I mean they result in hard-to-notice and
> hard-to-debug bugs. Some are exposed in tests. But I think most of them
> are invisible and will just surprise people at some time.
> 
> For example, I had spent a lot of time on test-progress.t, ended up with
> printing ids and stacktraces with every ui creation. It turns out the
> "ui.copy()" in runcommand of commandsesrver.py causes inconsistency with the
> old ui object kept in progress.py by "self.ui = ui".
> 
> That's why I want to get rid of "ui.{copy,__init__}" very strongly.

Hmm, but you can't eliminate all of these copies, right? What's the benefit to
skip only one copy at chgcmdserver.__init__? It will soon be copied at
runcommand() anyway.

> The possible bug hidden by this chgui is not easily exposed because extensions 
> do not use "system" often for interactive commands. But I do believe it will
> hide bugs because of the 45 "self.ui = ui"s.

In order to expose such bugs, I'd rather make chgui.system() do crash if it
isn't initialized appropriately.

> It's also easy to be reverted once we
> are ready to get rid of fork. I will be happy to add detailed comments about
> the story if we decide to accept the patch.

Well, if you want to change _newchgui() to _wrapchgui(), move _wrapchgui() to
uisetup(). That will make sure the class wrapping is processed only once.


More information about the Mercurial-devel mailing list