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

[Matt Harbison]

On Sun, 14 Dec 2014 22:33:05 -0500, Pierre-Yves David  
<pierre-yves.david at 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 at ens-lyon.org> wrote:
>>
>>>
>>>
>>> On 12/08/2014 06:31 PM, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison at 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