Re: Add a configuration file for dircolors' default colors [patch]

[Eric Blake]

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

According to Anthony DeRobertis on 11/13/2005 11:33 AM:
> "dircolors" uses a compiled-in set of default colors. This makes it hard
> for the sysadmin to change the defaults. Here is a patch which makes it
> look in /etc/dircolors.conf for the defaults, and fall back to the
> compiled-in ones if that file is not present.

Thanks for the patch.  In general, I like the idea, but I have a concern.
  If we start using a default filename in preference to the builtin
database, then we should also take pains to ensure that the filename we
use is selectable by ./configure rather than hard-coding it to be
/etc/dircolors.conf, and also that src/dircolors.hin gets installed to
this location.  In other words, we also need to see at least a patch to
src/Makefile.am.

Also, see my comments below.  You should attach a ChangeLog entry for
proposed patches.

> 
> diff -Nru3 ./coreutils-5.2.1/src/dircolors.c 
> ../build-tree.new/coreutils-5.2.1/src/dircolors.c
> --- ./coreutils-5.2.1/src/dircolors.c 2004-01-21 17:27:02.000000000 -0500
> +++ ../build-tree.new/coreutils-5.2.1/src/dircolors.c 2005-11-13 
> 12:04:16.000000000 -0500
> @@ -375,7 +375,7 @@
>  }
>  
>  static int
> -dc_parse_file (const char *filename)
> +dc_parse_file (const char *filename, int ignore_open_fail)
                                        ^^^
You are using this parameter as a boolean, so please make it type bool.

>  {
>    FILE *fp;
>    int err;
> @@ -394,6 +394,10 @@
>        fp = fopen (filename, "r");
>        if (fp == NULL)
>       {
> +       if (ignore_open_fail)
> +         {
> +           return -1;
> +         }
            ^
GNU coding standards state that if there is only one dependent statement
to a conditional, you should not use {}.

>         error (0, errno, "%s", quote (filename));
>         return 1;
>       }
> @@ -500,9 +504,13 @@
>  
>        obstack_init (&lsc_obstack);
>        if (argc == 0)
> -     err = dc_parse_stream (NULL, NULL);
> +             {
> +       err = dc_parse_file("/etc/dircolors.conf", 1);
> +       if (-1 == err)
> +         err = dc_parse_stream (NULL, NULL);
                                         ^^^^
NULL is not an int, but a pointer.  Use the right type in your call.

> +     }
>        else
> -     err = dc_parse_file (argv[0]);
> +     err = dc_parse_file (argv[0], 0);
>  
>        if (!err)
>       {
> @@ -533,3 +541,5 @@
>  
>    exit (err == 0 ? EXIT_SUCCESS : EXIT_FAILURE);
>  }
> +
> +/* vim: set ts=8 sw=2: */

None of the other files in coreutils have trailing editor hints (and most
of the core developers seem to prefer emacs over vi), so this change is
spurious.

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

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

iD8DBQFDeJNN84KuGfSFAYARAlGAAKCx7SiJqPNIsdI/MKZcqHF9lFwYlwCfcr8a
gF7lxWjI5knwJIZGvmm3Wzc=
=inJE
-----END PGP SIGNATURE-----