Re: [PATCH v5 3/6] builtin/repo: introduce structure subcommand

From: Justin Tobler <jltobler@gmail.com>
Date: Oct 21, 2025, 16:04 UTC
Message-ID: <4csjjw5sin2luhsl2pq5g3ra56r463h6arc2vwfnitwqhhrww7@s6y7hhvgzwui>
In-Reply-To: <aPDPyEVOTSU6NuOH@pks.im>

On 25/10/16 12:58PM, Patrick Steinhardt wrote:

Show 13 quoted lines

> I only noticed this when taking a look at the last patch that introduces
> a progress meter, but I think we should change how we count references.
> The way you do it here means that we have to temporarily store all refs
> in an array, which is completely unnecessary and thus a waste of memory.
> Furthermore, the resulting progress meter will be somewhat useless
> because it only starts counting _after_ we have enumerated all
> references already. The second phase where we basically just classify
> the refs by type is going to be orders of magnitude faster and probably
> not noticeable even with millions of refs.
> 
> Instead, I think we should use e.g. `refs_for_each_ref()` and count them
> in the callback function. This means we don't have to store them
> anymore, and also the progress meter becomes more useful.

One nice thing about `filter_refs()` is that we can easily set it up to exclude certain sets of references. In the future, I do forsee options being introduced that restrict what references we use when performing this operation. We really only need to iterate through the references once though to count references and append OIDs in preparation for the path walk. So I agree that storing all the references in an array is quite wasteful.

In the next version, I'll update to use `refs_for_each_ref()` and set up a callback that counts references and appends their OIDs in preparation for the path walk in a single iteration. When we introduce the ability to restrict references, we could export `do_filter_refs()` via "ref-filter.h" and be able to reuse the existing filtering machinery.

> The only downside is that we cannot set up the progress meter with an
> upper limit. But I think that is an acceptable tradeoff.

I agree that this is only a minor downside and sounds reasonable.

-Justin