From: Justin Tobler <jltobler@gmail.com>
Date: Wed, 10 Dec 2025 15:21:43 GMT
Subject: Re: [PATCH 4/6] builtin/repo: add inflated object info to structure table
Message-ID: <vrlxdgvibiuohfc6k6nbmloibivntex33ucnmfdqpqu4dparbi@piyq3kckcyje>
In-Reply-To: <aTkTEselZ4yL11qd@pks.im>

On 25/12/10 07:28AM, Patrick Steinhardt wrote:
> On Tue, Dec 09, 2025 at 04:58:18PM -0600, Justin Tobler wrote:
> > Update the table output format for the git-repo(1) structure command to
> > begin printing the total inflated object size info by object type. To be
> > more human-friendly, larger values are scaled down and displayed with
> > the appropriate unit prefix. Output for the keyvalue and nul formats
> > remains unchanged.
> > 
> > Signed-off-by: Justin Tobler <jltobler@gmail.com>
> > ---
> >  builtin/repo.c            | 57 +++++++++++++++++++++++++++++++++--
> >  t/t1901-repo-structure.sh | 62 +++++++++++++++++++++++----------------
> >  2 files changed, 90 insertions(+), 29 deletions(-)
> > 
> > diff --git a/builtin/repo.c b/builtin/repo.c
> > index a67215ae31..5c37f4116f 100644
> > --- a/builtin/repo.c
> > +++ b/builtin/repo.c
> > @@ -315,6 +315,44 @@ static void stats_table_count_addf(struct stats_table *table, size_t value,
> >  	va_end(ap);
> >  }
> >  
> > +static const char *unit_B = "B";
> > +static const char *unit_KiB = "KiB";
> > +static const char *unit_MiB = "MiB";
> > +static const char *unit_GiB = "GiB";
> 
> Okay, nice, you already use KiB et al as I suggested in an earlier
> comment. But I guess these should also be marked as translatable.

Will do.

> > +static void stats_table_size_addf(struct stats_table *table, size_t value,
> > +				  const char *format, ...)
> > +{
> > +	struct stats_table_entry *entry;
> > +	va_list ap;
> > +
> > +	CALLOC_ARRAY(entry, 1);
> > +
> > +	if (value > 1 << 30) {
> > +		uintmax_t x = (uintmax_t)value + 5368709;
> > +		entry->value = xstrfmt("%" PRIuMAX ".%02" PRIuMAX, x >> 30,
> > +				       ((x & ((1 << 30) - 1)) * 100) >> 30);
> > +		entry->unit = unit_GiB;
> > +	} else if (value > 1 << 20) {
> > +		uintmax_t x = (uintmax_t)value + 5243;
> > +		entry->value = xstrfmt("%" PRIuMAX ".%02" PRIuMAX, x >> 20,
> > +				       ((x & ((1 << 20) - 1)) * 100) >> 20);
> > +		entry->unit = unit_MiB;
> > +	} else if (value > 1 << 10) {
> > +		uintmax_t x = (uintmax_t)value + 5;
> > +		entry->value = xstrfmt("%" PRIuMAX ".%02" PRIuMAX, x >> 10,
> > +				       ((x & ((1 << 10) - 1)) * 100) >> 10);
> > +		entry->unit = unit_KiB;
> > +	} else {
> > +		entry->value = xstrfmt("%" PRIuMAX, (uintmax_t)value);
> > +		entry->unit = unit_B;
> > +	}
> 
> Euh. What kind of black magic is this? This block at least warrants a
> comment how you came up with these incantations.

Ya, I'll add some comments to explain what is going on here. :)

> Also, git-rev-list(1) already has logic to output human-formatted disk
> sizes via `git rev-list --disk-usage=human`. Can we share the logic?

So I believe `git rev-list --disk-usage=human` relies on
strbuf_humanise_bytes() under the hood. The problem here is that it
combines the value and unit prefix together. For alignment purposes in
the table output, we need to store the value and unit prefix separately.
I couldn't immediately think of a great way to share logic here so I
opted to implement it separately to accommodate this specific use case.

-Justin