Re: [PATCH] rev-list: support `--human-readable` option when applied `disk-usage`

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 5, 2022, 10:03 UTC
Message-ID: <220805.864jyrro9k.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1313.git.1659686097163.gitgitgadget@gmail.com>

On Fri, Aug 05 2022, Li Linchao via GitGitGadget wrote:

Show 6 quoted lines

> From: Li Linchao <lilinchao@oschina.cn>
>
> The '--disk-usage' option for git-rev-list was introduced in 16950f8384
> (rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).
> This is very useful for people inspect their git repo's objects usage
> infomation, but the result number is quit hard for human to read.

s/the result number/the resulting number/ s/for human/for a human/

>
> Teach git rev-list to output more human readable result when using

s/to output more human/to output a human/

> '--disk-usage' to calculate objects disk usage.

For this I'd just s/ to calculate objects disk usage//. I.e. we already discussed what --disk-usage does...

Show 6 quoted lines

> +
> +-H::
> +--human-readable::
> +	Print on-disk objects size in human readable format. This option
> +	must be combined with `--disk-usage` together.
>  endif::git-rev-list[]

I'd really prefer if we didn't squat on -H, rev-list is overridden enough, but how about:

	--disk-usage
	--disk-usage=human

Rather than introducing a new option?

Show 19 quoted lines

>  	struct bitmap_index *bitmap_git;
> +	struct strbuf bitmap_size_buf = STRBUF_INIT;
> +	off_t size_from_bitmap;
>  
>  	if (!show_disk_usage)
>  		return -1;
> @@ -481,8 +484,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
>  	if (!bitmap_git)
>  		return -1;
>  
> -	printf("%"PRIuMAX"\n",
> -	       (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));
> +	size_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);
> +	if (human_readable) {
> +		strbuf_humanise_bytes(&bitmap_size_buf, size_from_bitmap);
> +		printf("%s\n", bitmap_size_buf.buf);
> +	} else
> +		printf("%"PRIuMAX"\n", (uintmax_t)size_from_bitmap);
> +	strbuf_release(&bitmap_size_buf);

I think this would be better if we just use the strbuf unconditionally (and a short &sb is conventional in such a short one-use function). So just:

	if (human_readable)
        	strbuf_humanise_bytes(&sb, size_from_bitmap);
	else
		strbuf_addf(&sb, "%"PRIuMAX", (uintmax_t)size_from_bitmap);
	puts(sb.buf);

It gets you rid of the need for {} braces, and I think makes for a nicer read.

Show 9 quoted lines

> -	if (show_disk_usage)
> -		printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage);
> +	if (show_disk_usage) {
> +		if (human_readable) {
> +			strbuf_humanise_bytes(&disk_buf, total_disk_usage);
> +			printf("%s\n", disk_buf.buf);
> +		} else
> +			printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage);
> +	}

Ditto, and we could make the &sb scoped to that "if (show_disk_usage)".

> +test_expect_success 'rev-list --disk-usage with --human-readable' '
> +	git rev-list --objects HEAD --disk-usage --human-readable >actual &&
> +	test_i18ngrep -e "446 bytes" actual

use grep, not test_i18ngrep (the latter should be going away entirely).

But actually we should use test_cmp here, isn't that the *entire* output? I.e. won't this pass?

	echo 446 bytes >expect &&
	... >expect &&
	test_cmp expect actual

If so let's test what we really mean, i.e. we want *this* to be the output, not to have output that has that sub-string on any arbitrary amount of lines somewhere...

In this case it's unlikely to do the wrong thing, but it's a good habit to get into...

> +test_expect_success 'rev-list --disk-usage with bitmap and --human-readable' '
> +	git rev-list --objects HEAD --use-bitmap-index --disk-usage -H >actual &&
> +	test_i18ngrep -e "446 bytes" actual

ditto.

Show 6 quoted lines

> +'
> +
> +test_expect_success 'rev-list use --human-readable without --disk-usage' '
> +	test_must_fail git rev-list --objects HEAD --human-readable 2> err &&
> +	echo "fatal: option '\''--human-readable/-H'\'' should be used with" \
> +	"'\''--disk-usage'\'' together" >expect &&

You can make this a bit nicer by not using echo, use a here-doc instead:

	cat >expect <<-\EOF
        fatal: ...
	EOF

But you'll still need the '\'' quoting, but I thing it'll be better, and avoids the line-wrapping (which we try to avoid for this sort of thing).