From: Justin Tobler Date: Tue, 23 Sep 2025 15:19:51 GMT Subject: Re: [PATCH 2/4] builtin/repo: add object counts in stats output Message-ID: In-Reply-To: On 25/09/23 12:52PM, Patrick Steinhardt wrote: > 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. Makes sense. > > 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. Will do. > > 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`. That's fair. I quite like the name `repo_stats`. I'll use that instead. Thanks > > > @@ -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. Ya, I'll change this in the next version. > > + 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? Good suggestion. Will update. > > @@ -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. I agree it doesn't hurt to be more defensive here. I'll update in the next version. > > + 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. Ya, I was planning on adding a progress meter in the future. It may not be too much to add it as part of this series though. I'll take a look. -Justin