threads / patch / 33350

patchcount-objects: output "KiB" instead of "kilobytes"

Subject: [PATCH] count-objects: output "KiB" instead of "kilobytes"

## tl;dr

21 messages between Apr 2, 2013 and Apr 10, 2013. Diffs are folded; open one to read it.

replies: 20people: 5as markdown or json

Mihai Capotă· Apr 2, 2013, 11:43 UTC · lore
The code uses division by 1024. Also, the manual uses "KiB".
Signed-off-by: Mihai Capotă <mihai@mihaic.ro>
---
 builtin/count-objects.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/count-objects.c +1 −1
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9afaa88..ecc13b0 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -124,7 +124,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		printf("garbage: %lu\n", garbage);
 	}
 	else
-		printf("%lu objects, %lu kilobytes\n",
+		printf("%lu objects, %lu KiB\n",
 		       loose, (unsigned long) (loose_size / 1024));
 	return 0;
 }
-- 
1.7.9.5
Junio C Hamano· Apr 2, 2013, 17:41 UTC · re: Mihai Capotă · lore

Re: [PATCH] count-objects: output "KiB" instead of "kilobytes"

Mihai Capotă <mihai@mihaic.ro> writes:
Show 20 quoted lines
> The code uses division by 1024. Also, the manual uses "KiB".
>
> Signed-off-by: Mihai Capotă <mihai@mihaic.ro>
> ---
>  builtin/count-objects.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index 9afaa88..ecc13b0 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -124,7 +124,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
>  		printf("garbage: %lu\n", garbage);
>  	}
>  	else
> -		printf("%lu objects, %lu kilobytes\n",
> +		printf("%lu objects, %lu KiB\n",
>  		       loose, (unsigned long) (loose_size / 1024));
>  	return 0;
>  }
I guess nobody reads this in scripts, so it should be OK.
Will queue. Thanks.
Junio C Hamano· Apr 2, 2013, 22:01 UTC · re: Mihai Capotă · lore

Re: [PATCH] count-objects: output "KiB" instead of "kilobytes"

Mihai Capotă <mihai@mihaic.ro> writes:
Show 20 quoted lines
> The code uses division by 1024. Also, the manual uses "KiB".
>
> Signed-off-by: Mihai Capotă <mihai@mihaic.ro>
> ---
>  builtin/count-objects.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index 9afaa88..ecc13b0 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -124,7 +124,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
>  		printf("garbage: %lu\n", garbage);
>  	}
>  	else
> -		printf("%lu objects, %lu kilobytes\n",
> +		printf("%lu objects, %lu KiB\n",
>  		       loose, (unsigned long) (loose_size / 1024));
>  	return 0;
>  }

This breaks existing tests (5301, 7408 and 5700); I noticed it too late and wasted 20 minutes, having to re-run today's integration cycle.

Next time, please run the testsuite before sending a patch.
Mihai Capotă· Apr 3, 2013, 06:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] count-objects: output "KiB" instead of "kilobytes"

I'm really sorry about that. I'll make sure to run the tests before sending patches in the future.

On Wed, Apr 3, 2013 at 12:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> Mihai Capotă <mihai@mihaic.ro> writes:
>
>> The code uses division by 1024. Also, the manual uses "KiB".
>>
>> Signed-off-by: Mihai Capotă <mihai@mihaic.ro>
>> ---
>>  builtin/count-objects.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
>> index 9afaa88..ecc13b0 100644
>> --- a/builtin/count-objects.c
>> +++ b/builtin/count-objects.c
>> @@ -124,7 +124,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
>>               printf("garbage: %lu\n", garbage);
>>       }
>>       else
>> -             printf("%lu objects, %lu kilobytes\n",
>> +             printf("%lu objects, %lu KiB\n",
>>                      loose, (unsigned long) (loose_size / 1024));
>>       return 0;
>>  }
>
> This breaks existing tests (5301, 7408 and 5700); I noticed it too
> late and wasted 20 minutes, having to re-run today's integration
> cycle.
>
> Next time, please run the testsuite before sending a patch.
Mihai Capotă· Apr 3, 2013, 12:48 UTC · re: Junio C Hamano · lore

[PATCH v2] count-objects: output "KiB" instead of "kilobytes"

The code uses division by 1024. The master branch count-objects manual also uses "KiB".

Also updated the code that reads count-objects output (t5301, t5700, t7408, and git-cvsimport) and the Git User's Manual.

Signed-off-by: Mihai Capotă <mihai@mihaic.ro>
---
 Documentation/user-manual.txt  |    4 ++--
 builtin/count-objects.c        |    2 +-
 git-cvsimport.perl             |    8 ++++----
 t/t5301-sliding-window.sh      |    4 ++--
 t/t5700-clone-reference.sh     |    4 ++--
 t/t7408-submodule-reference.sh |    4 ++--
 6 files changed, 13 insertions(+), 13 deletions(-)
Show changes to 6 files +13 −13

Documentation/user-manual.txt, builtin/count-objects.c, git-cvsimport.perl, t/t5301-sliding-window.sh, t/t5700-clone-reference.sh, t/t7408-submodule-reference.sh

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index e831cc2..b61a09c 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -3175,7 +3175,7 @@ lot of objects.  Try this on an old project:
 
 ------------------------------------------------
 $ git count-objects
-6930 objects, 47620 kilobytes
+6930 objects, 47620 KiB
 ------------------------------------------------
 
 The first number is the number of objects which are kept in
@@ -3215,7 +3215,7 @@ You can verify that the loose objects are gone by looking at the
 
 ------------------------------------------------
 $ git count-objects
-0 objects, 0 kilobytes
+0 objects, 0 KiB
 ------------------------------------------------
 
 Although the object files are gone, any commands that refer to those
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 9afaa88..ecc13b0 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -124,7 +124,7 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		printf("garbage: %lu\n", garbage);
 	}
 	else
-		printf("%lu objects, %lu kilobytes\n",
+		printf("%lu objects, %lu KiB\n",
 		       loose, (unsigned long) (loose_size / 1024));
 	return 0;
 }
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 73d367c..de44e33 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -1126,12 +1126,12 @@ unless ($opt_P) {
 }
 
 # The heuristic of repacking every 1024 commits can leave a
-# lot of unpacked data.  If there is more than 1MB worth of
+# lot of unpacked data.  If there is more than 1MiB worth of
 # not-packed objects, repack once more.
 my $line = `git count-objects`;
