Hi Sergei, ok to push. A few minor questions inline.
On Mon, Jun 16, 2014 at 06:51:53PM +0200, [email protected] 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
