| 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
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.
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.
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
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 >
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
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
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