From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 27 Oct 2026 14:55:19 GMT
Subject: Re: [PATCH v2 1/3] repo: add level of concern reporting
Message-ID: <auC7TjiE_KdoTGyO@pks.im>
In-Reply-To: <20261026-b4-repo-structure-concern-v2-1-e0df34228dc8@fastmail.com>

On Mon, Oct 26, 2026 at 01:43:54PM -0400, Mark C. Chu-Carroll wrote:
> "git repo structure" computes a set of statistics
> that are useful for diagnosing performance issues in
> a git repository. But for a typical user who hasn't
> encountered any issues yet, it can be difficult to
> interpret the data - when should a value be considered
> concerning?
> 
> Add an option to "git repo structure" that adds a "degree
> of concern" column to the structure report, which indicates
> when a stat values has reached a point where it might be
> concerning, and indicate how severe the concern should be.

Missing your Signed-off-by.

I think the commit message should also mention git-sizer(1) here, which
is the direct inspiration that we have for this additional data. To the
best of my knowledge, the level of concern is essentially the last item
that is missing for feature compatibility between these two tools.

Another interesting thing to document here is how you picked the level
of concern in the first place.

> diff --git a/builtin/repo.c b/builtin/repo.c
> index 84e012f83f..a450497d4e 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -348,8 +348,118 @@ struct stats_table {
>  	int name_col_width;
>  	int value_col_width;
>  	int unit_col_width;
> +	int concern_col_width;
>  };
>  
> +/*
> + * For computing level of concern about a stat, we need to have some
> + * baseline values. For each of these, we'define a minimum level
> + * of concern, which will appear as 1 star. Then we scale exponentially:
> + * each time that value doubles, we add another star.
> + *
> + * The threshold values for concern are expressed as log_2 values of the size
> + * at which the statistic becomes concerning. The default values for the
> + * threshold were selected based on observed values from a collection of
> + * representative-ish repositories of varying size, by finding the mean log_2
> + * value and standard deviation (sigma) for each statistic over the collection.
> + * The threshold for each metric is then set at mean(log_2) + sigma.
> + */
> +struct concern_stat_threshold {
> +	const char *name;
> +	size_t threshold;
> +};
> +
> +enum concern_stat {
> +	CONCERN_STAT_COMMIT_COUNT,
> +	CONCERN_STAT_TREE_COUNT,
> +	CONCERN_STAT_BLOB_COUNT,
> +	CONCERN_STAT_TAG_COUNT,
> +	CONCERN_STAT_COMMIT_SIZE,
> +	CONCERN_STAT_TREE_SIZE,
> +	CONCERN_STAT_BLOB_SIZE,
> +	CONCERN_STAT_TAG_SIZE,
> +	CONCERN_STAT_COMMIT_LARGEST,
> +	CONCERN_STAT_TREE_LARGEST,
> +	CONCERN_STAT_BLOB_LARGEST,
> +	CONCERN_STAT_TAG_LARGEST,
> +	CONCERN_STAT_COMMIT_INFLATED_SIZE,
> +	CONCERN_STAT_TREE_INFLATED_SIZE,
> +	CONCERN_STAT_BLOB_INFLATED_SIZE,
> +	CONCERN_STAT_TAG_INFLATED_SIZE,
> +};
> +
> +struct concern_stat_threshold repo_structure_concern_thresholds[] = {
> +	{ .name = "commit_count", .threshold = 19 },
> +	{.name = "tree_count", .threshold = 21 },

Nit: missing space.

> +	{ .name = "blob_count", .threshold = 22 },
> +	{ .name = "tag_count", .threshold = 15 },
> +	{ .name = "commit_size", .threshold = 28 },
> +	{ .name = "tree_size", .threshold = 27 },
> +	{ .name = "blob_size", .threshold = 36 },
> +	{ .name = "tags_size", .threshold = 18 },
> +	{ .name = "commit_largest", .threshold = 18  },
> +	{ .name = "tree_largest", .threshold = 18 },
> +	{ .name = "blob_largest", .threshold = 26 },
> +	{ .name = "tag_largest", .threshold =  15 },
> +	{ .name = "commit_inflated_size", .threshold = 28 },
> +	{ .name = "tree_inflated_size", .threshold = 32 },
> +	{ .name = "blob_inflated_size", .threshold = 34 },
> +	{ .name = "tag_inflated_size", .threshold = 25}
> +};

