>
>On Fri, Aug 05 2022, Li Linchao via GitGitGadget wrote:
>
>> From: Li Linchao <lilinchao@xxxxxxxxxx>
>>
>> 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 :)
