#launchpad-reviews 2010-02-15
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: are you ok with a short cover letter?
<jtv> henninge: sure, just mention anything that might be surprising.
 * henninge surprises himself sometime ... ;)
<henninge> *sometimes*
<jtv> Oops, my gnome just screwed itself up.
<henninge> jelmer: Hi! you have two branches ready for review, which one do you want reviewed?
<jelmer> henninge: both, if possible
<jelmer> henninge: the cleanup one is trivial but the upload log one is more important
<henninge> jelmer: ok, I'll look at both
<henninge> jelmer: thanks for the clean-up! ;) If your team is switching to tag-based QA (I think all are), you should attach a bug to it.
<henninge> jelmer: r=me, though.
<henninge> ;)
<jelmer> henninge: Will do. Thanks for the review!
* henninge changed the topic of #launchpad-reviews to: on call: henninge || reviewing: jelmer || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: back up and running
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-517080-pottery-split/+merge/19325
<henninge> ?
<jtv> henninge: yes, back up and running...  This gets a bit scary though.  I wonder if it's the temperature differences.
<henninge> jtv: which is close to 40 degrees, isn't it? ... ;)
<henninge> 35 at this very moment
<jtv> The difference is probably about 20Â°C since I'm working in a building (though unheated).  But I think it's what caused the trouble with the backup disk in my office.
<jtv> henninge: have a look at TestCaseWithFactory.useTempDir
<henninge> jtv: yes, I actually saw that just today. Do you know if I can use TestCaseWithFactory without setting a layer? I am quite happy about the quick start-up of the test.
<jtv> oh, you probably need a layer for it, true.  So why not move that helper into lp.testing.TestCase?
<jtv> I don't see it using anything that depends on any layers
<henninge> jtv: good idea.
<jtv> henninge: also, line 91 of the diff (not touched here, but still): packagename+".tar.bz2"...   spaces around the + please
<henninge> right, wonder who reviewed that line before ...
<henninge> :-P
<henninge> jelmer: the other mp has a merge conflict
<henninge> jelmer: I guess you need to merge a current devel and push the branch again.
<jelmer> henninge: will do
<jtv> henninge: could you replace the comments in TestDetectIntltoolInBzrTree with less confusing ones?
<henninge> jtv: oh, sorry, copy-and-paste error ... ;)
<jtv> henninge: Good idea to have the "with" here.
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: jelmer || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> henninge: it'd be nice to have some kind of clear distinction between "server-safe" and "slave-side" code.
<henninge> jtv: yes, I've been wondering about that. Should I move it to its own module?
<jtv> henninge: I'm not sure...  it can also be useful to have it close to the template-generation code whose preconditions it mimicks.
<jtv> All I'm really interested in "from the outside" is (1) a server-side check whether pottery thinks it'll be able to produce templates for my branch, and (2) a slave-side operation to generate the templates.  Maybe in the future we'll want to have that plus a bunch of modules for the different kinds of translation setup that pottery may encounter.
<jtv> So I'd suggest not splitting this up into modules until a pattern emerges.
<jtv> henninge: Can you come up with another clear way to mark the unsafe code?
<jelmer> henninge: new proposal at https://code.edge.launchpad.net/~jelmer/launchpad/bug499115/+merge/19328
<henninge> jtv: "unsafe" as in "should run on slave", right?
<jtv> henninge: right
<henninge> s/should/must/
<henninge> jtv: I had been thinking about moving the functions into a class. That way they are clearly groped. Maybe the class could even do some automatic checking that it is really running on a slave.
<jtv> henninge: a class could be very nice.  I wouldn't do any checking there though; that's hard to test and at the same time security-sensitive.
<jtv> It makes more sense to isolate as much as you can in code that doesn't care where it runs.
<henninge> jtv: I didn't meant to add the checks right now, anyway.
<jtv> Another problem is getting the code onto the slave.  It'll need to be either in the buildd package, or some other package we can install on the slave.
<henninge> jelmer: no test?
<jelmer> henninge, unfortunately, no :-/ I couldn't find a good way to test this.
 * henninge lunches
