Patchwork largefiles: don't create chain of __contains__ calls

login
register
mail settings
Submitter Martin von Zweigbergk
Date March 12, 2015, 4:49 a.m.
Message ID <049a57bd8db8ade4d29b.1426135759@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/8019/
State Accepted
Commit f78252429e0aff26196c096fd17460ae8adf86f0
Headers show

Comments

Martin von Zweigbergk - March 12, 2015, 4:49 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1426135008 25200
#      Wed Mar 11 21:36:48 2015 -0700
# Node ID 049a57bd8db8ade4d29b949609f05595b536a0e4
# Parent  7cf9a9e0cf893e7ae82dc576a03c843fd6640438
largefiles: don't create chain of __contains__ calls

Matt Harbison pointed out that my recent 2720f967a7b1 might cause
__contains__ to continously get replaced by another version that calls
itself, since the manifest instance returned by the super method is
always the same instance due to @propertycache. He also suggested
replacing the class instead, as is done with the context class in the
surrounding code. Do so.
Matt Mackall - March 12, 2015, 9:15 p.m.
On Wed, 2015-03-11 at 21:49 -0700, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1426135008 25200
> #      Wed Mar 11 21:36:48 2015 -0700
> # Node ID 049a57bd8db8ade4d29b949609f05595b536a0e4
> # Parent  7cf9a9e0cf893e7ae82dc576a03c843fd6640438
> largefiles: don't create chain of __contains__ calls

Queued for default, thanks.
Martin von Zweigbergk - March 23, 2015, 11:53 p.m.
On Wed, Mar 11, 2015 at 9:49 PM Martin von Zweigbergk <martinvonz@google.com>
wrote:

> +                        class lfilesmanifest(man1.__class):
>

Drew (drgott) pointed out to me that it's spelled "__class__", not
"__class", so it looks like this patch of the other patch will need to be
patched itself :-( And there is no test coverage (clearly). I'll try to add
a test if it's not too much work.
Matt Harbison - March 24, 2015, 1:29 a.m.
On Mon, 23 Mar 2015 19:53:56 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Wed, Mar 11, 2015 at 9:49 PM Martin von Zweigbergk  
> <martinvonz@google.com>
> wrote:
>
>> +                        class lfilesmanifest(man1.__class):
>>
>
> Drew (drgott) pointed out to me that it's spelled "__class__", not
> "__class", so it looks like this patch of the other patch will need to be
> patched itself :-( And there is no test coverage (clearly). I'll try to  
> add
> a test if it's not too much work.

Yep, I missed that too.  A quick search of the code leads me to believe it  
isn't being called anywhere.  It can only be called if lfstatus is True,  
and that only happens in

1) removelargefiles()
2) status (but repo.status() turns this off for the duration of its  
processing)
3) dirty()
4) bailifchanged()
5) forget()
6) summary

I'm certain these things have test coverage.

I'll try to add an override for the manifest command to hit this.  There's  
something else broken here, because I'm getting a TypeError...

--Matt
Martin von Zweigbergk - March 24, 2015, 1:34 a.m.
If it's of any help, the only caller of filectx.manifest() I could find was
annotate in the web UI. I could trigger a call to the unmodified version by
visiting the annotate page for a file, but I couldn't trigger the
overridden version.

On Mon, Mar 23, 2015 at 6:29 PM Matt Harbison <mharbison72@gmail.com> wrote:

> On Mon, 23 Mar 2015 19:53:56 -0400, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > On Wed, Mar 11, 2015 at 9:49 PM Martin von Zweigbergk
> > <martinvonz@google.com>
> > wrote:
> >
> >> +                        class lfilesmanifest(man1.__class):
> >>
> >
> > Drew (drgott) pointed out to me that it's spelled "__class__", not
> > "__class", so it looks like this patch of the other patch will need to be
> > patched itself :-( And there is no test coverage (clearly). I'll try to
> > add
> > a test if it's not too much work.
>
> Yep, I missed that too.  A quick search of the code leads me to believe it
> isn't being called anywhere.  It can only be called if lfstatus is True,
> and that only happens in
>
> 1) removelargefiles()
> 2) status (but repo.status() turns this off for the duration of its
> processing)
> 3) dirty()
> 4) bailifchanged()
> 5) forget()
> 6) summary
>
> I'm certain these things have test coverage.
>
> I'll try to add an override for the manifest command to hit this.  There's
> something else broken here, because I'm getting a TypeError...
>
> --Matt
>
Matt Harbison - March 24, 2015, 1:38 a.m.
On Mon, 23 Mar 2015 21:34:14 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> If it's of any help, the only caller of filectx.manifest() I could find  
> was
> annotate in the web UI. I could trigger a call to the unmodified version  
> by
> visiting the annotate page for a file, but I couldn't trigger the
> overridden version.

