Re: [PATCH 2] count-objects: add human-readable size option

["Shawn O. Pearce" <spearce@spearce.org>]

Marcus Griep <marcus@griep.us> wrote:
> diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
> index 75a8da1..291bc5e 100644
> --- a/Documentation/git-count-objects.txt
> +++ b/Documentation/git-count-objects.txt
> +++ b/builtin-count-objects.c
...
> +void human_readable_size(char *buf, int buf_size, double size /* in bytes */)

Hmm.  This probably should be static.  Or if it really is meant
to be a utility for use elsewhere in Git, moved to someplace where
string handling is done.  Its not strbuf related, but maybe strbuf.c
is a better location for this sort of library function.

If you do move this to strbuf.c, how about having it take a strbuf
in and appending the formatted text onto it?  You'll neer have to
worry about the buffer being too small and it fits into the whole
strbuf.c module thing.

If you keep this static here in builtin-count-objects.c, how about
making the char *buf static scoped to the function, so you don't
need to pass the buffer, its size, nor check its size?

> +{
> +	char human_readable_prefixes[10] = "BKMGTPEZY";
> +	if (buf_size < 5)
> +		die("insufficient buffer size");
> +	int i = 0;

We don't declare variables after statements.  Please declare all
variables at the start of the block as not all compilers we support
support this C99 syntax.

Oh, and welcome to Git.  I saw your SVN patches.  Glad to see
you hacking.  ;-)

> +	for (; i < 8 && size >= 1000 ; ++i, size = size / 1024)
> +		;
> +	if (size >= 1000)
> +		die("size greater than 999Y");
> +	sprintf(buf, "%.*f%c",
> +		size < 10 ? 1 : 0,
> +		size,
> +		human_readable_prefixes[i]
> +		);
> +}
> +

-- 
Shawn.