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

[Mikkel Fahnøe Jørgensen]

On 02/02/2008, Matt Mackall <address@hidden> wrote:
On 03/02/2008, Maxim Dounin <address@hidden> wrote:

Thanks for the response - I'll reply to both emails here.

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

I guess the problem dictated the choice of language :-)

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

I'll fix this and the other style pointers.

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

Matt: > Why not just raise NoRepo("...")?

Because I use it in two places. First to check if the repository
exists, then - if the mtn tool is installed, I do an extra sanity
check by listing all heads.

Maxim: It's probably a good idea to use _() here to allow localization of
> error messages.

Will do.


> [...]
> >+    def mtncmd(self, arg):
> >+        cmdline = "mtn -d %s automate %s" % (util.shellquote(self.path), 
> >arg)
> >+        self.ui.debug(cmdline, '\n')
> >+        p = util.popen(cmdline)
> >+        result = p.read()
> >+        if p.close():
> >+            raise IOError()
> >+        return result
>
Maxim: > Please consider using commandline class for this instead. It looks
> for me that commandline.run0() does exactly what you want.
> Examples are in darcs_source and svn_sink code.

commandline.run0 raises Abort, not IOError.
The convert engine expects IOErrror in order to detect a deleted or
renamed file. Thus, when I try to call get_file_of a renamed file,
IOError is raised. I have later added a mtnisfile sanity check in
getfile that also handles this - but for maintainability I think it's
best to keep the IOError exception in mtncmd. Of course, a more
specific rename / delete interface in the convert engine would be
nice.

If mtn automate stdio is used, mtncmd would need to be changed to use
popen2 to access read and write sockets.

Mikkel