[PATCH 2 of 3] convert/mtn: use mtn "automate stdio" when available
Patrick Mézard
pmezard at gmail.com
Wed Mar 23 19:03:03 CDT 2011
Le 23/03/11 20:24, daniel.atallah at gmail.com a écrit :
> # HG changeset patch
> # User Daniel Atallah <daniel.atallah at gmail.com>
> # Date 1300904816 14400
> # Branch stable
> # Node ID d2ef3495b1a62785683fc2a57212e8f2d2500fc6
> # Parent 0fb617760b931cd25537e3362af52b2cfbd1a329
> convert/mtn: use mtn "automate stdio" when available
>
> This causes a single long-running mtn process to be used instead of spawning a
> new process per operation.
This is really good, even the short tests run faster with this code.
I have many remarks below, most of them related to coding style which you can fix easily. Two error checking may be improved. And I am a little worried about the byte by byte string concatenation but this can be fixed later.
> diff --git a/hgext/convert/monotone.py b/hgext/convert/monotone.py
> --- a/hgext/convert/monotone.py
> +++ b/hgext/convert/monotone.py
> @@ -19,6 +19,7 @@
>
> self.ui = ui
> self.path = path
> + self.automatestdio = False
>
> norepo = NoRepo(_("%s does not look like a monotone repository")
> % path)
> @@ -73,9 +74,105 @@
> self.rev = rev
>
> def mtnrun(self, *args, **kwargs):
> + if self.automatestdio:
> + return self.mtnrunstdio(*args, **kwargs)
> + else:
> + return self.mtnrunsingle(*args, **kwargs)
> +
> + def mtnrunsingle(self, *args, **kwargs):
> kwargs['d'] = self.path
> return self.run0('automate', *args, **kwargs)
>
> + def mtnrunstdio(self, *args, **kwargs):
> + #Prepare the command in automate stdio format
Nitpicking: please insert a space before the # and the rest of the comment
> + command = []
> + for k, v in kwargs.iteritems():
> + command.append("%s:%s" % (len(k), k))
> + try:
> + command.append("%s:%s" % (len(v), v))
> + except TypeError:
> + pass
What is the TypeError caused by? None perhaps? Could we filter these instead?
> + if len(command) > 0:
You can test the container directly here like:
if commands:
command.insert[...]
> + command.insert(0, 'o')
> + command.append('e')
> +
> + command.append('l')
> + for arg in args:
> + command += "%s:%s" % (len(arg), arg)
> + command.append('e')
> + commandstr = ''.join(command)
It's fine to reassign command to itself even if its type changes.
> +
> + self.ui.debug("mtn: Sending '%s'\n" % commandstr)
Lowercase in the message please.
> + self.mtnwritefp.write(commandstr)
> + self.mtnwritefp.flush()
> +
> + return self.mtnstdioreadcommandoutput(commandstr)
> +
> + def mtnstdioreadpacket(self):
> + read = None
> + commandnbr = ''
> + while not read == ':':
while read != ':'
> + read = self.mtnreadfp.read(1)
> + if read == '':
if not read:
> + raise util.Abort('bad mtn packet - no end of commandnbr')
Regular ui messages (everything else but debug() calls) should be marked for translations. This is done by using the _() function like:
raise util.Abort(_('bad mtn packet - no end of commandnbr'))
(just remember to wrap the string before parameter substitution if any).
> + commandnbr += read
Concatenating strings character by character is usually not a good idea performance-wise in python. It could be better to use a StringIO. That said you can let this for now, the use of a command server already makes thing much much faster.
> + commandnbr = commandnbr[:-1]
> +
> + stream = self.mtnreadfp.read(1)
> + if not stream in ['m', 'e', 'w', 'p', 't', 'l']:
People usually prefer tuples for constant sequences of items. And here you can even use a string like:
if stream not in 'mewptl':
> + raise util.Abort('bad mtn packet - bad stream type %s' % stream)
> +
> + read = self.mtnreadfp.read(1)
> + if not read == ':':
if read != ':'
> + raise util.Abort('bad mtn packet - no divider before size')
> +
> + read = None
> + lengthstr = ''
> + while not read == ':':
> + read = self.mtnreadfp.read(1)
> + if read == '':
> + raise util.Abort('bad mtn packet - no end of packet size')
> + lengthstr += read
> + try:
> + length = long(lengthstr[:-1])
> + except TypeError:
> + raise util.Abort('bad mtn packet - bad packet size %s' % lengthstr)
> +
> + read = self.mtnreadfp.read(length)
> + if not len(read) == length:
if len(read) != length:
> + raise util.Abort('bad mtn packet - unable to read full packet " \
> + " read %s of %s' % (len(read), length))
> +
> + return (commandnbr, stream, length, read)
> +
> + def mtnstdioreadcommandoutput(self, command):
> + retval = ''
> + done = False
> + while not done:
> + commandnbr, stream, length, output = self.mtnstdioreadpacket()
> + self.ui.debug('mtn: read packet %s:%s:%s\n' %
> + (commandnbr, stream, length))
> +
> + if stream == 'l':
> + #End of command
> + if not output == '0':
> + raise util.Abort('mtn command \'%s\' returned %s' %
You can use double quotes and single quotes interchangeably. So instead of escaping the single quotes:
raise util.Abort("mtn command '%s' returned %s" %
> + (command, output))
> + done = True
> + elif stream in ['e', 'w']:
> + #Error, warning output
> + self.ui.warn(_('%s error:\n') % self.command)
> + self.ui.warn(output)
> + elif stream == 'p':
> + #Progress messages
> + self.ui.debug('mtn: ' + output)
> + elif stream == 'm':
> + #Main stream - command output
> + retval = output
> +
> + return retval
> +
> +
> def mtnloadmanifest(self, rev):
> if self.manifest_rev == rev:
> return
> @@ -225,3 +322,43 @@
> # This function is only needed to support --filemap
> # ... and we don't support that
> raise NotImplementedError()
> +
> + def before(self):
> + #Check if we have a new enough version to use automate stdio
> + version = 0.0
> + try:
> + versionstr = self.mtnrunsingle("interface_version")
> + version = float(versionstr)
> + except:
Prefer:
except Exception:
the bare version usually catches too many things.
> + raise util.Abort("unable to determine mtn automate interface "
> + "version")
> +
> + if version >= 12.0:
> + self.automatestdio = True
> + self.ui.debug("mtn automate version %s - using automate stdio\n" %
> + (version))
No need to put single format parameters in parenthesis:
self.ui.debug("mtn automate version %s - using automate stdio\n" %
version)
> +
> + #Mark commandline not to redirect stdin
> + self.redirectstdin = False
> + #launch the long-running automate stdio process
> + self.mtnwritefp, self.mtnreadfp = self._run2('automate', 'stdio',
> + '-d', self.path)
> + #read the headers
> + read = self.mtnreadfp.readline()
> + if not read == 'format-version: 2\n':
if read != 'format-version: 2\n':
> + raise util.Abort('mtn automate stdio header unexpected: %s' %
> + (read))
> + while not read == '\n':
while read != '\n':
> + read = self.mtnreadfp.readline()
> + if read == '':
if not read:
> + raise util.Abort("failed to reach end of mtn automate stdio"
> + " headers")
> + else:
> + self.ui.debug("mtn automate version %s - not using automate stdio "
> + "(automate >= 12.0 - mtn >= 0.46 is needed)\n" %(version))
> +
> + def after(self):
> + if self.automatestdio:
> + self.mtnwritefp.close()
> + self.mtnreadfp.close()
More information about the Mercurial-devel
mailing list