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

[Patrick Steinhardt <ps@pks.im>]

On Sat, Oct 10, 2026 at 11:05:41AM -0400, Mark C. Chu-Carroll wrote:
[snip]
> This implementation makes an awkward trade-off involving
> "setup_revisions". In order to accept standard the standard
> revs parameters, it uses `setup_revisions`, which does a ref
> walk and populates the pending objects list that will be used
> by "repo structure". But this ref walk doesn't count refs in the
> way that "repo structure" requires, and because "setup_revisions"
> doesn't support a custom callback for its ref-walk, that's not
> changeable.

It's a tradeoff indeed, but I think it's a sensible one. I see these as
counting two different things, refs and objects, and the revisions only
filter objects.

> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> index ed7d80c690..67be7efd23 100644
> --- a/Documentation/git-repo.adoc
> +++ b/Documentation/git-repo.adoc
> @@ -66,6 +66,12 @@ 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 revisions to select which objects will be counted. These parameters
> +follow the same syntax as the parameters to similar commands like
> +`git log`. If no includes are specified, then the include set is all
> +reachable objects.

Nit: I think it would make sense to define "reachable" in this case, as
there are a bunch of data structures that could in theory make an object
reachable. How about this instead:

  Defaults to all objects reachable from a reference.

> @@ -141,6 +147,28 @@ using the `nul` format:
>  git repo info --format=nul layout.bare layout.shallow
>  ------------
>  
> +* Generates information about storage usage in the repository:
> ++
> +------------
> +git repo structure
> +------------
> ++
> +
> +* Generates information about storage usage in the repository omitting
> +the branch "foo":
> ++
> +------------
> +git repo structure ^foo
> +------------

Wait, wouldn't this result in the empty set of objects because there are
no positive revisions?

> diff --git a/builtin/repo.c b/builtin/repo.c
> index 84e012f83f..081940a85c 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -766,10 +767,12 @@ static int count_references(const struct reference *ref, void *cb_data)
>  	}
>  
>  	/*
> -	 * While iterating through references for counting, also add OIDs in
> +	 * While iterating through references for counting, if we didn't already
> +	 * populate the pending list in setup_revisions, also add OIDs in
>  	 * preparation for the path walk.
>  	 */
> -	add_pending_oid(data->revs, NULL, ref->oid, 0);
> +	if (data->add_to_pending)
> +		add_pending_oid(data->revs, NULL, ref->oid, 0);
>  
>  	ref_count = get_total_reference_count(stats);
>  	display_progress(data->progress, ref_count);

Right. An alternative would be to simply never call `add_pending_oid()`
here and always let the revision walk handle this by defaulting to
"--all". That'd have the downside of walking references twice, but the
upside of making each of the steps completely self-contained.

I think ultimately it doesn't matter too much which of these two
approaches we pick. I'd have picked defaulting to "--all" myself, but I
won't insist on it.

> @@ -947,16 +953,20 @@ static int cmd_repo_structure(int argc, const char **argv, const char *prefix,
>  		OPT_END()
>  	};
>  
> -	argc = parse_options(argc, argv, prefix, options, repo_structure_usage, 0);
> -	if (argc)
> -		usage(_("too many arguments"));
> -
> -	repo_init_revisions(repo, &revs, prefix);
> -
> +	argc = parse_options(argc, argv, prefix, options, repo_structure_usage,
> +			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT);

Yes. We require `ARGV0` because of `seutp_revisions()` weird API, and
unknown options should of course be forwarded to it.

>  	if (show_progress < 0)
>  		show_progress = isatty(2);
>  
> -	structure_count_references(&stats.refs, &revs, repo, show_progress);
> +	repo_init_revisions(repo, &revs, prefix);
> +	if (argc > 1) {
> +		argc = setup_revisions(argc, argv, &revs, NULL);
> +		included_revision_args = true;
> +	}
> +	if (argc > 1)
> +		usage(_("too many arguments"));

Tiny nit: this reads a bit odd because we have the same condition twice,
and the second evaluation really only needs to happen in case we have
called `setup_revisions()`. So I'd have moved the second check right
after the call to that function.

