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

["Lénaïc Huard" <lenaic@lhuard.fr>]

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.

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.

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