Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2022, 05:20 UTC
Message-ID: <xmqqy1vvgxv5.fsf@gitster.g>
In-Reply-To: <YvQhHOkjZatIqlFr@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> I don't care much between "printf directly vs strbuf" for the non-human
> case, but it was an earlier review suggestion to connect them. I do
> think the result is a little easier to follow, but mostly I want to make
> it clear that the author is getting stuck between warring review
> comments here. ;)

The function does look like it can later be called millions of times, at which time we may want to measure, but as the only use of this helper function is to get called just once at the end with a single number and print it (as opposed to getting called once per each object), I do not care too much about the normal case being made unnecessarily more expensive, either.

I still do not see the point of changing it to print to a strbuf and puts() the result, though. It does not make the code shorter, more efficient, or more readable. Once "if (we are producing humanize format)" condition is hit, both of its branches can either be (1) responsible to print the number to the standard output stream, using whatever implementation, or (2) responsible to print the number to a strbuf, so that somebody outside the if statement will be respohnsible for printing that string to the standard output stream.

The patch chooses (2), which is more complex, for no good reason. A good thing about (1) is that the non-human codepath can STAY to be the same as before, which is one fewer chance to introduce unnecessary bugs.

Anyway...