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.
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
Jeff King <peff@peff.net>