From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 25 Sep 2025 13:01:20 GMT
Subject: Re: [PATCH v2 3/6] builtin/repo: introduce stats subcommand
Message-ID: <a3qgun7qrabs6wplbv5p75cokmr2flklnvyhzteytiu356b2pp@nruhrm4d2hhk>
In-Reply-To: <aNTVa3-RtYNPlcHc@pks.im>

On 25/09/25 07:38AM, Patrick Steinhardt wrote:
> On Wed, Sep 24, 2025 at 04:24:23PM -0500, Justin Tobler wrote:
[snip]
> > +static void stats_table_add(struct stats_table *table, const char *format,
> > +			    const char *name, struct stats_table_entry *entry)
> 
> We could of course accept varargs right from the start and thus allow
> the caller to pass arbitrary formatting directives. But I guess we don't
> need it now, so it's fine to not do it.

I was on the fence about using varargs from the start. I figured since
we don't have a need right now for multiple formatting directives, we
could just pass the one. I might just go ahead and implement varargs
here in the next version though.

> [snip]
> > +static void stats_table_print(struct stats_table *table)
> 
> Nit: The table can be marked as `const` as we don't modify it.

Will update.

> > +{
> > +	const char *name_col_title = _("Repository stats");
> > +	const char *value_col_title = _("Value");
> > +	size_t name_title_len = strlen(name_col_title);
> > +	size_t value_title_len = strlen(value_col_title);
> > +	struct strbuf buf = STRBUF_INIT;
> > +	struct string_list_item *item;
> > +	int name_col_width;
> > +	int value_col_width;
> > +
> > +	name_col_width = cast_size_t_to_int(
> > +		max_size_t(table->name_col_width, name_title_len));
> > +	value_col_width = cast_size_t_to_int(
> > +		max_size_t(table->value_col_width, value_title_len));
> > +
> > +	strbuf_addf(&buf, "| %-*s | %-*s |\n", name_col_width, name_col_title,
> > +		    value_col_width, value_col_title);
> > +	strbuf_addstr(&buf, "| ");
> > +	strbuf_addchars(&buf, '-', name_col_width);
> > +	strbuf_addstr(&buf, " | ");
> > +	strbuf_addchars(&buf, '-', value_col_width);
> > +	strbuf_addstr(&buf, " |\n");
> > +
> > +	for_each_string_list_item(item, &table->rows) {
> > +		struct stats_table_entry *entry = item->util;
> > +		const char *value = "";
> > +
> > +		if (entry) {
> > +			struct stats_table_entry *entry = item->util;
> > +			value = entry->value;
> > +		}
> > +
> > +		strbuf_addf(&buf, "| %-*s | %*s |\n", name_col_width,
> > +			    item->string, value_col_width, value);
> > +	}
> > +
> > +	fputs(buf.buf, stdout);
> > +	strbuf_release(&buf);
> > +}
> 
> By the way, is there any specific reason we do the detour via the strbuf
> instead of printing the data to stdout directly?

Not really. I think it's just a holdover from a previous implementation
that was passing around a strbuf before printing. I'll go ahead and just
print directly to stdout via printf in the next version.

-Justin