Re: [PATCH 2/6] builtin/repo: humanise count values in structure output

[Patrick Steinhardt <ps@pks.im>]

On Tue, Dec 09, 2025 at 04:58:16PM -0600, Justin Tobler wrote:
> diff --git a/builtin/repo.c b/builtin/repo.c
> index a69699857a..8fb728b3a5 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -266,6 +275,10 @@ static void stats_table_addf(struct stats_table *table, const char *format, ...)
>  	va_end(ap);
>  }
>  
> +static const char *unit_k = "k";
> +static const char *unit_M = "M";
> +static const char *unit_G = "G";
> +
>  static void stats_table_count_addf(struct stats_table *table, size_t value,
>  				   const char *format, ...)
>  {

I would assume that these units should be translatable.

> @@ -273,7 +286,26 @@ static void stats_table_count_addf(struct stats_table *table, size_t value,
>  	va_list ap;
>  
>  	CALLOC_ARRAY(entry, 1);
> -	entry->value = xstrfmt("%" PRIuMAX, (uintmax_t)value);
> +
> +	if (value >= 1000000000) {
> +		uintmax_t x = (uintmax_t)value + 5000000;
> +		entry->value = xstrfmt("%" PRIuMAX ".%02" PRIuMAX,
> +				       x / 1000000000,
> +				       x % 1000000000 / 10000000);
> +		entry->unit = unit_G;
> +	} else if (value >= 1000000) {
> +		uintmax_t x = (uintmax_t)value + 5000;
> +		entry->value = xstrfmt("%" PRIuMAX ".%02" PRIuMAX,
> +				       x / 1000000, x % 1000000 / 10000);
> +		entry->unit = unit_M;
> +	} else if (value >= 1000) {
> +		uintmax_t x = (uintmax_t)value + 5;
> +		entry->value = xstrfmt("%" PRIuMAX ".%02" PRIuMAX,
> +				       x / 1000, x % 1000 / 10);
> +		entry->unit = unit_k;
> +	} else {
> +		entry->value = xstrfmt("%" PRIuMAX, (uintmax_t)value);
> +	}
>  
>  	va_start(ap, format);
>  	stats_table_vaddf(table, entry, format, ap);

These units are decimal-based (1000), whereas in "parse.c" we have
`get_unit_factor()` that is binary-based (1024). Arguably, it's
"parse.c" that is wrong because "k" is generally decimal-based whereas
"Ki" would be binary-based.

Not quite sure what to do with this. For counts it _could_ be okay if we
continue to use the wrong unit prefix. But as soon as we get to disk
sizes we certainly should use the correct units, which would probably be
KiB.

> diff --git a/t/t1901-repo-structure.sh b/t/t1901-repo-structure.sh
> index 36a71a144e..55fd13ad1b 100755
> --- a/t/t1901-repo-structure.sh
> +++ b/t/t1901-repo-structure.sh
> @@ -10,21 +10,21 @@ test_expect_success 'empty repository' '
>  	(
>  		cd repo &&
>  		cat >expect <<-\EOF &&
> -		| Repository structure | Value |
> -		| -------------------- | ----- |
> -		| * References         |       |
> -		|   * Count            |     0 |
> -		|     * Branches       |     0 |
> -		|     * Tags           |     0 |
> -		|     * Remotes        |     0 |
> -		|     * Others         |     0 |
> -		|                      |       |
> -		| * Reachable objects  |       |
> -		|   * Count            |     0 |
> -		|     * Commits        |     0 |
> -		|     * Trees          |     0 |
> -		|     * Blobs          |     0 |
> -		|     * Tags           |     0 |
> +		| Repository structure | Value  |
> +		| -------------------- | ------ |
> +		| * References         |        |
> +		|   * Count            |     0  |
> +		|     * Branches       |     0  |
> +		|     * Tags           |     0  |
> +		|     * Remotes        |     0  |
> +		|     * Others         |     0  |
> +		|                      |        |
> +		| * Reachable objects  |        |
> +		|   * Count            |     0  |
> +		|     * Commits        |     0  |
> +		|     * Trees          |     0  |
> +		|     * Blobs          |     0  |
> +		|     * Tags           |     0  |
>  		EOF
>  
>  		git repo structure >out 2>err &&

It's a bit weird that this test here changes even though we don't even
use any units. But I don't mind it too much.

Patrick