=== lifeless_ is now known as lifeless [07:21] hi vila [07:21] poolie_: hey, not fully here yet ;) [07:21] i'm not fully gone, but i will [07:21] be [07:21] reading the thread about locales on python... [07:22] finger syndrom [07:22] L [07:22] ? [07:22] the user is wrong, python is wrong, bzr get the finger :) [07:23] i like the way python and linux are pointing at each other [07:23] oh well [07:23] Joyeux Noël [07:23] Linux? People still use that thing? [07:23] http://paste.ubuntu.com/778346/ [07:24] poolie_: hehe, thanks :) === fullermd_ is now known as fullermd [07:26] more seriously perhaps we should back out gz's hack [07:26] anyhow [07:26] night all [08:14] Man, it sure did get quiet this week. [08:19] It all happened while you slept ? [08:20] All sorts of things happen while I sleep. Like, where did all the trilobites go? [08:25] no doubt about that: australia [10:49] vila: let me know if you'd like to discus feature flags some more [10:50] I've updated the docs [10:51] I'll look into it, I was reviewing switch-possible-transports and there are still 2 test failures :-/ [10:51] comment posted [10:52] vila: switch-possible-transports has two prerequisites, one of which isn't approved either.. so there are other branches I'm more interested in tbh :) [10:53] yeah, well... [11:01] vila: that's not to say I don't appreciate the review.. [11:05] so, if the feature name is global, _present_features should contain all features provided by the installed code base (ie. bzr and plugins) right ? [11:06] yep [11:06] hmm, I was about to ask for some help associated with the feature but that's useless for features that are not "installed" [11:07] (thinking out aloud) [11:08] lock_write raises for write acces, but what about read access ? [11:09] if the format talks about feature, why does each line says feature ? Didn't we settle to put all features on the same line for each kind of necessity ? And consider all unknown necessities as required ? [11:10] vila: All unknown necessities are considered required by the existing code [11:10] (given that when a plugin provides a feature it's present even if the necessity s unknown) [11:10] vila: I don't think we discussed the layout [11:10] ha, misunderstanding then :) [11:11] vila: I agree there are in concept two lists at the moment - the optional and the required features [11:11] and optional additional lists for unknown necessities [11:12] the core issues are still the same whatever format we chose :) [11:12] I'm trying to agree with you on the simplest one that allows the most robust parser :) [11:13] we're close to a simple scanner and scanners are more robust than parsers [11:13] what's the difference? [11:14] wow, many [11:14] roughly a scanner doesn't need to take a context into account nor handle error recovery [11:14] it sees a stream of text and split it into tokens [11:15] a parser sees a stream of tokens and recognizes more complex constructs [11:15] vila: right, I don't see how we can really avoid a parser? [11:15] we'll need one in either case, even if it's very simple [11:16] right, scanners are often called parsers even where there is no grammar to parse [11:16] I think there is some really basic grammar here :) [11:17] no, only tokens separated by spaces [11:17] and newlines [11:17] grammar exists over those tokens [11:18] well, if you insist... but it's so trivial it doesn't need a parser [11:18] we only allow "optional feature foo", not "foo bar bla" [11:18] anyway, forget about the distinction [11:18] sure [11:18] I'd prefer: optional foo, bar [11:18] required biz, bob [11:19] 'feature' is just noise here [11:19] vila: I've considered that as well, but it's less extendable in the future [11:19] it doesn't allow adding extra data at the end (like per-feature parameters), or data other than features [11:20] we're back to the start of yesterday's discussion ? [11:20] maybe a bit [11:20] we ruled out per-feature parameters are being under plugin responsibility [11:21] I fail to see why you want to open for data other than features on the basis that your actual parser is robust without desmonstrating that [11:22] i.e. if you don't clarify the unknowns I can't see what you're talking about [11:22] vila: I don't want to make extension of the format file in the future impossible [11:22] I can imagine us wanting to add other things to the format file than features, or to add more data per feature [11:23] "optional index bar" [11:24] same as optional bar [11:24] index is under bar responsibility [11:24] vila: no, because it's different from "optional feature bar" [11:25] does it change the fact that the format can be read if the feature is not there ? [11:25] vila: it's something entirely different, unrelated to features [11:25] vila: "bar" is not a feature in "optional index bar". it's something entirely different [11:26] vila: it can be read, as it's optional too - and the parser won't barf on that [11:26] what's the API for that ? [11:26] who benefits from it ? [11:27] vila: there is no API, it's to allow future extension of the format file [11:28] vila: we didn't foresee features in the past, but the reason they're possible is that older versions of bzr read the entire format file, not just the first line [11:29] vila: I'd like to keep that flexibility present in case we have something similar in the future, that's why the format is a bit more verbose [11:29] vila: perhaps we can fold it into one line still, e.g.: [11:29] "optional features = foo, bar, bla" [11:29] "required features = blie" [11:30] that'll still be a bit ugly if there are going to be many and the line doesn't wrap, but I could live with that.. :) [11:30] I do like it more verbose though, also because it means we can add more per-feature things in the future if we wanted to for some reason [11:31] that's a path I don't want to follow, you're opening a can of worms with no idea about what kind of worms will come out [11:31] why ? [11:31] vila: how is this opening a can of worms? [11:32] interop, upgrades [11:32] I'm not proposing we add anything else. I just don't want to paint ourselves into a corner. [11:33] why don't you want to get out the corner we're in right now with small and safe steps ? [11:33] vila: we're not in a corner now - we have plenty of room to extend the existing format [11:33] "anythin else" == the unknowns I'm referring to, [11:34] allowing optional features and required features is already a huge opportunity worth exploring with limited risks, [11:34] what's the point of raising the risks ? [11:34] how is this raising the risks? I don't see how there are any additional risks unless we actually extend the format further. [11:34] then why allowing it ? [11:35] The only thing I want to prevent is that we can't extend it further if we want to, in the future. [11:36] how is that raising the risks? [11:36] we've already discussed that, I proposed an already supported format allowing a huge kind of tricks and we couldn't agree on what should be recognized there to allow a single backport [11:37] designing an extensible format is more than splitting lines (or hairs ;) [11:37] ruling out ini format on the basis that it's too much overhead but not comparing it to an existing implementation... is questionable [11:38] and almost irrelevant until what is taken out from it clearly defined [11:38] and my understanding was that you wanted to extract only: is there a required feature there or something that should be considered like a required feature [11:38] nothing else [11:39] vila: that is what I want to extract, and the only thing the current parser does today [11:40] but what guarantees does it offer that your unknown future additions will still be parsed and ignored or considered required ? [11:41] vila: it means we *can* add more stuff in the future apart from features, without having older clients blow up in our face too badly [11:42] I have no idea what that stuff is, but it seems sensible to keep the option open [11:42] meh, I'm saying your parser doesn't allow that while the ini format does and you're just replying: no [11:43] how can you say 'I have no idea' and 'without blowing' ? [11:43] vila: how does my parser not allow that? It allows opening things if there is a line that starts with "optional " that it doesn't understand [11:44] anything that doesn't start with optional it *will* complain about [11:46] so let's back up [11:46] you would prefer something like "Bzr format x\noptional foo,bar,bla\n" ? [11:47] yes, preferably allowing 'required biz,bob\n' and everything else being handled as 'required you-can-t-represent-me-anyway' [11:48] which give us the ability to support older bzr release and experimenting like mad with bzr.dev [11:49] no doubt we will gather a lot of understanding [11:49] vila: could we do [11:49] one point I don't get: [11:49] "required features biz,box\n" ? [11:49] are you trying to never require format bump at all ? [11:49] s/ features// :-p [11:50] vila: I would really like to have the namespace just in case we need it [11:50] I can understand the redundancy in having each feature on a separate line and repeating "optional feature ..." [11:50] we're adding one, you're asking for two or mores ;) [11:51] meh, we're adding two: optional, required [11:52] vila: that's just one - they share a namespace [11:52] meh again, yup [11:52] vila: we don't want to bail on everything we encounter that doesn't start with required or optional [11:52] with an attribute being 'optional, required' but let's add 'read-optional' at least :) [11:53] we won't bail, we will handle it as required [11:53] so even corruptions will be seen as required... [11:53] vila: but then we don't have the ability to ever extend the file again [11:53] and this is why I would really like to have that word "features" in there [11:53] what difference will that make for older bzrs ? [11:54] none if nothing is backported ? [11:54] vila: if we add something other than "features" in the future, but it's optional, they will know to ignore it [11:55] why can't a plugin does that with only optional ? [11:55] s/does/do/ [11:56] vila: why can't a plugin do what with only optional? [11:56] adding something other than features [11:57] that *is* optional [11:58] Add bugs rather than features, maybe? ;) [11:58] vila: it's not really plugins that I have in mind, it's whatever we as bzr devs add in the future [11:59] I'm very new to this topic, but I'm not sure what sorts of flags you could have in a format that couldn't be sensibly called "features" — parameters maybe? [11:59] spiv: possibly [12:00] (Although parameters strike me as something I'd probably prefer to have in a separate file, rather than encouraging anyone from editing format files!) [12:00] spiv: I'm mostly concerned that we if we only have one namespace, we can't ever add anything else [12:00] Hmm. [12:01] So if they are one-per-line, I'm not sure there's much to worry about [12:01] format bumps allow adding anything, are you trying to entirely avoid format bumps ? [12:01] spiv: the discussion is (among other things) about whether they should be one line :) [12:01] Because "optional non-feature=17" would still be usable backwards and forwards. [12:01] vila: yes, I would realy like to avoid another one [12:02] vila: "entirely" is an extreme goal, but "greatly minimise" sounds like a good (and plausible) goal to me [12:02] spiv: precisely my question [12:03] Well, I'm not sure the distinction matters really? [12:03] vila: if we have them in a list, it greatly limits what can be in there [12:03] Because we already have the fallback to format bumps if we find no other way, but if we try hard to not have any more we might just achieve that. [12:04] well, I see feature flags as a way to *prepare* format bumps [12:04] vila: I like the way ext2 → ext3 → ext4 has evolved surprisingly smoothly with in-place upgrades [12:05] vila: and it's in part due to having something quite like feature flags AIUI [12:05] format bumps indeed imply smooth upgrades [12:05] http://pastebin.ubuntu.com/778530/ [12:05] but saying feature flags will allow avoiding format bumps without having experimenting with them... [12:05] Do you mean “imply we need to add support for smooth upgrades?” [12:05] that's basically the different options as I see them [12:06] spiv: we need to improve the upgrade experience, there is a consensus that we are not there yet [12:06] Because that's not what we have now, nor quite how I feel the ext2/3/4 example has worked [12:06] or http://pastebin.ubuntu.com/778531/, with read-optional [12:06] I'd say that for years now, format bumps have implied very rocky upgrades in practice [12:07] And rather than just saying "well we'll fix that next time", perhaps we should look for a different path entirely [12:07] spiv: (1) is the current format, (2) is what vila is suggesting, (3) and (4) are alternatives that I could also live with [12:08] spiv: yup, but the code has evolve a bit since the last bump but some more polish is still needed [12:08] spiv: Yeah, I think ext2/3/4 is indeed a good example of a nicer way of doing this [12:08] jelmer: you didn't mention required ? [12:09] spiv: (and they don't have the problem of people sending each other disk blocks over the interwebz) [12:09] We may need to do something to make the bump to with-feature-flags less rocky than in the past, but my point is that feature-flags is a highly plausible route for much smoother upgrades in future [12:09] spiv: we all agree on that [12:09] spiv: enabling feature flags doesn't require a format bump, the only real bump is when you actually enable a feature [12:09] vila: yeah, this is mostly an example - I cna add some if you like [12:09] lunch time here, daughters calling [12:10] jelmer: my initial preference is for 4 [12:10] jelmer: nope, just checking (the first line may vary too) [12:10] jelmer: if it's flexibility you want, that way has got it: [12:10] http://pastebin.ubuntu.com/778535/ [12:11] jelmer: you could e.g. imagine instead of a registry of feature-flag keys, a registry of "can-you-interpret-feature-line" functions [12:11] jelmer: initially all we might have are one-word keys, and indeed a single function that looks for and understands those [12:12] spiv: yeah, that makes sense [12:12] jelmer: but it leaves scope for future readers (including the same version of bzrlib with shiny new plugin) to do arbitrary fancy stuff if you really want [12:12] jelmer: bbl, using examples is far clearer, add garbage in all places where it would be ignored or seen as required and we should get a far better picture [12:12] and I'm a big fan of registries too ;) [12:13] spiv: that's indeed the sort of option I would like to keep open [12:13] jelmer: I'd think probably we don't need to worry about the slight bloat vs. all-one-line, because it'd take quite a few features to have this exceed even 1k [12:14] The one-per-line format is also dead-simple to parse (or tokenise ;) [12:15] spiv: yep :) [12:15] And also easier to add new flags to than a comma-separated format, assuming no ordering restrictions: just "echo 'optional new-feature' >> format" [12:16] (unless you're on a platform with a non-\n EOL :) [12:16] Hmm, one other comment: [12:16] I'd actually suggest changing the first line [12:17] For the zero-flags case, where it's the same as today, sure keep it the same. [12:17] But otherwise a new format would be good as a hint to users with older bzrs trying to google for errors [12:18] spiv: I'm not sure [12:18] It'll be slightly inconvenient I'm sure [12:19] spiv: It makes things hard as features are removed again [12:19] spiv: and we'd backport this to older series, so hopefully most people will end up with a version of bzr that understands it [12:19] But it will avoid giving confusing errors where the user looks at a big long string that has the first 90% the same as another big long string that works [12:20] Which is very much the sort of error that will confuse people, because which users actually read every byte of the format string in that sort of error? [12:20] * vila passing around, jelmer: isn't this turning into something a prober should handle ? [12:21] jelmer: It definitely does suck for the "remove last feature flag from format" case. [12:21] jelmer: but that's a modest code inconvenience in exchange for confusing unlucky users less [12:22] Think of it as part of the "making upgrades smooth" goal that we all agree on ;) [12:23] Anyway, that's just some quick opinions before bedtime. You're welcome to disagree, I know you've been thinking about this for more than the 10 minutes that I have been! [12:24] jelmer: parting thought: do you have concrete examples of feature-flags you think we might have? [12:25] jelmer: the case that springs to mind for me is some of the optional look-aside indices that we've sometimes talked about adding to repos (and in some cases have plugins for, like search or revnocache) [12:26] spiv: nested-trees, git-cache, bzr-search-index [12:26] jelmer: I could imagine e.g. an "optional revision-graph-heads-cache" [12:27] spiv: yep [12:27] spiv: that also seems like something that could be read-optional - i.e. you can access the repository without having it installed, but if you write to it you need to have the feature [12:27] jelmer: (readers that want to use that cache would need to do something like compare the current pack-names with the pack-names at the time the cache was last regenerated, in case a writer without the flag added revisions) [12:28] jelmer: or that! But that's less smooth ;) [12:28] (great for safe prototyping though before adding the extra backwards-compat polish) [12:29] jelmer, vila: it intrigues me that with the possible exception of nested-trees these all seem like primarily flags on the repo component [12:29] spiv: search indexes are on branches [12:29] Ah! Interesting. [12:30] Anyway, bedtime over here. [12:30] g'night :) [12:30] Merry winter solstice and/or other season-appropriate greetings, and good night :) [12:32] hehe, you too ! [12:58] jelmer: back [12:59] jelmer: so, how about probers ? If they can't handle that, why ? [13:01] jelmer: and is there a link with all the refactoring you did related to foreign plugins and feature flags ? If not, why ? [13:12] vila: back too [13:12] vila: what about probers? The bzr prober only checks for the .bzr/branch-format file, it doesn't try to parse it, that's all in Bzr(Dir)Format [13:12] vila: feature flags are bzr-specific [13:13] but isn't it the part that decide whether or not a format can be opened ? [13:13] (and already implement a registry with all the associated flexibility) [13:14] I realize a lot of ideas are thrown around against an *existing* proposal which is frustrating but I think it's worth it [13:15] vila: the prober just returns the format if it can find it [13:16] vila: if it can't, it raises UnknownFormatError [13:16] whether or not the format is supported isn't checked until later [13:17] ha, ok, and all formats inherit from BzrFormat (with your proposal at least, may be worth splitting :-}) [13:17] right, there is the check_support_status [13:18] and the parsing, sorry, not familiar with this code yet ;) [13:20] jelmer: and the other question ? [13:20] vila: what was the other question (sorry) ? [13:22] jelmer: and is there a link with all the refactoring you did related to foreign plugins and feature flags ? If not, why ? [13:22] vila: ah [13:22] http://paste.ubuntu.com/778596/ (4) with garbage added [13:23] vila: there is no link, because feature flags are specific to bzr formats [13:24] vila: that last line would be treated as requiring a feature name "not starting with (optional, read-optional or required) is handled as if a required feature was not present" [13:24] vila: (so we can add new necessities in the future, while not breaking old versions of bzr too badly) [13:24] good for bzr but not good enough for foreign plugins ? [13:25] vila: it's a format-specific feature [13:25] vila: we can't write to .svn/format and have svn clients honor that and not break :) [13:25] ha :) [13:27] hmm, so better use http://paste.ubuntu.com/778603/ then [13:28] vila: no, that would break the ability to add things to the end of the line [13:28] vila: what's wrong with the interpretation I mentioned earlier? [13:29] feature names should not contain spaces [13:29] vila: but what does swapping the name and the necessity help? [13:29] it makes it clearer what the most important stuff is: the feature name [13:30] and matches your implementation, key=name, value=necessity [13:30] sigh, I fear we're back where we started :( [13:30] vila: that breaks extensibility again [13:30] well, you still haven't answered my initial objections... [13:30] vila: what were those? [13:31] vila: you were reasonably happy with (4) right? [13:31] vila: if you don't see us needing the extensibility, what's the problem with interpreting "everything not starting with (optional, read-optional or required) is handled as if a required feature was not present [13:31] " [13:31] as a feature named "not starting with (optional, read-optional or required) is handled as if a required feature was not present [13:32] " [13:32] with necessity "everything" (which, because it is unknown, gets treated as required) [13:32] I haven't agreed yet on the format, I'm discussing it [13:34] I didn't say you had [13:34] ha, sorry, misread reasonably happy, let me clarify then [13:34] (4) seems like a good alternate for (2) [13:35] I want to see where garbage is interpreted [13:35] meh [13:35] I want to see where garbage is accepted and how it is interpreted [13:36] if you want extensibility at the feature level, it makes sense to start with line format where only two things are required: the feature name and the necessity [13:36] vila: I agree [13:37] vila: I think the reason I like the necessity first is that it makes the file read like a check list [13:37] from there, since additional stuff is feature specific, it makes sense to make the necessity a feature attribute (which your implementation already does) [13:37] vila: and it means it's possible to allow spaces in feature names (and do fancy stuff later) [13:38] meh bad idea, if you start with spaces you want to allow what more ? unicode ? [13:38] vila: allow spaces, not encourage spaces :) It means we can tack on stuff at the end later /if we want to/ [13:39] re-read my paste, it allows more optional stuff after the necessity, making it mandatory will require a backport though [13:40] vila: for now, everything after the necessity would be part of the feature name [13:40] and anyway we said parameters should not be part of the format or do we disagree on that ? [13:40] vila: that way we wouldn't need a backport later [13:40] huh ? [13:40] vila: yeah, parameters shouldn't be added to the format, at least not now [13:40] you won't recognize the feature anymore [13:41] vila: right, and that's the point [13:41] shudder [13:41] better to not recognize a feature than to load it with the wrong data [13:42] would you mind mention your motives when you know them instead of waiting for me to discover it by poking randomly? :-D [13:42] s/it/them/ [13:42] you can get the same effect with less magic by *changing* the feature name when it needs more stuff [13:43] vila: right, I'm just saying there's no particular reason spaces couldn't be part of the name [13:43] I have one such reason: identifiers cannot contain spaces :) [13:44] vila: who says they'll just be identifiers forever? [13:44] what would be the need ? [13:45] on the other hand, enforcing identifiers allows useful stuff like mapping to the file system, mapping to python modules, etc [13:45] vila: as I said earlier - I don't know, but I would like to keep the ability to add stuff open [13:45] I don't know either but I won't forbid such uses for no good reasons [13:46] vila: you mean, you would ? :) [13:47] yeah :) I don't want to allow things that will make my life harder [13:47] handling spaces in identifiers has always make my life harder [13:47] vila: again, I'm not saying it would be good practice to allow spaces in feature names [13:48] vila: and we're not mapping features to anything, they're just arbitrary strings [13:49] vila: s/allow/use/ [13:49] you're using them in a set() and a dict() already, the later *is* a mapping [13:49] vila: not in the sense you meant [13:51] ? [13:51] vila: either way, spaces after feature names wouldn't usually happen.. this is just a corner case of what we do in the parser with when we encounter them [13:51] but that's the whole point of defining a format [13:51] vila: we don't need to map features to python modules, file systems, etc [13:51] if it's not defined you can't write a robust parser [13:51] vila: so you would just ignore everything after a space? [13:53] so you can use split(' ', 2) yes or even split(' ', 1) and delegate to a registry as spiv suggested [13:53] vila: if you don't see the use of ever having spaces in the feature data, what's the problem with just including the in the feature name for the moment? [13:53] vila: but we're not doing that (delegating to a registry) just yet [13:54] vila: when we do, then we'll start splitting and passing the rest of the string (after the space) to the feature [13:54] that's still a property of the format we're discussing [13:54] vila: but old bzr versions shouldn't try to open a feature while discarding data in the file [13:54] who forbids adding new stuff now ? [13:55] vila: what do you mean with now? [13:56] if old bzr versions should not ignore adding data you can't add it [13:56] added [13:57] vila: why not? I'd rather have the old version of bzr not be able to load the feature at all than load it without the extra data [13:57] you lost me here [13:58] vila: let's say we do add feature parameters in bzr 2.7 [13:58] I've been advocating a robust format (which we don't have yet) for this reason (don't open what you don't understand) and you've been complaining that it will forbid adding more unknown stuff [13:59] vila: and bzr-search starts setting "optional search with-authors-data" [14:00] bzr-search business, since that requires additional data put 'with-authors-data' there and deal with it [14:00] vila: we don't want older versions of bzr to open that branch and not see the "with-authors-data" flag when they write to the index - because they wouldn't add the authors data and make the index incomplete [14:01] vila: sorry, but I still don't still what's not robust about the current format. [14:02] it won't be robust until we know what is recognized and what is not and what happens in all cases [14:02] there should be no unknown as far as (format + parser) is concerned [14:02] it's pretty well defined what is recognized and what isn't [14:02] there are ways to allow some unknown in the format [14:02] where is that defined ? [14:03] ok, that's a fair point [14:03] I was going to say.. in the parser :) [14:03] pfew [14:03] the spaces handling isn't defined in the docs or the tests, but all the other stuff is [14:03] progress ! :) [14:04] yes, many things are defined, some better than others but I don't mind that too much, [14:05] what I care about is that there is *no* unknown [14:05] and the simplest the format is, the less unknowns we have to care about [14:06] vila: again, I think that's true for the current format :) [14:06] for current == ? [14:06] http://paste.ubuntu.com/778603/ ? http://pastebin.ubuntu.com/778535/ (4) ? [14:06] vila: "format 4" - basically lp:~jelmer/bzr/feature-flags with " feature " removed [14:07] in short: [14:07] haaa [14:07] we allow additional lines in the format file, each consisting of: a necessity (not containing a space), a space, and a feature name (anything but a newline), and a newline [14:07] hmm, that was as in: 'ok, let's look at a summary to re-sync' [14:09] anything but a newline is too magic to my taste without being the only way to provide something [14:09] vila: it's the simplest and allows for the most extensibility in the future [14:10] vila: we can disallow registering features with spaces in their name? [14:10] better disallow at the source [14:11] allowing stuff and then saying "no, you can't" is frustrating :) [14:11] vila: "be liberal in what you accept, conservative in what you send" [14:11] yup, we generate the format file :) [14:12] vila: I don't want us writing feature names with spaces in them, I want us to be able to deal with and not misinterpret format files that we receive [14:12] but me too damn it, why do you think I'm discussing ? [14:13] ok, perhaps there's some miscommunication here [14:14] vila: I think we're agreed that we shouldn't allow spaces in feature names? [14:14] vila: the question is what to do with spaces we find in files we receive [14:14] yes (no spaces) [14:15] spaces we receive.. [14:15] first, [14:15] we have lines [14:15] vila: I think we should basically treat anything with a space in it as unknown [14:15] as long as we recognize a line, we continue, [14:16] when we encounter a line we don't recognize => as if a required feature was not there [14:16] ha [14:16] what kind of unknown ? [14:17] if all unknowns are considered as an absent required feature, it's easy [14:17] vila: the feature flags doc describes this [14:17] ooooh, are you saying you don't want to allow optional parts after the feature name ? Never ? [14:18] vila: if we encounter "optional foo bar" I want to still load the file, because there is an optional feature we don't know about [14:18] vila: even if foo is registered [14:19] vila: if we encounter "required foo bar" I think we should abort [14:19] vila: the first word is always interpreted as the necessity - unknown necessities are considered equal to "required" [14:20] could you write tests for that ? I have a gut feeling there is a hole there [14:21] vila: there are tests for this [14:21] <"optional foo bar" I want to still load the file > but it's an unknown [14:21] vila: not for the space handling, but for the unknown necessities [14:21] I'll write some for the space handling [14:22] isolating the parser and writing direct tests will probably help [14:22] the expected output being: can/can't open [14:22] the parser is literally three lines [14:23] but I can split it out [14:24] from_string ? [14:24] good enough then [14:25] but even with 3 lines I think it will choke on 'not-asingle-space-your-split-will-blowup' [14:26] vila: I'm using .split(" ", 1) [14:26] you changed it since the version I'm looking at then :) [14:27] I've just pushed a version that disallows registering features with spaces in their name [14:31] jelmer: on the basis that a feature should not use spaces so can't be recognized if more stuff appears after it in the format ? [14:32] grr diverged branches [14:32] vila: yeah [14:36] far simpler :) [14:38] and necessity first to be allows to add more in the future (could have worked if it was second too ;) [14:39] ordereddict requires python-2.7 [14:41] oh, ok [14:41] I guess it's not the worst thing in the world if we write things back in the wrong order [14:42] when do you write them and from what ? as_string() ? How are they *created* then ? [14:43] vila: some other API (probably e.g. Repository.update_features) updates format.features and then writes out format.as_string() to its format file [14:43] I may miss something but the only place I see self.features modified is by from_string() [14:43] ha, but that's not addressed by your proposal right ? [14:44] vila: indeed [14:45] right. well, that would help the discussion but the format looks simpler now, let me recap: [14:45] a line is if feature is not registered => can't open (msg to the user ? NotABranch and such ?) [14:46] if a line doesn't match that => can't open (msg ?) [14:47] ha no, MissingFeature [14:47] jelmer: correct ? [14:48] vila: if the feature isn't registered (and required) the user indeed gets MissingFeature [14:48] an invalid line (ie one missing a space) causes ValueError at the moment, that should probably be a UnknownFormatError instead [14:49] hmm, more probably broken, the first line have been recognized [14:50] well, who knows what extensions we might add in the future :) [14:50] FormatCorrupt ? [14:50] hungh [14:50] hmm, I think corrupt sounds a bit alarming [14:50] I want this case to be covered :) [14:50] vila: perhaps just leaving it as Valueerror for the moment? [14:51] vila: test_from_string tests it [14:51] so we can indeed not emit a FormatCorrupt unless the content is indeed broken [14:51] We have DirstateCorrupt for this reason [14:52] and LockCorrupt (if you want a case closer to the format file) [14:52] I mean covered as in: we open or not and if not because a feature is missing [14:53] don't add a new unknown again :) [14:54] vila: so, I'm not particularly concerned about this either way. I think a ValueError isn't the worst thing in the world for this, but I'd be happy to add FormatCorrupt too [14:56] And the ValueError is caught ? [14:56] Or does it bubble up to the user ? [14:57] vila: I'm happy for it to bubble up - we do that in a lot of other cases too, and it provides the best data to report back to us developers [14:57] e.g. we have the same when we read the last-revision file [14:58] meh that's not a good reason [14:59] vila: either way, I don't really care what we do in this case [14:59] vila: let's focus on the core stuff [14:59] I do [14:59] This code will be backported [15:00] (necessity, feature) = 'foo'.split(' ', 1) breaks [15:00] vila: that only occurs if people somehow have a corrupted format file [15:00] exactly [15:01] we keep getting bug reports about corrupted files, if we can diagnose them in a 3 lines parser why not do it ? [15:01] vila: we don't catch ValueErrors in a lot of other cases either if there are corrupt files [15:02] vila: I think adding CorruptFormat will make it harder to debug, not easier [15:03] vila: unless I'm missing something? [15:03] I've never see a finer error handling render debug harder... the code is fresh, there is an issue, handling it later will cost more (probably less than the time we already spent discussing it) [15:04] hey wait, [15:04] invalid format header is already a ValueError ? [15:04] oh, that's what is presented to the user for unknown formats ? Naaaah [15:04] vila: no [15:04] then it's caught somewhere ? [15:05] vila: we already check the first line earlier, because we look that up in a registry [15:05] raise ValueError("Invalid format header %r" % format_string) [15:05] err [15:05] vila: line 1175 [15:05] raise ValueError("Invalid format header %r" % text) in your proposal [15:05] I guess this could be an AssertionError [15:06] yes, better than ValueError [15:06] the object can't be open anyway [15:06] especially if the line is displayed [15:07] but wait [15:07] where is the ValueError caught ? [15:07] vila: nowhere [15:07] vila: that first line is looked up in the format registry [15:08] vila: and if that raises a KeyError, we raise UnknownFormatError [15:08] only later ? [15:08] see line 1171-1175 [15:08] vila: before [15:08] I'm there but that's where the exception is raised not caught [15:09] vila: where UnknownFormatError is raised you mean? [15:09] 'invalid format header' is informative, 'need more than 1 value to unpack' is not, especially if the line is not displayed [15:09] no no, ValueError [15:09] vila: sorry, 1243 [15:09] KeyError here [15:10] oh, I see what you mean [15:10] right, probe_transport :) [15:11] hmm, UnknownFormatError(line, kind='feature-flag') ? [15:12] vila: who says the unparseable line is a feature flag? [15:12] vila: I think the format as a whole that is unknown is the bzrdir/repository/branch/workingtree format [15:13] it's where feature flags are defined and should indicate a corruption ? May be FormatCorrupt is better ? [15:13] vila: sure [15:14] anyway, we agree it's a case where we can't open right ? [15:14] yes [15:15] so as long as the message is better than 'need more than 1 value to unpack' with no context... [15:15] AssertionError will do [15:16] if it's corrupted, the line should give enough hints, if it's not (a mythical unknwon :) it will be clear too [15:16] right [15:17] the plan to backport is to call the parser code everywhere it's needed in old bzr right ? (As in: without backporting BzrFormat related changes) [15:18] hmm, a bit more than the parser, the set() and dict() handling too right ? [15:18] vila: depends on how much we want to backport [15:19] vila: actually, yeah [15:19] vila: at least backporting the parser and calling that everywhere [15:19] and then probably ignoring optional lines and raising MissingFeature for each non-optional feature we encounter [15:20] that's check_support_status right ? [15:20] vila: yep [15:21] or a simplified version of it [15:21] not by much if you want to allow support for required features but may be that could be done separately [15:22] or may be we only provide support for optional features which would be ok with me [15:22] vila: I'm happy if old bzrs can just skip optional features and print a sane error if they encounter required features [15:23] yup, which a robust parser should provide [15:24] * vila looks if the parser can be made even more robust by deleting one line or two [15:24] :-p [15:26] :) [15:29] jelmer: hmm, you haven't included read-optional yet ? [15:29] vila: no, that would be too big a change, and there's no particular reason to already include it [15:29] If that requires a backport and then a release and then an install... [15:30] I'm lost, what change ? [15:30] vila: hooking into lock_write [15:30] right [15:30] but [15:30] oh, to forbid writes [15:31] ok [15:33] vila: pushed use of ParseFormatError [15:39] hehe lineno+2 [15:40] oooh, and you added format to the previous ValueError too ! [15:41] vila: s/ValueError/AssertionError/ :) [15:42] no, I'm referring to the one for invalid header, the format was there already ? [15:42] oh no, you turned it into an AssertionError ! [15:42] yes [15:42] lp incremental diff is totally broken [15:44] ghaa, no, it conflates all the revisions, nm [15:44] and splitted tests ! It's Christmas ! [15:45] jelmer: do you want a second review ? [15:45] * vila ducks [15:46] ghaaa, you resubmitted it ? I voted on an old one... [15:46] whoops [15:46] I resubmitted it because I wanted a new diff, lp was taking too long to update.. [15:46] vila: and THANKS [15:48] pfew, thanks for your patience, I hope you don't mind the time spent here but I feel confident that we would have spend far more in backporting and handling spurious bugs otherwise [15:48] that was probably the longest IRC discussion I have ever had about a single MP, but it seems to've worked :) [15:48] hehe [15:50] jelmer: there is no urgency (hmm) but don't forget that we'll need to release whatever you backport so it could take a while before we can safely deploy even optional features [15:50] it may not matter if we start experimenting with people already using 2.5 but... [15:51] ...the sooner the minimal stuff is backported the sooner we can experiment freely [15:53] vila: yeah === yofel_ is now known as yofel [17:25] is this a known bug ? http://paste.ubuntu.com/778868/ [17:25] is think i saw poolie mention issues with etc keeper and locale on a list. [17:27] that is a pristine precise install then some automated 'late_command' run after installer. [17:28] smoser: yeah, depending who you ask, the bug can be in bzr/python/ubuntu or yourself for not getting the file system encoding right [17:28] well i certainly blame anyone but me. [17:28] since the locale is C... [17:28] sorry, was kidding as there was a heated discussion around a patch proposal for python [17:28] :) [17:28] i'm joking too. [17:29] i understood. [17:29] the python guys said: tell your users to set the right locale [17:29] i was fairly surprised that apt didn't fail [17:29] that in itself is almost a bug. something tried to run bzr (etc-keeper i think). i would have thought it would cause install failure. [17:29] well, it probably doesn't try to find a valid unicode representation of the path [17:31] so what bug would i subscribe to just to follow? [17:31] the surprising bit is that several packagers (in a related but less heated discussion) mentioned that utf8_something was the default for ubuntu [17:31] ha, mgz knows [17:31] but he've been pretty quiet today so I'm not sure he's around [17:32] justasec [17:32] Bug #794353 [17:32] Launchpad bug 794353 in Bazaar "ascii is a bad default filesystem encoding" [High,Fix released] https://launchpad.net/bugs/794353 [17:33] ha, fix released :) Let's check where [17:33] 2.5b5 [17:35] smoser: for the attempt at fixing python itself [17:37] smoser: in other words, this is in flux but a fix is currently in bzr.dev (2.5b5 will be released around 2012-01-16) [17:37] vila, thanks. [17:37] good enough for me [20:16] help? 'bzr commit' used to show me the diff in the commit message template, but now it stopped [20:16] I'm yak shaving already, can anybody point me to a quick setting I could check? [20:17] * mgedmin looks at ~/.bazaar/bazaar.conf and notices [ALIASES] ci = ci --show-diff [20:17] oh, so I used to type 'bzr ci' and now I started typing 'bzr commit', apparently [20:18] I thought I read in a recent message to the mailing list that show-diff was on by default? [20:18] hi mgedmin [20:18] mgedmin: I'm pretty sure --show-diff isn't on by default [20:20] hi!