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

[Petr Baudis <pasky@suse.cz>]

On Thu, Aug 14, 2008 at 06:18:27PM -0400, Marcus Griep wrote:
> Takes a strbuf as its first argument and appends the human-readable
> form of 'value', the second argument, to that buffer.
> 
> Argument 3, 'maxlen', specifies the longest string that should
> be returned. That will make it easier for any pretty-ish formatting
> like `ls` and `du` use. A value of 0 is unlimited length.

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?

> 'scale' specifies a boundary, above which 'value' should be
> reduced, and below which it should be reported. Commonly this is
> 1000.  If 0, then it will find a scale that best fits into 'maxlen'.
> If both 'maxlen' and 'scale' are 0, then scale will default to 1000.
> 
> 'suffix' is appended onto every formatted string.  This is often
> "", "B", "bps", "objects" etc.
> 
> 'flags' provides the ability to switch between a binary (1024)
> and an si (1000) period (HR_USE_SI).  Also, adding a space between
> number and unit (HR_SPACE).
> 
> On success, returns 0.  If maxlen is specified and there is not
> enough space given the scale or an inordinately large value, returns
> -n, where n is the amount of additional length necessary.
> 
> e.g. strbuf_append_human_readable(sb, 1012, 0, 0, "bps", HR_SPACE)
> produces "0.9 Kbps".

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

> Also, add in test cases to ensure it produces the expected output
> and to demonstrate what different arguments do.
> 
> Signed-off-by: Marcus Griep <marcus@griep.us>

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.

> +int strbuf_append_human_readable(struct strbuf *sb,
> +				double val,
> +				int maxlen, int scale,
> +				const char *suffix,
> +				int flags)
> +{
> +	const int maxscale = 7;
> +
> +        char *hr_prefixes[] = {
> +		"", "K", "M", "G", "T", "P", "E", "Z", "Y", NULL
> +	};
> +        char **prefix = &hr_prefixes[0];

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

> +	int period = 1024;
> +	int sign = val < 0 ? -1 : 1;
> +	/* Baselen is (sign, if needed) (digit) (space, if needed)
> +			(prefix) (suffix) */
> +	int baselen = (val < 0 ? 1 : 0) + 1 + (flags & HR_SPACE ? 1 : 0)
> +			+ 1 + strlen(suffix);
> +
> +	val *= sign;
> +
> +	if (flags & HR_USE_SI) {
> +		period = 1000;
> +		hr_prefixes[1] = "k";

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
+	};

;-)

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates