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