-if ($line =~ /^(\d+) objects, (\d+) kilobytes$/) {
-  my ($n_objects, $kb) = ($1, $2);
-  1024 < $kb
+if ($line =~ /^(\d+) objects, (\d+) KiB$/) {
+  my ($n_objects, $kib) = ($1, $2);
+  1024 < $kib
     and system(qw(git repack -a -d));
 }
 
diff --git a/t/t5301-sliding-window.sh b/t/t5301-sliding-window.sh
index 2fc5af6..37931d2 100755
--- a/t/t5301-sliding-window.sh
+++ b/t/t5301-sliding-window.sh
@@ -20,7 +20,7 @@ test_expect_success \
      commit1=`git commit-tree $tree </dev/null` &&
      git update-ref HEAD $commit1 &&
      git repack -a -d &&
-     test "`git count-objects`" = "0 objects, 0 kilobytes" &&
+     test "`git count-objects`" = "0 objects, 0 KiB" &&
      pack1=`ls .git/objects/pack/*.pack` &&
      test -f "$pack1"'
 
@@ -46,7 +46,7 @@ test_expect_success \
      commit2=`git commit-tree $tree -p $commit1 </dev/null` &&
      git update-ref HEAD $commit2 &&
      git repack -a -d &&
-     test "`git count-objects`" = "0 objects, 0 kilobytes" &&
+     test "`git count-objects`" = "0 objects, 0 KiB" &&
      pack2=`ls .git/objects/pack/*.pack` &&
      test -f "$pack2" &&
      test "$pack1" \!= "$pack2"'
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index c47d450..e5cfd6a 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -46,7 +46,7 @@ cd "$base_dir"
 
 test_expect_success 'that reference gets used' \
 'cd C &&
-echo "0 objects, 0 kilobytes" > expected &&
+echo "0 objects, 0 KiB" > expected &&
 git count-objects > current &&
 test_cmp expected current'
 
@@ -73,7 +73,7 @@ test_expect_success 'pulling from reference' \
 cd "$base_dir"
 
 test_expect_success 'that reference gets used' \
-'cd D && echo "0 objects, 0 kilobytes" > expected &&
+'cd D && echo "0 objects, 0 KiB" > expected &&
 git count-objects > current &&
 test_cmp expected current'
 
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index b770b2f..aeface6 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -49,7 +49,7 @@ cd "$base_dir"
 
 test_expect_success 'that reference gets used with add' \
 'cd super/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
+echo "0 objects, 0 KiB" > expected &&
 git count-objects > current &&
 diff expected current'
 
@@ -72,7 +72,7 @@ cd "$base_dir"
 
 test_expect_success 'that reference gets used with update' \
 'cd super-clone/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
+echo "0 objects, 0 KiB" > expected &&
 git count-objects > current &&
 diff expected current'
 
-- 
1.7.9.5
Junio C Hamano· Apr 3, 2013, 14:38 UTC · re: Mihai Capotă · lore

Re: [PATCH v2] count-objects: output "KiB" instead of "kilobytes"

Mihai Capotă <mihai@mihaic.ro> writes:
Show 38 quoted lines
> The code uses division by 1024. The master branch count-objects manual also
> uses "KiB".
>
> Also updated the code that reads count-objects output (t5301, t5700, t7408, and
> git-cvsimport) and the Git User's Manual.
>
> Signed-off-by: Mihai Capotă <mihai@mihaic.ro>
> ---
>  Documentation/user-manual.txt  |    4 ++--
>  builtin/count-objects.c        |    2 +-
>  git-cvsimport.perl             |    8 ++++----
>  t/t5301-sliding-window.sh      |    4 ++--
>  t/t5700-clone-reference.sh     |    4 ++--
>  t/t7408-submodule-reference.sh |    4 ++--
>  6 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
> index e831cc2..b61a09c 100644
> --- a/Documentation/user-manual.txt
> +++ b/Documentation/user-manual.txt
> @@ -3175,7 +3175,7 @@ lot of objects.  Try this on an old project:
>  
>  ------------------------------------------------
>  $ git count-objects
> -6930 objects, 47620 kilobytes
> +6930 objects, 47620 KiB
>  ------------------------------------------------
>  
>  The first number is the number of objects which are kept in
> @@ -3215,7 +3215,7 @@ You can verify that the loose objects are gone by looking at the
>  
>  ------------------------------------------------
>  $ git count-objects
> -0 objects, 0 kilobytes
> +0 objects, 0 KiB
>  ------------------------------------------------
>  
>  Although the object files are gone, any commands that refer to those

It is good to see the patch being thorough, adjusting even documentation.

Show 20 quoted lines
> diff --git a/git-cvsimport.perl b/git-cvsimport.perl
> index 73d367c..de44e33 100755
> --- a/git-cvsimport.perl
> +++ b/git-cvsimport.perl
> @@ -1126,12 +1126,12 @@ unless ($opt_P) {
>  }
>  
>  # The heuristic of repacking every 1024 commits can leave a
> -# lot of unpacked data.  If there is more than 1MB worth of
> +# lot of unpacked data.  If there is more than 1MiB worth of
>  # not-packed objects, repack once more.
>  my $line = `git count-objects`;
> -if ($line =~ /^(\d+) objects, (\d+) kilobytes$/) {
> -  my ($n_objects, $kb) = ($1, $2);
> -  1024 < $kb
> +if ($line =~ /^(\d+) objects, (\d+) KiB$/) {
> +  my ($n_objects, $kib) = ($1, $2);
> +  1024 < $kib
>      and system(qw(git repack -a -d));
>  }

This hunk makes me wonder if this s/kilobytes/kib/ is a good idea in the first place. This in-tree user was lucky enough to have been caught and adjusted, but we don't know how many out-of-tree scripts are broken the same way and in need of a similar treatment.

Mihai Capotă· Apr 4, 2013, 13:18 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] count-objects: output "KiB" instead of "kilobytes"

On Wed, Apr 3, 2013 at 4:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> Mihai Capotă <mihai@mihaic.ro> writes:
>> diff --git a/git-cvsimport.perl b/git-cvsimport.perl
>> index 73d367c..de44e33 100755
>> --- a/git-cvsimport.perl
>> +++ b/git-cvsimport.perl
>> @@ -1126,12 +1126,12 @@ unless ($opt_P) {
>>  }
>>
>>  # The heuristic of repacking every 1024 commits can leave a
>> -# lot of unpacked data.  If there is more than 1MB worth of
>> +# lot of unpacked data.  If there is more than 1MiB worth of
>>  # not-packed objects, repack once more.
>>  my $line = `git count-objects`;
>> -if ($line =~ /^(\d+) objects, (\d+) kilobytes$/) {
>> -  my ($n_objects, $kb) = ($1, $2);
>> -  1024 < $kb
>> +if ($line =~ /^(\d+) objects, (\d+) KiB$/) {
>> +  my ($n_objects, $kib) = ($1, $2);
>> +  1024 < $kib
>>      and system(qw(git repack -a -d));
>>  }
>
> This hunk makes me wonder if this s/kilobytes/kib/ is a good idea in
> the first place.  This in-tree user was lucky enough to have been
> caught and adjusted, but we don't know how many out-of-tree scripts
> are broken the same way and in need of a similar treatment.

The git manual contains an explicit warning about the output of a porcelain command changing: "The interface to Porcelain commands on the other hand are subject to change in order to improve the end user experience."

Junio C Hamano· Apr 4, 2013, 16:27 UTC · re: Mihai Capotă · lore

Re: [PATCH v2] count-objects: output "KiB" instead of "kilobytes"

Mihai Capotă <mihai@mihaic.ro> writes:
> The git manual contains an explicit warning about the output of a
> porcelain command changing: "The interface to Porcelain commands on
> the other hand are subject to change in order to improve the end user
> experience."
Yeah, I know that, as I wrote it ;-)

Aside from count-object being not exactly a Porcelain, the statement does not give us a blank check to make random changes as we see fit. There needs to be a clear improvement.

I am just having a hard time weighing the benefit of using more accurate kibibytes over kilobytes and the possible downside of breaking other peoples' tools.

Perhaps it would be alright if the change was accompanied by a warning in the Release Notes to say something like:

        If you have scripts that decide when to run "git repack" by
	parsing the output from "git count-objects", this release
	may break them.  Sorry about that.  One of the scripts
	shipped by git-core itself also had to be adjusted.  The
	command reports the total diskspace used to store loose
	objects in kibibytes, but it was labelled as "kilobytes".
	The number now is shown with "KiB", e.g. "6750 objects,
	50928 KiB".
	You may want to consider updating such scripts to always
	call "git gc --auto" to let it decide when to repack for
	you.

Also, I suspect that for the purpose of this exact output field, nobody cares the difference between kibibytes and kilobytes. Depending on the system, we add up either st.st_blocks or st.st_size and the result is not that exact as "how much diskspace is consumed".

Mihai Capotă· Apr 5, 2013, 09:38 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] count-objects: output "KiB" instead of "kilobytes"

