Hi Lénaïc
On 30/05/2021 07:39, Lénaïc Huard wrote:
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.
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.
+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
