On Wed, Mar 15, 2023 at 03:45:28PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
