Re: misalignment in ls -l in fr_FR locale

[Jim Meyering]

Pádraig Brady wrote:
> I expect to push the attached updated patch soon.

Hi Pádraig,

Thanks for working on this, but please hold off until after 7.2.
I'm trying to stabilize for a bug-fix(was "-only") release.

> Subject: [PATCH] ls: Fix alignment when month names have varying widths
...
> diff --git a/gl/lib/mbsalign.c b/gl/lib/mbsalign.c
> new file mode 100644
> index 0000000..95e303a
> --- /dev/null
> +++ b/gl/lib/mbsalign.c
> @@ -0,0 +1,176 @@
> +/* Align/Truncate a string in a given screen width
> +   Copyright (C) 2009 Free Software Foundation, Inc.
> +
> +   This program is free software: you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation, either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +/* Written by Pádraig Brady.  */
> +
> +#include <config.h>
> +#include "mbsalign.h"
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <wchar.h>
> +#include <wctype.h>
> +#include "xalloc.h"
> +
> +static int wc_ensure_printable (wchar_t * wchars);
> +static size_t wc_truncate (wchar_t * wchars, size_t width);
> +static int rpl_wcswidth (const wchar_t *s, size_t n);
> +
> +/* Align a string in a given screen width, handling multi-byte characters.
> +   In addition if the string is too large for the width it's truncated.

use active voice, i.e.,

   In addition if the string is too large for the width, truncate it to fit.

> +   When centering, the number of trailing spaces may be 1 less than the
> +   number of leading spaces.
> +   Returned is the number of bytes written to or required in dest (without

Return the number...

> +   the trailing NUL).  A value >= dest_size means there wasn't enough space.
> +   The width parameter both specifies the width to align/pad/truncate to,
> +   and is updated to return the width used before padding.  */

Would "desired_width" be a better parameter name for dest_size?
"size" makes me think of a buffer size, i.e., number of bytes allocated.

> +int
> +mbsalign (const char *src, char *dest, size_t dest_size,
> +          int *width, mbs_align_t align)
> +{
> +  int src_len = strlen (src);

Please use size_t, not int for anything length-related.
And especially for things you pass to malloc.

> +  int ret = 0;
> +  char *newstr = NULL;
> +  wchar_t *str_wc = NULL;
> +  const char *str_to_print = src;
> +  int used = src_len, spaces, wc_conversion = 0, wc_enabled = 0;

The name "used" could mean something boolean, or a length.
Call it "n_used" and there is no ambiguity.
n_spaces would be clearer, in the same way.
use size_t for those two.

wc_conversion and wc_enabled should be of type "bool".

> +  if (MB_CUR_MAX > 1)
> +    {
> +      int src_chars = mbstowcs (NULL, src, 0) + 1;
> +      str_wc = xmalloc (src_chars * sizeof (wchar_t));

This function would be more generally useful (usable in a library) if it
did not rely on xmalloc (i.e., use malloc instead and handle failure),
and probably not much harder to implement.  maybe worth the trouble...

> +      if (mbstowcs (str_wc, src, src_chars) > 0)
> +        {
> +          str_wc[src_chars - 1] = L'\0';
> +          wc_enabled = 1;
> +          wc_conversion = wc_ensure_printable (str_wc);
> +          used = rpl_wcswidth (str_wc, src_chars);
> +        }
> +    }
> +
> +  if (wc_conversion || used > *width)
> +    {
> +      newstr = xmalloc (src_len);
> +      str_to_print = newstr;
> +      if (wc_enabled)
> +        {
> +          used = wc_truncate (str_wc, *width);
> +          wcstombs (newstr, str_wc, src_len);
> +        }
> +      else
> +        {
> +          memcpy (newstr, src, *width);
> +          newstr[*width] = '\0';
> +        }
> +    }
> +
> +  spaces = *width - used;
> +  spaces = (spaces < 0 ? 0 : spaces);
> +  *width = used;  /* indicate to called how many cells used.  */
> +
> +  /* FIXME: Should I be padding with "figure space" (\u2007)
> +     rather than spaces below? (only if non ascii data present)  */
> +  switch (align)
> +    {
> +    case MBS_ALIGN_CENTER:
> +      ret = snprintf (dest, dest_size, "%*s%s%*s",
> +                      spaces / 2 + spaces % 2, "",
> +                      str_to_print, spaces / 2, "");

For a potential-library function like this, I'd be inclined
to use stpcpy+memchr rather than the heavy-weight snprintf.

> +      break;
> +    case MBS_ALIGN_LEFT:
> +      ret = snprintf (dest, dest_size, "%s%*s", str_to_print, spaces, "");
> +      break;
> +    case MBS_ALIGN_RIGHT:
> +      ret = snprintf (dest, dest_size, "%*s%s", spaces, "", str_to_print);
> +      break;
> +    }
> +
> +  free (str_wc);
> +  free (newstr);
> +
> +  return ret;
> +}
> +
> +/* Replace non printable chars.
> +   Return 1 if replacement made, 0 otherwise.  */
> +
> +static int

static bool

> +wc_ensure_printable (wchar_t * wchars)

this spacing, "wchar_t * wchars", looks like an artifact of running
the code through indent.  You can make it format properly by
adding -Twchar_t to ~/.indent.pro.  I wonder if GNU indent is
still maintained...

> +{
> +  int replaced = 0;

bool

> +  wchar_t *wc = wchars;
> +  while (*wc)
> +    {
> +      if (!iswprint ((wint_t) * wc))

spacing: s/ wc/wc/

> +        {
> +          *wc = 0xFFFD; /* L'\uFFFD' (replacement char) */
> +          replaced = 1;

... = true

> +        }
> +      wc++;
> +    }
> +  return replaced;
> +}

I'm stopping here, for now.