[01:19] Morning folks. [02:28] hi spiv [02:28] my turn to flip from waking at 3 to waking at 10 :) === idnaria is now known as idnar [02:34] Heh [05:44] lifeless: i don't totally understand your comment in https://code.launchpad.net/~gagern/bzr/bug421776-sftp-move/+merge/62708 [05:44] Ubuntu bug 62708 in bluefish (Ubuntu) "Please sync bluefish_1.0.6-1 from Debian unstable (main)" [Wishlist,Fix released] [05:44] poolie: we don't know the provenance of the existing file [05:45] hm [05:45] we can safely assume it came from another bzr process? [05:45] we don't know whether that process is still running, dead, etc [05:45] right [05:46] so...? [05:47] I would be wary of assuming the file matches what we're uploading [05:47] if it exists, the process that wrote it thought it was correct at the time it finished writing [05:47] it may have been wrong (buggy) [05:47] or there might have been file corruption [05:48] true. And either that process failed to link it in, failed to unlink it, died, or raced with us on linking it in and did so immediately before we last read the pack list [05:48] but, those possiblities don't seem any more likely here than for any other existing pack file [05:48] well [05:48] Call me conservative :) [05:49] if you're asserting for example "a race means it's more likely the file suffered corruption" that's fine, i'm happy to consider it [05:49] i just want to understand what you're saying [05:50] I couldn't really put a probability on it [05:50] but I would worry about trusting something that may have crashed when we don't have to [05:51] ok [05:51] for instance [05:52] if the machine powered off, without ordering between file renames etc, we might have a zero length file with the right name, not linked into pack-names [05:53] yes, that's very possible [05:53] it's also possible it would end up showing it mentioned in pack-names and yet also have the wrong content [05:53] yes [05:53] so, being extra careful in this case seems a bit odd [05:54] uhm [05:54] this doesn't seem like being extra careful [05:54] you say "we should check if the existing file is intact" [05:55] once its in pack names (in the absence of attackers) we know that the prior bzr process completed [05:55] i don't understand why we would do that over sftp but not locally [05:55] we don't know if the fs went awol or not [05:55] poolie: we would do it locally too, wouldn't we ? [05:55] hang on, "we know that the prior bzr process [05:56] completed" --> that's not true if the machine crashed without an ordered journal of renames [05:56] aiui this bug is sftp specific, about the general pack upload code not working the same way over sftp [05:59] imbw [05:59] i'm going to read the general pack code [05:59] poolie: so IIRC locally we rename over the top [06:00] we resolve by assuming our new file is correct - which is the ultimate form of lack-of-trust [06:00] so this case (which for -very- narrow windows win32 might display too) is when we cannot atomically rename over the top [06:01] and we don't have a safe rename-out-of-the-way dance that others could use to handle the file-is-temporarily-missing case [06:01] right, we do [06:03] ok, so using .move (which really means, force replace) would be most consistent with what we do locally [06:03] yes [06:04] I was suggesting a verification scan (for the pack and the indices) as a workaround to deal with the race condition. [06:04] in other words you like his current patch? [06:04] uhm, I think its evolved since I commented [06:05] yes [06:05] perhaps rather than delete-and-replace we should move the existing pack into the obsolete directory, so if we're interrupted after deleting it, we won't lose [06:05] so move has this race condition [06:06] I don't particularly like that :) [06:06] I don't want to block anyone [06:06] I was trying to highlight the interacting constraints [06:07] don't like moving it to obsolete-packs? [06:07] thanks for the review [06:07] i do appreciate it [06:07] am just trying to understand it :( [06:07] s//:) [06:08] so there is actually an sftp rename operation argument saying what behaviour you want [06:08] ideally we'd make it behave like a local fs :) [06:09] hm [06:10] seems there is possibly a small race on windows too, if we delete then rename [06:16] ok, thanks for that [06:17] its possible to address the race by having a predictable path to look for fallback files [06:17] or even just spin on the path in clients [06:17] in the hope it will appear again soon? [06:18] right [06:18] right [06:18] they're probably separate bugs [06:18] but if its linked in pack-names [06:18] I think you need 2 paths - a normal one and a before-naming-into-place one [06:18] for here, i think asking sftp to overwrite, and forcing the overwrite, is probably safe [06:18] so that repo recovery can do something sensible [06:18] s/safe/an improvement [06:18] I think we have retry-on-missing-packs now [06:18] I worry about a network interrupt during that race though [06:19] how will the repo be recovered [06:19] right, i'm most concerned by the connection dropping (or the server crashing) after it applies the deletion but before the new file moves into place [06:26] spiv, teddy, should i actually do a patch for jml's try_import? [06:26] it does clean things up and it should make pylint cleaner [06:27] bit of an optional yak though [06:27] (there are something like 100 'except ImportError' catches at present) [07:20] poolie: after it deletes but before it renames, is probably before we write pack-names [07:20] I was concerned at one point, but now... locally we overwrite on posix systems [07:20] and I think "crash" on Windows [07:20] it seems fine to overwrite on remote systems *without* validation [07:20] becuase we justuploaded content *we* are happy with [07:20] who cares if there is unreferenced garbage that we get rid of [07:20] corrupted or not [07:21] we aren't re-instating the pack file that we think was corrupted [07:21] "might have been" [07:21] right [07:21] we are putting the new hottness there [07:22] right [07:22] hello btw [07:22] The only race I can think of is 2 processes both renaming into place, and being ready to update pack-names. p1's rename succeeds so it goes to write pack-names [07:23] p2 sees the collision, renames out of the way [07:23] then p1 writes pack-names, and the machine crashes [07:23] before p2 finallizes its insert [07:24] poolie: but, a) I think sftp rename dance does try to preserve the old file as a .tmp [07:25] so you could possibly manually recover [07:25] hm, fancy_rename asserts sftp rename won't let you overwrite [07:26] perhaps either we didn't know of that option, or it wasn't supported then [07:26] or still isn't supported by servers [07:26] so it sounds like you're ok with my last comment [07:27] hi all [07:27] poolie: "rename" is intentionally for lock/held directories [07:27] we don't want to overwrite dirs that aren't empty [07:27] that happened accidentally on some sftp implementations, like launchpads [07:27] move is the "rename according to posix" [07:28] poolie, jam: Sounds like another concurrency testing case, how about trying the no-threads approach there to see how it works ? [07:28] vila: in this case, a callback at the right time seems very appropriate, because it is *very* sensitive to timing [07:30] that's why I suggest trying it [07:42] poolie: I'm checking your comments. [07:42] vila: I think there is a hole, I'm not sure it is worth fixing [07:42] hi vila [07:43] i agree using a callback and no threads would be good [07:43] vila i will try to reply to your opus [07:44] poolie: I believe the stardand secsh spec is actually 3, at least back when I was researching it a lot, you couldn't guarantee support for anything newer [07:44] poolie: not urgetn, I'd rather be unblocked on the reviews (well, unless they are tied) [07:44] there were some file open apis and some remote hash apis that would have been promising at th etime [07:44] anyway, if we wanted to change Transport.move() to try to use that flag, i'm for it [07:44] but *not* Transport.rename [07:44] jam btw you're pp this week [07:45] poolie: yep [07:45] vila, i think the reviews are unblocked [07:45] for sftp we don't want to overwrite a held/info directory that already exists [07:45] i would really rather the tests were not threaded [07:45] but i won't absolutely block them [07:45] otoh it seems easy to change them [07:45] jam, right, the directory overwrite behaviour is very important it not change [07:46] meh, neither one got a vote, am I being too conservative reading the comments ? [07:46] vila, i guess i'd put it in a similar case to tests that use subprocesses: use it if you have to, but if we end up doing it a lot something may be wrong, and we don't want to set a pattern of doing it [07:48] vila: I think the reason I haven't been faster at approving stuff is that I'm still pretty uncomfortable with lots of little bits across the whole stack. Which doesn't give me confidence in any given piece. [07:48] I do realize that you've had your head in it more [07:48] I agree with the principle, but I don't feel it applies to a case where tests were copied before you raised the issue [07:48] vila, i think i meant them both to be +tweak [07:48] so I'm willing to defer to your focused insight where it matters [07:48] vila i think this is kind of showing the cost of landing goes up more than linearly [07:48] with the size [07:48] otoh some things perhaps can't be done piecewise [07:49] I think I like the idea in broad terms [07:49] poolie: you mean that splitting this work in smaller pieces didn't help ? [07:49] hm, not sure [07:50] this was a bit like: write one big thing; chop it up; propose all the pieces [07:50] as opposed to proposing smaller bits as you go [07:50] but perhaps there are hidden costs to that [07:51] I think we discussed being a bit less risk averse so I'd delighted to see our policy go more in the direction: this bit is controversial, add A FIXME, address it later, rather than: this bit is controversial, let's reach a consensus before landing it [07:52] vila: I think I've tried to do that, I think the fact that it seems to come up at most stages concerns me [07:52] especially when there is not enough energy spent on reaching a consensus by all parties involved [07:52] (10 stages, and 5 of them have "FIXME" seems a bit odd, if only 1 or 2...) [07:52] i think the thing is that when there are many related patches in flight the whole thing gets a bit harder to discuss [07:52] maybe I'm mis-estimating [07:52] but if they are all in the same patch they are hard (~impossible) to review [07:53] well, there is doing without consensus; and then there is also being more willing to get consensus even on something that's imperfect [07:53] personally i like the second concept better [07:54] the above remarks were general, in this specific case, I've been proposing scaffolding code (and tests) that I think shouldn't be reviewed as strictly as code that will stay (especially tests) [07:55] I can't reach a point where we can discuss on the end result if I had to constantly refine code I intend to throw away in the end, this start discussions on code that is not there to discuss about [07:55] vila: having code that you are throwing away in the end is odd, and inconsistent with how we usually do it, hence we don't really know how to act on it. [07:56] I understand your view point [07:56] just saying it is coming from a place we don't usually look at [07:56] hence confusion and delay [07:56] right [07:56] what's a good example of scaffolding here? [07:56] are you talking about code to support apis you intend to get rid of? [07:56] this a good discussion, I'm not throwing stones, I feel there is several issues and no easy solution, so we should discuss [07:57] _CompatibleStack, the tests with threads are the most obvious ones [07:57] the Stores (already have FIXME), the xxxStacks (already have FIXME) [07:58] hm [07:58] i think generally speaking if there's some code that's a bit ugly or inefficient but there's a comment explaining it will be soon deprecated, people ought to take that into account in reviewing [07:58] (they might still think it's too buggy or something) [07:59] i think generally "I will throw this out in the future" is not very relevant to reviews [07:59] I don't think I can implement a full deployment, add new features, provide a migration path and only then break into pieces for review without incurring a huge push back... (not to mention the delay just to reach that) [07:59] because it's hard to know for sure that you actually will come back to do it [07:59] something more important could come up next week, or you could get sick or whatever [07:59] yup, it's hard to know [07:59] so they need to make sense individually [07:59] which is why I insist on putting a name and a date in FIXMEs [08:00] shrug, there are named dated fixmes from years ago [08:00] it's a good practice but it is no guarantee they'll be fixed [08:00] we should be ok with stepwise refinement; promises it'll be better in future are really not that important imo [08:01] I think it's better than not putting them [08:01] the fixme comments? [08:01] yes [08:01] i agree they're good [08:03] and my goal for config is to have none of them left in config.py [08:03] (I just review them and I strongly confirm) [08:04] so it's hard for me to buy the "you won't fix it" argument there and instead it makes my life harder to fix them *now* [08:04] which I think is close to the root cause of my frustration there [08:05] hm [08:05] well, it's good to talk it over [08:05] one way to look at it, which might be oversimplified [08:05] is that there are things that must be fixed before landing; and things that don't need to be [08:05] we should be careful where we draw the line [08:06] but, if something really is in the first class, then it actually must be fixed; an earnest intention to do it later is by definition not enough [08:08] right, I generally tend to fix things I don't fully agree with to make landing easier, if I don't intend to fix quickly, I file bugs or add FIXMEs, the distinction between the two may not be clear enough as sometimes I use FIXME for issues that are so big I don't even have a good answer for them [08:09] or are so unrelated to the proposal I just punt to avoid falling into yak-shaving mode [08:10] poolie: correct. Though if enough of "doesn't need to be fixed before landing's it gives some unease about the stack [08:11] like 'external_url should accepts an optional relpath' where the implications are all over the place [08:11] ha, no, I filed a bug for this one :) [08:11] jam what do you mean "about the stack"? [08:11] the whole config work [08:12] so, i haven't reviewed every branch [08:12] poolie: the stack of changes that are being proposed. If you have 5 patches pending, and each one has a FIXME or 2, it makes (at least me) concerned [08:12] i think on the whole, having a bunch of branches that possibly interacts, or possibly fix some problems in lower layers [08:12] it's actually harder to review than just one big patch [08:12] Sometimes unnecessarily so. [08:12] possibly this is partly a bug inlp [08:13] jam, right [08:13] also it's harder to see what the actual eventual big picture will be [08:13] poolie: I think it depends on how inter-related the stack of patches is. [08:13] sometimes layering can easily separate out bits [08:13] also, the approach I tool was to (try to) minimize the scaffolding so adding each piece can raise its own issues (say: lock remote config files currently ignores control.conf which is used only on lp but doesn't have (AFAIK) corresponding tests to exercise against) [08:13] probably [08:13] but if a later patch modifies *the same code* [08:13] then it isn't really separated [08:14] I tried to use devnotes to track the big picture, but the issue here is that it conflates know issues with solutions that requires new features and I'm only at the no-new-features replacement stage :-/ [08:14] * vila nods about higher threads fixing issues in lower threads [08:14] that makes reviewers life hard [08:15] i'd like to be tracking it against a doc in the developer guide [08:15] i'm not sure which active branch, if any, might be adding or updating that [08:15] is there one? or is it landen? [08:15] on the other hand, the past experience with big patches is there to demonstrate this doesn't work either so I had to try something else which we are debugging right now :-} [08:15] also, that's an issue with having a stack of patches [08:15] you can't easily see the overall change to some thing [08:16] (couldbe fixed with tooling, or even just having an overall integration branch) [08:19] vila, so, i don't know [08:20] it would be good to work out how to make this less frustrating [08:20] https://code.launchpad.net/~vila/bzr/new-config/+merge/56887 ? [08:21] i feel it's a bit hard to see the big picture [08:22] right, so, do you confirm you're ok to land the current branches ? [08:22] The good thing I see in this approach is that we get rid of agreed upon code with some pieces still needing work [08:23] at least it becomes possible to make independent proposals for these pieces [08:23] As a reviewer myself, I prefer a smaller followup than a bigger proposal [08:23] so to me the thing that seems to work best is to just have one mp at a time, make it self contained (both without introducing new serious problems, and hopefully also improving something) and then do a series of those [08:24] Sometimes it means *I* should do the followup though... [08:24] is new-config current as the overall thing, or have the others moved? [08:24] because i feel like that diff is not so big as to be impossible to review [08:24] argh, but the current series was pushed to the point I could actually present a use case because people weren't seeing how the lower threads will work ! [08:24] and it would address this thing of not having an overall picture [08:25] let me check [08:25] hm [08:26] hmm, it seems to be current (in my local loom) , tip is pushed, but lp diff is bogus :-( [08:26] re the issue of not seeing how lower levels will work [08:26] i think [08:26] it's not necessary to see them used from code [08:27] but i would look for, either it makes the current code at that level cleaner by refactoring it [08:27] or, at least the cover letter can tell a story about how introducing this will later let us do a particular thing [08:27] a prose description of what you're steering for would i think be enough [08:28] hmm, you mean something with a restricted target compared to devnotes ? [08:28] what's a good example [08:29] of what ? [08:29] anyhow "weren't seeing how the lower threads will work" --> I think just a good cover letter should be able to solve this; we don't need hundreds of lines of code [08:29] example of not seeing how the lower levels will work [08:31] well, the weakref branch ? [08:32] the _compatibleStack ? [08:32] can you give me the url? [08:33] so your current-config is everything merged in together? [08:33] https://code.launchpad.net/~vila/bzr/config-lock-remote/+merge/62418 [08:33] poolie: don't know yet, waiting for lp to refresh [08:34] right, looks better, n oidea about the conflicts though... [08:34] for instance, in that branch, i'd like to know whether there is developer documentation of .unload() [08:35] is the fact that 5 FIXMEs are in red there saying something ? :D [08:35] it's not easy to see whether that's true [08:35] well, that's good [08:35] i guess i could just ask you [08:36] there is a doc string for unload() [08:36] yes [08:36] argh, self._save() ??? Wtf [08:36] which is good [08:37] it's a bit like the conversation with jr at the sprint [08:37] hmm, which one ? [08:37] that docstrings i think aren't so good at giving people an overview of what they'resupposed to know about [08:38] hmpf, you think the actual ones are better ? Not throwing stones again but... I try to improve and I try to not wait for perfection before proposing :D [08:39] which actual ones? [08:39] hehe, exactly :) [08:39] you're saying the developer guide is not better than the docstrings at the moment? [08:41] well, i don't think we should block your landings on having a perfect developer guide [08:41] i think it's reasonable to ask about it [08:41] I don't think better/worse is the way to compare the dev guide to the docstrings [08:41] and in fact that developer guide content could be a good way to communicate where this is going [08:41] They serve different purposes I think [08:41] "guide" vs. "reference manual" broadly speaking [08:42] right [08:42] the guide should tell you where to begin [08:42] right, I think issues about unload() would be encountered during tests for new Stores rather than by people using them [08:42] Unfortunately I have to log off now due to ill wife, but it's an interesting discussion you've been having. [08:42] :) [08:42] hope she gets better soon [08:43] spiv: take care of her, there will be logs ;) [08:43] i'm sure she'll find that reassuring :) [08:43] hehe [08:44] back to the dev guide, just at this minute, I don't know whether it's good or not [08:45] It may be a bit too soon to put too much effort into it though since I haven't reach the point where I can say: ok, I've been able to deploy for all actual uses (no new features), the Store implementation is complete [08:45] But at that point, I will be able to write a doc saying: this is how it works, this is what is needed to implement a new one [08:46] So I'd say a bug is appropriate to track this [08:46] hmm [08:46] hm [08:46] i think it's easier to write as you go along [08:47] but perhaps i should do a followon patch that tries to explain it, as i understand it [08:47] * vila falls to his knees [08:47] please do so ! [08:47] That would be so much more productive as far as I'm concerned ! [08:49] And probably illustrates how I think the reviews should evolve: many comments should just be : blah, blah, if you dont get the picture, look at lp:~bzr/bzr/blah-blah [08:49] will avoid a bunch of round-trips [08:50] ? [08:50] what do you mean? [08:51] followon patches could be done *during* review [08:52] to the reviewee (sp?) can just merge them [08:52] speed improvements are main thing arriving in 2.4? [08:53] a trivial example is spelling mistakes, it's almost as fast to fix them and push a branch for the reviewer, but it avoids a roundtrip for the reviewee [08:54] well, roundtrips may be more for less trivial stuff but you get the idea [08:54] gour: probably the largest thing: http://doc.bazaar.canonical.com/bzr.dev/en/whats-new/whats-new-in-2.4.html [08:54] a more involved example would be the tests with threads ;-) [08:54] vila, yes, and there too lp could handle it a bit better [08:55] poolie: thanks...i was looking for such doc, but could not find it [08:55] arguably we should make some of these branches team-owned so that other people can directly commit to them [08:55] perhaps [08:55] vila, i'm reading your doc [08:55] the devnotes one ? [08:55] no, the threads mail [08:56] oh, start with the followup :) [08:56] you can just send it to the list if you want [08:56] it's a bit rambly [08:56] and come back to the main one then [08:56] i agree that these tests should mostly look like a sequence diagram [08:56] that is a good way to express it [08:57] so the question then i guess is, what way of writing the test best expresses the sequence diagram [08:57] and, which way of writing it is least likely to have accidental problems [08:57] yup [08:57] i think writing it as tests is no clearer (probably less clear); and much more likely to have bad consequences such as hangs [08:58] I don't think there is a clear cut to favor one above the other [08:58] to me it is clear from our experience so far [08:58] there are clear tests for concurrency that work in a single thread [08:59] I've made significant progress regarding hangs (and there really are several kind of hangs, the babune ones are only one) [08:59] * gour considers that 2.4 will be one very fine release [08:59] s//are only one sort) [08:59] well, i'm glad, and i appreciate that [08:59] but, it's better if we can just avoid the issue [08:59] gour :) me too [08:59] indeed [09:00] for instance if you look at test_lockdir [09:00] and perhaps as a single example test_20_lock_contested [09:00] i'm moving full to bzr now (will migrate some repos from fossil via git) [09:00] this *could* be written with threads, but i think it's pretty clear, and much less likely to have weird problems, the way it is [09:00] *fully [09:01] :) [09:01] poolie: right (nit: addCleanup(lf1.unlock)) [09:02] good point [09:02] it's a very old test, from before we had that [09:02] (though it could have used finally, of course) [09:02] poolie: sure, it's fine as is [09:03] What I would love on this thread in test issue is that we go a bit further using the config ones as an example [09:04] I've looked at them again and frankly, they are old too [09:04] in the sense that in they were written for a bug which in the end is really addressed by using an atomic write [09:05] and since it's atomic, the initial issue of having two processes mixing its content is moot and the tests are just... relying on this assumption anyway [09:06] hm [09:06] they *were* useful for me in my actual work as ~blackbox tests ensuring the new code paths were covered [09:06] but since the new code path *also* assumes an atomic write, the same remark applies... [09:06] mm i see what you mean about you copying existing test [09:06] simply deleting them seems extreme though [09:07] ok; i have to talk to jam now [09:08] poolie: before you left: you didn't answer to the "meh, neither one got a vote, am I being too conservative reading the comments ?" [09:09] AfC: hiya, is bzr still your preferred dvcs? (/me still remembers your 'orthogonal patches' post) [09:10] config-lock is needsinfo [09:11] * vila blinks [09:11] not approved then ? [09:14] gour: hello [09:15] AfC: hiya [09:15] gour: yes, I still use Bazaar pretty much exclusively. [09:16] AfC: that's nice to hear...i did experiment (after leaving) darcs with mtn and fossil for some time, but now i'm moving fully to bzr === hunger_ is now known as hunger [09:17] gour: that puts me at odds with most of the other hackers I collaborate with, but I don't really care. Our having switched to Bazaar predates their having switched from CVS to Subversion, and their subsequent Git infatuation doesn't impress me much either. [09:18] he he...i also cannot grok git and use bzr-git in such cases...it's quite good [09:18] I always assume that when I disagree with everyone around me, it's 'cuz I'm right and they're wrong. [09:19] ... of course, when I _agree_ with everyone around me, that's also because I'm right and they're wrong. They're just wrong in more subtle ways. [09:20] fullermd: have you ever considered moving to a medium sized mountain top in India and becoming a guru? [09:20] lol [09:20] Oh, absolutely. But have you priced out decent 'net connectivity on mountain tops lately?! [09:21] wisdom is not bought without sacrifice. [09:21] Sure, but couldn't I just trade a firstborn or a limb instead? A man's gotta draw the line somewhere. [09:23] here is some wisdom from the vedas: "In a place where there are no scholars, even a men of petty knowledge is worshiped just as in the land devoid of trees, even a castor oil plant is considered as a big tree." [09:24] gour: castor is the new kid in town but I'd rather stick with goats [09:25] Goatherds Unanimous [10:05] hi, do you have any suggestions how i could fix an inventory corruption? https://bugs.launchpad.net/bzr/+bug/620684 [10:05] Ubuntu bug 620684 in Bazaar "ERROR file id "None" is not present in the tree when checking the root entry" [Medium,Confirmed] [10:06] vila: ;) [10:37] poolie: just foudn you comment on config-lock-remote with needsinfo (which *is* approved), but I was referring to config-locks and config-lock-branch :-/ [10:40] vila, sorry, i want to get these fixed up and landed but it's getting too late here [10:41] poolie: I'll switch to something else then, enjoy your evening [10:44] jam or i or both will concentrate on them later this week and try to get you unblocked [10:45] vila: actually I'd like to do some phone chatting with you if you want. *right* now I need to go eat some food. [10:45] hehe, jam (and I) won't be there that much later this week :D [10:45] Thursday is a holiday here [10:59] jam: sure, I will eat a bit later though. What subjects do you have in mind ? [11:00] vila: just to go over your config stuff in a live chat and get it done, rather than high latency email/irc [11:01] vila: I feel like we had you do a big work on your own, and we need to rectify that. "big" (more than 1 weeks worth of work) should really be a collaboration. [11:01] for config, you're the only one that really has the state in your head, and we've had disjoint reviewers, etc. [11:02] so nobody else really understands it. So either we are rubber stamping because we know you're smart, or we are fragmenting on details we don't understand. [11:03]