Re: [PATCH v2 3/4] index-pack: add --progress-title option

[Derrick Stolee <stolee@gmail.com>]

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:
> +--progress-title::
> +	For internal use only.
> ++
> +Set the title of the "Receiving objects" progress bar (it's "Indexing
> +objects" under `--stdin`).

May I suggest a minor edit:

  Set the title of the progress bar. The title is "Receiving objects"
  by default and "Indexing objects" when `--stdin` is specified.

> @@ -1806,6 +1808,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
>  				input_len = sizeof(*hdr);
>  			} else if (!strcmp(arg, "-v")) {
>  				verbose = 1;
> +			} else if (!strcmp(arg, "--progress-title")) {

Unfortunate that we are not using the parse-opts API. Not your fault.

> +				if (progress_title || (i+1) >= argc)

style nit:

	if (progress_title || i + 1 >= argc)

Although, I notice a similar line elsewhere in the file, so this gets
a pass.

	if (index_name || (i+1) >= argc)

> +					usage(index_pack_usage);
> +				progress_title = argv[++i];

One downside to this organization is that `--progress-title=X` will
not work here. There are other `--<option-name>=X` options in this
builtin, and the index output name is specified with the short name
`-o X`. We should probably err to match the `--<option-name>=X`
pattern in this file for now. An eventual conversion to standard
option parsing would be helpful here, but I don't think is worth
blocking this series.

Thanks,
-Stolee