Re: [PATCH 1/4] builtin/repo: introduce stats subcommand

> > +	struct string_list_item *item;
> > +
> > +	item = string_list_append(&table->rows, name);
> > +	item->util = entry;
> > +
> > +	if (name_width > table->name_col_width)
> > +		table->name_col_width = name_width;
> > +	if (entry) {
> > +		int value_width = strlen(entry->value);
> > +		if (value_width > table->value_col_width)
> > +			table->value_col_width = value_width;
> > +	}
> 
> I was wondering at first why you'd ever want to not pass an entry, but
> we use that to have "dividers" in the table. Makes sense.

> > +}
> > +
> > +static void stats_table_add_count(struct stats_table *table, const char *name,
> > +				  size_t value)
> > +{
> > +	struct stats_table_entry *entry;
> > +
> > +	CALLOC_ARRAY(entry, 1);
> > +	entry->value = xstrfmt("%" PRIuMAX, (uintmax_t)value);
> > +	stats_table_add(table, name, entry);
> > +}
> > +
> > +static void stats_table_setup(struct stats_table *table, struct stats *stats)
> > +{
> > +	size_t ref_total;
> > +
> > +	ref_total = stats->branches + stats->remotes + stats->tags + stats->others;
> > +	stats_table_add(table, _("* References"), NULL);
> > +	stats_table_add_count(table, _("  * Count"), ref_total);
> > +	stats_table_add_count(table, _("    * Branches"), stats->branches);
> > +	stats_table_add_count(table, _("    * Tags"), stats->tags);
> > +	stats_table_add_count(table, _("    * Remotes"), stats->remotes);
> > +	stats_table_add_count(table, _("    * Others"), stats->others);
> > +}
> 
> Would it make sense to not translate the formatting directives, but only
> the actual words?

> > +	struct strbuf buf = STRBUF_INIT;
> > +	struct string_list_item *item;
> > +
> > +	if (table->name_col_width > name_col_width)
> > +		name_col_width = table->name_col_width;
> > +	if (table->value_col_width > value_col_width)
> > +		value_col_width = table->value_col_width;
> > +
> > +	strbuf_addf(&buf, "| %-*s | %-*s |\n", name_col_width, name_col_title,
> > +		    value_col_width, value_col_title);
> 
> Aha, that's why you went with `int`. You can use `cast_size_to_to_int()`
> to convert between the types.

> > +	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) {
> 
> We typically don't have a space after between the macro and its
> arguments.

> > +		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);
> > +
> > +		if (entry)
> > +			free(entry->value);
> 
> It's a bit weird that we free the values when we pretend to only print
> data. Sure, we probably don't ever have a usecase where we want to print
> data a second time. But I still think it would be nice to separate
> concerns.

> > +	}
> > +
> > +	fputs(buf.buf, stdout);
> > +	strbuf_release(&buf);
> > +}
> > +
> > +static void stats_count_references(struct 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;
> 
> Do we want to have a `default:` case where we `BUG()`? Otherwise we may
> not notice that we undercount the overall number of refs.

> > +		}
> > +	}
> > +}
> > +
> > +static int repo_stats(int argc UNUSED, const char **argv UNUSED,
> > +		      const char *prefix UNUSED, struct repository *repo UNUSED)
> 
> Not a new issue, but I'd rather call this `cmd_repo_stats()` to note
> that this is the entrypoint. We might as well adapt the other subcommand
> to follow that naming schema in a preparatory commit.

> > +{
> > +	struct ref_filter filter = REF_FILTER_INIT;
> > +	struct strvec ref_patterns = STRVEC_INIT;
> > +	struct stats_table table = { 0 };
> > +	struct ref_array refs = { 0 };
> > +	struct stats stats = { 0 };
> > +
> > +	filter.name_patterns = ref_patterns.v;
> > +	filter_refs(&refs, &filter, FILTER_REFS_REGULAR);
> 
> `filter_refs()` may return an error code which we should probably
> handle.

> > diff --git a/t/t1901-repo-stats.sh b/t/t1901-repo-stats.sh
> > new file mode 100755
> > index 0000000000..27c32ec45f
> > --- /dev/null
> > +++ b/t/t1901-repo-stats.sh
> > @@ -0,0 +1,59 @@
> > +#!/bin/sh
> > +
> > +test_description='test git repo stats'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'empty repository stats' '
> 
> Nit: I don't think it's necessary to repeat "repository stats" in every
> test name. That's already clear from the test suite.