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

[Eric Blake]

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

There is no rush here - it will be some time before a 6.0 release with
new features.  I have several patches that I have not had time to
complete, myself.  Good patches are favored over incomplete ones.
Since this change would be user-visible, you should also provide a
patch to the documentation.

> 
> > 
> > Also, see my comments below.  You should attach a ChangeLog entry for
> > proposed patches.
> 
> Ok. I'll do that.

Also, it sounds like your changes will probably be large enough to
need a copyright disclaimer in place, if you haven't done that before.

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

Two things here - coreutils uses the gnulib 'stdbool' module, so you
can use bool with impunity even on a C89 compiler (where an
appropriate #define or enum is used instead).  Also, coreutils
6.0-cvs now requires C99 (well, at least the ability to declare variables
after statements without opening a new {}).  Search the mailing list
archives for Jim's announcement of this fact.

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

Feel free to improve that if you wish - it is a general TODO item
to replace int with bool throughout coreutils where that is the
usage pattern of the variable.

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

Hmm, maybe we should request an update to the coding standards
to point out the gnulib module.

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

Sorry - I guess I did misread that.

--
Eric Blake