Fix alignment in ls -l with cldr locales

[bapt]

Summary

month abbreviation can be of various length, hence it breaks the alignement of
ls -l
Pad abbreviated month with space to the maximum possible length of the
abbreviated month for the given locale.
This way for locale C the output is preserved.

[bapt]

Use wide char functions

[bapt]

Test multibyte functions return
ensure we do count visible columns and not the len

[bapt]

Really handle double width and zero width unicode

[bapt]

Fix with locale ja_JP.UTF-8

[bapt]

Remove debug printf

[bapt]

Use wcwidth instead of wcswidth given we are testing only 1 wchar
if wcwidth fails (return -1) then tuncate the provided month to the
last valid characters found.
Use more efficient and readable method for padding with spaces
Rename max_width_month into max_month_width for consistency
Only populate abbreviated month if %b is actually used in the strftime format
If anything goes wrong with mbstowcs/wcstombs then fallback on non aligned
localized output

[bapt]

Fix the fallback on mbstowcs

[bapt]

Fix the tuncate fallback (wcwidth == -1 case)

[bapt]

if wcwidth fails, fallback like when other multibyte functions fails.
Use wcsncat rather than wcslcat because we do not care about the size of the
buffer but rather care about the number of spaces we have to append

[ed]

Hi Baptiste,

I hope these comments help. Be sure to get in touch if I haven't been clear enough. :-)

Ed

bin/ls/print.c

110 ↗

(On Diff #10493)

As it is used for the array sizes, this should be called MAX_ABMON_SIZE instead; not _WIDTH. Most other limits usually use _MAX as a suffix (<limits.h>). Maybe ABMON_SIZE_MAX is better?

That said, it may well be the case that implementing this functionality without any fixed limits does not require substantially more code...

113 ↗

(On Diff #10493)

It looks like this array is only an intermediate product of populate_abbreviated_month(). Maybe move it into that function instead? The multiplication by MB_LEN_MAX also looks a bit odd and should likely be removed.

158 ↗

(On Diff #10493)

My criticism on this function is that it seems overly complex. In theory, we only need to do two things:

• Compute the visible width of all the months.
• Based on that, determine the amount of spaces we need to add to the end of the month names, so that they are all aligned.

In the end the only thing that you should theoretically yield is a table that says: for month x we need y additional spaces at the end. This function currently does a conversion back and forth, which isn't needed. You could just use the original strings that were returned by nl_langinfo().

Computing the visible width can be done without keeping track of a wide character string buffer. You could just call mbrtowc() and wcwidth() in a loop. Maybe put that in a separate function above called mbswidth()?

Also, please don't do arithmetic on ABMON_1. Maybe you could consider adding a utility function like this:

const char *get_abmon(int mon) {
  switch (mon) {
  case 0:
    return nl_langinfo(ABMON_1);
  ...

The optimizer is smart enough to rewrite it to nl_langinfo(ABMON_1 + i) in case all twelve constants are consecutive.

509 ↗

(On Diff #10493)

Possible alternative: instead of storing strings with space padding at the end, you could also use printf() to do the padding for you:

snprintf(...., "%s%*s", ab_months[tm->tm_mon], amount_of_padding_needed_for_month[tm->tm_mon]);

513 ↗

(On Diff #10493)

What happens if the name of the month contains an ampersand? Fortunately it's strftime(), so it won't try to fetch random garbage from the stack...

More and more I get the feeling we're trying to solve a problem in ls(1) that should be solved in strftime(3), for example by adding a special modifier. But that may be too much work for now.

[jilles]

I tried various locales and the output looks good to me. I found one minor nit in the code.

bin/ls/print.c

508 ↗

(On Diff #10493)

This can be nested in the previous if, avoiding the extra posb != NULL check.

[ache]

Trying to use phabricator :)

bin/ls/print.c

509 ↗

(On Diff #10493)

I agree that doing all padding directly in snprintf() will be better than prepare them earlier. It not only save space, but eliminate one array with really unknown element length - wab_months[]. Second array ab_months[] with unknown element length still exists and better should be eliminated too with malloc()ed elements.

[bapt]

Complete change the approach to match @ed proposal

[bapt]

Rename populate_ to compute_

[bapt]

Fix error checking for mbrtowc

[bapt]

shorten get_abmon and return abort() is passed an argument higher than 12

Discussed with: @jilles

[bapt]

Style fixes

[ache]

bin/ls/print.c

180 ↗

(On Diff #10794)

wcwidth() can return negative here still. bugcheck is needed (return(-1))

504 ↗

(On Diff #10794)

Debugging printf in the line above?

[bapt]

Remove debugging printf
Test return of wcwidth

[ed]

Fine by me, but please take a look at my comment. :-)

bin/ls/print.c

111 ↗

(On Diff #10794)

Do we need to keep track of the maximum size explicitly? We currently only test against this to see whether we've already initialized. Maybe it would be better to replace it by a static bool padding_for_month_initialized or something?

[bapt]

bin/ls/print.c

111 ↗

(On Diff #10796)

Well we need 3 states: initialized, non initialized, fail to initialize (to not try using it in that case)

[ed]

bin/ls/print.c

500 ↗

(On Diff #10796)

Maybe BUFSIZ is not the right limit here? Maybe NL_TEXTMAX?

[ache]

bin/ls/print.c

500 ↗

(On Diff #10796)

NL_TEXTMAX surprisingly not for nl_* but for catgets()

[ache]

bin/ls/print.c

196 ↗

(On Diff #10796)

You can just call wcswidth(get_abmon(i), INT_MAX) and drop its replacement.

[bapt]

bin/ls/print.c

196 ↗

(On Diff #10796)

No get_abmon(i) is not widechar

[ache]

bin/ls/print.c

196 ↗

(On Diff #10796)

Of course with conversion. I am not sure what we win here right now, but potentially wcswidth() can be faster than wcwidth() wchar by wchar.

[ed]

bin/ls/print.c

111 ↗

(On Diff #10796)

As discussed on IRC briefly: you can still treat it as two states. If we fail to initialize, you could just treat it as if the initialization succeeded, but that all padding values are set to 0.

[ache]

bin/ls/print.c

196 ↗

(On Diff #10796)

Forget it. Conversion will need another buffer with unknown length.

[bapt]

bin/ls/print.c

196 ↗

(On Diff #10796)

yup, well I can malloc, but I have tested both and haven't seen any performance change.

So are you ok with the current version of the patch?

[ume]

In D4239#90029, @bapt wrote:

Fix with locale ja_JP.UTF-8

The fix itself seems good to me. It fixes alignment for the locales which abbreviated month are left aligned.
Unfortunately, abbreviated month of the CJK locales are right aligned. So, we still need existing left padding for the CJK locales.
Just FYI.