Re: [PATCH 1/4] test: add a library to detect an en_US.UTF-8 locale
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 7, 2011, 18:37 UTC
- Message-ID
- <7vy66w7e6j.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <4490926da28dcbfedc779cd32c5a59e20a1b55a2.1294312018.git.yann@droneaud.fr>
Yann Droneaud <yann@droneaud.fr> writes:
Yann Droneaud <yann@droneaud.fr> writes:
Show 14 quoted lines
> Since one can't rely on "locale" command availability nor its -a output, > perl script t/lib-locale.pl first use setlocale() and langinfo(CODESET) > to search for a working en_US.UTF-8 locale among many name variants. > > If this fail, the script fallback to "locale" usage with two steps: > - try the "charmap" keyword, for example LC_ALL=en_US locale charmap > - then try "-a" option and match a pattern looking to the > locale names > > Signed-off-by: Yann Droneaud <yann@droneaud.fr> > --- > t/lib-locale.pl | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 167 insertions(+), 0 deletions(-) > create mode 100755 t/lib-locale.pl
The remainder of the series looks more or less sane, but a new 170-line script somehow feels a bit overengineered solution to a minor problem that has already been solved in a much simpler way.
Show 23 quoted lines
> +# try to execute "locale -a" command
> +# this command is not always available and
> +# output format is not normalized
> +#
> +use IPC::Open3;
> +use File::Spec;
> +
> +open(NULLR, "<", File::Spec->devnull) || die "Can't open devnull: $!";
> +open(NULLW, ">", File::Spec->devnull) || die "Can't open devnull: $!";
> +
> +my $pid = open3("<&NULLR", \*LOCALES, ">&NULLW" , "locale", "-a") || die "Can't launch locale -a: $!";
> +
> +while(<LOCALES>) {
> + chomp;
> + if (/(en_US\.([\w-]+))/i) {
> + if (codeset_check($2)) {
> + $locale = $1;
> + last;
> + }
> + }
> +}
> +
> +waitpid($pid, 0);You are trying to buy something with the complexity of Open3 for doing this logic, over a bog-naive "for (`locale -a`) { ... }", but is that something really worth the money?
"lib-locale.pl" is a gross misnomer for this script.
It may be a good helper to be used in 2/4 (test_utf8), but later people may want to have more helper feature in something called "lib-locale", not just "pick a single UTF-8 locale randomly from available ones on the system" (especially when ab/i18n starts moving again). I'd suggest either to (1) rename it "pick-utf8-locale.pl" (i.e. honest naming) or (2) prepare the helper command to be extensible from the beginning, i.e. require a command word e.g. "lib-locale.pl pick-utf8-locale" to trigger the currently implemented feature (i.e. forward looking naming).