On Thu, Apr 4, 2013 at 6:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> Mihai Capotă <mihai@mihaic.ro> writes:
>
>> The git manual contains an explicit warning about the output of a
>> porcelain command changing: "The interface to Porcelain commands on
>> the other hand are subject to change in order to improve the end user
>> experience."
>
> Yeah, I know that, as I wrote it ;-)
>
> Aside from count-object being not exactly a Porcelain, the statement
> does not give us a blank check to make random changes as we see fit.
> There needs to be a clear improvement.
>
> I am just having a hard time weighing the benefit of using more
> accurate kibibytes over kilobytes and the possible downside of
> breaking other peoples' tools.
>
> Perhaps it would be alright if the change was accompanied by a
> warning in the Release Notes to say something like:
>
>         If you have scripts that decide when to run "git repack" by
>         parsing the output from "git count-objects", this release
>         may break them.  Sorry about that.  One of the scripts
>         shipped by git-core itself also had to be adjusted.  The
>         command reports the total diskspace used to store loose
>         objects in kibibytes, but it was labelled as "kilobytes".
>         The number now is shown with "KiB", e.g. "6750 objects,
>         50928 KiB".
>
>         You may want to consider updating such scripts to always
>         call "git gc --auto" to let it decide when to repack for
>         you.
>
> Also, I suspect that for the purpose of this exact output field,
> nobody cares the difference between kibibytes and kilobytes.
> Depending on the system, we add up either st.st_blocks or st.st_size
> and the result is not that exact as "how much diskspace is
> consumed".

I agree completely. I think the release notes warning is a good plan. Just in case you decide against it, I'm also sending a completely different patch to document the issue.

Mihai
Mihai Capotă· Apr 5, 2013, 09:39 UTC · re: Junio C Hamano · lore

[PATCH] count-objects doc: document use of kibibytes

Document the use of kibibytes, not kilobytes, in the output of count-objects and the reason for not correcting the output.

Also, make cvsimport comment and variable name reflect unit actually used.
Signed-off-by: Mihai Capotă <mihai@mihaic.ro>
---
 Documentation/git-count-objects.txt |    7 +++++++
 git-cvsimport.perl                  |    6 +++---
 2 files changed, 10 insertions(+), 3 deletions(-)
Show changes to 2 files +10 −3

Documentation/git-count-objects.txt, git-cvsimport.perl

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index 23c80ce..d562dad 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -26,6 +26,13 @@ OPTIONS
 	and number of objects that can be removed by running
 	`git prune-packed`.
 
+
+BUGS
+----
+Consumed space is actually expressed in kibibytes, not kilobytes as stated in
+the output. The output is kept as it is for compatibility reasons.
+
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 73d367c..6803f04 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -1126,12 +1126,12 @@ unless ($opt_P) {
 }
 
 # The heuristic of repacking every 1024 commits can leave a
-# lot of unpacked data.  If there is more than 1MB worth of
+# lot of unpacked data.  If there is more than 1MiB worth of
 # not-packed objects, repack once more.
 my $line = `git count-objects`;
 if ($line =~ /^(\d+) objects, (\d+) kilobytes$/) {
-  my ($n_objects, $kb) = ($1, $2);
-  1024 < $kb
+  my ($n_objects, $kib) = ($1, $2);
+  1024 < $kib
     and system(qw(git repack -a -d));
 }
 
-- 
1.7.9.5
Antoine Pelisse· Apr 5, 2013, 20:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] count-objects: output "KiB" instead of "kilobytes"

Should we use that opportunity to implement an option like -h (for humanize) similar to what ls(1), df(1), du(1) does ? Of course "-h" is already used for help, so we could use -H or any other sensible choice. It can become tough to read the size when it gets big enough.

On Thu, Apr 4, 2013 at 6:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 42 quoted lines
> Mihai Capotă <mihai@mihaic.ro> writes:
>
>> The git manual contains an explicit warning about the output of a
>> porcelain command changing: "The interface to Porcelain commands on
>> the other hand are subject to change in order to improve the end user
>> experience."
>
> Yeah, I know that, as I wrote it ;-)
>
> Aside from count-object being not exactly a Porcelain, the statement
> does not give us a blank check to make random changes as we see fit.
> There needs to be a clear improvement.
>
> I am just having a hard time weighing the benefit of using more
> accurate kibibytes over kilobytes and the possible downside of
> breaking other peoples' tools.
>
> Perhaps it would be alright if the change was accompanied by a
> warning in the Release Notes to say something like:
>
>         If you have scripts that decide when to run "git repack" by
>         parsing the output from "git count-objects", this release
>         may break them.  Sorry about that.  One of the scripts
>         shipped by git-core itself also had to be adjusted.  The
>         command reports the total diskspace used to store loose
>         objects in kibibytes, but it was labelled as "kilobytes".
>         The number now is shown with "KiB", e.g. "6750 objects,
>         50928 KiB".
>
>         You may want to consider updating such scripts to always
>         call "git gc --auto" to let it decide when to repack for
>         you.
>
> Also, I suspect that for the purpose of this exact output field,
> nobody cares the difference between kibibytes and kilobytes.
> Depending on the system, we add up either st.st_blocks or st.st_size
> and the result is not that exact as "how much diskspace is
> consumed".
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antoine Pelisse· Apr 8, 2013, 18:18 UTC · re: Antoine Pelisse · lore

[PATCH 1/2] progress: create public humanize() to show sizes

Currently, humanization of downloaded size is done in the same function as text formatting. This is an issue if anyone else wants to use this.

Separate text formatting from size simplification and make the function public so that it can easily be used by other clients.

We now can use humanize() for both downloaded size and download speed calculation. One of the drawbacks is that speed will no look like this when download is stalled: "0 bytes/s" instead of "0 KiB/s".

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 progress.c |   60 ++++++++++++++++++++++++++++++++++--------------------------
 progress.h |    2 ++
 2 files changed, 36 insertions(+), 26 deletions(-)
Show changes to 2 files +36 −26

progress.c, progress.h

diff --git a/progress.c b/progress.c
index 3971f49..76c1e42 100644
--- a/progress.c
+++ b/progress.c
@@ -8,8 +8,11 @@
  * published by the Free Software Foundation.
  */
 
+#include <string.h>
+
 #include "git-compat-util.h"
 #include "progress.h"
+#include "strbuf.h"
 
 #define TP_IDX_MAX      8
 
@@ -112,34 +115,33 @@ static int display(struct progress *progress, unsigned n, const char *done)
 	return 0;
 }
 
