Re: [Monotone-devel] time for a release?

[Thomas Keller]

Am 18.05.2010 07:03, schrieb Derek Scherger:
> On Mon, May 17, 2010 at 2:33 AM, Thomas Keller <address@hidden> wrote:
> 
>> \2) I'd like to get my nvm.experiment.database-management branch ready
>> and merged in as well, so the change I did earlier to mtn setup (which
>> now creates a database if none is given) is changed to "create a
>> database in the default location or use the default database there". The
>> code compiles already and partially works, but I had to do a couple of
>> non-related changes to make database objects easier constructable and
>> other things, which make mtn currently crash badly. I'll commit my
>> current state tonight, so others have time to look at it. Maybe I'm on a
>> wrong path there.
>>
> 
> I had a quick look over your changes and they seem fairly reasonable. A few
> minor things:
> 
> We seem to be using lots of redundant std:: prefixes in various .cc files
> (not just in your changes) where we also have using std::foo declarations to
> avoid needing the std:: prefixes throughout the implementation. Do people
> have a general preference for explicit prefixes? Personally I much prefer a
> using declaration and less cluttered sources, iterators are already bad
> enough, sprinkling them with std:: makes them worse! ;)

I'm almost everytime use the std:: prefixes to make it explicit, but you
are right it makes everything even more hard to read. I'll change that
accordingly.

> Some of your changelog comments use very long lines and are somewhat hard to
> read without a tool that does automatic, but careful, wrapping (viewing
> these in emacs diff-mode is not particularly pleasant). Can you try and wrap
> these at some reasonable length please?

Of course, totally my fault. I'm too used to this style for guitone and
guitone properly wraps lines... :)
On the other hand one could see that as a feature request for the log
output - wrap long lines with an  \ at the end if f.e. some --wrap
option is given.

> Based on all the shenanigans with ".mtn" extensions it seems like maybe the
> path handling code could use a more formal concept of extension.

Hrm... I'd see this a bit out of scope for this branch, but if you're in
the mood, go on, I won't hold you off :)

> A shared_ptr for the lazy_rng would probably be better than a raw pointer.

I will change that - it was a raw pointer before and I haven't thought
about changing that.

> We should probably have a ":memory:" constant somewhere, rather than
> multiple string instances.

Good catch, will change that as well.

> I don't think you want to set the foo_given flags when reading options from
> the options file do you? Isn't the point of these to know whether the user
> specified such an option on the command line?

In this context it actually means if the particular stanza is written
out at all in _MTN/options, i.e. not empty. This may not make sense much
for the current options there, but I thought it is stringent to how the
command line options parser works. Since we use a options structure to
fill in the options from _MTN/options there, I aimed for 100%
compatibility with the command line parser.

Thanks for the 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