On 28/12/18 00:20, Assaf Gordon wrote: > Hello Pádraig, > > On 2018-12-27 7:37 a.m., Pádraig Brady wrote: >> On 23/12/18 19:35, Amin Bandali wrote: >>> >>> It would be great if ls had an option to group directory symlinks with >>> the real directories when --group-directories-first is specified. >> >> Good point. >> It probably should just do that with no other options. >> The attached implements that. >> > > A cursory test seem to indicate this doesn't work on Free/OpenBSDs > (but works well on Debian). > > On BSDs, inside "is_linked_directoccry", when visiting a > symlink-to-direcotory, f->filetype=6 (which is symlink), but > f->linkmode is zero, so S_ISDIR(f->linkmode) returns false. > > When adding "-L" (dereference symlinks), it works as expected > (but in such cases, f->filetype=3, not 6). > > I'm happy to test more on BSDs if needed, and also my BSD virtual > machines are now in the gcc-compile-farm making testing even > easier (https://cfarm.tetaneutral.net/machines/list/). > > Lastly, > > In the commit message, I suggest: > s/used/use/ > and > Suggested by Amin Bandali in > https://lists.gnu.org/r/coreutils/2018-12/msg00017.html
I updated to fix to always set check_symlink_mode with --group. I also simplified gobble_file() a bit to unconditionally set linkmode. cheers, Pádraig
>From 88d767e00465217bd7318866f539bacd59752501 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Thu, 27 Dec 2018 06:12:05 -0800 Subject: [PATCH] ls: with --group-directories-first, also group symlinked dirs * src/ls.c (is_linked_directory): A new function to also consider symlinked directories. (main): Rename check_symlink_color to check_symlink_mode, and enable that with --group-directories-first. (DIRFIRST_CHECK): Adjust to use is_linked_directory, rather than just is_directory. (gobble_file): Simplify to always update f->linkmode if the stat() succeeds. * tests/ls/group-dirs.sh: A new test. * tests/local.mk: Reference the new test. * NEWS: Mention the change in behavior. Suggested by Amin Bandali in https://lists.gnu.org/r/coreutils/2018-12/msg00017.html --- NEWS | 2 ++ src/ls.c | 40 ++++++++++++++++++++-------------------- tests/local.mk | 1 + tests/ls/group-dirs.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 20 deletions(-) create mode 100755 tests/ls/group-dirs.sh diff --git a/NEWS b/NEWS index 4a57d22..01c1358 100644 --- a/NEWS +++ b/NEWS @@ -36,6 +36,8 @@ GNU coreutils NEWS -*- outline -*- approach is still used in situations where hard links to directories are allowed (e.g., NetBSD when superuser). + ls --group-directories-first will also group symlinks to directories. + 'test -a FILE' is not supported anymore. Long ago, there were concerns about the high probability of humans confusing the -a primary with the -a binary operator, so POSIX changed this to 'test -e FILE'. Scripts using it were diff --git a/src/ls.c b/src/ls.c index 5ad9bc2..243c186 100644 --- a/src/ls.c +++ b/src/ls.c @@ -640,9 +640,9 @@ static struct color_ext_type *color_ext_list = NULL; static char *color_buf; /* True means to check for orphaned symbolic link, for displaying - colors. */ + colors, or to group symlink to directories with other dirs. */ -static bool check_symlink_color; +static bool check_symlink_mode; /* True means mention the inode number of each file. -i */ @@ -1477,13 +1477,15 @@ main (int argc, char **argv) /* Test print_with_color again, because the call to parse_ls_color may have just reset it -- e.g., if LS_COLORS is invalid. */ - if (print_with_color) + if (directories_first) + check_symlink_mode = true; + else if (print_with_color) { /* Avoid following symbolic links when possible. */ if (is_colored (C_ORPHAN) || (is_colored (C_EXEC) && color_symlink_as_referent) || (is_colored (C_MISSING) && format == long_format)) - check_symlink_color = true; + check_symlink_mode = true; } if (dereference == DEREF_UNDEFINED) @@ -3149,7 +3151,7 @@ gobble_file (char const *name, enum filetype type, ino_t inode, || ((print_inode || format_needs_type) && (type == symbolic_link || type == unknown) && (dereference == DEREF_ALWAYS - || color_symlink_as_referent || check_symlink_color)) + || color_symlink_as_referent || check_symlink_mode)) /* Command line dereferences are already taken care of by the above assertion that the inode number is not yet known. */ || (print_inode && inode == NOT_AN_INODE_NUMBER) @@ -3300,7 +3302,7 @@ gobble_file (char const *name, enum filetype type, ino_t inode, } if (S_ISLNK (f->stat.st_mode) - && (format == long_format || check_symlink_color)) + && (format == long_format || check_symlink_mode)) { struct stat linkstats; @@ -3315,21 +3317,11 @@ gobble_file (char const *name, enum filetype type, ino_t inode, /* Avoid following symbolic links when possible, ie, when they won't be traced and when no indicator is needed. */ if (linkname - && (file_type <= indicator_style || check_symlink_color) + && (file_type <= indicator_style || check_symlink_mode) && stat (linkname, &linkstats) == 0) { f->linkok = true; - - /* Symbolic links to directories that are mentioned on the - command line are automatically traced if not being - listed as files. */ - if (!command_line_arg || format == long_format - || !S_ISDIR (linkstats.st_mode)) - { - /* Get the linked-to file's mode for the filetype indicator - in long listings. */ - f->linkmode = linkstats.st_mode; - } + f->linkmode = linkstats.st_mode; } free (linkname); } @@ -3443,6 +3435,14 @@ is_directory (const struct fileinfo *f) return f->filetype == directory || f->filetype == arg_directory; } +/* Return true if F refers to a (symlinked) directory. */ +static bool +is_linked_directory (const struct fileinfo *f) +{ + return f->filetype == directory || f->filetype == arg_directory + || S_ISDIR (f->linkmode); +} + /* Put the name of the file that FILENAME is a symbolic link to into the LINKNAME field of 'f'. COMMAND_LINE_ARG indicates whether FILENAME is a command-line argument. */ @@ -3588,8 +3588,8 @@ typedef int (*qsortFunc)(V a, V b); #define DIRFIRST_CHECK(a, b) \ do \ { \ - bool a_is_dir = is_directory ((struct fileinfo const *) a); \ - bool b_is_dir = is_directory ((struct fileinfo const *) b); \ + bool a_is_dir = is_linked_directory ((struct fileinfo const *) a);\ + bool b_is_dir = is_linked_directory ((struct fileinfo const *) b);\ if (a_is_dir && !b_is_dir) \ return -1; /* a goes before b */ \ if (!a_is_dir && b_is_dir) \ diff --git a/tests/local.mk b/tests/local.mk index e494eff..290e30f 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -595,6 +595,7 @@ all_tests = \ tests/ls/file-type.sh \ tests/ls/follow-slink.sh \ tests/ls/getxattr-speedup.sh \ + tests/ls/group-dirs.sh \ tests/ls/hex-option.sh \ tests/ls/infloop.sh \ tests/ls/inode.sh \ diff --git a/tests/ls/group-dirs.sh b/tests/ls/group-dirs.sh new file mode 100755 index 0000000..b3b266b --- /dev/null +++ b/tests/ls/group-dirs.sh @@ -0,0 +1,43 @@ +#!/bin/sh +# test --group-directories-first + +# Copyright (C) 2018 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 <https://www.gnu.org/licenses/>. + +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src +print_ver_ ls + +# Isolate output files from directory being listed +mkdir dir dir/b || framework_failure_ +touch dir/a || framework_failure_ +ln -s b dir/bl || framework_failure_ + +ls --group dir > out || fail=1 +cat <<\EOF > exp +b +bl +a +EOF +compare exp out || fail=1 + +ls --group -d dir/* > out || fail=1 +cat <<\EOF > exp +dir/b +dir/bl +dir/a +EOF +compare exp out || fail=1 + +Exit $fail -- 2.9.3
