[00:14] cjwatson: That's quite a nice diff... [00:50] blr: BTW in one of your test cases I noticed that you had " \n" as an inter-patch junk line, which seems inaccurate - normally in bzr it would just be an empty line, "\n" [00:50] I begin to wonder whether just copying the code from bzrlib.patches and customising it to our particular needs mightn't be more sensible at this point. [00:51] wgrant: Quite :-) I ran a subset of tests to my satisfaction, though not the whole suite [00:51] Will land tomorrow, don't want to have to testfix tonight [00:52] cjwatson: Heh, good plan. [00:53] cjwatson: wouldn't have mattered in that case, it just incremented the linecount for every dirty header other than the first [00:54] Right, but in general a line starting with a space is part of a patch. [00:54] So it probably wouldn't have actually been considered dirty. [00:56] Hence why line.strip() is inaccurate - line.rstrip("\n") would be better, although it still only catches a restrictive subset of non-patch lines where really we should be open-ended. [00:57] as far as I can tell, bzrlib patches would need to be re-written, it has no notion of intra-patch junk [00:57] it only parses junk until the first patch header [00:59] it only works currently as we're explicitly matching against "=== " [00:59] So, there are two basic approaches. [01:00] Instead of throwing away lines that don't fit into any of its categories, it could remember them to attach as dirty headers to the next patch (maybe some slightly special handling of "=== ") [01:01] Or, given that we have a full list of the lines in Launchpad and that bzrlib is currently returning a subset of them in monotonic order, we could spot when it's skipped lines [01:01] sorry called to lunch, but please continue [01:01] The second could be done while continuing to use bzrlib.patches as it stands today [01:02] But it involves at least one extra string comparison per line, so is less efficient [01:02] The first could perhaps be done in bzrlib.patches, but rather than continuing to hack that about and fiddle with its semantics, it's not that much code, maybe it's better at this point to just clone-and-hack it and make it handle unrecognised lines in some more useful way for us than "continue" [01:22] ls [01:22] well, that could have been worse [01:22] cjwatson: I'll try that latter [01:29] will give me opportunity to remove the semicolons at least. [01:54] wgrant: cjwatson: shall we land this in it's current state to fix prod? https://code.launchpad.net/~blr/launchpad/bug-1472045-demangle-inlinecomment-mail/+merge/264105 [01:55] will continue to work on a more general solution [01:55] blr: Have you tested whether that makes the git situation worse? [01:56] wgrant: no, there are no git specific tests. [02:03] hmm cgit renders a space between patches [02:32] wgrant: have a test with a simple git diff, could you have a look? [02:36] blr: That has a blank line between patches, unlike real git diffs. [03:10] wgrant: I've added a space in a hunk in the test, and fixed the spaces between patches. [03:13] it does appear to be working, aware that it could be better. [05:34] wgrant: where best to document the origin of patches.py, before or after the copyright notice? [05:36] blr: Perhaps the top. [05:56] wgrant: see you at some ungodly hour :) [05:59] blr: Heh, yep. [05:59] blr: Hm, test failure. [06:06] Ah, small.diff is buggy, fixing. [06:07] It has a blank line when it should contain a single space. [06:07] Trust the least thorough test to be the one that breaks :) [06:20] gah sorry, only ran the TestInlineCommentsSection tests. [06:20] wgrant: want me to get that, or are you already on to it? [06:22] blr: Just running the rest to ensure it doesn't break anything. [06:25] thanks [06:25] blr: Looks like it was just that. Will submit when it fails in a minute. [06:39] wgrant: great thanks [08:47] Ahhh === anthonyf is now known as Guest97002 [08:47] I know why elmo's diff was weird now. [08:47] bzrlib is quite cunning. [08:48] '\ No newline at end of file' ends up tacked onto the end of the preceding line. [08:48] So there's a line with two \ns. [09:30] wgrant: hmm, well that and the blank line handling [09:31] blr: Right, but we worked out that that didn't explain all of it. [09:31] hmm, any ideas on resolving that? [09:31] I have a fix which isn't too bad, writing a test atm. [09:32] ah great. although, I _did_ reparse elmo's diff and it seemed to align? [09:47] Oh, indeed, it doesn't suffer from this case. [09:47] I just threw other big diffs at it until it went wrong. [10:29] cjwatson: https://code.launchpad.net/~wgrant/launchpad/diff-no-newline/+merge/264254 [10:37] wgrant: huh, that is surprising. [10:38] blr: Yes, not exactly what I'd expected. [10:41] thanks for persisting with that wgrant [10:42] blr: Thanks for the review. Will land once Colin fixes the weird xx-person-edit.txt failure. [10:42] That is also not the sort of thing I'd expect... [10:42] wgrant: How entertaining, although the "line is a ContextLine/ReplaceLine" comment can no longer be true. [10:42] wgrant: Annoyingly, I spotted the problem by code inspection just before buildbot did. [10:42] cjwatson: Ah, fair point. [10:42] Heh [10:43] Misplaced (and unnecessary) classImplements transformation [10:43] * wgrant out for a bit. [10:44] see you in a few hours [11:31] Huh [11:31] cjwatson: Interesting failures now... [11:32] the hell? [11:32] I was going to go a bit stronger than that, but yes. [11:33] I mean, sure, I touched announcement, but only rather trivially. [11:34] It also didn't fail last time, did it? [11:34] Nope. [11:34] And the test count was at least roughly corrrect. [11:34] It doesn't look plausibly spurious, but it must be. [11:36] Not in stdio from the last run either. [11:36] Unless a testrunner died, but I don't see that either. [11:36] Force and pray. [11:36] And you'd expect more missing tests from that. [11:36] I'll just run those ones locally a few times to make sure [11:36] Unless you caught the end of a small layerr. [11:36] Indeed. [11:36] gah, bad keyboard. [11:45] cjwatson: No luck reproducing the failures? [11:47] wgrant: Sorry, that container was busy with other tests at the time, I'm running them now [11:54] Can't reproduce so far. Forcing and praying. [11:58] Yep... [11:59] postgres was probably just having a bad day. [12:22] Looks like buildbot is happy, good. [12:27] Don't jinx it! [12:29] Come at me, buildbot. [12:37] cjwatson: Not even a thousand lines? [12:39] 79 +@adapter(IBugBranch, IWebServiceClientRequest) [12:39] 80 @implementer(Interface) [12:39] Does that actually make sense? [12:41] Hm, something must be referencing it. How odd. [12:42] Oh, the registration is named, right. [13:30] night [13:30] Night blr [13:41] night, thanks [20:07] wgrant,blr: If you could get QA done and a deployment sorted out over my night, that would be very helpful to the phone folks [20:08] Then I could turn on ubuntu-rtm/15.04 translation imports [20:44] cjwatson: ack [20:54] hi. how come comments in launchpad don't have anchor tags? [20:58] pepee: if that would be useful to you, by all means file a bug and tag it 'feature ui' [20:59] ... I'm guessing it would be useful for me and everyone else. I haven't found a way of showing a comment in its context [20:59] I doubt no one has suggested that [21:00] pepee: yep, appears there is a bug [21:00] which one? [21:01] https://bugs.launchpad.net/launchpad/+bug/124166 [21:01] Bug #124166: browsing to bug comment permalink should show comment in context [21:01] https://bugs.launchpad.net/launchpad/+bug/627252 [21:01] Bug #627252: no comment permalinks for issue comments [21:01] hehe [21:01] I'll see if I can get to those next week, shouldn't be too hard. [21:02] thanks! [23:47] blr: Yeah, I'd much prefer that the comment-specific URLs redirected to an anchor on the bug page. The dynamic loading JS complicates that, however. [23:53] wgrant: yep, the single comment view is of questionable value. [23:54] wgrant: we could check the location on comment load for an anchor and scrollTo [23:54] blr: But we'd have to also force that chunk of comments to load. [23:55] can there be both things? like a URL parameter to select between a single comment and the whol context [23:55] like in forums... [23:56] wgrant: true, that is a little tricky. could we always pre-render a map of ids to comment chunks? [23:57] err s/render/generate/ [23:57] on load, check the location for an achor, match to the map, load the appropriate chunks [23:58] no idea how feasible that is, I need to look at the code again [23:58] pepee: Why? [23:58] What's the use of seeing just the comment? [23:59] wgrant, same reason people do both things in forums, some may want to show only one comment, some may want to show the whole thread, and one specific comment in context