On Wed, Oct 02, 2024 at 10:36:33PM -0400, Derrick Stolee wrote:
> > It does make me wonder what leak Patrick saw that caused him to
> > write 394affd46d, and whether we're now leaking in some case that
> > I'm missing.
>
> Looking at the change, I can only guess that it was the previous use of
>
> char *prefix = "";
>
> that signaled that an unfreeable string was being assigned to a non-const
> pointer. This signals that _something_ is wrong with the function, but
> the way that the buffer is returned by the function pointer is suspicious,
> too.
Ah, of course. I saw Patrick's name and just assumed it was part of
leak-checking fixes. But of course he also fixed -Wwrite-strings issues.
> > I do think this would have been a harder mistake to make if the callback
> > simply returned a "const char *" pointer. We'd lose the ability to show
> > prefixes with embedded NULs, but I'm not sure that's a big deal. It
> > would also help for line-log to use the existing helper rather than
> > inventing its own. So together on top something like this (which I'd
> > probably turn into two patches if this seems to others like it's
> > valuable and not just churn):
>
> I do agree that changing the return type will make this easier to prevent
> and the code should be easier to read as well.
>
> Your diffed patch looks pretty good. I made an attempt at guessing where
> you would have split them (first remove the duplicate method, then change
> the method prototype and callers).
Yep, exactly. I actually ended up with a third patch which is a nearby
cleanup. I'll hold them back for now, though. Your patch is a regression
fix which we should prioritize (though it sounds like it is in 2.46, not
the upcoming 2.47?). I'll post my on top as a separate series.
> I even took some time to attempt to remove the static strbuf from
> diff_output_prefix_callback() in favor of using the 'data' member of the
> diff_options struct, but it was not incredibly obvious how to communicate
> ownership of the struct which would need to store both the graph struct
> and the strbuf. Perhaps this would be good for #leftoverbits.
Yeah, I think probably "struct git_graph" would need to own the buffer,
initialize it in graph_init(), and then discard it in graph_clear().
But that gets weird because apparently you can set the callback without
a git_graph? Looks like that is triggered by "--line-prefix" without
"--graph". Yuck.
But in that case we are just showing the line_prefix string, so we could
return that directly. Something like the patch below.
The whole thing feels a bit over-engineered with the callback. The graph
code is the only one that needs anything beyond a static string. And the
way --line-prefix interacts with it is odd, since some callers override
the callback (e.g., "range-diff --line-prefix=foo" is accepted, but
doesn't do anything). I don't think there's a bug anybody cares about,
but, well...it's not how I would have written it. ;)
diff --git a/graph.c b/graph.c
index c046f6285d..bf000fdbe1 100644
--- a/graph.c
+++ b/graph.c
@@ -309,21 +309,28 @@ struct git_graph {
* stored as an index into the array column_colors.
*/
unsigned short default_column_color;
+
+ /*
+ * Scratch buffer for generating prefixes to be used with
+ * diff_output_prefix_callback().
+ */
+ struct strbuf prefix_buf;
};
static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)
{
struct git_graph *graph = data;
- static struct strbuf msgbuf = STRBUF_INIT;
assert(opt);
- strbuf_reset(&msgbuf);
+ if (!graph)
+ return opt->line_prefix;
+
+ strbuf_reset(&graph->prefix_buf);
if (opt->line_prefix)
- strbuf_addstr(&msgbuf, opt->line_prefix);
- if (graph)
- graph_padding_line(graph, &msgbuf);
- return msgbuf.buf;
+ strbuf_addstr(&graph->prefix_buf, opt->line_prefix);
+ graph_padding_line(graph, &graph->prefix_buf);
+ return graph->prefix_buf.buf;
}
static const struct diff_options *default_diffopt;
@@ -393,6 +400,7 @@ struct git_graph *graph_init(struct rev_info *opt)
* The diff output prefix callback, with this we can make
* all the diff output to align with the graph lines.
*/
+ strbuf_init(&graph->prefix_buf, 0);
opt->diffopt.output_prefix = diff_output_prefix_callback;
opt->diffopt.output_prefix_data = graph;
@@ -408,6 +416,7 @@ void graph_clear(struct git_graph *graph)
free(graph->new_columns);
free(graph->mapping);
free(graph->old_mapping);
+ strbuf_release(&graph->prefix_buf);
free(graph);
}
-Peff
