On Wed, Mar 20, 2024, at 01:43, Jeff King wrote:
> 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?
> + */
> +
Hmm. I’ll look into that. This seems like a nicer place to do it
compared to `log.c`.
> 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. ;)
Hah! Definitely don’t worry about that, this has been very helpful.
> 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
I think your series should take precedence. I’ll put my series on the
backburner for a while. There’s no rush with that one. These changes of
yours will make extending the header logic easier overall.
Then when yours is merged I’ll have an even easier time.
Thanks again
Kristoffer