I think this is a tiny bit unwieldy, as you now have to remember to
update multiple sites whenever you add another metric to this command.
Ideally, we'd have a single structure that knows to track all
information that we're about to print so that we can define this in a
single, central location.

For example, we could update `struct object_data` to have a configurable
threshold, and then do the same for other such structures. But that
might also get unwieldy by itself.

I think what bugs me a bit is that we have not only two, but three
locations because we have the enum, the thresholds and then also the
data structures themselves.

I'm not quite sure how to fix that myself though.

> +/*
> + * Concern thresholds aren't universal. We try to set reasonable default
> + * values, but projects may want to set these thresholds differently. To
> + * support that, they can set the thresholds for their project in the
> + * project's git config, in the section "repo.structure.concern".
> +  */
> +static int config_concern_thresholds(const char *prefixed_var,
> +				     const char *str_val,
> +				     const struct config_context *ctx,
> +				     void *cb UNUSED) {

We should probably use `cb` to pass around the structure that you
populate so that we can avoid global variables.

> +	const char *config_prefix = "repo.structure.concern.";
> +	if (!strncmp(config_prefix, prefixed_var, strlen(config_prefix))) {

We have `starts_with()`. But also note that if this is a config key then
it should match case-insensitively, so it should rather be
`istarts_with()`.

The config keys are also a bit off as our format only has nested keys up
to three levels deep, not four.

> +		const char *concern_name = &prefixed_var[strlen(config_prefix)];
> +		for (int i = 0; concern_stat_thresholds[i].name != 0; i++) {

We typically don't compare with a zero value directly, and indices
should be of type `size_t` when iterating, so this should rather be:

        for (size_t i = 0; concern_stat_thresholds[i].name; i++)

But the loop condition is a bit weird, because you don't have a final
entry there that has a guaranteed NULL name. So shouldn't we rather
iterate through `ARRAY_SIZE()`?

Also, I may be blind, but where is that struct getting defined? I can
only see `repo_structure_concern_thresholds`.

> +			if (!strcmp(concern_stat_thresholds[i].name, concern_name)) {
> +				int value = git_config_int(prefixed_var, str_val, ctx->kvi);
> +				if (value < 0)
> +					return -1;
> +				concern_stat_thresholds[i].threshold = value;
> +				return 0;
> +			}
> +		}
> +		warning(_("Invalid concern threshold name: '%s'"), concern_name);
> +		return 0;
> +	}
> +	return 0;
> +}
> +
> +static size_t approx_log2(long value) {

We have `log2u()` in "wrapper.h". Can we use that instead?

> @@ -681,12 +887,15 @@ static void print_object_data(const char *key, char key_delim,
>  }
>  
>  static void structure_keyvalue_print(struct repo_structure *stats,
> -				     char key_delim, char value_delim)
> +				     char key_delim, char value_delim,
> +				     bool show_concern)
>  {
>  	print_keyvalue("references.branches.count", key_delim,
>  		       stats->refs.branches, value_delim);
> +
>  	print_keyvalue("references.tags.count", key_delim,
>  		       stats->refs.tags, value_delim);
> +

Nit: stray newlines.

> diff --git a/sideband.c b/sideband.c
> index 374187402e..0e75ab2317 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -1,4 +1,4 @@
> -#define USE_THE_REPOSITORY_VARIABLE
> +define USE_THE_REPOSITORY_VARIABLE
>  #define DISABLE_SIGN_COMPARE_WARNINGS
>  
>  #include "git-compat-util.h"

This change probably shouldn't be here.

Patrick