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

[Anthony DeRobertis]

Eric Blake wrote:

> 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.

Good point. Not only that, the patch I wrote would completely ignore
--prefix, which is surely a bad thing.

I'll work on a new, better patch to fix this issue and also the ones you
point out below. I may not have a chance to work on this again until
Wednesday, though (or worse, next week).

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

Ok. I'll do that.

> 
> 
>>>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.

Type bool is a C++ extension; this is a C file. Now, given, it was added
to C99, so maybe it's OK.

I notice that, e.g., dc_parse_file is actually using "int" effectively
as bool (at least before I patched it); is it OK to use bool?

Looking quickly at
http://www.gnu.org/prep/standards/standards.html#Standard-C it appears
C99 is to be avoided, so (unless I'm missing something obvious) there is
no bool type.

> 
> 
>>> {
>>>   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 {}.

Ok. Will fix.

> 
> 
>>>       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.

Umm...

        static int
        dc_parse_stream (FILE *fp, const char *filename)

is the prototype. Did you maybe confuse dc_parse_file and dc_parse_stream?



> 
> 
>>>+    }
>>>       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.

Yeah, that last bit of the patch shouldn't have been sent.