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

From: Jeff King <peff@peff.net>
Date: Aug 11, 2022, 08:38 UTC
Message-ID: <YvTACxGqVPBa+IDx@coredump.intra.peff.net>
In-Reply-To: <xmqqy1vvgxv5.fsf@gitster.g>

On Wed, Aug 10, 2022 at 10:20:14PM -0700, Junio C Hamano wrote:

Show 8 quoted lines

> 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.

I was not the original reviewer who suggested that change, but FWIW, I think the argument for cases like these is something like this. Between:

  if (some_cond) {
    foo(do_some_prep());
  } else {
    foo(do_some_other_prep());
  }

and:

  if (some_cond) {
    x = do_some_prep();
  } else {
    x = do_some_other_prep();
  }
  foo(x);

the latter makes it more clear that foo() is called on both sides of the conditional. In this case the "foo" is not exactly the same: on one it is printing a strbuf, and on the other it is a printf that direct-formats. But the principle is the same, if you want to make it clear that both sides will result in printing something.

As you note, it loses the possibility for one side of the conditional to do its "foo" in a more efficient way, but I don't think that's very important for this particular call site.

> 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.

True.

Again, I don't care much either way. But I am not quite on the "I do not see the point at all" side.

-Peff