Re: /bin/ls formatting broken for non-C(?) locales

On Wed, Nov 25, 2015 at 04:31:21AM +0300, Andrey Chernov wrote:
> On 25.11.2015 3:15, Baptiste Daroussin wrote:
> > On Sat, Nov 21, 2015 at 11:57:46PM +0300, Andrey Chernov wrote:
> >> On 21.11.2015 15:18, Ed Schouten wrote:
> >>> Hi Baptiste,
> >>>
> >>> I suppose you should use the wcswidth() function somewhere to compute
> >>> the visible width of the month name. Some characters may be
> >>> double-width, others may have no effective width at all.
> >>>
> >>
> >> I agree. Checking error return of wide chars functions with some
> >> fallback will be good too.
> > 
> > I have updated the code https://reviews.freebsd.org/D4239
> > 
> > Tested by modifying some locales to add double width and zero width unicode in
> > the locales
> > 
> > Also added the error checking for the return of wide chars functions. For now I
> > haven't added fallback, suggestions welcome if needed.
> 
> 1) For just 1 char in wcswidth(&wab_months[i][j], 1); it is better to
> use another function wcwidth(wab_months[i][j]);

done
> 
> 2) By fallback I mean something which not stops ls working with
> incorrect for some reason locale, like setting max_width_month to
> MAX_ABMON_WIDTH on error return (from
> mbstowcs/wcwidth/wcswidth/wcswidth) and exit from
> populate_abbreviated_month().

Not that easy to provide a fallback (or better to say I can't find a proper way)
it if fails on mbstocws then ab_months is not populated so unusable.
What I did for now is set max_month_width to -1 and in ls_strftime I fallback on
the plain strftime meaning you keep localized information but the alignement is
broken as of now.

> 
> 3) wcwidth/wcswidth may return -1 too, it needs to be checked too.
done and truncate the name of the month to the latest valid character
> 
> 4) The whole processing looks overcomplicated and not effective. What
> about this instead?
> for (i = 0; i < 12; i++) {
>     count wcswidth() of each month and store it in wab_months_width[].
>     count max_width_month.
> }
> for (i = 0; i < 12; i++) {
>     if ((n = max_width_month - wab_months_width[i]) > 0)
> 	call wcscat(wab_months[i], L" ") n times.
> }

Done, along with your optimisation from your next mail
> 
> 5) If there is no %b is strftime() format, there is no sense to spend
> CPU cycles on from populate_abbreviated_month(), so it should be called
> only once inside ls_strftime() on first %b instead of calling it in
> printtime() for all cases.

done

Review updated (if you prefer a diff by mail just tell me, given you do not have
a phabricator account.)

Thanks for your patience! and reviews!

Bapt