> diff --git a/t/t1901-repo-structure.sh b/t/t1901-repo-structure.sh
> index 02cc2b594a..e35b46a277 100755
> --- a/t/t1901-repo-structure.sh
> +++ b/t/t1901-repo-structure.sh
> @@ -21,58 +21,6 @@ object_type_disk_usage() {
>  	fi
>  }
>  
> -test_expect_success 'empty repository' '
> -	test_when_finished "rm -rf repo" &&
> -	git init repo &&
> -	(
> -		cd repo &&
> -		cat >expect <<-\EOF &&
> -		| Repository structure      | Value  |
> -		| ------------------------- | ------ |
> -		| * References              |        |
> -		|   * Count                 |    0   |
> -		|     * Branches            |    0   |
> -		|     * Tags                |    0   |
> -		|     * Remotes             |    0   |
> -		|     * Others              |    0   |
> -		|                           |        |
> -		| * Reachable objects       |        |
> -		|   * Count                 |    0   |
> -		|     * Commits             |    0   |
> -		|     * Trees               |    0   |
> -		|     * Blobs               |    0   |
> -		|     * Tags                |    0   |
> -		|   * Inflated size         |    0 B |
> -		|     * Commits             |    0 B |
> -		|     * Trees               |    0 B |
> -		|     * Blobs               |    0 B |
> -		|     * Tags                |    0 B |
> -		|   * Disk size             |    0 B |
> -		|     * Commits             |    0 B |
> -		|     * Trees               |    0 B |
> -		|     * Blobs               |    0 B |
> -		|     * Tags                |    0 B |
> -		|                           |        |
> -		| * Largest objects         |        |
> -		|   * Commits               |        |
> -		|     * Maximum size        |    0 B |
> -		|     * Maximum parents     |    0   |
> -		|   * Trees                 |        |
> -		|     * Maximum size        |    0 B |
> -		|     * Maximum entries     |    0   |
> -		|   * Blobs                 |        |
> -		|     * Maximum size        |    0 B |
> -		|   * Tags                  |        |
> -		|     * Maximum size        |    0 B |
> -		EOF
> -
> -		git repo structure >out 2>err &&
> -
> -		test_cmp expect out &&
> -		test_line_count = 0 err
> -	)
> -'

Huh, why did you drop this test?

> @@ -144,6 +92,165 @@ test_expect_success SHA1 'repository with references and objects' '
>  	)
>  '
>  
> +test_expect_success SHA1 'repository with branches' '
> +	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 repo structure --format=lines  >actual 2>actual-err &&

There's two spaces here between `--format=lines  >actual`.

> +		test_line_count = 0 actual-err &&

`test_must_be_empty actual-err`. The same applies to some lines further
down.

> +		test_grep "objects.commits.inflated.size=231976" actual &&
> +		test_grep "objects.commits.count=1046" actual &&
> +		test_grep "objects.blobs.count=1006" actual &&
> +		test_grep "objects.trees.count=1006" actual &&
> +		test_grep "objects.trees.disk_size=284547" actual &&
> +		test_grep "objects.blobs.disk_size=21022" actual &&
> +		test_grep "objects.tags.disk_size=126" actual &&

I think this would be a bit easier to parse if you instead:

    cat >expect <<-\EOF &&
    objects.commits.inflated.size=231976
    objects.commits.count=1046
    objects.blobs.count=1006
    objects.trees.count=1006
    objects.trees.disk_size=284547
    objects.blobs.disk_size=21022
    objects.tags.disk_size=126
    EOF
    test_grep "^objects" actual >actual.objects &&
    test_cmp expect actual.objects

It's also more efficient because we don't have to invoke grep 8 times.
But we'd also have to add the inflated size (which I've omitted out of
laziness).

> +test_expect_success SHA1 'filter by both positive and negative revs' '
> +	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 repo structure --format=lines  >actual 2>actual-err &&
> +		test_line_count = 0 actual-err &&
> +
> +		git repo structure --format=lines grobble ^master >actual 2>actual-err &&
> +		test_grep "objects.commits.count=20" actual &&
> +		test_grep "objects.commits.disk_size=3122" actual &&
> +		test_grep "objects.commits.inflated.size=4411" actual &&
> +		test_grep "objects.blobs.count=1005" actual &&
> +		test_grep "objects.blobs.disk_size=21003" actual &&
> +		test_grep "objects.blobs.inflated_size=11958" actual &&
> +		test_grep "objects.trees.count=1" actual &&
> +		test_grep "objects.trees.disk_size=53" actual &&
> +		test_grep "objects.trees.inflated_size=33063" actual &&
> +		test_grep "objects.tags.disk_size=0" actual &&
> +		test_line_count = 0 actual-err
> +	)
> +'
> +
> +test_expect_success 'filter by object type' '
> +	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 repo structure --format=lines --objects --all --filter=object:type=blob >actual 2>actual-err &&
> +		cp actual /tmp/test-actual-exclude-blob &&
> +		test_grep "objects.commits.disk_size=0" actual &&
> +		test_grep "objects.commits.max_size=0" actual &&
> +		test_grep "objects.tags.disk_size=0" actual &&
> +		test_grep "objects.tags.inflated_size=0" actual &&
> +		test_grep "objects.trees.disk_size=0" actual &&
> +		test_grep "objects.trees.inflated_size=0" actual &&
> +		test_grep "objects.blobs.disk_size=21022" actual &&
> +		test_grep "objects.blobs.count=1006" actual &&
> +		test_grep "objects.blobs.disk_size=21022" actual &&
> +		test_grep "objects.blobs.inflated_size=11962" actual &&
> +		test_line_count = 0 actual-err
> +	)
> +'
> +
> +test_expect_success 'filter using by ref^{tree}' '

I think I'd drop this test here as we're now starting to cross the
border of exercising the rev-walk machinery. The preceding tests seem
sufficient as you have tested positive revs, negative revs and options.

Other than that though I think this is on the right track, thanks!

Patrick