ls: improve alignment of quoted names

This provides better alignment when some names are quoted,
which also provides better indication that quotes are not
part of the name.

* src/ls.c (align_variable_outer_quotes): A new variable
set when ls is aligning columns (not using -m, non-zero -w),
and has a variable quoting style (shell, shell-escape, c-maybe).
(quote_name_buf): Writes to buffer rather than FILE,
taking care to avoid data copying if possible.  Refactored from...
(quote_name): ...here.  This now manages the buffer passed
to quote_name_buf() and outputs the padding, colors and name
in the appropriate order, while managing the --dired offsets.
(get_color_indicator): A new function to return the color sequence,
refactored from...
(print_color_indicator): ...here.  This now simply outputs.
(print_dir): Refactor common parts to quote_name().
(clear_files): Reset the flag indicating at least one
file is quoted in the current directory.
(needs_quoting): A new function to indicate at the scan stage
whether a name needs quoting.  Called from...
(gobble_file): ...here, until we find the first quoted file.
(print_name_with_quoting): Mostly refactored to quote_name().
* tests/ls/quote-align.sh: A new test for various output formats.
* tests/local.mk: Reference the new test.
* NEWS: Mention the improvement.

4 files changed, 266 insertions, 65 deletions

diff --git a/NEWS b/NEWS
index e607d32a8..41c1e3c8c 100644
--- a/NEWS
+++ b/NEWS
@@ -95,6 +95,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   ls is now fully responsive to signals until the first escape sequence is
   written to a terminal.
 
+  ls now aligns quoted items with non quoted items, which is easier to read,
+  and also better indicates that the quote is not part of the actual name.
+
   stat and tail now know about "prl_fs" (a parallels file system),
   "m1fs" (a Plexistor file system), "wslfs" (Windows Subsystem for Linux),
   and "smb2".  stat -f --format=%T now reports the file system type, and
diff --git a/src/ls.c b/src/ls.c
index d42b9f4d0..f462e6157 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -223,6 +223,9 @@ struct fileinfo
 
     /* For color listings, true if a regular file has capability info.  */
     bool has_capability;
+
+    /* Whether file name needs quoting. tri-state with -1 == unknown.  */
+    int quoted;
   };
 
 #define LEN_STR_PAIR(s) sizeof (s) - 1, s
@@ -241,17 +244,24 @@ struct bin_str
 # define tcgetpgrp(Fd) 0
 #endif
 
-static size_t quote_name (FILE *out, const char *name,
+static size_t quote_name (char const *name,
                           struct quoting_options const *options,
-                          size_t *width);
+                          int needs_general_quoting,
+                          const struct bin_str *color,
+                          bool allow_pad, struct obstack *stack);
+static size_t quote_name_buf (char **inbuf, size_t bufsize, char *name,
+                              struct quoting_options const *options,
+                              int needs_general_quoting, size_t *width,
+                              bool *pad);
 static char *make_link_name (char const *name, char const *linkname);
 static int decode_switches (int argc, char **argv);
 static bool file_ignored (char const *name);
 static uintmax_t gobble_file (char const *name, enum filetype type,
                               ino_t inode, bool command_line_arg,
                               char const *dirname);
-static bool print_color_indicator (const struct fileinfo *f,
-                                   bool symlink_target);
+static const struct bin_str * get_color_indicator (const struct fileinfo *f,
+                                                   bool symlink_target);
+static bool print_color_indicator (const struct bin_str *ind);
 static void put_indicator (const struct bin_str *ind);
 static void add_ignore_pattern (const char *pattern);
 static void attach (char *dest, const char *dirname, const char *name);
@@ -317,6 +327,13 @@ static size_t cwd_n_alloc;
 /* Index of first unused slot in 'cwd_file'.  */
 static size_t cwd_n_used;
 
+/* Whether files needs may need padding due to quoting.  */
+static bool cwd_some_quoted;
+
+/* Whether quoting style _may_ add outer quotes,
+   and whether aligning those is useful.  */
+static bool align_variable_outer_quotes;
+
 /* Vector of pointers to files, in proper sorted order, and the number
    of entries allocated for it.  */
 static void **sorted_file;
@@ -2065,8 +2082,15 @@ decode_switches (int argc, char **argv)
      or line_lengths shorter than MIN_COLUMN_WIDTH.  */
   max_idx += line_length % MIN_COLUMN_WIDTH != 0;
 
+  enum quoting_style qs = get_quoting_style (NULL);
+  align_variable_outer_quotes = format != with_commas
+                                && format != one_per_line
+                                && (line_length || format == long_format)
+                                && (qs == shell_quoting_style
+                                    || qs == shell_escape_quoting_style
+                                    || qs == c_maybe_quoting_style);
   filename_quoting_options = clone_quoting_options (NULL);
