[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] grub-setup.c: Use argp instead of getopt.
From: |
Colin Watson |
Subject: |
Re: [PATCH] grub-setup.c: Use argp instead of getopt. |
Date: |
Tue, 14 Sep 2010 10:28:58 +0100 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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