patchAdd MIME information to outgoing email

Add MIME-Version/Content-Type/Content-Transfer-Encoding headers in messages generated with git-format-patch. Without it, messages generated without using --attach or --inline didn't have any content type information.

I got hit with this problem yesterday when sending a patch to linux-kernel with a commit message containing the name "Pádraig" in it. Moreover, the mailing-list software added an incorrect ISO-8859-1 encoding information which mangled Pádraig's name.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>
---
 log-tree.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

On Thu, Mar 13, 2008 at 05:40:19PM +0100, Samuel Tardieu wrote:

It's supposed to handle this automatically if the commit message contains non-ascii characters. What version of git were you using?

-Peff

On 13/03, Jeff King wrote:

| It's supposed to handle this automatically if the commit message
| contains non-ascii characters. What version of git were you using?

A version from "next" from March 8: ea6cde37d54121c5a1e1da51e1cd5cf27bfa3904 (+ 1 unrelated patch)

E.g, does "git format-patch a1eebf~1..a1eebf" add MIME headers for you without my patch?

On Thu, Mar 13, 2008 at 06:14:36PM +0100, Samuel Tardieu wrote:

Hmm, that is certainly recent enough.

Sorry, I don't have that commit. What repo is it in?

It does work for me with this simple test:

  mkdir repo && cd repo && git init
  echo content >file && git add file && git commit -m one
  echo more >>file && git commit -a -m 'two

  utf8 body: ñ'
  git format-patch HEAD^
  cat 0001-two.patch

I get:

-- >8 --
From a9e3222c0dca0a2b1e1a53ab9b7a7526ed359b79 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 14 Mar 2008 09:27:30 -0400
Subject: [PATCH] two
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

utf8 body: ñ
---
 file |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

On 14/03, Jeff King wrote:

| Sorry, I don't have that commit. What repo is it in?

Junio's "next" branch.

| So I assume there is some bug in git that is being triggered by the
| commit you mention.

Maybe, that's why I'd be interested by the behaviour you get with it.

On Fri, Mar 14, 2008 at 02:40:22PM +0100, Samuel Tardieu wrote:

Ah, sorry, for some reason I couldn't find it before. I must have typo'd it.

Yes, it works fine for me:

$ git-format-patch a1eebf~1..a1eebf
0001-git.el-find-the-git-status-buffer-whatever-its-name.patch
$ head 0001*
From a1eebfb3a90b6c240afd1a32cfebe6ee5dbd72c5 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?R=C3=A9mi=20Vanicat?= <vanicat@debian.org>
Date: Fri, 29 Feb 2008 19:28:19 +0100
Subject: [PATCH] git.el: find the git-status buffer whatever its name is
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

git-status used the buffer name to find git-status buffers, and that can fail if the buffer has another name, for example when multiple

-Peff

On 14/03, Jeff King wrote:

| Yes, it works fine for me:

Ahhhhh, found it. It looks like you have no format.headers configuration variable, do you?

On Fri, Mar 14, 2008 at 02:50:48PM +0100, Samuel Tardieu wrote:

No, I don't. Having peeked a few days ago at the pretty-printing code, that is almost certainly the problem (I think the extra_headers parameter is overloaded to handle both of these conditions). Can you work up a patch?

-Peff

On 14/03, Jeff King wrote:

| No, I don't. Having peeked a few days ago at the pretty-printing code,
| that is almost certainly the problem (I think the extra_headers
| parameter is overloaded to handle both of these conditions). Can you
| work up a patch?

Sure, but not before some time, I am quite busy with urgent work right now. If you want to beat me at it, be my guest :)

Jeff King <peff@peff.net> writes:

You are right.  The call-chain looks like this:

    log_tree_diff_flush()
     show_log()
      log_write_email_headers()     writes mbox From
      pretty_print_commit()
                                    check commit log if it is pure ascii
       pp_header()
        pp_user_info()              writes RFC2822 From:
       pp_title_line()              writes RFC2822 Subject:
                                    writes MIME-Version: and friends if needed
       pp_remainder()               writes the remainder of the log message
      append_signoff()
     printf("---\n")
     diff_flush()                   writes the patch

At the beginning of pretty_print_commit() we look at the log and if it is not ascii we pass that information down to pp_title_line() which is responsible for writing MIME header at the appropriate place.