-  if (get_quoting_style (filename_quoting_options) == escape_quoting_style)
+  if (qs == escape_quoting_style)
     set_char_quoting (filename_quoting_options, ' ', 1);
   if (file_type <= indicator_style)
     {
@@ -2686,24 +2710,24 @@ print_dir (char const *name, char const *realname, bool command_line_arg)
       dev_ino_push (dir_stat.st_dev, dir_stat.st_ino);
     }
 
+  clear_files ();
+
   if (recursive || print_dir_name)
     {
       if (!first)
         DIRED_PUTCHAR ('\n');
       first = false;
       DIRED_INDENT ();
-      PUSH_CURRENT_DIRED_POS (&subdired_obstack);
-      dired_pos += quote_name (stdout, realname ? realname : name,
-                               dirname_quoting_options, NULL);
-      PUSH_CURRENT_DIRED_POS (&subdired_obstack);
+
+      quote_name (realname ? realname : name, dirname_quoting_options, -1,
+                  NULL, true, &subdired_obstack);
+
       DIRED_FPUTS_LITERAL (":\n", stdout);
     }
 
   /* Read the directory entries, and insert the subfiles into the 'cwd_file'
      table.  */
 
-  clear_files ();
-
   while (1)
     {
       /* Set errno to zero so we can distinguish between a readdir failure
@@ -2911,6 +2935,7 @@ clear_files (void)
     }
 
   cwd_n_used = 0;
+  cwd_some_quoted = false;
   any_has_acl = false;
   inode_number_width = 0;
   block_size_width = 0;
@@ -3009,6 +3034,15 @@ has_capability_cache (char const *file, struct fileinfo *f)
   return b;
 }
 
+static bool
+needs_quoting (char const* name)
+{
+  char test[2];
+  size_t len = quotearg_buffer (test, sizeof test , name, -1,
+                                filename_quoting_options);
+  return *name != *test || strlen (name) != len;
+}
+
 /* Add a file to the current table of files.
    Verify that the file exists, and print an error message if it does not.
    Return the number of blocks that the file occupies.  */
@@ -3034,6 +3068,15 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
   f->stat.st_ino = inode;
   f->filetype = type;
 
+  f->quoted = -1;
+  if ((! cwd_some_quoted) && align_variable_outer_quotes)
+    {
+      /* Determine if any quoted for padding purposes.  */
+      f->quoted = needs_quoting (name);
+      if (f->quoted)
+        cwd_some_quoted = 1;
+    }
+
   if (command_line_arg
       || format_needs_stat
       /* When coloring a directory (we may know the type from
@@ -4111,34 +4154,59 @@ print_long_format (const struct fileinfo *f)
     print_type_indicator (f->stat_ok, f->stat.st_mode, f->filetype);
 }
 
-/* Output to OUT a quoted representation of the file name NAME,
-   using OPTIONS to control quoting.  Produce no output if OUT is NULL.
+/* Write to *BUF a quoted representation of the file name NAME, if non-NULL,
+   using OPTIONS to control quoting.  *BUF is set to NAME if no quoting
+   is required.  *BUF is allocated if more space required (and the original
+   *BUF is not deallocated).
    Store the number of screen columns occupied by NAME's quoted
-   representation into WIDTH, if non-NULL.  Return the number of bytes
-   produced.  */
+   representation into WIDTH, if non-NULL.
+   Store into PAD whether an initial space is needed for padding.
+   Return the number of bytes in *BUF.  */
 
 static size_t
-quote_name (FILE *out, const char *name, struct quoting_options const *options,
-            size_t *width)
+quote_name_buf (char **inbuf, size_t bufsize, char *name,
+                struct quoting_options const *options,
+                int needs_general_quoting, size_t *width, bool *pad)
 {
-  char smallbuf[BUFSIZ];
-  size_t len = quotearg_buffer (smallbuf, sizeof smallbuf, name, -1, options);
-  char *buf;
+  char *buf = *inbuf;
   size_t displayed_width IF_LINT ( = 0);
+  size_t len = 0;
+  bool quoted;
 
-  if (len < sizeof smallbuf)
-    buf = smallbuf;
-  else
+  enum quoting_style qs = get_quoting_style (options);
+  bool needs_further_quoting = qmark_funny_chars
+                               && (qs == shell_quoting_style
+                                   || qs == shell_always_quoting_style
+                                   || qs == literal_quoting_style);
+
+  if (needs_general_quoting != 0)
     {
-      buf = alloca (len + 1);
-      quotearg_buffer (buf, len + 1, name, -1, options);
+      len = quotearg_buffer (buf, bufsize, name, -1, options);
+      if (bufsize <= len)
+        {
+          buf = xmalloc (len + 1);
+          quotearg_buffer (buf, len + 1, name, -1, options);
+        }
+
+      quoted = (*name != *buf) || strlen (name) != len;
     }
+  else if (needs_further_quoting)
+    {
+      len = strlen (name);
+      if (bufsize <= len)
+        buf = xmalloc (len + 1);
+      memcpy (buf, name, len + 1);
 
-  enum quoting_style qs = get_quoting_style (options);
+      quoted = false;
+    }
+  else
+    {
+      len = strlen (name);
+      buf = name;
+      quoted = false;
+    }
 
-  if (qmark_funny_chars
-      && (qs == shell_quoting_style || qs == shell_always_quoting_style
-          || qs == literal_quoting_style))
+  if (needs_further_quoting)
     {
       if (MB_CUR_MAX > 1)
         {
@@ -4274,45 +4342,108 @@ quote_name (FILE *out, const char *name, struct quoting_options const *options,
         }
     }
 
-  if (out != NULL)
-    fwrite (buf, 1, len, out);
+  /* Set padding to better align quoted items,
+     and also give a visual indication that quotes are
+     not actually part of the name.  */
+  *pad = (align_variable_outer_quotes && cwd_some_quoted && ! quoted);
+
   if (width != NULL)
     *width = displayed_width;
+
+  *inbuf = buf;
+
   return len;
 }
 
 static size_t
-print_name_with_quoting (const struct fileinfo *f,
-                         bool symlink_target,
-                         struct obstack *stack,
-                         size_t start_col)
+quote_name_width (const char *name, struct quoting_options const *options,
+                  int needs_general_quoting)
 {
-  const char* name = symlink_target ? f->linkname : f->name;
+  char smallbuf[BUFSIZ];
+  char *buf = smallbuf;
+  size_t width;
+  bool pad;
+
+  quote_name_buf (&buf, sizeof smallbuf, (char *) name, options,
+                  needs_general_quoting, &width, &pad);
+
+  if (buf != smallbuf && buf != name)
+    free (buf);
+
+  width += pad;
 
-  bool used_color_this_time
-    = (print_with_color
-        && (print_color_indicator (f, symlink_target)
-            || is_colored (C_NORM)));
+  return width;
+}
+
+static size_t
+quote_name (char const *name, struct quoting_options const *options,
+            int needs_general_quoting, const struct bin_str *color,
+            bool allow_pad, struct obstack *stack)
+{
+  char smallbuf[BUFSIZ];
+  char *buf = smallbuf;
+  size_t len;
+  bool pad;
+
+  len = quote_name_buf (&buf, sizeof smallbuf, (char *) name, options,
+                        needs_general_quoting, NULL, &pad);
+
+  if (pad && allow_pad)
+      DIRED_PUTCHAR (' ');
+
+  if (color)
+    print_color_indicator (color);
 
   if (stack)
     PUSH_CURRENT_DIRED_POS (stack);
 
-  size_t width = quote_name (stdout, name, filename_quoting_options, NULL);
-  dired_pos += width;
+  fwrite (buf, 1, len, stdout);
+
+  if (buf != smallbuf && buf != name)
+    free (buf);
+
+  dired_pos += len;
 
   if (stack)
     PUSH_CURRENT_DIRED_POS (stack);
 
+  return len + pad;
+}
+
+static size_t
+print_name_with_quoting (const struct fileinfo *f,
+                         bool symlink_target,
+                         struct obstack *stack,
+                         size_t start_col)
+{
+  const char* name = symlink_target ? f->linkname : f->name;
+
+  const struct bin_str *color = print_with_color ?
+                                get_color_indicator (f, symlink_target) : NULL;
+
+  bool used_color_this_time = (print_with_color
+                               && (color || is_colored (C_NORM)));
+
+  size_t len = quote_name (name, filename_quoting_options, f->quoted,
+                           color, !symlink_target, stack);
+
   process_signals ();
   if (used_color_this_time)
     {
       prep_non_filename_text ();
+
+      /* We use the byte length rather than display width here as
+         an optimization to avoid accurately calculating the width,
+         because we only output the clear to EOL sequence if the name
+         _might_ wrap to the next line.  This may output a sequence
+         unnecessarily in multi-byte locales for example,
+         but in that case it's inconsequential to the output.  */
       if (line_length
-          && (start_col / line_length != (start_col + width - 1) / line_length))
+          && (start_col / line_length != (start_col + len - 1) / line_length))
         put_indicator (&color_indicator[C_CLR_TO_EOL]);
     }
 
-  return width;
+  return len;
 }
 
 static void
@@ -4403,9 +4534,26 @@ print_type_indicator (bool stat_ok, mode_t mode, enum filetype type)
   return !!c;
 }
 
-/* Returns whether any color sequence was printed. */
+/* Returns if color sequence was printed.  */
 static bool
-print_color_indicator (const struct fileinfo *f, bool symlink_target)
+print_color_indicator (const struct bin_str *ind)
+{
+  if (ind)
+    {
+      /* Need to reset so not dealing with attribute combinations */
+      if (is_colored (C_NORM))
+        restore_default_color ();
+      put_indicator (&color_indicator[C_LEFT]);
+      put_indicator (ind);
+      put_indicator (&color_indicator[C_RIGHT]);
+    }
+
+  return ind != NULL;
+}
+
+/* Returns color indicator or NULL if none.  */
+static const struct bin_str* _GL_ATTRIBUTE_PURE
+get_color_indicator (const struct fileinfo *f, bool symlink_target)
 {
   enum indicator_no type;
   struct color_ext_type *ext;	/* Color extension */
@@ -4508,22 +4656,10 @@ print_color_indicator (const struct fileinfo *f, bool symlink_target)
         type = C_ORPHAN;
     }
 
-  {
-    const struct bin_str *const s
-      = ext ? &(ext->seq) : &color_indicator[type];
-    if (s->string != NULL)
-      {
-        /* Need to reset so not dealing with attribute combinations */
-        if (is_colored (C_NORM))
-          restore_default_color ();
-        put_indicator (&color_indicator[C_LEFT]);
-        put_indicator (s);
-        put_indicator (&color_indicator[C_RIGHT]);
-        return true;
-      }
-    else
-      return false;
-  }
+  const struct bin_str *const s
+    = ext ? &(ext->seq) : &color_indicator[type];
+
+  return s->string ? s : NULL;
 }
 
 /* Output a color indicator (which may contain nulls).  */
