Re: [PATCH] pretty: add extra headers and MIME boundary directly

["René Scharfe" <l.s.r@web.de>]

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é