Re: [PATCH] grub-setup.c: Use argp instead of getopt.

[Colin Watson]

On Mon, Sep 13, 2010 at 04:06:02PM +0200, Yves Blusseau wrote:
>  util/i386/pc/grub-setup.c |  315 
> ++++++++++++++++++++++++---------------------

Unless and until they're unified, please keep
util/sparc64/ieee1275/grub-setup.c in sync with this as best you can.

> -static struct option options[] =
> -  {
> -    {"boot-image", required_argument, 0, 'b'},
> -    {"core-image", required_argument, 0, 'c'},
> -    {"directory", required_argument, 0, 'd'},
> -    {"device-map", required_argument, 0, 'm'},
> -    {"root-device", required_argument, 0, 'r'},
> -    {"force", no_argument, 0, 'f'},
> -    {"skip-fs-probe", no_argument, 0, 's'},
> -    {"help", no_argument, 0, 'h'},
> -    {"version", no_argument, 0, 'V'},
> -    {"verbose", no_argument, 0, 'v'},
> -    {0, 0, 0, 0}
> -  };
> -
> +/* Print the version information.  */
>  static void
> -usage (int status)
> +print_version (FILE *stream, struct argp_state *state)
>  {
> -  if (status)
> -    fprintf (stderr, _("Try `%s --help' for more information.\n"), 
> program_name);
> -  else
> -    printf (_("\
> -Usage: %s [OPTION]... DEVICE\n\
> -\n\
> +  fprintf (stream, "%s (%s) %s\n", program_name, PACKAGE_NAME, 
> PACKAGE_VERSION);
> +}
> +void (*argp_program_version_hook) (FILE *, struct argp_state *) = 
> print_version;
> +
> +const char *argp_program_bug_address = "<"PACKAGE_BUGREPORT">";
> +
> +static struct argp_option options[] = {
> +  {"boot-image",  'b', N_("FILE"), 0,
> +   N_("Use FILE as the boot image [default=")DEFAULT_BOOT_FILE"]", 0},

I don't think this translates well.  What if some language wants the
translation to be of the form:

  Use FILE [default=blah] as the boot image

The rule of thumb is that you should not split up translatable strings
into any units smaller than sentences.  Instead, you should write this
as N_("Use FILE as the boot image [default=%s]"), write a function that
looks a bit like this:

  static char *
  help_filter (int key, const char *text, void *input __attribute__ ((unused)))
  {
    switch (key)
      {
        case 'b':
          return xasprintf (text, DEFAULT_BOOT_FILE);
        ...
        default:
          return (char *) text;
      }
  }

... and add that function as the sixth element in your 'struct argp'.

> +struct arguments
> +{
> +  char *boot_file;  /* --boot-file     */
> +  char *core_file;  /* --core-file     */
> +  char *dir;        /* --directory     */
> +  char *dev_map;    /* --device-map    */
> +  char *root_dev;   /* --root-device   */
> +  int  force;       /* --force         */
> +  int  fs_probe;    /* --skip-fs-probe */
> +  char *device;     /* DEVICE          */
> +};

I think this is perhaps overcommented. :-)

> +static struct argp argp = {
> +  options, argp_parser, "DEVICE",
> +"\n\
>  Set up images to boot from DEVICE.\n\
> -DEVICE must be a GRUB device (e.g. `(hd0,1)').\n\
> -\n\
> -You should not normally run %s directly.  Use grub-install instead.\n\
> -\n\
> -  -b, --boot-image=FILE   use FILE as the boot image [default=%s]\n\
> -  -c, --core-image=FILE   use FILE as the core image [default=%s]\n\
> -  -d, --directory=DIR     use GRUB files in the directory DIR [default=%s]\n\
> -  -m, --device-map=FILE   use FILE as the device map [default=%s]\n\
> -  -r, --root-device=DEV   use DEV as the root device [default=guessed]\n\
> -  -f, --force             install even if problems are detected\n\
> -  -s, --skip-fs-probe     do not probe for filesystems in DEVICE\n\
> -  -h, --help              display this message and exit\n\
> -  -V, --version           print version information and exit\n\
> -  -v, --verbose           print verbose messages\n\
>  \n\
> -Report bugs to <%s>.\n\
> -"),
> -         program_name, program_name,
> -         DEFAULT_BOOT_FILE, DEFAULT_CORE_FILE, DEFAULT_DIRECTORY,
> -         DEFAULT_DEVICE_MAP, PACKAGE_BUGREPORT);
> +You should not normally run this program directly.  Use grub-install 
> instead.\n\
> +\v\
> +DEVICE must be an OS device (e.g. /dev/sda1).",
> +  NULL, NULL, NULL
> +};

The strings here should be enclosed in N_() so that they can be
translated.

> +  /* Parse our arguments */
> +  argp_parse (&argp, argc, argv, 0, 0, &arguments);

You need to check the return value.  exit (1) would probably be
appropriate handling.

-- 
Colin Watson                                       address@hidden