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

From: Lénaïc Huard <lenaic@lhuard.fr>
Date: May 30, 2021, 06:39 UTC
Message-ID: <2953592.SyOBHIlnlS@coruscant.lhuard.fr>
In-Reply-To: <ce0e096d-cd82-800a-9ef5-5bcc4b25046d@gmail.com>

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 :

Show 8 quoted lines

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

Show 9 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 ?