If your patch itself has some non-ASCII material, and if your commit log message is pure ASCII, the above would end up not writing MIME at all. If your commit log message is non ASCII, then we will mark it as if the entire message is in the encoding of the log in pp_title_line(). This might look like a problem, but it is not something non multipart output of format-patch should even try to cater to. The payload (i.e. the patch) out of git has always been uninterpreted sequence of bytes (and it is not going to change).

A patch to i18n po/ files for example could contain patches to different files encoded in KOI-8, BIG5, EUC-JP and UTF-8 at the same time. There is no way to say "text/plain; charset=X" for such a payload (because there is no single charset used in such a patch), and git simply does not know nor care about what encoding each file is in. The output from git marks only the part git knows the encoding about (i.e. the commit log message).

Having said all that, I notice that addition of format.headers variable (which I think is a later invention) was done not quite correctly. In the callchain above, pretty_print_commit() function checks the commit log but it is meant to do so only when we haven't emitted MIME Content-Type: (because the user told us to do multipart), and "after_subject" parameter was getting passed around for it (and its callees) to detect exactly that. But format.headers misused that variable to carry its contents along --- there needs a way to pass "have we said MIME-Version crap already" separately.

Junio C Hamano <gitster@pobox.com> writes:

I think the real culprit was the way the "after_subject" was added to the callchain (it had loaded semantics -- "here is what we want to say after emitting Subject: line" and "have we done any MIME yet?"), not the poor guy who did format.headers.

In any case, this patch would hopefully separate the two. The old "plain_non_ascii" parameter is now need_8bit_ct_header and now can have one of three values:

 -1 : we've already done MIME crap so never add extra header to say this
      is 8bit;

 0  : we haven't done MIME and we have not seen anything that is 8bit yet.

 1  : we haven't done MIME and we have seen something that is 8bit.
      pp_title_line() needs to add MIME header.

(As with any other patches I send during my lunchtime, this is untested).

---

 builtin-log.c |    6 ++++--
 commit.h      |    4 ++--
 log-tree.c    |   15 ++++++++++++---
 log-tree.h    |    4 +++-
 pretty.c      |   24 +++++++++++-------------
 5 files changed, 32 insertions(+), 21 deletions(-)

On Fri, Mar 14, 2008 at 01:21:27PM -0700, Junio C Hamano wrote:

I was just about to submit a patch splitting after_subject into "mime_headers" and "extra_headers".

I noticed another bug while doing mine: we are sometimes not strict _enough_ in squelching headers. A patch made with "-s --attach" when the signoff has non-ascii characters would end up with duplicated MIME headers. Your patch handles this fine.

I think your approach is a little nicer. Here is the test case I wrote for my patch. It covers the original problem and the one I mentioned above; both fail with current master but pass with your patch.

---

Samuel Tardieu <sam@rfc1149.net> writes:

Isn't that job for send-email (or user's MUA)? I really do not think we want to clutter format-patch output any more than necessary.

On 13/03, Junio C Hamano wrote:

| Samuel Tardieu <sam@rfc1149.net> writes:
| 
| > Add MIME-Version/Content-Type/Content-Transfer-Encoding headers in
| > messages generated with git-format-patch. Without it, messages generated
| > without using --attach or --inline didn't have any content type information.
| 
| Isn't that job for send-email (or user's MUA)?  I really do not think we
| want to clutter format-patch output any more than necessary.

Only format-patch knows what encoding has been used by itself to generate the message. Doing it at any later stage would have to guess what the correct charset is.

[Samuel Tardieu <sam@rfc1149.net>]

When the encoded string is entirely ascii except for one or two characters (such as occurs in a lot of patches I handle from people with names not represented in plain ascii) guessing later on seems to run pretty high risk of guessing wrong.

I've taken to manually adding UTF-8 content-type/transfer-encoding headers to avoid the routine mangling of my coworkers' names and would welcome a change to do this automatically.

Considering that UTF-8 is the expected default encoding (right?) for git metadata, it seems to be the sane thing to indicate if the default is unchanged.

Brian

On 14/03, Brian Swetland wrote:

