Patchwork [1,of,2] repoview: allow methods on the proxy class to be replaced

login
register
mail settings
Submitter Matt Harbison
Date Dec. 9, 2014, 2:31 a.m.
Message ID <299fc395d9c4f633a07e.1418092291@Envy>
Download mbox | patch
Permalink /patch/7026/
State Accepted
Commit ced3ecfc2e57f3e178c48c1bd1c8358dadd9b95e
Headers show

Comments

Matt Harbison - Dec. 9, 2014, 2:31 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1417967576 18000
#      Sun Dec 07 10:52:56 2014 -0500
# Node ID 299fc395d9c4f633a07ef08187f46151ca0ab298
# Parent  b46876c94a935f570ac915ff3d345583c42989bd
repoview: allow methods on the proxy class to be replaced

It doesn't seem to be a common idiom for repo instances, but the status() method
is replaced in largefiles' purge() override.  Since __setattr__ is implemented
in repoview to setattr() on the unfiltered repo, the replacement method wouldn't
get called unless it was invoked with the unfiltered repo, because the filtered
repo remains unchanged.

Since this doesn't seem to be commonly used, I didn't bother to filter out
methods that perhaps shouldn't be replaced, such as changelog().
Pierre-Yves David - Dec. 11, 2014, midnight
On 12/08/2014 06:31 PM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1417967576 18000
> #      Sun Dec 07 10:52:56 2014 -0500
> # Node ID 299fc395d9c4f633a07ef08187f46151ca0ab298
> # Parent  b46876c94a935f570ac915ff3d345583c42989bd
> repoview: allow methods on the proxy class to be replaced
>
> It doesn't seem to be a common idiom for repo instances, but the status() method
> is replaced in largefiles' purge() override.  Since __setattr__ is implemented
> in repoview to setattr() on the unfiltered repo, the replacement method wouldn't
> get called unless it was invoked with the unfiltered repo, because the filtered
> repo remains unchanged.
>
> Since this doesn't seem to be commonly used, I didn't bother to filter out
> methods that perhaps shouldn't be replaced, such as changelog().

This seems a bit too fragile to me. This mean that setting the method on 
a filtered version will have no effect on other filtered version.
The current approach will no doubt works in your case but I wonder if we 
can find other approach that would prevent other people to get bitten in 
the future.
Matt Harbison - Dec. 11, 2014, 12:53 a.m.
On Wed, 10 Dec 2014 19:00:09 -0500, Pierre-Yves David  
<pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 12/08/2014 06:31 PM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1417967576 18000
>> #      Sun Dec 07 10:52:56 2014 -0500
>> # Node ID 299fc395d9c4f633a07ef08187f46151ca0ab298
>> # Parent  b46876c94a935f570ac915ff3d345583c42989bd
>> repoview: allow methods on the proxy class to be replaced
>>
>> It doesn't seem to be a common idiom for repo instances, but the  
>> status() method
>> is replaced in largefiles' purge() override.  Since __setattr__ is  
>> implemented
>> in repoview to setattr() on the unfiltered repo, the replacement method  
>> wouldn't
>> get called unless it was invoked with the unfiltered repo, because the  
>> filtered
>> repo remains unchanged.
>>
>> Since this doesn't seem to be commonly used, I didn't bother to filter  
>> out
>> methods that perhaps shouldn't be replaced, such as changelog().
>
> This seems a bit too fragile to me. This mean that setting the method on  
> a filtered version will have no effect on other filtered version.
> The current approach will no doubt works in your case but I wonder if we  
> can find other approach that would prevent other people to get bitten in  
> the future.

I see what you are saying, but I'm not sure what else to do.  It seems  
like to make this work for all repo views, it needs to set the method  
through to the unfiltered repo (as it is currently doing).  And then each  
filtered repo needs to check the unfiltered repo to see if the method  
being called differs between the filtered and unfiltered repos, calling  
the unfiltered method if they differ (and was explicitly set, since no  
doubt there are natural differences to do the filtering).  I have no idea  
how to spell that in python, and I'm guessing it would come with a large  
performance impact.  Especially if this is the only case of a repo method  
needing to be replaced.

