Hi Sergei, ok to push. A few minor questions inline. On Mon, Jun 16, 2014 at 06:51:53PM +0200, serg@mariadb.org wrote:
revision-id: 0eb94ca810500977eb0336ee30b19e9c0b769bca parent(s): f61f36b386e8d0c6448297bb84c92e8d9b82be6a committer: Sergei Golubchik branch nick: maria timestamp: 2014-06-16 18:43:26 +0200 message:
MDEV-6137 better help for SET/ENUM sysvars
Auto-generate the allowed list of values for enum/set/flagset options in --help output. But don't do that when the help text already has them.
Also, remove lists of values from help strings of various options, where they were simply listed without any additional information.
--- mysys/my_getopt.c | 81 +++++++++++++++++++++++++++------ plugin/server_audit/server_audit.c | 2 +- sql/log.cc | 3 +- sql/mysqld.cc | 8 ++-- sql/sql_plugin.cc | 2 +- sql/sys_vars.cc | 90 ++++++++----------------------------- storage/maria/ha_maria.cc | 14 ++---- storage/myisam/ha_myisam.cc | 7 ++- storage/xtradb/handler/ha_innodb.cc | 35 +++++++-------- 9 files changed, 116 insertions(+), 126 deletions(-) Hmmm... no test case affected?
diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c index 2e97417..3662757 100644 --- a/mysys/my_getopt.c +++ b/mysys/my_getopt.c @@ -1300,6 +1300,48 @@ static uint print_name(const struct my_option *optp) return s - optp->name; }
+/** prints option comment with indentation and wrapping. + + The comment column starts at startpos, and has width of width + Current cursor position is curpos, returns new cursor position + +Note: can print one character beyond width!
The above line, is it properly indented?
+*/ +static uint print_comment(const char *comment, + int curpos, int startpos, int width) Probably make width local to this function, it doesn't seem to be used by the caller anyway?
+{ + const char *end= strend(comment); + int endpos= startpos + width; + + for (; curpos < startpos; curpos++) + putchar(' '); + + if (*comment == '.' || *comment == ',') + { + putchar(*comment); + comment++; + curpos++; + } + + while (end - comment > endpos - curpos) + { + const char *line_end; + for (line_end= comment + endpos - curpos; + line_end > comment && *line_end != ' '; + line_end--); + for (; comment < line_end; comment++) + putchar(*comment); I would probably replace it with fwrite(), but fine with this form too.
+ while (*comment == ' ') + comment++; /* skip the space, as a newline will take it's place now */ + putchar('\n'); + for (curpos= 0; curpos < startpos; curpos++) + putchar(' '); + } + printf("%s", comment); + return curpos + (end - comment); +} + + /* function: my_print_options
@@ -1309,12 +1351,12 @@ static uint print_name(const struct my_option *optp) void my_print_help(const struct my_option *options) { uint col, name_space= 22, comment_space= 57; - const char *line_end; const struct my_option *optp; DBUG_ENTER("my_print_help");
for (optp= options; optp->name; optp++) { + const char *typelib_help= 0; if (!optp->comment) continue; if (optp->id && optp->id < 256) @@ -1359,25 +1401,36 @@ void my_print_help(const struct my_option *options) col= 0; } } - for (; col < name_space; col++) - putchar(' '); if (optp->comment && *optp->comment) Cleanup suggestion: optp->comment can't be NULL here.
{ - const char *comment= optp->comment, *end= strend(comment); + col= print_comment(optp->comment, col, name_space, comment_space);
- while ((uint) (end - comment) > comment_space) + switch (optp->var_type & GET_TYPE_MASK) { + case GET_ENUM: + typelib_help= ". One of: "; + break; + case GET_SET: + typelib_help= ". Any combination of: "; + break; + case GET_FLAGSET: + typelib_help= ". Takes a comma-separated list of option=value pairs, " + "where value is on or off, and options are: "; + break; + } + if (typelib_help && + strstr(optp->comment, optp->typelib->type_names[0]) == NULL)
Should we handle case when optp->typelib == NULL || optp->typelib->type_names[0] == NULL? Probably not. I believe this condition is fragile: comment string may mention type_names[0], but not list possible values. OTOH since this condition is for very limited data set it should cause problems only in rare cases.
{ - for (line_end= comment + comment_space; *line_end != ' '; line_end--); - for (; comment != line_end; comment++) - putchar(*comment); - comment++; /* skip the space, as a newline will take it's place now */ - putchar('\n'); - for (col= 0; col < name_space; col++) - putchar(' '); + int i; + col= print_comment(typelib_help, col, name_space, comment_space); + col= print_comment(optp->typelib->type_names[0], col, name_space, comment_space); + for (i= 1; i < optp->typelib->count; i++) + { + col= print_comment(", ", col, name_space, comment_space); + col= print_comment(optp->typelib->type_names[i], col, name_space, comment_space); + } } - printf("%s", comment); + putchar('\n'); } - putchar('\n');
Does this mean if comment is empty there won't be a new line?
if ((optp->var_type & GET_TYPE_MASK) == GET_BOOL) { if (optp->def_value != 0)
...skip...
@@ -2785,7 +2745,7 @@ static Sys_var_mybool Sys_master_verify_checksum(
/* These names must match RPL_SKIP_XXX #defines in slave.h. */ static const char *replicate_events_marked_for_skip_names[]= { - "replicate", "filter_on_slave", "filter_on_master", 0 + "REPLICATE", "FILTER_ON_SLAVE", "FILTER_ON_MASTER", 0 }; Does this mean there is a convention that values must be in upper case?
...skip... Regards, Sergey