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 ?
