Re: [Monotone-devel] Help on subcommands (help-rewrite branch)

[Julio M. Merino Vidal]

On 23/04/2007, at 16:26, Markus Schiltknecht wrote:

> Hi,
>
> Julio M. Merino Vidal wrote:
> > I think that the n.v.m.help-rewrite branch is in pretty good shape  
> > now.  Basically, it has the following changes over n.v.m:
>
> cool. Two questions popped up when reviewing your changes compared  
> to net.venge.monotone:
>
> I've stumbled across the following comment in cmd.hh:
>
>      // NB: these strings are stored _un_translated, because we cannot
>      // translate them until after main starts, by which time the
>      // command objects have all been constructed.
>
> Does your change from 'std::string name' to 'utf8 m_primary_name'  
> affect that in any way? Do you know what that comment means?

As far as I understand it, the strings in the monotone code are  
supposed to be utf8.  Therefore, it should be safe to store them in  
utf8 objects.  Note that, however, the getters for those values (the  
ones that do the translation) return a std::string.  (And for the  
specific case of m_primary_name... that one oughtn't be translated,  
so it seems to me it is safe to always handle it as utf8... isn't it?)

That's what I deduced from the wiki page on encodings and that  
comment.  But maybe I got it all wrong...

> Why is command a struct, but automate a class?

It was a struct before, but it now is a class.  I have changed all  
direct access to command's members to calls to getters for  
consistency.  I think it was quite a mess before (some fields were  
public but had to be accessed through getters, others not, etc.)

> Why does automate have an exec() and a run() method? Is it possible  
> to merge that into one?

It is just to avoid some code duplication.  exec is the function  
defined in the parent class, "command"; it does some setup for the  
automate command and delegates to the 'run' function.  'run' is not  
supposed to be called directly, but only through 'exec'.

Confusing, it seems.  I will rename 'run' to make this more clear and  
make it protected.

> [ BTW: your branch compiled and passed the testsuite successfully  
> on my (quite standard AMD Debian GNU/Linux) machine here at work. ]

Cool.  So far I'm only testing in Mac OS X where everything works  
too :-)

Thanks for your review!

--
Julio M. Merino Vidal <address@hidden>