Re: [PATCH v2 3/3] count-objects: add human-readable size option

From: Petr Baudis <pasky@suse.cz>
Date: Aug 14, 2008, 22:37 UTC
Message-ID: <20080814223740.GD10544@machine.or.cz>
In-Reply-To: <1218752308-3173-4-git-send-email-marcus@griep.us>

On Thu, Aug 14, 2008 at 06:18:28PM -0400, Marcus Griep wrote:

Show 18 quoted lines

> @@ -21,9 +21,14 @@ OPTIONS
>  --verbose::
>  	In addition to the number of loose objects and disk
>  	space consumed, it reports the number of in-pack
> -	objects, number of packs, and number of objects that can be
> -	removed by running `git prune-packed`.
> -
> +	objects, number of packs, disk space consumed by those packs
> +	and number of objects that can be removed by running
> +	`git prune-packed`.
> +
> +-H::
> +--human-sizes::
> +	Displays sizes reported by `--verbose` in a more
> +	human-readable format. (e.g. 22M or 1.5G)
>  
>  Author
>  ------

Can you guess what would I bug you about? ;-)

Show 11 quoted lines

> -		printf("size-pack: %lu\n", size_pack / 1024);
> +		printf("size-pack: ");
> +		if (human_readable) {
> +			struct strbuf sb;
> +			strbuf_init(&sb, 0);
> +			strbuf_append_human_readable(&sb, size_pack,
> +							0, 0, "", 0);
> +			printf("%s\n", sb.buf);
> +		}
> +		else
> +			printf("%lu\n", size_pack / 1024);

If it's non-human-readable anyway, why are you dividing this by 1024? At any rate, it is not obvious at all that the size-pack is not actually size-pack but size-pack/1024. You should either add the (fixed) unit string behind or name it size-pack-kb - or just not divide it at all?

This also applies to PATCH1/3 in case it would get applied but the other two wouldn't.

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