| > Only format-patch knows what encoding has been used by itself to
| > generate the message. Doing it at any later stage would have to guess
| > what the correct charset is.
| 
| When the encoded string is entirely ascii except for one or two characters
| (such as occurs in a lot of patches I handle from people with names
| not represented in plain ascii) guessing later on seems to run pretty
| high risk of guessing wrong.
| [...]
| Considering that UTF-8 is the expected default encoding (right?) for
| git metadata, it seems to be the sane thing to indicate if the default
| is unchanged.

Yup.

Junio C Hamano kirjoitti:

By the way, 'git send-email --compose' does not add MIME headers to introductory message. All non-Ascii chars will output something undefined in receivers' end.

I guess the right way would be to detect user's charset (locale) and add appropriate MIME headers. Also, the Subject field should be encoded if it contains non-Ascii characters.

On Tue, Mar 25, 2008 at 2:31 PM, Teemu Likonen <tlikonen@iki.fi> wrote:

I stuck this in my config and it works-for-me:

[format]
	headers = \
"MIME-Version: 1.0\n\
Content-Type: text/plain; charset=UTF-8\n\
Content-Transfer-Encoding: 8bit\n"

Shrug. (Never tried putting non-ascii in the subject tho.)

j.

"Jay Soffian" <jaysoffian@gmail.com> writes:

I suspect that you shouldn't do this. This would badly interfere both with existing format-patch behaviour that adds these MIME-Version and Content-Type headers by looking at the contents, and with recent format-patch fix 6bf4f1b (format-patch: generate MIME header as needed even when there is format.header, 2008-03-14) to make the detection based on contents (and presense of format.headers).

On Tue, Mar 25, 2008 at 4:47 PM, Junio C Hamano <gitster@pobox.com> wrote:

Fair enough. But I never send out a patch w/o looking at it in an editor first so I would've caught that. Thanks for the heads-up though.

j.

On Tue, Mar 25, 2008 at 01:47:05PM -0700, Junio C Hamano wrote:

Yes, I can confirm that that is problematic without even testing. The whole point of 6bf4f1b was that we _should_ add MIME headers even if the user has set format.headers.

We could be more clever about parsing format.headers and mark the "we have already added MIME" flag (I think we already have to do such parsing because of to/cc magic). But I have to wonder what the real goal is here. There has sometimes been a call for "please add MIME headers unconditionally"; maybe that is an option that people would like.

-Peff

On Tue, Mar 25, 2008 at 05:56:14PM -0400, Jeff King wrote:

I started on this out of curiosity, and it _is_ really simple, but it's also wrong. It can't be right to set your MIME headers statically because some options (like --attach) might cause us to have to put in _different_ MIME headers. So we are left with either conflicting headers, disallowing --attach, ignoring some of your format.headers, or possibly picking out those headers and making them part of the header of that patch part of the multipart. All of which seem a bit ugly to me.

If this is something people really want, I think just adding an "always add mime headers" option makes the most sense.

-Peff

On Tue, Mar 25, 2008 at 08:31:16PM +0200, Teemu Likonen wrote:

I just posted some patches to fix this; however, they always encode as utf-8. I'm not sure what is the best way to find the user's encoding. AIUI, locale environment variables are not enough, since, e.g., "en_US" could come in iso8859-1 and utf-8 flavors. Is there a portable way to figure this out? Should we be pulling it from .git/config? Neither the commitEncoding nor the logOutputEncoding really makes sense to reuse.

-Peff

On Tue, Mar 25, 2008 at 7:06 PM, Jeff King <peff@peff.net> wrote:

I think so. There's no reason the message encoding necessarily matches the locale anyway. There are ways to guess, but I think .git/config is sanest with UTF-8 as the default.

j.

On Tue, Mar 25, 2008 at 19:06:49 -0400, Jeff King wrote:

The portable way is to use the locale environment variables, but you have to read them via the libc locale interface. You set the LC_CTYPE locale category via setlocale (which will consult LC_CTYPE, LC_ALL and LANG environment AND the locale database) and than ask for charset using nl_langinfo(CODESET). To do the equivalent from the shell, call `locale charmap`. From perl, either use langinfo in I18N::Langinfo, or just use the ':locale' IO stream option provided by encoding pragma to read the file in unicode no matter what the locale encoding was (this is perl 5.8.<something> -- in older one, you'd have to use I18N::Langinfo anyway).