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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 05, 2026 at 01:40:44PM -0400, Mark C. Chu-Carroll wrote:
> Implement filtering for repo structure, imitating the mechanism
> used in "git log".

The message should give an explanation of what this change does, and
what the motivation behind it is.

> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> index ed7d80c690..5cbdf8e727 100644
> --- a/Documentation/git-repo.adoc
> +++ b/Documentation/git-repo.adoc
> @@ -10,7 +10,7 @@ SYNOPSIS
>  [synopsis]
>  git repo info [--format=(lines|nul) | -z] [--all | <key>...]
>  git repo info --keys [--format=(lines|nul) | -z]
> -git repo structure [--format=(table|lines|nul) | -z]
> +git repo structure [--format=(table|lines|nul) | -z] [<include|^exclude>...]

I think we should probably have this be `[<revs>...]`.

> @@ -56,9 +56,10 @@ supported:
>  `nul`:::
>  	Similar to `lines`, but using a _NUL_ character after each value.
>  
> -`structure [--format=(table|lines|nul) | -z]`::
> -	Retrieve statistics about the current repository structure. The
> -	following kinds of information are reported:
> +`structure [--format=(table|lines|nul) | -z] [<include|^exclude>...]::

Same here.

> @@ -66,6 +67,16 @@ supported:
>  * Total disk size of reachable objects by type
>  * Largest reachable objects in the repository by type
>  +
> +The set of objects counted can be filtered by specifying a
> +collection of query clauses to select which objects will be

s/query clauses/revisions/, which is a well-defined term. So with this
change I think we can drop most of the remaining paragraph, except for
the last sentence.

> +counted. These parameters follow the same syntax as the parameters
> +to similar commands like `git log`. Semantically, these parameters
> +are treated as a collection of include and exclude specifiers. Th
> +set of objects counted will consist of all objects reachable from
> +an object included by one of the include specifiers via a path that
> +does not include an object in an exclude clause. If no includes
> +are specified, then the include set is all reachable objects. 
> ++
>  The output format can be chosen through the flag `--format`. Three formats are
>  supported:
>  +
> diff --git a/builtin/repo.c b/builtin/repo.c
> index 84e012f83f..b3aca71298 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -946,12 +946,20 @@ static int cmd_repo_structure(int argc, const char **argv, const char *prefix,
>  		OPT_BOOL(0, "progress", &show_progress, N_("show progress")),
>  		OPT_END()
>  	};
> +	struct setup_revision_opt s_r_opt;
> +	memset(&s_r_opt, 0, sizeof(s_r_opt));
> +	s_r_opt.def = "HEAD";
> +	s_r_opt.revarg_opt = REVARG_COMMITTISH;

This can be:

    struct setup_revision_opt s_r_opt = {
        .def = "HEAD",
        .revarg_opt = REVARG_COMMITTISH,
    };

But I wonder whether we want to pass it at all:

  - `.def` specifies the default, but do we even want to have one when
    the user has passed arguments?

  - `.revarg_opt` makes us treat it like a committish by default, but a
    user may for example want to figure out the size of all objects
    reachable from a specific tree, only.

So maybe we shouldn't be setting this at all and just pass `NULL` to
`setup_revisions()`?

> -	argc = parse_options(argc, argv, prefix, options, repo_structure_usage, 0);
> -	if (argc)
> -		usage(_("too many arguments"));
> +	argc = parse_options(argc, argv, prefix, options, repo_structure_usage,
> +			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);

Makes sense. Here we keep argv0 because of `setup_revisions()`' weird
calling convention. And we also ignore any unknown options so that we
can pass them along, too.

>  	repo_init_revisions(repo, &revs, prefix);
> +	if (argc > 1) {
> +		argc = setup_revisions(argc, argv, &revs, &s_r_opt);
> +		if (argc > 1)
> +			usage(_("too many arguments"));
> +	}
>  
>  	if (show_progress < 0)
>  		show_progress = isatty(2);

And then, if we have any additional parameters then we pass it on to
`setup_revisions()`.

> diff --git a/t/t1901-repo-structure.sh b/t/t1901-repo-structure.sh
> index 02cc2b594a..eb2c595955 100755
> --- a/t/t1901-repo-structure.sh
> +++ b/t/t1901-repo-structure.sh
> @@ -144,6 +144,90 @@ test_expect_success SHA1 'repository with references and objects' '
>  	)
>  '
>  
> +test_expect_success SHA1 'repository with references and objects, filtered' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit_bulk 1005 &&
> +		git tag -a foo -m bar &&
> +
> +		oid="$(git rev-parse HEAD)" &&
> +		git update-ref refs/remotes/origin/foo "$oid" &&
> +		git checkout -b grobble &&
> +		test_commit_bulk --ref=refs/heads/grobble 20 &&
> +		git checkout master &&
> + 		test_commit_bulk 20 &&
> +		# Also creates a commit, tree, and blob.
> +		git notes add -m foo &&
> +
> +		# git-rev-list(1) --disk-usage=human option printing the full
> +		# "byte/bytes" unit string instead of just "B".
> +		cat >expect <<-EOF &&
> +		| Repository structure      | Value      |
> +		| ------------------------- | ---------- |
> +		| * References              |            |
> +		|   * Count                 |      5     |
> +		|     * Branches            |      2     |
> +		|     * Tags                |      1     |
> +		|     * Remotes             |      1     |
> +		|     * Others              |      1     |
> +		|                           |            |
> +		| * Reachable objects       |            |
> +		|   * Count                 |   3.06 k   |
> +		|     * Commits             |   1.05 k   |
> +		|     * Trees               |   1.01 k   |
> +		|     * Blobs               |   1.01 k   |
> +		|     * Tags                |      1     |
> +		|   * Inflated size         |  16.04 MiB |
> +		|     * Commits             | 226.54 KiB |
> +		|     * Trees               |  15.81 MiB |
> +		|     * Blobs               |  11.68 KiB |
> +		|     * Tags                |    132 B   |
> +		|   * Disk size             | $(object_type_disk_usage all true) |
> +		|     * Commits             | $(object_type_disk_usage commit true) |
> +		|     * Trees               | $(object_type_disk_usage tree true) |
> +		|     * Blobs               |  $(object_type_disk_usage blob true) |
> +		|     * Tags                |    $(object_type_disk_usage tag) B   |
> +		|                           |            |
> +		| * Largest objects         |            |
> +		|   * Commits               |            |
> +		|     * Maximum size    [1] |    223 B   |
> +		|     * Maximum parents [2] |      1     |
> +		|   * Trees                 |            |
> +		|     * Maximum size    [3] |  32.29 KiB |
> +		|     * Maximum entries [4] |   1.01 k   |
> +		|   * Blobs                 |            |
> +		|     * Maximum size    [5] |     13 B   |
> +		|   * Tags                  |            |
> +		|     * Maximum size    [6] |    132 B   |
> +
> +		[1] 0dc91eb18580102a3a216c8bfecedeba2b9f9b9a
> +		[2] df6400c01440c329f1011669c4c26cc0c7852887
> +		[3] 60665251ab71dbd8c18d9bf2174f4ee0d58aa06c
> +		[4] 60665251ab71dbd8c18d9bf2174f4ee0d58aa06c
> +		[5] 97d808e45116bf02103490294d3d46dad7a2ac62
> +		[6] 4dae4f5954f5e6feb3577cfb1b181daa3fd3afd2
> +		EOF
> +
> +		git repo structure  >actual 2>actual-err &&
> +		cp actual /tmp/actual &&
> +		cp expect /tmp/expect &&
> +		test_cmp expect actual &&
> +		test_line_count = 0 actual-err &&
> +
> +		git repo structure grobble ^master >actual 2>actual-err &&
> +		cp actual /tmp &&
> +		cp actual-err /tmp &&
> +		test_grep "|     \* Commits             |    21     |" actual &&
> +		test_grep "|     \* Trees               |     2     |" actual &&
> +		test_grep "|     \* Commits             |  4.50 KiB |" actual &&
> +		test_grep "|     \* Trees               | 32.35 KiB |" actual &&
> +		test_grep "|     \* Blobs               | 11.68 KiB |" actual &&
> +		test_line_count = 0 actual-err
> +	)
> +'

I wonder whether we maybe want to have some additional tests that assert
that you can also pass e.g.:

  - A tree or blob.

  - Revision options, like for example `--all --filter=object:type=blob`.

To make the test a bit less repetitive we might also want to use
`--format=lines` and then only check for
"objects.*.{inflated,disk}_size" to exercise only the parts that matter
to this test.

Thanks!

Patrick