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

login
register
mail settings
Submitter Matt Mackall
Date April 18, 2013, 5:36 a.m.
Message ID <1366263398.4336.91.camel@calx>
Download mbox | patch
Permalink /patch/1406/
State Accepted, archived
Headers show

Comments

Matt Mackall - April 18, 2013, 5:36 a.m.
On Fri, 2013-03-22 at 02:20 +0100, Simon Heimberg wrote:
> # HG changeset patch
> # User Simon Heimberg <simohe@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:



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.
Simon Heimberg - July 17, 2013, 1:14 a.m.
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@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.
>
Matt Mackall - July 17, 2013, 5:56 p.m.
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@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.

Patch

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
 
         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
+
         # 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