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

[Matt Mackall]

On Wed, 2013-07-17 at 03:14 +0200, Simon Heimberg wrote:
> Code freeze has passed a long time ago, so let's finally continue this
> discussion.

News to me. I was planning on starting it tomorrow? But perhaps you mean
the last code freeze.

> On Thu, 2013-04-18 at 00:36 -0500, Matt Mackall wrote: 

Yes, it seems you do. We're on a three month cycle.

> > 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.

Yep.

-- 
Mathematics is the supreme nostalgia of our time.