Re: [PATCH 2/8] fetch: move reference width calculation into `display_state`

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 17, 2023, 10:03 UTC
Message-ID: <ZBQ612KWqYg4ky2j@ncase>
In-Reply-To: <xmqqpm98mzzy.fsf@gitster.g>

On Thu, Mar 16, 2023 at 09:18:09AM -0700, Junio C Hamano wrote:

Show 28 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> Given that in the previous step, what used to be called display got
> >> renamed to display_buffer (I think "buffer" ought to be sufficient
> >> in this context, though), the variable of "struct display_state"
> >> type should NOT be named "display", as it would be confusing when
> >> two things are related to "display" and only one of them is called
> >> as such.  Either "display_state" or "state" would be fine.
> >
> > Fair enough. In that case I may just as well drop the first patch.
> 
> If you plan to get rid of an independent "display_buffer" in the
> endgame by moving it into the bigger struct as its .buffer member,
> then I think the naming is fine as there will remain only one thing
> that is "display".  The fact that I didn't see that plan through
> when I read only the first two patches would probably mean that the
> route this iteration of the series took was somewhat roundabout, and
> there may be a more transparent and possibly a more direct way to
> get to that goal?
> 
> I am not entirely sure if the buffer should go inside the
> display_state structure in the endgame.  An alternative may be to
> make it a on-stack variable of format_display() (which will later be
> modified to do everything up to and including writing out the
> result) and pass it through the callchain below to its helpers, just
> like the current code already does.  And in such an approach, you'd
> still need to name that variable passed to the helper functions
> called by format_display()---"buffer" would be a good name for that.

Well, we could make it an on-stack variable just fine. But I suspect that the only reason that this buffer exists is to optimize memory allocations: a git-fetch(1) can easily end up printing thousands or even hundreds of thousands of updated references, and reallocating that buffer for each of them is quite wasteful.

Another alternative would be to make the buffer static and local to the function. But you now have shared state again, and furthermore you have no easy way to unleak its contents.

In the end, I think that having the buffer as a member of the display state is the most straight-forward approach. It's self-contained and allows us to reduce the number of allocations to a minimum. That being said, I'm obviously biased here.

Patrick