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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2023, 22:45 UTC
Message-ID: <xmqq4jqlocqf.fsf@gitster.g>
In-Reply-To: <d45ec31eeaf42ee042bad04efd69668144df3138.1678878623.git.ps@pks.im>

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.