Re: [Monotone-devel] Re: netsync connection info cleanup

[Timothy Brownawell]

On 06/09/2010 06:34 PM, Thomas Keller wrote:
> Am 09.06.10 19:01, schrieb Timothy Brownawell:
> > On 04/28/2010 06:06 AM, Thomas Keller wrote:
> > > Am 28.04.2010 12:36, schrieb Thomas Moschny:
> > > > Am Sun, 18 Apr 2010 20:49:37 +0200
> > > > schrieb Thomas Keller<address@hidden>:
> > > >
> > > > > Secondly, I'd actively deprecate any branch name which does not match
> > > > > the following regular expression, i.e. by throwing a warning when a
> > > > > branch cert which a different value is created:
> > > > >
> > > > >      ^([\w\d]+([-][\w\d]+)*)(\.[\w\d]+([-][\w\d]+)*)*$
> > > >
> > > > Sounds good to me, but maybe we have to ask our users, whether that'd
> > > > be ok for them. And we should still allow them to use other branch names
> > > > if they wish so (because technically, there's no need for such a
> > > > restriction).
> > >
> > > Depending on the actual URI scheme we'll be using, we can further adapt
> > > the regular expression above, like f.e. if the URI scheme will look like
> > > this:
> > >
> > >      scheme://address@hidden/db?include,-exclude,include
> >
> > This is in the nvm.connection_info_cleanup branch now. Are there any
> > objections to merging it? It doesn't include any string changes.
>
> I understand that it might be a good idea to have this in before
> 0.99/1.0.0 because it breaks BC a bit (well, if anybody used this
> feature at all) and because you need to know the proper URI scheme for
> usher as well, but I was just about to start hacking on this branch
> again and wanted to do a couple of other things on my way. So I wouldn't
> mind to see the whole thing in 1.1.0 or later.

What sort of other things, more user-visible changes or internal code 
hygiene? Right now mtn:// sync doesn't work at all, and it really would 
be nice for 1.0 to support non-hacky virtual hosting without needing a 
special DNS setup.

> > > this should be sufficient I think (untested):
> > >
> > >      ^[^-,*][^,*]+$
> >
> > I think we also don't like '+' and '%' due to urlencoding.
>
> Apropos '+' - this shouldn't be needed - I forgot to exclude whitespace
> above. I agree with '%' and would also add '?'. The above regex also
> disallowed single char branch names, so this should work better:
>
>         ^[^-,*%\s][^,*%\s]*$
>
> We could still disallow '+' if we'd want to make inclusion also
> explicit, but some people disagreed on this. Personally I won't mind.

Well, the reason for '+' is more that a '+' in a url translates to a 
space after urldecoding. So for example 'mtn://foo.com/bar?abc+def' 
would translate to an include pattern of "abc def" and to get an include 
pattern of "abc+def" you'd need 'mtn://foo.com/bar?abc%2Bdef'. Which 
would be annoying to remember, especially if you don't typically work 
with urls.

...hm. Except that our urldecode is broken and doesn't actually do this. 
Which could be annoying for people who do work with urls regularly and 
do put spaces in their branch names for some reason.

> > Any
> > objections to requiring an --allow-discouraged-branch-names option to
> > create branch certs that don't match /^[^-,*+%][^,*+%]*$/?
>
> Hrm - should we really disallow them by default? Another option could be
> to just issue a warning and let the user go ahead. Wireing in the code
> which errors out in an invalid case and correctly rolling back might be
> cumbersome, given the fact that we have a couple of places from which we
> create branch certs (approve, commit -b, cert, automate cert, setup,
> import, cvs_import, ...)

A warning after the fact doesn't help much, by the time you see it 
you've already got your hard-to-work-with branch name. Unless we want to 
add a 'db rename_branch_locally' or similar to make this fixable 
after-the-fact, and then point that out in the warning. That might 
actually be the better solution.

> > I guess this would have to be after this upcoming release, due to the
> > new translatable strings it would have.
>
> Yes, definitely.
>
> Thomas.

--
Timothy

Free public monotone hosting: http://mtn-host.prjek.net