-static void throughput_string(struct throughput *tp, off_t total,
-			      unsigned int rate)
+void humanize(struct strbuf *buf, off_t bytes)
 {
-	int l = sizeof(tp->display);
-	if (total > 1 << 30) {
-		l -= snprintf(tp->display, l, ", %u.%2.2u GiB",
-			      (int)(total >> 30),
-			      (int)(total & ((1 << 30) - 1)) / 10737419);
-	} else if (total > 1 << 20) {
-		int x = total + 5243;  /* for rounding */
-		l -= snprintf(tp->display, l, ", %u.%2.2u MiB",
-			      x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
-	} else if (total > 1 << 10) {
-		int x = total + 5;  /* for rounding */
-		l -= snprintf(tp->display, l, ", %u.%2.2u KiB",
-			      x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
+	if (bytes > 1 << 30) {
+		strbuf_addf(buf, "%u.%2.2u GiB",
+			    (int)(bytes >> 30),
+			    (int)(bytes & ((1 << 30) - 1)) / 10737419);
+	} else if (bytes > 1 << 20) {
+		int x = bytes + 5243;  /* for rounding */
+		strbuf_addf(buf, "%u.%2.2u MiB",
+			    x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
+	} else if (bytes > 1 << 10) {
+		int x = bytes + 5;  /* for rounding */
+		strbuf_addf(buf, "%u.%2.2u KiB",
+			    x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
 	} else {
-		l -= snprintf(tp->display, l, ", %u bytes", (int)total);
+		strbuf_addf(buf, "%u bytes", (int)bytes);
 	}
+}
 
-	if (rate > 1 << 10) {
-		int x = rate + 5;  /* for rounding */
-		snprintf(tp->display + sizeof(tp->display) - l, l,
-			 " | %u.%2.2u MiB/s",
-			 x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
-	} else if (rate)
-		snprintf(tp->display + sizeof(tp->display) - l, l,
-			 " | %u KiB/s", rate);
+static void throughput_string(struct strbuf *buf, off_t total,
+			      unsigned int rate)
+{
+	strbuf_addstr(buf, ", ");
+	humanize(buf, total);
+	strbuf_addstr(buf, " | ");
+	humanize(buf, rate * 1024);
+	strbuf_addstr(buf, "/s");
 }
 
 void display_throughput(struct progress *progress, off_t total)
@@ -183,6 +185,7 @@ void display_throughput(struct progress *progress, off_t total)
 	misecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;
 
 	if (misecs > 512) {
+		struct strbuf buf = STRBUF_INIT;
 		unsigned int count, rate;
 
 		count = total - tp->prev_total;
@@ -197,7 +200,9 @@ void display_throughput(struct progress *progress, off_t total)
 		tp->last_misecs[tp->idx] = misecs;
 		tp->idx = (tp->idx + 1) % TP_IDX_MAX;
 
-		throughput_string(tp, total, rate);
+		throughput_string(&buf, total, rate);
+		strncpy(tp->display, buf.buf, sizeof(tp->display));
+		strbuf_release(&buf);
 		if (progress->last_value != -1 && progress_update)
 			display(progress, progress->last_value, NULL);
 	}
@@ -253,9 +258,12 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 
 		bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
 		if (tp) {
+			struct strbuf strbuf = STRBUF_INIT;
 			unsigned int rate = !tp->avg_misecs ? 0 :
 					tp->avg_bytes / tp->avg_misecs;
-			throughput_string(tp, tp->curr_total, rate);
+			throughput_string(&strbuf, tp->curr_total, rate);
+			strncpy(tp->display, strbuf.buf, sizeof(tp->display));
+			strbuf_release(&strbuf);
 		}
 		progress_update = 1;
 		sprintf(bufp, ", %s.\n", msg);
diff --git a/progress.h b/progress.h
index 611e4c4..0e70f55 100644
--- a/progress.h
+++ b/progress.h
@@ -2,7 +2,9 @@
 #define PROGRESS_H
 
 struct progress;
+struct strbuf;
 
+void humanize(struct strbuf *buf, off_t bytes);
 void display_throughput(struct progress *progress, off_t total);
 int display_progress(struct progress *progress, unsigned n);
 struct progress *start_progress(const char *title, unsigned total);
-- 
1.7.9.5
Antoine Pelisse· Apr 8, 2013, 18:18 UTC · re: Antoine Pelisse · lore

[PATCH 2/2] count-objects: add -H option to humanize sizes

Use the new humanize() function to print loose objects size, pack size, and garbage size in verbose mode, or loose objects size in regular mode. This patch doesn't change the way anything is displayed when the option is not used.

Also update the documentation.
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 Documentation/git-count-objects.txt |   14 ++++++++---
 builtin/count-objects.c             |   47 +++++++++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 12 deletions(-)
Show changes to 2 files +49 −12

Documentation/git-count-objects.txt, builtin/count-objects.c

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index da6e72e..b300e84 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -8,7 +8,7 @@ git-count-objects - Count unpacked number of objects and their disk consumption
 SYNOPSIS
 --------
 [verse]
-'git count-objects' [-v]
+'git count-objects' [-v] [-H | --human-readable]
 
 DESCRIPTION
 -----------
@@ -24,11 +24,11 @@ OPTIONS
 +
 count: the number of loose objects
 +
-size: disk space consumed by loose objects, in KiB
+size: disk space consumed by loose objects, in KiB (unless -H is specified)
 +
 in-pack: the number of in-pack objects
 +
-size-pack: disk space consumed by the packs, in KiB
+size-pack: disk space consumed by the packs, in KiB (unless -H is specified)
 +
 prune-packable: the number of loose objects that are also present in
 the packs. These objects could be pruned using `git prune-packed`.
@@ -36,7 +36,13 @@ the packs. These objects could be pruned using `git prune-packed`.
 garbage: the number of files in object database that are not valid
 loose objects nor valid packs
 +
-size-garbage: disk space consumed by garbage files, in KiB
+size-garbage: disk space consumed by garbage files, in KiB (unless -H is
+specified)
+
+-H::
+--human-readable::
+
+Print sizes in human readable format
 
 GIT
 ---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 0343e35..9836f6a 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -8,6 +8,7 @@
 #include "dir.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "progress.h"
 
 static unsigned long garbage;
 static off_t size_garbage;
@@ -79,13 +80,13 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
 }
 
 static char const * const count_objects_usage[] = {
-	N_("git count-objects [-v]"),
+	N_("git count-objects [-v] [-H | --human-readable]"),
 	NULL
 };
 
 int cmd_count_objects(int argc, const char **argv, const char *prefix)
 {
-	int i, verbose = 0;
+	int i, verbose = 0, human_readable = 0;
 	const char *objdir = get_object_directory();
 	int len = strlen(objdir);
 	char *path = xmalloc(len + 50);
@@ -93,6 +94,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	off_t loose_size = 0;
 	struct option opts[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
+		OPT_BOOLEAN('H', "human-readable", &human_readable,
+			    N_("print sizes in human readable format")),
 		OPT_END(),
 	};
 
@@ -119,6 +122,9 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		struct packed_git *p;
 		unsigned long num_pack = 0;
 		off_t size_pack = 0;
+		struct strbuf loose_buf = STRBUF_INIT;
+		struct strbuf pack_buf = STRBUF_INIT;
+		struct strbuf garbage_buf = STRBUF_INIT;
 		if (!packed_git)
 			prepare_packed_git();
 		for (p = packed_git; p; p = p->next) {
@@ -130,17 +136,42 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 			size_pack += p->pack_size + p->index_size;
 			num_pack++;
 		}
+
+		if (human_readable) {
+			humanize(&loose_buf, loose_size);
+			humanize(&pack_buf, size_pack);
+			humanize(&garbage_buf, size_garbage);
+		}
+		else {
+			strbuf_addf(&loose_buf, "%lu",
+				    (unsigned long)(loose_size / 1024));
+			strbuf_addf(&pack_buf, "%lu",
+				    (unsigned long)(size_pack / 1024));
+			strbuf_addf(&garbage_buf, "%lu",
+				    (unsigned long)(size_garbage / 1024));
+		}
+
 		printf("count: %lu\n", loose);
-		printf("size: %lu\n", (unsigned long) (loose_size / 1024));
+		printf("size: %s\n", loose_buf.buf);
 		printf("in-pack: %lu\n", packed);
 		printf("packs: %lu\n", num_pack);
-		printf("size-pack: %lu\n", (unsigned long) (size_pack / 1024));
+		printf("size-pack: %s\n", pack_buf.buf);
 		printf("prune-packable: %lu\n", packed_loose);
 		printf("garbage: %lu\n", garbage);
-		printf("size-garbage: %lu\n", (unsigned long) (size_garbage / 1024));
+		printf("size-garbage: %s\n", garbage_buf.buf);
+		strbuf_release(&loose_buf);
+		strbuf_release(&pack_buf);
+		strbuf_release(&garbage_buf);
+	}
+	else {
+		struct strbuf buf = STRBUF_INIT;
+		if (human_readable)
+			humanize(&buf, loose_size);
+		else
+			strbuf_addf(&buf, "%lu KiB",
+				    (unsigned long)(loose_size / 1024));
+		printf("%lu objects, %s\n", loose, buf.buf);
+		strbuf_release(&buf);
 	}
-	else
-		printf("%lu objects, %lu KiB\n",
-		       loose, (unsigned long) (loose_size / 1024));
 	return 0;
 }
-- 
1.7.9.5
Junio C Hamano· Apr 8, 2013, 21:40 UTC · re: Antoine Pelisse · lore

Re: [PATCH 1/2] progress: create public humanize() to show sizes

Antoine Pelisse <apelisse@gmail.com> writes:
Show 12 quoted lines
> Currently, humanization of downloaded size is done in the same function
> as text formatting. This is an issue if anyone else wants to use this.
>
> Separate text formatting from size simplification and make the function
> public so that it can easily be used by other clients.
>
> We now can use humanize() for both downloaded size and download speed
> calculation. One of the drawbacks is that speed will no look like this
> when download is stalled: "0 bytes/s" instead of "0 KiB/s".
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---

Sounds good, but I think this helper function should live in strbuf.[ch], where many other text/string helpers that take a strbuf as their first parameter live.

Show 14 quoted lines
>  progress.c |   60 ++++++++++++++++++++++++++++++++++--------------------------
>  progress.h |    2 ++
>  2 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 3971f49..76c1e42 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -8,8 +8,11 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <string.h>
> +
Please do not do this.

If you somehow need to have <string.h>, a suitable place should be found in git-compat-util.h; various platforms seem to have quirks with the ordering of system header files, and git-compat-util.h is meant to encapsulate them.

In fact, git-compat-util.h should already include it.
Show 111 quoted lines
>  #include "git-compat-util.h"
>  #include "progress.h"
> +#include "strbuf.h"
>  
>  #define TP_IDX_MAX      8
>  
> @@ -112,34 +115,33 @@ static int display(struct progress *progress, unsigned n, const char *done)
>  	return 0;
>  }
>  
> -static void throughput_string(struct throughput *tp, off_t total,
> -			      unsigned int rate)
> +void humanize(struct strbuf *buf, off_t bytes)
>  {
> -	int l = sizeof(tp->display);
> -	if (total > 1 << 30) {
> -		l -= snprintf(tp->display, l, ", %u.%2.2u GiB",
> -			      (int)(total >> 30),
> -			      (int)(total & ((1 << 30) - 1)) / 10737419);
> -	} else if (total > 1 << 20) {
> -		int x = total + 5243;  /* for rounding */
> -		l -= snprintf(tp->display, l, ", %u.%2.2u MiB",
> -			      x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
> -	} else if (total > 1 << 10) {
> -		int x = total + 5;  /* for rounding */
> -		l -= snprintf(tp->display, l, ", %u.%2.2u KiB",
> -			      x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> +	if (bytes > 1 << 30) {
> +		strbuf_addf(buf, "%u.%2.2u GiB",
> +			    (int)(bytes >> 30),
> +			    (int)(bytes & ((1 << 30) - 1)) / 10737419);
> +	} else if (bytes > 1 << 20) {
> +		int x = bytes + 5243;  /* for rounding */
> +		strbuf_addf(buf, "%u.%2.2u MiB",
> +			    x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
> +	} else if (bytes > 1 << 10) {
> +		int x = bytes + 5;  /* for rounding */
> +		strbuf_addf(buf, "%u.%2.2u KiB",
> +			    x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
>  	} else {
> -		l -= snprintf(tp->display, l, ", %u bytes", (int)total);
> +		strbuf_addf(buf, "%u bytes", (int)bytes);
>  	}
> +}
>  
> -	if (rate > 1 << 10) {
> -		int x = rate + 5;  /* for rounding */
> -		snprintf(tp->display + sizeof(tp->display) - l, l,
> -			 " | %u.%2.2u MiB/s",
> -			 x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> -	} else if (rate)
> -		snprintf(tp->display + sizeof(tp->display) - l, l,
> -			 " | %u KiB/s", rate);
> +static void throughput_string(struct strbuf *buf, off_t total,
> +			      unsigned int rate)
> +{
> +	strbuf_addstr(buf, ", ");
> +	humanize(buf, total);
> +	strbuf_addstr(buf, " | ");
> +	humanize(buf, rate * 1024);
> +	strbuf_addstr(buf, "/s");
>  }
>  
>  void display_throughput(struct progress *progress, off_t total)
> @@ -183,6 +185,7 @@ void display_throughput(struct progress *progress, off_t total)
>  	misecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;
>  
>  	if (misecs > 512) {
> +		struct strbuf buf = STRBUF_INIT;
>  		unsigned int count, rate;
>  
>  		count = total - tp->prev_total;
> @@ -197,7 +200,9 @@ void display_throughput(struct progress *progress, off_t total)
>  		tp->last_misecs[tp->idx] = misecs;
>  		tp->idx = (tp->idx + 1) % TP_IDX_MAX;
>  
> -		throughput_string(tp, total, rate);
> +		throughput_string(&buf, total, rate);
> +		strncpy(tp->display, buf.buf, sizeof(tp->display));
> +		strbuf_release(&buf);
>  		if (progress->last_value != -1 && progress_update)
>  			display(progress, progress->last_value, NULL);
>  	}
> @@ -253,9 +258,12 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
>  
>  		bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
>  		if (tp) {
> +			struct strbuf strbuf = STRBUF_INIT;
>  			unsigned int rate = !tp->avg_misecs ? 0 :
>  					tp->avg_bytes / tp->avg_misecs;
> -			throughput_string(tp, tp->curr_total, rate);
> +			throughput_string(&strbuf, tp->curr_total, rate);
> +			strncpy(tp->display, strbuf.buf, sizeof(tp->display));
> +			strbuf_release(&strbuf);
>  		}
>  		progress_update = 1;
>  		sprintf(bufp, ", %s.\n", msg);
> diff --git a/progress.h b/progress.h
> index 611e4c4..0e70f55 100644
> --- a/progress.h
> +++ b/progress.h
> @@ -2,7 +2,9 @@
>  #define PROGRESS_H
>  
>  struct progress;
> +struct strbuf;
>  
> +void humanize(struct strbuf *buf, off_t bytes);
>  void display_throughput(struct progress *progress, off_t total);
>  int display_progress(struct progress *progress, unsigned n);
>  struct progress *start_progress(const char *title, unsigned total);
Antoine Pelisse· Apr 10, 2013, 19:03 UTC · re: Junio C Hamano · lore

[PATCH 1/2] strbuf: create strbuf_humanize() to show byte sizes

Currently, humanization of downloaded size is done in the same function as text formatting in 'process.c'. This is an issue if anyone else wants to use this.

Separate text formatting from size simplification and make the function public in strbuf so that it can easily be used by other clients.

We now can use strbuf_humanize() for both downloaded size and download speed calculation. One of the drawbacks is that speed will now look like this when download is stalled: "0 bytes/s" instead of "0 KiB/s".

Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 Documentation/technical/api-strbuf.txt |    5 ++++
 progress.c                             |   43 +++++++++++---------------------
 strbuf.c                               |   19 ++++++++++++++
 strbuf.h                               |    1 +
 4 files changed, 40 insertions(+), 28 deletions(-)
Show changes to 4 files +40 −28

Documentation/technical/api-strbuf.txt, progress.c, strbuf.c, strbuf.h

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index 2c59cb2..7b6ecda 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -230,6 +230,11 @@ which can be used by the programmer of the callback as she sees fit.
 	destination. This is useful for literal data to be fed to either
 	strbuf_expand or to the *printf family of functions.
 
+`strbuf_humanize`::
+
+	Append the given byte size as a human-readable string (i.e. 12.23 KiB,
+	3.50 MiB).
+
 `strbuf_addf`::
 
 	Add a formatted string to the buffer.
diff --git a/progress.c b/progress.c
index 3971f49..8e09058 100644
--- a/progress.c
+++ b/progress.c
@@ -10,6 +10,7 @@
 
 #include "git-compat-util.h"
 #include "progress.h"
+#include "strbuf.h"
 
 #define TP_IDX_MAX      8
 
@@ -112,34 +113,14 @@ static int display(struct progress *progress, unsigned n, const char *done)
 	return 0;
 }
 
-static void throughput_string(struct throughput *tp, off_t total,
+static void throughput_string(struct strbuf *buf, off_t total,
 			      unsigned int rate)
 {
-	int l = sizeof(tp->display);
-	if (total > 1 << 30) {
-		l -= snprintf(tp->display, l, ", %u.%2.2u GiB",
-			      (int)(total >> 30),
-			      (int)(total & ((1 << 30) - 1)) / 10737419);
-	} else if (total > 1 << 20) {
-		int x = total + 5243;  /* for rounding */
-		l -= snprintf(tp->display, l, ", %u.%2.2u MiB",
-			      x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
-	} else if (total > 1 << 10) {
-		int x = total + 5;  /* for rounding */
-		l -= snprintf(tp->display, l, ", %u.%2.2u KiB",
-			      x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
-	} else {
-		l -= snprintf(tp->display, l, ", %u bytes", (int)total);
-	}
-
-	if (rate > 1 << 10) {
-		int x = rate + 5;  /* for rounding */
-		snprintf(tp->display + sizeof(tp->display) - l, l,
-			 " | %u.%2.2u MiB/s",
-			 x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
-	} else if (rate)
-		snprintf(tp->display + sizeof(tp->display) - l, l,
-			 " | %u KiB/s", rate);
+	strbuf_addstr(buf, ", ");
+	strbuf_humanize(buf, total);
+	strbuf_addstr(buf, " | ");
+	strbuf_humanize(buf, rate * 1024);
+	strbuf_addstr(buf, "/s");
 }
 
 void display_throughput(struct progress *progress, off_t total)
@@ -183,6 +164,7 @@ void display_throughput(struct progress *progress, off_t total)
 	misecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;
 
 	if (misecs > 512) {
+		struct strbuf buf = STRBUF_INIT;
 		unsigned int count, rate;
 
 		count = total - tp->prev_total;
@@ -197,7 +179,9 @@ void display_throughput(struct progress *progress, off_t total)
 		tp->last_misecs[tp->idx] = misecs;
 		tp->idx = (tp->idx + 1) % TP_IDX_MAX;
 
-		throughput_string(tp, total, rate);
+		throughput_string(&buf, total, rate);
+		strncpy(tp->display, buf.buf, sizeof(tp->display));
+		strbuf_release(&buf);
 		if (progress->last_value != -1 && progress_update)
 			display(progress, progress->last_value, NULL);
 	}
@@ -253,9 +237,12 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 
 		bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
 		if (tp) {
+			struct strbuf strbuf = STRBUF_INIT;
 			unsigned int rate = !tp->avg_misecs ? 0 :
 					tp->avg_bytes / tp->avg_misecs;
-			throughput_string(tp, tp->curr_total, rate);
+			throughput_string(&strbuf, tp->curr_total, rate);
+			strncpy(tp->display, strbuf.buf, sizeof(tp->display));
+			strbuf_release(&strbuf);
 		}
 		progress_update = 1;
 		sprintf(bufp, ", %s.\n", msg);
diff --git a/strbuf.c b/strbuf.c
index 48e9abb..8a50e66 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -528,6 +528,25 @@ void strbuf_addstr_urlencode(struct strbuf *sb, const char *s,
 	strbuf_add_urlencode(sb, s, strlen(s), reserved);
 }
 
+void strbuf_humanize(struct strbuf *buf, off_t bytes)
+{
+	if (bytes > 1 << 30) {
+		strbuf_addf(buf, "%u.%2.2u GiB",
+			    (int)(bytes >> 30),
+			    (int)(bytes & ((1 << 30) - 1)) / 10737419);
+	} else if (bytes > 1 << 20) {
+		int x = bytes + 5243;  /* for rounding */
+		strbuf_addf(buf, "%u.%2.2u MiB",
+			    x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
+	} else if (bytes > 1 << 10) {
+		int x = bytes + 5;  /* for rounding */
+		strbuf_addf(buf, "%u.%2.2u KiB",
+			    x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
+	} else {
+		strbuf_addf(buf, "%u bytes", (int)bytes);
+	}
+}
+
 int printf_ln(const char *fmt, ...)
 {
 	int ret;
diff --git a/strbuf.h b/strbuf.h
index 958822c..317c5a8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -170,6 +170,7 @@ extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
 extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
 				    int reserved);
+extern void strbuf_humanize(struct strbuf *buf, off_t bytes);
 
 __attribute__((format (printf,1,2)))
 extern int printf_ln(const char *fmt, ...);
-- 
1.7.9.5
Antoine Pelisse· Apr 10, 2013, 19:03 UTC · re: Antoine Pelisse · lore

[PATCH 2/2] count-objects: add -H option to humanize sizes

Use the new humanize() function to print loose objects size, pack size, and garbage size in verbose mode, or loose objects size in regular mode. This patch doesn't change the way anything is displayed when the option is not used.

Also update the documentation.
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
 Documentation/git-count-objects.txt |   14 ++++++++---
 builtin/count-objects.c             |   46 +++++++++++++++++++++++++++++------
 2 files changed, 48 insertions(+), 12 deletions(-)
Show changes to 2 files +48 −12

Documentation/git-count-objects.txt, builtin/count-objects.c

diff --git a/Documentation/git-count-objects.txt b/Documentation/git-count-objects.txt
index da6e72e..b300e84 100644
--- a/Documentation/git-count-objects.txt
+++ b/Documentation/git-count-objects.txt
@@ -8,7 +8,7 @@ git-count-objects - Count unpacked number of objects and their disk consumption
 SYNOPSIS
 --------
 [verse]
-'git count-objects' [-v]
+'git count-objects' [-v] [-H | --human-readable]
 
 DESCRIPTION
 -----------
@@ -24,11 +24,11 @@ OPTIONS
 +
 count: the number of loose objects
 +
-size: disk space consumed by loose objects, in KiB
+size: disk space consumed by loose objects, in KiB (unless -H is specified)
 +
 in-pack: the number of in-pack objects
 +
-size-pack: disk space consumed by the packs, in KiB
+size-pack: disk space consumed by the packs, in KiB (unless -H is specified)
 +
 prune-packable: the number of loose objects that are also present in
 the packs. These objects could be pruned using `git prune-packed`.
@@ -36,7 +36,13 @@ the packs. These objects could be pruned using `git prune-packed`.
 garbage: the number of files in object database that are not valid
 loose objects nor valid packs
 +
-size-garbage: disk space consumed by garbage files, in KiB
+size-garbage: disk space consumed by garbage files, in KiB (unless -H is
+specified)
+
+-H::
+--human-readable::
+
+Print sizes in human readable format
 
 GIT
 ---
diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index 0343e35..935ad9e 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -79,13 +79,13 @@ static void count_objects(DIR *d, char *path, int len, int verbose,
 }
 
 static char const * const count_objects_usage[] = {
-	N_("git count-objects [-v]"),
+	N_("git count-objects [-v] [-H | --human-readable]"),
 	NULL
 };
 
 int cmd_count_objects(int argc, const char **argv, const char *prefix)
 {
-	int i, verbose = 0;
+	int i, verbose = 0, human_readable = 0;
 	const char *objdir = get_object_directory();
 	int len = strlen(objdir);
 	char *path = xmalloc(len + 50);
@@ -93,6 +93,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 	off_t loose_size = 0;
 	struct option opts[] = {
 		OPT__VERBOSE(&verbose, N_("be verbose")),
+		OPT_BOOLEAN('H', "human-readable", &human_readable,
+			    N_("print sizes in human readable format")),
 		OPT_END(),
 	};
 
@@ -119,6 +121,9 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 		struct packed_git *p;
 		unsigned long num_pack = 0;
 		off_t size_pack = 0;
+		struct strbuf loose_buf = STRBUF_INIT;
+		struct strbuf pack_buf = STRBUF_INIT;
+		struct strbuf garbage_buf = STRBUF_INIT;
 		if (!packed_git)
 			prepare_packed_git();
 		for (p = packed_git; p; p = p->next) {
@@ -130,17 +135,42 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)
 			size_pack += p->pack_size + p->index_size;
 			num_pack++;
 		}
+
+		if (human_readable) {
+			strbuf_humanize(&loose_buf, loose_size);
+			strbuf_humanize(&pack_buf, size_pack);
+			strbuf_humanize(&garbage_buf, size_garbage);
+		}
+		else {
+			strbuf_addf(&loose_buf, "%lu",
+				    (unsigned long)(loose_size / 1024));
+			strbuf_addf(&pack_buf, "%lu",
+				    (unsigned long)(size_pack / 1024));
+			strbuf_addf(&garbage_buf, "%lu",
+				    (unsigned long)(size_garbage / 1024));
+		}
+
 		printf("count: %lu\n", loose);
-		printf("size: %lu\n", (unsigned long) (loose_size / 1024));
+		printf("size: %s\n", loose_buf.buf);
 		printf("in-pack: %lu\n", packed);
 		printf("packs: %lu\n", num_pack);
-		printf("size-pack: %lu\n", (unsigned long) (size_pack / 1024));
+		printf("size-pack: %s\n", pack_buf.buf);
 		printf("prune-packable: %lu\n", packed_loose);
 		printf("garbage: %lu\n", garbage);
-		printf("size-garbage: %lu\n", (unsigned long) (size_garbage / 1024));
+		printf("size-garbage: %s\n", garbage_buf.buf);
+		strbuf_release(&loose_buf);
+		strbuf_release(&pack_buf);
+		strbuf_release(&garbage_buf);
+	}
+	else {
+		struct strbuf buf = STRBUF_INIT;
+		if (human_readable)
+			strbuf_humanize(&buf, loose_size);
+		else
+			strbuf_addf(&buf, "%lu KiB",
+				    (unsigned long)(loose_size / 1024));
+		printf("%lu objects, %s\n", loose, buf.buf);
+		strbuf_release(&buf);
 	}
-	else
-		printf("%lu objects, %lu KiB\n",
-		       loose, (unsigned long) (loose_size / 1024));
 	return 0;
 }
-- 
1.7.9.5
Jonathan Nieder· Apr 10, 2013, 19:43 UTC · re: Antoine Pelisse · lore

Re: [PATCH 1/2] strbuf: create strbuf_humanize() to show byte sizes

Antoine Pelisse wrote:
Show 5 quoted lines
> Separate text formatting from size simplification and make the function
> public in strbuf so that it can easily be used by other clients.
>
> We now can use strbuf_humanize() for both downloaded size and download
> speed calculation.
Sounds like a good thing to do.
>                    One of the drawbacks is that speed will now look like
> this when download is stalled: "0 bytes/s" instead of "0 KiB/s".

At first glance that is neither obviously a benefit nor obviously a drawback. Can you spell this out more?

Show 10 quoted lines
> --- a/Documentation/technical/api-strbuf.txt
> +++ b/Documentation/technical/api-strbuf.txt
> @@ -230,6 +230,11 @@ which can be used by the programmer of the callback as she sees fit.
>  	destination. This is useful for literal data to be fed to either
>  	strbuf_expand or to the *printf family of functions.
>  
> +`strbuf_humanize`::
> +
> +	Append the given byte size as a human-readable string (i.e. 12.23 KiB,
> +	3.50 MiB).

Based on the function name alone, it is not easy to guess what it will do (e.g., maybe it will paraphrase 3 to "three" and 10000000 to "enormous"). How about something like strbuf_filesize?

If I understand the code correctly, this jumps units each time it exceeds 1.0 of the next unit (bytes, KiB, MiB, GiB), which sounds like a fine behavior.

Hope that helps, Jonathan

Antoine Pelisse· Apr 10, 2013, 20:00 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] strbuf: create strbuf_humanize() to show byte sizes

On Wed, Apr 10, 2013 at 9:43 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
> Antoine Pelisse wrote:
>>                    One of the drawbacks is that speed will now look like
>> this when download is stalled: "0 bytes/s" instead of "0 KiB/s".
>
> At first glance that is neither obviously a benefit nor obviously a
> drawback.  Can you spell this out more?

The drawback to me is that it changes the user experience with no reason. But that's really a minor change, I agree. (maybe I should have put it as a comment/question after ---)

Show 14 quoted lines
>> --- a/Documentation/technical/api-strbuf.txt
>> +++ b/Documentation/technical/api-strbuf.txt
>> @@ -230,6 +230,11 @@ which can be used by the programmer of the callback as she sees fit.
>>       destination. This is useful for literal data to be fed to either
>>       strbuf_expand or to the *printf family of functions.
>>
>> +`strbuf_humanize`::
>> +
>> +     Append the given byte size as a human-readable string (i.e. 12.23 KiB,
>> +     3.50 MiB).
>
> Based on the function name alone, it is not easy to guess what it will
> do (e.g., maybe it will paraphrase 3 to "three" and 10000000 to
> "enormous").  How about something like strbuf_filesize?

I think the suggestion from Junio makes more sense, as it can be used for download speed.

> If I understand the code correctly, this jumps units each time it
> exceeds 1.0 of the next unit (bytes, KiB, MiB, GiB), which sounds like
> a fine behavior.
The code has simply been extracted from the former function and kept unmodified.

Thanks for the help ! Antoine,

Junio C Hamano· Apr 10, 2013, 19:57 UTC · re: Antoine Pelisse · lore

Re: [PATCH 1/2] strbuf: create strbuf_humanize() to show byte sizes

Antoine Pelisse <apelisse@gmail.com> writes:
Show 10 quoted lines
> Currently, humanization of downloaded size is done in the same
> function as text formatting in 'process.c'. This is an issue if anyone
> else wants to use this.
>
> Separate text formatting from size simplification and make the function
> public in strbuf so that it can easily be used by other clients.
>
> We now can use strbuf_humanize() for both downloaded size and download
> speed calculation. One of the drawbacks is that speed will now look like
> this when download is stalled: "0 bytes/s" instead of "0 KiB/s".

Personally, I do not think the "drawback" is so big an issue. If the caller really cares, we could always add another parameter to this formatter to tell it the minimum unit we care about (e.g. pass 1024 to say "Don't bother showing scale lower than kibi").

This is a bit late response, but if we ever want to count something in a dimention other than "bytes", like time (e.g. "kiloseconds") or number of commits (e.g. "centicommits"), etc., we cannot reuse this formatter very easily. We may want to have "byte" somewhere in its name for now to make sure the callers understand its limitation.

I'll tentatively rename it to "strbuf_humanize_bytes()" while queuing.
Thanks.
Show 152 quoted lines
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
>  Documentation/technical/api-strbuf.txt |    5 ++++
>  progress.c                             |   43 +++++++++++---------------------
>  strbuf.c                               |   19 ++++++++++++++
>  strbuf.h                               |    1 +
>  4 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
> index 2c59cb2..7b6ecda 100644
> --- a/Documentation/technical/api-strbuf.txt
> +++ b/Documentation/technical/api-strbuf.txt
> @@ -230,6 +230,11 @@ which can be used by the programmer of the callback as she sees fit.
>  	destination. This is useful for literal data to be fed to either
>  	strbuf_expand or to the *printf family of functions.
>  
> +`strbuf_humanize`::
> +
> +	Append the given byte size as a human-readable string (i.e. 12.23 KiB,
> +	3.50 MiB).
> +
>  `strbuf_addf`::
>  
>  	Add a formatted string to the buffer.
> diff --git a/progress.c b/progress.c
> index 3971f49..8e09058 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -10,6 +10,7 @@
>  
>  #include "git-compat-util.h"
>  #include "progress.h"
> +#include "strbuf.h"
>  
>  #define TP_IDX_MAX      8
>  
> @@ -112,34 +113,14 @@ static int display(struct progress *progress, unsigned n, const char *done)
>  	return 0;
>  }
>  
> -static void throughput_string(struct throughput *tp, off_t total,
> +static void throughput_string(struct strbuf *buf, off_t total,
>  			      unsigned int rate)
>  {
> -	int l = sizeof(tp->display);
> -	if (total > 1 << 30) {
> -		l -= snprintf(tp->display, l, ", %u.%2.2u GiB",
> -			      (int)(total >> 30),
> -			      (int)(total & ((1 << 30) - 1)) / 10737419);
> -	} else if (total > 1 << 20) {
> -		int x = total + 5243;  /* for rounding */
> -		l -= snprintf(tp->display, l, ", %u.%2.2u MiB",
> -			      x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
> -	} else if (total > 1 << 10) {
> -		int x = total + 5;  /* for rounding */
> -		l -= snprintf(tp->display, l, ", %u.%2.2u KiB",
> -			      x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> -	} else {
> -		l -= snprintf(tp->display, l, ", %u bytes", (int)total);
> -	}
> -
> -	if (rate > 1 << 10) {
> -		int x = rate + 5;  /* for rounding */
> -		snprintf(tp->display + sizeof(tp->display) - l, l,
> -			 " | %u.%2.2u MiB/s",
> -			 x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> -	} else if (rate)
> -		snprintf(tp->display + sizeof(tp->display) - l, l,
> -			 " | %u KiB/s", rate);
> +	strbuf_addstr(buf, ", ");
> +	strbuf_humanize(buf, total);
> +	strbuf_addstr(buf, " | ");
> +	strbuf_humanize(buf, rate * 1024);
> +	strbuf_addstr(buf, "/s");
>  }
>  
>  void display_throughput(struct progress *progress, off_t total)
> @@ -183,6 +164,7 @@ void display_throughput(struct progress *progress, off_t total)
>  	misecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;
>  
>  	if (misecs > 512) {
> +		struct strbuf buf = STRBUF_INIT;
>  		unsigned int count, rate;
>  
>  		count = total - tp->prev_total;
> @@ -197,7 +179,9 @@ void display_throughput(struct progress *progress, off_t total)
>  		tp->last_misecs[tp->idx] = misecs;
>  		tp->idx = (tp->idx + 1) % TP_IDX_MAX;
>  
> -		throughput_string(tp, total, rate);
> +		throughput_string(&buf, total, rate);
> +		strncpy(tp->display, buf.buf, sizeof(tp->display));
> +		strbuf_release(&buf);
>  		if (progress->last_value != -1 && progress_update)
>  			display(progress, progress->last_value, NULL);
>  	}
> @@ -253,9 +237,12 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
>  
>  		bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
>  		if (tp) {
> +			struct strbuf strbuf = STRBUF_INIT;
>  			unsigned int rate = !tp->avg_misecs ? 0 :
>  					tp->avg_bytes / tp->avg_misecs;
> -			throughput_string(tp, tp->curr_total, rate);
> +			throughput_string(&strbuf, tp->curr_total, rate);
> +			strncpy(tp->display, strbuf.buf, sizeof(tp->display));
> +			strbuf_release(&strbuf);
>  		}
>  		progress_update = 1;
>  		sprintf(bufp, ", %s.\n", msg);
> diff --git a/strbuf.c b/strbuf.c
> index 48e9abb..8a50e66 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -528,6 +528,25 @@ void strbuf_addstr_urlencode(struct strbuf *sb, const char *s,
>  	strbuf_add_urlencode(sb, s, strlen(s), reserved);
>  }
>  
> +void strbuf_humanize(struct strbuf *buf, off_t bytes)
> +{
> +	if (bytes > 1 << 30) {
> +		strbuf_addf(buf, "%u.%2.2u GiB",
> +			    (int)(bytes >> 30),
> +			    (int)(bytes & ((1 << 30) - 1)) / 10737419);
> +	} else if (bytes > 1 << 20) {
> +		int x = bytes + 5243;  /* for rounding */
> +		strbuf_addf(buf, "%u.%2.2u MiB",
> +			    x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);
> +	} else if (bytes > 1 << 10) {
> +		int x = bytes + 5;  /* for rounding */
> +		strbuf_addf(buf, "%u.%2.2u KiB",
> +			    x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);
> +	} else {
> +		strbuf_addf(buf, "%u bytes", (int)bytes);
> +	}
> +}
> +
>  int printf_ln(const char *fmt, ...)
>  {
>  	int ret;
> diff --git a/strbuf.h b/strbuf.h
> index 958822c..317c5a8 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -170,6 +170,7 @@ extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
>  
>  extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
>  				    int reserved);
> +extern void strbuf_humanize(struct strbuf *buf, off_t bytes);
>  
>  __attribute__((format (printf,1,2)))
>  extern int printf_ln(const char *fmt, ...);
Antoine Pelisse· Apr 10, 2013, 20:12 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] strbuf: create strbuf_humanize() to show byte sizes

On Wed, Apr 10, 2013 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> Currently, humanization of downloaded size is done in the same
>> function as text formatting in 'process.c'. This is an issue if anyone
>> else wants to use this.
>>
>> Separate text formatting from size simplification and make the function
>> public in strbuf so that it can easily be used by other clients.
>>
>> We now can use strbuf_humanize() for both downloaded size and download
>> speed calculation. One of the drawbacks is that speed will now look like
>> this when download is stalled: "0 bytes/s" instead of "0 KiB/s".
>
> Personally, I do not think the "drawback" is so big an issue.  If
> the caller really cares, we could always add another parameter to
> this formatter to tell it the minimum unit we care about (e.g. pass
> 1024 to say "Don't bother showing scale lower than kibi").
I thought about that, but decided it was not worth it (at least for the moment)
Show 5 quoted lines
> This is a bit late response, but if we ever want to count something
> in a dimention other than "bytes", like time (e.g. "kiloseconds") or
> number of commits (e.g. "centicommits"), etc., we cannot reuse this
> formatter very easily.  We may want to have "byte" somewhere in its
> name for now to make sure the callers understand its limitation.

I'm not in a hurry. But it look tough to make it generic: one is binary, another is sexagesimal, and the last is decimal

> I'll tentatively rename it to "strbuf_humanize_bytes()" while queuing.

I like the idea, Thanks,

Eric Sunshine· Apr 8, 2013, 21:55 UTC · re: Antoine Pelisse · lore

Re: [PATCH 1/2] progress: create public humanize() to show sizes

On Mon, Apr 8, 2013 at 2:18 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
Show 8 quoted lines
> Currently, humanization of downloaded size is done in the same function
> as text formatting. This is an issue if anyone else wants to use this.
>
> Separate text formatting from size simplification and make the function
> public so that it can easily be used by other clients.
>
> We now can use humanize() for both downloaded size and download speed
> calculation. One of the drawbacks is that speed will no look like this
s/no/now/
> when download is stalled: "0 bytes/s" instead of "0 KiB/s".
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>

← back to recent threads