From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Li Linchao via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
Li Linchao <lilinchao@oschina.cn>
Subject: Re: [PATCH] rev-list: support `--human-readable` option when applied `disk-usage`
Date: Fri, 05 Aug 2022 12:03:29 +0200 [thread overview]
Message-ID: <220805.864jyrro9k.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <pull.1313.git.1659686097163.gitgitgadget@gmail.com>
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...
> +
> +-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).
next prev parent reply other threads:[~2022-08-05 10:14 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-05 7:54 [PATCH] rev-list: support `--human-readable` option when applied `disk-usage` Li Linchao via GitGitGadget
2022-08-05 10:03 ` Ævar Arnfjörð Bjarmason [this message]
2022-08-05 11:01 ` lilinchao
2022-08-08 8:35 ` [PATCH v2] rev-list: support human-readable output for `--disk-usage` Li Linchao via GitGitGadget
2022-08-08 9:37 ` lilinchao
2022-08-09 13:22 ` Jeff King
2022-08-09 16:46 ` lilinchao
2022-08-10 6:01 ` [PATCH v3] " Li Linchao via GitGitGadget
2022-08-10 7:18 ` Johannes Sixt
2022-08-10 11:14 ` [PATCH v4] " Li Linchao via GitGitGadget
2022-08-10 17:34 ` Junio C Hamano
2022-08-10 21:20 ` Jeff King
2022-08-10 21:25 ` Junio C Hamano
2022-08-11 5:20 ` Junio C Hamano
2022-08-11 8:38 ` Jeff King
2022-08-11 4:47 ` [PATCH v5] " Li Linchao via GitGitGadget
2022-08-11 20:49 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220805.864jyrro9k.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=lilinchao@oschina.cn \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).