Re: [PATCH v2 1/3] revision: add a per-email field to rev-info

> On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:
>
>> Having now stared at this code for a bit, I do think there's another,
>> much simpler option for your series: keep the same ugly static-strbuf
>> allocation pattern in log_write_email_headers(), but extend it further.
>> I'll show that in a moment, too.
>
> So something like this:
>
> diff --git a/log-tree.c b/log-tree.c
> index e5438b029d..ae0f4fc502 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -474,12 +474,21 @@ void log_write_email_headers(struct rev_info
> *opt, struct commit *commit,
>  			     int *need_8bit_cte_p,
>  			     int maybe_multipart)
>  {
> -	const char *extra_headers = opt->extra_headers;
> +	static struct strbuf headers = STRBUF_INIT;
>  	const char *name = oid_to_hex(opt->zero_commit ?
>  				      null_oid() : &commit->object.oid);
>
>  	*need_8bit_cte_p = 0; /* unknown */
>
> +	strbuf_reset(&headers);
> +	if (opt->extra_headers)
> +		strbuf_addstr(&headers, opt->extra_headers);
> +	/*
> +	 * here's where you'd do your pe_headers; I wonder if you could even
> +	 * just run the header command directly here and not need to shove the
> +	 * string into rev_info?
> +	 */
> +

>  	fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n",
> name);
>  	graph_show_oneline(opt->graph);
>  	if (opt->message_id) {
> @@ -496,16 +505,13 @@ void log_write_email_headers(struct rev_info
> *opt, struct commit *commit,
>  		graph_show_oneline(opt->graph);
>  	}
>  	if (opt->mime_boundary && maybe_multipart) {
> -		static struct strbuf subject_buffer = STRBUF_INIT;
>  		static struct strbuf buffer = STRBUF_INIT;
>  		struct strbuf filename =  STRBUF_INIT;
>  		*need_8bit_cte_p = -1; /* NEVER */
>
> -		strbuf_reset(&subject_buffer);
>  		strbuf_reset(&buffer);
>
> -		strbuf_addf(&subject_buffer,
> -			 "%s"
> +		strbuf_addf(&headers,
>  			 "MIME-Version: 1.0\n"
>  			 "Content-Type: multipart/mixed;"
>  			 " boundary=\"%s%s\"\n"
> @@ -516,10 +522,8 @@ void log_write_email_headers(struct rev_info *opt,
> struct commit *commit,
>  			 "Content-Type: text/plain; "
>  			 "charset=UTF-8; format=fixed\n"
>  			 "Content-Transfer-Encoding: 8bit\n\n",
> -			 extra_headers ? extra_headers : "",
>  			 mime_boundary_leader, opt->mime_boundary,
>  			 mime_boundary_leader, opt->mime_boundary);
> -		extra_headers = subject_buffer.buf;
>
>  		if (opt->numbered_files)
>  			strbuf_addf(&filename, "%d", opt->nr);
> @@ -539,7 +543,7 @@ void log_write_email_headers(struct rev_info *opt,
> struct commit *commit,
>  		opt->diffopt.stat_sep = buffer.buf;
>  		strbuf_release(&filename);
>  	}
> -	*extra_headers_p = extra_headers;
> +	*extra_headers_p = headers.len ? headers.buf : NULL;
>  }
>
>  static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
>
> And then the callers can continue not caring about how or when to free
> the returned pointer. I think in the long run the cleanups I showed are
> a nicer place to end up, but I'd just worry that your feature work will
> be held hostage by my desire to clean. ;)

> If you did it this way (probably as a separate preparatory patch minus
> the pe_headers comment), then either I could do my cleanups on top, or
> they could even graduate independently (though obviously there will be a
> little bit of tricky merging at the end).
>
> -Peff