From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 25 Sep 2025 05:38:51 GMT
Subject: Re: [PATCH v2 3/6] builtin/repo: introduce stats subcommand
Message-ID: <aNTVa3-RtYNPlcHc@pks.im>
In-Reply-To: <20250924212426.2930029-4-jltobler@gmail.com>

On Wed, Sep 24, 2025 at 04:24:23PM -0500, Justin Tobler wrote:
> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> index 209afd1b61..a009bf8cf1 100644
> --- a/Documentation/git-repo.adoc
> +++ b/Documentation/git-repo.adoc
> @@ -156,12 +159,188 @@ static int cmd_repo_info(int argc, const char **argv, const char *prefix,
>  	return print_fields(argc, argv, repo, format);
>  }
>  
> +struct ref_stats {
> +	size_t branches;
> +	size_t remotes;
> +	size_t tags;
> +	size_t others;
> +};
> +
> +struct stats_table {
> +	struct string_list rows;
> +
> +	size_t name_col_width;
> +	size_t value_col_width;
> +};
> +
> +/*
> + * Holds column data that gets stored for each row.
> + */
> +struct stats_table_entry {
> +	char *value;
> +};
> +
> +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.

[snip]
> +static void stats_table_print(struct stats_table *table)

Nit: The table can be marked as `const` as we don't modify it.

> +{
> +	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?

> +static void stats_table_clear(struct stats_table *table)
> +{
> +	struct stats_table_entry *entry;
> +	struct string_list_item *item;
> +
> +	for_each_string_list_item(item, &table->rows) {
> +		entry = item->util;
> +		if (entry)
> +			free(entry->value);
> +	}
> +
> +	string_list_clear(&table->rows, 1);
> +}

Yeah, this is much nicer now.

> +static void stats_count_references(struct ref_stats *stats, struct ref_array *refs)
> +{
> +	for (int i = 0; i < refs->nr; i++) {
> +		struct ref_array_item *ref = refs->items[i];
> +
> +		switch (ref->kind) {
> +		case FILTER_REFS_BRANCHES:
> +			stats->branches++;
> +			break;
> +		case FILTER_REFS_REMOTES:
> +			stats->remotes++;
> +			break;
> +		case FILTER_REFS_TAGS:
> +			stats->tags++;
> +			break;
> +		case FILTER_REFS_OTHERS:
> +			stats->others++;
> +			break;
> +		default:
> +			BUG("unexpected reference type");
> +		}
> +	}
> +}

Here we're now being defensive. Good.

Patrick