diff: return const char from output_prefix callback

The diff_options structure has an output_prefix callback for returning a prefix string, but it does so by returning a pointer to a strbuf. This makes the interface awkward. There's no reason the callback should need to use a strbuf, and it creates questions about whether the ownership of the resulting buffer should be transferred to the caller (it should not be, but a recent attempt to clean up this code led to a double-free in some cases). The one advantage we get is that the strbuf contains a ptr/len pair, so we could in theory have a prefix with embedded NULs. But we can observe that none of the existing callbacks would ever produce such a NUL (they are usually just indentation or graph symbols, and even the "--line-prefix" option takes a NUL-terminated string). And anyway, only one caller (the one in log_tree_diff_flush) actually looks at the strbuf length. In every other case we use a helper function which discards the length and just returns the NUL-terminated string. So let's just have the callback return a "const char *" pointer. It's up to the callbacks themselves if they want to use a strbuf under the hood. And now the caller in log_tree_diff_flush() can just use the helper function along with everybody else. That lets us even simplify out the function pointer check, since the helper returns an empty string (technically this does mean we'll sometimes issue an empty fputs() call, but I don't think this code path is hot enough to care about that). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 3, 2024 at 17:09 UTC 436728fe9d75d05fa2439f867ca2039012b86e69
6 files changed +11 -19
diff-lib.c
+2 -2
@@ -704,7 +704,7 @@ int index_differs_from(struct repository *r,
704 return (has_changes != 0);
705 }
706
707 -static struct strbuf *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)
707 +static const char *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)
708 {
709 return data;
710 }
@@ -719,7 +719,7 @@ void show_interdiff(const struct object_id *oid1, const struct object_id *oid2,
719 opts.output_format = DIFF_FORMAT_PATCH;
720 opts.output_prefix = idiff_prefix_cb;
721 strbuf_addchars(&prefix, ' ', indent);
722 - opts.output_prefix_data = &prefix;
722 + opts.output_prefix_data = prefix.buf;
723 diff_setup_done(&opts);
724
725 diff_tree_oid(oid1, oid2, "", &opts);
diff.c
+3 -6
@@ -2313,12 +2313,9 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)
2313
2314 const char *diff_line_prefix(struct diff_options *opt)
2315 {
2316 - struct strbuf *msgbuf;
2317 - if (!opt->output_prefix)
2318 - return "";
2319 -
2320 - msgbuf = opt->output_prefix(opt, opt->output_prefix_data);
2321 - return msgbuf->buf;
2316 + return opt->output_prefix ?
2317 + opt->output_prefix(opt, opt->output_prefix_data) :
2318 + "";
2319 }
2320
2321 static unsigned long sane_truncate_line(char *line, unsigned long len)
diff.h
+1 -1
@@ -94,7 +94,7 @@ typedef void (*add_remove_fn_t)(struct diff_options *options,
94 typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
95 struct diff_options *options, void *data);
96
97 -typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);
97 +typedef const char *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);
98
99 #define DIFF_FORMAT_RAW 0x0001
100 #define DIFF_FORMAT_DIFFSTAT 0x0002
graph.c
+2 -2
@@ -309,7 +309,7 @@ struct git_graph {
309 unsigned short default_column_color;
310 };
311
312 -static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)
312 +static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)
313 {
314 struct git_graph *graph = data;
315 static struct strbuf msgbuf = STRBUF_INIT;
@@ -321,7 +321,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
321 strbuf_addstr(&msgbuf, opt->line_prefix);
322 if (graph)
323 graph_padding_line(graph, &msgbuf);
324 - return &msgbuf;
324 + return msgbuf.buf;
325 }
326
327 static const struct diff_options *default_diffopt;
log-tree.c
+1 -6
@@ -922,12 +922,7 @@ int log_tree_diff_flush(struct rev_info *opt)
922 * diff/diffstat output for readability.
923 */
924 int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
925 - if (opt->diffopt.output_prefix) {
926 - struct strbuf *msg = NULL;
927 - msg = opt->diffopt.output_prefix(&opt->diffopt,
928 - opt->diffopt.output_prefix_data);
929 - fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
930 - }
925 + fputs(diff_line_prefix(&opt->diffopt), opt->diffopt.file);
926
927 /*
928 * We may have shown three-dashes line early
range-diff.c
+2 -2
@@ -478,7 +478,7 @@ static void patch_diff(const char *a, const char *b,
478 diff_flush(diffopt);
479 }
480
481 -static struct strbuf *output_prefix_cb(struct diff_options *opt UNUSED, void *data)
481 +static const char *output_prefix_cb(struct diff_options *opt UNUSED, void *data)
482 {
483 return data;
484 }
@@ -506,7 +506,7 @@ static void output(struct string_list *a, struct string_list *b,
506 opts.flags.suppress_hunk_header_line_count = 1;
507 opts.output_prefix = output_prefix_cb;
508 strbuf_addstr(&indent, " ");
509 - opts.output_prefix_data = &indent;
509 + opts.output_prefix_data = indent.buf;
510 diff_setup_done(&opts);
511
512 /*