Re: [PATCH v2 1/3] repo: add level of concern reporting

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 27, 2026, 14:55 UTC
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:

Show 11 quoted lines

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

Show 51 quoted lines

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

Show 15 quoted lines

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

Show 10 quoted lines

> +/*
> + * 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`.

Show 15 quoted lines

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

Show 14 quoted lines

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

Show 10 quoted lines

> 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