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

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

[Alex Chiang]

This is v2 of the series that starts teaching stg mail how to
call git send-email.

I made all the changes that Karl recommended, and made sure to
run the t1900-mail.sh test at every point in the series (passes
successfully).

I also experimented with adding another test case for --git
mode, basically duplicating t1900-mail.sh, and then adding the
--git argument wherever it made sense.

However, that resulted in failure of the last 3 test cases,
which is due to the fact that we no longer parse To/Cc/Bcc
command line args in --git mode, and the resulting mbox file was
missing the expected recipient addresses.

I played around with that for a while, thinking that I could use
git send-email --dry-run to do something equivalent, but then
realized that git send-email's run-run mode is definitely not
analogous to stg mail's --mbox mode.

The upshot is that in stg mail, --git and --mbox don't interact
well, and the resulting mbox file will lack the recipients. This
might be fixed in the future if we teach git send-email how to
generate mbox files, but then we introduce a versioning problem.

So let's just accept this wart for now, and say, if you want an
mbox file generated, don't use --git. That seems reasonable to
me.

This mail was sent with the following command line:

	./stg mail --git -a -e --auto -v v2 --prefix=StGit
	--to=catalin.marinas@gmail.com --cc=git

Note that the --cc= contains an alias for the git mailing list
that I defined in my ~/.mutt.aliases file (and specified in
.gitconfig -> sendemail.aliasesfile and sendemail.aliasfiletype.

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 |  196 +++++++++++++++++++++++++++---------------------
 1 files changed, 112 insertions(+), 84 deletions(-)

[StGit PATCH v2 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..a38e3e6 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 Exception('SMTP password supplied, username needed')
+    if (smtpusetls and not smtpuser):
+        raise Exception('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:

[StGit PATCH v2 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 a38e3e6..35194a8 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)

[StGit PATCH v2 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 |   65 +++++++++++++++++++++---------------------------
 1 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 35194a8..edff878 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -228,17 +228,39 @@ 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(type, tmpl, options, *args):
     """Message sending dispatcher.
     """
-    smtpserver = options.smtp_server or config.get('stgit.smtpserver')
+    (build, outstr) = {'cover': (__build_cover, 'the cover message'),
+                       'patch': (__build_message, 'patch "%s"' % args[0])}[type]
+    if type == 'patch':
+        (patch_nr, total_nr) = (args[1], args[2])
+
+    msg_id = email.Utils.make_msgid('stgit')
+    msg = build(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
+
+    out.start('Sending ' + outstr)
 
+    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
+    if type == 'cover' or (type == 'patch' and patch_nr < total_nr):
+        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 +606,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 +620,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('cover', 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()
@@ -628,25 +637,9 @@ def func(parser, options, args):
         if not tmpl:
             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)
+    for (p, n) in zip(patches, range(1, total_nr + 1)):
+        msg_id = __send_message('patch', tmpl, options, p, n, 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()

[StGit PATCH v2 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 edff878..f430a13 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -262,25 +262,25 @@ def __send_message(type, 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 = ''
@@ -300,18 +300,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
@@ -349,6 +345,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')

[StGit PATCH v2 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 |   50 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index f430a13..8a33c22 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,42 @@ 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")
+    if not options.auto:
+        cmd.append("--suppress-cc=body")
+
+    # We only support To/Cc/Bcc in git send-email for now.
+    for x in ['to', 'cc', 'bcc']:
+        if getattr(options, x):
+            cmd.extend('--%s=%s' % (x, a) for a in getattr(options, x))
+
+    # 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:
+        try:
+            cmd.append(path)
+            call(cmd)
+        except Exception, err:
+            raise CmdException, str(err)
+    finally:
+        os.unlink(path)
+
 def __send_message(type, tmpl, options, *args):
     """Message sending dispatcher.
     """
@@ -245,10 +283,13 @@ def __send_message(type, tmpl, options, *args):
         out.stdout_raw(msg_str + '\n')
         return msg_id
 
-    out.start('Sending ' + outstr)
+    if not options.git:
+        out.start('Sending ' + outstr)
 
     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:
@@ -259,7 +300,8 @@ def __send_message(type, tmpl, options, *args):
     if type == 'cover' or (type == 'patch' and patch_nr < total_nr):
         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 = ()):

[StGit PATCH v2 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 |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 8a33c22..d108e75 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py
@@ -247,12 +247,6 @@ def __send_message_git(msg, options):
         if getattr(options, x):
             cmd.extend('--%s=%s' % (x, a) for a in getattr(options, x))
 
-    # 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)
@@ -277,13 +271,13 @@ def __send_message(type, tmpl, options, *args):
     msg_id = email.Utils.make_msgid('stgit')
     msg = build(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
 
     if not options.git:
+        from_addr, to_addrs = __parse_addresses(msg)
         out.start('Sending ' + outstr)
 
     smtpserver = options.smtp_server or config.get('stgit.smtpserver')
@@ -499,7 +493,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)
 
@@ -609,7 +604,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: [StGit PATCH v2 0/6] add support for git send-email

[Karl Wiberg]

On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:

> I also experimented with adding another test case for --git mode,
> basically duplicating t1900-mail.sh, and then adding the --git
> argument wherever it made sense.

Ah, good.

> However, that resulted in failure of the last 3 test cases, which is
> due to the fact that we no longer parse To/Cc/Bcc command line args
> in --git mode, and the resulting mbox file was missing the expected
> recipient addresses.
>
> I played around with that for a while, thinking that I could use git
> send-email --dry-run to do something equivalent, but then realized
> that git send-email's run-run mode is definitely not analogous to
> stg mail's --mbox mode.
>
> The upshot is that in stg mail, --git and --mbox don't interact
> well, and the resulting mbox file will lack the recipients. This
> might be fixed in the future if we teach git send-email how to
> generate mbox files, but then we introduce a versioning problem.

One wild idea: git send-email's --smtp-server flag will accept the
(full) path of a sendmail program; writing such a program, just
capable enough to receive the outgoing emails and dumping them to a
file, should be easy. Another option would be a program that speaks
just enough SMTP to accept the mails. (Incidentally, these two would
be useful in testing stg mail even without the --git option.)

I fully understand if you'd rather get on with scratching your actual
itch, though ...

> So let's just accept this wart for now, and say, if you want an mbox
> file generated, don't use --git. That seems reasonable to me.

Sure.

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

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

[Karl Wiberg]

On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:

> +    if (smtppassword and not smtpuser):
> +        raise Exception('SMTP password supplied, username needed')
> +    if (smtpusetls and not smtpuser):
> +        raise Exception('SMTP over TLS requested, username needed')
> +    if (smtpuser and not smtppassword):
> +        smtppassword = getpass.getpass("Please enter SMTP password: ")

Sorry if I confused you with my earlier explanation; I only meant that
you should use the _form_ "raise Exception('message')", not that you
should change the exception type from CmdException to Exception. If
you try to trigger these errors, I think you'll find that in the case
of CmdException, StGit will print just the message and exit with an
error; whereas for straight Exception, it'll print the full backtrace
as well under the assumption that it's a program bug.

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

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

[Karl Wiberg]

Just pointing out a couple of Python tricks you might've wanted to
use. No need to update the patch, though.

On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:

> +        (patch_nr, total_nr) = (args[1], args[2])

Can be written as

  (patch_nr, total_nr) = args[1:3]

or, if args[2] is the last element of the list (which it isn't in this
case?),

  (patch_nr, total_nr) = args[1:]

> +    for (p, n) in zip(patches, range(1, total_nr + 1)):
> +        msg_id = __send_message('patch', tmpl, options, p, n, total_nr, ref_id)

Can be written as

  for (n, p) in enumerate(patches):

if you use n + 1 instead of n in the loop body.

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

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

[Karl Wiberg]

On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:

> This is v2 of the series that starts teaching stg mail how to
> call git send-email.

Acked-by: Karl Wiberg <kha@treskal.com>

when the CmdException/Exception thing is fixed in the first patch.

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

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

[Catalin Marinas]

Hi Alex,

2009/12/2 Alex Chiang <achiang@hp.com>:
> This is v2 of the series that starts teaching stg mail how to
> call git send-email.

Thanks for posting these patches (and thanks to Karl for reviewing
them). I don't have much to comment (Karl did the hard work here) but
I'll give them a try tomorrow and let you know.

Thanks.

-- 
Catalin

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

[Alex Chiang]

* Karl Wiberg <kha@treskal.com>:
> On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:
> > The upshot is that in stg mail, --git and --mbox don't interact
> > well, and the resulting mbox file will lack the recipients. This
> > might be fixed in the future if we teach git send-email how to
> > generate mbox files, but then we introduce a versioning problem.
> 
> One wild idea: git send-email's --smtp-server flag will accept the
> (full) path of a sendmail program; writing such a program, just
> capable enough to receive the outgoing emails and dumping them to a
> file, should be easy. Another option would be a program that speaks
> just enough SMTP to accept the mails. (Incidentally, these two would
> be useful in testing stg mail even without the --git option.)
 
Hm, I think this is getting to be a bit of overkill. I could see
adding --mbox support to git send-email as being a better use of
time (IMO).

> I fully understand if you'd rather get on with scratching your actual
> itch, though ...
 
:)

> > So let's just accept this wart for now, and say, if you want an mbox
> > file generated, don't use --git. That seems reasonable to me.
> 
> Sure.

Thanks,
/ac

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

[Alex Chiang]

* Karl Wiberg <kha@treskal.com>:
> On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:
> 
> > +    if (smtppassword and not smtpuser):
> > +        raise Exception('SMTP password supplied, username needed')
> > +    if (smtpusetls and not smtpuser):
> > +        raise Exception('SMTP over TLS requested, username needed')
> > +    if (smtpuser and not smtppassword):
> > +        smtppassword = getpass.getpass("Please enter SMTP password: ")
> 
> Sorry if I confused you with my earlier explanation; I only meant that
> you should use the _form_ "raise Exception('message')", not that you
> should change the exception type from CmdException to Exception. If
> you try to trigger these errors, I think you'll find that in the case
> of CmdException, StGit will print just the message and exit with an
> error; whereas for straight Exception, it'll print the full backtrace
> as well under the assumption that it's a program bug.

Ah, ok. Will update.

/ac

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

[Alex Chiang]

* Karl Wiberg <kha@treskal.com>:
> Just pointing out a couple of Python tricks you might've wanted to
> use. No need to update the patch, though.
> 
> On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:
> 
> > +        (patch_nr, total_nr) = (args[1], args[2])
> 
> Can be written as
> 
>   (patch_nr, total_nr) = args[1:3]

Thanks, I did it this way.

> or, if args[2] is the last element of the list (which it isn't in this
> case?),
> 
>   (patch_nr, total_nr) = args[1:]

No, ref_id is the last arg, so that won't work.

> > +    for (p, n) in zip(patches, range(1, total_nr + 1)):
> > +        msg_id = __send_message('patch', tmpl, options, p, n, total_nr, ref_id)
> 
> Can be written as
> 
>   for (n, p) in enumerate(patches):
> 
> if you use n + 1 instead of n in the loop body.

That is a little cleaner, but I decided to keep it as zip(). Why?
Because using n + 1 in the loop body will push that line past 80
columns. ;)

It's also the original code (albeit with a simple variable
rename).

I know this isn't the kernel, and that there are plenty of other
lines that are 80+ characters, but if you can keep it short, why
not?

Thanks,
/ac

[StGit PATCH v3 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>
---

Catalin,

This is the only patch in the series that changed, so no sense in
sending out all the others.

 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..777ee36 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:

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

[Karl Wiberg]

On Thu, Dec 3, 2009 at 8:30 PM, Alex Chiang <achiang@hp.com> wrote:

> * Karl Wiberg <kha@treskal.com>:
>
> > On Wed, Dec 2, 2009 at 1:46 AM, Alex Chiang <achiang@hp.com> wrote:
> >
> > > +    for (p, n) in zip(patches, range(1, total_nr + 1)):
> > > +        msg_id = __send_message('patch', tmpl, options, p, n, total_nr, ref_id)
> >
> > Can be written as
> >
> >   for (n, p) in enumerate(patches):
> >
> > if you use n + 1 instead of n in the loop body.
>
> That is a little cleaner, but I decided to keep it as zip(). Why?
> Because using n + 1 in the loop body will push that line past 80
> columns. ;)
>
> It's also the original code (albeit with a simple variable rename).
>
> I know this isn't the kernel, and that there are plenty of other
> lines that are 80+ characters, but if you can keep it short, why
> not?

Oh, I fully favor keeping lines within the 80 columns allotted to us
by the ancestors---I just didn't realize it was going to be a problem
here.

In general, though, programmer time is worth optimizing for, and
thinking through exactly what zip(patches, range(1, total_nr + 1))
means (and getting it right!) is a small but not insignificant cost
every time someone reads the code.

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

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

[Catalin Marinas]

2009/12/2 Alex Chiang <achiang@hp.com>:
> This is v2 of the series that starts teaching stg mail how to
> call git send-email.

I merged these patches into the "proposed" branch for now and test
them for a bit more. It would be nice for some of the stgit options to
be translated into git send-email options (I don't mind renaming the
stgit options to match the git ones):

--refid -> --in-reply-to
--noreply -> --no-thread

Thanks.

-- 
Catalin

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

[Karl Wiberg]

On Sun, Dec 6, 2009 at 11:16 PM, Catalin Marinas
<catalin.marinas@gmail.com> wrote:

> --refid -> --in-reply-to
> --noreply -> --no-thread

And I must say, the git options are better named than ours anyway ...

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