Re: [PATCH] line-log: protect inner strbuf from free

>>> 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. ;)