[StGit RFC PATCH 0/6] add support for git send-email

[StGit RFC PATCH 0/6] add support for git send-email

[Alex Chiang]

Hi Catalin,

This series starts down the path of eventually converting stg mail to
simply become a wrapper around git send-email.

The first 4 patches do some refactoring of internal APIs to set us up
for the money patch 5/6 which actually adds the call to git send-email.

Patch 6/6 shows a hint of the future, where we can start leveraging
some of the features in git send-email for our own purposes. With it,
you can now use mail aliases as supported by git send-email.

stg mail still has some nice features over git send-email, such
as the -v command line parameter and --prefix. Maybe at some point
in the future, we can migrate those features into git send-email and
continue thinning out stg mail.

But I wanted to get some feedback first to make sure I'm going in the
right direction before going too much further.

Disclaimer: I'm not really a python coder. Particularly, patch 5/6
is ugly in how we look at the various stg mail options. I'm sure there's
lots of room for improvement.

This mail was sent with:
	./stg mail -a --git --auto -e 

[Karl, sorry about the earlier accidental mail. That was a testing mishap.]

Thanks,
/ac

---

Alex Chiang (6):
      stg mail: Refactor __send_message and friends
      stg mail: reorder __build_[message|cover] parameters
      stg mail: make __send_message do more
      stg mail: factor out __update_header
      stg mail: add basic support for git send-email
      stg mail: don't parse To/Cc/Bcc in --git mode


 stgit/commands/mail.py |  205 ++++++++++++++++++++++++++++--------------------
 1 files changed, 121 insertions(+), 84 deletions(-)

[PATCH 1/6] stg mail: Refactor __send_message and friends

[Alex Chiang]

Instead of passing all the various smtp* args to __send_message
individually, let's just pass the options list instead.

The main motivation is for future patches. The end goal is to
thin out stg mail's implementation and make it a minimal wrapper
around git send-email. By passing the options list to __send_message
we prepare to pass options directly to git send-email.

As a bonus, this change results in a cleaner internal API.

Finally, it also pushes the smtp logic where it belongs, viz. into
__send_message_smtp, instead of cluttering up the main body of
mail.func().

Cc: Karl Wiberg <kha@treskal.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

 stgit/commands/mail.py |   43 +++++++++++++++++++------------------------
 1 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index abd42e4..3978f5e 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -190,10 +190,20 @@ def __send_message_sendmail(sendmail, msg):
     cmd = sendmail.split()
     Run(*cmd).raw_input(msg).discard_output()
 
-def __send_message_smtp(smtpserver, from_addr, to_addr_list, msg,
-                        smtpuser, smtppassword, use_tls):
+def __send_message_smtp(smtpserver, from_addr, to_addr_list, msg, options):
     """Send the message using the given SMTP server
     """
+    smtppassword = options.smtp_password or config.get('stgit.smtppassword')
+    smtpuser = options.smtp_user or config.get('stgit.smtpuser')
+    smtpusetls = options.smtp_tls or config.get('stgit.smtptls') == 'yes'
+
+    if (smtppassword and not smtpuser):
+        raise CmdException, 'SMTP password supplied, username needed'
+    if (smtpusetls and not smtpuser):
+        raise CmdException, 'SMTP over TLS requested, username needed'
+    if (smtpuser and not smtppassword):
+        smtppassword = getpass.getpass("Please enter SMTP password: ")
+
     try:
         s = smtplib.SMTP(smtpserver)
     except Exception, err:
