Re: [PATCH v2 2/3] strbuf: Add method to convert byte-size to human readable form

[Marcus Griep <marcus@griep.us>]

Petr Baudis wrote:
> Frankly, I doubt this has too much value, and it complicates the code _a
> lot_. If you can't fit your stuff into pretty column, it's better to
> just print whatever you have to and disrupt the columns instead of
> _failing_, isn't it?

Generally, the only reason for such a failure would be requesting
conflicting scale and maxlen values or requesting a maxlen which would
be too small to reasonably display any value, hence an empty string and
a number reporting how much more is necessary to get appropriate output.

In other words, a failure reports that you probably requested an
irrational number for maxlen.  It would probably be easier to understand
if it were in terms of the numeric output rather than the entire string.
If I change it that way, then there shouldn't be an "irrational" positive
number to request, eliminating the need for these failures.

> Shouldn't pretty much all of this be documented in the code too?

Should I stuff this in comments in the header file, source file, or both?

> My point still stands - in case of binary units, we should always
> consistently use the i suffix. So having an example in the commit
> message that advertises "bps" is simply wrong when it should read "iB/s"
> (like it does with the current progress.c code).
> 
> I may sound boring, but it seems to me that you're still ignoring my
> point quitly without proper counter-argumentation and I think it's an
> important want, and since it's so hard to keep things consistent across
> the wide Git codebase, we should do all we can to keep it.

From a consistency standpoint, I can certainly agree.  It's not hard to
implement.  I wanted to avoid pigeon-holeing, but to keep our reporting
consistent, using '*i' for all binaries works for me.

> Whitespace damage? Also at a lot of other places in your patch.

No damage.  It was indicated to me that 80-ish was the preferred width, so
I was trying to follow that.  If that's not true in the C sources, I'll
bundle things up a bit more.

> Hmmm. We could have
> 
> +        char *hr_prefixes[] = {
> +		"", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi", "Yi", NULL
> +	};
> +        char *hr_si_prefixes[] = {
> +		"", "k", "M", "G", "T", "P", "E", "Z", "Y", NULL
> +	};
> 
> ;-)

Per previous, sounds good to me.

Overall, I was looking to create a generic function that could be used
across Git without making assumptions of the consumer.  Hence the maxlen,
scale, SI, and space configurability.

Thanks for the input, and I'll work up another draft.

-- 
Marcus Griep
GPG Key ID: 0x5E968152
——
http://www.boohaunt.net
את.ψο´