Re: [PATCH v2 2/3] strbuf: Add method to convert byte-size to human readable form
- From
Petr Baudis <pasky@suse.cz>
- Date
- Aug 14, 2008, 22:34 UTC
- Message-ID
- <20080814223429.GC10544@machine.or.cz>
- In-Reply-To
- <1218752308-3173-3-git-send-email-marcus@griep.us>
On Thu, Aug 14, 2008 at 06:18:27PM -0400, Marcus Griep wrote:
Show 6 quoted lines
> 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?
Show 18 quoted lines
> '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.
Show 12 quoted lines
> +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.
Show 12 quoted lines
> + 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