<jtv> intellectronica, got time for a pretty big one?  https://code.launchpad.net/~jtv/launchpad/bug-507678/+merge/19131
<intellectronica> jtv: i'm just about to break for lunch, but if it can wait until i'm back then sure
<jtv> intellectronica: it can wait.  thanks.
<intellectronica> np
<jtv> on call: henninge, intellectronica || reviewing: jelmer || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> wgrant: still around?
<adiroiban> -/n
<adiroiban> henninge: can you take a look at the last comments for bug 127171 ?
<mup> Bug #127171: Rosetta experts not allowed to "Change translators" <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/127171>
<adiroiban> I would like to finalize the work and submit a MP
<henninge> adiroiban: looking
<henninge> adiroiban: you need to merge a current devel, first. Your branch seeems to be rotting.
<adiroiban> henninge: I just found bug 516317, which is about renaming that URL
<mup> Bug #516317: Product:+changetranslators should become +settings <ui> <Launchpad Translations:In Progress by danilo> <https://launchpad.net/bugs/516317>
<adiroiban> I guess I will have to wait for Danilo to merge that branch and then continue my work
<henninge> adiroiban: yes and you should finish the pre-imp discussion with him, I guess.
<henninge> adiroiban: but the thing about updating your branch is true. Your addition to security.py won't work the way it is now because of a branch I landed a few weeks ago.
<adiroiban> henninge: true. I have merged the branch on my local machine, but did not push the changes to LP. I will continue with Danilo. Thanks!
<intellectronica> jtv: there are merge conflict artifacts in your diff
<jtv> intellectronica: I guess I accidentally proposed for merging in db-devel.
<intellectronica> indeed
<intellectronica> care to paste the correct diff for me?
<jtv> intellectronica: I guess I can make a corrected copy of the MP.
<intellectronica> jtv: even better
<jtv> intellectronica: https://code.launchpad.net/~jtv/launchpad/bug-507678/+merge/19335
<intellectronica> jtv: cheers
<intellectronica> jtv: in extractBuildStatus, how about raising a ValueError instead of asserting?
<jtv> intellectronica: I'd rather be conservative there... that part is a refactoring, in code that's basically impossible to integration-test.
<jtv> intellectronica: I do agree it would make more sense though.
<intellectronica> jtv: oh, you mean the assertion isn't a new addition but just code moved from somewhere?
<jtv> intellectronica: eso.
<intellectronica> eso?
<intellectronica> exactly so?
<jtv> Right.  :)
<jtv> Well, from Spanish, but semantically yes.
<intellectronica> gotcha
<jtv> "That."
<intellectronica> oooh i like FakeMethod
<intellectronica> it's a nice thing to have
<jtv> thanks...  I'm sure you can guess what name I _wanted_ to give it but didn't in consideration for a colleague who is geographically capable of getting to me physically?
<jtv> I did write (and heavily use) a class of that name in the ec2test unit-tests I wrote, so I'll also file a bug for replacing that with FakeMethod.
<jtv> Or better yet, I could do it in this branch.  But I'll leave that for after the main review so it doesn't clutter up the diff.
<intellectronica> jtv: r=me. great work.
<jtv> intellectronica: thanks!  No mean compliment from you.  :)
<leonardr> henninge or intellectronica: please review https://code.edge.launchpad.net/~leonardr/lazr.restful/exported-subclass/+merge/19337
<intellectronica> leonardr: i'll take it
<intellectronica> leonardr: r=me
<henninge> jelmer: sorry, got distracted. I sent a review with a suggestion on how to add a test. Please simply push to the branch, don't "resubmit" the proposal, like you did earlier.
<jelmer> henninge, ok, thanks
* henninge changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> jtv: you did not mention moving the functions into a class in your review - are you with holding that back for a later branch?
<jtv> henninge: yes, I think it makes sense to leave that for a next step: getting the right code to the slave.
<leonardr> intellectronica, i need a launchpadlib and a launchpad branch reviewed in tandem
<intellectronica> leonardr: oright
<leonardr> intellectronica: if you can give the launchpadlib branch the once-over i can make a preliminary package for it and put it in my site-dist to run the ec2 test on the launchpad branch
<intellectronica> leonardr: ok
<intellectronica> leonardr: MP?
<leonardr> intellectronica: on the way
<intellectronica> cool
<leonardr> intellectronica: https://code.edge.launchpad.net/~leonardr/launchpadlib/multiversion/+merge/19343
<intellectronica> leonardr: r=me
<leonardr> cool
<leonardr> working on launchpad one now
<leonardr> intellectronica: https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346
<intellectronica> leonardr: looks good, as far as i can understand
<intellectronica> leonardr: can you explain briefly how the grok thing works and why it's necessary?
<leonardr> intellectronica: sure
<leonardr> the way we use grok, it checks to see if you subclassed a certain class, and then performs some magic
<leonardr> in this case, the magic looks like this:
<leonardr> http://pastebin.ubuntu.com/376972/
<leonardr> intellectronica: so when the magic code runs, we get an autogenerated marker interface for each version of the web service
<leonardr> IWebServiceClientRequestVersion_beta
<leonardr> IWebServiceClientRequestVersion_1_0
<intellectronica> gotcha
<leonardr> IWebServiceClientRequestVersion_devel
<intellectronica> leonardr: r=me
<leonardr> cool
<leonardr> now to get it all together and run the test
<intellectronica> good luck
* adiroiban changed the topic of #launchpad-reviews to: on call: henninge, intellectronica || reviewing: -, - || queue [adiroiban(codereview bug-340664)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> adiroiban: i apologize, but i probably won't have time to review your branch today
<intellectronica> it's quite a big branch
<adiroiban> intellectronica: no hurry, I just put in there to be reviewed when someone has a bit of free time
* intellectronica changed the topic of #launchpad-reviews to: on call: henninge || reviewing: - || queue [adiroiban(codereview bug-340664)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-02-16
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/only-show-latest-bmp-in-revision-mail/+merge/19377
* henninge changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(codereview bug-340664)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(codereview bug-340664),adiroiban(bug-522188)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> bigjools: You poked me about 10 minutes too late last night?
<bigjools> wgrant: it was about the branch I reviewed
<bigjools> I put the question on the MP
<wgrant> bigjools: Ah, right. I fixed those this morning.
<bigjools> sweet
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adiroiban || queue [adiroiban(bug-522188)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: gmb,bac || reviewing: adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> gmb, bac: could one of you review please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-283941-show-patch-numbers-on-upstream-report/+merge/19405 ?
<bac> adeuring: i'll take it
<adeuring> bac: thanks!
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, abel || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> adeuring: the MP shows lots of text conflicts
<bac> adeuring: have they been resolved?
<adeuring> bac: no, let me check...
<adeuring> bac: The conflicts were mostly caused by me and somebody else removing some lint in lp.bugs.browser.bugtask ;) I pushed a new vresion of the branch
<bac> thanks adeuring
<bac> adeuring: is your branch really based on db-devel not devel/
<adeuring> bac: yes. I ue the column Bug.latest_patch_uploaded in the modified SQL query, and thi column is not yet in the production DB
<adeuring> s/ue/use/
<bac> right
* noodles775 changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, abel || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb, bac, when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/506203-ppa-privatisation-check/+merge/19415
<gmb> noodles775: I'll take a look at it in a minute or two.
<noodles775> Thanks gmb.
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: noodles, abel || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> rockstar, if you're feeling better, i have a multiversion branch coming up
<rockstar> leonardr, "better" is a relative term, but yeah, I'm able to review.
<leonardr> rockstar: it's pretty simple
<bac> adeuring: what is the lp.dev URL to see your new work?
<adeuring> bac: https://launchpad.dev/ubuntu/+upstreamreport (sorry for not mentioning it in the MP...)
<bac> np.  that's why interactive reviews are so nice
<leonardr> rockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-request-user/+merge/19418
<gmb> noodles775: r=me with a couple of tweaks.
<gmb> Nothing major.
<noodles775> Great, thanks gmb.
* gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: abel || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> gmb: lol, re. the is_empty()... we hit the same point last week. I tried is_empty first, but it's not part of ISQLObjectResultSet.
<gmb> noodles775: Oh, sod. I hate that shim. Okay, checking .count() is an acceptable alternative then.
<leonardr> intellectronica, can i trouble you to take another look at https://code.edge.launchpad.net/~leonardr/launchpad/multiversion-integration/+merge/19346 ?
<intellectronica> leonardr: ok
<intellectronica> have you made changes to it?
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> intellectronica: yes
<leonardr> i can paste you an incremental diff
<intellectronica> leonardr: that would be great
<intellectronica> just reading your comment now
<leonardr> http://paste.ubuntu.com/377720/
<intellectronica> leonardr: thanks
<intellectronica> it would be nice if the revision entries in an mp could expand to contain the incremental diff
<intellectronica> abentley, rockstar: ^^^^^
<intellectronica> leonardr: i don't understand why on line 65 of the diff you still have '/beta/' in the url
<intellectronica> isn't it supposed to be replaced by the 'devel' you pass to api_version?
<leonardr> intellectronica: hm, it's possible i didn't run that test, it's not a javascript test
<abentley> intellectronica, see bug #485625
<mup> Bug #485625: mail out interdiffs when updating review diffs <code-review> <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/485625>
<intellectronica> leonardr: everything else looks fine to me, so just check this one test first to see if it's ok. other than that r=me
<intellectronica> abentley: nice :)
* sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: You will see my mp in a few minutes. It ill look familiar. I decided to use a tales formatter for boolean icons. We can reconcile my branch with yours after our branches land
<bac> sinzui: ok
<jtv> abentley: 'howbout dis den: https://code.edge.launchpad.net/~jtv/launchpad/branch-url/+merge/19422
<abentley> jtv, why are you listifying a tuple and then adding a list to it, instead of just adding a tuple to the tuple?
<jtv> abentley: IIRC somebody decided once upon a time that tuples were bad for these "in constant_bunch" checks...  I'm fine with making it a tuple, but this is less sensitive to change too.
<abentley> jtv, why are lists better for that kind of check?  Wouldn't a set be better?
<jtv> abentley: true, it would save one letter of typing
<abentley> jtv, I would like to see the rationale for not using tuples for that kind of check.
<jtv> abentley: it was a surprisingly long discussion, and frankly I didn't care enough to get involved.  If it's performance-sensitive, that's different.
<abentley> jtv, I know I suggested bzr_identity, but I think it makes more sense to use unique_name, as xmlrpc version does.
<jtv> abentley: I guess all that skips is the "abbreviation" of the name, right?
<abentley> jtv, it means you don't have to slice, but it also means you won't get short names like lp:bzr.
<jtv> abentley: off the top of my head, it'd give me '~abentley/launchpad/mybranch', right?
<abentley> jtv, unique_name will give you that.
<jtv> abentley: then change it I shall.
<jtv> definitely cleaner than adding the "lp:", checking for its presence, and removing it again.  :-)
<abentley> jtv, why are you composing the URL using strings rather than the URI class?
<jtv> abentley: never occurred to me, but it's clearer now that I just simplified things.
<abentley> jtv, cool.
<jtv> (changing)
<abentley> jtv, test_composePublicURL_unknown_scheme needs a comment.
<abentley> jtv, could you also export the method over the API, please?
<jtv> abentley: working on it
<abentley> jtv, (I've already had to compensate for the lack of it several times in plugins).
<jtv> abentley: any idea why URI doesn't insert the "/" between host/port and path?
<abentley> jtv, not really.
<jtv> especially with the temptation to use os.path, which is wrong...
<jtv> (except maybe for "file:" :-)
<leonardr> rockstar, any progress on the review?
<rockstar> leonardr, sorry, was looking after another issue real quick.
<jtv> abentley: the diff is updated.
<abentley> jtv, were you going to change accepted_schemes to a set?
<jtv> abentley: if you care enough to ask me to, yes.
<abentley> jtv, okay, please do.
<abentley> jtv, r=me with that change.
<jtv> abentley: thanks!  The change is being pushed.
<EdwinGrubbs> bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-99395-linking-sourcepackages-to-projects/+merge/19429
<bac> EdwinGrubbs: sure
<abentley> bac, could you please review https://code.edge.launchpad.net/~abentley/launchpad/twisted-oopsdir/+merge/19431 ?
<bac> abentley: sure
* abentley changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [sinzui, abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: edwin || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: abentley || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: I just created an mp for a branch that got too large. I need to fix an oops, but I had to migrate some code to lp.registry first. So I have a mechanical branch for you to review.
<bac> ok
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on call: bac || reviewing: sinzui || queue [adiroiban(bug-127171)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> sinzui: done
* bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [adiroiban(bug-127171)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> is anyone around to review a truly trivial launchpadlib branch? maybe jml?
#launchpad-reviews 2010-02-17
<leonardr> rockstar, if you're still around?
<mwhudson> leonardr: i'm here
<leonardr> mwhudson, huzzah
<leonardr> give me a few min to write an mp
<mwhudson> leonardr: sure
<mwhudson> request the mp
<mwhudson> leonardr: i might be having lunch but i won't be away for very long
<leonardr> mwhudson: https://code.edge.launchpad.net/~leonardr/launchpadlib/use-beta-service/+merge/19453
<mwhudson> leonardr: done
<leonardr> mwhudson: actually i need you again--i ran the launchpad-based tests but not the unit test, and my change caused a tiny test failure (which is good, imo)
<mwhudson> leonardr: same merge proposal?
<leonardr> mwhudson: yes, i had to change one line in a test in addition to one line in the code :)
<leonardr> i just pushed it
<leonardr> hmm
<leonardr> i'm doing another merge proposal
<leonardr> since, sensibly, the old one stopped covering my changes after you approved it
<mwhudson> leonardr: er no, it just took a while
<leonardr> ah, ok
<leonardr> mwhudson: thanks, all done!
<mwhudson> yay
<thumper> trivial review for someone: https://code.launchpad.net/~thumper/launchpad/fix-2a-description/+merge/19455
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: adiroiban(bug-127171) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adeuring: just otp... I can do a ui review, but if it's not urgent (ie. can wait until this evening) I'd recommend requesting one from sinzui (or checking with him).
<noodles775> As he's wanting to build up his ui reviews, afaik.
<adeuring> noodles775: OK; I'll ask him
<jtv> adiroiban: thanks for a great branch!  Can I land it in the form I reviewed?
<adiroiban> jtv: hi. I have fixed the formatting problems and they should be pushed
<adiroiban> aren't they?
<jtv> adiroiban: then please push!
<adiroiban> let me check what happend
<adiroiban> I can see them here https://code.edge.launchpad.net/~adiroiban/launchpad/bug-127171/+merge/19444
<adiroiban> on the MP page
<jtv> adiroiban: looking... sorry for the confusion, got a bit distracted here
<adiroiban> jtv: don't worry
<adiroiban> better safe, than sorry
* noodles775 changed the topic of #launchpad-reviews to: on call: jtv || reviewing: - || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi jtv, if you're still reviewing, I've got a branch that is just a test refactoring:
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-privatisation-test-refactor/+merge/19482
* henninge changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> noodles775: jtv is unavailable this afternooon
<henninge> I am sorry. ;)
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles,adiroiban(This change renders the AdminDistroSeriesLanguage class from security.py useless, so I have also removed it.] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles,adiroiban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [noodles,adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> henninge: np, thanks for the heads-up!
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [noodles,adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> noodles775: do you still need someone to review your branch?
<noodles775> EdwinGrubbs: yeah, that'd be great.
<EdwinGrubbs> noodles775: are there really 774 other people with the nick "noodles"?
<adeuring> sinzui: thanks for your review!
<sinzui> your welcome.
* allenap changed the topic of #launchpad-reviews to: on call: Edwin, allenap || reviewing: -, - || queue [noodles,adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> EdwinGrubbs: Are you free? If so, can you review an incremental change to a branch? http://paste.ubuntu.com/378410/
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: noodles || queue [adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> allenap: I'm already started on a branch, but it shouldn't take too long.
<allenap> EdwinGrubbs: Thanks :)
<noodles775> EdwinGrubbs: heh, I just didn't like noodles2 (-:
<abentley> noodles775, your nick implies that anyone can execute you.  Have you committed some kind of capital crime?
<noodles775> hah ha.... ha :)
<jml> and the obscure pun of the day award goes too...
* salgado changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: noodles || queue [adiroiban(bug-509252-take-2), salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> EdwinGrubbs, I have a trivial one (https://code.launchpad.net/~salgado/canonical-identity-provider/dont-use-launchpad-login/+merge/18940) for review.  will you have time for it?
<EdwinGrubbs> noodles775: why is the SoyuzTestPublisher needed in doc/archive.txt?
<EdwinGrubbs> noodles775: also, what is the point of this?
<EdwinGrubbs> naked_cprov_archive.sources_cached = 1000
<noodles775> EdwinGrubbs: the SoyuzTestPublisher is used to create consistent publishing records for an archive (without going through the publisher).
<noodles775> It's needed there to populate the private ppa with some packages.
<noodles775> And the point of setting the sources_cached is to show that they are *not* included in the getNumberOfPPA*() calls as the archive is private.
<noodles775> Perhaps the string "Caches for private PPAs are not considered.' needs to be expanded?
<noodles775> (I'm just refactoring the test to pass, I didn't update the text unless it was wrong or something I noted as easy to improve).
<noodles775> EdwinGrubbs: I'm going for dinner, but will check back in case you have any other questions (or pop them on the MP, whatever you prefer). Thanks
<EdwinGrubbs> noodles775: I don't have any more questions. I'll send the review in a couple of minutes.
<EdwinGrubbs> salgado: I can review that. Where did it go? the link doesn't work.
<EdwinGrubbs> salgado: oh, I just don't have permission to view it
<salgado> EdwinGrubbs, can you try again now?
<EdwinGrubbs> salgado: I can view it now, but it doesn't look like I have permission to review it.
<salgado> EdwinGrubbs, I've requested a review from you now
<EdwinGrubbs> salgado: how do you run the tests.
<salgado> EdwinGrubbs, need to merge that branch into lib/canonical/signon and then you can run the tests using bin/test
<EdwinGrubbs> salgado: r=me
<salgado> thanks EdwinGrubbs
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue [adiroiban(bug-509252-take-2), sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> can anyone review a tiny diff to fix the failure in db-devel?
<intellectronica> deryck: maybe you, if you're still around? ^^^ it's only two lines
<deryck> intellectronica, sure.  ping me the MP please.
<intellectronica> deryck: http://pastebin.ubuntu.com/378575/
<intellectronica> i'll create an MP
<intellectronica> basically, i'm committing the transaction to get the trigger that populates the patch age column fired
<deryck> intellectronica, ah, ok.  makes sense.  r=me.
<intellectronica> deryck: thanks!
<deryck> intellectronica, I'll stamp the MP when it's ready if you ping me again.
<intellectronica> deryck: https://code.edge.launchpad.net/~intellectronica/launchpad/test-failure-fix/+merge/19529
<deryck> intellectronica, done.
<intellectronica> deryck: thanks again
<deryck> np!
<rockstar> Edwin-lunch, may I add a small branch to your queue?
<EdwinGrubbs> rockstar: yes, you may
<rockstar> EdwinGrubbs, great.
<abentley> mwhudson, https://code.launchpad.net/~abentley/launchpad/branch-url/+merge/19441
<mwhudson> abentley: noted
<jtv> EdwinGrubbs, time for a big one?  https://code.launchpad.net/~jtv/launchpad/bug-507681/+merge/19531
<EdwinGrubbs> jtv: probably not, but I haven't seen rockstar's branch yet, so it might go quick enough.
<rockstar> EdwinGrubbs, https://code.edge.launchpad.net/~rockstar/launchpad/makefile-hackery/+merge/19533
<rockstar> EdwinGrubbs, mine really is quite small.
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: sinzui || queue [rockstar, adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: sinzui || queue [rockstar, jtv, adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> rockstar: btw you still haven't OK'ed this one: https://code.edge.launchpad.net/~jtv/launchpad/bug-499405-translationtemplates-buildmanager/+merge/17811
<rockstar> jtv, done.
<jtv> rockstar: \o/ thanks
<EdwinGrubbs> rockstar: your branch has conflicts
<rockstar> EdwinGrubbs, fixing now.
<rockstar> EdwinGrubbs, conflict resolved, pushing now.
<EdwinGrubbs> rockstar: r=me
<rockstar> EdwinGrubbs, thanks!
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: jtv || queue [adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-02-18
<thumper> EdwinGrubbs: https://code.launchpad.net/~thumper/launchpad/branch-api-expose-package-bits/+merge/19550 ?
<thumper> EdwinGrubbs: very trivial
<mwhudson> thumper: feel free to review https://code.edge.launchpad.net/~mwhudson/launchpad/unicod-branch-names-bug-449528/+merge/19572 later if you like :-)
<noodles775> on call: noodles775 || reviewing: - || queue [adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue [adiroiban(bug-509252-take-2)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adiroiban(bug-509252-take-2) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> noodles775, I have one with 2500 removed lines; can you take it?
<noodles775> salgado: pop it in the queue :), it sounds *fab* (-:
* salgado changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adiroiban(bug-509252-take-2) || queue [salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adiroiban: hi! thanks for all the cleanups that you included in your branch!
<adiroiban> noodles775: hi.
<adiroiban> everything is ok?
<adiroiban> I should have done those changed one month ago, but by then I didn't know to much about LP
<noodles775> :), I was keen to know more of the background for removing the security check from the model code?
<adiroiban> basicaly, I was cleaning some mess generated be me
<noodles775> Usually it needs to be both on the model and the view as view checks won't be relevant for API calls?
<noodles775> s/to be/to be used
<adiroiban> talking with henninge, and I think there was also an email on lp-dev ml,
 * noodles775 reads
<adiroiban> we decided not to include check_permission code in the model
<henninge> noodles775: the problem is that check_permission does not take a user parameter and assumes the request user. You don't have that in the model.
<henninge> the request
<adiroiban> henninge: maybe we can add a new check_permission_with_user helper
<henninge> adiroiban: no, we just need to add a user parameter to check_permission. Somebody suggested that on the ML, too.
<adiroiban> :)
<adiroiban> or that :)
<adiroiban> noodles775: even in the code that is now in trunk there is no security check in the model
<adiroiban> and they are done in 4 other places in the view
<adiroiban> and the model.distroseries.checkTranslationViewable() is only called from the view
<adiroiban> there is no other place in the model to call it
<noodles775> henninge, adiroiban: So if I'm understanding the email thread correctly, this branch is going *against* what was recommended isn't it?
<noodles775> adiroiban: yeah, I'm just trying to find where that security adapter is used.
<henninge> noodles775: sorry, which email thread?
<noodles775> henninge: RFD: Overhauling the Launchpad authorization adapters
<adiroiban> noodles775: I don't think it is against... as there is no check_permission in the model, or why do you think it is agains the recomandations?
<noodles775> AFAICS, the security check should go on the model, and be called by both the security adapter and the view?
<noodles775> adiroiban: if someone comes along later and exposes this via the API?
<henninge> noodles775: yes, that is the current recommendation.
<adiroiban> henninge, noodles775 , then we have to extend check_permission
<adiroiban> because the current code from the model
<adiroiban> is only doing half of the security checks
<noodles775> adiroiban: if you take a look at lib/lp/registry/interfaces/person.py:addMember()
<noodles775> you'll see it's got a reviewer attribute, which is set to the request user when this is accessed via the API.
<noodles775> Could adding a similar param to your model code check work similarly?
<noodles775> Or would that be dependent on check_permission having the extra param... hrm.
<adiroiban> noodles775: no, because in my case, the security is done using classes from security.py
<adiroiban> I am not sure that addind security related check in the model is a good idea. There must be a good reason why in zope we have the securityproxy, and the security checks are not done in the model layer
<noodles775> adiroiban: did you see BjornT's reply in the email?
<adiroiban> noodles775: this one: https://lists.launchpad.net/launchpad-dev/msg02063.html ?
<noodles775> no, from the more recent thread above... hang on.
<noodles775> adiroiban: https://lists.launchpad.net/launchpad-dev/msg02504.html
<adiroiban> noodles775: no. I'm reading the thread now
<noodles775> Specifically: We already have this. Have the check in model code, and have the security adapter ask the model.
<noodles775> OK.
<adiroiban> noodles775: thanks. then I will have to extend check_permission and then come back to this branch
<noodles775> adiroiban: I'm not sure that you will need to (based on BjornT's reply)...
<noodles775> adiroiban: if you need to include the current user in the permission check, can't you:
<noodles775> 1. add an optional requestor param to the model security check, and then,
<noodles775> 2. Call this directly from the view, and
<adiroiban> noodles775: this will not solve the current problem, as I need to use check_permissions
<adiroiban> in the model
<adiroiban> and check_permission does not accept an user param
<noodles775> adiroiban: ok, I didn't yet see the need to call check_permission from the model code... where is that call?
<adiroiban> and like Barry Warsaw said, adding more security checks in the model is not a good idea for the long term
<adiroiban> noodles775: in my branch, look at check_distroseries_translations_viewable from browser_helper.py
<noodles775> Sure, it's something the security infrastructure should manage, but in cases like these, until it does, moving them to the view has issues too.
 * noodles775 checks
<adiroiban> if you look in the diff, you will see that checkTranslationsViewable() was allwasy called toghere with check_permission
<adiroiban> together
<adiroiban> and when changing check_permission in one place, it is easy to forget about the other place it is called together with checkTranslationsViewable
<noodles775> ?
<noodles775> adiroiban: yeah, so unless I've missed something, all you need to do is update AdminDistroSeriesLanguage.checkAuthenticated() to additionally call distroseries.checkTranslationsViewable()?
<adiroiban> yes... but then distroseries.checkTranslationsViewable() will have to call AdminDistributionTranslations.checkAuthenticated
<noodles775> why? if all callsites use check_permission? (sorry if I've missed something obvious)
<noodles775> (ie. ignore my point 2 above, you'd use the check_permission call in the view as well, as is done currently)
<adiroiban> from my point of view checkTranslationsViewable() is design for an exception
<adiroiban> not for the rule
<adiroiban> and all the ânormalâ rules are in security.py
<adiroiban> this is why I was expecting to see the exception based on the ânormal rulesâ
<adiroiban> and not the ânormal rulesâ based on the exception code
<adiroiban> please let me know if I did not make myself understood
<noodles775> I hadn't thought about it in terms of what is based on what, but rather, how can the code be organised so that the check is in the one place to cover both the view, and other modes of access (such as the API).
<noodles775> s/view/browser view
<adiroiban> the new check_distroseries_translations_viewable is called from the URL traversal code for each object
<adiroiban> so it should also protect the API... or not ?
<noodles775> adiroiban: I hadn't seen the context of that, so yes (I think) it would, but couldn't/shouldn't it use check_permission?
<adiroiban> it does use check_permission
<noodles775> (assuming the above changes.)
<noodles775> No, I mean the other way around, it should call the standard check_permission(), which would call the model's extra permission check.
<noodles775> gar, s/check_permission()/check_permission() directly
<adiroiban> noodles775: Then I will have to move the logic from browser_helpers.py to security.py
<adiroiban> is that what you are saying?
<noodles775> noodles775: no, as per the email, the logic would stay on the model (where it currently is), but be called by the security check in security.py.
<noodles775> adiroiban: ^^
<adiroiban> :)
<adiroiban> but I can not use check_permission() from the model
 * noodles775 thinks it's way past his lunch break - brain is doing crazy things ;)
<adiroiban> no hurry
<adiroiban> we can continue after lunch
<noodles775> adiroiban: OK, that's the thing that I need clarified... I'm not currently seeing why you need to call check_permission from the model code.
 * noodles775 goes to lunch
<adiroiban> enjoy your meal :)
<noodles775> Ta!
<noodles775> So adiroiban, why would you need to call check_permission from the model code?
<adiroiban> because launchpad admins are allowed to see series with hidden translations
<adiroiban> while for other the translations should be hidden
<adiroiban> even if we move that method/function in the model
<adiroiban> it will not be called from the model
<adiroiban> or is URL traversal part of the model?
<noodles775> adiroiban: yes, but the view would be calling check_permission() directly, which would check exactly that (as it currently does) and then additionally call the IDistroSeries.checkTranslationsViewable() where necessary (ie. when they are not an admin).
<noodles775> And the URL traversal would also call check_permission()
<adiroiban> noodles775: ok. I think I got it
<adiroiban> :)
<adiroiban> let me move the code and then show you the diff
<noodles775> OK, I'll summarise that on the MP and mark it as needs fixing... you can then attach the diff to your reply, thanks!
<adiroiban> noodles775: so you want to replace all calls for check_distroseries_translations_viewable from the view
<adiroiban> with just check_permission
<noodles775> Yes, assuming that your check_permission will also call IDistroSeries.checkTranslationsViewable().
<adiroiban> noodles775: it should :)
<adiroiban> thanks!
<noodles775> np!
<adiroiban> noodles775: there is still one detail
<noodles775> yep?
<adiroiban> Danilo told me that the security check was not done in the security.py
<adiroiban> so that the users will see a proper message
<adiroiban> not âaccees deniedâ
<noodles775> Right... hrm...
<adiroiban> my check_distroseries_translations_viewable is doing 2 checks
<adiroiban> one for launchpad.view
<adiroiban> and one for launchpad.TranslatonsAdmin
<noodles775> adiroiban: where is that currently? All I can see are difirent exception strings in checkTranslationsViewable()... which are not permission related?
<noodles775> s/difirent/different ugh.
<adiroiban> in the current code from trunk, the model.distroseries.checkTranslationsViewable is not doing any security checks
<adiroiban> but if you search for the places where it is called
<adiroiban> you will see there is a security check before each call
 * noodles775 looks
<adiroiban> and there is also some delegation from translations.browser.distroseries.checkTranslationsViewable
<adiroiban> which is doing some security checks and the it call registry.model.distroseries.checkTranslationsViewable
* salgado changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adiroiban(bug-509252-take-2) || queue [salgado*2] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adiroiban: so is this an accurate summary: The view code needs to call the model code's check directly as it depends on exceptions being raised for non-admins, to give an appropriate message?
<noodles775> sorry salgado :/
<salgado> noodles775, ?
<adiroiban> noodles775: not sure if is âmodel code's checkâ
<noodles775> salgado: that I still haven't gotten to your first review.
<salgado> oh, no worries
<adiroiban> the view needs to choose between checking for launchpad.View or launchpad.TranslationsAdmin
<adiroiban> and this decision is based on the state of the distroseries
<adiroiban> so yes. the view needs to call/read something from the model
<adiroiban> in order to take the correct decission regarding what permission and error message to display
<noodles775> Right, so simply: if the user isn't a translations admin and translations are hidden, return a meaningful error.
<adiroiban> yes... a meaningful error based on the series status
<noodles775> Yep...
<noodles775> OK, I think you were right to move most of checkTranslationsViewable() to the view, as most of it is not a security check, but error generation. Only the first 3 lines should be part of the security.
 * noodles775 thinks a bit more.
<adiroiban> noodles775: the first 3 lines are in fact http://paste.ubuntu.com/379087/
<adiroiban> i guess
<noodles775> adiroiban: yes, exactly.
<adiroiban> the probles is with this conditional security check
<adiroiban> problem
<adiroiban> it was the same issue that triggered Henning's email in December https://lists.launchpad.net/launchpad-dev/msg02061.html
<noodles775> adiroiban: http://pastebin.ubuntu.com/379097/
<noodles775> if they have admin, they will see it, if they have view, they will see it, if they don't have view permission (because the hidden check would be in the security.py check for launchpad.View), they get the relevant error?
<noodles775> Ah, this then hits the problem that someone with Admin won't necessarily have View?
<noodles775> But that's easy to fix in security.py (by being explicit).
<adiroiban> looking
<noodles775> So we're really missing a DistroSeriesLanguageView class in security.py I think?
<adiroiban> noodles775: DistroSeriesLanguageAdminTranslations should call DistroSeriesAdminTranslation, which should call DistributionAdminTranslations
<noodles775> adiroiban: what's your skype id... might be quicker to discuss this :)
<adiroiban> and you code is also in DistroSeriesNavigation
<adiroiban> and your code is also in DistroSeriesNavigation
<adiroiban> and it should be called from the translation URL traversal for DistroSeries and SourcePackage
<adiroiban> hm...
<adiroiban> let me install skype first :)
<noodles775> yes, it's an example just to separate the security from the error msg.
<adiroiban> the id should be adiroiban
<abentley> noodles775, could you please review https://code.edge.launchpad.net/~abentley/launchpad/restricted-diffs/+merge/19607 ?
<noodles775> abentley: please add it to the queue, if I don't get to it, someone else will.
* abentley changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adiroiban(bug-509252-take-2) || queue [salgado*2, abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> noodles775: please allow me to install and make skype working on my computer, and also have lunch and will come back in about 30 minutes
<noodles775> adiroiban: np, I'll update the MP for the moment, enjoy your lunch!
<noodles775> henninge: can you please take a look at my comment on adiroiban's MP at: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-509252-take-2/+merge/19484
<noodles775> and let me know if it's sane?
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: salgado || queue [salgado, abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775, got another one for your queue... is that too much? https://code.launchpad.net/~jtv/launchpad/bug-507681/+merge/19531
<noodles775> jtv: yeah, I probably won't get to it (as I'll need to come back to an earlier review too), but put it on and we'll see :)
<jtv> noodles775: will do
* jtv changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: salgado || queue [salgado, abentley, jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> noodles775: back. skype test was working, skypeid adiroiban
<noodles775> salgado-lunch: when you're back, are there further branches coming? If I run a local server, I can't login...
<noodles775> adiroiban: great... one tic.
* bigjools changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: salgado || queue [salgado, abentley, jtv, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> abentley: everything's stuck on 507681 now... can I get you to review it?
<jtv> abentley: this mp: https://code.launchpad.net/~jtv/launchpad/bug-507681/+merge/19531
<abentley> jtv, is there a reason you want me specifically?  Otherwise, I'd rather leave it to the OCR.
<jtv> abentley: one of the wellington gang would be nice.  Plus, OCR is backlogged, and this way you get to unblock your own branch-url branch.  :-)
 * jtv watches shouting mobs outside...  must be a big football match on
<jtv> here comes the police...
<jtv> ...and the riot police vehicle.  Sport at its best!
<bigjools> jtv: it didn't happen without pictures
<jtv> damn
<jtv> hang on
<abentley> jtv, I'll see.  I've got a lot going on right now.
<jtv> bigjools: the action seems to have shifted out of range.  Thank you, Erwin Rommel, for popularizing the meeting engagement.
<jtv> abentley: I'll shop around a bit more.
<bigjools> does Godwin's law apply here already?
<jtv> bigjools: it doesn't work that way.
<jtv> bigjools: but since we're talking, can I get you to review a buildfarm branch that we have several other branches blocked on?
<bigjools> jtv: how big?
<jtv> bigjools: see for yourself... https://code.launchpad.net/~jtv/launchpad/bug-507681/+merge/19531
<bigjools> jtv: might be able to look a bit later
<bigjools> in an hour or so
<jtv> bigjools: I would be indebted to the tune of several good beers
<abentley> jtv, the diff has conflicts.
 * bigjools will hold you to that at the next epic
<jtv> bigjools: let's hope it's in Belgium.
<bigjools> ah you're going to uds?
<jtv> abentley: thanks for pointing that out!
<jtv> bigjools: no plans, no
<bigjools> oh, beers... belgium....
<bigjools> I prefer ale from England ;)
<salgado> noodles775, what happens when you try to login?
<jtv> bigjools: I only recently had the chance to learn about proper ales
<jtv> fixing the conflicts now...  that's what I get for updating copyright dates.
<noodles775> salgado: DiscoveryFailure: HTTP Response status from identity URL host is not 200. Got status 503<br />
<jtv> ah, sirens now
<salgado> noodles775, when people review one of these branches I always forget to mention they need to add testopenid.dev to /etc/hosts
<salgado> sorry
<salgado> noodles775, btw, flacoste has just approved that branch of mine
<noodles775> salgado: great.
<abentley> jtv, with pipelines, the recommended way to merge is to merge into the first pipe, then run 'bzr pump'.
 * noodles775 checks the size of abentley's branch.
<jtv> abentley: just what I was doing... I'm getting used to pipelines already
<noodles775> Nice.
<abentley> jtv, cool.  Then you can check out pump --from-submit next time, which automates it even further.
<flacoste> salgado: i wanted to share the pleasure of remoing 2k lines!
<jtv> abentley: oh!  That's nice.
* noodles775 changed the topic of #launchpad-reviews to: noodles775 || reviewing: abentley || queue [jtv, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> flacoste, it felt good indeed. :)
<henninge> noodles775: I will take care of Adi's branch.
<noodles775> henninge: thanks.
<noodles775> abentley: your branch looks great. The only question I have is whether there's really a need to include each segment of the url in the doctest: http://code.launchpad.dev/~person-name.../product-name.../branch.../+merge/.../+preview-diff/+files/preview.diff
<abentley> noodles775, I would be fine with just showing "/+preview-diff/+files/preview.diff".  Would you prefer that?
<noodles775> abentley: yeah (not that I care much either way, it'd just mean it was within 78chars :))
<jtv> kfogel, care to review a wording change you suggested?  https://code.launchpad.net/~jtv/launchpad/bug-299008/+merge/19628
<kfogel> jtv: looking  (but I'm not officially a reviewer)
<jtv> kfogel: I don't think anybody would object in this case
<kfogel> jtv: I'll just note that in my review so it's all above board, np.
<jtv> kfogel: thanks
<kfogel> jtv: heh, nice note
<jtv> :)
<kfogel> jtv: approved
<jtv> kfogel: sheishei
<jtv> kfogel: "ää"?
<kfogel> jtv: :-)  modern romanization: "xie xie"; older romanization system: "hsieh hsieh"; actual pronounciation "shuh-yea shuh-yea" :-)
<kfogel> jtv: hunh, my xchat isn't displaying that right, unfortunately
<jtv> kfogel: China Airlines, apparently, is not the most precise of Mandarin schools.
<jtv> kfogel: wrong character set... never mind
<kfogel> jtv: odd, because their academic reputation is excelelnt
<jtv> gotta be better than their flight safety reputation :)
<jtv> (never had any trouble myself, but istrm hearing something about that)
* noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jtv, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> thanks noodles775, enjoy your evening!
<noodles775> jtv: was that sarcasm? I didn't get to your branch ;) You too!
<jtv> noodles775: no sarcasmâat least I'm at the front of the queue now, when there was a fair backlog ahead of me earlier
<gary_poster> uh. nobody on call.
<leonardr> rockstar: are you available to review a multiversion branch?
<rockstar> leonardr, yes.
<leonardr> gary: if it's something i can take, i can review while my own branch is in review
<leonardr> rockstar: ok, on the way
<rockstar> gary_poster, I am OCR today, but had a personal errand this morning.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: leonardr || queue [jtv, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> leonardr, rockstar: ok thank you.  I got it taken care of I think.
* bigjools changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: leonardr || queue [jtv, bigjools*2] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> rockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/launchpad-integration/+merge/19639/
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: bigjools || queue [jtv, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: bigjools || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * rockstar cooks through the backlog of reviews
 * abentley reminds rockstar of the UI review
<rockstar> abentley, yup, I know.
<rockstar> abentley, ui=rockstar for you
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar, thanks!
<abentley> rockstar, what do you think about the two shades of yellow?
<rockstar> abentley, well, I'm not sure if it really matters.
<rockstar> abentley, maybe the diff message should be blue, indicating "informational" but yellow also seems to indicate "stale" so I'm a little conflicted.
<rockstar> I didn't think it was a big enough deal to even worry about.
<rockstar> If someone complains, then we can re-evaluate.
<abentley> rockstar, I didn't actually get that "yellow means stale".  I thought it meant "note".
<rockstar> abentley, I think it means "caution" but when I was looking at the comments, I almost felt like it was like old yellowed paper.  I don't know.
<abentley> rockstar, Anyhow, we might want to look at settling on one shade of yellow.
<rockstar> abentley, yeah, but I don't think it's a huge issue, so don't go out of your way for it.
<EdwinGrubbs> bac: can you look at the incremental diff I added to https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-99395-linking-sourcepackages-to-projects/+merge/19429
 * bac looks
<bac> looks good EdwinGrubbs
<EdwinGrubbs> thanks
* sinzui changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> rockstar: do you have time for a short branch: https://code.launchpad.net/~sinzui/launchpad/packaging-timeout-bug-523886/+merge/19660
<rockstar> sinzui, for you, sure.
* rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-02-19
<mwhudson> thumper: can you review https://code.edge.launchpad.net/~mwhudson/launchpad/reduce-concurrent-job-count/+merge/19574 pls?
 * thumper looks
<thumper> mwhudson: are you happy with the value 3?
<thumper> mwhudson: I'm wondering what we'll do with a new quad core machine soon
<mwhudson> thumper: yeah, that did occur to me too
<thumper> mwhudson: anyway, happy for this to go in.
<thumper> r=me
<mwhudson> thumper: maybe i should change it to be a paramter to getJobsForMachine, and have the slave pass it in
<mwhudson> that's much easier to change via cp
<thumper> yeah
<thumper> can we have it as a command line param?
<thumper> or at least have a command line override the default if supplied
<thumper> that way we can keep this config variable
 * thumper runs to collect Rachel and a coffee
<mwhudson> thumper: want to review incremental-code-imports-bug-512683 ?
<thumper> mwhudson: sure
 * mwhudson boggles
<mwhudson> i can't type new lines into this textarea
 * mwhudson restarts ff, it works now
<mwhudson> stupid free software crap
<mwhudson> <wink>
<mwhudson> thumper: you have mail
<thumper> ok
<thumper> mwhudson: damn out of order emails
<mwhudson> thumper: :-)
<thumper> mwhudson: is 5k too much?
<thumper> mwhudson: how many on the kernel before it goes slow?
<mwhudson> thumper: ot
<mwhudson> it's a bit of a guess, to be sure
 * mwhudson goes to look at some log files
<mwhudson> it probably is too many, takes about 12 hours for the kernel?
<mwhudson> 2500 took about 6 hours
<thumper> 1000?
<mwhudson> 1000 about 1hr 30
<thumper> lets use that
<mwhudson> thumper: ok
<thumper> mwhudson: I don't have an issue with an intial git import taking 20 or 30 partial successes
<thumper> mwhudson: is this mainline revisions or total revisions?
<mwhudson> thumper: total
<mwhudson> thumper: linux is 168 k :-)
<thumper> hmm...
<thumper> well, worth a crack nigel
<mwhudson> it'll be easy to chage with a cowboy
<mwhudson> you could even do a time based limit i guess
<mwhudson> though that'd require changing bzr-git again
<thumper> mwhudson:  in test_worker don't you want TestCaseWithFactory rather than just TestCase?
<mwhudson> thumper: no, these tests don't interact with the database
<thumper> mwhudson: ok
<mwhudson> thumper: small changes pushed btw
<mwhudson> (i forgot to commit some self-review)
<mwhudson> thumper: thanks for the review
<thumper> mwhudson: np
<thumper> it is Friday afternoon
<mwhudson> thumper: do you think we should get a UI review?
<thumper> short answer: no
<mwhudson> ok
<thumper> we may want to get someone to look at it later
<thumper> but we shouldn't block on it
<mwhudson> yeah ok
<thumper> the actual ui change is almost unnoticable
<thumper> and only visible in certain early circumstances
<thumper> noticable by us, but probably not too many others
 * mwhudson ec2 lands
<mwhudson> thumper: can you look at https://code.edge.launchpad.net/~mwhudson/launchpad/reduce-concurrent-job-count/+merge/19574 again?
<thumper> ok
<mwhudson> ta
<thumper> mwhudson: email coming your way
<mwhudson> heh, that's what i get for not writing tests i guess
<mwhudson> *running
<mwhudson> (also coding on a friday afternoon)
<thumper> https://devpad.canonical.com/~tim/description1.png
<thumper> https://devpad.canonical.com/~tim/description2.png
<thumper> https://devpad.canonical.com/~tim/description3.png
<thumper> https://devpad.canonical.com/~tim/description4.png
<thumper> https://devpad.canonical.com/~tim/description5.png
<thumper> mwhudson: ^^
<thumper> comments?
<mwhudson> thumper: why a private server?
 * mwhudson looks
<mwhudson> bah
<thumper> mwhudson: because it is easy?
 * mwhudson wants focus follows eyes
<mwhudson> fair enough
<thumper> I could have copied them to penhey.net...
<mwhudson> train your fingers to type people.canonical.com instead :-)
<thumper> I have people.canonical.com?
<mwhudson> yes
<thumper> where?
<mwhudson> tim@
<thumper> or is it devpad?
<mwhudson> same as everything else
<mwhudson> no
<mwhudson> it used to be rookery, but it's another machine now
<mwhudson> lillypilly or something
<thumper> heh, first time I've logged in to there
<thumper> ok all on https://people.canonical.com/~tim/description*.png too
<mwhudson> thumper: do you have a screeny of the description in the lower position when both description and commit message are set?
 * thumper makes one
<mwhudson> thumper: boring one for you
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/bzr-2.1-gold/+merge/19678
<thumper> http://people.canonical.com/~tim/description6.png
<thumper> mwhudson: 6400 lines isn't trivial :)
<thumper> mwhudson: proposed against db-devel?
<mwhudson> thumper: crap
<mwhudson> yeah
<thumper> mwhudson: I want to be able to have an editor to change that
<thumper> mwhudson: to save the whole delete, recreate screwage
<mwhudson> thumper: yes please
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/bzr-2.1-gold/+merge/19679 then
<thumper> done
<mwhudson> three things in ec2
<mwhudson> might be enough for today...
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adeuring: was that ^ meant to go into the subject? ;-)
<adeuring> henninge: argh, yes...
 * adeuring needs more coffee
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adeuring: cool, that means you have time for my branch! ;-)
<adeuring> henninge: sure, but first more coffee ;)
<henninge> adeuring: whatever you need ... ;)
<henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-523810-needs-information-age/+merge/19689
* henninge changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adeuring: thanks
<adeuring> henninge: review sent. I have a few questions
<henninge> adeuring: thanks, I'll look at it
<henninge> adeuring: about the placement of the constants in the inteface module.
<adeuring> henninge: yes?
<henninge> adeuring: I remember a rule that browser and test code should not import from model code.
<adeuring> henninge: yes, you're right
<henninge> adeuring: I have to admit, I have not found anybody with that same memory yet .... ;)
<henninge> adeuring: oh, there you are! ;-D
<henninge> adeuring: that's why I moved this into the interface module.
<adeuring> ok, so let's leave it there
<henninge> adeuring: also, I consider these expiration times as implementation-independent.
<adeuring> ok
<henninge> and that is the idea of the interface-model split, isn't?
<adeuring> henninge: in general, yes. But it is a bit ober-bureaucratic in ths case, IHMO ;)
<adeuring> s/ober/over/
<henninge> ;)
<henninge> adeuring: but I don't see a pressing reason not to have this in the interface module
<adeuring> right
* henninge changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adeuring: replied ;)
 * adeuring is looking