@@ -203,7 +213,7 @@ def __send_message_smtp(smtpserver, from_addr, to_addr_list, msg,
     try:
         if smtpuser and smtppassword:
             s.ehlo()
-            if use_tls:
+            if smtpusetls:
                 if not hasattr(socket, 'ssl'):
                     raise CmdException,  "cannot use TLS - no SSL support in Python"
                 s.starttls()
@@ -218,17 +228,17 @@ def __send_message_smtp(smtpserver, from_addr, to_addr_list, msg,
 
     s.quit()
 
-def __send_message(smtpserver, from_addr, to_addr_list, msg,
-                   smtpuser, smtppassword, use_tls):
+def __send_message(from_addr, to_addr_list, msg, options):
     """Message sending dispatcher.
     """
+    smtpserver = options.smtp_server or config.get('stgit.smtpserver')
+
     if smtpserver.startswith('/'):
         # Use the sendmail tool
         __send_message_sendmail(smtpserver, msg)
     else:
         # Use the SMTP server (we have host and port information)
-        __send_message_smtp(smtpserver, from_addr, to_addr_list, msg,
-                            smtpuser, smtppassword, use_tls)
+        __send_message_smtp(smtpserver, from_addr, to_addr_list, msg, options)
 
 def __build_address_headers(msg, options, extra_cc = []):
     """Build the address headers and check existing headers in the
@@ -543,8 +553,6 @@ def func(parser, options, args):
     """Send the patches by e-mail using the patchmail.tmpl file as
     a template
     """
-    smtpserver = options.smtp_server or config.get('stgit.smtpserver')
-
     applied = crt_series.get_applied()
 
     if options.all:
@@ -564,17 +572,6 @@ def func(parser, options, args):
             raise CmdException, 'Cannot send empty patch "%s"' % p
     out.done()
 
-    smtppassword = options.smtp_password or config.get('stgit.smtppassword')
-    smtpuser = options.smtp_user or config.get('stgit.smtpuser')
-    smtpusetls = options.smtp_tls or config.get('stgit.smtptls') == 'yes'
-
-    if (smtppassword and not smtpuser):
-        raise CmdException, 'SMTP password supplied, username needed'
-    if (smtpusetls and not smtpuser):
-        raise CmdException, 'SMTP over TLS requested, username needed'
-    if (smtpuser and not smtppassword):
-        smtppassword = getpass.getpass("Please enter SMTP password: ")
-
     total_nr = len(patches)
     if total_nr == 0:
         raise CmdException, 'No patches to send'
@@ -616,8 +613,7 @@ def func(parser, options, args):
             out.stdout_raw(msg_string + '\n')
         else:
             out.start('Sending the cover message')
-            __send_message(smtpserver, from_addr, to_addr_list, msg_string,
-                           smtpuser, smtppassword, smtpusetls)
+            __send_message(from_addr, to_addr_list, msg_string, options)
             time.sleep(sleep)
             out.done()
 
@@ -648,8 +644,7 @@ def func(parser, options, args):
             out.stdout_raw(msg_string + '\n')
         else:
             out.start('Sending patch "%s"' % p)
-            __send_message(smtpserver, from_addr, to_addr_list, msg_string,
-                           smtpuser, smtppassword, smtpusetls)
+            __send_message(from_addr, to_addr_list, msg_string, options)
             # give recipients a chance of receiving related patches in the
             # correct order.
             if patch_nr < total_nr:

[PATCH 2/6] stg mail: reorder __build_[message|cover] parameters

[Alex Chiang]

Reorder the argument lists for both __build_cover and __build_message.

This change will aid readability of a subsequent refactoring patch.

Cc: Karl Wiberg <kha@treskal.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

 stgit/commands/mail.py |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 3978f5e..b6fc3d9 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -370,7 +370,7 @@ def __edit_message(msg):
 
     return msg
 
-def __build_cover(tmpl, patches, msg_id, options):
+def __build_cover(tmpl, msg_id, options, patches):
     """Build the cover message (series description) to be sent via SMTP
     """
     sender = __get_sender()
@@ -439,7 +439,7 @@ def __build_cover(tmpl, patches, msg_id, options):
 
     return msg
 
-def __build_message(tmpl, patch, patch_nr, total_nr, msg_id, ref_id, options):
+def __build_message(tmpl, msg_id, options, patch, patch_nr, total_nr, ref_id):
     """Build the message to be sent via SMTP
     """
     p = crt_series.get_patch(patch)
@@ -600,7 +600,7 @@ def func(parser, options, args):
                 raise CmdException, 'No cover message template file found'
 
         msg_id = email.Utils.make_msgid('stgit')
-        msg = __build_cover(tmpl, patches, msg_id, options)
+        msg = __build_cover(tmpl, msg_id, options, patches)
         from_addr, to_addr_list = __parse_addresses(msg)
 
         msg_string = msg.as_string(options.mbox)
@@ -630,8 +630,8 @@ def func(parser, options, args):
 
     for (p, patch_nr) in zip(patches, range(1, total_nr + 1)):
         msg_id = email.Utils.make_msgid('stgit')
-        msg = __build_message(tmpl, p, patch_nr, total_nr, msg_id, ref_id,
-                              options)
+        msg = __build_message(tmpl, msg_id, options, p, patch_nr, total_nr,
+                              ref_id)
         from_addr, to_addr_list = __parse_addresses(msg)
 
         msg_string = msg.as_string(options.mbox)

[PATCH 3/6] stg mail: make __send_message do more

[Alex Chiang]

Factor out the common code required to send either a cover mail
or patch, and implement it in __send_message.

WRY? DRY.

Cc: Karl Wiberg <kha@treskal.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

 stgit/commands/mail.py |   61 ++++++++++++++++++++----------------------------
 1 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index b6fc3d9..fe5742e 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -228,17 +228,37 @@ def __send_message_smtp(smtpserver, from_addr, to_addr_list, msg, options):
 
     s.quit()
 
-def __send_message(from_addr, to_addr_list, msg, options):
+def __send_message(tmpl, options, *args):
     """Message sending dispatcher.
     """
-    smtpserver = options.smtp_server or config.get('stgit.smtpserver')
+    msg_id = email.Utils.make_msgid('stgit')
+    build = { 1: __build_cover, 4: __build_message }
+    msg = build[len(args)](tmpl, msg_id, options, *args)
+
+    from_addr, to_addrs = __parse_addresses(msg)
+    msg_str = msg.as_string(options.mbox)
+    if options.mbox:
+        out.stdout_raw(msg_str + '\n')
+        return msg_id
+
+    outstr = { 1: 'the cover message', 4: 'patch "%s"' % args[0] }
+    out.start('Sending ' + outstr[len(args)])
 
+    smtpserver = options.smtp_server or config.get('stgit.smtpserver')
     if smtpserver.startswith('/'):
         # Use the sendmail tool
-        __send_message_sendmail(smtpserver, msg)
+        __send_message_sendmail(smtpserver, msg_str)
     else:
         # Use the SMTP server (we have host and port information)
-        __send_message_smtp(smtpserver, from_addr, to_addr_list, msg, options)
+        __send_message_smtp(smtpserver, from_addr, to_addrs, msg_str, options)
+
+    # give recipients a chance of receiving related patches in correct order
+    #                                       patch_nr < total_nr
+    if len(args) == 1 or (len(args) == 4 and args[1] < args[2]):
+        sleep = options.sleep or config.getint('stgit.smtpdelay')
+        time.sleep(sleep)
+    out.done()
+    return msg_id
 
 def __build_address_headers(msg, options, extra_cc = []):
     """Build the address headers and check existing headers in the
@@ -584,7 +604,6 @@ def func(parser, options, args):
     else:
         ref_id = None
 
-    sleep = options.sleep or config.getint('stgit.smtpdelay')
 
     # send the cover message (if any)
     if options.cover or options.edit_cover:
@@ -599,24 +618,12 @@ def func(parser, options, args):
             if not tmpl:
                 raise CmdException, 'No cover message template file found'
 
-        msg_id = email.Utils.make_msgid('stgit')
-        msg = __build_cover(tmpl, msg_id, options, patches)
-        from_addr, to_addr_list = __parse_addresses(msg)
-
-        msg_string = msg.as_string(options.mbox)
+        msg_id = __send_message(tmpl, options, patches)
 
         # subsequent e-mails are seen as replies to the first one
         if not options.noreply:
             ref_id = msg_id
 
-        if options.mbox:
-            out.stdout_raw(msg_string + '\n')
-        else:
-            out.start('Sending the cover message')
-            __send_message(from_addr, to_addr_list, msg_string, options)
-            time.sleep(sleep)
-            out.done()
-
     # send the patches
     if options.template:
         tmpl = file(options.template).read()
@@ -629,24 +636,8 @@ def func(parser, options, args):
             raise CmdException, 'No e-mail template file found'
 
     for (p, patch_nr) in zip(patches, range(1, total_nr + 1)):
-        msg_id = email.Utils.make_msgid('stgit')
-        msg = __build_message(tmpl, msg_id, options, p, patch_nr, total_nr,
-                              ref_id)
-        from_addr, to_addr_list = __parse_addresses(msg)
-
-        msg_string = msg.as_string(options.mbox)
+        msg_id = __send_message(tmpl, options, p, patch_nr, total_nr, ref_id)
 
         # subsequent e-mails are seen as replies to the first one
         if not options.noreply and not options.unrelated and not ref_id:
             ref_id = msg_id
-
-        if options.mbox:
-            out.stdout_raw(msg_string + '\n')
-        else:
-            out.start('Sending patch "%s"' % p)
-            __send_message(from_addr, to_addr_list, msg_string, options)
-            # give recipients a chance of receiving related patches in the
-            # correct order.
-            if patch_nr < total_nr:
-                time.sleep(sleep)
-            out.done()

[PATCH 4/6] stg mail: factor out __update_header

[Alex Chiang]

Factor __update_header out of __build_address_headers.

Headers like Reply-To, Mail-Reply-To, and Mail-Followup-To are now
handled in __build_extra_headers.

We make this change because in the future, we do not want to call
__build_address_headers if using git send-email but we will always
want to call __build_extra_headers.

Cc: Karl Wiberg <kha@treskal.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

 stgit/commands/mail.py |   48 +++++++++++++++++++++++++-----------------------
 1 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index fe5742e..7f811e8 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -260,25 +260,25 @@ def __send_message(tmpl, options, *args):
     out.done()
     return msg_id
 
-def __build_address_headers(msg, options, extra_cc = []):
-    """Build the address headers and check existing headers in the
-    template.
-    """
+def __update_header(msg, header, addr = '', ignore = ()):
     def __addr_pairs(msg, header, extra):
         pairs = email.Utils.getaddresses(msg.get_all(header, []) + extra)
         # remove pairs without an address and resolve the aliases
         return [address_or_alias(p) for p in pairs if p[1]]
 
-    def __update_header(header, addr = '', ignore = ()):
-        addr_pairs = __addr_pairs(msg, header, [addr])
-        del msg[header]
-        # remove the duplicates and filter the addresses
-        addr_dict = dict((addr, email.Utils.formataddr((name, addr)))
-                         for name, addr in addr_pairs if addr not in ignore)
-        if addr_dict:
-            msg[header] = ', '.join(addr_dict.itervalues())
-        return set(addr_dict.iterkeys())
+    addr_pairs = __addr_pairs(msg, header, [addr])
+    del msg[header]
+    # remove the duplicates and filter the addresses
+    addr_dict = dict((addr, email.Utils.formataddr((name, addr)))
+                     for name, addr in addr_pairs if addr not in ignore)
+    if addr_dict:
+        msg[header] = ', '.join(addr_dict.itervalues())
+    return set(addr_dict.iterkeys())
 
+def __build_address_headers(msg, options, extra_cc = []):
+    """Build the address headers and check existing headers in the
+    template.
+    """
     to_addr = ''
     cc_addr = ''
     extra_cc_addr = ''
@@ -298,18 +298,14 @@ def __build_address_headers(msg, options, extra_cc = []):
         bcc_addr = autobcc
 
     # if an address is on a header, ignore it from the rest
-    to_set = __update_header('To', to_addr)
-    cc_set = __update_header('Cc', cc_addr, to_set)
-    bcc_set = __update_header('Bcc', bcc_addr, to_set.union(cc_set))
+    to_set = __update_header(msg, 'To', to_addr)
+    cc_set = __update_header(msg, 'Cc', cc_addr, to_set)
+    bcc_set = __update_header(msg, 'Bcc', bcc_addr, to_set.union(cc_set))
 
     # --auto generated addresses, don't include the sender
-    from_set = __update_header('From')
-    __update_header('Cc', extra_cc_addr, to_set.union(bcc_set).union(from_set))
-
-    # update other address headers
-    __update_header('Reply-To')
-    __update_header('Mail-Reply-To')
-    __update_header('Mail-Followup-To')
+    from_set = __update_header(msg, 'From')
+    __update_header(msg, 'Cc', extra_cc_addr,
+                    to_set.union(bcc_set).union(from_set))
 
 def __get_signers_list(msg):
     """Return the address list generated from signed-off-by and
@@ -347,6 +343,12 @@ def __build_extra_headers(msg, msg_id, ref_id = None):
         msg['References'] = ref_id
     msg['User-Agent'] = 'StGit/%s' % version.version
 
+    # update other address headers
+    __update_header(msg, 'Reply-To')
+    __update_header(msg, 'Mail-Reply-To')
+    __update_header(msg, 'Mail-Followup-To')
+
+
 def __encode_message(msg):
     # 7 or 8 bit encoding
     charset = email.Charset.Charset('utf-8')

[PATCH 5/6] stg mail: add basic support for git send-email

[Alex Chiang]

This is the first step in turning stg mail into a wrapper for
git send-email. It requires passing the --git option to stg mail
for now.

Only a few basic options are supported for now, namely To/Cc/Bcc.

git send-email options used:
  --suppress-cc=self	prevent further information prompts
  --quiet		reduce git send-email output

Cc: Karl Wiberg <kha@treskal.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

 stgit/commands/mail.py |   60 +++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 7f811e8..81ec77e 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -140,7 +140,9 @@ options = [
     opt('-b', '--branch', args = [argparse.stg_branches],
         short = 'Use BRANCH instead of the default branch'),
     opt('-m', '--mbox', action = 'store_true',
-        short = 'Generate an mbox file instead of sending')
+        short = 'Generate an mbox file instead of sending'),
+    opt('--git', action = 'store_true',
+        short = 'Use git send-email (EXPERIMENTAL)')
     ] + argparse.diff_opts_option()
 
 directory = DirectoryHasRepository(log = False)
@@ -228,6 +230,52 @@ def __send_message_smtp(smtpserver, from_addr, to_addr_list, msg, options):
 
     s.quit()
 
+def __send_message_git(msg, options):
+    """Send the message using git send-email
+    """
+    from subprocess import call
+    from tempfile import mkstemp
+
+    cmd = ["git", "send-email", "--from=%s" % msg['From']]
+    cmd.append("--quiet")
+    cmd.append("--suppress-cc=self")
+
+    # XXX: yuck, there's gotta be a more pythonic way. Ideally we'd like
+    # to use the git_opts dictionary as our mapping between stg mail and
+    # git send-email; extract k, v pairs from git_opts, and use those
+    # to iterate across options somehow.
+    git_opts = { 'to': '--to=', 'cc': '--cc=', 'bcc': '--bcc=' }
+    if options.to:
+        for a in options.to:
+            cmd.append("--to=%s" % a)
+    if options.cc:
+        for a in options.cc:
+            cmd.append("--cc=%s" % a)
+    if options.bcc:
+        for a in options.bcc:
+            cmd.append("--bcc=%s" % a)
+    if not options.auto:
+        cmd.append("--suppress-cc=body")
+
+    # XXX: hack for now so that we don't duplicate To/Cc/Bcc headers
+    # in the mail, as git send-email inserts those for us.
+    del msg['To']
+    del msg['Cc']
+    del msg['Bcc']
+
+    (fd, path) = mkstemp()
+    os.write(fd, msg.as_string(options.mbox))
+    os.close(fd)
+
+    try:
+        cmd.append(path)
+        call(cmd)
+    except Exception, err:
+        os.unlink(path)
+        raise CmdException, str(err)
+
+    os.unlink(path)
+
 def __send_message(tmpl, options, *args):
     """Message sending dispatcher.
     """
@@ -242,10 +290,13 @@ def __send_message(tmpl, options, *args):
         return msg_id
 
     outstr = { 1: 'the cover message', 4: 'patch "%s"' % args[0] }
-    out.start('Sending ' + outstr[len(args)])
+    if not options.git:
+        out.start('Sending ' + outstr[len(args)])
 
     smtpserver = options.smtp_server or config.get('stgit.smtpserver')
-    if smtpserver.startswith('/'):
+    if options.git:
+        __send_message_git(msg, options)
+    elif smtpserver.startswith('/'):
         # Use the sendmail tool
         __send_message_sendmail(smtpserver, msg_str)
     else:
@@ -257,7 +308,8 @@ def __send_message(tmpl, options, *args):
     if len(args) == 1 or (len(args) == 4 and args[1] < args[2]):
         sleep = options.sleep or config.getint('stgit.smtpdelay')
         time.sleep(sleep)
-    out.done()
+    if not options.git:
+        out.done()
     return msg_id
 
 def __update_header(msg, header, addr = '', ignore = ()):

[PATCH 6/6] stg mail: don't parse To/Cc/Bcc in --git mode

[Alex Chiang]

When using stg mail in --git mode, do not parse command-line To/Cc/Bcc
addresses.

Instead, we pass them directly to git send-email.

This allows us to leverage git send-email's support for email aliases.

Cc: Karl Wiberg <kha@treskal.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

 stgit/commands/mail.py |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 81ec77e..c01e14b 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -257,12 +257,6 @@ def __send_message_git(msg, options):
     if not options.auto:
         cmd.append("--suppress-cc=body")
 
-    # XXX: hack for now so that we don't duplicate To/Cc/Bcc headers
-    # in the mail, as git send-email inserts those for us.
-    del msg['To']
-    del msg['Cc']
-    del msg['Bcc']
-
     (fd, path) = mkstemp()
     os.write(fd, msg.as_string(options.mbox))
     os.close(fd)
@@ -283,7 +277,8 @@ def __send_message(tmpl, options, *args):
     build = { 1: __build_cover, 4: __build_message }
     msg = build[len(args)](tmpl, msg_id, options, *args)
 
-    from_addr, to_addrs = __parse_addresses(msg)
+    if not options.git:
+        from_addr, to_addrs = __parse_addresses(msg)
     msg_str = msg.as_string(options.mbox)
     if options.mbox:
         out.stdout_raw(msg_str + '\n')
@@ -507,7 +502,8 @@ def __build_cover(tmpl, msg_id, options, patches):
     except Exception, ex:
         raise CmdException, 'template parsing error: %s' % str(ex)
 
-    __build_address_headers(msg, options)
+    if not options.git:
+        __build_address_headers(msg, options)
     __build_extra_headers(msg, msg_id, options.refid)
     __encode_message(msg)
 
@@ -617,7 +613,8 @@ def __build_message(tmpl, msg_id, options, patch, patch_nr, total_nr, ref_id):
     else:
         extra_cc = []
 
-    __build_address_headers(msg, options, extra_cc)
+    if not options.git:
+        __build_address_headers(msg, options, extra_cc)
     __build_extra_headers(msg, msg_id, ref_id)
     __encode_message(msg)
 

Re: [PATCH 1/6] stg mail: Refactor __send_message and friends

[Karl Wiberg]

Alex Chiang wrote:

> Instead of passing all the various smtp* args to __send_message
> individually, let's just pass the options list instead.

Looks good.

> +    if (smtppassword and not smtpuser):
> +        raise CmdException, 'SMTP password supplied, username needed'
> +    if (smtpusetls and not smtpuser):
> +        raise CmdException, 'SMTP over TLS requested, username needed'

Python style nit: Use "raise Exception('message')" in new code. (And
yes, I know you just moved these lines around.)

-- 
Karl Wiberg, kha@treskal.com
    subrabbit.wordpress.com
    www.treskal.com/kalle

Re: [PATCH 3/6] stg mail: make __send_message do more

[Karl Wiberg]

On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:

> Factor out the common code required to send either a cover mail
> or patch, and implement it in __send_message.

Nice code size reduction.

> +    msg_id = email.Utils.make_msgid('stgit')
> +    build = { 1: __build_cover, 4: __build_message }
> +    msg = build[len(args)](tmpl, msg_id, options, *args)
> +
> +    from_addr, to_addrs = __parse_addresses(msg)
> +    msg_str = msg.as_string(options.mbox)
> +    if options.mbox:
> +        out.stdout_raw(msg_str + '\n')
> +        return msg_id
> +
> +    outstr = { 1: 'the cover message', 4: 'patch "%s"' % args[0] }
> +    out.start('Sending ' + outstr[len(args)])

You could consolidate the two dictionaries like this, to avoid making
the same choice twice and make the code more pleasant to read:

  (build, outstr) = { 1: (__build_cover, 'the cover message'), 4:
(__build_message, 'patch "%s"' % args[0]) }

> +    # give recipients a chance of receiving related patches in correct order
> +    #                                       patch_nr < total_nr
> +    if len(args) == 1 or (len(args) == 4 and args[1] < args[2]):
> +        sleep = options.sleep or config.getint('stgit.smtpdelay')
> +        time.sleep(sleep)

Hmm. I must say I find all the args[x] a bit hard to read. I'd prefer
symbolic names.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle

Re: [PATCH 5/6] stg mail: add basic support for git send-email

[Karl Wiberg]

On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:

> +    # XXX: yuck, there's gotta be a more pythonic way. Ideally we'd like
> +    # to use the git_opts dictionary as our mapping between stg mail and
> +    # git send-email; extract k, v pairs from git_opts, and use those
> +    # to iterate across options somehow.
> +    git_opts = { 'to': '--to=', 'cc': '--cc=', 'bcc': '--bcc=' }
> +    if options.to:
> +        for a in options.to:
> +            cmd.append("--to=%s" % a)
> +    if options.cc:
> +        for a in options.cc:
> +            cmd.append("--cc=%s" % a)
> +    if options.bcc:
> +        for a in options.bcc:
> +            cmd.append("--bcc=%s" % a)
> +    if not options.auto:
> +        cmd.append("--suppress-cc=body")

Like this?

for x in ['to', 'cc', 'bcc']:
    if getattr(options, x):
        cmd.extend('--%s=%s' % (x, a) for a in getattr(options, x))

> +    (fd, path) = mkstemp()
> +    os.write(fd, msg.as_string(options.mbox))
> +    os.close(fd)
> +
> +    try:
> +        cmd.append(path)
> +        call(cmd)
> +    except Exception, err:
> +        os.unlink(path)
> +        raise CmdException, str(err)
> +
> +    os.unlink(path)

To avoid having to remember to call unlink in all paths, you can write

try:
    try:
        cmd.append(path)
        call(cmd)
    except Exception, e:
        raise CmdException(str(e))
finally:
    os.unlink(path)

(The combined try...except...finally statement didn't appear until
python 2.5, but we'd like to stay compatible with 2.4.)

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle

Re: [StGit RFC PATCH 0/6] add support for git send-email

[Karl Wiberg]

On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:

> stg mail still has some nice features over git send-email, such
> as the -v command line parameter and --prefix. Maybe at some point
> in the future, we can migrate those features into git send-email and
> continue thinning out stg mail.

Yes. But note that we tend to be conservative and not require a
too-new git, so a patch adding such a dependency would have to wait a
while. (I'm currently carrying two such patches in my experimental
branch.)

> But I wanted to get some feedback first to make sure I'm going in the
> right direction before going too much further.

I've read the patches, and it looks about right from where I stand.

Did you remember to run the regression tests? It's very helpful when
reviewing to know that the regression suite passes at every point in
the series.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle

Re: [PATCH 1/6] stg mail: Refactor __send_message and friends

[Alex Chiang]

* Karl Wiberg <kha@treskal.com>:
> Alex Chiang wrote:
>
>> +    if (smtppassword and not smtpuser):
>> +        raise CmdException, 'SMTP password supplied, username needed'
>> +    if (smtpusetls and not smtpuser):
>> +        raise CmdException, 'SMTP over TLS requested, username needed'
>
> Python style nit: Use "raise Exception('message')" in new code. (And
> yes, I know you just moved these lines around.)

Changed, thanks.

Re: [PATCH 3/6] stg mail: make __send_message do more

[Alex Chiang]

* Karl Wiberg <kha@treskal.com>:
> On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:
> 
> > Factor out the common code required to send either a cover mail
> > or patch, and implement it in __send_message.
> 
> Nice code size reduction.

Thanks.

> > +    msg_id = email.Utils.make_msgid('stgit')
> > +    build = { 1: __build_cover, 4: __build_message }
> > +    msg = build[len(args)](tmpl, msg_id, options, *args)
> > +
> > +    from_addr, to_addrs = __parse_addresses(msg)
> > +    msg_str = msg.as_string(options.mbox)
> > +    if options.mbox:
> > +        out.stdout_raw(msg_str + '\n')
> > +        return msg_id
> > +
> > +    outstr = { 1: 'the cover message', 4: 'patch "%s"' % args[0] }
> > +    out.start('Sending ' + outstr[len(args)])
> 
> You could consolidate the two dictionaries like this, to avoid making
> the same choice twice and make the code more pleasant to read:
> 
>   (build, outstr) = { 1: (__build_cover, 'the cover message'), 4:
> (__build_message, 'patch "%s"' % args[0]) }

Hm, I don't think that's valid. I ended up doing something like
this:

    d = { 'cover': (__build_cover, 'the cover message'),
          'patch': (__build_message, 'patch "%s"' % args[0]) }
    
    (build, outstr) = d[type]

> > +    # give recipients a chance of receiving related patches in correct order
> > +    #                                       patch_nr < total_nr
> > +    if len(args) == 1 or (len(args) == 4 and args[1] < args[2]):
> > +        sleep = options.sleep or config.getint('stgit.smtpdelay')
> > +        time.sleep(sleep)
> 
> Hmm. I must say I find all the args[x] a bit hard to read. I'd prefer
> symbolic names.

Ok, I changed this up.

Thanks for the review.
/ac

Re: [PATCH 5/6] stg mail: add basic support for git send-email

[Alex Chiang]

* Karl Wiberg <kha@treskal.com>:
> On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:
> 
> > +    # XXX: yuck, there's gotta be a more pythonic way. Ideally we'd like
> > +    # to use the git_opts dictionary as our mapping between stg mail and
> > +    # git send-email; extract k, v pairs from git_opts, and use those
> > +    # to iterate across options somehow.
> > +    git_opts = { 'to': '--to=', 'cc': '--cc=', 'bcc': '--bcc=' }
> > +    if options.to:
> > +        for a in options.to:
> > +            cmd.append("--to=%s" % a)
> > +    if options.cc:
> > +        for a in options.cc:
> > +            cmd.append("--cc=%s" % a)
> > +    if options.bcc:
> > +        for a in options.bcc:
> > +            cmd.append("--bcc=%s" % a)
> > +    if not options.auto:
> > +        cmd.append("--suppress-cc=body")
> 
> Like this?
> 
> for x in ['to', 'cc', 'bcc']:
>     if getattr(options, x):
>         cmd.extend('--%s=%s' % (x, a) for a in getattr(options, x))

Yeah, that looks nice. Re-implemented with your suggestion.

> > +    (fd, path) = mkstemp()
> > +    os.write(fd, msg.as_string(options.mbox))
> > +    os.close(fd)
> > +
> > +    try:
> > +        cmd.append(path)
> > +        call(cmd)
> > +    except Exception, err:
> > +        os.unlink(path)
> > +        raise CmdException, str(err)
> > +
> > +    os.unlink(path)
> 
> To avoid having to remember to call unlink in all paths, you can write
> 
> try:
>     try:
>         cmd.append(path)
>         call(cmd)
>     except Exception, e:
>         raise CmdException(str(e))
> finally:
>     os.unlink(path)
> 
> (The combined try...except...finally statement didn't appear until
> python 2.5, but we'd like to stay compatible with 2.4.)

This statement confuses me a bit. The way I read it, I shouldn't
use your suggestion due to compat reasons?

Thanks,
/ac

Re: [StGit RFC PATCH 0/6] add support for git send-email

[Alex Chiang]

* Karl Wiberg <kha@treskal.com>:
> On Sat, Nov 28, 2009 at 8:50 PM, Alex Chiang <achiang@hp.com> wrote:
> 
> > stg mail still has some nice features over git send-email, such
> > as the -v command line parameter and --prefix. Maybe at some point
> > in the future, we can migrate those features into git send-email and
> > continue thinning out stg mail.
> 
> Yes. But note that we tend to be conservative and not require a
> too-new git, so a patch adding such a dependency would have to wait a
> while. (I'm currently carrying two such patches in my experimental
> branch.)

Understood. For now, of course, all the changes that I'm
proposing should work with bog-standard, oldish git, since I
don't think the git send-email interface has changed in a while.

> > But I wanted to get some feedback first to make sure I'm going in the
> > right direction before going too much further.
> 
> I've read the patches, and it looks about right from where I stand.

Thank you very much for the review.

> Did you remember to run the regression tests? It's very helpful when
> reviewing to know that the regression suite passes at every point in
> the series.

Good idea. I've been running t/t1900-mail.sh at each stage since
my changes seem rather localized to sending mail.

Should I be running the entire suite?

/ac

Re: [PATCH 3/6] stg mail: make __send_message do more

[Karl Wiberg]

On Tue, Dec 1, 2009 at 12:59 AM, Alex Chiang <achiang@hp.com> wrote:

> * Karl Wiberg <kha@treskal.com>:
>
> > You could consolidate the two dictionaries like this, to avoid
> > making the same choice twice and make the code more pleasant to
> > read:
> >
> > (build, outstr) = { 1: (__build_cover, 'the cover message'), 4: (__build_message, 'patch "%s"' % args[0]) }
>
> Hm, I don't think that's valid. I ended up doing something like
> this:
>
> d = { 'cover': (__build_cover, 'the cover message'), 'patch': (__build_message, 'patch "%s"' % args[0]) }
> (build, outstr) = d[type]

Duh. That's what I get for posting untested code. It should be

(build, outstr) = { 1: (__build_cover, 'the cover message'), 4:
(__build_message, 'patch "%s"' % args[0]) }[len(args)]

That is, we create a dictionary only to immediately use it once,
without ever explicitly storing a reference to it.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle

Re: [PATCH 5/6] stg mail: add basic support for git send-email

[Karl Wiberg]

On Tue, Dec 1, 2009 at 1:00 AM, Alex Chiang <achiang@hp.com> wrote:

> * Karl Wiberg <kha@treskal.com>:
>
> > try:
> >     try:
> >         cmd.append(path)
> >         call(cmd)
> >     except Exception, e:
> >         raise CmdException(str(e))
> > finally:
> >     os.unlink(path)
> >
> > (The combined try...except...finally statement didn't appear until
> > python 2.5, but we'd like to stay compatible with 2.4.)
>
> This statement confuses me a bit. The way I read it, I shouldn't use
> your suggestion due to compat reasons?

Oh. No, the "combined" statement would look like this:

try:
    cmd.append(path)
    call(cmd)
except Exception, e:
    raise CmdException(str(e))
finally:
    os.unlink(path)

It works exactly like the nested try...finally and try...except
statement above, but results in less indentation---and would thus be
preferred, if not for the 2.4 compatibility issue.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle

Re: [StGit RFC PATCH 0/6] add support for git send-email

[Karl Wiberg]

On Tue, Dec 1, 2009 at 1:02 AM, Alex Chiang <achiang@hp.com> wrote:

> * Karl Wiberg <kha@treskal.com>:
>
>> Did you remember to run the regression tests? It's very helpful when
>> reviewing to know that the regression suite passes at every point in
>> the series.
>
> Good idea. I've been running t/t1900-mail.sh at each stage

Perfect.

> since my changes seem rather localized to sending mail.
>
> Should I be running the entire suite?

Just running that single test should be sufficient in this case; as
you say, the changes are rather well contained.

Running the whole suite takes a few minutes, so it might be worth
doing once in a while.

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle