Skip to contentSkip to conversation

rev-list: support `--human-readable` option when applied `disk-usage`

PATCH In review Li Linchao via GitGitGadget 6 participants 17 messages Aug 5, 2022 – Aug 11, 2022 Documentation

lore
Share & tools
lore

Li Linchao via GitGitGadget

author
#0 lore raw

PATCH 1/1 rev-list: support `--human-readable` option when applied `disk-usage`

From: Li Linchao <[email protected]>

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.

Teach git rev-list to output more human readable result when using '--disk-usage' to calculate objects disk usage.

Signed-off-by Li Linchao <[email protected]>

---
rev-list: support --human-readable option when applied disk-usage

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.

Teach git rev-list to output more human readable result when using '--disk-usage' to calculate objects disk usage.

Signed-off-by Li Linchao [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1313

 Documentation/rev-list-options.txt |  5 +++++
 builtin/rev-list.c                 | 31 ++++++++++++++++++++++++++----
 t/t6115-rev-list-du.sh             | 18 +++++++++++++++++
 3 files changed, 50 insertions(+), 4 deletions(-)
Documentation/rev-list-options.txt +5 -0
index 195e74eec63..d30301a9159 100644
@@ -249,6 +249,11 @@ ifdef::git-rev-list[]
 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
 	section in linkgit:git-cat-file[1] for the limitations of what
 	"on-disk storage" means.
+
+-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[]
 
 --cherry-mark::
builtin/rev-list.c +27 -4
index 30fd8e83eaf..be677f29070 100644
@@ -81,6 +81,7 @@ static int arg_show_object_names = 1;
 
 static int show_disk_usage;
 static off_t total_disk_usage;
+static int human_readable;
 
 static off_t get_object_disk_usage(struct object *obj)
 {

@@ -473,6 +474,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
 				 int filter_provided_objects)
 {
 	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);
 	return 0;
 }
 

@@ -490,6 +498,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct rev_list_info info;
+	struct strbuf disk_buf = STRBUF_INIT;
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};

@@ -630,9 +639,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
+		if (!strcmp(arg, "--human-readable") || !strcmp(arg, "-H")) {
+			human_readable = 1;
+			continue;
+		}
+
 		usage(rev_list_usage);
 
 	}
+
+	if (!show_disk_usage && human_readable)
+		die(_("option '%s' should be used with '%s' together"), "--human-readable/-H", "--disk-usage");
 	if (revs.commit_format != CMIT_FMT_USERFORMAT)
 		revs.include_header = 1;
 	if (revs.commit_format != CMIT_FMT_UNSPECIFIED) {

@@ -752,10 +769,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			printf("%d\n", revs.count_left + revs.count_right);
 	}
 
-	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);
+	}
 
 cleanup:
 	release_revisions(&revs);
+	strbuf_release(&disk_buf);
 	return ret;
 }
t/t6115-rev-list-du.sh +18 -0
index b4aef32b713..614ebb72aaa 100755
@@ -48,4 +48,22 @@ check_du HEAD
 check_du --objects HEAD
 check_du --objects HEAD^..HEAD
 
+
+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
+'
+
+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
+'
+
+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 &&
+	test_cmp err expect
+'
+
 test_done

base-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 6 lines · Li Linchao via GitGitGadget

From: Li Linchao <[email protected]>

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/

Quoted 2 lines

Teach git rev-list to output more human readable result when using

s/to output more human/to output a human/

Quoted 1 line

'--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...

Documentation/rev-list-options.txt ifdef::git-rev-list[] · line 257

Quoted 6 lines
Documentation/rev-list-options.txt +5 -0
+
+-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?

Quoted 9 lines
builtin/rev-list.c +2 -0
 	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;

builtin/rev-list.c static int try_bitmap_disk_usage(struct rev_info *revs, · line 493

Quoted 10 lines
builtin/rev-list.c +7 -2
 
-	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.

builtin/rev-list.c int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 778

Quoted 9 lines
builtin/rev-list.c +7 -2
-	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)".

t/t6115-rev-list-du.sh check_du HEAD · line 54

Quoted 3 lines
t/t6115-rev-list-du.sh +3 -0
+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...

t/t6115-rev-list-du.sh check_du HEAD · line 59

Quoted 3 lines
t/t6115-rev-list-du.sh +3 -0
+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.

t/t6115-rev-list-du.sh check_du HEAD · line 65

Quoted 6 lines

+'

t/t6115-rev-list-du.sh +5 -0
+
+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).

#2 lore raw
Quoted 14 lines · Li Linchao via GitGitGadget
Quoted 7 lines

From: Li Linchao <[email protected]>

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/

Quoted 1 line
Quoted 8 lines
Quoted 1 line

Teach git rev-list to output more human readable result when using

s/to output more human/to output a human/

Quoted 1 line

'--disk-usage' to calculate objects disk usage.

For this I'd just s/ to calculate objects disk usage//. I.e. we already discussed what --disk-usage does...

OK

Quoted 11 lines
Quoted 7 lines
Documentation/rev-list-options.txt +5 -0
+
+-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:

Quoted 4 lines

--disk-usage
--disk-usage=human

Rather than introducing a new option?

Yes, this makes sense.

Quoted 24 lines
Quoted 11 lines

  struct bitmap_index *bitmap_git;

builtin/rev-list.c +2 -0
+	struct strbuf bitmap_size_buf = STRBUF_INIT;
+	off_t size_from_bitmap;


  if (!show_disk_usage)
  return -1;

builtin/rev-list.c +0 -0
@@ -481,8 +484,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
   if (!bitmap_git)
   return -1;

builtin/rev-list.c static int try_bitmap_disk_usage(struct rev_info *revs, · line 493

Quoted 9 lines
builtin/rev-list.c +7 -2
-	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:

Quoted 8 lines

if (human_readable)

        strbuf_humanise_bytes(&sb, size_from_bitmap);
	else
	strbuf_addf(&sb, "%"PRIuMAX", (uintmax_t)size_from_bitmap);
	puts(sb.buf);

It gets you rid of the need for {} braces, and I think makes for a nicer read.

Agree

Quoted 13 lines

builtin/rev-list.c int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 778

Quoted 10 lines
builtin/rev-list.c +7 -2
-	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)".

Quoted 5 lines

t/t6115-rev-list-du.sh check_du HEAD · line 54

Quoted 3 lines
t/t6115-rev-list-du.sh +3 -0
+test_expect_success 'rev-list --disk-usage with --human-readable' '
+	git rev-list --objects HEAD --disk-usage --human-readable >actual &&
+	test_i18ngrep -e "446 bytes" actual

use grep, not test_i18ngrep (the latter should be going away entirely).

OK

Quoted 12 lines

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...

Quoted 8 lines

In this case it's unlikely to do the wrong thing, but it's a good habit to get into...

t/t6115-rev-list-du.sh check_du HEAD · line 59

Quoted 3 lines
t/t6115-rev-list-du.sh +3 -0
+test_expect_success 'rev-list --disk-usage with bitmap and --human-readable' '
+	git rev-list --objects HEAD --use-bitmap-index --disk-usage -H >actual &&
+	test_i18ngrep -e "446 bytes" actual

ditto.

The output here is just "446  bytes" if we use '--disk-usage' option in this rest repo. But Github CI/linux-sha256 reminded me that I made a mistake that
I should avoid to hardcore actual size here.

Quoted 11 lines

t/t6115-rev-list-du.sh check_du HEAD · line 65

Quoted 8 lines

+'

t/t6115-rev-list-du.sh +5 -0
+
+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:

Quoted 6 lines
	cat >expect <<-\EOF

        fatal: ...
EOF

But you'll still need the '\'' quoting, but I thing it'll be better, and avoids the line-wrapping (which we try to avoid for this sort of thing).

OK.

Many thanks for all your review comments :)

author
#3 lore raw

PATCH v2 1/1 rev-list: support human-readable output for `--disk-usage`

In reply to Li Linchao via GitGitGadget

From: Li Linchao <[email protected]>

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 resulting number is quit hard for a human to read.

Teach git rev-list to output a human readable result when using '--disk-usage'.

Signed-off-by Li Linchao <[email protected]>

---
rev-list: support human-readable output for disk-usage

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.

Teach git rev-list to output more human readable result when using '--disk-usage' to calculate objects disk usage.

Signed-off-by Li Linchao [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1313

Range-diff vs v1:

1: 7f8a7d3f61d ! 1: 7e34d16efe4 rev-list: support --human-readable option when applied disk-usage

     @@ Metadata
      Author: Li Linchao <[email protected]>
      
       ## Commit message ##
     -    rev-list: support `--human-readable` option when applied `disk-usage`
     +    rev-list: support human-readable output for `--disk-usage`
      
          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.
     +    infomation, but the resulting number is quit hard for a human to read.
      
     -    Teach git rev-list to output more human readable result when using
     -    '--disk-usage' to calculate objects disk usage.
     +    Teach git rev-list to output a human readable result when using
     +    '--disk-usage'.
      
          Signed-off-by: Li Linchao <[email protected]>
      
       ## Documentation/rev-list-options.txt ##
      @@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]
     - 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
     - 	section in linkgit:git-cat-file[1] for the limitations of what
     - 	"on-disk storage" means.
     -+
     -+-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[]
     + 	to `/dev/null` as the output does not have to be formatted.
       
     - --cherry-mark::
     + --disk-usage::
     ++--disk-usage=human::
     + 	Suppress normal output; instead, print the sum of the bytes used
     +-	for on-disk storage by the selected commits or objects. This is
     ++	for on-disk storage by the selected commits or objects.
     ++	When it accepts a value `human`, like: `--disk-usage=human`, this
     ++	means to print objects size in human readable format. This is
     + 	equivalent to piping the output into `git cat-file
     + 	--batch-check='%(objectsize:disk)'`, except that it runs much
     + 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
      
       ## builtin/rev-list.c ##
     +@@ builtin/rev-list.c: static const char rev_list_usage[] =
     + "    --parents\n"
     + "    --children\n"
     + "    --objects | --objects-edge\n"
     ++"    --disk-usage | --disk-usage=human\n"
     + "    --unpacked\n"
     + "    --header | --pretty\n"
     + "    --[no-]object-names\n"
      @@ builtin/rev-list.c: static int arg_show_object_names = 1;
       
       static int show_disk_usage;
     @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,
       				 int filter_provided_objects)
       {
       	struct bitmap_index *bitmap_git;
     -+	struct strbuf bitmap_size_buf = STRBUF_INIT;
     ++	struct strbuf disk_buf = STRBUF_INIT;
      +	off_t size_from_bitmap;
       
       	if (!show_disk_usage)
     @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,
      -	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);
     ++	if (human_readable)
     ++		strbuf_humanise_bytes(&disk_buf, size_from_bitmap);
     ++	else
     ++		strbuf_addf(&disk_buf, "%"PRIuMAX"", (uintmax_t)size_from_bitmap);
     ++	puts(disk_buf.buf);
     ++	strbuf_release(&disk_buf);
       	return 0;
       }
       
     -@@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)
     - {
     - 	struct rev_info revs;
     - 	struct rev_list_info info;
     -+	struct strbuf disk_buf = STRBUF_INIT;
     - 	struct setup_revision_opt s_r_opt = {
     - 		.allow_exclude_promisor_objects = 1,
     - 	};
      @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)
       			continue;
       		}
       
     -+		if (!strcmp(arg, "--human-readable") || !strcmp(arg, "-H")) {
     -+			human_readable = 1;
     -+			continue;
     -+		}
     -+
     - 		usage(rev_list_usage);
     +-		if (!strcmp(arg, "--disk-usage")) {
     +-			show_disk_usage = 1;
     +-			info.flags |= REV_LIST_QUIET;
     +-			continue;
     ++		if (skip_prefix(arg, "--disk-usage", &arg)) {
     ++			if (*arg == '=') {
     ++				if (!strcmp(++arg, "human")) {
     ++					human_readable = 1;
     ++					show_disk_usage = 1;
     ++					info.flags |= REV_LIST_QUIET;
     ++					continue;
     ++				} else
     ++					die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);
     ++			} else {
     ++				show_disk_usage = 1;
     ++				info.flags |= REV_LIST_QUIET;
     ++				continue;
     ++			}
     + 		}
       
     - 	}
     -+
     -+	if (!show_disk_usage && human_readable)
     -+		die(_("option '%s' should be used with '%s' together"), "--human-readable/-H", "--disk-usage");
     - 	if (revs.commit_format != CMIT_FMT_USERFORMAT)
     - 		revs.include_header = 1;
     - 	if (revs.commit_format != CMIT_FMT_UNSPECIFIED) {
     + 		usage(rev_list_usage);
      @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)
       			printf("%d\n", revs.count_left + revs.count_right);
       	}
     @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr
      -	if (show_disk_usage)
      -		printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage);
      +	if (show_disk_usage) {
     -+		if (human_readable) {
     ++		struct strbuf disk_buf = STRBUF_INIT;
     ++		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);
     ++		else
     ++			strbuf_addf(&disk_buf, "%"PRIuMAX"", (uintmax_t)total_disk_usage);
     ++		puts(disk_buf.buf);
     ++		strbuf_release(&disk_buf);
      +	}
       
       cleanup:
       	release_revisions(&revs);
     -+	strbuf_release(&disk_buf);
     - 	return ret;
     - }
      
       ## t/t6115-rev-list-du.sh ##
      @@ t/t6115-rev-list-du.sh: check_du HEAD
       check_du --objects HEAD
       check_du --objects HEAD^..HEAD
       
     -+
     -+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
     ++# As mentioned above, don't use hardcode sizes as actual size, but use the
     ++# output from git cat-file.
     ++test_expect_success 'rev-list --disk-usage=human' '
     ++	git rev-list --objects HEAD --disk-usage=human >actual &&
     ++	disk_usage_slow --objects HEAD >actual_size &&
     ++	grep "$(cat actual_size) bytes" actual
      +'
      +
     -+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
     ++test_expect_success 'rev-list --disk-usage=human with bitmaps' '
     ++	git rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&
     ++	disk_usage_slow --objects HEAD >actual_size &&
     ++	grep "$(cat actual_size) bytes" actual
      +'
      +
     -+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 &&
     ++test_expect_success 'rev-list use --disk-usage unproperly' '
     ++	test_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&
     ++	cat >expect <<-\EOF &&
     ++	fatal: invalid value for '\''--disk-usage'\'': '\''typo'\'', try --disk-usage=human
     ++	EOF
      +	test_cmp err expect
      +'
      +
 Documentation/rev-list-options.txt |  5 +++-
 builtin/rev-list.c                 | 42 ++++++++++++++++++++++++------
 t/t6115-rev-list-du.sh             | 22 ++++++++++++++++
 3 files changed, 60 insertions(+), 9 deletions(-)
Documentation/rev-list-options.txt +4 -1
index 195e74eec63..9966ce4ef91 100644
@@ -242,8 +242,11 @@ ifdef::git-rev-list[]
 	to `/dev/null` as the output does not have to be formatted.
 
 --disk-usage::
+--disk-usage=human::
 	Suppress normal output; instead, print the sum of the bytes used
-	for on-disk storage by the selected commits or objects. This is
+	for on-disk storage by the selected commits or objects.
+	When it accepts a value `human`, like: `--disk-usage=human`, this
+	means to print objects size in human readable format. This is
 	equivalent to piping the output into `git cat-file
 	--batch-check='%(objectsize:disk)'`, except that it runs much
 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
builtin/rev-list.c +34 -8

78 lines not on this page · raw message

t/t6115-rev-list-du.sh +22 -0
index b4aef32b713..b34841a4ba8 100755
@@ -48,4 +48,26 @@ check_du HEAD
 check_du --objects HEAD
 check_du --objects HEAD^..HEAD
 
+# As mentioned above, don't use hardcode sizes as actual size, but use the
+# output from git cat-file.
+test_expect_success 'rev-list --disk-usage=human' '
+	git rev-list --objects HEAD --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list --disk-usage=human with bitmaps' '
+	git rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list use --disk-usage unproperly' '
+	test_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&
+	cat >expect <<-\EOF &&
+	fatal: invalid value for '\''--disk-usage'\'': '\''typo'\'', try --disk-usage=human
+	EOF
+	test_cmp err expect
+'
+
 test_done

base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9

Signature · 1 line
gitgitgadget
#4 lore raw

Re: [PATCH v2] rev-list: support human-readable output for `--disk-usage`

Quoted 8 lines

       static int show_disk_usage;
     @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs, int filter_provided_objects)
       {
       struct bitmap_index *bitmap_git;
     -+ struct strbuf bitmap_size_buf = STRBUF_INIT;
     ++ struct strbuf disk_buf = STRBUF_INIT;
      + off_t size_from_bitmap;

In next iteration, will move these two lines to more close to their caller place to avoid early return.

Quoted 20 lines


       if (!show_disk_usage)
     @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,
      - 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);
     ++ if (human_readable)
     ++ strbuf_humanise_bytes(&disk_buf, size_from_bitmap);
     ++ else
     ++ strbuf_addf(&disk_buf, "%"PRIuMAX"", (uintmax_t)size_from_bitmap);
     ++ puts(disk_buf.buf);
     ++ strbuf_release(&disk_buf);
       return 0;
       }

#5 lore raw

Re: [PATCH v2] rev-list: support human-readable output for `--disk-usage`

In reply to Li Linchao via GitGitGadget
Quoted 7 lines · Li Linchao via GitGitGadget

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 resulting number is quit hard for a human to read.

Teach git rev-list to output a human readable result when using '--disk-usage'.

OK. When adding --disk-usage, I never really dreamed people would use it for human output, since "du .git" is usually a suitable approximation. :) But I don't have any real objection. I'm curious what your use case is like, if you don't mind sharing. We used it at GitHub for computing per-fork sizes for analysis, etc (so the result was always fed into another script).

Quoted 4 lines
 Documentation/rev-list-options.txt |  5 +++-
 builtin/rev-list.c                 | 42 ++++++++++++++++++++++++------
 t/t6115-rev-list-du.sh             | 22 ++++++++++++++++
 3 files changed, 60 insertions(+), 9 deletions(-)

The patch itself looks pretty sensible (and thanks Ævar for the first round of review; the suggestions there all looked good). A few small comments:

builtin/rev-list.c static int try_bitmap_disk_usage(struct rev_info *revs, · line 494

Quoted 13 lines
builtin/rev-list.c +7 -2
@@ -481,8 +485,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(&disk_buf, size_from_bitmap);
+	else
+		strbuf_addf(&disk_buf, "%"PRIuMAX"", (uintmax_t)size_from_bitmap);
+	puts(disk_buf.buf);
+	strbuf_release(&disk_buf);

It's not a lot of duplicated lines, but since it is implementing policy logic, I think it would be nice to move the formatting decision into a function. Something like:

static void show_disk_usage(off_t size)

  {
	struct strbuf sb = STRBUF_INIT;
	if (human_readable)
		strbuf_humanise_bytes(&sb, size);
	else
		strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size_from_bitmap);
	puts(sb.buf);
	strbuf_release(&sb);

}

and then you can call it from here, and from the non-bitmap path below.

(Also, while typing it out, I noticed that you don't need the extra "" after PRIuMAX; that just concatenates an empty string).

Quoted 9 lines
builtin/rev-list.c +5 -4
-		if (!strcmp(arg, "--disk-usage")) {
-			show_disk_usage = 1;
-			info.flags |= REV_LIST_QUIET;
-			continue;
+		if (skip_prefix(arg, "--disk-usage", &arg)) {
+			if (*arg == '=') {
+				if (!strcmp(++arg, "human")) {
+					human_readable = 1;
+					show_disk_usage = 1;

builtin/rev-list.c int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 650

Quoted 10 lines
builtin/rev-list.c +9 -0
+					info.flags |= REV_LIST_QUIET;
+					continue;
+				} else
+					die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);
+			} else {
+				show_disk_usage = 1;
+				info.flags |= REV_LIST_QUIET;
+				continue;
+			}
 		}

We can put the common parts of each side of the conditional into the outer block to avoid repeating ourselves. Also, your code matches --show-disk-usage-without-an-equals, since it nows uses skip_prefix(). You could fix that by checking for '\0' in *arg. So together, something like:

  if (skip_prefix(arg, "--disk-usage", &arg)) {
	if (*arg == '=') {
		if (!strcmp(++arg, "human"))
			human_readable = 1;
		else
			die(...);
	} else if (*arg) {
		/*
		 * Arguably should goto a label to continue chain of ifs?
		 * Doesn't matter unless we try to add --disk-usage-foo
		 * afterwards
		 */
		usage(rev_list_usage);
	}
	show_disk_usage = 1;
	info.flags |= REV_LIST_QUIET;
	continue;

}

-Peff

#6 lore raw

Re: Re: [PATCH v2] rev-list: support human-readable output for `--disk-usage`

Quoted 10 lines · Li Linchao via GitGitGadget
Quoted 8 lines

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 resulting number is quit hard for a human to read.

Teach git rev-list to output a human readable result when using '--disk-usage'.

Quoted 6 lines

OK. When adding --disk-usage, I never really dreamed people would use it for human output, since "du .git" is usually a suitable approximation. :) But I don't have any real objection. I'm curious what your use case is like, if you don't mind sharing. We used it at GitHub for computing per-fork sizes for analysis, etc (so the result was always fed into another script).

Sometimes, I need objects disk size for analysis too, but I do this by hand not by script. And I also noticed that git-count-objects has an option '-H' for better output, so I think if it could be applied to "git-rev-list --disk-usage".

Quoted 37 lines
Quoted 5 lines

  Documentation/rev-list-options.txt |  5 +++- builtin/rev-list.c                 | 42 ++++++++++++++++++++++++------ t/t6115-rev-list-du.sh             | 22 ++++++++++++++++

  3 files changed, 60 insertions(+), 9 deletions(-)

The patch itself looks pretty sensible (and thanks Ævar for the first round of review; the suggestions there all looked good). A few small comments:

builtin/rev-list.c static int try_bitmap_disk_usage(struct rev_info *revs, · line 494

Quoted 13 lines
builtin/rev-list.c +0 -0
@@ -481,8 +485,13 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
   if (!bitmap_git)
   return -1;

builtin/rev-list.c +7 -2
-	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(&disk_buf, size_from_bitmap);
+	else
+	strbuf_addf(&disk_buf, "%"PRIuMAX"", (uintmax_t)size_from_bitmap);
+	puts(disk_buf.buf);
+	strbuf_release(&disk_buf);

It's not a lot of duplicated lines, but since it is implementing policy logic, I think it would be nice to move the formatting decision into a function. Something like:

  static void show_disk_usage(off_t size)

  {
	struct strbuf sb = STRBUF_INIT;
	if (human_readable)
	strbuf_humanise_bytes(&sb, size);
	else
	strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size_from_bitmap);
	puts(sb.buf);
	strbuf_release(&sb);
Quoted 3 lines

  }

and then you can call it from here, and from the non-bitmap path below.

Oh, this is really good.

Quoted 47 lines

(Also, while typing it out, I noticed that you don't need the extra "" after PRIuMAX; that just concatenates an empty string).

Quoted 18 lines
builtin/rev-list.c +14 -4
-	if (!strcmp(arg, "--disk-usage")) {
-	show_disk_usage = 1;
-	info.flags |= REV_LIST_QUIET;
-	continue;
+	if (skip_prefix(arg, "--disk-usage", &arg)) {
+	if (*arg == '=') {
+	if (!strcmp(++arg, "human")) {
+	human_readable = 1;
+	show_disk_usage = 1;
+	info.flags |= REV_LIST_QUIET;
+	continue;
+	} else
+	die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);
+	} else {
+	show_disk_usage = 1;
+	info.flags |= REV_LIST_QUIET;
+	continue;
+	}
Quoted 1 line

  }

We can put the common parts of each side of the conditional into the outer block to avoid repeating ourselves. Also, your code matches --show-disk-usage-without-an-equals, since it nows uses skip_prefix(). You could fix that by checking for '\0' in *arg. So together, something like:

  if (skip_prefix(arg, "--disk-usage", &arg)) {
	if (*arg == '=') {
	if (!strcmp(++arg, "human"))
	human_readable = 1;
	else
	die(...);
	} else if (*arg) {
	/*
	* Arguably should goto a label to continue chain of ifs?
	* Doesn't matter unless we try to add --disk-usage-foo
	* afterwards
	*/
	usage(rev_list_usage);
	}
	show_disk_usage = 1;
	info.flags |= REV_LIST_QUIET;
	continue;
Quoted 1 line

  }

Thank you a lot for your very nice suggestions.

Quoted 2 lines

-Peff

author
#7 lore raw

PATCH v3 1/1 rev-list: support human-readable output for `--disk-usage`

In reply to Li Linchao via GitGitGadget

From: Li Linchao <[email protected]>

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 resulting number is quit hard for a human to read.

Teach git rev-list to output a human readable result when using '--disk-usage'.

Signed-off-by Li Linchao <[email protected]>

---
rev-list: support human-readable output for disk-usage

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.

Teach git rev-list to output more human readable result when using '--disk-usage' to calculate objects disk usage.

Signed-off-by Li Linchao [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1313

Range-diff vs v2:

1: 7e34d16efe4 ! 1: 000a6b37ec9 rev-list: support human-readable output for --disk-usage

     @@ builtin/rev-list.c: static int arg_show_object_names = 1;
       
       static off_t get_object_disk_usage(struct object *obj)
       {
     +@@ builtin/rev-list.c: static int show_object_fast(
     + 	return 1;
     + }
     + 
     ++static void print_disk_usage(off_t size)
     ++{
     ++	struct strbuf sb = STRBUF_INIT;
     ++	if (human_readable)
     ++		strbuf_humanise_bytes(&sb, size);
     ++	else
     ++		strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size);
     ++	puts(sb.buf);
     ++	strbuf_release(&sb);
     ++}
     ++
     + static inline int parse_missing_action_value(const char *value)
     + {
     + 	if (!strcmp(value, "error")) {
      @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,
       				 int filter_provided_objects)
       {
       	struct bitmap_index *bitmap_git;
     -+	struct strbuf disk_buf = STRBUF_INIT;
      +	off_t size_from_bitmap;
       
       	if (!show_disk_usage)
     @@ builtin/rev-list.c: static int try_bitmap_disk_usage(struct rev_info *revs,
      -	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(&disk_buf, size_from_bitmap);
     -+	else
     -+		strbuf_addf(&disk_buf, "%"PRIuMAX"", (uintmax_t)size_from_bitmap);
     -+	puts(disk_buf.buf);
     -+	strbuf_release(&disk_buf);
     ++	print_disk_usage(size_from_bitmap);
       	return 0;
       }
       
     @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr
       		}
       
      -		if (!strcmp(arg, "--disk-usage")) {
     --			show_disk_usage = 1;
     --			info.flags |= REV_LIST_QUIET;
     --			continue;
      +		if (skip_prefix(arg, "--disk-usage", &arg)) {
      +			if (*arg == '=') {
      +				if (!strcmp(++arg, "human")) {
      +					human_readable = 1;
     -+					show_disk_usage = 1;
     -+					info.flags |= REV_LIST_QUIET;
     -+					continue;
      +				} else
      +					die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);
     -+			} else {
     -+				show_disk_usage = 1;
     -+				info.flags |= REV_LIST_QUIET;
     -+				continue;
     ++			} else if (*arg) {
     ++				/*
     ++				* Arguably should goto a label to continue chain of ifs?
     ++				* Doesn't matter unless we try to add --disk-usage-foo
     ++				* afterwards
     ++				*/
     ++				usage(rev_list_usage);
      +			}
     - 		}
     - 
     - 		usage(rev_list_usage);
     + 			show_disk_usage = 1;
     + 			info.flags |= REV_LIST_QUIET;
     + 			continue;
      @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *prefix)
     - 			printf("%d\n", revs.count_left + revs.count_right);
       	}
       
     --	if (show_disk_usage)
     + 	if (show_disk_usage)
      -		printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage);
     -+	if (show_disk_usage) {
     -+		struct strbuf disk_buf = STRBUF_INIT;
     -+		if (human_readable)
     -+			strbuf_humanise_bytes(&disk_buf, total_disk_usage);
     -+		else
     -+			strbuf_addf(&disk_buf, "%"PRIuMAX"", (uintmax_t)total_disk_usage);
     -+		puts(disk_buf.buf);
     -+		strbuf_release(&disk_buf);
     -+	}
     ++		print_disk_usage(total_disk_usage);
       
       cleanup:
       	release_revisions(&revs);
 Documentation/rev-list-options.txt |  5 ++++-
 builtin/rev-list.c                 | 35 ++++++++++++++++++++++++++----
 t/t6115-rev-list-du.sh             | 22 +++++++++++++++++++
 3 files changed, 57 insertions(+), 5 deletions(-)
Documentation/rev-list-options.txt +4 -1
index 195e74eec63..9966ce4ef91 100644
@@ -242,8 +242,11 @@ ifdef::git-rev-list[]
 	to `/dev/null` as the output does not have to be formatted.
 
 --disk-usage::
+--disk-usage=human::
 	Suppress normal output; instead, print the sum of the bytes used
-	for on-disk storage by the selected commits or objects. This is
+	for on-disk storage by the selected commits or objects.
+	When it accepts a value `human`, like: `--disk-usage=human`, this
+	means to print objects size in human readable format. This is
 	equivalent to piping the output into `git cat-file
 	--batch-check='%(objectsize:disk)'`, except that it runs much
 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
builtin/rev-list.c +31 -4
index 30fd8e83eaf..df42e1b667e 100644
@@ -46,6 +46,7 @@ static const char rev_list_usage[] =
 "    --parents\n"
 "    --children\n"
 "    --objects | --objects-edge\n"
+"    --disk-usage | --disk-usage=human\n"
 "    --unpacked\n"
 "    --header | --pretty\n"
 "    --[no-]object-names\n"

@@ -81,6 +82,7 @@ static int arg_show_object_names = 1;
 
 static int show_disk_usage;
 static off_t total_disk_usage;
+static int human_readable;
 
 static off_t get_object_disk_usage(struct object *obj)
 {

@@ -368,6 +370,17 @@ static int show_object_fast(
 	return 1;
 }
 
+static void print_disk_usage(off_t size)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (human_readable)
+		strbuf_humanise_bytes(&sb, size);
+	else
+		strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size);
+	puts(sb.buf);
+	strbuf_release(&sb);
+}
+
 static inline int parse_missing_action_value(const char *value)
 {
 	if (!strcmp(value, "error")) {

@@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
 				 int filter_provided_objects)
 {
 	struct bitmap_index *bitmap_git;
+	off_t size_from_bitmap;
 
 	if (!show_disk_usage)
 		return -1;

@@ -481,8 +495,8 @@ 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);
+	print_disk_usage(size_from_bitmap);
 	return 0;
 }
 

@@ -624,7 +638,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!strcmp(arg, "--disk-usage")) {
+		if (skip_prefix(arg, "--disk-usage", &arg)) {
+			if (*arg == '=') {
+				if (!strcmp(++arg, "human")) {
+					human_readable = 1;
+				} else
+					die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);
+			} else if (*arg) {
+				/*
+				* Arguably should goto a label to continue chain of ifs?
+				* Doesn't matter unless we try to add --disk-usage-foo
+				* afterwards
+				*/
+				usage(rev_list_usage);
+			}
 			show_disk_usage = 1;
 			info.flags |= REV_LIST_QUIET;
 			continue;

@@ -753,7 +780,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	}
 
 	if (show_disk_usage)
-		printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage);
+		print_disk_usage(total_disk_usage);
 
 cleanup:
 	release_revisions(&revs);
t/t6115-rev-list-du.sh +22 -0
index b4aef32b713..b34841a4ba8 100755
@@ -48,4 +48,26 @@ check_du HEAD
 check_du --objects HEAD
 check_du --objects HEAD^..HEAD
 
+# As mentioned above, don't use hardcode sizes as actual size, but use the
+# output from git cat-file.
+test_expect_success 'rev-list --disk-usage=human' '
+	git rev-list --objects HEAD --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list --disk-usage=human with bitmaps' '
+	git rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list use --disk-usage unproperly' '
+	test_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&
+	cat >expect <<-\EOF &&
+	fatal: invalid value for '\''--disk-usage'\'': '\''typo'\'', try --disk-usage=human
+	EOF
+	test_cmp err expect
+'
+
 test_done

base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9

Signature · 1 line
gitgitgadget
#8 lore raw

Re: [PATCH v3] rev-list: support human-readable output for `--disk-usage`

Am 10.08.22 um 08:01 schrieb Li Linchao via GitGitGadget:

Documentation/rev-list-options.txt ifdef::git-rev-list[] · line 252

Quoted 10 lines

--disk-usage::
+--disk-usage=human::

Documentation/rev-list-options.txt +3 -1
 	Suppress normal output; instead, print the sum of the bytes used
-	for on-disk storage by the selected commits or objects. This is
+	for on-disk storage by the selected commits or objects.
+	When it accepts a value `human`, like: `--disk-usage=human`, this
+	means to print objects size in human readable format. This is
 	equivalent to piping the output into `git cat-file
 	--batch-check='%(objectsize:disk)'`, except that it runs much
 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`

The original paragraph flows very well and explains what the option does and how it computes the result. Please do not interrupt the flow of the text with a whole sentence that should be just a parenthetical remark, but add a sentence at the end, not in the middle.

You added a new feature, and I understand that it is important *to you*. But do make it a habit to ask yourself if it is also important for the general audience. Generally, a new feature is not as important as existing features, otherwise, it would have been added earlier, wouldn't it?

-- Hannes

author
#9 lore raw

PATCH v4 1/1 rev-list: support human-readable output for `--disk-usage`

In reply to Li Linchao via GitGitGadget

From: Li Linchao <[email protected]>

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 resulting number is quit hard for a human to read.

Teach git rev-list to output a human readable result when using '--disk-usage'.

Signed-off-by Li Linchao <[email protected]>

---
rev-list: support human-readable output for disk-usage

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.

Teach git rev-list to output more human readable result when using '--disk-usage' to calculate objects disk usage.

Signed-off-by Li Linchao [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1313

Range-diff vs v3:

1: 000a6b37ec9 ! 1: e56da057a9a rev-list: support human-readable output for --disk-usage
@@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]
--disk-usage::
+--disk-usage=human::
Suppress normal output; instead, print the sum of the bytes used
-- for on-disk storage by the selected commits or objects. This is
-+ for on-disk storage by the selected commits or objects.
-+ When it accepts a value human, like: --disk-usage=human, this
-+ means to print objects size in human readable format. This is
+ for on-disk storage by the selected commits or objects. This is
equivalent to piping the output into `git cat-file
- --batch-check='%(objectsize:disk)'`, except that it runs much
+@@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]
faster (especially with --use-bitmap-index). See the CAVEATS
+ section in linkgit:git-cat-file[1] for the limitations of what
+ "on-disk storage" means.
++ When it accepts a value human, like: --disk-usage=human, this
++ means to print objects size in human readable format.
+ endif::git-rev-list[]
+
+ --cherry-mark::

## builtin/rev-list.c ##
@@ builtin/rev-list.c: static const char rev_list_usage[] =

 Documentation/rev-list-options.txt |  3 +++
 builtin/rev-list.c                 | 35 ++++++++++++++++++++++++++----
 t/t6115-rev-list-du.sh             | 22 +++++++++++++++++++
 3 files changed, 56 insertions(+), 4 deletions(-)
Documentation/rev-list-options.txt +3 -0
index 195e74eec63..5d3880874fc 100644
@@ -242,6 +242,7 @@ ifdef::git-rev-list[]
 	to `/dev/null` as the output does not have to be formatted.
 
 --disk-usage::
+--disk-usage=human::
 	Suppress normal output; instead, print the sum of the bytes used
 	for on-disk storage by the selected commits or objects. This is
 	equivalent to piping the output into `git cat-file

@@ -249,6 +250,8 @@ ifdef::git-rev-list[]
 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
 	section in linkgit:git-cat-file[1] for the limitations of what
 	"on-disk storage" means.
+	When it accepts a value `human`, like: `--disk-usage=human`, this
+	means to print objects size in human readable format.
 endif::git-rev-list[]
 
 --cherry-mark::
builtin/rev-list.c +31 -4
index 30fd8e83eaf..df42e1b667e 100644
@@ -46,6 +46,7 @@ static const char rev_list_usage[] =
 "    --parents\n"
 "    --children\n"
 "    --objects | --objects-edge\n"
+"    --disk-usage | --disk-usage=human\n"
 "    --unpacked\n"
 "    --header | --pretty\n"
 "    --[no-]object-names\n"

@@ -81,6 +82,7 @@ static int arg_show_object_names = 1;
 
 static int show_disk_usage;
 static off_t total_disk_usage;
+static int human_readable;
 
 static off_t get_object_disk_usage(struct object *obj)
 {

@@ -368,6 +370,17 @@ static int show_object_fast(
 	return 1;
 }
 
+static void print_disk_usage(off_t size)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (human_readable)
+		strbuf_humanise_bytes(&sb, size);
+	else
+		strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size);
+	puts(sb.buf);
+	strbuf_release(&sb);
+}
+
 static inline int parse_missing_action_value(const char *value)
 {
 	if (!strcmp(value, "error")) {

@@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
 				 int filter_provided_objects)
 {
 	struct bitmap_index *bitmap_git;
+	off_t size_from_bitmap;
 
 	if (!show_disk_usage)
 		return -1;

@@ -481,8 +495,8 @@ 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);
+	print_disk_usage(size_from_bitmap);
 	return 0;
 }
 

@@ -624,7 +638,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!strcmp(arg, "--disk-usage")) {
+		if (skip_prefix(arg, "--disk-usage", &arg)) {
+			if (*arg == '=') {
+				if (!strcmp(++arg, "human")) {
+					human_readable = 1;
+				} else
+					die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);
+			} else if (*arg) {
+				/*
+				* Arguably should goto a label to continue chain of ifs?
+				* Doesn't matter unless we try to add --disk-usage-foo
+				* afterwards
+				*/
+				usage(rev_list_usage);
+			}
 			show_disk_usage = 1;
 			info.flags |= REV_LIST_QUIET;
 			continue;

@@ -753,7 +780,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	}
 
 	if (show_disk_usage)
-		printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage);
+		print_disk_usage(total_disk_usage);
 
 cleanup:
 	release_revisions(&revs);
t/t6115-rev-list-du.sh +22 -0
index b4aef32b713..b34841a4ba8 100755
@@ -48,4 +48,26 @@ check_du HEAD
 check_du --objects HEAD
 check_du --objects HEAD^..HEAD
 
+# As mentioned above, don't use hardcode sizes as actual size, but use the
+# output from git cat-file.
+test_expect_success 'rev-list --disk-usage=human' '
+	git rev-list --objects HEAD --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list --disk-usage=human with bitmaps' '
+	git rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list use --disk-usage unproperly' '
+	test_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&
+	cat >expect <<-\EOF &&
+	fatal: invalid value for '\''--disk-usage'\'': '\''typo'\'', try --disk-usage=human
+	EOF
+	test_cmp err expect
+'
+
 test_done

base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9

Signature · 1 line
gitgitgadget
#10 lore raw

Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`

Quoted 13 lines · Li Linchao via GitGitGadget
Documentation/rev-list-options.txt +1 -0
index 195e74eec63..5d3880874fc 100644
@@ -242,6 +242,7 @@ ifdef::git-rev-list[]
 	to `/dev/null` as the output does not have to be formatted.
 
 --disk-usage::
+--disk-usage=human::
 	Suppress normal output; instead, print the sum of the bytes used
 	for on-disk storage by the selected commits or objects. This is
 	equivalent to piping the output into `git cat-file

Documentation/rev-list-options.txt ifdef::git-rev-list[] · line 254

Quoted 5 lines
Documentation/rev-list-options.txt +2 -0
@@ -249,6 +250,8 @@ ifdef::git-rev-list[]
 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
 	section in linkgit:git-cat-file[1] for the limitations of what
 	"on-disk storage" means.
+	When it accepts a value `human`, like: `--disk-usage=human`, this
+	means to print objects size in human readable format.

"When it accepts" sounds wrong, because it implies there are two cases, i.e. the user gives "--disk-usage=human" and the command somehow decides to accept it, or to reject it, which is not what is going on. How about phrasing it more like ...

With the optional value human, on-disk storage size is shown in human-readable string (e.g. 12.23 KiB, 3.50 MiB).

perhaps?

Documentation/rev-list-options.txt static const char rev_list_usage[] = · line 49

Quoted 5 lines
Documentation/rev-list-options.txt +1 -0
@@ -46,6 +46,7 @@ static const char rev_list_usage[] =
 "    --parents\n"
 "    --children\n"
 "    --objects | --objects-edge\n"
+"    --disk-usage | --disk-usage=human\n"

Writing it like

	"--disk-usage[=human]\n"

would be more in line with ...

Documentation/rev-list-options.txt static const char rev_list_usage[] = · line 50

Quoted 1 line
Documentation/rev-list-options.txt +0 -0
 "    --[no-]object-names\n"

... this one.

Quoted 4 lines
Documentation/rev-list-options.txt +0 -0
@@ -368,6 +370,17 @@ static int show_object_fast(
 	return 1;
 }

Documentation/rev-list-options.txt static int show_object_fast( · line 382

Quoted 10 lines
Documentation/rev-list-options.txt +10 -0
+static void print_disk_usage(off_t size)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (human_readable)
+		strbuf_humanise_bytes(&sb, size);
+	else
+		strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size);
+	puts(sb.buf);
+	strbuf_release(&sb);
+}

Hmph, I am not sure if we want to make it a helper like this. The normal case does not need to prepare the string into a strbuf but just can send the output to the standard output stream.

It is probably easy to fix, like so:

	if (!human_readable) {
		printf("%" PRIuMAX "\n", disk_usage);
	} else {
		strbuf sb = STRBUF_INIT;
		strbuf_humanise_bytes(&sb, disk_usage);
		puts(sb.buf);
		strbuf_release(&sb);
	}

Documentation/rev-list-options.txt static int try_bitmap_disk_usage(struct rev_info *revs, · line 489

Quoted 8 lines
Documentation/rev-list-options.txt +1 -0
 static inline int parse_missing_action_value(const char *value)
 {
 	if (!strcmp(value, "error")) {

@@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
 				 int filter_provided_objects)
 {
 	struct bitmap_index *bitmap_git;
+	off_t size_from_bitmap;

Questionably named variable.

Documentation/rev-list-options.txt static int try_bitmap_disk_usage(struct rev_info *revs, · line 499

Quoted 10 lines
Documentation/rev-list-options.txt +2 -2
 	if (!show_disk_usage)
 		return -1;

@@ -481,8 +495,8 @@ 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);
+	print_disk_usage(size_from_bitmap);

It makes sense to make the function declare how it gets disk usage in its name, but once we call the function to get what we want, there is no need to keep saying we got it from bitmap. If we ever gained another function that obtains the disk usage from other means, then this part of the code would become

	if (use_bitmap)
		variable = get_disk_usage_from_bitmap(...);
	else
		variable = get_disk_usage_from_other_means(...);

	print_disk_usage(variable);

Now, what should that variable be named? It is clear that it shouldn't be named "from_bitmap" at all, because it may very well have come from somewhere else.

You call it simply 'size' in print_disk_usage(), and that would probably be a good name to use here. Or even better, "disk_usage".

However, I think all of the above badness stems from the horrible way the existing code deals with "use_bitmap_index", and it is not in the scope of this patch to fix it, let's keep the above code as-is. This is only called from the "if (use_bitmap_index)" so the size can only be from bitmap.

Documentation/rev-list-options.txt int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 646

Quoted 11 lines
Documentation/rev-list-options.txt +6 -1
@@ -624,7 +638,20 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!strcmp(arg, "--disk-usage")) {
+		if (skip_prefix(arg, "--disk-usage", &arg)) {
+			if (*arg == '=') {
+				if (!strcmp(++arg, "human")) {
+					human_readable = 1;
+				} else
+					die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);

This makes the hardcoded "--disk-usage=human" subject to translation, which it should not. Perhaps like this:

	die(_("invalid value for '%s': '%s', the only allowed format is "%s"),
	    "--disk-usage=<format>", arg, "human");

The main point is to ensure that "human" that we are not allowing the user to type in their language is left out of the _("translatable string").

Documentation/rev-list-options.txt int cmd_rev_list(int argc, const char **argv, const char *prefix) · line 652

Quoted 6 lines
Documentation/rev-list-options.txt +6 -0
+			} else if (*arg) {
+				/*
+				* Arguably should goto a label to continue chain of ifs?
+				* Doesn't matter unless we try to add --disk-usage-foo
+				* afterwards
+				*/

Wrong indentation for a long comment?

Quoted 2 lines
Documentation/rev-list-options.txt +2 -0
+				usage(rev_list_usage);
+			}
#11 lore raw

Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`

Quoted 17 lines · Junio C Hamano

builtin/rev-list.c static int show_object_fast( · line 382

Quoted 10 lines
builtin/rev-list.c +10 -0
+static void print_disk_usage(off_t size)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (human_readable)
+		strbuf_humanise_bytes(&sb, size);
+	else
+		strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size);
+	puts(sb.buf);
+	strbuf_release(&sb);
+}

Hmph, I am not sure if we want to make it a helper like this. The normal case does not need to prepare the string into a strbuf but just can send the output to the standard output stream.

It is probably easy to fix, like so:

Quoted 8 lines
	if (!human_readable) {
		printf("%" PRIuMAX "\n", disk_usage);
	} else {
		strbuf sb = STRBUF_INIT;
		strbuf_humanise_bytes(&sb, disk_usage);
		puts(sb.buf);
		strbuf_release(&sb);
	}

It was my suggestion to turn it into a helper, because the same code needs to be present in two distant spots (the bitmap and non-bitmap cases).

I don't care much between "printf directly vs strbuf" for the non-human case, but it was an earlier review suggestion to connect them. I do think the result is a little easier to follow, but mostly I want to make it clear that the author is getting stuck between warring review comments here. ;)

Quoted 4 lines
Quoted 4 lines
builtin/rev-list.c +0 -0
@@ -481,8 +495,8 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
 	if (!bitmap_git)
 		return -1;
Quoted 10 lines

builtin/rev-list.c static int try_bitmap_disk_usage(struct rev_info *revs, · line 499

Quoted 4 lines
builtin/rev-list.c +2 -2
-	printf("%"PRIuMAX"\n",
-	       (uintmax_t)get_disk_usage_from_bitmap(bitmap_git, revs));
+	size_from_bitmap = get_disk_usage_from_bitmap(bitmap_git, revs);
+	print_disk_usage(size_from_bitmap);

It makes sense to make the function declare how it gets disk usage in its name, but once we call the function to get what we want, there is no need to keep saying we got it from bitmap. If we ever gained another function that obtains the disk usage from other means, then this part of the code would become

Keep in mind that we are in try_bitmap_disk_usage() here. :) There is indeed similar code to use other means, but it's far away, and this code will always use bitmaps.

That said, I'd have just written:

  print_disk_usage(get_disk_usage_from_bitmap(bitmap_git, revs));

since the variable is not otherwise used. But arguably that's harder to read.

-Peff

#12 lore raw

Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`

Quoted 5 lines · Jeff King

It was my suggestion to turn it into a helper, because the same code needs to be present in two distant spots (the bitmap and non-bitmap cases).
...
Keep in mind that we are in try_bitmap_disk_usage() here.

Yeah, both of them I notice in a later parts of the message you are responding to ;-)

#13 lore raw

Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`

In reply to Jeff King
Quoted 5 lines · Jeff King

I don't care much between "printf directly vs strbuf" for the non-human case, but it was an earlier review suggestion to connect them. I do think the result is a little easier to follow, but mostly I want to make it clear that the author is getting stuck between warring review comments here. ;)

The function does look like it can later be called millions of times, at which time we may want to measure, but as the only use of this helper function is to get called just once at the end with a single number and print it (as opposed to getting called once per each object), I do not care too much about the normal case being made unnecessarily more expensive, either.

I still do not see the point of changing it to print to a strbuf and puts() the result, though. It does not make the code shorter, more efficient, or more readable. Once "if (we are producing humanize format)" condition is hit, both of its branches can either be (1) responsible to print the number to the standard output stream, using whatever implementation, or (2) responsible to print the number to a strbuf, so that somebody outside the if statement will be respohnsible for printing that string to the standard output stream.

The patch chooses (2), which is more complex, for no good reason. A good thing about (1) is that the non-human codepath can STAY to be the same as before, which is one fewer chance to introduce unnecessary bugs.

Anyway...

#14 lore raw

Re: [PATCH v4] rev-list: support human-readable output for `--disk-usage`

Quoted 8 lines · Junio C Hamano

I still do not see the point of changing it to print to a strbuf and puts() the result, though. It does not make the code shorter, more efficient, or more readable. Once "if (we are producing humanize format)" condition is hit, both of its branches can either be (1) responsible to print the number to the standard output stream, using whatever implementation, or (2) responsible to print the number to a strbuf, so that somebody outside the if statement will be respohnsible for printing that string to the standard output stream.

I was not the original reviewer who suggested that change, but FWIW, I think the argument for cases like these is something like this. Between:

  if (some_cond) {
    foo(do_some_prep());
  } else {
    foo(do_some_other_prep());
  }

and:

  if (some_cond) {
    x = do_some_prep();
  } else {
    x = do_some_other_prep();
  }
  foo(x);

the latter makes it more clear that foo() is called on both sides of the conditional. In this case the "foo" is not exactly the same: on one it is printing a strbuf, and on the other it is a printf that direct-formats. But the principle is the same, if you want to make it clear that both sides will result in printing something.

As you note, it loses the possibility for one side of the conditional to do its "foo" in a more efficient way, but I don't think that's very important for this particular call site.

Quoted 4 lines

The patch chooses (2), which is more complex, for no good reason. A good thing about (1) is that the non-human codepath can STAY to be the same as before, which is one fewer chance to introduce unnecessary bugs.

True.

Again, I don't care much either way. But I am not quite on the "I do not see the point at all" side.

-Peff

author
#15 lore raw

PATCH v5 1/1 rev-list: support human-readable output for `--disk-usage`

In reply to Li Linchao via GitGitGadget

From: Li Linchao <[email protected]>

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 resulting number is quit hard for a human to read.

Teach git rev-list to output a human readable result when using '--disk-usage'.

Signed-off-by Li Linchao <[email protected]>

---
rev-list: support human-readable output for disk-usage

Signed-off-by Li Linchao [email protected] Cc: Jeff King

[email protected] cc: Ævar Arnfjörð Bjarmason [email protected] cc: Johannes Sixt [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1313%2FCactusinhand%2Fllc%2Fadd-human-readable-option-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1313/Cactusinhand/llc/add-human-readable-option-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1313

Range-diff vs v4:

1: e56da057a9a ! 1: 7fbf4b636d6 rev-list: support human-readable output for --disk-usage

     @@ Documentation/rev-list-options.txt: ifdef::git-rev-list[]
       	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
       	section in linkgit:git-cat-file[1] for the limitations of what
       	"on-disk storage" means.
     -+	When it accepts a value `human`, like: `--disk-usage=human`, this
     -+	means to print objects size in human readable format.
     ++	With the optional value `human`, on-disk storage size is shown
     ++	in human-readable string(e.g. 12.24 Kib, 3.50 Mib).
       endif::git-rev-list[]
       
       --cherry-mark::
     @@ builtin/rev-list.c: static const char rev_list_usage[] =
       "    --parents\n"
       "    --children\n"
       "    --objects | --objects-edge\n"
     -+"    --disk-usage | --disk-usage=human\n"
     ++"    --disk-usage[=human]\n"
       "    --unpacked\n"
       "    --header | --pretty\n"
       "    --[no-]object-names\n"
     @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr
      +				if (!strcmp(++arg, "human")) {
      +					human_readable = 1;
      +				} else
     -+					die(_("invalid value for '%s': '%s', try --disk-usage=human"), "--disk-usage", arg);
     ++					die(_("invalid value for '%s': '%s', the only allowed format is '%s'"),
     ++					    "--disk-usage=<format>", arg, "human");
      +			} else if (*arg) {
      +				/*
     -+				* Arguably should goto a label to continue chain of ifs?
     -+				* Doesn't matter unless we try to add --disk-usage-foo
     -+				* afterwards
     -+				*/
     ++				 * Arguably should goto a label to continue chain of ifs?
     ++				 * Doesn't matter unless we try to add --disk-usage-foo
     ++				 * afterwards.
     ++				 */
      +				usage(rev_list_usage);
      +			}
       			show_disk_usage = 1;
     @@ t/t6115-rev-list-du.sh: check_du HEAD
      +test_expect_success 'rev-list use --disk-usage unproperly' '
      +	test_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&
      +	cat >expect <<-\EOF &&
     -+	fatal: invalid value for '\''--disk-usage'\'': '\''typo'\'', try --disk-usage=human
     ++	fatal: invalid value for '\''--disk-usage=<format>'\'': '\''typo'\'', the only allowed format is '\''human'\''
      +	EOF
      +	test_cmp err expect
      +'
 Documentation/rev-list-options.txt |  3 +++
 builtin/rev-list.c                 | 36 ++++++++++++++++++++++++++----
 t/t6115-rev-list-du.sh             | 22 ++++++++++++++++++
 3 files changed, 57 insertions(+), 4 deletions(-)
Documentation/rev-list-options.txt +3 -0
index 195e74eec63..bd08d18576f 100644
@@ -242,6 +242,7 @@ ifdef::git-rev-list[]
 	to `/dev/null` as the output does not have to be formatted.
 
 --disk-usage::
+--disk-usage=human::
 	Suppress normal output; instead, print the sum of the bytes used
 	for on-disk storage by the selected commits or objects. This is
 	equivalent to piping the output into `git cat-file

@@ -249,6 +250,8 @@ ifdef::git-rev-list[]
 	faster (especially with `--use-bitmap-index`). See the `CAVEATS`
 	section in linkgit:git-cat-file[1] for the limitations of what
 	"on-disk storage" means.
+	With the optional value `human`, on-disk storage size is shown
+	in human-readable string(e.g. 12.24 Kib, 3.50 Mib).
 endif::git-rev-list[]
 
 --cherry-mark::
builtin/rev-list.c +32 -4
index 30fd8e83eaf..fba6f5d51f3 100644
@@ -46,6 +46,7 @@ static const char rev_list_usage[] =
 "    --parents\n"
 "    --children\n"
 "    --objects | --objects-edge\n"
+"    --disk-usage[=human]\n"
 "    --unpacked\n"
 "    --header | --pretty\n"
 "    --[no-]object-names\n"

@@ -81,6 +82,7 @@ static int arg_show_object_names = 1;
 
 static int show_disk_usage;
 static off_t total_disk_usage;
+static int human_readable;
 
 static off_t get_object_disk_usage(struct object *obj)
 {

@@ -368,6 +370,17 @@ static int show_object_fast(
 	return 1;
 }
 
+static void print_disk_usage(off_t size)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (human_readable)
+		strbuf_humanise_bytes(&sb, size);
+	else
+		strbuf_addf(&sb, "%"PRIuMAX, (uintmax_t)size);
+	puts(sb.buf);
+	strbuf_release(&sb);
+}
+
 static inline int parse_missing_action_value(const char *value)
 {
 	if (!strcmp(value, "error")) {

@@ -473,6 +486,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs,
 				 int filter_provided_objects)
 {
 	struct bitmap_index *bitmap_git;
+	off_t size_from_bitmap;
 
 	if (!show_disk_usage)
 		return -1;

@@ -481,8 +495,8 @@ 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);
+	print_disk_usage(size_from_bitmap);
 	return 0;
 }
 

@@ -624,7 +638,21 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
-		if (!strcmp(arg, "--disk-usage")) {
+		if (skip_prefix(arg, "--disk-usage", &arg)) {
+			if (*arg == '=') {
+				if (!strcmp(++arg, "human")) {
+					human_readable = 1;
+				} else
+					die(_("invalid value for '%s': '%s', the only allowed format is '%s'"),
+					    "--disk-usage=<format>", arg, "human");
+			} else if (*arg) {
+				/*
+				 * Arguably should goto a label to continue chain of ifs?
+				 * Doesn't matter unless we try to add --disk-usage-foo
+				 * afterwards.
+				 */
+				usage(rev_list_usage);
+			}
 			show_disk_usage = 1;
 			info.flags |= REV_LIST_QUIET;
 			continue;

@@ -753,7 +781,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	}
 
 	if (show_disk_usage)
-		printf("%"PRIuMAX"\n", (uintmax_t)total_disk_usage);
+		print_disk_usage(total_disk_usage);
 
 cleanup:
 	release_revisions(&revs);
t/t6115-rev-list-du.sh +22 -0
index b4aef32b713..d59111dedec 100755
@@ -48,4 +48,26 @@ check_du HEAD
 check_du --objects HEAD
 check_du --objects HEAD^..HEAD
 
+# As mentioned above, don't use hardcode sizes as actual size, but use the
+# output from git cat-file.
+test_expect_success 'rev-list --disk-usage=human' '
+	git rev-list --objects HEAD --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list --disk-usage=human with bitmaps' '
+	git rev-list --objects HEAD --use-bitmap-index --disk-usage=human >actual &&
+	disk_usage_slow --objects HEAD >actual_size &&
+	grep "$(cat actual_size) bytes" actual
+'
+
+test_expect_success 'rev-list use --disk-usage unproperly' '
+	test_must_fail git rev-list --objects HEAD --disk-usage=typo 2>err &&
+	cat >expect <<-\EOF &&
+	fatal: invalid value for '\''--disk-usage=<format>'\'': '\''typo'\'', the only allowed format is '\''human'\''
+	EOF
+	test_cmp err expect
+'
+
 test_done

base-commit: 679aad9e82d0dfd8ef3d1f98fa4629665496cec9

Signature · 1 line
gitgitgadget
#16 lore raw

Re: [PATCH v5] rev-list: support human-readable output for `--disk-usage`

Quoted 7 lines · Li Linchao via GitGitGadget

From: Li Linchao <[email protected]>

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 resulting number is quit hard for a human to read.

Quoted 5 lines

Teach git rev-list to output a human readable result when using '--disk-usage'.

Signed-off-by Li Linchao <[email protected]>

---

Thanks, queued.