Patchwork [1,of,6] largefiles: replace manifestdict.__contains__, don't extend class

login
register
mail settings
Submitter Martin von Zweigbergk
Date March 11, 2015, 4:23 a.m.
Message ID <1c855bc74470ff7f5a78.1426047813@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/7995/
State Accepted
Commit 2720f967a7b139f2935cf2e6b3916a890c1c0857
Headers show

Comments

Martin von Zweigbergk - March 11, 2015, 4:23 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1425946402 25200
#      Mon Mar 09 17:13:22 2015 -0700
# Node ID 1c855bc74470ff7f5a7805c085f95c279f0ef590
# Parent  60c279ab7bd3bb08779cd6c74230d6739a63ebea
largefiles: replace manifestdict.__contains__, don't extend class

We're soon going to add an alternative manifest class
(treemanifest). Rather than extending both those classes by
largesfiles versions, let's replace the method on the manifest
instances.
Matt Harbison - March 12, 2015, 1 a.m.
On Wed, 11 Mar 2015 00:23:33 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1425946402 25200
> #      Mon Mar 09 17:13:22 2015 -0700
> # Node ID 1c855bc74470ff7f5a7805c085f95c279f0ef590
> # Parent  60c279ab7bd3bb08779cd6c74230d6739a63ebea
> largefiles: replace manifestdict.__contains__, don't extend class
>
> We're soon going to add an alternative manifest class
> (treemanifest). Rather than extending both those classes by
> largesfiles versions, let's replace the method on the manifest
> instances.
>
> diff -r 60c279ab7bd3 -r 1c855bc74470 hgext/largefiles/reposetup.py
> --- a/hgext/largefiles/reposetup.py	Fri Mar 06 22:43:47 2015 -0800
> +++ b/hgext/largefiles/reposetup.py	Mon Mar 09 17:13:22 2015 -0700
> @@ -10,7 +10,7 @@
>  import copy
>  import os
> -from mercurial import error, manifest, match as match_, util
> +from mercurial import error, match as match_, util
>  from mercurial.i18n import _
>  from mercurial import scmutil, localrepo
> @@ -38,17 +38,17 @@
>          def __getitem__(self, changeid):
>              ctx = super(lfilesrepo, self).__getitem__(changeid)
>              if self.lfstatus:
> -                class lfilesmanifestdict(manifest.manifestdict):
> -                    def __contains__(self, filename):
> -                        orig = super(lfilesmanifestdict,  
> self).__contains__
> -                        return orig(filename) or  
> orig(lfutil.standin(filename))
>                  class lfilesctx(ctx.__class__):
>                      def files(self):
>                          filenames = super(lfilesctx, self).files()
>                          return [lfutil.splitstandin(f) or f for f in  
> filenames]
>                      def manifest(self):
>                          man1 = super(lfilesctx, self).manifest()
> -                        man1.__class__ = lfilesmanifestdict
> +                        orig = man1.__contains__
> +                        def __contains__(self, filename):
> +                            return (orig(filename) or
> +                                    orig(lfutil.standin(filename)))
> +                        man1.__contains__ = __contains__.__get__(man1)
>                          return man1
>                      def filectx(self, path, fileid=None, filelog=None):
>                          orig = super(lfilesctx, self).filectx

Is this safe to do, given that _manifest is @propertycached in the base  
class?  IOW, the first time manifest() is called, the original method is  
replaced by the local method.  The second time it is called, is the  
original local method now replaced with another local method (which calls  
the original)?  If so, the result would still be correct, but it would get  
more expensive each time this is called, when the file is not in the  
manifest.

And just for my python learning, is there a reason you chose this instead  
of having one class, call the base class method to get the object, and  
update __class__?  That's basically what the surrounding code does for the  
context object.  I can't get my mind around how that is correct, given  
that there are several different context base classes that can be in play  
at the same time, but only one lfilescontext class.

--Matt
Martin von Zweigbergk - March 12, 2015, 4:27 a.m.
On Wed, Mar 11, 2015 at 6:00 PM Matt Harbison <mharbison72@gmail.com> wrote:

