maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

ok to push. A few minor questions inline.

On Mon, Jun 16, 2014 at 06:51:53PM +0200, serg@xxxxxxxxxxx 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