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

["Mark C. Chu-Carroll" <markchucarroll@fastmail.com>]

On Tue Oct 13, 2026 at 7:50 AM EDT, Patrick Steinhardt wrote:
> 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.

I guess my wording wasn't clear. The tradeoff here isn't that
the revision filters don't affect refs - it's that when we use
setup_revisions, we end up doing the ref-walk twice. 

>> 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.

Sounds good to me.

>
>> @@ -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?

Yes. One of these days, I'm going to learn to type. That's supposed to
be "git repo structure --all ^foo". 


>> 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.

I went back and forth on this a fair bit. Both approaches work,
and both have their downsides. In the end, I felt like munging argv
to add the "--all" was uglier than this. (Plus, when you do the default
"--all", the command will always fail on an empty repos, because the
"all" logic tries to call "refs_head_ref", but there are no head refs in
a newly initialized empty repo.) 

>>  	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.

Will do.

>
>> 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' '
>> ...
>> -	)
>> -'
>
> Huh, why did you drop this test?

When I was floundering about whether to use "--all" default, I found that the
command will fail on an empty repos. Even without the "--all" default, it seems
strange that some version of "git repo structure" will succeed, and others will
fail with a confusing error ("'HEAD': unknown revision or path not in the
working tree."). It seemed to me that when you consider that running this
command in an empty repo is pointless, I thought it made sense to drop that.
I'm happy to either add a line to the documentation, or to reinstate the test.


>> @@ -144,6 +92,165 @@ test_expect_success SHA1 'repository with references and objects' '
>> ...
>> +		git repo structure --format=lines  >actual 2>actual-err &&
>
> There's two spaces here between `--format=lines  >actual`.

Good catch, I have no idea how anyone can see that.

>
>> +		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).

Good idea, will do.






-- 
Mark Craig Chu-Carroll (@MarkChuCarroll at gitlab)
*** Software Tools/Math Geek - Software Engineer at Gitlab
*** Work Email: mcarroll@gitlab.com / markchucarroll@fastmail.com
*** Personal Blog: http://goodmath.org/blog / Personal email: markcc@gmail.com