[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Add a configuration file for dircolors' default colors [patch]
From: |
Anthony DeRobertis |
Subject: |
Re: Add a configuration file for dircolors' default colors [patch] |
Date: |
Mon, 14 Nov 2005 23:40:59 -0500 |
User-agent: |
Debian Thunderbird 1.0.7 (X11/20051019) |
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.