On Thu, Oct 03, 2024 at 08:23:22AM -0400, Derrick Stolee wrote: > > 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. Yeah. I actually ended up pulling that into its own patch because it's sufficiently subtle. I'll send that along in a minute. -Peff
