Re: [PATCH v2 2/5] builtin/repo: collect largest inflated objects

[Justin Tobler <jltobler@gmail.com>]

On 26/02/26 11:50AM, Junio C Hamano wrote:
> 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 ;-).

Ya, I was thinking about adding another patch that reduces the
duplication for the output here. I'll go ahead and do that in the next
version.

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

So in cases where we do not record an OID for an object, the table
output format knows not to show any annotations and the machine parsable
formats display null OIDs. In the example you provided though, this
technically wouldn't be correct though as it possible we could have an
empty blob.

One way we could deal which this is have a sentinel value of -1 for the
size value as you mentioned. Another option could be to check if the OID
is a null value and if so record the value regardless. I'll work on this
in the next version.

Thanks,
-Justin