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

[Martin von Zweigbergk]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150312/a502b027/attachment.html>