From: René Scharfe Date: Sat, 25 Mar 2017 16:56:55 GMT Subject: Re: [PATCH] pretty: add extra headers and MIME boundary directly Message-ID: In-Reply-To: <20170325161705.wh4zueasik6iwktv@sigill.intra.peff.net> Am 25.03.2017 um 17:17 schrieb Jeff King: > On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote: >> @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, >> graph_show_oneline(opt->graph); >> } >> if (opt->mime_boundary) { >> - static char subject_buffer[1024]; >> static char buffer[1024]; > > We still have this other buffer, which ends up in stat_sep. It should > probably get the same treatment, though I think the module boundaries > make it a little more awkward. We look at it in diff_flush(), which > otherwise doesn't need to know much about the pretty-printing. > > Perhaps stat_sep should be a callback? Yes, it would be nice to avoid it, but I haven't found a clean way, yet. In diff.c, where it's used, we don't have commit and rev_info available (which we'd have to pass to a callback, or consume right there), and that's probably how it should be. Perhaps preparing the filename in advance and passing that as a string together with mime_boundary and no_inline might be the way to go. >> diff --git a/pretty.c b/pretty.c >> index d0f86f5d85..56e668781a 100644 >> --- a/pretty.c >> +++ b/pretty.c >> @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp, >> if (pp->after_subject) { >> strbuf_addstr(sb, pp->after_subject); >> } >> + if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) { >> + strbuf_addf(sb, >> + "MIME-Version: 1.0\n" > > In the original, this would have been in "after_subject". Which means we > would print it even if print_email_subject is not true. Why do we need > to check it in the new conditional? No, we only would have printed it if log_write_email_headers() was called to append it to the static buffer. print_email_subject is only set when we call log_write_email_headers(), so checking it makes sure that we get the same behavior as before. > Not that I expect the behavior to be wrong either way; why would we have > a mime boundary without setting print_email_subject? But I would think > that "do we have a mime boundary" would be the right conditional to > trigger printing it. FWIW, the test suite still passes with the print_email_subject check removed. And currently only cmd_format_patch() sets mime_boundary, so we don't need the check indeed. René