<adeuring> henninge: r=me. Thanks for the changes!
<gmb> adeuring: Do you have time to review https://code.edge.launchpad.net/~gmb/launchpad/link-attachment-bug-524296/+merge/19694 ?
<henninge> adeuring: thank you
* gmb changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> gmb: r=me, one small additional request
<gmb> adeuring: Thanks.
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews abgeÃ¤ndert
<adeuring> gmb: welcome :)
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews abgeÃ¤ndert
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi guys, one for the queue: https://code.launchpad.net/~michael.nelson/launchpad/ppa-privatisation-test-refactor2/+merge/19696
<noodles775> ... if you don't mind me being gone for lunch :)
<adeuring> noodles775: I'll have lunch too soon, but after that i can take it.
<bac> hi adeuring, salgado: can i add https://code.launchpad.net/~bac/launchpad/bug-487793/+merge/19700 to the queue?
* bac changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [noodles,bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> bac, sure; I'll take it
<bac> thx
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,noodles || queue [bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,bac || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Thanks for the review salgado
<noodles775> Sheesh, I've no idea what the garbage in the testing factory...
<adeuring> salgado: can you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-201015-bug-branch-search/+merge/19699?
<salgado> adeuring, sure.  would you like to take bac's so that I go straight to yours?  otherwise I'll do yours after I finish his
<adeuring> salgado: sure, np
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> salgado: so the stuff that looks like garbage in factory.py is actually in trunk, it's a sample diff used by some factory methods. My editor automatically removed some trailing whitespace from it.
<salgado> oh, I see.  the lines starting with a white space followed by a '+' are just context to your diff
<noodles775> Yep.
<salgado> cool
<jtv> salgado, one more for the queue: https://code.launchpad.net/~jtv/launchpad/bug-523926/+merge/19704
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> thanks
<salgado> adeuring, did you ask for a UI review of your branch?  a UI reviewer might have other ideas for where to place the new radio button
<adeuring> salgado: no, not yet...
<salgado> adeuring, is there a compelling use case for including only the bugs *without* a linked branch?  I don't see a similar option for including only bugs without patches/CVEs
<salgado> disclaimer: I haven't read the bug report yet
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> salgado: well, the wish is mentioned in the bug report ;)
<adeuring> salgado: and a use case is: "look for bugs that seem to be uninished", for example
<salgado> adeuring, right, but having support for that makes the radio button necessary, thus adding some complexity and making for a not-very-clear UI
<adeuring> right
<salgado> I'd rather address just the most common use case (searching only for bugs with linked branches), which would allow for a simpler UI.  and later, if necessary, we can extend it
<salgado> but if we can come up with a simple UI that accommodates both use cases, I'd be ok, I think
<salgado> although we might be adding a feature that may never be used
<salgado> adeuring, did you have a pre-implementation call?
<adeuring> salgado: no...
<adeuring> salgado: shall we also ask the ui reviewer about the "no branches" option?
<salgado> adeuring, sure, but it might be a good idea to first decide whether or not it's worth allowing users to search for bugs without branches
<adeuring> salgado: well, one option is to search fro bugs which are "unfinished", even if this simply means that a branch in fact exists but is not linked.
<adeuring> We have the use case to better suport upstream
<adeuring> so being able to provide upstream with branches for bugs seems to be useful
<salgado> adeuring, can you bring that up for discussion with deryck?  I think you guys are better equipped than myself to decide whether or not it's useful
<adeuring> ok
<salgado> I was just trying to ensure we discussed it before going any further, to avoid having to rework later or trying to simplify a UI that's may not be necessary
 * deryck looks at the initial bug report
<deryck> adeuring, salgado -- I haven't looked at the UI for this, i.e. haven't branched and run this myself, but if the UI is horrible to enable the negative, I would lean toward just searching for bugs *with* branches.
<adeuring> deryck: let me make a sreecnshot...
<deryck> adeuring, salgado -- hearing you guys discuss this, though, I do wonder why a radio button is required.  maybe a "with branch" and "without branch" check box, and just form validate that only one is selected.  Or else do nothing different than the default. :-)
<deryck> adeuring, ok, will wait on screenie
<adeuring> deryck: an interesting idea ;)
<salgado> deryck, that wouldn't be a great UI either, I think
<adeuring> deryck, salgado: http://people.canonical.com/~adeuring/branchsearch.png
<deryck> adeuring, salgado -- so I don't think the radio is bad.  I think it should be made to look like the affects option -- a check for "show with branches" (or some better phrase) and then "with linked branch" and "without linked branch" toggle.
<deryck> we shouldn't have to have it toggled on for the default.  but admittedly, I'm thinking only UI here and not what's involved to code this.
<salgado> ain't that how we should think when designing the UI? ;)
<adeuring> deryck: nice idea!
<deryck> heh
<deryck> salgado, adeuring -- of course, this is just an idea.  I'm not a qualified UI reviewer. :-)  I would pre-imp with a ui person.
<adeuring> deryck: but... I think I know what you mean, but the "any" and "all" buttons below "show bugs affecting me" relalte to the "tags" field on left ;)
<salgado> yeah, it took me ages to realize that
<deryck> yeah, so it's not a perfect idea.  and the grouping of the options could generally be better.
<salgado> adeuring, care to check with a UI reviewer what they think of these two options?
<adeuring> noodles775, sinzui: ^^^^
<sinzui> adeuring: noodles775: I can do it in 30 minutes
<noodles775> OK, thanks sinzui.
* BjornT changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv, sinzui, bjornt] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> adeuring, salgado: here's a small patch for you to review when you have time: https://code.edge.launchpad.net/~bjornt/launchpad/quiet-update-sourcecode/+merge/19710
<adeuring> I'll look at it
<BjornT> thanks
* sinzui changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv, sinzui, bjornt, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> BjornT: r=me
* adeuring changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, adeuring || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> thanks adeuring
* bigjools changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, adeuring || queue [sinzui, sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> rockstar: can you check if I understood you correctly?  https://code.edge.launchpad.net/~jtv/launchpad/bug-523449/+merge/19622
<jtv> rockstar: and its sibling, https://code.edge.launchpad.net/~jtv/launchpad/bug-507681/+merge/19531
<sinzui> adeuring: I am sorry my review is taking so long. bugs advanced search has many ui bugs that have irked me. Instead of doing a simple review, I am looking at why this page is different from other forms and will propose a fix instead of asking you to find the fix
<adeuring> sure, sounds good!
<rockstar> jtv, so basically, you kept moved some work into the second branch that could have been in the first?
<jtv> rockstar: yes
<jtv> rockstar: I moved the "if work_is_appropriate: work()" structure into the utility.
<jtv> Could've done better, I know.
<rockstar> jtv, it's fine, was just a bit confusing.
* salgado changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, adeuring || queue [bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> bigjools, around?
* salgado changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, bigjools || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> adeuring, I'll get back to yours once we've heard from sinzui, ok?
<adeuring> salgado: sur
* adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, bigjools || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> adeuring: salgado: I am almost done. I have discovered that advanced search is not just a handcrafted form, all the markup and css rules for legend were updated for all forms except this one. I think I can propose a fix
<adeuring> sounds good
<salgado> cool; that's great
<bigjools> salgado: yes
<salgado> bigjools, I was just wondering if it'd be possible to use a newly created distribution in that test, to have it not depending on any sample data
<bigjools> salgado: that would be quite a lot of work :/
<salgado> I imagined
<bigjools> the other tests in the file all do the same thing
<salgado> bigjools, do we need to have the suffixes hard-coded there?  can't we take them from a constant somewhere?
<bigjools> salgado: that's the only place they will appear - is there a benefit to doing that?
<salgado> in that case, no
<salgado> approved. :)
<bigjools> one of these days we'll get rid of the sampledata crack for Soyuz
* salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> salgado: cheers
<bigjools> salgado: I'd love to replace this particular sample data with a function that will set it up instead
<bigjools> check out lucilleconfig if you want to see why that's tough :)
<salgado> I believe you it's tough, and that's why I'm not convinced we'll ever manage to get rid of sampledata for soyuz/registry/bugs tests
<bigjools> well tough doesn't mean impossible :)
<bigjools> it will just take a very long time :/
<jtv> rockstar: any chance of finishing the review today?
<rockstar> jtv, which review?
<jtv> rockstar: oh, I since you commented, I thought you were reviewing my branch.
<jtv> well, and since you promised to look at the changes.  :-)
<rockstar> jtv, haven't I approved everything outstanding?
 * rockstar looks
