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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 13, 2026 at 09:56:53AM -0400, Mark C. Chu-Carroll wrote:
> 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. 

Ah, right, that makes sense. I really don't think that's much of an
issue though. We of course shouldn't be unreasonably wasteful, but it's
not like `git repo structure` would be on any critical path. Also, in
most repositories I would claim that the time it takes to enumerate all
objects is going to significantly outweight the time it takes to
enumerate all refs anyway.

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

Yeah, that part is a bit ugly indeed.

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

I think we should ideally know to handle this case alright. It's
probably fine to abort in the case where the user explicitly asks for
refs that we cannot parse, including in the case of "--all".

But I really think we should succeed in an empty repository, because
that's a state that is quite common. So I'd definitely retain that test
and make sure that at least `git repo structure` without any args works
alright.

Patrick