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