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

["lilinchao@oschina.cn" <lilinchao@oschina.cn>]

>
>On Fri, Aug 05 2022, Li Linchao via GitGitGadget wrote:
>
>> 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... 
OK
>
>> +
>> +-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? 
Yes, this makes sense.
>
>>  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. 
Agree
>
>> -	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). 
OK
>
>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. 
The output here is just "446  bytes" if we use '--disk-usage' option in this rest repo.
But Github CI/linux-sha256 reminded me that I made a mistake that
I should avoid to hardcore actual size here.
>
>
>> +'
>> +
>> +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). 
OK.

Many thanks for all your review comments :)