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...
> +
> +-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?
> 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.
> - 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.
> +'
> +
> +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).
