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

[Derrick Stolee <stolee@gmail.com>]

On 10/3/24 2:11 AM, Jeff King wrote:
> On Wed, Oct 02, 2024 at 10:36:33PM -0400, Derrick Stolee wrote:

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

Thanks for exploring this with the diff you sent. It's subtle, but you
did a good job of recognizing that when the callback is used without
a 'graph' value, it returns opt->line_prefix.

> +	if (!graph)
> +		return opt->line_prefix;

I was wondering why there was no need to edit graph_setup_line_prefix()
in your diff, and this explains why. Your change is simple enough to
handle that change while being robust to a future assignment of the
callback data.

For what it's worth, I attempted creating a custom callback with a
BUG() statement in it and the only test failure was t4013-diff-various.sh.
That test is very non-standard, and it did not recognize the process
failure, only the output difference. #leftoverbits could make that test
fail on process failure.

Thanks,
-Stolee