Re: [PATCH v2 3/4] index-pack: add --progress-title option
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 24, 2021, 21:40 UTC
- Message-ID
- <87a6l6ed1e.fsf@evledraar.gmail.com>
- In-Reply-To
- <f081ac63-cb5c-1777-d9df-828382637ed1@gmail.com>
On Tue, Aug 24 2021, Derrick Stolee wrote:
Show 11 quoted lines
> 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.
Thanks.
Show 18 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)Yeah, it's exactly copy/pasted from another thing doing the same sort of parsing in this file. Will keep it the same for this new code, and just note it.
I think any subsequent change to refactor it will be easier if it's all consistent, v.s. some using i + 1, another putting it in parenthesis etc.
Show 10 quoted lines
>> + 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.
*nod*.