From: Karthik Nayak Date: Tue, 23 Sep 2025 15:22:17 GMT Subject: Re: [PATCH 1/4] builtin/repo: introduce stats subcommand Message-ID: In-Reply-To: <20250923025700.3046260-2-jltobler@gmail.com> Justin Tobler writes: [snip] > diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc > index 209afd1b61..7762329551 100644 > --- a/Documentation/git-repo.adoc > +++ b/Documentation/git-repo.adoc > @@ -9,6 +9,7 @@ SYNOPSIS > -------- > [synopsis] > git repo info [--format=(keyvalue|nul)] [-z] [...] > +git repo stats > > DESCRIPTION > ----------- > @@ -43,6 +44,12 @@ supported: > + > `-z` is an alias for `--format=nul`. > > +stats:: > + Retrieve stats about the current repository. All references in the > + repository are categorized and counted accordingly. > ++ Nit: Should we use bullet points for the information provided? As of this commit it is only references, but with time, this will grow, so would be nice to have a set of bullet points to understand the different information retrieved. > +The table output format may change and is not intended for machine parsing. > + > INFO KEYS > --------- > In order to obtain a set of values from `git repo info`, you should provide > diff --git a/builtin/repo.c b/builtin/repo.c > index bbb0966f2d..15899dd74c 100644 > --- a/builtin/repo.c > +++ b/builtin/repo.c > @@ -4,12 +4,15 @@ > #include "environment.h" > #include "parse-options.h" > #include "quote.h" > +#include "ref-filter.h" > #include "refs.h" > #include "strbuf.h" > +#include "string-list.h" > #include "shallow.h" > > static const char *const repo_usage[] = { > "git repo info [--format=(keyvalue|nul)] [-z] [...]", > + "git repo stats", > NULL > }; > > @@ -156,12 +159,160 @@ static int repo_info(int argc, const char **argv, const char *prefix, > return print_fields(argc, argv, repo, format); > } > > +struct stats { > + size_t branches; > + size_t remotes; > + size_t tags; > + size_t others; Maybe we can use a nested structure here, which would reflect the output table. That would be much nicer, as the code would grow, currently we know 'others' here refers to references, but with other fields coming in, this would no longer be obvious. > +}; > + > +struct stats_table { > + struct string_list rows; > + > + int name_col_width; > + int value_col_width; Can these be negative? > +}; > + Nit: Could we add a comment about what this structure is and what it holds? I think it is sort of obvious, but would be nice to clarify. > +struct stats_table_entry { > + char *value; > +}; > + > +static void stats_table_add(struct stats_table *table, const char *name, > + struct stats_table_entry *entry) > +{ > + int name_width = strlen(name); > + 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; > + } > +} > + > +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); > +} > + > +static void stats_table_print(struct stats_table *table) > +{ > + const char *name_col_title = _("Repository stats"); > + const char *value_col_title = _("Value"); > + int name_col_width = strlen(name_col_title); > + int value_col_width = strlen(value_col_title); > + 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); > + 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); > + > + if (entry) > + free(entry->value); > + } > + > + 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; > + } > + } > +} > + > +static int repo_stats(int argc UNUSED, const char **argv UNUSED, > + const char *prefix UNUSED, struct repository *repo UNUSED) > +{ > + 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); > + I was wondering why we need the filter mechanism here, but seems like this is to obtain the type of the reference. This is setup automatically by the filter mechanism and so it's okay. We could replicate the same using the ref iterator code. This would be a little more involved, but would remove the need to store all refs in a 'ref_array' and also the need to loop over references twice. But it doesn't really matter in this usecase I assume. > + stats_count_references(&stats, &refs); > + > + stats_table_setup(&table, &stats); > + stats_table_print(&table); > + > + string_list_clear(&table.rows, 1); > + strvec_clear(&ref_patterns); Huh. So `ref_patterns` is simply a dummy variable, I was wondering why we simply can't set `filter.name_patterns = NULL`. I see that it is because in `filter_pattern_match()` we do `if (!*filter->name_patterns)`. This is not a great interface for ref-filter. Perhaps we could add a precursor commit like: diff --git a/ref-filter.c b/ref-filter.c index 520d2539c9..20284b5918 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2664,7 +2664,7 @@ static int match_name_as_path(const char **pattern, const char *refname, /* Return 1 if the refname matches one of the patterns, otherwise 0. */ static int filter_pattern_match(struct ref_filter *filter, const char *refname) { - if (!*filter->name_patterns) + if (!filter->name_patterns || !*filter->name_patterns) return 1; /* No pattern always matches */ if (filter->match_as_path) return match_name_as_path(filter->name_patterns, refname, diff --git a/ref-filter.h b/ref-filter.h index f22ca94b49..44d9b481ad 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -109,6 +109,7 @@ struct ref_format { #define REF_FILTER_INIT { \ .points_at = OID_ARRAY_INIT, \ .exclude = STRVEC_INIT, \ + .name_pattern = NULL, \ } #define REF_FORMAT_INIT { \ .use_color = -1, \ and then we could modify this commit: diff --git a/builtin/repo.c b/builtin/repo.c index 15899dd74c..09733b8df7 100644 --- a/builtin/repo.c +++ b/builtin/repo.c @@ -286,12 +286,11 @@ static int repo_stats(int argc UNUSED, const char **argv UNUSED, const char *prefix UNUSED, struct repository *repo UNUSED) { 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); stats_count_references(&stats, &refs); @@ -300,7 +299,6 @@ static int repo_stats(int argc UNUSED, const char **argv UNUSED, stats_table_print(&table); string_list_clear(&table.rows, 1); - strvec_clear(&ref_patterns); ref_array_clear(&refs); return 0; > + ref_array_clear(&refs); > + > + return 0; > +} > + > int cmd_repo(int argc, const char **argv, const char *prefix, > struct repository *repo) > { > parse_opt_subcommand_fn *fn = NULL; > struct option options[] = { > OPT_SUBCOMMAND("info", &fn, repo_info), > + OPT_SUBCOMMAND("stats", &fn, repo_stats), > OPT_END() > }; > [snip]