Re: [libvirt] [PATCH v3 06/48] remote: stop trying to print help as giant blocks of text

> On 30 Jul 2019, at 12:04, Daniel P. Berrangé <berrange(a)redhat.com&gt; wrote: > > On Tue, Jul 30, 2019 at 11:56:45AM +0200, Christophe de Dinechin wrote: >> >> Daniel P. Berrangé writes: >> >>> The remote daemon tries to print out its help text in a couple of giant >>> blocks of text. This has already lead to duplication of the text for the >>> privileged vs unprivileged execution mode. With the introduction of more >>> daemons, this text is going to be duplicated many more times with small >>> variations. This is very unfriendly to translators as they have to >>> translate approximately the same text many times with small tweaks. >>> >>> Splitting the text up into individual strings to print means that each >>> piece will only need translating once. It also gets rid of all the >>> layout information from the translated strings, so avoids the problem of >>> translators breaking formatting by mistake. >>> >>> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; >>> --- >>> src/remote/remote_daemon.c | 128 ++++++++++++++++++------------------- >>> src/remote/remote_driver.h | 1 - >>> 2 files changed, 64 insertions(+), 65 deletions(-) >>> >>> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c >>> index d887b7abfb..69385af1c4 100644 >>> --- a/src/remote/remote_daemon.c >>> +++ b/src/remote/remote_daemon.c >>> @@ -859,75 +859,75 @@ daemonSetupHostUUID(const struct daemonConfig *config) >>> return 0; >>> } >>> >>> +typedef struct { >>> + const char *opts; >>> + const char *help; >>> +} virOptionHelp; >>> + >>> /* Print command-line usage. */ >>> static void >>> daemonUsage(const char *argv0, bool privileged) >>> { >>> - fprintf(stderr, >>> - _("\n" >>> - "Usage:\n" >>> - " %s [options]\n" >>> - "\n" >>> - "Options:\n" >>> - " -h | --help Display program help:\n" >>> - " -v | --verbose Verbose messages.\n" >>> - " -d | --daemon Run as a daemon & write PID file.\n" >>> - " -l | --listen Listen for TCP/IP connections.\n" >>> - " -t | --timeout <secs> Exit after timeout period.\n" >>> - " -f | --config <file> Configuration file.\n" >>> - " -V | --version Display version information.\n" >>> - " -p | --pid-file <file> Change name of PID file.\n" >>> - "\n" >>> - "libvirt management daemon:\n"), >>> - argv0); >>> + size_t i; >>> + virOptionHelp opthelp[] = { >>> + { "-h | --help", N_("Display program help") }, >> >> Why use N_ both here and in the printout code (copied below)? >> >> fprintf(stderr, " %-22s %s\n", opthelp[i].opts, N_(opthelp[i].help)); >> >> When is the message translated? > > Yeah that's a screwup. The fprintf() should be calling gettext > >>> + fprintf(stderr, "\n"); >>> + fprintf(stderr, "%s:\n", _("Usage")); >>> + fprintf(stderr, " %s [%s]\n", argv0, _("options")); >> >> Here, despite your argument regarding formatting, I believe that >> translators should have a larger context. Also, as the gettext >> documentation points out, ":" needs a space before in French, so it >> should be placed within the part to translate. > > What documentation are you seeing this in ? I'm not come across > this suggestion before and want to read more > >> >> fprintf(stderr, _("Usage:\n %s [options]\n\n"), argv0); >> >> >>> + fprintf(stderr, "\n"); >>> + >>> + fprintf(stderr, "%s:\n", _("Options")); >> >> Same, for localization of whitespace around : it would be better to have >> >> fprintf(stderr, _("Options:\n")); >> >> This applies to all cases where you have %s: below. >> >> >>> + for (i = 0; i < ARRAY_CARDINALITY(opthelp); i++) >>> + fprintf(stderr, " %-22s %s\n", opthelp[i].opts, N_(opthelp[i].help)); >> >> Based on comment above, replace N_ with _ ? >> >>> + fprintf(stderr, "\n"); >>> + >>> + fprintf(stderr, "%s:\n", _("libvirt management daemon")); >> >>> + >>> + fprintf(stderr, "\n"); >>> + fprintf(stderr, " %s:\n", _("Default paths")); >> >>> + fprintf(stderr, "\n"); >>> + >>> + fprintf(stderr, " %s:\n", _("Configuration file (unless overridden by -f)")); >> >> >>> + fprintf(stderr, " %s/libvirt/libvirtd.conf\n", >>> + privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME"); >> >> Add a N_ ? > > File paths have no translatable text in them. Well, I’ve always been confused with what “N_” meant exactly, but to me it was a tell to translator “don’t try translating this one”. If there is nothing, the translator might waste time looking at it. Maybe I got that backwards, though. Can’t say the doc is super clear.