[PATCH] Added new send extension

[John Goerzen]

On Wed, Mar 21, 2007 at 07:30:48PM -0500, Matt Mackall wrote:
> > +    # present on windows
> > +    import readline
> > +except ImportError: pass
> 
> We're pretty consistent about doing a line break after colons.

On this and a lot of the other style questions, I have no problem
changing, but I should say I was following the style in patchbomb.py.
Some of the things you highlighted -- such as the above -- were actually
literally copied and pasted from it.  Would you like me to fix my code,
and also send in patches to fix the same things in patchbomb.py?

> > +try:
> > +    # Dev branch has internationalization function
> > +    from mercurial.i18n import _
> > +except ImportError:
> > +    def _(val):
> > +        return val
> 
> We'll want to drop this bit for merge.

Right, OK.

> > +    def prompt(prompt, default = None, rest = ': ', empty_ok = False):
> > +        if default: prompt += ' [%s]' % default
> > +        prompt += rest
> > +        while True:
> 
> ui.prompt already does this.

Excellent, I will modify that.  I can also send you patches against
patchbomb to make it do the same, if you like.

> > +        if not prompt(s, default = 'y', rest = '? ').lower().startswith('y'):
> > +            raise ValueError
> 
> And this should be done with ui.prompt.

Ditto this.

> > +    tmpdir = tempfile.mkdtemp(prefix='hg-send-')
> > +    tmpfn = os.path.join(tmpdir, "bundle")
> 
> Why not mkstemp? We should probably have a util function to do this
> sort of thing consistently.

Because the bundle command has no option to let it overwrite an existing
file.  mkstemp creates the file (it must, in order to be secure), and if
we remove the file and let bundle re-create it, we lose the security.

> Ideally we'd use generators to create and stream out the bundle, but
> that's obviously harder.

Yes, I noticed that patchbomb used that approach with hg export, but
bundle takes a filename only.

> > +        def genmsgid(id):
> > +            return '<%s.%s@%s>' % (id[:20], int(start_time[0]), socket.getfqdn())
> 
> This probably wants to be moved to mercurial.email.

OK, I can do that and also move the relevant code from patchbomb as
well.

There is a lot of code in patchbomb that is generally useful for tools
that send email.  Perhaps down the road I will send some additional
patches for consideration to move more into mercurial.mail.

> > +        try:
> > +            os.unlink(tmpfn)
> > +        except:
> > +            pass
> > +        os.rmdir(tmpdir)
> 
> This stuff should all be done with try: finally:.

Parse error.  What you quoted is part of a finally: block; this is the
code that cleans up the temporary directory.  The reason for the except:
pass for the call to unlink is that we don't want an error if an
exception was raised prior to being able to have bundle create the file.
But this whole chunk is part of a finally.

I will post a revised patch tomorrow.

-- John