From: Justin Tobler Date: Tue, 23 Sep 2025 15:10:50 GMT Subject: Re: [PATCH 1/4] builtin/repo: introduce stats subcommand Message-ID: In-Reply-To: On 25/09/23 12:52PM, Patrick Steinhardt wrote: > On Mon, Sep 22, 2025 at 09:56:57PM -0500, Justin Tobler wrote: [snip] > > Signed-off-by: Justin Tobler > > Is this command built on Derrick's git-survey(1)? If so, it would > probably be nice to add a "Based-on-patch-by" tag. The git-survey(1) series sent by Derrick certainly served as inspiration for this series. I didn't build this series from those patches, but there is probably some overlap since I'm trying to accomplish something very similar. I don't mind at all adding a "Based-on-patch-by" tag though. Will do in the next version :) [snip] > > + 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. Yup. Also, some rows like "* References" may have a name, but no value. > > +} > > + > > +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? From a simplicity stand point, it is quite nice to have the formatted offsets baked-in. It is probably better to separate out the transalations though? I'll interate on this in the next version. > > + 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. Yep :) I'll adapt following your suggestion in the next version. > > + 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. Your right! I seem to recall the style linter wanted me to have it this way, but I'll change it back to be consistent. > > + 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. That's fair. I'll probably add a stats_table_clear() in the next version. > > + } > > + > > + 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. Since filter_refs() is only checking regular references, we shouldn't ever encounter other types, but it doesn't hurt to BUG() here if it were to happen. Will do. > > + } > > + } > > +} > > + > > +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. Make sense. I'll update repo_info() in a preparatory commit as suggested. > > +{ > > + 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. Will do. > > 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. That's fair. Will adapt the tests accordingly. Thanks for the review, -Justin