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

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 24, 2021, 17:18 UTC
Message-ID: <f081ac63-cb5c-1777-d9df-828382637ed1@gmail.com>
In-Reply-To: <patch-v2-3.4-67197064a8b-20210823T110136Z-avarab@gmail.com>

On 8/23/2021 7:02 AM, Ævar Arnfjörð Bjarmason wrote:

Show 5 quoted lines

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

Show 5 quoted lines

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