> On Wed, 11 Mar 2015 00:23:33 -0400, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1425946402 25200
> > #      Mon Mar 09 17:13:22 2015 -0700
> > # Node ID 1c855bc74470ff7f5a7805c085f95c279f0ef590
> > # Parent  60c279ab7bd3bb08779cd6c74230d6739a63ebea
> > largefiles: replace manifestdict.__contains__, don't extend class
> >
> > We're soon going to add an alternative manifest class
> > (treemanifest). Rather than extending both those classes by
> > largesfiles versions, let's replace the method on the manifest
> > instances.
> >
> > diff -r 60c279ab7bd3 -r 1c855bc74470 hgext/largefiles/reposetup.py
> > --- a/hgext/largefiles/reposetup.py   Fri Mar 06 22:43:47 2015 -0800
> > +++ b/hgext/largefiles/reposetup.py   Mon Mar 09 17:13:22 2015 -0700
> > @@ -10,7 +10,7 @@
> >  import copy
> >  import os
> > -from mercurial import error, manifest, match as match_, util
> > +from mercurial import error, match as match_, util
> >  from mercurial.i18n import _
> >  from mercurial import scmutil, localrepo
> > @@ -38,17 +38,17 @@
> >          def __getitem__(self, changeid):
> >              ctx = super(lfilesrepo, self).__getitem__(changeid)
> >              if self.lfstatus:
> > -                class lfilesmanifestdict(manifest.manifestdict):
> > -                    def __contains__(self, filename):
> > -                        orig = super(lfilesmanifestdict,
> > self).__contains__
> > -                        return orig(filename) or
> > orig(lfutil.standin(filename))
> >                  class lfilesctx(ctx.__class__):
> >                      def files(self):
> >                          filenames = super(lfilesctx, self).files()
> >                          return [lfutil.splitstandin(f) or f for f in
> > filenames]
> >                      def manifest(self):
> >                          man1 = super(lfilesctx, self).manifest()
> > -                        man1.__class__ = lfilesmanifestdict
> > +                        orig = man1.__contains__
> > +                        def __contains__(self, filename):
> > +                            return (orig(filename) or
> > +                                    orig(lfutil.standin(filename)))
> > +                        man1.__contains__ = __contains__.__get__(man1)
> >                          return man1
> >                      def filectx(self, path, fileid=None, filelog=None):
> >                          orig = super(lfilesctx, self).filectx
>
> Is this safe to do, given that _manifest is @propertycached in the base
> class?  IOW, the first time manifest() is called, the original method is
> replaced by the local method.  The second time it is called, is the
> original local method now replaced with another local method (which calls
> the original)?  If so, the result would still be correct, but it would get
> more expensive each time this is called, when the file is not in the
> manifest.
>

Good point! I didn't even think of thinking about that.


> And just for my python learning, is there a reason you chose this instead
> of having one class, call the base class method to get the object, and
> update __class__?  That's basically what the surrounding code does for the
> context object.


No good reason; I simply didn't see it. Patch coming up.

I can't get my mind around how that is correct, given
> that there are several different context base classes that can be in play
> at the same time, but only one lfilescontext class.
>

