Re: [PATCH v2 2/5] builtin/repo: collect largest inflated objects
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Mar 2, 2026, 17:28 UTC
- Message-ID
- <aaXFcz8AQrwRtr5C@denethor>
- In-Reply-To
- <xmqqv7fj1dzg.fsf@gitster.g>
On 26/02/26 11:50AM, Junio C Hamano wrote:
Show 26 quoted lines
> 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.
Show 23 quoted lines
> > @@ -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