Re: [PATCH RFC 4/5] backfill: add --dry-run option

> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> Users have no way to know how many blobs git backfill is going to
>> download before running it.
>>
>> Add a new --dry-run option to the backfill command. The objects are
>> walked as usual, but instead of fetching each batch of missing blobs
>> they are only counted, and the total is printed at the end.
>>
>> A subsequent commit will also print their size when the server supports
>> the object-info capability.
>>
>> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
>> ---
>>  Documentation/git-backfill.adoc |  6 +++++-
>>  builtin/backfill.c              | 37 +++++++++++++++++++++++++++++++++----
>>  t/t5620-backfill.sh             | 26 ++++++++++++++++++++++++++
>>  3 files changed, 64 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/git-backfill.adoc b/Documentation/git-backfill.adoc
>> index 82d6a1969d..08f19fea17 100644
>> --- a/Documentation/git-backfill.adoc
>> +++ b/Documentation/git-backfill.adoc
>> @@ -9,7 +9,7 @@ git-backfill - Download missing objects in a partial clone
>>  SYNOPSIS
>>  --------
>>  [synopsis]
>> -git backfill [--min-batch-size=<n>] [--[no-]sparse] [--[no-]include-edges] [<revision-range>]
>> +git backfill [--min-batch-size=<n>] [--[no-]sparse] [--[no-]include-edges] [--dry-run] [<revision-range>]
>>
>>  DESCRIPTION
>>  -----------
>> @@ -70,6 +70,10 @@ OPTIONS
>>  	--onto TARGET A..B`, where A..B normally excludes A but you need
>>  	the blobs from A as well.  `--include-edges` is the default.
>>
>> +`--dry-run`::
>> +	Do not download any objects. Instead, print the number of
>> +	missing blobs that would be downloaded.
>> +
>>  `<revision-range>`::
>>  	Backfill only blobs reachable from commits in the specified
>>  	revision range.  When no _<revision-range>_ is specified, it
>> diff --git a/builtin/backfill.c b/builtin/backfill.c
>> index e71e0f4742..6019112966 100644
>> --- a/builtin/backfill.c
>> +++ b/builtin/backfill.c
>> @@ -26,7 +26,7 @@
>>  #include "path-walk.h"
>>
>>  static const char * const builtin_backfill_usage[] = {
>> -	N_("git backfill [--min-batch-size=<n>] [--[no-]sparse] [--[no-]include-edges] [<revision-range>]"),
>> +	N_("git backfill [--min-batch-size=<n>] [--[no-]sparse] [--[no-]include-edges] [--dry-run] [<revision-range>]"),
>>  	NULL
>>  };
>>
>> @@ -36,6 +36,8 @@ struct backfill_context {
>>  	size_t min_batch_size;
>>  	int sparse;
>>  	int include_edges;
>> +	int dry_run;
>
> Nit: This could be a bool, since `OPT__DRY_RUN` uses `OPT_BOOL` internally.

>
>> +	size_t total_batch_nr;
>
>
>>  	struct rev_info revs;
>>  };
>>
>> @@ -58,6 +60,15 @@ static void download_batch(struct backfill_context *ctx)
>>  	odb_reprepare(ctx->repo->objects);
>>  }
>>
>> +static void dry_run_batch(struct backfill_context *ctx)
>> +{
>
> While it is used during dry_run, probably makes more sense to rename it
> to `count_batch()` since that's what it does.

>
>> +	if (!ctx->current_batch.nr)
>> +		return;
>> +
>> +	ctx->total_batch_nr += ctx->current_batch.nr;
>> +	oid_array_clear(&ctx->current_batch);
>> +}
>> +
>>  static int fill_missing_blobs(const char *path UNUSED,
>>  			      struct oid_array *list,
>>  			      enum object_type type,
>> @@ -73,8 +84,12 @@ static int fill_missing_blobs(const char *path UNUSED,
>>  			oid_array_append(&ctx->current_batch, &list->oid[i]);
>>  	}
>>
>> -	if (ctx->current_batch.nr >= ctx->min_batch_size)
>> -		download_batch(ctx);
>> +	if (ctx->current_batch.nr >= ctx->min_batch_size) {
>> +		if (ctx->dry_run)
>> +			dry_run_batch(ctx);
>> +		else
>> +			download_batch(ctx);
>> +	}
>>
>>  	return 0;
>>  }
>> @@ -131,10 +146,23 @@ static int do_backfill(struct backfill_context *ctx)
>>
>>  	ret = walk_objects_by_path(&info);
>>
>> +	if (ret)
>> +		goto end;
>> +
>>  	/* Download the objects that did not fill a batch. */
>> -	if (!ret)
>> +	if (!ctx->dry_run) {
>>  		download_batch(ctx);
>> +		goto end;
>> +	}
>> +
>> +	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);
>>
>
> Nit: Okay so we have a goto inside the first if(...), which skips this
> section. I would have found it easier to read if it was
>
> if (dry_run)
>    count()
> else
>    download()