This isn't a filectx- it's a changectx.  The manifest command definitely  
calls that.  You need to have the lfstatus attribute set to True in order  
to trigger it, but the key thing is, this has to be set before the context  
is fetched.

> On Mon, Mar 23, 2015 at 6:29 PM Matt Harbison <mharbison72@gmail.com>  
> wrote:
>
>> On Mon, 23 Mar 2015 19:53:56 -0400, Martin von Zweigbergk
>> <martinvonz@google.com> wrote:
>>
>> > On Wed, Mar 11, 2015 at 9:49 PM Martin von Zweigbergk
>> > <martinvonz@google.com>
>> > wrote:
>> >
>> >> +                        class lfilesmanifest(man1.__class):
>> >>
>> >
>> > Drew (drgott) pointed out to me that it's spelled "__class__", not
>> > "__class", so it looks like this patch of the other patch will need  
>> to be
>> > patched itself :-( And there is no test coverage (clearly). I'll try  
>> to
>> > add
>> > a test if it's not too much work.
>>
>> Yep, I missed that too.  A quick search of the code leads me to believe  
>> it
>> isn't being called anywhere.  It can only be called if lfstatus is True,
>> and that only happens in
>>
>> 1) removelargefiles()
>> 2) status (but repo.status() turns this off for the duration of its
>> processing)
>> 3) dirty()
>> 4) bailifchanged()
>> 5) forget()
>> 6) summary
>>
>> I'm certain these things have test coverage.
>>
>> I'll try to add an override for the manifest command to hit this.   
>> There's
>> something else broken here, because I'm getting a TypeError...
>>
>> --Matt
Matt Harbison - March 24, 2015, 4:07 a.m.
On Mon, 23 Mar 2015 19:53:56 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Wed, Mar 11, 2015 at 9:49 PM Martin von Zweigbergk  
> <martinvonz@google.com>
> wrote:
>
>> +                        class lfilesmanifest(man1.__class):
>>
>
> Drew (drgott) pointed out to me that it's spelled "__class__", not
> "__class", so it looks like this patch of the other patch will need to be
> patched itself :-( And there is no test coverage (clearly). I'll try to  
> add
> a test if it's not too much work.

Well, adding manifest command support turns out to be not so easy.  I  
think I'm running into issues with the repo filtering, and hacking around  
that, it wants to print the standins instead of the largefiles because it  
is iterating over context.  I've got something that works, but I need to  
run all of the tests, and think about unintended consequences on the off  
chance the tests run successfully.

I'll put this on my list of things to figure out.  Thanks for pointing  
this out.

--Matt
Martin von Zweigbergk - March 24, 2015, 4:14 a.m.
On Mon, Mar 23, 2015 at 9:07 PM Matt Harbison <mharbison72@gmail.com> wrote:

> On Mon, 23 Mar 2015 19:53:56 -0400, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>
> > On Wed, Mar 11, 2015 at 9:49 PM Martin von Zweigbergk
> > <martinvonz@google.com>
> > wrote:
> >
> >> +                        class lfilesmanifest(man1.__class):
> >>
> >
> > Drew (drgott) pointed out to me that it's spelled "__class__", not
> > "__class", so it looks like this patch of the other patch will need to be
> > patched itself :-( And there is no test coverage (clearly). I'll try to
> > add
> > a test if it's not too much work.
>
> Well, adding manifest command support turns out to be not so easy.  I
> think I'm running into issues with the repo filtering, and hacking around
> that, it wants to print the standins instead of the largefiles because it
> is iterating over context.  I've got something that works, but I need to
> run all of the tests, and think about unintended consequences on the off
> chance the tests run successfully.
>
> I'll put this on my list of things to figure out.  Thanks for pointing
> this out.


Thank you! I have no personal interest in largefiles, so I'm very happy if
someone who does deals with it.

Patch

diff -r 7cf9a9e0cf89 -r 049a57bd8db8 hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py	Wed Mar 11 15:22:34 2015 -0700
+++ b/hgext/largefiles/reposetup.py	Wed Mar 11 21:36:48 2015 -0700
@@ -44,11 +44,12 @@ 
                         return [lfutil.splitstandin(f) or f for f in filenames]
                     def manifest(self):
                         man1 = super(lfilesctx, self).manifest()
-                        orig = man1.__contains__
-                        def __contains__(self, filename):
-                            return (orig(filename) or
-                                    orig(lfutil.standin(filename)))
-                        man1.__contains__ = __contains__.__get__(man1)
+                        class lfilesmanifest(man1.__class):
+                            def __contains__(self, filename):
+                                orig = super(lfilesmanifest, self).__contains__
+                                return (orig(filename) or
+                                        orig(lfutil.standin(filename)))
+                        man1.__class__ = lfilesmanifest
                         return man1
                     def filectx(self, path, fileid=None, filelog=None):
                         orig = super(lfilesctx, self).filectx