Re: [PATCH]: ls: do not show long iso time format for en_* locales

2009-09-27 Thread Pádraig Brady
Jim Meyering wrote:
 Pádraig Brady wrote:
 
 Paul Eggert wrote:
 Ondřej Vašík ova...@redhat.com writes:

 as reported in https://bugzilla.redhat.com/show_bug.cgi?id=525134 by
 Daniel Qarras, ls -l shows iso long format for en_* locales.
 I just now read that Bugzilla report, and the diagnosis and the
 patch do not seem correct.  The diagnosis says:

 In ls.c (case locale_time_style)  is dcgettext (NULL, long_time_format[i],
 LC_TIME); ... that translates the string, but the translation is THE SAME 
 as
 the default - as the format is the same for en_* locales.
 But that is not what the ls.c source code does.  The code does this:

 char const *locale_format =
   dcgettext (NULL, long_time_format[i], LC_TIME);
 if (locale_format == long_time_format[i])
   goto case_long_iso_time_style;

 The == test returns true when dcgettext returns the msgid (its 2nd
 argument) because it finds no translation.
 Right. We don't have any translations for en.
 
 This is key.
 
 Looking at Makefile.in.in, you can see that
 the file /usr/share/locale/en/LC_TIME/coreutils.mo is never installed,
 and neither is .../LC_MESSAGES/coreutils.mo, because coreutils has
 no po/en.po file.  Pre-optimizers might argue that not installing the
 latter is a good thing, because gettext will then not waste time with the
 fstat+mmap+close that it normally performs after each successful .mo file
 open.  Not to mention the cost of each subsequent failed message lookup.
 
 I try not to pre-optimize, so question whether the Makefile.in.in patch
 is worthwhile, but from an aesthetics point of view, I prefer not to
 install the LC_MESSAGES/coreutils.mo file.  The core of the patch
 is this two-line addition:
 
 ++lang=en; for lc in '' $(EXTRA_LOCALE_CATEGORIES); do \ 
 ++  test -n $$lc  mv -f $(message_mo) $(lc_mo_file); done
 
 All of the rest is factoring.  I'm leaning towards rewriting it to
 insert the non-factored equivalent.  However, one advantage of using the
 patch is that it records the context: if we update to a newer gettext
 that changes the body of that rule, the patch will no longer apply,
 and that will make bootstrap fail.  One alternative that could be used
 with the 3-line-insertion approach is to record (and always cross-check)
 a checksum of the rule contents.
 
 With this patch, in an en* locale, ls -l now uses the conventional
 date formats.
 
 Here's an incomplete patch.
 It needs a test and a NEWS entry.
 Ondřej, can you adjust your test to work (or skip)
 if there is no en* locale?
 
From 53c88d531d88e5d4ef393a61758bc3fee4894e48 Mon Sep 17 00:00:00 2001
 From: Jim Meyering meyer...@redhat.com
 Date: Sat, 26 Sep 2009 14:59:05 +0200
 Subject: [PATCH] ls: use conventional date format in long listing for en* 
 locales
 
 * bootstrap.conf: Generate po/en.po with identity translations for
 the two LC_TIME strings required by ls.c.
 * configure.ac: Require gettext version 0.17.
 * po/Makefile.in.in-patch: Patch autopoint-provided file so that
 it provides the LC_TIME .mo file, but not the LC_MESSAGES one.
 * po/.gitattributes: Allow space-before-TAB in the patch.
 ---
  bootstrap.conf  |   41 +
  configure.ac|2 +-
  po/.gitattributes   |1 +
  po/Makefile.in.in-patch |   65 
 +++

So that will apply generate an en.po with the
traditional unix format to apply to en_* for e.g.

$ locale -a | sed -n 's/\(en_..\).*/\1/p' | sort -u |
 while read LANG; do echo $LANG $(locale territory); done

en_AG Antigua and Barbuda
en_AU Australia
en_BW Botswana
en_CA Canada
en_DK Denmark
en_GB Great Britain
en_HK Hong Kong
en_IE Ireland
en_IN India
en_NG Nigeria
en_NZ New Zealand
en_PH Philippines
en_SG Singapore
en_US USA
en_ZA South Africa
en_ZW Zimbabwe

Is that not functionally equivalent to Ondřej's patch
which is much simpler and probably more efficient?
His patch will also use an en_ translation if supplied
(say if en_HK wanted a different format).

cheers,
Pádraig.






Re: [PATCH]: ls: do not show long iso time format for en_* locales

2009-09-27 Thread Jim Meyering
Pádraig Brady wrote:
...
 So that will apply generate an en.po with the
 traditional unix format to apply to en_* for e.g.

Right.

 $ locale -a | sed -n 's/\(en_..\).*/\1/p' | sort -u |
 while read LANG; do echo $LANG $(locale territory); done

 en_AG Antigua and Barbuda
 en_AU Australia
 en_BW Botswana
...
 Is that not functionally equivalent to Ondřej's patch

Yes.
However, ...

 which is much simpler and probably more efficient?
 His patch will also use an en_ translation if supplied

his added setlocale call (unnecessary in a non-en_* locale)
is not a plus.  Though that could be fixed.

 (say if en_HK wanted a different format).

The only advantage is that my patch uses the existing framework,
rather than adding special case code in ls.c proper.
Whether that is worth the apparent complexity...

If you prefer his patch and want to adjust it and
handle the rest, I have no objection.




Re: [PATCH]: ls: do not show long iso time format for en_* locales

2009-09-27 Thread Pádraig Brady
Jim Meyering wrote:
 The only advantage is that my patch uses the existing framework,
 rather than adding special case code in ls.c proper.
 Whether that is worth the apparent complexity...
 
 If you prefer his patch and want to adjust it and
 handle the rest, I have no objection.

Yes it's debatable.
I'll take a closer look at both
and send an updated one if I think
the special case in ls.c more appropriate.

cheers,
Pádraig.