[Monotone-devel] Re: patch: monotone source for mercurial convert extension

[Matt Mackall]

On Sat, 2008-02-02 at 19:39 +0100, Mikkel Fahnøe Jørgensen wrote:
> Here is an export from monotone to mercurial.
> 
> It is based on the mecurial convert extension in hgext/convert.
> It's my first Python program, so ...

Always makes me smile when people use Mercurial to try writing some
Python.

Your patch has some line wrap damage but mostly looks sane.

> Tested with monotone automation interface v. 6.0, monotone version 
> 0.38 on OS-X 10.4 Intel and current mercurial source tip.
> 
> examples:
> 
> hg convert mydb.mtn mynewdb.hg
> hg convert --authors myauthormappingfile --datesort mydb.mtn mynewdb.hg
> hg convert --debug mydb.mtn mynewdb.hg
> 
> If this patch is accepted for mercurial, I wouldn't mind if a monotone
> core developer took over maintainance, since the automation interface
> seems to change occasionally.

Hmmm, that's a little worrisome.

A few style pointers: 
> +# monotone support for the convert extension
> +
> +import os
> +import re
> +import time

Put these all on one line.

> +from mercurial import util
> +
> +from common import NoRepo, commit, converter_source, checktool
> +
> +class monotone_source(converter_source):
> +    def __init__(self, ui, path=None, rev=None):
> +        converter_source.__init__(self, ui, path, rev)
> +
> +        self.ui = ui
> +        self.path = path
> +
> +

One line of whitespace is plenty.

> +        # regular expressions for parsing monotone output
> +
> +        space    = r'\s*'
> +        name     = r'\s+"((?:[^"]|\\")*)"\s*'
> +        value    = name
> +        revision = r'\s+\[(\w+)\]\s*'
> +        lines    = r'(?:.|\n)+'
> +
> +        self.dir_re      = re.compile(space + "dir"      + name)
> +        self.file_re     = re.compile(space + "file"     + name +
> "content" + revision)
> +        self.add_file_re = re.compile(space + "add_file" + name +
> "content" + revision)
> +        self.patch_re    = re.compile(space + "patch"    + name +
> "from" + revision + "to" + revision)
> +        self.rename_re   = re.compile(space + "rename"   + name + "to" + 
> name)
> +        self.tag_re      = re.compile(space + "tag"      + name +
> "revision" + revision)
> +        self.cert_re     = re.compile(lines + space + "name" + name +
> "value" + value)

I'm generally not a fan of making things line up like this..

> +        norepo = NoRepo("%s does not look like a monotone repo" % path)
> +        if not os.path.exists(path):
> +            raise norepo

Why not just raise NoRepo("...")?

> +      &#-1;&#-1; try :

No space before the colon, please.

-- 
Mathematics is the supreme nostalgia of our time.