On Wed, Mar 15, 2023 at 03:45:28PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Both callsites that call `format_display()` and then print the result to
> > standard error use the same formatting directive " %s\n" to print the
> > reference to disk, thus duplicating a small part of the logic.
> 
> Hmph.
> 
> If format_display() were a function whose role was to prepare the
> contents on a single line, it can be argued that it is caller's job
> to give a leading indent that is appropriate for the line in the
> context of the display it is producing.  "store-updated-refs" and
> "prune-refs" may be showing a list of refs that were affected under
> different heading, together with different kind of information, and
> depending on the way each of these callers organize its output, the
> appropriate indentation level for the line might be different.  So I
> think the current product format_display() gives its callers is
> perfectly defensible in that sense.
> 
> On the other hand, if format_display() is only about showing a
> single line in the tightly limited context (in other words, both of
> its callers promise that they will forever be happy with the
> function showing exactly the same output), then this refactoring
> would be OK.  In addition, it may even make more sense, if that were
> the role of this callee, to do the actual printing, not just
> preparing a line of string into a strbuf, in this callee, by moving
> the fputs() from caller to callee.
> 
> So, I dunno.  The result of applying this patch leaves it in an
> in-between state, where the division of labor between the caller and
> the callee smells somewhat iffy.
> 
> Thanks.

I totally agree with you here. From my point of view this "division of
labor" is getting fixed in the final patch that then also moves the
printing logic into `format_display()`.

Patrick