From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 26 Feb 2026 19:50:11 GMT
Subject: Re: [PATCH v2 2/5] builtin/repo: collect largest inflated objects
Message-ID: <xmqqv7fj1dzg.fsf@gitster.g>
In-Reply-To: <20260223174120.2356504-3-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

> @@ -485,6 +514,23 @@ static void structure_keyvalue_print(struct repo_structure *stats,
>  	printf("objects.tags.disk_size%c%" PRIuMAX "%c", key_delim,
>  	       (uintmax_t)stats->objects.disk_sizes.tags, value_delim);
>  
> +	printf("objects.commits.max_size%c%" PRIuMAX "%c", key_delim,
> +	       (uintmax_t)stats->objects.largest.commit_size.value, value_delim);
> +	printf("objects.commits.max_size_oid%c%s%c", key_delim,
> +	       oid_to_hex(&stats->objects.largest.commit_size.oid), value_delim);
> +	printf("objects.trees.max_size%c%" PRIuMAX "%c", key_delim,
> +	       (uintmax_t)stats->objects.largest.tree_size.value, value_delim);
> +	printf("objects.trees.max_size_oid%c%s%c", key_delim,
> +	       oid_to_hex(&stats->objects.largest.tree_size.oid), value_delim);
> +	printf("objects.blobs.max_size%c%" PRIuMAX "%c", key_delim,
> +	       (uintmax_t)stats->objects.largest.blob_size.value, value_delim);
> +	printf("objects.blobs.max_size_oid%c%s%c", key_delim,
> +	       oid_to_hex(&stats->objects.largest.blob_size.oid), value_delim);
> +	printf("objects.tags.max_size%c%" PRIuMAX "%c", key_delim,
> +	       (uintmax_t)stats->objects.largest.tag_size.value, value_delim);
> +	printf("objects.tags.max_size_oid%c%s%c", key_delim,
> +	       oid_to_hex(&stats->objects.largest.tag_size.oid), value_delim);

The repetition tires reviewers' eyes.  I am reasonably sure if there
were an intentional copy-and-paste error, I wouldn't be able to spot
it.  But I tried to be careful and read it over three times ;-).

> @@ -553,6 +599,15 @@ struct count_objects_data {
>  	struct progress *progress;
>  };
>  
> +static void check_largest(struct object_data *data, struct object_id *oid,
> +			  size_t value)
> +{
> +	if (value > data->value) {
> +		oidcpy(&data->oid, oid);
> +		data->value = value;
> +	}
> +}

How important is it for this application to end up with a valid
value in data->oid?

If data->value is initialized to a valid value, instead of an
impossible sentinel value that is strictly smaller than any valid
values, this can leave data->value to a valid value from an existing
object without recording its object name.  Imagine a repository with
a single empty blob, and data->value initialized to zero (it cannot
be initialized to a sentinel -1, as use of size_t here makes it
impossible to have any reasonable sentinel values).


> @@ -138,6 +158,14 @@ test_expect_success SHA1 'keyvalue and nul format' '
>  		objects.trees.disk_size=$(object_type_disk_usage tree)
>  		objects.blobs.disk_size=$(object_type_disk_usage blob)
>  		objects.tags.disk_size=$(object_type_disk_usage tag)
> +		objects.commits.max_size=221
> +		objects.commits.max_size_oid=de3508174b5c2ace6993da67cae9be9069e2df39
> +		objects.trees.max_size=1335
> +		objects.trees.max_size_oid=09931deea9d81ec21300d3e13c74412f32eacec5
> +		objects.blobs.max_size=11
> +		objects.blobs.max_size_oid=eaeeedced46482bd4281fda5a5f05ce24854151f
> +		objects.tags.max_size=132
> +		objects.tags.max_size_oid=1ee0f2b16ea37d895dbe9dbd76cd2ac70446176c
>  		EOF
>  
>  		git repo structure --format=keyvalue >out 2>err &&