[PATCH 08 of 10] hg: when testing, warn if repo.ui is passed to a new peer or localrepo

[Simon Heimberg]

Code freeze has passed a long time ago, so let's finally continue this
discussion.

Your proposed solution looks better and much simpler than mine.

But I have some concerns about the handling in commandserver. (see
below)

On Thu, 2013-04-18 at 00:36 -0500, Matt Mackall wrote: 
> On Fri, 2013-03-22 at 02:20 +0100, Simon Heimberg wrote:
> > # HG changeset patch
> > # User Simon Heimberg <simohe at besonet.ch>
> > # Date 1349372803 -7200
> > # Node ID bc3341ad23a61fa0ad272be98cdc00cf11d4b8eb
> > # Parent  edcf5f72fdb5621bf6c1c8074ea66428932f0f96
> > hg: when testing, warn if repo.ui is passed to a new peer or localrepo
> >
> > For not getting the configuration of another repo, baseui has to be used when
> > crating a new repository (or a new peer). Therefore we warn the programmer if
> > the ui of a repo is passed.
> > For not warning normal users, the warning is only printed when we are running
> > in run-tests.py (when TESTTMP is in env), when unittest is loaded (running in a
> > test suite of an other project) or when debugging is enabled.
> 
> Well, I still am not thrilled about this approach, and I don't think the
> changes you've made have made it more palatable. Having the code know
> when it's running inside the test environment is not great.
> 
> Perhaps something like this:
> 
> diff -r 7d31f2e42a8a mercurial/commandserver.py
> --- a/mercurial/commandserver.py	Mon Apr 15 18:57:04 2013 -0300
> +++ b/mercurial/commandserver.py	Thu Apr 18 00:34:21 2013 -0500
> @@ -147,6 +147,7 @@
>          self.ui = repo.baseui
>          self.repo = repo
>          self.repoui = repo.ui
> +        del self.repoui.copy # defeat copy protection

Here we restore the original copy function because it is used later as a
"backup" function. But this change will propagate to all commands
resulting in possibly wrong configuration in unrelaed repos.

I think the simplest approach is to not use ui.copy in commandserver,
but to use the ui constructor instead (what ui.copy also does):

=== (+4,-2) mercurial/commandserver.py ===
@@ -184,7 +184,9 @@
         # persist between requests
         copiedui = self.ui.copy()
         self.repo.baseui = copiedui
-        self.repo.ui = self.repo.dirstate._ui = self.repoui.copy()
+        repoui = self.repoui.__class__(self.repoui) # copy is proteced
+        repoui.copy = self.repoui.copy # protect copying local
configuration
+        self.repo.ui = self.repo.dirstate._ui = repoui
         self.repo.invalidate()
         self.repo.invalidatedirstate()

> 
>          if mode == 'pipe':
>              self.cerr = channeledoutput(sys.stderr, sys.stdout, 'e')
> diff -r 7d31f2e42a8a mercurial/localrepo.py
> --- a/mercurial/localrepo.py	Mon Apr 15 18:57:04 2013 -0300
> +++ b/mercurial/localrepo.py	Thu Apr 18 00:34:21 2013 -0500
> @@ -163,6 +163,8 @@
>          self.opener = self.vfs
>          self.baseui = baseui
>          self.ui = baseui.copy()
> +        self.ui.copy = baseui.copy # prevent copying
> +

Does this mean calling repo.ui.copy() actually calls repo.baseui.copy()?
This should do the trick. Great.
A side effect could be that repo.ui.copy has returns a different class
than repo.ui when an extension only changed this one but not repo.baseui
(like the color extension did). 

> # A list of callback to shape the phase if no data were found.
>          # Callback are in the form: func(repo, roots) --> processed root.
>          # This list it to be filled by extension during repo setup
> 
> 
> This mostly passes tests, though there's an interaction between mq and
> color I'm sure you've noticed. Let's talk about this post-2.6.
>