Re: ls: new feature proposal

[Julio Auto]

Thanks for your reply! :)

Too bad my patch has little chances to be incorporated into ls :(
Anyway, if you feel these chances grow bigger, just let me know and
I'll promptly get the correct posting style done (diff, ChangeLog,
etc..). Also, the copyright papers would be no problem.

As to the use of alloca(), the char* casting, and the "name[0] == '/'"
issue, they were already present in the version I got from CVS, that's
why i didn't mind changing them (or even paying much attention, in
fact - i just assumed everything there was correct, my mistake).

But, hey! Seems like we've found a few small bugs after all :)

Once again, thank you for your reply. It's always good to learn
something to improve your knowledge on different library functions,
your coding style, and even your patch format!

Greetings from Brazil,

           Julio Auto

On 3/29/06, Eric Blake <address@hidden> wrote:
> -----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-----
>