Re: [PATCH 2/6] builtin/repo: humanise count values in structure output
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Dec 10, 2025, 15:10 UTC
- Message-ID
- <kf7vavs5yetooe6u2ygttzfriul4u5ywdnhtyksh2pbar4mpfz@orlg7ppajd7s>
- In-Reply-To
- <aTkS_kBlNsnbPyP5@pks.im>
On 25/12/10 07:28AM, Patrick Steinhardt wrote:
Show 18 quoted lines
> 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.
Show 38 quoted lines
> > @@ -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.
Show 44 quoted lines
> > 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