Re: [PATCH v4 3/4] maintenance: `git maintenance run` learned `--scheduler=<scheduler>`

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 30, 2021, 10:16 UTC
Message-ID: <295e8a2c-913f-ec53-0865-d3153c54b9cc@gmail.com>
In-Reply-To: <2953592.SyOBHIlnlS@coruscant.lhuard.fr>

Hi Lénaïc

On 30/05/2021 07:39, Lénaïc Huard wrote:

Show 17 quoted lines

> Hello Phillip,
> 
> I’m working on the next iteration of this patch, but I would have a question
> about one comment of the review.
> 
> Le lundi 24 mai 2021, 12:12:10 CEST Phillip Wood a écrit :
>>> -static int maintenance_start(void)
>>> +static const char *const builtin_maintenance_start_usage[] = {
>>> +	N_("git maintenance start [--scheduler=<scheduler>]"), NULL
>>> +};
>>
>> I'm not sure what the { } and NULL are doing here, it should just be
>> assigning a string. You could put it inside maintenance_start() and just
>> call the variable "usage"
> 
> I did this because `parse_options(…, usage, …)` expects a NULL-terminated
> array of strings and not a single string.

Oh sorry I got confused by the definition of builtin_maintenance_usage which is just a string but cmd_maintenance() uses usage() rather than parse_options() which has a different api.

Show 15 quoted lines

> While I agree that this variable doesn’t need to be global as it is used only
> from inside a single function, I followed the pattern that I saw elsewhere.
> 
> For ex. `builting_gc_usage` or `builtin_maintenance_run_usage` in `builtin/
> gc.c`.
> 
> In fact, we’re even told to do so in `Documentation/technical/api-parse-
> options.txt` which says:
> 
>      . define a NULL-terminated
>        `static const char * const builtin_foo_usage[]` array
>        containing alternative usage strings
> 
> In the files that I looked at, the command usage was always defined as a global
> long-named variable even if it was used in a single function.

Yeah, I'm not sure why that convention has built up. If there is a single command in a file then defining the usage string at the top of the file arguably acts as some form of documentation but when there is more than one command in a file I'm not so sure. Anyway feel free to leave it as it is.

Show 20 quoted lines

>>> +static int maintenance_start(int argc, const char **argv, const char
>>> *prefix)>
>>>    {
>>>
>>> +	struct maintenance_start_opts opts;
>>> +	struct option builtin_maintenance_start_options[] = {
>>
>> As this variable is local to the function you could call it something
>> shorter like options.
> 
> I agree.
> I also followed the pattern that I saw elsewhere.
> For ex., still in `builtin/gc.c`, there are `builtin_gc_options` and
> `builtin_maintenance_run_options` which are local variables, but still defined
> with an explicit and unique long name.
> 
> So, I’m wondering if I should follow the existing pattern or if I should
> shorten the name of the local variable.
> I thought the existing convention could be useful when grepping for option or
> usage of a command in the code ?

It's up to you, personally I'd lean towards a shorter name defined in the function where it is used but if you're following a existing pattern then that should be fine too and would mean that you don't need to spend time changing what you've got already.

Best Wishes

Phillip