Re: ls and root directory indicator

[Eric Blake]

On 02/25/2013 12:51 PM, Sami Kerola wrote:
> Did I understood correctly that a patch such below would be more preferred?

Closer, but not quite there yet.

> 
> Secondly,pardon my ignorance, I thought '/' and '//' or how ever many
> slashes are the same root.

"///", "////", and any other larger number of other slashes are all the
same as "/".  Only "//" is special.

> Is this some non-obvious portability
> gotcha? A link to education material would be great.

POSIX says:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12

"A pathname consisting of a single <slash> shall resolve to the root
directory of the process. A null pathname shall not be successfully
resolved. A pathname that begins with two successive <slash> characters
may be interpreted in an implementation-defined manner, although more
than two leading <slash> characters shall be treated as a single <slash>
character."

Linux defines "//" to be a synonym for "/", which is why lots of people
fail to realize the portability issue.  But Cygwin defines "//" to be a
separate root, where "//machine/share" is the preferred way to access
remote share drives.  There, 'ls //' shows a list of accessible remote
machines (speaking from personal experience, that is nice for small home
networks, but painfully slow on large corporate networks)

> @@ -3970,7 +3971,7 @@ print_long_format (const struct fileinfo *f)
>              print_type_indicator (true, f->linkmode, unknown);
>          }
>      }
> -  else if (indicator_style != none)
> +  else if (indicator_style != none && is_root_only(f->name))
>      print_type_indicator (f->stat_ok, f->stat.st_mode, f->filetype);

This logic feels backwards - you want to print_type_indicator() on
non-root, but when reading the code as written, it looks like you are
only printing indicators on roots.

>  }
> 
> @@ -4212,7 +4213,7 @@ print_file_name_and_frills (const struct
> fileinfo *f, size_t start_col)
> 
>    size_t width = print_name_with_quoting (f, false, NULL, start_col);
> 
> -  if (indicator_style != none)
> +  if (indicator_style != none && is_root_only(f->name))
>      width += print_type_indicator (f->stat_ok, f->stat.st_mode, f->filetype);

Again, this logic feels backwards.

> 
>    return width;
> @@ -4397,6 +4398,21 @@ put_indicator (const struct bin_str *ind)
>    fwrite (ind->string, ind->len, 1, stdout);
>  }
> 
> +/* Find out if the output name is root, and represent it as a single
> + * slash even if file type indication is requested.  */
> +static int _GL_ATTRIBUTE_PURE
> +is_root_only (const char *path)
> +{
> +  while (*path)
> +    {
> +      if (*path == '/')
> +        path++;
> +      else
> +        return 1;

Ah, that explains why your logic feels backwards.  Based on your choice
of names, I was expecting true (non-zero) for root, false (zero) if only
slashes were seen, but you did the exact opposite.

We prefer bool over int for predicate functions that only return a yes
or no answer.  Also, detecting roots is shorter (less code, and
potentially faster operation if libc optimized strspn() to do
word-at-a-time searching instead of byte-at-a-time) when done as:

static bool _GL_ATTRIBUTE_PURE
is_root (const char *path)
{
  return path[strspn(path, "/")] == '\0';
}

Hmm, that form is optimized to POSIX; it might need a bit more
hand-holding to be portable to mingw, where drive letters are also
treated as roots, and where backslash can be used in place of slash.
But I'm okay if we leave the case of mingw to someone that develops on
that platform; again, isolating things into an is_root() helper function
already gives them a nice place to tackle if they want to enhance the
function to work on drive letters.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature