[PATCH] chgserver: wrap ui without calling its constructor

Jun Wu quark at fb.com
Sun Mar 13 07:11:07 EDT 2016


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.

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.

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.

So I still want this in for now. It's much easier than fixing "self.ui = ui"s 
one by one or do the huge ui refactoring. 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.


More information about the Mercurial-devel mailing list