From: Patrick Steinhardt Date: Tue, 23 Sep 2025 10:52:54 GMT Subject: Re: [PATCH 2/4] builtin/repo: add object counts in stats output Message-ID: In-Reply-To: <20250923025700.3046260-3-jltobler@gmail.com> On Mon, Sep 22, 2025 at 09:56:58PM -0500, Justin Tobler wrote: > diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc > index 7762329551..2a67abfca8 100644 > --- a/Documentation/git-repo.adoc > +++ b/Documentation/git-repo.adoc > @@ -45,8 +45,9 @@ 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. > + Retrieve stats about the current repository. All references and > + reachable objects in the repository are categorized and counted > + accordingly. > + > The table output format may change and is not intended for machine parsing. I already wanted to mention this on the first commit, but would it maybe make sense if this was a bulleted list of information that we surface right from the start? Then we don't have to reflow the whole paragraph every time we surface new information. > diff --git a/builtin/repo.c b/builtin/repo.c > index 15899dd74c..a24ea0e66b 100644 > --- a/builtin/repo.c > +++ b/builtin/repo.c > @@ -159,13 +161,25 @@ static int repo_info(int argc, const char **argv, const char *prefix, > return print_fields(argc, argv, repo, format); > } > > -struct stats { > +struct ref_stats { Nit: let's call it `ref_stats` right from the start instead of renaming. > size_t branches; > size_t remotes; > size_t tags; > size_t others; > }; > > +struct object_stats { > + size_t tags; > + size_t commits; > + size_t trees; > + size_t blobs; > +}; > + > +struct stats { I'd maybe call this `struct repo_stats`. `stats` feels quite generic and very close to a collision with `struct stat`. > @@ -207,15 +221,27 @@ static void stats_table_add_count(struct stats_table *table, const char *name, > > static void stats_table_setup(struct stats_table *table, struct stats *stats) > { > + struct object_stats objects = stats->objects; > + struct ref_stats refs = stats->refs; We can avoid the copies by making these pointers. Not that it'd really matter all that much. > + size_t object_total; > size_t ref_total; > > - ref_total = stats->branches + stats->remotes + stats->tags + stats->others; > + ref_total = refs.branches + refs.remotes + refs.tags + refs.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); > + stats_table_add_count(table, _(" * Branches"), refs.branches); > + stats_table_add_count(table, _(" * Tags"), refs.tags); > + stats_table_add_count(table, _(" * Remotes"), refs.remotes); > + stats_table_add_count(table, _(" * Others"), refs.others); > + > + object_total = objects.commits + objects.trees + objects.blobs + objects.tags; > + stats_table_add(table, "", NULL); > + stats_table_add(table, _("* Objects"), NULL); Should we maybe say "Reachable objects" here to clarify that this doesn't count unreachable ones? > @@ -282,25 +308,80 @@ static void stats_count_references(struct stats *stats, struct ref_array *refs) > } > } > > +static int count_objects(const char *path UNUSED, struct oid_array *oids, > + enum object_type type, void *data) > +{ > + struct object_stats *stats = data; > + > + switch (type) { > + case OBJ_TAG: > + stats->tags += oids->nr; > + break; > + case OBJ_COMMIT: > + stats->commits += oids->nr; > + break; > + case OBJ_TREE: > + stats->trees += oids->nr; > + break; > + case OBJ_BLOB: > + stats->blobs += oids->nr; > + break; > + default: Let's `BUG()` here. This case should never happen, and if it does something is seriously wrong. > + break; > + } > + > + return 0; > +} > + > +static void stats_count_objects(struct object_stats *stats, > + struct ref_array *refs, struct rev_info *revs) > +{ > + struct path_walk_info info = PATH_WALK_INFO_INIT; > + > + info.revs = revs; > + info.path_fn = count_objects; > + info.path_fn_data = stats; > + > + for (int i = 0; i < refs->nr; i++) { > + struct ref_array_item *ref = refs->items[i]; > + > + switch (ref->kind) { > + case FILTER_REFS_BRANCHES: > + case FILTER_REFS_TAGS: > + case FILTER_REFS_REMOTES: > + case FILTER_REFS_OTHERS: > + add_pending_oid(revs, NULL, &ref->objectname, 0); > + break; > + } > + } > + > + walk_objects_by_path(&info); > + path_walk_info_clear(&info); > +} I guess this can take a while, so having a progress meter would be great to have to give the user some info what's happening. I guess it doesn't have to be part of the first iteration thuogh as long as this is something we plan to add at a later point. > diff --git a/t/t1901-repo-stats.sh b/t/t1901-repo-stats.sh > index 27c32ec45f..c6a7f08be5 100755 > --- a/t/t1901-repo-stats.sh > +++ b/t/t1901-repo-stats.sh > @@ -20,6 +20,13 @@ test_expect_success 'empty repository stats' ' > | * Tags | 0 | > | * Remotes | 0 | > | * Others | 0 | > + | | | > + | * Objects | | > + | * Count | 0 | > + | * Commits | 0 | > + | * Trees | 0 | > + | * Blobs | 0 | > + | * Tags | 0 | > EOF > > test_cmp expect out && > @@ -49,6 +56,45 @@ test_expect_success 'repository stats with references' ' > | * Tags | 1 | > | * Remotes | 1 | > | * Others | 1 | > + | | | > + | * Objects | | > + | * Count | 5 | > + | * Commits | 2 | > + | * Trees | 2 | > + | * Blobs | 1 | > + | * Tags | 0 | > + EOF > + > + test_cmp expect out && > + test_line_count = 0 err > + ) > +' > + > +test_expect_success 'repository stats with objects' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + test_commit_bulk 42 && > + git tag -a foo -m bar && > + git repo stats >out 2>err && > + > + cat >expect <<-EOF && > + | Repository stats | Value | > + | ---------------- | ----- | > + | * References | | > + | * Count | 2 | > + | * Branches | 1 | > + | * Tags | 1 | > + | * Remotes | 0 | > + | * Others | 0 | > + | | | > + | * Objects | | > + | * Count | 127 | > + | * Commits | 42 | > + | * Trees | 42 | > + | * Blobs | 42 | > + | * Tags | 1 | > EOF I quite like the output format, by the way. It's nice to read and makes it sufficiently clear that this is not expected to be parsed by a machine. Patrick