Re: [PATCH RFC 5/5] backfill: report total size of missing blobs in --dry-run

[Junio C Hamano <gitster@pobox.com>]

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

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