Re: [PATCH 5/8] fetch: deduplicate handling of per-reference format

[Junio C Hamano <gitster@pobox.com>]

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.