It's a separate class for each call, right? It seems like the classes are
dynamically created with ctx.__class__ as super type.
Matt Harbison - March 13, 2015, 1:53 a.m.
On Thu, 12 Mar 2015 00:27:12 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Wed, Mar 11, 2015 at 6:00 PM Matt Harbison <mharbison72@gmail.com>  
> wrote:
>
>> On Wed, 11 Mar 2015 00:23:33 -0400, Martin von Zweigbergk
>> <martinvonz@google.com> wrote:
>>
>> > # HG changeset patch
>> > # User Martin von Zweigbergk <martinvonz@google.com>
>> > # Date 1425946402 25200
>> > #      Mon Mar 09 17:13:22 2015 -0700
>> > # Node ID 1c855bc74470ff7f5a7805c085f95c279f0ef590
>> > # Parent  60c279ab7bd3bb08779cd6c74230d6739a63ebea
>> > largefiles: replace manifestdict.__contains__, don't extend class
>> >
>> > We're soon going to add an alternative manifest class
>> > (treemanifest). Rather than extending both those classes by
>> > largesfiles versions, let's replace the method on the manifest
>> > instances.
>> >
>> > diff -r 60c279ab7bd3 -r 1c855bc74470 hgext/largefiles/reposetup.py
>> > --- a/hgext/largefiles/reposetup.py   Fri Mar 06 22:43:47 2015 -0800
>> > +++ b/hgext/largefiles/reposetup.py   Mon Mar 09 17:13:22 2015 -0700
>> > @@ -10,7 +10,7 @@
>> >  import copy
>> >  import os
>> > -from mercurial import error, manifest, match as match_, util
>> > +from mercurial import error, match as match_, util
>> >  from mercurial.i18n import _
>> >  from mercurial import scmutil, localrepo
>> > @@ -38,17 +38,17 @@
>> >          def __getitem__(self, changeid):
>> >              ctx = super(lfilesrepo, self).__getitem__(changeid)
>> >              if self.lfstatus:
>> > -                class lfilesmanifestdict(manifest.manifestdict):
>> > -                    def __contains__(self, filename):
>> > -                        orig = super(lfilesmanifestdict,
>> > self).__contains__
>> > -                        return orig(filename) or
>> > orig(lfutil.standin(filename))
>> >                  class lfilesctx(ctx.__class__):
>> >                      def files(self):
>> >                          filenames = super(lfilesctx, self).files()
>> >                          return [lfutil.splitstandin(f) or f for f in
>> > filenames]
>> >                      def manifest(self):
>> >                          man1 = super(lfilesctx, self).manifest()
>> > -                        man1.__class__ = lfilesmanifestdict
>> > +                        orig = man1.__contains__
>> > +                        def __contains__(self, filename):
>> > +                            return (orig(filename) or
>> > +                                    orig(lfutil.standin(filename)))
>> > +                        man1.__contains__ =  
>> __contains__.__get__(man1)
>> >                          return man1
>> >                      def filectx(self, path, fileid=None,  
>> filelog=None):
>> >                          orig = super(lfilesctx, self).filectx
>>
>> Is this safe to do, given that _manifest is @propertycached in the base
>> class?  IOW, the first time manifest() is called, the original method is
>> replaced by the local method.  The second time it is called, is the
>> original local method now replaced with another local method (which  
>> calls
>> the original)?  If so, the result would still be correct, but it would  
>> get
>> more expensive each time this is called, when the file is not in the
>> manifest.
>>
>
> Good point! I didn't even think of thinking about that.
>
>
>> And just for my python learning, is there a reason you chose this  
>> instead
>> of having one class, call the base class method to get the object, and
>> update __class__?  That's basically what the surrounding code does for  
>> the
>> context object.
>
>
> No good reason; I simply didn't see it. Patch coming up.
>
>> I can't get my mind around how that is correct, given
>> that there are several different context base classes that can be in  
>> play
>> at the same time, but only one lfilescontext class.
>>
>
> It's a separate class for each call, right? It seems like the classes are
> dynamically created with ctx.__class__ as super type.

I have no idea how that works, and don't know where to look to find out.   
It's mind boggling to me that one instance of lfilesctx can have  
workingctx as a base class while another instance can have changectx as a  
base class *at the same time*.

I've seen hashes with a '+' on the end that shouldn't have been there  
(enabling largefiles when running test-hook.t shows one case), and I was  
wondering if it was because the base class wasn't dynamically created for  
each call.  I suppose a lot more would be broken if it didn't work as you  
describe though.

--Matt

Patch

diff -r 60c279ab7bd3 -r 1c855bc74470 hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py	Fri Mar 06 22:43:47 2015 -0800
+++ b/hgext/largefiles/reposetup.py	Mon Mar 09 17:13:22 2015 -0700
@@ -10,7 +10,7 @@ 
 import copy
 import os
 
-from mercurial import error, manifest, match as match_, util
+from mercurial import error, match as match_, util
 from mercurial.i18n import _
 from mercurial import scmutil, localrepo
 
@@ -38,17 +38,17 @@ 
         def __getitem__(self, changeid):
             ctx = super(lfilesrepo, self).__getitem__(changeid)
             if self.lfstatus:
-                class lfilesmanifestdict(manifest.manifestdict):
-                    def __contains__(self, filename):
-                        orig = super(lfilesmanifestdict, self).__contains__
-                        return orig(filename) or orig(lfutil.standin(filename))
                 class lfilesctx(ctx.__class__):
                     def files(self):
                         filenames = super(lfilesctx, self).files()
                         return [lfutil.splitstandin(f) or f for f in filenames]
                     def manifest(self):
                         man1 = super(lfilesctx, self).manifest()
-                        man1.__class__ = lfilesmanifestdict
+                        orig = man1.__contains__
+                        def __contains__(self, filename):
+                            return (orig(filename) or
+                                    orig(lfutil.standin(filename)))
+                        man1.__contains__ = __contains__.__get__(man1)
                         return man1
                     def filectx(self, path, fileid=None, filelog=None):
                         orig = super(lfilesctx, self).filectx