Re: [Monotone-devel] review of nvm.connection_info_cleanup

[Thomas Keller]

Am 17.06.2010 01:32, schrieb Stephen Leake:
> Thomas Keller <address@hidden> writes:
> 
>> The branch nvm.connection_info_cleanup should be feature-complete from
>> my side, all tests pass and the docs have been updated accordingly.
>> Please read over it and give me feedback if you find gotchas or think of
>> other improvements.
> 
> I reviewed monotone.texi, but not the code.
> 
> The documentation is pretty clear. However, I think in this syntax:
> 
> mtn automate pull [--set-default] address@hidden address@hidden [...] 
> address@hidden
> 
> the 'glob' should only be there if the 'uri-or-address' is the old style
> host address? If true, I think it would be better to list this as two lines:
> 
> mtn automate pull [--set-default] address@hidden [...] address@hidden
> 
> mtn automate pull [--set-default] address@hidden address@hidden [...] 
> address@hidden
> 
> Then it is easier to say the second one is deprecated. Similarly for
> push and sync, of course.

Yes, you're right, I'll change that accordingly.

> There needs to be a NEWS entry.

Of course.

> I compiled the code on MinGW; parse_date.cc needed the '#include
> "sanity.hh"' patch.
> 
> Running 'mtn sync' in an existing monotone workspace gave no warnings;
> that's good.
> 
> However, running with an explicit old-style address:
> 
> mtn  pull  monotone.ca  net.venge.monotone*
> 
> gave these messages:
> 
> mtn: connecting to mtn://monotone.ca
> mtn: first time connecting to server mtn://monotone.ca
> ...
> mtn: setting default include pattern for server 'mtn://monotone.ca' to 
> 'net.venge.monotone*'
> mtn: setting default exclude pattern for server 'mtn://monotone.ca' to ''

Are you sure you have the latest revision? These two progress messages
should no longer come up.

> This would be better if there was an explicit "you specified a
> deprecated argument" message first.

I can add one.

> In addition, the default-server and known-server db vars are now
> inconsistent:
> 
> database: default-server monotone.ca
> known-servers: monotone.ca 3e6f5225bc2fffacbc20c9de37ff2dae1e20892e
> known-servers: mtn://monotone.ca 3e6f5225bc2fffacbc20c9de37ff2dae1e20892e
> 
> Is this intended?

Lets say, its expected. I don't know how how I should properly address
this issue, i.e. if I should write a database migration which only
touches the _data_, not the schema. One could of course add some code to
handle this deprecated old variables and change them accordingly, but I
don't know if that is such a good idea (spaghetti, spaghetti!).

> mtn 0.46 is ok with these db vars, so I think this is ok in general.
> 
> 
> I also compiled on Cygwin to test ssh: and file: syncs. g++ reported
> missing '\' in uri.cc:
> 
> ../monotone.connection_info_cleanup/uri.cc: In function 'bool 
> try_parse_bare_authority(const std::string&, uri_t&, origin::type)':
> ../monotone.connection_info_cleanup/uri.cc:86: error: unknown escape sequence 
> '\]'
> 
> this: 
> 
>                        "\[[:0-9a-fA-F]+\](:[0-9]+)?" // bracketed ipv6
> 
> should be:
> 
>                        "\\[[:0-9a-fA-F]+\\](:[0-9]+)?" // bracketed ipv6

I'll fix that - thanks!

> With this current ssh: command line:
> 
> mtn --db /Stephe/monotone-dbs/gds.db sync \
> ssh://address@hidden/home/stephe/monotone-dbs/gds.db "*"
> 
> I get the error:
> 
> mtn: misuse: explicit port-number specification in URI has no digits
> 
> This syntax should be accepted. I was hoping for a 'deprecated' warning,
> that also gave the equivalent command in the new syntax.
> 
> Changing the command to:
> 
> mtn --db /Stephe/monotone-dbs/gds.db sync \
> "ssh://address@hidden/home/stephe/monotone-dbs/gds.db?*"
> 
> works.
> 
> I get the same error for file:
> 
> mtn --db /Stephe/monotone-dbs/gds.db sync --ticker=count 
> file:/cygdrive/f/gds.db "*"
> 
> mtn: misuse: explicit port-number specification in URI has no digits
> 
> Again, changing to the new URI syntax works.

Something is wrong with our URI parsing here, I will look into this one
later on.

> Both of the above sync commands set server vars in the db:
> 
> database: default-server 
> ssh://address@hidden/export/services/monotone/gds/gds.db
> server-exclude: file:/cygdrive/f/gds.db 
> server-exclude: ssh://address@hidden/home/stephe/monotone-dbs/gds.db 
> server-include: file:/cygdrive/f/gds.db *
> server-include: ssh://address@hidden/home/stephe/monotone-dbs/gds.db *
> 
> I guess that makes sense, but it did not happen with the current version
> of mtn.

Tim introduced the concept of a "resource" part in an URI which is
basically the complete URI without any query and hash part. Previously
we saved the complete URI (including the branch pattern!) as default
server which was certainly wrong, so I think this is more correct.

> The paragraph that describes saving these defaults talks about 'address'
> and 'glob'; it needs to be updated:
> 
>     The @command{pull}, @command{push}, and @command{sync} commands only
>     require you pass @var{address} and @var{glob} the first time you use
>     one of them; monotone will memorize this use and in the future
>     default to the same server and glob. For instance, if Bob wants to
>     @command{sync} with Alice again, he can simply run:

Oh, I forgot that indeed!

Thanks for the test and review!
Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | http://thomaskeller.biz
Please note that according to the EU law on data retention, information
on every electronic information exchange might be retained for a period
of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en

signature.asc
Description: OpenPGP digital signature