From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 16 Oct 2025 10:58:16 GMT
Subject: Re: [PATCH v5 3/6] builtin/repo: introduce structure subcommand
Message-ID: <aPDPyEVOTSU6NuOH@pks.im>
In-Reply-To: <20251015211213.361797-4-jltobler@gmail.com>

On Wed, Oct 15, 2025 at 04:12:10PM -0500, Justin Tobler wrote:
> diff --git a/builtin/repo.c b/builtin/repo.c
> index eeeab8fbd2..4575cf9467 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -156,12 +160,201 @@ static int cmd_repo_info(int argc, const char **argv, const char *prefix,
[snip]
> +static void structure_count_references(struct ref_stats *stats,
> +				       struct ref_array *refs)
> +{
> +	for (int i = 0; i < refs->nr; i++) {
> +		struct ref_array_item *ref = refs->items[i];
> +
> +		switch (ref->kind) {
> +		case FILTER_REFS_BRANCHES:
> +			stats->branches++;
> +			break;
> +		case FILTER_REFS_REMOTES:
> +			stats->remotes++;
> +			break;
> +		case FILTER_REFS_TAGS:
> +			stats->tags++;
> +			break;
> +		case FILTER_REFS_OTHERS:
> +			stats->others++;
> +			break;
> +		default:
> +			BUG("unexpected reference type");
> +		}
> +	}
> +}
> +
> +static int cmd_repo_structure(int argc, const char **argv, const char *prefix,
> +			      struct repository *repo UNUSED)
> +{
> +	struct ref_filter filter = REF_FILTER_INIT;
> +	struct stats_table table = {
> +		.rows = STRING_LIST_INIT_DUP,
> +	};
> +	struct ref_stats stats = { 0 };
> +	struct ref_array refs = { 0 };
> +	struct option options[] = { 0 };
> +
> +	argc = parse_options(argc, argv, prefix, options, repo_usage, 0);
> +	if (argc)
> +		usage(_("too many arguments"));
> +
> +	if (filter_refs(&refs, &filter, FILTER_REFS_REGULAR))
> +		die(_("unable to filter refs"));
> +
> +	structure_count_references(&stats, &refs);

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.

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

The other patches all look good to me, thanks!

Patrick