I don't know if there was anything inherently wrong with the old code- the  
comment about it being buggy and needing to be investigated was what got  
my attention.  As long as we get rid of it by fixing it or explaining why  
unfiltered is necessary so nobody else digs into it, it will be an  
improvement.

--Matt
Pierre-Yves David - Dec. 15, 2014, 3:33 a.m.
On 12/10/2014 04:53 PM, Matt Harbison wrote:
> On Wed, 10 Dec 2014 19:00:09 -0500, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 12/08/2014 06:31 PM, Matt Harbison wrote:
>>> # HG changeset patch
>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>> # Date 1417967576 18000
>>> #      Sun Dec 07 10:52:56 2014 -0500
>>> # Node ID 299fc395d9c4f633a07ef08187f46151ca0ab298
>>> # Parent  b46876c94a935f570ac915ff3d345583c42989bd
>>> repoview: allow methods on the proxy class to be replaced
>>>
>>> It doesn't seem to be a common idiom for repo instances, but the
>>> status() method
>>> is replaced in largefiles' purge() override.  Since __setattr__ is
>>> implemented
>>> in repoview to setattr() on the unfiltered repo, the replacement
>>> method wouldn't
>>> get called unless it was invoked with the unfiltered repo, because
>>> the filtered
>>> repo remains unchanged.
>>>
>>> Since this doesn't seem to be commonly used, I didn't bother to
>>> filter out
>>> methods that perhaps shouldn't be replaced, such as changelog().
>>
>> This seems a bit too fragile to me. This mean that setting the method
>> on a filtered version will have no effect on other filtered version.
>> The current approach will no doubt works in your case but I wonder if
>> we can find other approach that would prevent other people to get
>> bitten in the future.
>
> I see what you are saying, but I'm not sure what else to do.  It seems
> like to make this work for all repo views, it needs to set the method
> through to the unfiltered repo (as it is currently doing).  And then
> each filtered repo needs to check the unfiltered repo to see if the
> method being called differs between the filtered and unfiltered repos,
> calling the unfiltered method if they differ (and was explicitly set,
> since no doubt there are natural differences to do the filtering).  I
> have no idea how to spell that in python, and I'm guessing it would come
> with a large performance impact.  Especially if this is the only case of
> a repo method needing to be replaced.

So I think that duck punching this status stuff on the repo is hacky 
(and now that I read it, it should bit in a try, finally). We should 
probably not optimize our code to make it easier. The change to the 
change to repoview implementation is fragile and likely to fails in 
other cases, I think we should back it out. I would rather see the 
fragile special case in the hacky extension code than in the main code.

> I don't know if there was anything inherently wrong with the old code-
> the comment about it being buggy and needing to be investigated was what
> got my attention.  As long as we get rid of it by fixing it or
> explaining why unfiltered is necessary so nobody else digs into it, it
> will be an improvement.

I think it would be better to install a permanent wrapping of status and 
use a special attribute to signal when it should be disabled.

Another interesting fact is that most of the status logic have been 
moved out of the repository. So one could probably wrapped that instead 
of the stricky repoview/repo.

