bug-coreutils
[Top][All Lists]
Advanced

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

Re: ls: new feature proposal


From: Eric Blake
Subject: Re: ls: new feature proposal
Date: Tue, 28 Mar 2006 20:33:55 -0700
User-agent: Thunderbird 1.5 (Windows/20051201)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Julio Auto on 3/28/2006 8:06 PM:
> What:
> A new feature for the 'ls' program, initially designed to be triggered
> by the (randomly picked) '-e' option, that shall enable the printing
> of the absolute paths to the files listed, as opposed to the "filename
> only" standard approach used today.

Thanks for the patch.  Not to discourage you, but ls is already so
full-featured (some would say too-featured) that adding another option is
a VERY difficult task to convince people that it is necessary.  In this
case, you admit that find can already do what you desire; hence I
seriously doubt that your patch will be incorporated.  Even if your idea
were to be used, I would prefer only a long option (without a good
mnemonic, -e doesn't make much sense).

As to your patch, we prefer patches in unified format (diff -up), rather
than ed format (context is essential for properly applying a patch).  And
make sure it is against CVS head.  Also, don't break the patch into
multiple chunks; just provide a single patch along with a ChangeLog entry
that tells what was done.  Any new option requires mention in NEWS and
coreutils.texi, as well as additions to the testsuite.  And finally, your
patch is big enough that if something of this magnitude were to be
applied, you would need to sign copyright papers and mail them to the FSF
before your code could be incorporated.

As an example of a recent patch that follows these guidelines and was
accepted, check out my recent patch that added 'rm -I':
http://lists.gnu.org/archive/html/bug-coreutils/2006-02/msg00098.html

Finally, some comments about your coding style, that would need to be
addressed before your patch could be applied:

>>   /* Absolute name of this file.  */
>>   char *absolute_name;
>>
>>   if (name[0] == '/')

This is not portable.  Look at lib/dirname.h for the ISSLASH macro.

>>     absolute_name = (char *) name;

Why are you casting here?

>>   else
>>     {
>>       if ((dirname[0] == '.' && dirname[1] == '\0') ||

GNU Coding standards put operators at the start of the new line, not at
the end of the line where you are breaking.

>>        (dirname[0] == 0))

This set of parentheses is redundant.

>>         {
>>        char *current_working_dir = getenv("PWD");

This is overkill, and prone to failure.  Instead, look at using lib/xgetcwd.c.

>>        if (current_working_dir != NULL)
>>          dirname = current_working_dir;
>>      }
>>
>>       absolute_name = alloca (strlen (name) + strlen (dirname) + 2);

alloca on arbitrary-length names is prone to stack overflow.  Consider
using xmalloc instead.

- --
Life is short - so eat dessert first!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEKgAi84KuGfSFAYARAuzZAKCV+MPzeur3PSy9vDUB3oFpvUYKvwCgoLjp
ZPq6bn0JlmoKxbDq9LxGtg8=
=F0fy
-----END PGP SIGNATURE-----




reply via email to

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