From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Wed, 30 Sep 2026 18:31:31 GMT
Subject: Re: [PATCH RFC 5/5] backfill: report total size of missing blobs in --dry-run
Message-ID: <DLSV653N0668.1OZOWE9P5ZJV6@gmail.com>
In-Reply-To: <xmqqqziabqw8.fsf@gitster.g>

On Wed Sep 30, 2026 at 6:17 PM WEST, Junio C Hamano wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> +
>> +	if (!ctx->object_info_enabled)
>> +		goto cleanup;
>> +
>> +	if (!ctx->object_info_transport) {
>> +		struct promisor_remote *promise =
>> +			repo_promisor_remote_find(ctx->repo, NULL);
>> +		struct remote *remote = NULL;
>> +
>> +		if (!promise || !(remote = remote_get(promise->name)))
>> +			die(_("--dry-run requires a promisor remote"));
>> +
>> +		ctx->object_info_transport = transport_get(remote, NULL);
>> +
>> +		if (!ctx->object_info_transport->smart_options)
>> +			die(_("failed to get object info: smart options required"));
>> +	}
>> +
>> +	results->wants_size = 1;
>> +	status = transport_fetch_object_info(ctx->object_info_transport,
>> +					     &ctx->current_batch,
>> +					     results);
>> +
>> +	if (status == FETCH_OBJECT_INFO_NOT_ENABLED ||
>> +	    !results->sizes) {
>> +		ctx->object_info_enabled = 0;
>
> Yuck.
>
> Because we cannot tell if they allow you to look at the information,
> this cannot be helped, but it means anybody that looks at this
> ctx->object_info_enabled member to decide what to do must be careful.
>
> Is it guaranteed that results.sizes[] have been populated as long as
> status is not FETCH_OBJECT_INFO_NOT_ENABLED?  Can there be other
> errors that makes result.sizes[] unusable?  If that is the case,
> then it would be cleaner to have a dedicated ctx->sizes_valid member
> rather than relying on ctx->object_info_enabled member to carry this
> information ...

Not quite, transport_fetch_object_info() can also return
FETCH_OBJECT_INFO_ERR when finish_connect() fails in
fetch_object_info_via_pack(). I'll change the condition to:

	if (status != FETCH_OBJECT_INFO_OK || !results->sizes) {

With FETCH_OBJECT_INFO_OK, fetch_object_info() only allocates
results->sizes when the server answers with the "size" attribute,
and it die()s on any incomplete or malformed response, so a
non-NULL sizes[] is always fully populated. A NULL sizes[] with
FETCH_OBJECT_INFO_OK means object-info is available but the server
does not support "size".

Agreed, the member is also cleared in that last case, where
object-info does work but "size" is not advertised, so I get that it can
be misleading. Since its only use is deciding whether to report the 
total size, I'll rename it to ctx->sizes_valid instead of adding a 
separate member.

>
>> +		goto cleanup;
>> +	}
>> +
>> +	for (size_t i = 0; i < results->nr; i++)
>> +		ctx->total_batch_size += results->sizes[i];
>> +
>> +cleanup:
>> +	free_fetch_object_info_results(&ctx->object_info_results);
>>  	oid_array_clear(&ctx->current_batch);
>>  }
>>  
>> @@ -157,12 +201,25 @@ static int do_backfill(struct backfill_context *ctx)
>>  
>>  	dry_run_batch(ctx);
>>  
>> -	printf(Q_("After backfill, %" PRIuMAX " blob would be fetched.\n",
>> -		  "After backfill, %" PRIuMAX " blobs would be fetched.\n",
>> -		  (unsigned long)ctx->total_batch_nr),
>> -	       (uintmax_t)ctx->total_batch_nr);
>> +	if (ctx->object_info_enabled && ctx->total_batch_nr) {
>
> ... and use it here.  Within the design presented in this series, we
> know we have asked the other end at this point, and the above
> function may have turned ctx->object_info member off if the
> information is not there, so this may be safe.  But as I said, I am
> not sure what happens when fetch-object-info returned other kind of
> errors.

The condition above is checked on every batch, and any status other
than FETCH_OBJECT_INFO_OK or a NULL sizes[] clears ctx->sizes_valid
(currently ctx->object_info_enabled), so by the time we get here it
is only set when all the batches returned valid sizes.