bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [patch #3596] Sort directories before files in "ls"


From: Francesco Montorsi
Subject: Re: [patch #3596] Sort directories before files in "ls"
Date: Thu, 15 Dec 2005 20:44:30 +0100
User-agent: Mozilla Thunderbird 1.0.7-1.1.fc4 (X11/20050929)

Hi,

Eric Blake wrote:
Usually, posting patches directly to the list instead of to the savannah
web site gets more response for this project.
Ok thanks; I'll keep it in mind for next patches.


Index: ChangeLog


Usually, ChangeLog entries are just attached separately from the patch
(chances are, by the time your patch is incorporated, other changes will
have been committed, leading to needless patch conflicts if you include
ChangeLog in your patch).
Ok, I'll make a separate patch


+   * src/ls.c: Added the -e,--group-directories option to group
+   directories before files (Francesco Montorsi).
   * src/rm.c (long_opts): Change the name of each undocumented, for-
   testing-only option to start with `-', so that it cannot render


WHOA - Don't add your entry into the middle of someone else's.  Make a
brand new entry.  Also, your entry needs to call out all the functions you
touched, not just an overview.  Your change is big enough that you will
need to assign copyright, and it is more formal than just a disclaimer on
the savannah website.  Ask Paul or Jim for the paperwork you will need to
mail into the FSF.
Ok, I did it in my local copy.
Just one thing about copyright: do I need to mail the documents or I can just 
email them ?


Index: NEWS
===================================================================
RCS file: /cvsroot/coreutils/coreutils/NEWS,v
retrieving revision 1.349
diff -b -u -2 -r1.349 NEWS
--- NEWS    26 Nov 2005 07:51:27 -0000  1.349
+++ NEWS    9 Dec 2005 13:42:49 -0000
@@ -24,4 +24,7 @@
  attempts to have the default be the best of both worlds.

+  ls now supports the '-e,--group-directories' option to group directories
+  before files.


Call out the two spellings in separate words.
done



+/* True means that directories are grouped before files.

- -e,--group-directories  */

Follow the GNU coding conventions, plus the example of existing code in
this file.  In particular, comments end in a period, and must not exceed
80 columns.
I followed the example for the immediate_dirs variable comment which ends the
comment with a period (like I did) and then adds the name of the option which
enable that behaviour. I did not respect the 80 columns though; I changed it.


+static bool group_dirs;
+
/* Which files to ignore.  */

@@ -751,4 +755,5 @@
  {"directory", no_argument, NULL, 'd'},
  {"dired", no_argument, NULL, 'D'},
+  {"group-directories", no_argument, NULL, 'e'},
  {"full-time", no_argument, NULL, FULL_TIME_OPTION},
  {"human-readable", no_argument, NULL, 'h'},
@@ -1490,5 +1495,5 @@

  while ((c = getopt_long (argc, argv,
-              "abcdfghiklmnopqrstuvw:xABCDFGHI:LNQRST:UX1",
+               "abcdefghiklmnopqrstuvw:xABCDFGHI:LNQRST:UX1",


Careful on leading whitespace.  For changes, coreutils prefers leading
tabs except in string literals that wrap lines (although, as I recently
found out, you should not change whitespace on lines that are otherwise
unaffected, as it makes diff's to prior versions harder to follow).
if I understand correctly, the problem is that added space... I removed it.


              long_options, NULL)) != -1)
    {
@@ -1511,4 +1516,8 @@
     break;

+    case 'e':
+      group_dirs = true;
+      break;
+
   case 'f':
     /* Same as enabling -a -U and disabling -l -s.  */
@@ -2728,4 +2737,11 @@
}

+/* Returns true if the given fileinfo refers to a directory */
+static bool is_directory (const struct fileinfo *f)
+{
+  return f->filetype == directory || f->filetype == arg_directory;
+}


You didn't ensure that f->filetype was always populated.  You will have to
look for the existing code that checks whether to stat files, such as when
- -F is in effect, and add that a stat must take place if your proposed -e
is in effect.
Ok; I changed the following code from:

  format_needs_stat = sort_type == sort_time || sort_type == sort_size
    || format == long_format
    || dereference == DEREF_ALWAYS
    || print_block_size || print_inode;

to:

  format_needs_stat = sort_type == sort_time || sort_type == sort_size
    || format == long_format
    || dereference == DEREF_ALWAYS
    || print_block_size || print_inode
->  || group_dirs;



-    if ((files[i].filetype == directory || files[i].filetype ==

arg_directory)

-   && (!ignore_dot_and_dot_dot
+    if (is_directory(&files[i]) && (!ignore_dot_and_dot_dot
       || !basename_is_dot_or_dotdot (files[i].name)))


You broke coding conventions again.  Nested conditionals indent to the
level of the open parenthesis that groups them.
ok, moved that || a space on the right.



      {
@@ -2956,4 +2971,34 @@
static int rev_str_extension (V a, V b) { return compstr_extension (b,

a); }

+/* Groups directories at the beginning of the array */
+
+static inline int
+cmp_directories (V a, V b)
+{
+  bool a_isfolder = is_directory((struct fileinfo const *)a);
+  bool b_isfolder = is_directory((struct fileinfo const *)b);
+  if (a_isfolder && !b_isfolder)
+     return -1;        // a goes before b
+  else if (!a_isfolder && b_isfolder)


When the previous if statement only does a return, the else is redundant.
 You can make this line just say if instead of else if.
done





+     return 1;          // b goes before a
+
+  /* a and b are both files or both folders;
+     will be sorted later using the user-selected sortkey */
+  return 0;
+}
+
+int group_dirs_first()
+{
+  int i, n=0;
+  qsort (files, files_index, sizeof *files, cmp_directories);


EVIL.  You are calling qsort twice for every element (once to put
directories first, then twice again on the subsets).  This is twice as
slow as just writing a proper sort function that does everything all in
one comparison, so that you only call qsort once.
however if we want to make a single qsort call, then we'll need to declare a lot of new functions which merge previous tests with the groupdir test:

static int groupdir_compare_name (V a, V b)
static int groupdir_compstr_name (V a, V b)
static int groupdir_rev_cmp_name (V a, V b)
static int groupdir_rev_str_name (V a, V b)

static int groupdir_compare_extension (V a, V b)
static int groupdir_compstr_extension (V a, V b)
static int groupdir_rev_cmp_extension (V a, V b)
static int groupdir_rev_str_extension (V a, V b)

...


is it right ?
Won't that clutter the source file ?



Thanks,
Francesco Montorsi





reply via email to

[Prev in Thread] Current Thread [Next in Thread]