{"thread":{"id":"58270","subject":"[PATCH] rev-list: support `--human-readable` option when applied `disk-usage`","startedAt":"2022-08-05T07:55:12Z","lastAt":"2022-08-11T20:49:57Z","messageCount":17,"participants":["Li Linchao via GitGitGadget","Ævar Arnfjörð Bjarmason","lilinchao@oschina.cn","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"460698","messageId":"pull.1313.git.1659686097163.gitgitgadget@gmail.com","threadId":"58270","inReplyTo":null,"subject":"[PATCH] rev-list: support `--human-readable` option when applied `disk-usage`","fromName":"Li Linchao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-05T07:54:57Z","receivedAt":"2022-08-05T07:55:12Z","isPatch":true,"sender":{"key":"name:Li Linchao","avatar":null},"body":"From: Li Linchao \n\nThe '--disk-usage' option for git-rev-list was introduced in 16950f8384\n(rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\nThis is very useful for people inspect their git repo's objects usage\ninfomation, but the result number is quit hard for human to read.\n\nTeach git rev-list to output more human readable result when using\n'--disk-usage' to calculate objects disk usage.\n\nSigned-off-by: Li Linchao \n---\n rev-list: support --human-readable option when applied disk-usage\n \n The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n (rev-list: add --disk-usage option for calculating disk usage,\n 2021-02-09). This is very useful for people inspect their git repo's\n objects usage infomation, but the result number is quit hard for human\n to read.\n \n Teach git rev-list to output more human readable result when using\n '--disk-usage' to calculate objects disk usage.\n \n Signed-off-by: Li Linchao lilinchao@oschina.cn\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1313\n\n Documentation/rev-list-options.txt | 5 +++++\n builtin/rev-list.c | 31 ++++++++++++++++++++++++++----\n t/t6115-rev-list-du.sh | 18 +++++++++++++++++\n 3 files changed, 50 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..d30301a9159 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -249,6 +249,11 @@ ifdef::git-rev-list[]\n \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n \tsection in linkgit:git-cat-file[1] for the limitations of what\n \t\"on-disk storage\" means.\n+\n+-H::\n+--human-readable::\n+\tPrint on-disk objects size in human readable format. This option\n+\tmust be combined with `--disk-usage` together.\n endif::git-rev-list[]\n \n --cherry-mark::\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 30fd8e83eaf..be677f29070 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -81,6 +81,7 @@ static int arg_show_object_names = 1;\n \n static int show_disk_usage;\n static off_t total_disk_usage;\n+static int human_readable;\n \n static off_t get_object_disk_usage(struct object *obj)\n {\n@@ -473,6 +474,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \t\t\t\t int filter_provided_objects)\n {\n \tstruct bitmap_index *bitmap_git;\n+\tstruct strbuf bitmap_size_buf = STRBUF_INIT;\n+\toff_t size_from_bitmap;\n \n \tif (!show_disk_usage)\n \t\treturn -1;\n@@ -481,8 +484,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \tif (!bitmap_git)\n \t\treturn -1;\n \n-\tprintf(\"%\"PRIuMAX\"\\n\",\n-\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n+\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n+\tif (human_readable) {\n+\t\tstrbuf_humanise_bytes(&bitmap_size_buf, size_from_bitmap);\n+\t\tprintf(\"%s\\n\", bitmap_size_buf.buf);\n+\t} else\n+\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)size_from_bitmap);\n+\tstrbuf_release(&bitmap_size_buf);\n \treturn 0;\n }\n \n@@ -490,6 +498,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n \tstruct rev_list_info info;\n+\tstruct strbuf disk_buf = STRBUF_INIT;\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n@@ -630,9 +639,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!strcmp(arg, \"--human-readable\") || !strcmp(arg, \"-H\")) {\n+\t\t\thuman_readable = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tusage(rev_list_usage);\n \n \t}\n+\n+\tif (!show_disk_usage && human_readable)\n+\t\tdie(_(\"option '%s' should be used with '%s' together\"), \"--human-readable/-H\", \"--disk-usage\");\n \tif (revs.commit_format != CMIT_FMT_USERFORMAT)\n \t\trevs.include_header = 1;\n \tif (revs.commit_format != CMIT_FMT_UNSPECIFIED) {\n@@ -752,10 +769,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tprintf(\"%d\\n\", revs.count_left + revs.count_right);\n \t}\n \n-\tif (show_disk_usage)\n-\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n+\tif (show_disk_usage) {\n+\t\tif (human_readable) {\n+\t\t\tstrbuf_humanise_bytes(&disk_buf, total_disk_usage);\n+\t\t\tprintf(\"%s\\n\", disk_buf.buf);\n+\t\t} else\n+\t\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n+\t}\n \n cleanup:\n \trelease_revisions(&revs);\n+\tstrbuf_release(&disk_buf);\n \treturn ret;\n }\ndiff --git a/t/t6115-rev-list-du.sh b/t/t6115-rev-list-du.sh\nindex b4aef32b713..614ebb72aaa 100755\n--- a/t/t6115-rev-list-du.sh\n+++ b/t/t6115-rev-list-du.sh\n@@ -48,4 +48,22 @@ check_du HEAD\n check_du --objects HEAD\n check_du --objects HEAD^..HEAD\n \n+\n+test_expect_success 'rev-list --disk-usage with --human-readable' '\n+\tgit rev-list --objects HEAD --disk-usage --human-readable >actual &&\n+\ttest_i18ngrep -e \"446 bytes\" actual\n+'\n+\n+test_expect_success 'rev-list --disk-usage with bitmap and --human-readable' '\n+\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage -H >actual &&\n+\ttest_i18ngrep -e \"446 bytes\" actual\n+'\n+\n+test_expect_success 'rev-list use --human-readable without --disk-usage' '\n+\ttest_must_fail git rev-list --objects HEAD --human-readable 2> err &&\n+\techo \"fatal: option '\\''--human-readable/-H'\\'' should be used with\" \\\n+\t\"'\\''--disk-usage'\\'' together\" >expect &&\n+\ttest_cmp err expect\n+'\n+\n test_done\n\nbase-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385\n-- \ngitgitgadget\n"},{"id":"460705","messageId":"220805.864jyrro9k.gmgdl@evledraar.gmail.com","threadId":"58270","inReplyTo":"pull.1313.git.1659686097163.gitgitgadget@gmail.com","subject":"Re: [PATCH] rev-list: support `--human-readable` option when applied `disk-usage`","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-05T10:03:29Z","receivedAt":"2022-08-05T10:14:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 05 2022, Li Linchao via GitGitGadget wrote:\n\n> From: Li Linchao \n>\n> The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n> (rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\n> This is very useful for people inspect their git repo's objects usage\n> infomation, but the result number is quit hard for human to read.\n\ns/the result number/the resulting number/\ns/for human/for a human/\n\n>\n> Teach git rev-list to output more human readable result when using\n\ns/to output more human/to output a human/\n\n> '--disk-usage' to calculate objects disk usage.\n\nFor this I'd just s/ to calculate objects disk usage//. I.e. we already\ndiscussed what --disk-usage does...\n\n> +\n> +-H::\n> +--human-readable::\n> +\tPrint on-disk objects size in human readable format. This option\n> +\tmust be combined with `--disk-usage` together.\n> endif::git-rev-list[]\n\nI'd really prefer if we didn't squat on -H, rev-list is overridden\nenough, but how about:\n\n\t--disk-usage\n\t--disk-usage=human\n\nRather than introducing a new option?\n\n> \tstruct bitmap_index *bitmap_git;\n> +\tstruct strbuf bitmap_size_buf = STRBUF_INIT;\n> +\toff_t size_from_bitmap;\n> \n> \tif (!show_disk_usage)\n> \t\treturn -1;\n> @@ -481,8 +484,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n> \tif (!bitmap_git)\n> \t\treturn -1;\n> \n> -\tprintf(\"%\"PRIuMAX\"\\n\",\n> -\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n> +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n> +\tif (human_readable) {\n> +\t\tstrbuf_humanise_bytes(&bitmap_size_buf, size_from_bitmap);\n> +\t\tprintf(\"%s\\n\", bitmap_size_buf.buf);\n> +\t} else\n> +\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)size_from_bitmap);\n> +\tstrbuf_release(&bitmap_size_buf);\n\nI think this would be better if we just use the strbuf unconditionally\n(and a short &sb is conventional in such a short one-use function). So just:\n\n\tif (human_readable)\n \tstrbuf_humanise_bytes(&sb, size_from_bitmap);\n\telse\n\t\tstrbuf_addf(&sb, \"%\"PRIuMAX\", (uintmax_t)size_from_bitmap);\n\tputs(sb.buf);\n\nIt gets you rid of the need for {} braces, and I think makes for a nicer\nread.\n\n> -\tif (show_disk_usage)\n> -\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n> +\tif (show_disk_usage) {\n> +\t\tif (human_readable) {\n> +\t\t\tstrbuf_humanise_bytes(&disk_buf, total_disk_usage);\n> +\t\t\tprintf(\"%s\\n\", disk_buf.buf);\n> +\t\t} else\n> +\t\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n> +\t}\n\nDitto, and we could make the &sb scoped to that \"if (show_disk_usage)\".\n\n> +test_expect_success 'rev-list --disk-usage with --human-readable' '\n> +\tgit rev-list --objects HEAD --disk-usage --human-readable >actual &&\n> +\ttest_i18ngrep -e \"446 bytes\" actual\n\nuse grep, not test_i18ngrep (the latter should be going away entirely).\n\nBut actually we should use test_cmp here, isn't that the *entire*\noutput? I.e. won't this pass?\n\n\techo 446 bytes >expect &&\n\t... >expect &&\n\ttest_cmp expect actual\n\nIf so let's test what we really mean, i.e. we want *this* to be the\noutput, not to have output that has that sub-string on any arbitrary\namount of lines somewhere...\n\nIn this case it's unlikely to do the wrong thing, but it's a good habit\nto get into...\n\n> +test_expect_success 'rev-list --disk-usage with bitmap and --human-readable' '\n> +\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage -H >actual &&\n> +\ttest_i18ngrep -e \"446 bytes\" actual\n\nditto.\n\n\n> +'\n> +\n> +test_expect_success 'rev-list use --human-readable without --disk-usage' '\n> +\ttest_must_fail git rev-list --objects HEAD --human-readable 2> err &&\n> +\techo \"fatal: option '\\''--human-readable/-H'\\'' should be used with\" \\\n> +\t\"'\\''--disk-usage'\\'' together\" >expect &&\n\nYou can make this a bit nicer by not using echo, use a here-doc instead:\n\n\tcat >expect <<-\\EOF\n fatal: ...\n\tEOF\n\nBut you'll still need the '\\'' quoting, but I thing it'll be better, and\navoids the line-wrapping (which we try to avoid for this sort of thing).\n"},{"id":"460706","messageId":"2022080519002378872119@oschina.cn","threadId":"58270","inReplyTo":"220805.864jyrro9k.gmgdl@evledraar.gmail.com","subject":"Re: Re: [PATCH] rev-list: support `--human-readable` option when applied `disk-usage`","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2022-08-05T11:01:24Z","receivedAt":"2022-08-05T11:01:33Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":">\n>On Fri, Aug 05 2022, Li Linchao via GitGitGadget wrote:\n>\n>> From: Li Linchao \n>>\n>> The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n>> (rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\n>> This is very useful for people inspect their git repo's objects usage\n>> infomation, but the result number is quit hard for human to read.\n>\n>s/the result number/the resulting number/\n>s/for human/for a human/\n>\n>>\n>> Teach git rev-list to output more human readable result when using\n>\n>s/to output more human/to output a human/\n>\n>> '--disk-usage' to calculate objects disk usage.\n>\n>For this I'd just s/ to calculate objects disk usage//. I.e. we already\n>discussed what --disk-usage does... \nOK\n>\n>> +\n>> +-H::\n>> +--human-readable::\n>> +\tPrint on-disk objects size in human readable format. This option\n>> +\tmust be combined with `--disk-usage` together.\n>>  endif::git-rev-list[]\n>\n>I'd really prefer if we didn't squat on -H, rev-list is overridden\n>enough, but how about:\n>\n>\t--disk-usage\n>\t--disk-usage=human\n>\n>Rather than introducing a new option? \nYes, this makes sense.\n>\n>>  struct bitmap_index *bitmap_git;\n>> +\tstruct strbuf bitmap_size_buf = STRBUF_INIT;\n>> +\toff_t size_from_bitmap;\n>> \n>>  if (!show_disk_usage)\n>>  return -1;\n>> @@ -481,8 +484,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n>>  if (!bitmap_git)\n>>  return -1;\n>> \n>> -\tprintf(\"%\"PRIuMAX\"\\n\",\n>> -\t       (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n>> +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n>> +\tif (human_readable) {\n>> +\tstrbuf_humanise_bytes(&bitmap_size_buf, size_from_bitmap);\n>> +\tprintf(\"%s\\n\", bitmap_size_buf.buf);\n>> +\t} else\n>> +\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)size_from_bitmap);\n>> +\tstrbuf_release(&bitmap_size_buf);\n>\n>I think this would be better if we just use the strbuf unconditionally\n>(and a short &sb is conventional in such a short one-use function). So just:\n>\n>\tif (human_readable)\n>        strbuf_humanise_bytes(&sb, size_from_bitmap);\n>\telse\n>\tstrbuf_addf(&sb, \"%\"PRIuMAX\", (uintmax_t)size_from_bitmap);\n>\tputs(sb.buf);\n>\n>It gets you rid of the need for {} braces, and I think makes for a nicer\n>read. \nAgree\n>\n>> -\tif (show_disk_usage)\n>> -\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n>> +\tif (show_disk_usage) {\n>> +\tif (human_readable) {\n>> +\tstrbuf_humanise_bytes(&disk_buf, total_disk_usage);\n>> +\tprintf(\"%s\\n\", disk_buf.buf);\n>> +\t} else\n>> +\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n>> +\t}\n>\n>Ditto, and we could make the &sb scoped to that \"if (show_disk_usage)\".\n>\n>> +test_expect_success 'rev-list --disk-usage with --human-readable' '\n>> +\tgit rev-list --objects HEAD --disk-usage --human-readable >actual &&\n>> +\ttest_i18ngrep -e \"446 bytes\" actual\n>\n>use grep, not test_i18ngrep (the latter should be going away entirely). \nOK\n>\n>But actually we should use test_cmp here, isn't that the *entire*\n>output? I.e. won't this pass?\n>\n>\techo 446 bytes >expect &&\n>\t... >expect &&\n>\ttest_cmp expect actual\n>\n>If so let's test what we really mean, i.e. we want *this* to be the\n>output, not to have output that has that sub-string on any arbitrary\n>amount of lines somewhere...\n>\n>In this case it's unlikely to do the wrong thing, but it's a good habit\n>to get into...\n>\n>> +test_expect_success 'rev-list --disk-usage with bitmap and --human-readable' '\n>> +\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage -H >actual &&\n>> +\ttest_i18ngrep -e \"446 bytes\" actual\n>\n>ditto. \nThe output here is just \"446  bytes\" if we use '--disk-usage' option in this rest repo.\nBut Github CI/linux-sha256 reminded me that I made a mistake that\nI should avoid to hardcore actual size here.\n>\n>\n>> +'\n>> +\n>> +test_expect_success 'rev-list use --human-readable without --disk-usage' '\n>> +\ttest_must_fail git rev-list --objects HEAD --human-readable 2> err &&\n>> +\techo \"fatal: option '\\''--human-readable/-H'\\'' should be used with\" \\\n>> +\t\"'\\''--disk-usage'\\'' together\" >expect &&\n>\n>You can make this a bit nicer by not using echo, use a here-doc instead:\n>\n>\tcat >expect <<-\\EOF\n>        fatal: ...\n>\tEOF\n>\n>But you'll still need the '\\'' quoting, but I thing it'll be better, and\n>avoids the line-wrapping (which we try to avoid for this sort of thing). \nOK.\n\nMany thanks for all your review comments :)"},{"id":"460807","messageId":"pull.1313.v2.git.1659947722132.gitgitgadget@gmail.com","threadId":"58270","inReplyTo":"pull.1313.git.1659686097163.gitgitgadget@gmail.com","subject":"[PATCH v2] rev-list: support human-readable output for `--disk-usage`","fromName":"Li Linchao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-08T08:35:21Z","receivedAt":"2022-08-08T08:35:33Z","isPatch":true,"sender":{"key":"name:Li Linchao","avatar":null},"body":"From: Li Linchao \n\nThe '--disk-usage' option for git-rev-list was introduced in 16950f8384\n(rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\nThis is very useful for people inspect their git repo's objects usage\ninfomation, but the resulting number is quit hard for a human to read.\n\nTeach git rev-list to output a human readable result when using\n'--disk-usage'.\n\nSigned-off-by: Li Linchao \n---\n rev-list: support human-readable output for disk-usage\n \n The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n (rev-list: add --disk-usage option for calculating disk usage,\n 2021-02-09). This is very useful for people inspect their git repo's\n objects usage infomation, but the result number is quit hard for human\n to read.\n \n Teach git rev-list to output more human readable result when using\n '--disk-usage' to calculate objects disk usage.\n \n Signed-off-by: Li Linchao lilinchao@oschina.cn\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1313\n\nRange-diff vs v1:\n\n 1: 7f8a7d3f61d ! 1: 7e34d16efe4 rev-list: support `--human-readable` option when applied `disk-usage`\n @@ Metadata\n Author: Li Linchao \n \n ## Commit message ##\n - rev-list: support `--human-readable` option when applied `disk-usage`\n + rev-list: support human-readable output for `--disk-usage`\n \n The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n (rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\n This is very useful for people inspect their git repo's objects usage\n - infomation, but the result number is quit hard for human to read.\n + infomation, but the resulting number is quit hard for a human to read.\n \n - Teach git rev-list to output more human readable result when using\n - '--disk-usage' to calculate objects disk usage.\n + Teach git rev-list to output a human readable result when using\n + '--disk-usage'.\n \n Signed-off-by: Li Linchao \n \n ## Documentation/rev-list-options.txt ##\n @@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]\n - \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n - \tsection in linkgit:git-cat-file[1] for the limitations of what\n - \t\"on-disk storage\" means.\n -+\n -+-H::\n -+--human-readable::\n -+\tPrint on-disk objects size in human readable format. This option\n -+\tmust be combined with `--disk-usage` together.\n - endif::git-rev-list[]\n + \tto `/dev/null` as the output does not have to be formatted.\n \n - --cherry-mark::\n + --disk-usage::\n ++--disk-usage=human::\n + \tSuppress normal output; instead, print the sum of the bytes used\n +-\tfor on-disk storage by the selected commits or objects. This is\n ++\tfor on-disk storage by the selected commits or objects.\n ++\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n ++\tmeans to print objects size in human readable format. This is\n + \tequivalent to piping the output into `git cat-file\n + \t--batch-check='%(objectsize:disk)'`, except that it runs much\n + \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n \n ## builtin/rev-list.c ##\n +@@ builtin/rev-list.c: static const char rev_list_usage[] =\n + \" --parents\\n\"\n + \" --children\\n\"\n + \" --objects | --objects-edge\\n\"\n ++\" --disk-usage | --disk-usage=human\\n\"\n + \" --unpacked\\n\"\n + \" --header | --pretty\\n\"\n + \" --[no-]object-names\\n\"\n @@ builtin/rev-list.c: static int arg_show_object_names = 1;\n \n static int show_disk_usage;\n @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,\n \t\t\t\t int filter_provided_objects)\n {\n \tstruct bitmap_index *bitmap_git;\n -+\tstruct strbuf bitmap_size_buf = STRBUF_INIT;\n ++\tstruct strbuf disk_buf = STRBUF_INIT;\n +\toff_t size_from_bitmap;\n \n \tif (!show_disk_usage)\n @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,\n -\tprintf(\"%\"PRIuMAX\"\\n\",\n -\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n -+\tif (human_readable) {\n -+\t\tstrbuf_humanise_bytes(&bitmap_size_buf, size_from_bitmap);\n -+\t\tprintf(\"%s\\n\", bitmap_size_buf.buf);\n -+\t} else\n -+\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)size_from_bitmap);\n -+\tstrbuf_release(&bitmap_size_buf);\n ++\tif (human_readable)\n ++\t\tstrbuf_humanise_bytes(&disk_buf, size_from_bitmap);\n ++\telse\n ++\t\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)size_from_bitmap);\n ++\tputs(disk_buf.buf);\n ++\tstrbuf_release(&disk_buf);\n \treturn 0;\n }\n \n -@@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)\n - {\n - \tstruct rev_info revs;\n - \tstruct rev_list_info info;\n -+\tstruct strbuf disk_buf = STRBUF_INIT;\n - \tstruct setup_revision_opt s_r_opt = {\n - \t\t.allow_exclude_promisor_objects = 1,\n - \t};\n @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n -+\t\tif (!strcmp(arg, \"--human-readable\") || !strcmp(arg, \"-H\")) {\n -+\t\t\thuman_readable = 1;\n -+\t\t\tcontinue;\n -+\t\t}\n -+\n - \t\tusage(rev_list_usage);\n +-\t\tif (!strcmp(arg, \"--disk-usage\")) {\n +-\t\t\tshow_disk_usage = 1;\n +-\t\t\tinfo.flags |= REV_LIST_QUIET;\n +-\t\t\tcontinue;\n ++\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n ++\t\t\tif (*arg == '=') {\n ++\t\t\t\tif (!strcmp(++arg, \"human\")) {\n ++\t\t\t\t\thuman_readable = 1;\n ++\t\t\t\t\tshow_disk_usage = 1;\n ++\t\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n ++\t\t\t\t\tcontinue;\n ++\t\t\t\t} else\n ++\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n ++\t\t\t} else {\n ++\t\t\t\tshow_disk_usage = 1;\n ++\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n ++\t\t\t\tcontinue;\n ++\t\t\t}\n + \t\t}\n \n - \t}\n -+\n -+\tif (!show_disk_usage && human_readable)\n -+\t\tdie(_(\"option '%s' should be used with '%s' together\"), \"--human-readable/-H\", \"--disk-usage\");\n - \tif (revs.commit_format != CMIT_FMT_USERFORMAT)\n - \t\trevs.include_header = 1;\n - \tif (revs.commit_format != CMIT_FMT_UNSPECIFIED) {\n + \t\tusage(rev_list_usage);\n @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tprintf(\"%d\\n\", revs.count_left + revs.count_right);\n \t}\n @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr\n -\tif (show_disk_usage)\n -\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n +\tif (show_disk_usage) {\n -+\t\tif (human_readable) {\n ++\t\tstruct strbuf disk_buf = STRBUF_INIT;\n ++\t\tif (human_readable)\n +\t\t\tstrbuf_humanise_bytes(&disk_buf, total_disk_usage);\n -+\t\t\tprintf(\"%s\\n\", disk_buf.buf);\n -+\t\t} else\n -+\t\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n ++\t\telse\n ++\t\t\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)total_disk_usage);\n ++\t\tputs(disk_buf.buf);\n ++\t\tstrbuf_release(&disk_buf);\n +\t}\n \n cleanup:\n \trelease_revisions(&revs);\n -+\tstrbuf_release(&disk_buf);\n - \treturn ret;\n - }\n \n ## t/t6115-rev-list-du.sh ##\n @@ t/t6115-rev-list-du.sh: check_du HEAD\n check_du --objects HEAD\n check_du --objects HEAD^..HEAD\n \n -+\n -+test_expect_success 'rev-list --disk-usage with --human-readable' '\n -+\tgit rev-list --objects HEAD --disk-usage --human-readable >actual &&\n -+\ttest_i18ngrep -e \"446 bytes\" actual\n ++# As mentioned above, don't use hardcode sizes as actual size, but use the\n ++# output from git cat-file.\n ++test_expect_success 'rev-list --disk-usage=human' '\n ++\tgit rev-list --objects HEAD --disk-usage=human >actual &&\n ++\tdisk_usage_slow --objects HEAD >actual_size &&\n ++\tgrep \"$(cat actual_size) bytes\" actual\n +'\n +\n -+test_expect_success 'rev-list --disk-usage with bitmap and --human-readable' '\n -+\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage -H >actual &&\n -+\ttest_i18ngrep -e \"446 bytes\" actual\n ++test_expect_success 'rev-list --disk-usage=human with bitmaps' '\n ++\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&\n ++\tdisk_usage_slow --objects HEAD >actual_size &&\n ++\tgrep \"$(cat actual_size) bytes\" actual\n +'\n +\n -+test_expect_success 'rev-list use --human-readable without --disk-usage' '\n -+\ttest_must_fail git rev-list --objects HEAD --human-readable 2> err &&\n -+\techo \"fatal: option '\\''--human-readable/-H'\\'' should be used with\" \\\n -+\t\"'\\''--disk-usage'\\'' together\" >expect &&\n ++test_expect_success 'rev-list use --disk-usage unproperly' '\n ++\ttest_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfatal: invalid value for '\\''--disk-usage'\\'': '\\''typo'\\'', try --disk-usage=human\n ++\tEOF\n +\ttest_cmp err expect\n +'\n +\n\n\n Documentation/rev-list-options.txt | 5 +++-\n builtin/rev-list.c | 42 ++++++++++++++++++++++++------\n t/t6115-rev-list-du.sh | 22 ++++++++++++++++\n 3 files changed, 60 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..9966ce4ef91 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -242,8 +242,11 @@ ifdef::git-rev-list[]\n \tto `/dev/null` as the output does not have to be formatted.\n \n --disk-usage::\n+--disk-usage=human::\n \tSuppress normal output; instead, print the sum of the bytes used\n-\tfor on-disk storage by the selected commits or objects. This is\n+\tfor on-disk storage by the selected commits or objects.\n+\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n+\tmeans to print objects size in human readable format. This is\n \tequivalent to piping the output into `git cat-file\n \t--batch-check='%(objectsize:disk)'`, except that it runs much\n \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 30fd8e83eaf..05ef232dcbe 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -46,6 +46,7 @@ static const char rev_list_usage[] =\n \" --parents\\n\"\n \" --children\\n\"\n \" --objects | --objects-edge\\n\"\n+\" --disk-usage | --disk-usage=human\\n\"\n \" --unpacked\\n\"\n \" --header | --pretty\\n\"\n \" --[no-]object-names\\n\"\n@@ -81,6 +82,7 @@ static int arg_show_object_names = 1;\n \n static int show_disk_usage;\n static off_t total_disk_usage;\n+static int human_readable;\n \n static off_t get_object_disk_usage(struct object *obj)\n {\n@@ -473,6 +475,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \t\t\t\t int filter_provided_objects)\n {\n \tstruct bitmap_index *bitmap_git;\n+\tstruct strbuf disk_buf = STRBUF_INIT;\n+\toff_t size_from_bitmap;\n \n \tif (!show_disk_usage)\n \t\treturn -1;\n@@ -481,8 +485,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \tif (!bitmap_git)\n \t\treturn -1;\n \n-\tprintf(\"%\"PRIuMAX\"\\n\",\n-\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n+\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n+\tif (human_readable)\n+\t\tstrbuf_humanise_bytes(&disk_buf, size_from_bitmap);\n+\telse\n+\t\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)size_from_bitmap);\n+\tputs(disk_buf.buf);\n+\tstrbuf_release(&disk_buf);\n \treturn 0;\n }\n \n@@ -624,10 +633,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!strcmp(arg, \"--disk-usage\")) {\n-\t\t\tshow_disk_usage = 1;\n-\t\t\tinfo.flags |= REV_LIST_QUIET;\n-\t\t\tcontinue;\n+\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n+\t\t\tif (*arg == '=') {\n+\t\t\t\tif (!strcmp(++arg, \"human\")) {\n+\t\t\t\t\thuman_readable = 1;\n+\t\t\t\t\tshow_disk_usage = 1;\n+\t\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t} else\n+\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n+\t\t\t} else {\n+\t\t\t\tshow_disk_usage = 1;\n+\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \n \t\tusage(rev_list_usage);\n@@ -752,8 +771,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tprintf(\"%d\\n\", revs.count_left + revs.count_right);\n \t}\n \n-\tif (show_disk_usage)\n-\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n+\tif (show_disk_usage) {\n+\t\tstruct strbuf disk_buf = STRBUF_INIT;\n+\t\tif (human_readable)\n+\t\t\tstrbuf_humanise_bytes(&disk_buf, total_disk_usage);\n+\t\telse\n+\t\t\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)total_disk_usage);\n+\t\tputs(disk_buf.buf);\n+\t\tstrbuf_release(&disk_buf);\n+\t}\n \n cleanup:\n \trelease_revisions(&revs);\ndiff --git a/t/t6115-rev-list-du.sh b/t/t6115-rev-list-du.sh\nindex b4aef32b713..b34841a4ba8 100755\n--- a/t/t6115-rev-list-du.sh\n+++ b/t/t6115-rev-list-du.sh\n@@ -48,4 +48,26 @@ check_du HEAD\n check_du --objects HEAD\n check_du --objects HEAD^..HEAD\n \n+# As mentioned above, don't use hardcode sizes as actual size, but use the\n+# output from git cat-file.\n+test_expect_success 'rev-list --disk-usage=human' '\n+\tgit rev-list --objects HEAD --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list --disk-usage=human with bitmaps' '\n+\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list use --disk-usage unproperly' '\n+\ttest_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: invalid value for '\\''--disk-usage'\\'': '\\''typo'\\'', try --disk-usage=human\n+\tEOF\n+\ttest_cmp err expect\n+'\n+\n test_done\n\nbase-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9\n-- \ngitgitgadget\n"},{"id":"460809","messageId":"202208081735108064497@oschina.cn","threadId":"58270","inReplyTo":"pull.1313.v2.git.1659947722132.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] rev-list: support human-readable output for `--disk-usage`","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2022-08-08T09:37:27Z","receivedAt":"2022-08-08T09:37:52Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":">       static int show_disk_usage;\n>     @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,\n>       int filter_provided_objects)\n>       {\n>       struct bitmap_index *bitmap_git;\n>     -+\tstruct strbuf bitmap_size_buf = STRBUF_INIT;\n>     ++\tstruct strbuf disk_buf = STRBUF_INIT;\n>      +\toff_t size_from_bitmap;\nIn next iteration, will move these two lines to more close to their caller place to\navoid early return.\n>      \n>       if (!show_disk_usage)\n>     @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,\n>      -\tprintf(\"%\"PRIuMAX\"\\n\",\n>      -\t       (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n>      +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n>     -+\tif (human_readable) {\n>     -+\tstrbuf_humanise_bytes(&bitmap_size_buf, size_from_bitmap);\n>     -+\tprintf(\"%s\\n\", bitmap_size_buf.buf);\n>     -+\t} else\n>     -+\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)size_from_bitmap);\n>     -+\tstrbuf_release(&bitmap_size_buf);\n>     ++\tif (human_readable)\n>     ++\tstrbuf_humanise_bytes(&disk_buf, size_from_bitmap);\n>     ++\telse\n>     ++\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)size_from_bitmap);\n>     ++\tputs(disk_buf.buf);\n>     ++\tstrbuf_release(&disk_buf);\n>       return 0;\n>       }\n\n"},{"id":"460917","messageId":"YvJfpNSKMIPqVQmD@coredump.intra.peff.net","threadId":"58270","inReplyTo":"pull.1313.v2.git.1659947722132.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] rev-list: support human-readable output for `--disk-usage`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-09T13:22:44Z","receivedAt":"2022-08-09T13:22:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2022 at 08:35:21AM +0000, Li Linchao via GitGitGadget wrote:\n\n> The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n> (rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\n> This is very useful for people inspect their git repo's objects usage\n> infomation, but the resulting number is quit hard for a human to read.\n> \n> Teach git rev-list to output a human readable result when using\n> '--disk-usage'.\n\nOK. When adding --disk-usage, I never really dreamed people would use it\nfor human output, since \"du .git\" is usually a suitable approximation. :)\nBut I don't have any real objection. I'm curious what your use case is\nlike, if you don't mind sharing. We used it at GitHub for computing\nper-fork sizes for analysis, etc (so the result was always fed into\nanother script).\n\n> Documentation/rev-list-options.txt | 5 +++-\n> builtin/rev-list.c | 42 ++++++++++++++++++++++++------\n> t/t6115-rev-list-du.sh | 22 ++++++++++++++++\n> 3 files changed, 60 insertions(+), 9 deletions(-)\n\nThe patch itself looks pretty sensible (and thanks Ævar for the first\nround of review; the suggestions there all looked good). A few small\ncomments:\n\n> @@ -481,8 +485,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n> \tif (!bitmap_git)\n> \t\treturn -1;\n> \n> -\tprintf(\"%\"PRIuMAX\"\\n\",\n> -\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n> +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n> +\tif (human_readable)\n> +\t\tstrbuf_humanise_bytes(&disk_buf, size_from_bitmap);\n> +\telse\n> +\t\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)size_from_bitmap);\n> +\tputs(disk_buf.buf);\n> +\tstrbuf_release(&disk_buf);\n\nIt's not a lot of duplicated lines, but since it is implementing policy\nlogic, I think it would be nice to move the formatting decision into a\nfunction. Something like:\n\n static void show_disk_usage(off_t size)\n {\n\tstruct strbuf sb = STRBUF_INIT;\n\tif (human_readable)\n\t\tstrbuf_humanise_bytes(&sb, size);\n\telse\n\t\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size_from_bitmap);\n\tputs(sb.buf);\n\tstrbuf_release(&sb);\n }\n\nand then you can call it from here, and from the non-bitmap path below.\n\n(Also, while typing it out, I noticed that you don't need the extra \"\"\nafter PRIuMAX; that just concatenates an empty string).\n\n> -\t\tif (!strcmp(arg, \"--disk-usage\")) {\n> -\t\t\tshow_disk_usage = 1;\n> -\t\t\tinfo.flags |= REV_LIST_QUIET;\n> -\t\t\tcontinue;\n> +\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n> +\t\t\tif (*arg == '=') {\n> +\t\t\t\tif (!strcmp(++arg, \"human\")) {\n> +\t\t\t\t\thuman_readable = 1;\n> +\t\t\t\t\tshow_disk_usage = 1;\n> +\t\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t} else\n> +\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n> +\t\t\t} else {\n> +\t\t\t\tshow_disk_usage = 1;\n> +\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> \t\t}\n\nWe can put the common parts of each side of the conditional into the\nouter block to avoid repeating ourselves. Also, your code matches\n--show-disk-usage-without-an-equals, since it nows uses skip_prefix().\nYou could fix that by checking for '\\0' in *arg. So together, something\nlike:\n\n if (skip_prefix(arg, \"--disk-usage\", &arg)) {\n\tif (*arg == '=') {\n\t\tif (!strcmp(++arg, \"human\"))\n\t\t\thuman_readable = 1;\n\t\telse\n\t\t\tdie(...);\n\t} else if (*arg) {\n\t\t/*\n\t\t * Arguably should goto a label to continue chain of ifs?\n\t\t * Doesn't matter unless we try to add --disk-usage-foo\n\t\t * afterwards\n\t\t */\n\t\tusage(rev_list_usage);\n\t}\n\tshow_disk_usage = 1;\n\tinfo.flags |= REV_LIST_QUIET;\n\tcontinue;\n }\n\n-Peff\n"},{"id":"460922","messageId":"2022081000422739665018@oschina.cn","threadId":"58270","inReplyTo":"YvJfpNSKMIPqVQmD@coredump.intra.peff.net","subject":"Re: Re: [PATCH v2] rev-list: support human-readable output for `--disk-usage`","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2022-08-09T16:46:02Z","receivedAt":"2022-08-09T16:46:19Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":">On Mon, Aug 08, 2022 at 08:35:21AM +0000, Li Linchao via GitGitGadget wrote:\n>\n>> The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n>> (rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\n>> This is very useful for people inspect their git repo's objects usage\n>> infomation, but the resulting number is quit hard for a human to read.\n>>\n>> Teach git rev-list to output a human readable result when using\n>> '--disk-usage'.\n>\n>OK. When adding --disk-usage, I never really dreamed people would use it\n>for human output, since \"du .git\" is usually a suitable approximation. :)\n>But I don't have any real objection. I'm curious what your use case is\n>like, if you don't mind sharing. We used it at GitHub for computing\n>per-fork sizes for analysis, etc (so the result was always fed into\n>another script).\nSometimes, I need objects disk size for analysis too, but I do this by hand\nnot by script. And I also noticed that git-count-objects has an option '-H'\nfor better output, so I think if it could be applied to \"git-rev-list --disk-usage\".\n>\n>>  Documentation/rev-list-options.txt |  5 +++-\n>>  builtin/rev-list.c                 | 42 ++++++++++++++++++++++++------\n>>  t/t6115-rev-list-du.sh             | 22 ++++++++++++++++\n>>  3 files changed, 60 insertions(+), 9 deletions(-)\n>\n>The patch itself looks pretty sensible (and thanks Ævar for the first\n>round of review; the suggestions there all looked good). A few small\n>comments:\n>\n>> @@ -481,8 +485,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n>>  if (!bitmap_git)\n>>  return -1;\n>> \n>> -\tprintf(\"%\"PRIuMAX\"\\n\",\n>> -\t       (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n>> +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n>> +\tif (human_readable)\n>> +\tstrbuf_humanise_bytes(&disk_buf, size_from_bitmap);\n>> +\telse\n>> +\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)size_from_bitmap);\n>> +\tputs(disk_buf.buf);\n>> +\tstrbuf_release(&disk_buf);\n>\n>It's not a lot of duplicated lines, but since it is implementing policy\n>logic, I think it would be nice to move the formatting decision into a\n>function. Something like:\n>\n>  static void show_disk_usage(off_t size)\n>  {\n>\tstruct strbuf sb = STRBUF_INIT;\n>\tif (human_readable)\n>\tstrbuf_humanise_bytes(&sb, size);\n>\telse\n>\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size_from_bitmap);\n>\tputs(sb.buf);\n>\tstrbuf_release(&sb);\n>  }\n>\n>and then you can call it from here, and from the non-bitmap path below.\nOh, this is really good.\n>\n>(Also, while typing it out, I noticed that you don't need the extra \"\"\n>after PRIuMAX; that just concatenates an empty string).\n>\n>> -\tif (!strcmp(arg, \"--disk-usage\")) {\n>> -\tshow_disk_usage = 1;\n>> -\tinfo.flags |= REV_LIST_QUIET;\n>> -\tcontinue;\n>> +\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n>> +\tif (*arg == '=') {\n>> +\tif (!strcmp(++arg, \"human\")) {\n>> +\thuman_readable = 1;\n>> +\tshow_disk_usage = 1;\n>> +\tinfo.flags |= REV_LIST_QUIET;\n>> +\tcontinue;\n>> +\t} else\n>> +\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n>> +\t} else {\n>> +\tshow_disk_usage = 1;\n>> +\tinfo.flags |= REV_LIST_QUIET;\n>> +\tcontinue;\n>> +\t}\n>>  }\n>\n>We can put the common parts of each side of the conditional into the\n>outer block to avoid repeating ourselves. Also, your code matches\n>--show-disk-usage-without-an-equals, since it nows uses skip_prefix().\n>You could fix that by checking for '\\0' in *arg. So together, something\n>like:\n>\n>  if (skip_prefix(arg, \"--disk-usage\", &arg)) {\n>\tif (*arg == '=') {\n>\tif (!strcmp(++arg, \"human\"))\n>\thuman_readable = 1;\n>\telse\n>\tdie(...);\n>\t} else if (*arg) {\n>\t/*\n>\t* Arguably should goto a label to continue chain of ifs?\n>\t* Doesn't matter unless we try to add --disk-usage-foo\n>\t* afterwards\n>\t*/\n>\tusage(rev_list_usage);\n>\t}\n>\tshow_disk_usage = 1;\n>\tinfo.flags |= REV_LIST_QUIET;\n>\tcontinue;\n>  }\nThank you a lot for your very nice suggestions.\n\n>\n>-Peff"},{"id":"460962","messageId":"pull.1313.v3.git.1660111276934.gitgitgadget@gmail.com","threadId":"58270","inReplyTo":"pull.1313.v2.git.1659947722132.gitgitgadget@gmail.com","subject":"[PATCH v3] rev-list: support human-readable output for `--disk-usage`","fromName":"Li Linchao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T06:01:16Z","receivedAt":"2022-08-10T06:02:01Z","isPatch":true,"sender":{"key":"name:Li Linchao","avatar":null},"body":"From: Li Linchao \n\nThe '--disk-usage' option for git-rev-list was introduced in 16950f8384\n(rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\nThis is very useful for people inspect their git repo's objects usage\ninfomation, but the resulting number is quit hard for a human to read.\n\nTeach git rev-list to output a human readable result when using\n'--disk-usage'.\n\nSigned-off-by: Li Linchao \n---\n rev-list: support human-readable output for disk-usage\n \n The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n (rev-list: add --disk-usage option for calculating disk usage,\n 2021-02-09). This is very useful for people inspect their git repo's\n objects usage infomation, but the result number is quit hard for human\n to read.\n \n Teach git rev-list to output more human readable result when using\n '--disk-usage' to calculate objects disk usage.\n \n Signed-off-by: Li Linchao lilinchao@oschina.cn\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1313\n\nRange-diff vs v2:\n\n 1: 7e34d16efe4 ! 1: 000a6b37ec9 rev-list: support human-readable output for `--disk-usage`\n @@ builtin/rev-list.c: static int arg_show_object_names = 1;\n \n static off_t get_object_disk_usage(struct object *obj)\n {\n +@@ builtin/rev-list.c: static int show_object_fast(\n + \treturn 1;\n + }\n + \n ++static void print_disk_usage(off_t size)\n ++{\n ++\tstruct strbuf sb = STRBUF_INIT;\n ++\tif (human_readable)\n ++\t\tstrbuf_humanise_bytes(&sb, size);\n ++\telse\n ++\t\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size);\n ++\tputs(sb.buf);\n ++\tstrbuf_release(&sb);\n ++}\n ++\n + static inline int parse_missing_action_value(const char *value)\n + {\n + \tif (!strcmp(value, \"error\")) {\n @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,\n \t\t\t\t int filter_provided_objects)\n {\n \tstruct bitmap_index *bitmap_git;\n -+\tstruct strbuf disk_buf = STRBUF_INIT;\n +\toff_t size_from_bitmap;\n \n \tif (!show_disk_usage)\n @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,\n -\tprintf(\"%\"PRIuMAX\"\\n\",\n -\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n -+\tif (human_readable)\n -+\t\tstrbuf_humanise_bytes(&disk_buf, size_from_bitmap);\n -+\telse\n -+\t\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)size_from_bitmap);\n -+\tputs(disk_buf.buf);\n -+\tstrbuf_release(&disk_buf);\n ++\tprint_disk_usage(size_from_bitmap);\n \treturn 0;\n }\n \n @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr\n \t\t}\n \n -\t\tif (!strcmp(arg, \"--disk-usage\")) {\n --\t\t\tshow_disk_usage = 1;\n --\t\t\tinfo.flags |= REV_LIST_QUIET;\n --\t\t\tcontinue;\n +\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n +\t\t\tif (*arg == '=') {\n +\t\t\t\tif (!strcmp(++arg, \"human\")) {\n +\t\t\t\t\thuman_readable = 1;\n -+\t\t\t\t\tshow_disk_usage = 1;\n -+\t\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n -+\t\t\t\t\tcontinue;\n +\t\t\t\t} else\n +\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n -+\t\t\t} else {\n -+\t\t\t\tshow_disk_usage = 1;\n -+\t\t\t\tinfo.flags |= REV_LIST_QUIET;\n -+\t\t\t\tcontinue;\n ++\t\t\t} else if (*arg) {\n ++\t\t\t\t/*\n ++\t\t\t\t* Arguably should goto a label to continue chain of ifs?\n ++\t\t\t\t* Doesn't matter unless we try to add --disk-usage-foo\n ++\t\t\t\t* afterwards\n ++\t\t\t\t*/\n ++\t\t\t\tusage(rev_list_usage);\n +\t\t\t}\n - \t\t}\n - \n - \t\tusage(rev_list_usage);\n + \t\t\tshow_disk_usage = 1;\n + \t\t\tinfo.flags |= REV_LIST_QUIET;\n + \t\t\tcontinue;\n @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)\n - \t\t\tprintf(\"%d\\n\", revs.count_left + revs.count_right);\n \t}\n \n --\tif (show_disk_usage)\n + \tif (show_disk_usage)\n -\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n -+\tif (show_disk_usage) {\n -+\t\tstruct strbuf disk_buf = STRBUF_INIT;\n -+\t\tif (human_readable)\n -+\t\t\tstrbuf_humanise_bytes(&disk_buf, total_disk_usage);\n -+\t\telse\n -+\t\t\tstrbuf_addf(&disk_buf, \"%\"PRIuMAX\"\", (uintmax_t)total_disk_usage);\n -+\t\tputs(disk_buf.buf);\n -+\t\tstrbuf_release(&disk_buf);\n -+\t}\n ++\t\tprint_disk_usage(total_disk_usage);\n \n cleanup:\n \trelease_revisions(&revs);\n\n\n Documentation/rev-list-options.txt | 5 ++++-\n builtin/rev-list.c | 35 ++++++++++++++++++++++++++----\n t/t6115-rev-list-du.sh | 22 +++++++++++++++++++\n 3 files changed, 57 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..9966ce4ef91 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -242,8 +242,11 @@ ifdef::git-rev-list[]\n \tto `/dev/null` as the output does not have to be formatted.\n \n --disk-usage::\n+--disk-usage=human::\n \tSuppress normal output; instead, print the sum of the bytes used\n-\tfor on-disk storage by the selected commits or objects. This is\n+\tfor on-disk storage by the selected commits or objects.\n+\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n+\tmeans to print objects size in human readable format. This is\n \tequivalent to piping the output into `git cat-file\n \t--batch-check='%(objectsize:disk)'`, except that it runs much\n \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 30fd8e83eaf..df42e1b667e 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -46,6 +46,7 @@ static const char rev_list_usage[] =\n \" --parents\\n\"\n \" --children\\n\"\n \" --objects | --objects-edge\\n\"\n+\" --disk-usage | --disk-usage=human\\n\"\n \" --unpacked\\n\"\n \" --header | --pretty\\n\"\n \" --[no-]object-names\\n\"\n@@ -81,6 +82,7 @@ static int arg_show_object_names = 1;\n \n static int show_disk_usage;\n static off_t total_disk_usage;\n+static int human_readable;\n \n static off_t get_object_disk_usage(struct object *obj)\n {\n@@ -368,6 +370,17 @@ static int show_object_fast(\n \treturn 1;\n }\n \n+static void print_disk_usage(off_t size)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tif (human_readable)\n+\t\tstrbuf_humanise_bytes(&sb, size);\n+\telse\n+\t\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size);\n+\tputs(sb.buf);\n+\tstrbuf_release(&sb);\n+}\n+\n static inline int parse_missing_action_value(const char *value)\n {\n \tif (!strcmp(value, \"error\")) {\n@@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \t\t\t\t int filter_provided_objects)\n {\n \tstruct bitmap_index *bitmap_git;\n+\toff_t size_from_bitmap;\n \n \tif (!show_disk_usage)\n \t\treturn -1;\n@@ -481,8 +495,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \tif (!bitmap_git)\n \t\treturn -1;\n \n-\tprintf(\"%\"PRIuMAX\"\\n\",\n-\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n+\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n+\tprint_disk_usage(size_from_bitmap);\n \treturn 0;\n }\n \n@@ -624,7 +638,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!strcmp(arg, \"--disk-usage\")) {\n+\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n+\t\t\tif (*arg == '=') {\n+\t\t\t\tif (!strcmp(++arg, \"human\")) {\n+\t\t\t\t\thuman_readable = 1;\n+\t\t\t\t} else\n+\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n+\t\t\t} else if (*arg) {\n+\t\t\t\t/*\n+\t\t\t\t* Arguably should goto a label to continue chain of ifs?\n+\t\t\t\t* Doesn't matter unless we try to add --disk-usage-foo\n+\t\t\t\t* afterwards\n+\t\t\t\t*/\n+\t\t\t\tusage(rev_list_usage);\n+\t\t\t}\n \t\t\tshow_disk_usage = 1;\n \t\t\tinfo.flags |= REV_LIST_QUIET;\n \t\t\tcontinue;\n@@ -753,7 +780,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (show_disk_usage)\n-\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n+\t\tprint_disk_usage(total_disk_usage);\n \n cleanup:\n \trelease_revisions(&revs);\ndiff --git a/t/t6115-rev-list-du.sh b/t/t6115-rev-list-du.sh\nindex b4aef32b713..b34841a4ba8 100755\n--- a/t/t6115-rev-list-du.sh\n+++ b/t/t6115-rev-list-du.sh\n@@ -48,4 +48,26 @@ check_du HEAD\n check_du --objects HEAD\n check_du --objects HEAD^..HEAD\n \n+# As mentioned above, don't use hardcode sizes as actual size, but use the\n+# output from git cat-file.\n+test_expect_success 'rev-list --disk-usage=human' '\n+\tgit rev-list --objects HEAD --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list --disk-usage=human with bitmaps' '\n+\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list use --disk-usage unproperly' '\n+\ttest_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: invalid value for '\\''--disk-usage'\\'': '\\''typo'\\'', try --disk-usage=human\n+\tEOF\n+\ttest_cmp err expect\n+'\n+\n test_done\n\nbase-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9\n-- \ngitgitgadget\n"},{"id":"460963","messageId":"4e54c11d-d01d-7777-89e7-8799180c2931@kdbg.org","threadId":"58270","inReplyTo":"pull.1313.v3.git.1660111276934.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] rev-list: support human-readable output for `--disk-usage`","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-08-10T07:18:52Z","receivedAt":"2022-08-10T07:19:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.08.22 um 08:01 schrieb Li Linchao via GitGitGadget:\n> --disk-usage::\n> +--disk-usage=human::\n> \tSuppress normal output; instead, print the sum of the bytes used\n> -\tfor on-disk storage by the selected commits or objects. This is\n> +\tfor on-disk storage by the selected commits or objects.\n> +\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n> +\tmeans to print objects size in human readable format. This is\n> \tequivalent to piping the output into `git cat-file\n> \t--batch-check='%(objectsize:disk)'`, except that it runs much\n> \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n\nThe original paragraph flows very well and explains what the option does\nand how it computes the result. Please do not interrupt the flow of the\ntext with a whole sentence that should be just a parenthetical remark,\nbut add a sentence at the end, not in the middle.\n\nYou added a new feature, and I understand that it is important *to you*.\nBut do make it a habit to ask yourself if it is also important for the\ngeneral audience. Generally, a new feature is not as important as\nexisting features, otherwise, it would have been added earlier, wouldn't it?\n\n-- Hannes\n"},{"id":"460976","messageId":"pull.1313.v4.git.1660130072657.gitgitgadget@gmail.com","threadId":"58270","inReplyTo":"pull.1313.v3.git.1660111276934.gitgitgadget@gmail.com","subject":"[PATCH v4] rev-list: support human-readable output for `--disk-usage`","fromName":"Li Linchao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-10T11:14:32Z","receivedAt":"2022-08-10T11:14:41Z","isPatch":true,"sender":{"key":"name:Li Linchao","avatar":null},"body":"From: Li Linchao \n\nThe '--disk-usage' option for git-rev-list was introduced in 16950f8384\n(rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\nThis is very useful for people inspect their git repo's objects usage\ninfomation, but the resulting number is quit hard for a human to read.\n\nTeach git rev-list to output a human readable result when using\n'--disk-usage'.\n\nSigned-off-by: Li Linchao \n---\n rev-list: support human-readable output for disk-usage\n \n The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n (rev-list: add --disk-usage option for calculating disk usage,\n 2021-02-09). This is very useful for people inspect their git repo's\n objects usage infomation, but the result number is quit hard for human\n to read.\n \n Teach git rev-list to output more human readable result when using\n '--disk-usage' to calculate objects disk usage.\n \n Signed-off-by: Li Linchao lilinchao@oschina.cn\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1313\n\nRange-diff vs v3:\n\n 1: 000a6b37ec9 ! 1: e56da057a9a rev-list: support human-readable output for `--disk-usage`\n @@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]\n --disk-usage::\n +--disk-usage=human::\n \tSuppress normal output; instead, print the sum of the bytes used\n --\tfor on-disk storage by the selected commits or objects. This is\n -+\tfor on-disk storage by the selected commits or objects.\n -+\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n -+\tmeans to print objects size in human readable format. This is\n + \tfor on-disk storage by the selected commits or objects. This is\n \tequivalent to piping the output into `git cat-file\n - \t--batch-check='%(objectsize:disk)'`, except that it runs much\n +@@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]\n \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n + \tsection in linkgit:git-cat-file[1] for the limitations of what\n + \t\"on-disk storage\" means.\n ++\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n ++\tmeans to print objects size in human readable format.\n + endif::git-rev-list[]\n + \n + --cherry-mark::\n \n ## builtin/rev-list.c ##\n @@ builtin/rev-list.c: static const char rev_list_usage[] =\n\n\n Documentation/rev-list-options.txt | 3 +++\n builtin/rev-list.c | 35 ++++++++++++++++++++++++++----\n t/t6115-rev-list-du.sh | 22 +++++++++++++++++++\n 3 files changed, 56 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..5d3880874fc 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -242,6 +242,7 @@ ifdef::git-rev-list[]\n \tto `/dev/null` as the output does not have to be formatted.\n \n --disk-usage::\n+--disk-usage=human::\n \tSuppress normal output; instead, print the sum of the bytes used\n \tfor on-disk storage by the selected commits or objects. This is\n \tequivalent to piping the output into `git cat-file\n@@ -249,6 +250,8 @@ ifdef::git-rev-list[]\n \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n \tsection in linkgit:git-cat-file[1] for the limitations of what\n \t\"on-disk storage\" means.\n+\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n+\tmeans to print objects size in human readable format.\n endif::git-rev-list[]\n \n --cherry-mark::\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 30fd8e83eaf..df42e1b667e 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -46,6 +46,7 @@ static const char rev_list_usage[] =\n \" --parents\\n\"\n \" --children\\n\"\n \" --objects | --objects-edge\\n\"\n+\" --disk-usage | --disk-usage=human\\n\"\n \" --unpacked\\n\"\n \" --header | --pretty\\n\"\n \" --[no-]object-names\\n\"\n@@ -81,6 +82,7 @@ static int arg_show_object_names = 1;\n \n static int show_disk_usage;\n static off_t total_disk_usage;\n+static int human_readable;\n \n static off_t get_object_disk_usage(struct object *obj)\n {\n@@ -368,6 +370,17 @@ static int show_object_fast(\n \treturn 1;\n }\n \n+static void print_disk_usage(off_t size)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tif (human_readable)\n+\t\tstrbuf_humanise_bytes(&sb, size);\n+\telse\n+\t\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size);\n+\tputs(sb.buf);\n+\tstrbuf_release(&sb);\n+}\n+\n static inline int parse_missing_action_value(const char *value)\n {\n \tif (!strcmp(value, \"error\")) {\n@@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \t\t\t\t int filter_provided_objects)\n {\n \tstruct bitmap_index *bitmap_git;\n+\toff_t size_from_bitmap;\n \n \tif (!show_disk_usage)\n \t\treturn -1;\n@@ -481,8 +495,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \tif (!bitmap_git)\n \t\treturn -1;\n \n-\tprintf(\"%\"PRIuMAX\"\\n\",\n-\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n+\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n+\tprint_disk_usage(size_from_bitmap);\n \treturn 0;\n }\n \n@@ -624,7 +638,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!strcmp(arg, \"--disk-usage\")) {\n+\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n+\t\t\tif (*arg == '=') {\n+\t\t\t\tif (!strcmp(++arg, \"human\")) {\n+\t\t\t\t\thuman_readable = 1;\n+\t\t\t\t} else\n+\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n+\t\t\t} else if (*arg) {\n+\t\t\t\t/*\n+\t\t\t\t* Arguably should goto a label to continue chain of ifs?\n+\t\t\t\t* Doesn't matter unless we try to add --disk-usage-foo\n+\t\t\t\t* afterwards\n+\t\t\t\t*/\n+\t\t\t\tusage(rev_list_usage);\n+\t\t\t}\n \t\t\tshow_disk_usage = 1;\n \t\t\tinfo.flags |= REV_LIST_QUIET;\n \t\t\tcontinue;\n@@ -753,7 +780,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (show_disk_usage)\n-\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n+\t\tprint_disk_usage(total_disk_usage);\n \n cleanup:\n \trelease_revisions(&revs);\ndiff --git a/t/t6115-rev-list-du.sh b/t/t6115-rev-list-du.sh\nindex b4aef32b713..b34841a4ba8 100755\n--- a/t/t6115-rev-list-du.sh\n+++ b/t/t6115-rev-list-du.sh\n@@ -48,4 +48,26 @@ check_du HEAD\n check_du --objects HEAD\n check_du --objects HEAD^..HEAD\n \n+# As mentioned above, don't use hardcode sizes as actual size, but use the\n+# output from git cat-file.\n+test_expect_success 'rev-list --disk-usage=human' '\n+\tgit rev-list --objects HEAD --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list --disk-usage=human with bitmaps' '\n+\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list use --disk-usage unproperly' '\n+\ttest_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: invalid value for '\\''--disk-usage'\\'': '\\''typo'\\'', try --disk-usage=human\n+\tEOF\n+\ttest_cmp err expect\n+'\n+\n test_done\n\nbase-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9\n-- \ngitgitgadget\n"},{"id":"461008","messageId":"xmqqlerwm28n.fsf@gitster.g","threadId":"58270","inReplyTo":"pull.1313.v4.git.1660130072657.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T17:34:32Z","receivedAt":"2022-08-10T17:34:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Li Linchao via GitGitGadget\" writes:\n\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 195e74eec63..5d3880874fc 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -242,6 +242,7 @@ ifdef::git-rev-list[]\n> \tto `/dev/null` as the output does not have to be formatted.\n> \n> --disk-usage::\n> +--disk-usage=human::\n> \tSuppress normal output; instead, print the sum of the bytes used\n> \tfor on-disk storage by the selected commits or objects. This is\n> \tequivalent to piping the output into `git cat-file\n> @@ -249,6 +250,8 @@ ifdef::git-rev-list[]\n> \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n> \tsection in linkgit:git-cat-file[1] for the limitations of what\n> \t\"on-disk storage\" means.\n> +\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n> +\tmeans to print objects size in human readable format.\n\n\"When it accepts\" sounds wrong, because it implies there are two\ncases, i.e. the user gives \"--disk-usage=human\" and the command\nsomehow decides to accept it, or to reject it, which is not what is\ngoing on. How about phrasing it more like ...\n\n With the optional value `human`, on-disk storage size is shown\n in human-readable string (e.g. 12.23 KiB, 3.50 MiB).\n\nperhaps?\n\n> @@ -46,6 +46,7 @@ static const char rev_list_usage[] =\n> \" --parents\\n\"\n> \" --children\\n\"\n> \" --objects | --objects-edge\\n\"\n> +\" --disk-usage | --disk-usage=human\\n\"\n\nWriting it like\n\n\t\"--disk-usage[=human]\\n\"\n\nwould be more in line with ...\n\n> \" --[no-]object-names\\n\"\n\n... this one.\n\n> @@ -368,6 +370,17 @@ static int show_object_fast(\n> \treturn 1;\n> }\n> \n> +static void print_disk_usage(off_t size)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tif (human_readable)\n> +\t\tstrbuf_humanise_bytes(&sb, size);\n> +\telse\n> +\t\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size);\n> +\tputs(sb.buf);\n> +\tstrbuf_release(&sb);\n> +}\n\nHmph, I am not sure if we want to make it a helper like this. The\nnormal case does not need to prepare the string into a strbuf but\njust can send the output to the standard output stream.\n\nIt is probably easy to fix, like so:\n\n\tif (!human_readable) {\n\t\tprintf(\"%\" PRIuMAX \"\\n\", disk_usage);\n\t} else {\n\t\tstrbuf sb = STRBUF_INIT;\n\t\tstrbuf_humanise_bytes(&sb, disk_usage);\n\t\tputs(sb.buf);\n\t\tstrbuf_release(&sb);\n\t}\n\n> static inline int parse_missing_action_value(const char *value)\n> {\n> \tif (!strcmp(value, \"error\")) {\n> @@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n> \t\t\t\t int filter_provided_objects)\n> {\n> \tstruct bitmap_index *bitmap_git;\n> +\toff_t size_from_bitmap;\n\nQuestionably named variable.\n\n> \tif (!show_disk_usage)\n> \t\treturn -1;\n> @@ -481,8 +495,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n> \tif (!bitmap_git)\n> \t\treturn -1;\n> \n> -\tprintf(\"%\"PRIuMAX\"\\n\",\n> -\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n> +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n> +\tprint_disk_usage(size_from_bitmap);\n\nIt makes sense to make the function declare how it gets disk usage\nin its name, but once we call the function to get what we want,\nthere is no need to keep saying we got it from bitmap. If we ever\ngained another function that obtains the disk usage from other\nmeans, then this part of the code would become\n\n\tif (use_bitmap)\n\t\tvariable = get_disk_usage_from_bitmap(...);\n\telse\n\t\tvariable = get_disk_usage_from_other_means(...);\n\n\tprint_disk_usage(variable);\n\nNow, what should that variable be named? It is clear that it\nshouldn't be named \"from_bitmap\" at all, because it may very well\nhave come from somewhere else.\n\nYou call it simply 'size' in print_disk_usage(), and that would\nprobably be a good name to use here. Or even better, \"disk_usage\".\n\nHowever, I think all of the above badness stems from the horrible\nway the existing code deals with \"use_bitmap_index\", and it is not\nin the scope of this patch to fix it, let's keep the above code\nas-is. This is only called from the \"if (use_bitmap_index)\" so\nthe size can only be from bitmap.\n\n> @@ -624,7 +638,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tif (!strcmp(arg, \"--disk-usage\")) {\n> +\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n> +\t\t\tif (*arg == '=') {\n> +\t\t\t\tif (!strcmp(++arg, \"human\")) {\n> +\t\t\t\t\thuman_readable = 1;\n> +\t\t\t\t} else\n> +\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n\nThis makes the hardcoded \"--disk-usage=human\" subject to\ntranslation, which it should not. Perhaps like this:\n\n\tdie(_(\"invalid value for '%s': '%s', the only allowed format is \"%s\"),\n\t \"--disk-usage=\", arg, \"human\");\n\nThe main point is to ensure that \"human\" that we are not allowing\nthe user to type in their language is left out of the\n_(\"translatable string\").\n\n> +\t\t\t} else if (*arg) {\n> +\t\t\t\t/*\n> +\t\t\t\t* Arguably should goto a label to continue chain of ifs?\n> +\t\t\t\t* Doesn't matter unless we try to add --disk-usage-foo\n> +\t\t\t\t* afterwards\n> +\t\t\t\t*/\n\nWrong indentation for a long comment?\n\n> +\t\t\t\tusage(rev_list_usage);\n> +\t\t\t}\n"},{"id":"461033","messageId":"YvQhHOkjZatIqlFr@coredump.intra.peff.net","threadId":"58270","inReplyTo":"xmqqlerwm28n.fsf@gitster.g","subject":"Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-10T21:20:28Z","receivedAt":"2022-08-10T21:20:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 10:34:32AM -0700, Junio C Hamano wrote:\n\n> > +static void print_disk_usage(off_t size)\n> > +{\n> > +\tstruct strbuf sb = STRBUF_INIT;\n> > +\tif (human_readable)\n> > +\t\tstrbuf_humanise_bytes(&sb, size);\n> > +\telse\n> > +\t\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size);\n> > +\tputs(sb.buf);\n> > +\tstrbuf_release(&sb);\n> > +}\n> \n> Hmph, I am not sure if we want to make it a helper like this. The\n> normal case does not need to prepare the string into a strbuf but\n> just can send the output to the standard output stream.\n> \n> It is probably easy to fix, like so:\n> \n> \tif (!human_readable) {\n> \t\tprintf(\"%\" PRIuMAX \"\\n\", disk_usage);\n> \t} else {\n> \t\tstrbuf sb = STRBUF_INIT;\n> \t\tstrbuf_humanise_bytes(&sb, disk_usage);\n> \t\tputs(sb.buf);\n> \t\tstrbuf_release(&sb);\n> \t}\n\nIt was my suggestion to turn it into a helper, because the same code\nneeds to be present in two distant spots (the bitmap and non-bitmap\ncases).\n\nI don't care much between \"printf directly vs strbuf\" for the non-human\ncase, but it was an earlier review suggestion to connect them. I do\nthink the result is a little easier to follow, but mostly I want to make\nit clear that the author is getting stuck between warring review\ncomments here. ;)\n\n> > @@ -481,8 +495,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n> > \tif (!bitmap_git)\n> > \t\treturn -1;\n> > \n> > -\tprintf(\"%\"PRIuMAX\"\\n\",\n> > -\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n> > +\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n> > +\tprint_disk_usage(size_from_bitmap);\n> \n> It makes sense to make the function declare how it gets disk usage\n> in its name, but once we call the function to get what we want,\n> there is no need to keep saying we got it from bitmap. If we ever\n> gained another function that obtains the disk usage from other\n> means, then this part of the code would become\n\nKeep in mind that we are in try_bitmap_disk_usage() here. :) There is\nindeed similar code to use other means, but it's far away, and this code\nwill always use bitmaps.\n\nThat said, I'd have just written:\n\n print_disk_usage(get_disk_usage_from_bitmap(bitmap_git, revs));\n\nsince the variable is not otherwise used. But arguably that's harder to\nread.\n\n-Peff\n"},{"id":"461034","messageId":"xmqqbksrkcz8.fsf@gitster.g","threadId":"58270","inReplyTo":"YvQhHOkjZatIqlFr@coredump.intra.peff.net","subject":"Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-10T21:25:31Z","receivedAt":"2022-08-10T21:25:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It was my suggestion to turn it into a helper, because the same code\n> needs to be present in two distant spots (the bitmap and non-bitmap\n> cases).\n> ...\n> Keep in mind that we are in try_bitmap_disk_usage() here.\n\nYeah, both of them I notice in a later parts of the message you are\nresponding to ;-)\n"},{"id":"461057","messageId":"pull.1313.v5.git.1660193274336.gitgitgadget@gmail.com","threadId":"58270","inReplyTo":"pull.1313.v4.git.1660130072657.gitgitgadget@gmail.com","subject":"[PATCH v5] rev-list: support human-readable output for `--disk-usage`","fromName":"Li Linchao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-11T04:47:54Z","receivedAt":"2022-08-11T04:48:02Z","isPatch":true,"sender":{"key":"name:Li Linchao","avatar":null},"body":"From: Li Linchao \n\nThe '--disk-usage' option for git-rev-list was introduced in 16950f8384\n(rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\nThis is very useful for people inspect their git repo's objects usage\ninfomation, but the resulting number is quit hard for a human to read.\n\nTeach git rev-list to output a human readable result when using\n'--disk-usage'.\n\nSigned-off-by: Li Linchao \n---\n rev-list: support human-readable output for disk-usage\n \n Signed-off-by: Li Linchao lilinchao@oschina.cn Cc: Jeff King\n peff@peff.net cc: Ævar Arnfjörð Bjarmason avarab@gmail.com cc: Johannes\n Sixt j6t@kdbg.org\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1313\n\nRange-diff vs v4:\n\n 1: e56da057a9a ! 1: 7fbf4b636d6 rev-list: support human-readable output for `--disk-usage`\n @@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]\n \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n \tsection in linkgit:git-cat-file[1] for the limitations of what\n \t\"on-disk storage\" means.\n -+\tWhen it accepts a value `human`, like: `--disk-usage=human`, this\n -+\tmeans to print objects size in human readable format.\n ++\tWith the optional value `human`, on-disk storage size is shown\n ++\tin human-readable string(e.g. 12.24 Kib, 3.50 Mib).\n endif::git-rev-list[]\n \n --cherry-mark::\n @@ builtin/rev-list.c: static const char rev_list_usage[] =\n \" --parents\\n\"\n \" --children\\n\"\n \" --objects | --objects-edge\\n\"\n -+\" --disk-usage | --disk-usage=human\\n\"\n ++\" --disk-usage[=human]\\n\"\n \" --unpacked\\n\"\n \" --header | --pretty\\n\"\n \" --[no-]object-names\\n\"\n @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr\n +\t\t\t\tif (!strcmp(++arg, \"human\")) {\n +\t\t\t\t\thuman_readable = 1;\n +\t\t\t\t} else\n -+\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', try --disk-usage=human\"), \"--disk-usage\", arg);\n ++\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', the only allowed format is '%s'\"),\n ++\t\t\t\t\t \"--disk-usage=\", arg, \"human\");\n +\t\t\t} else if (*arg) {\n +\t\t\t\t/*\n -+\t\t\t\t* Arguably should goto a label to continue chain of ifs?\n -+\t\t\t\t* Doesn't matter unless we try to add --disk-usage-foo\n -+\t\t\t\t* afterwards\n -+\t\t\t\t*/\n ++\t\t\t\t * Arguably should goto a label to continue chain of ifs?\n ++\t\t\t\t * Doesn't matter unless we try to add --disk-usage-foo\n ++\t\t\t\t * afterwards.\n ++\t\t\t\t */\n +\t\t\t\tusage(rev_list_usage);\n +\t\t\t}\n \t\t\tshow_disk_usage = 1;\n @@ t/t6115-rev-list-du.sh: check_du HEAD\n +test_expect_success 'rev-list use --disk-usage unproperly' '\n +\ttest_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&\n +\tcat >expect <<-\\EOF &&\n -+\tfatal: invalid value for '\\''--disk-usage'\\'': '\\''typo'\\'', try --disk-usage=human\n ++\tfatal: invalid value for '\\''--disk-usage='\\'': '\\''typo'\\'', the only allowed format is '\\''human'\\''\n +\tEOF\n +\ttest_cmp err expect\n +'\n\n\n Documentation/rev-list-options.txt | 3 +++\n builtin/rev-list.c | 36 ++++++++++++++++++++++++++----\n t/t6115-rev-list-du.sh | 22 ++++++++++++++++++\n 3 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..bd08d18576f 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -242,6 +242,7 @@ ifdef::git-rev-list[]\n \tto `/dev/null` as the output does not have to be formatted.\n \n --disk-usage::\n+--disk-usage=human::\n \tSuppress normal output; instead, print the sum of the bytes used\n \tfor on-disk storage by the selected commits or objects. This is\n \tequivalent to piping the output into `git cat-file\n@@ -249,6 +250,8 @@ ifdef::git-rev-list[]\n \tfaster (especially with `--use-bitmap-index`). See the `CAVEATS`\n \tsection in linkgit:git-cat-file[1] for the limitations of what\n \t\"on-disk storage\" means.\n+\tWith the optional value `human`, on-disk storage size is shown\n+\tin human-readable string(e.g. 12.24 Kib, 3.50 Mib).\n endif::git-rev-list[]\n \n --cherry-mark::\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 30fd8e83eaf..fba6f5d51f3 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -46,6 +46,7 @@ static const char rev_list_usage[] =\n \" --parents\\n\"\n \" --children\\n\"\n \" --objects | --objects-edge\\n\"\n+\" --disk-usage[=human]\\n\"\n \" --unpacked\\n\"\n \" --header | --pretty\\n\"\n \" --[no-]object-names\\n\"\n@@ -81,6 +82,7 @@ static int arg_show_object_names = 1;\n \n static int show_disk_usage;\n static off_t total_disk_usage;\n+static int human_readable;\n \n static off_t get_object_disk_usage(struct object *obj)\n {\n@@ -368,6 +370,17 @@ static int show_object_fast(\n \treturn 1;\n }\n \n+static void print_disk_usage(off_t size)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tif (human_readable)\n+\t\tstrbuf_humanise_bytes(&sb, size);\n+\telse\n+\t\tstrbuf_addf(&sb, \"%\"PRIuMAX, (uintmax_t)size);\n+\tputs(sb.buf);\n+\tstrbuf_release(&sb);\n+}\n+\n static inline int parse_missing_action_value(const char *value)\n {\n \tif (!strcmp(value, \"error\")) {\n@@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \t\t\t\t int filter_provided_objects)\n {\n \tstruct bitmap_index *bitmap_git;\n+\toff_t size_from_bitmap;\n \n \tif (!show_disk_usage)\n \t\treturn -1;\n@@ -481,8 +495,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,\n \tif (!bitmap_git)\n \t\treturn -1;\n \n-\tprintf(\"%\"PRIuMAX\"\\n\",\n-\t (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));\n+\tsize_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);\n+\tprint_disk_usage(size_from_bitmap);\n \treturn 0;\n }\n \n@@ -624,7 +638,21 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!strcmp(arg, \"--disk-usage\")) {\n+\t\tif (skip_prefix(arg, \"--disk-usage\", &arg)) {\n+\t\t\tif (*arg == '=') {\n+\t\t\t\tif (!strcmp(++arg, \"human\")) {\n+\t\t\t\t\thuman_readable = 1;\n+\t\t\t\t} else\n+\t\t\t\t\tdie(_(\"invalid value for '%s': '%s', the only allowed format is '%s'\"),\n+\t\t\t\t\t \"--disk-usage=\", arg, \"human\");\n+\t\t\t} else if (*arg) {\n+\t\t\t\t/*\n+\t\t\t\t * Arguably should goto a label to continue chain of ifs?\n+\t\t\t\t * Doesn't matter unless we try to add --disk-usage-foo\n+\t\t\t\t * afterwards.\n+\t\t\t\t */\n+\t\t\t\tusage(rev_list_usage);\n+\t\t\t}\n \t\t\tshow_disk_usage = 1;\n \t\t\tinfo.flags |= REV_LIST_QUIET;\n \t\t\tcontinue;\n@@ -753,7 +781,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (show_disk_usage)\n-\t\tprintf(\"%\"PRIuMAX\"\\n\", (uintmax_t)total_disk_usage);\n+\t\tprint_disk_usage(total_disk_usage);\n \n cleanup:\n \trelease_revisions(&revs);\ndiff --git a/t/t6115-rev-list-du.sh b/t/t6115-rev-list-du.sh\nindex b4aef32b713..d59111dedec 100755\n--- a/t/t6115-rev-list-du.sh\n+++ b/t/t6115-rev-list-du.sh\n@@ -48,4 +48,26 @@ check_du HEAD\n check_du --objects HEAD\n check_du --objects HEAD^..HEAD\n \n+# As mentioned above, don't use hardcode sizes as actual size, but use the\n+# output from git cat-file.\n+test_expect_success 'rev-list --disk-usage=human' '\n+\tgit rev-list --objects HEAD --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list --disk-usage=human with bitmaps' '\n+\tgit rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&\n+\tdisk_usage_slow --objects HEAD >actual_size &&\n+\tgrep \"$(cat actual_size) bytes\" actual\n+'\n+\n+test_expect_success 'rev-list use --disk-usage unproperly' '\n+\ttest_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: invalid value for '\\''--disk-usage='\\'': '\\''typo'\\'', the only allowed format is '\\''human'\\''\n+\tEOF\n+\ttest_cmp err expect\n+'\n+\n test_done\n\nbase-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9\n-- \ngitgitgadget\n"},{"id":"461058","messageId":"xmqqy1vvgxv5.fsf@gitster.g","threadId":"58270","inReplyTo":"YvQhHOkjZatIqlFr@coredump.intra.peff.net","subject":"Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-11T05:20:14Z","receivedAt":"2022-08-11T05:20:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't care much between \"printf directly vs strbuf\" for the non-human\n> case, but it was an earlier review suggestion to connect them. I do\n> think the result is a little easier to follow, but mostly I want to make\n> it clear that the author is getting stuck between warring review\n> comments here. ;)\n\nThe function does look like it can later be called millions of\ntimes, at which time we may want to measure, but as the only use of\nthis helper function is to get called just once at the end with a\nsingle number and print it (as opposed to getting called once per\neach object), I do not care too much about the normal case being\nmade unnecessarily more expensive, either.\n\nI still do not see the point of changing it to print to a strbuf and\nputs() the result, though. It does not make the code shorter, more\nefficient, or more readable. Once \"if (we are producing humanize\nformat)\" condition is hit, both of its branches can either be (1)\nresponsible to print the number to the standard output stream, using\nwhatever implementation, or (2) responsible to print the number to a\nstrbuf, so that somebody outside the if statement will be\nrespohnsible for printing that string to the standard output stream.\n\nThe patch chooses (2), which is more complex, for no good reason. A\ngood thing about (1) is that the non-human codepath can STAY to be\nthe same as before, which is one fewer chance to introduce\nunnecessary bugs.\n\nAnyway...\n"},{"id":"461062","messageId":"YvTACxGqVPBa+IDx@coredump.intra.peff.net","threadId":"58270","inReplyTo":"xmqqy1vvgxv5.fsf@gitster.g","subject":"Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-08-11T08:38:35Z","receivedAt":"2022-08-11T08:38:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 10, 2022 at 10:20:14PM -0700, Junio C Hamano wrote:\n\n> I still do not see the point of changing it to print to a strbuf and\n> puts() the result, though. It does not make the code shorter, more\n> efficient, or more readable. Once \"if (we are producing humanize\n> format)\" condition is hit, both of its branches can either be (1)\n> responsible to print the number to the standard output stream, using\n> whatever implementation, or (2) responsible to print the number to a\n> strbuf, so that somebody outside the if statement will be\n> respohnsible for printing that string to the standard output stream.\n\nI was not the original reviewer who suggested that change, but FWIW, I\nthink the argument for cases like these is something like this. Between:\n\n if (some_cond) {\n foo(do_some_prep());\n } else {\n foo(do_some_other_prep());\n }\n\nand:\n\n if (some_cond) {\n x = do_some_prep();\n } else {\n x = do_some_other_prep();\n }\n foo(x);\n\nthe latter makes it more clear that foo() is called on both sides of the\nconditional. In this case the \"foo\" is not exactly the same: on one it\nis printing a strbuf, and on the other it is a printf that\ndirect-formats. But the principle is the same, if you want to make it\nclear that both sides will result in printing something.\n\nAs you note, it loses the possibility for one side of the conditional to\ndo its \"foo\" in a more efficient way, but I don't think that's very\nimportant for this particular call site.\n\n> The patch chooses (2), which is more complex, for no good reason. A\n> good thing about (1) is that the non-human codepath can STAY to be\n> the same as before, which is one fewer chance to introduce\n> unnecessary bugs.\n\nTrue.\n\nAgain, I don't care much either way. But I am not quite on the \"I do not\nsee the point at all\" side.\n\n-Peff\n"},{"id":"461100","messageId":"xmqqczd6ec9b.fsf@gitster.g","threadId":"58270","inReplyTo":"pull.1313.v5.git.1660193274336.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] rev-list: support human-readable output for `--disk-usage`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-11T20:49:52Z","receivedAt":"2022-08-11T20:49:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Li Linchao via GitGitGadget\" writes:\n\n> From: Li Linchao \n>\n> The '--disk-usage' option for git-rev-list was introduced in 16950f8384\n> (rev-list: add --disk-usage option for calculating disk usage, 2021-02-09).\n> This is very useful for people inspect their git repo's objects usage\n> infomation, but the resulting number is quit hard for a human to read.\n>\n> Teach git rev-list to output a human readable result when using\n> '--disk-usage'.\n>\n> Signed-off-by: Li Linchao \n> ---\n\nThanks, queued.\n"}]}