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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:

> Use the after_subject member of struct pretty_print_context to pass the
> extra_headers unchanged, and construct and add the MIME boundary headers
> directly in pretty.c::pp_title_line() instead of writing both to a
> static buffer in log-tree.c::log_write_email_headers() first.  That's
> easier, quicker and gets rid of said static buffer.

I'm definitely pleased with the direction. A few comments:

> @@ -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?

> 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?

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.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]