@@ -4551,7 +4687,6 @@ static size_t
 length_of_file_name_and_frills (const struct fileinfo *f)
 {
   size_t len = 0;
-  size_t name_width;
   char buf[MAX (LONGEST_HUMAN_READABLE + 1, INT_BUFSIZE_BOUND (uintmax_t))];
 
   if (print_inode)
@@ -4570,8 +4705,7 @@ length_of_file_name_and_frills (const struct fileinfo *f)
   if (print_scontext)
     len += 1 + (format == with_commas ? strlen (f->scontext) : scontext_width);
 
-  quote_name (NULL, f->name, filename_quoting_options, &name_width);
-  len += name_width;
+  len += quote_name_width (f->name, filename_quoting_options, f->quoted);
 
   if (indicator_style != none)
     {
diff --git a/tests/local.mk b/tests/local.mk
index af34e2428..33350021d 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -584,6 +584,7 @@ all_tests =					\
   tests/ls/no-arg.sh				\
   tests/ls/no-cap.sh				\
   tests/ls/proc-selinux-segfault.sh		\
+  tests/ls/quote-align.sh			\
   tests/ls/readdir-mountpoint-inode.sh		\
   tests/ls/recursive.sh				\
   tests/ls/root-rel-symlink-color.sh		\
diff --git a/tests/ls/quote-align.sh b/tests/ls/quote-align.sh
new file mode 100755
index 000000000..d9e2c6321
--- /dev/null
+++ b/tests/ls/quote-align.sh
@@ -0,0 +1,63 @@
+#!/bin/sh
+# test quote alignment combinations
+
+# Copyright (C) 2016 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/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ ls
+
+dirname='dir:name'
+mkdir "$dirname" || framework_failure_
+touch "$dirname/a b" "$dirname/c.foo" || framework_failure_
+
+e=$(printf '\033')
+color_code='0;31;42'
+c_pre="$e[0m$e[${color_code}m"
+c_post="$e[0m"
+
+cat <<EOF >exp || framework_failure_
+'$dirname':
+'a b'  ${c_pre}c.foo${c_post}
+'$dirname':
+'a b'   ${c_pre}c.foo${c_post}
+'$dirname':
+total 0
+'a b'
+ ${c_pre}c.foo${c_post}
+'$dirname':
+'a b'
+${c_pre}c.foo${c_post}
+'$dirname':
+'a b', ${c_pre}c.foo${c_post}
+'$dirname':
+'a b'   ${c_pre}c.foo${c_post}
+
+EOF
+
+for opt in '-w0 -x' '-x' '-og' '-1' '-m' '-C'; do
+  env TERM=xterm LS_COLORS="*.foo=$color_code" TIME_STYLE=+T \
+    ls $opt -R --quoting=shell-escape --color=always "$dirname" >> out || fail=1
+done
+
+# Append a newline, to accommodate less-capable versions of sed.
+echo >> out || fail=1
+
+# Strip possible varying portion of long format
+sed 's/.*T //' out > k && mv k out
+
+compare exp out || fail=1
+
+Exit $fail