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

[Matt Harbison]

On Thu, 12 Mar 2015 00:27:12 -0400, Martin von Zweigbergk  
<martinvonz at google.com> wrote:

> On Wed, Mar 11, 2015 at 6:00 PM Matt Harbison <mharbison72 at gmail.com>  
> wrote:
>
>> On Wed, 11 Mar 2015 00:23:33 -0400, Martin von Zweigbergk
>> <martinvonz at google.com> wrote:
>>
>> > # HG changeset patch
>> > # User Martin von Zweigbergk <martinvonz at 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