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

Matt Harbison mharbison72 at gmail.com
Wed Mar 11 20:00:37 CDT 2015


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.

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


More information about the Mercurial-devel mailing list