Re: [PATCH 1/1] repo: add filtering options to "repo structure"

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 30, 2026, 16:28 UTC
Message-ID: <ar04uStCZ4pnEJ38@pks.im>
In-Reply-To: <20260924164503.119506-2-markchucarroll@fastmail.com>

On Thu, Sep 24, 2026 at 12:45:03PM -0400, Mark C. Chu-Carroll wrote:

Show 10 quoted lines

> "git repo structure" provides a collection of useful information
> about the information stored in a repo. In particular, it's
> valuable for diagnosing performance issues caused by large objects
> stored in a repo.
> 
> The current implementation of "git repo stucture" provides summary
> information about everything in the repository - all of the
> branches, remotes, tags, stashes, and notes. But sometimes
> to properly diagnose a problem, it's useful to be able to exclude
> refs that are known to not be relevant to the issue at hand.

Yes, indeed. Sometimes you may for example want to figure out where exactly the storage size of a particular repository is going. Or in the case of GitLab for example, we may have bookkeeping references that are not controllable by customers. So we may only want to get the structure for all the customer-controllable branches there.

Show 9 quoted lines

> Add a set of flags that allow a user to selective exclude
> reference types from the report generated by "git repo structure".
> When a ref type is excluded by the filter, it no longer appears
> in the report (ie, if "--no-tags" is passed, the report line
> for "Branches" will no longer appear under "* References").
> Following the pattern of flags that are only used to
> disable functionality (eg, "--no-verify" in "builtins/push.c"),
> only the "--no-<reftype>" syntax is listed in the updated
> documentation.

Hmm, okay. I would have expected that the user can essentially pass arbitrary revisions as understood by git-log(1) et al. And if they pass any such revisions, we should not enumerate anything but what they have passed, so the flags shouldn't only be used to exclude.

So, for example:

    $ git repo structure --branches
    $ git repo structure master
    $ git repo structure --all --not --branches

I would hope that git-repo(1) can achieve that rather easily because I expect that it uses `struct rev_info`, but let's read on.

Show 11 quoted lines

> Overview of the changes:
> - Add an enum to represent the structure flags.
> - Add structure flags to the options for the "repo structure" commands.
> - For each reference flag, add a conditional in "count_references"
>   which decides whether or not to add a ref to the pending list.
>   If an references is not added to the pending list, the things it
>   transitively references will not be added to the stats.
> - Add a set of test cases to verify that reference counts
>   in the repo structure report correctly omit the specified
>   resource types.
> - Update the documentation for git-repo to include the new options.

Note that we typically don't have lists of what exactly has changed in the commit. That kind of information is already visible from the diff itself. So what the commit message itself should focus on is whether any of these changes are non-obvious or whethere there's any dragons to be found.

So in summary: everything that may surprise the reader should be part of the commit message, everything that's just obvious plumbing doesn't really have to be mentioned.

Show 33 quoted lines

> diff --git a/builtin/repo.c b/builtin/repo.c
> index 84e012f83f..c8f6e38011 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -490,7 +504,8 @@ static inline size_t get_total_object_values(struct object_values *values)
>  }
>  
>  static void stats_table_setup_structure(struct stats_table *table,
> -					struct repo_structure *stats)
> +					struct repo_structure *stats,
> +					enum repo_structure_filter_flags flags)
>  {
>  	struct object_stats *objects = &stats->objects;
>  	struct ref_stats *refs = &stats->refs;
> @@ -502,9 +517,15 @@ static void stats_table_setup_structure(struct stats_table *table,
>  	ref_total = get_total_reference_count(refs);
>  	stats_table_addf(table, "* %s", _("References"));
>  	stats_table_count_addf(table, ref_total, "  * %s", _("Count"));
> -	stats_table_count_addf(table, refs->branches, "    * %s", _("Branches"));
> -	stats_table_count_addf(table, refs->tags, "    * %s", _("Tags"));
> -	stats_table_count_addf(table, refs->remotes, "    * %s", _("Remotes"));
> +	if (flags & REPO_STRUCTURE_FILTER_BRANCHES) {
> +		stats_table_count_addf(table, refs->branches, "    * %s", _("Branches"));
> +	}
> +	if (flags & REPO_STRUCTURE_FILTER_TAGS) {
> +		stats_table_count_addf(table, refs->tags, "    * %s", _("Tags"));
> +	}
> +	if (flags & REPO_STRUCTURE_FILTER_REMOTES) {
> +		stats_table_count_addf(table, refs->remotes, "    * %s", _("Remotes"));
> +	}
>  	stats_table_count_addf(table, refs->others, "    * %s", _("Others"));
>  
>  	object_count_total = get_total_object_values(&objects->type_counts);

Coding style: we don't use curly braces around single-line statements.

But more importantly, I think this is where the mismatch in expectations comes from that I was pointing out further up. My expectation was that what we want to achieve is to filter the reachable objects by revisions, which I think is a much more useful thing to do. But what the flags do instead us to filter the output in the "References" count.

I think that we should rather go into the direction of filtering objects and not the ref output, as the latter isn't all that useful. It _may_ make sense to maybe make some sections of the output optional, but excluding individual ref types is arguably too fine-grained.

In any case, to go into the direction of filtering objects you'd want to adapt `parse_options()` so that it accepts unknown options (you can achieve that by passing `PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT`) and then pass argv to `setup_revisions()`. And I think that _should_ already achieve proper filtering of objects by revisions.

Thanks!

Patrick