Re: [PATCH 2/4] builtin/repo: add object counts in stats output

[Patrick Steinhardt <ps@pks.im>]

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