If you are okay with my argument, I will push a backout of the two 
changesets from this series.
Matt Harbison - Dec. 15, 2014, 4:12 a.m.
On Sun, 14 Dec 2014 22:33:05 -0500, Pierre-Yves David  
<pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 12/10/2014 04:53 PM, Matt Harbison wrote:
>> On Wed, 10 Dec 2014 19:00:09 -0500, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>
>>>
>>> On 12/08/2014 06:31 PM, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1417967576 18000
>>>> #      Sun Dec 07 10:52:56 2014 -0500
>>>> # Node ID 299fc395d9c4f633a07ef08187f46151ca0ab298
>>>> # Parent  b46876c94a935f570ac915ff3d345583c42989bd
>>>> repoview: allow methods on the proxy class to be replaced
>>>>
>>>> It doesn't seem to be a common idiom for repo instances, but the
>>>> status() method
>>>> is replaced in largefiles' purge() override.  Since __setattr__ is
>>>> implemented
>>>> in repoview to setattr() on the unfiltered repo, the replacement
>>>> method wouldn't
>>>> get called unless it was invoked with the unfiltered repo, because
>>>> the filtered
>>>> repo remains unchanged.
>>>>
>>>> Since this doesn't seem to be commonly used, I didn't bother to
>>>> filter out
>>>> methods that perhaps shouldn't be replaced, such as changelog().
>>>
>>> This seems a bit too fragile to me. This mean that setting the method
>>> on a filtered version will have no effect on other filtered version.
>>> The current approach will no doubt works in your case but I wonder if
>>> we can find other approach that would prevent other people to get
>>> bitten in the future.
>>
>> I see what you are saying, but I'm not sure what else to do.  It seems
>> like to make this work for all repo views, it needs to set the method
>> through to the unfiltered repo (as it is currently doing).  And then
>> each filtered repo needs to check the unfiltered repo to see if the
>> method being called differs between the filtered and unfiltered repos,
>> calling the unfiltered method if they differ (and was explicitly set,
>> since no doubt there are natural differences to do the filtering).  I
>> have no idea how to spell that in python, and I'm guessing it would come
>> with a large performance impact.  Especially if this is the only case of
>> a repo method needing to be replaced.
>
> So I think that duck punching this status stuff on the repo is hacky  
> (and now that I read it, it should bit in a try, finally). We should  
> probably not optimize our code to make it easier. The change to the  
> change to repoview implementation is fragile and likely to fails in  
> other cases, I think we should back it out. I would rather see the  
> fragile special case in the hacky extension code than in the main code.

I agree with that principle.  I don't understand enough of it to judge  
what is hacky, so I'll certainly defer to you on that.

>> I don't know if there was anything inherently wrong with the old code-
>> the comment about it being buggy and needing to be investigated was what
>> got my attention.  As long as we get rid of it by fixing it or
>> explaining why unfiltered is necessary so nobody else digs into it, it
>> will be an improvement.
>
> I think it would be better to install a permanent wrapping of status and  
> use a special attribute to signal when it should be disabled.
>
> Another interesting fact is that most of the status logic have been  
> moved out of the repository. So one could probably wrapped that instead  
> of the stricky repoview/repo.

Moved to the various contexts, right?  The lfilesrepo.status() seems  
pretty complicated, so I'm not sure how many people will jump on that.  I  
was hacking on the largefiles context last weekend trying to get filesets  
to work with largefiles, so maybe there's additional merit in fixing up  
the context class.

> If you are okay with my argument, I will push a backout of the two  
> changesets from this series.

I have no issue with a backout.  Just please follow up with something that  
replaces the XXX comment with a reason why it is this way, so nobody else  
looks into it.

BTW, I have a fix for the similar comment on lfilerepo.status(), which  
just looks to the unfiltered repo for the lfstatus value.  Since all views  
do that, I think it avoids your concern about different views getting out  
of sync.  That would avoid needing to wrap status and trying to signal  
which way to go.  I'll submit it in the next day or two depending on my  
workload, probably as an RFC so you can comment on it before it is queued.

--Matt

Patch

diff --git a/mercurial/repoview.py b/mercurial/repoview.py
--- a/mercurial/repoview.py
+++ b/mercurial/repoview.py
@@ -6,6 +6,7 @@ 
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 
+import types
 import copy
 import error
 import phases
@@ -310,6 +311,10 @@ 
         return getattr(self._unfilteredrepo, attr)
 
     def __setattr__(self, attr, value):
+        # Allow method replacement on filtered repos, like status() in
+        # largefiles' purge override
+        if type(value) == types.FunctionType:
+            object.__setattr__(self, attr, value)
         return setattr(self._unfilteredrepo, attr, value)
 
     def __delattr__(self, attr):