From: Justin Tobler <jltobler@gmail.com>
Date: Wed, 10 Dec 2025 15:10:29 GMT
Subject: Re: [PATCH 2/6] builtin/repo: humanise count values in structure output
Message-ID: <kf7vavs5yetooe6u2ygttzfriul4u5ywdnhtyksh2pbar4mpfz@orlg7ppajd7s>
In-Reply-To: <aTkS_kBlNsnbPyP5@pks.im>

On 25/12/10 07:28AM, Patrick Steinhardt wrote:
> 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.

Ya, you are right. I'll make units translatable in the next version.

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

For count values, such as number of references/objects, I'm using SI
unit prefixes which I think is more correct. In a subsequent patch where
we start collect size information, I add a separate
`stats_table_size_addf()` function which uses the IEC unit prefixes.
This way we use the most appropriate option for both scenarios.

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

Ya, the added space comes from the fixed space character between the
value and unit columns. I didn't think it mattered too much, but I may
try to only conditionally add it if needed in the next version.

-Justin