<jtv> rockstar: ah, so you did!  Thanks.  Whole lotta landing coming up...
<rockstar> abentley, https://code.edge.launchpad.net/~rockstar/launchpad/bug-517266/+merge/19730
 * maxb would like to enqueue https://code.edge.launchpad.net/~maxb/launchpad/bug-497731/+merge/19732
<salgado> maxb, I'll take it
<maxb> thanks
<salgado> maxb, is get_bzr_plugins_path() still used anywhere?
<maxb> Nowhere outside that file. I was wondering whether to remove it from __all__
<salgado> yeah, that's why I asked.  maybe we should just change it instead of having a separate one?
<salgado> after all, the only behaviour we want is the one of get_BZR_PLUGIN_PATH_for_subprocess(), right?
<maxb> It wouldn't even suck to just inline get_bzr_plugins_path at its two call-sites in lp/codehosting/__init__.py
<maxb> I agree I can't imagine why any code other than the load_plugins([...]) call itself should want the directory alone
<salgado> I'd be against inlining, but I'm all for keeping just one version of it -- the one that does the right thing. :)
<maxb> So your suggestion is to just drop it from __all__ ?
<salgado> no, just remove get_BZR_PLUGIN_PATH_for_subprocess() and change get_bzr_plugins_path() to append a "-site" to its return value
<maxb> That's not acceptable because the bare directory is required for the load_plugins([...]) call ~10 lines below
<salgado> oh, I missed that
<salgado> why is it required there?
<maxb> The way it works is that if you're calling that bzrlib call directly, bzrlib uses *exactly* the directories you pass it. If you're setting the environment variable, bzrlib augments it with the standard directories unless you use the magic tokens to tell it not to
<salgado> I see
<sinzui> bac: I approved the UI, but you need to fix the links first.
<bac> sinzui: i'm just reading it now
<sinzui> EdwinGrubbs: I just used the link to upstream multi-step form. It is very nice. It feels faster than the series picker. I think that has a lot to do with the design shown only the information you need to know
<salgado> maxb, then we can rename the existing one to _get_bzr_plugins_path() and remove it from __all__
<maxb> Sounds good
<salgado> maxb, also, it'd be nice to state in the new docstring why we use the "-site" magic token
<EdwinGrubbs> cool
<salgado> 65	             args, env_changes={'BZR_SSH': 'paramiko',
<salgado> 66	-                               'BZR_PLUGIN_PATH': get_bzr_plugins_path()},
<salgado> 67	+                'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()},
<salgado> maxb, can you also align the two dict keys in there too?
<maxb> pull the 'BZR_SSH' down onto a new line?
<salgado> yeah, that's what I'd do
<maxb> "The '-site' token tells bzrlib not to include the 'site specific plugins directory' (which is usually something like /usr/lib/pythonX.Y/dist-packages/bzrlib/plugins/) in the plugin search path, which would be inappropriate for Launchpad, which may be using a bzr egg of an incompatible version." ?
<salgado> sounds good to me
<maxb>         return self.run_bzr_subprocess(args,
<maxb>             env_changes={
<maxb>                 'BZR_SSH': 'paramiko',
<maxb>                 'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()
<maxb>             },
<maxb>             allow_plugins=True, retcode=retcode)
<maxb> Is that formatting acceptable?
<salgado> it's not -- the two arguments to run_bzr_subprocess() should be aligned
<maxb>         return self.run_bzr_subprocess(
<maxb>             args, env_changes={
<maxb> ?
<salgado> that works, yes
<salgado> args, env_changes={
<salgado>     'BZR_SSH': 'paramiko',
<salgado>     'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()
<maxb> ok, pushed
<EdwinGrubbs> sinzui: I didn't see an email from you on the mockups. Do you want to discuss that today or wait till Monday?
<sinzui> EdwinGrubbs: sorry. I am still doing reviews. I do need to send an email because I need to collect my thoughts
<bac> sinzui: thanks for catching the link issue.  here is a screenshot of the revision.
<bac> http://people.canonical.com/~bac/link-portlet-fixed.png
<sinzui> bac: That is lovely
<bac> i think that's a bit of a stretch but i'll take it and run off to ec2
* salgado-afk changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: -, - || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: Edwin-lunch: I have spent 7.5 hours doing just UI reviews and proposals. I am burned out.
<bac> sinzui: and you should be burned out
<EdwinGrubbs> sinzui: well, I hope you enjoy your weekend.
<sinzui> I wont. I am told I am attending multi-cultural night at my daughter's school and there will not be any food from asian :(
<sinzui> My youngest daughter caught my wife playing tooth faerie. I need to hack her computer to destroy the evidence
<sinzui> This same daughter just dressed the kittens in doll clothes
<sinzui> My son who  must has Asbergers just found all the puzzles I hid and is assembling them in my bedroom
<bac> sinzui: smuggle in some sriracha and put it on everything
<sinzui> well, I think that is an excellent idea. I will report back
<rockstar> Can anyone do a really simple review for me?
<rockstar> bac? ^^
<bac> rockstar: yes.  i'm a very simple reviewer.
<rockstar> bac, creating mp now.
<rockstar> bac, https://code.edge.launchpad.net/~rockstar/launchpad/js-no-bigger-than-512k/+merge/19751
 * bac looking
<bac> hi rockstar
<rockstar> bac, hi.
<bac> it looks good but i have three comments
<rockstar> bac, shoot.
<bac> 1 - add #!/usr/bin/python
<rockstar> bac, ah, yes.
<rockstar> Shouldn't it be python2.5 ?
<bac> 2 - dude, it's mid-february, start using 2010 already
<rockstar> Ugh, c-n-p error.
<bac> 3 - should max size be 512 * 1024 ?
<rockstar> bac, it could be, yeah.  I didn't think it was that big of a deal, but I'll changed it.
<bac> ok, cool
<bac> i'll add those to the MP and approve it
<rockstar> bac, fixes pushed.
<bac> done
#launchpad-reviews 2010-02-20
<wgrant> Any reviewers still around for a trivial API export?
<wgrant> https://code.edge.launchpad.net/~wgrant/launchpad/export-das-chroot/+merge/19759
#launchpad-reviews 2010-02-21
* jpds changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: -, - || queue [adeuring, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/no-project-in-ui-bug-409655/+merge/19828
<thumper> done
<mwhudson> ta
#launchpad-reviews 2011-02-16
<stub> lifeless: 'An iterable of slices' seems rather overkill, and possibly self defeating. What is the advantage of this over just (min_index, max_index) ?
<lifeless> stub: do you mean just special case the exact situation?
<stub> Do we ever see more than one slice passed in?
<lifeless> yes
<stub> If so, are the holes almost always a single comment?
<lifeless> the slices are
<lifeless> [:40], [-40:]
<lifeless> more or less
<stub> ok. That makes sense then.
<stub> If the holes were small, we could be better off pulling them and skipping unwanted ones client side than the more complex WHERE clause.
<lifeless> yeah
<lifeless> the prototypical case is bug 1
<_mup_> Bug #1: Microsoft has a majority market share <iso-testing> <ubuntu> <Clubdistro:Confirmed> <Computer Science Ubuntu:Invalid by compscibuntu-bugs> <EasyPeasy Overview:Invalid by ramvi> <GNOME Screensaver:Won't Fix> <Ichthux:Invalid by raphink> <JAK LINUX:Invalid> <The Linux OS Project:In Progress> <metacity:In Progress> <OpenOffice:In Progress by lh-maviya> <Tabuntu:Invalid by tinarussell> <Tivion:Invalid by shakaran> <Tv-Player:New> <Ubun
<lifeless> where the hole is 1200 rows long
<lifeless> stub: https://bugs.launchpad.net/launchpad/+bug/607935/comments/12
<_mup_> Bug #607935: timeout on bugtask:+index <lp-bugs> <pg83> <timeout> <Launchpad itself:Triaged> < https://launchpad.net/bugs/607935 >
<lifeless> stub: doing two queries is 40ms total; one with the complex clause 30ms total
<stub> set(row[1].ownerID for row in rows if row[1].ownerID)
<lifeless> rather than the discard? its slower
<lifeless> doing a discard is one check for None, discarding in the loop is a check per row
<stub> Any reason you want to keep eager_load_watches() in there?
<lifeless> oh
<lifeless> thinko, will delete.
<stub> lifeless: Hmm... if you do that, doesn't that mean build_comments_from_chunks() will be loading bugwatches one at a time from the DB?
<lifeless> no
<lifeless> well
<lifeless> we use that in one place only
<lifeless> and that place is a bug context
<lifeless> bugs have already preloaded all bug watches.
<lifeless> eager_load_watches is already commented out to be uncalled
<stub> Right. I've asked for a comment anyway - when I see code like that I assume database impedance mismatch issues and one-query-per-iteration performance because I don't know the call path.
<stub> And tests for slicing behavior.
<lifeless> fair enough
<lifeless> that method is /barely/ tested already; I'd like to nuke it entire for indexed_messages
<lifeless> but ETIME
<lifeless> I'll add some tests tomorrow morning
