Re: [bug] generic issue with git_config handlers

[Christian Couder <chriscool@tuxfamily.org>]

Le mardi 5 février 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Very unfortunate.
> >
> > I finally had the following patch that passed all tests (it changed
> > only one test), in case someone wants to suggest that we change
> > git_config_bool, hint, hint!
>
> Sorry, I do not get what you are hinting at. 

Well, I wanted someone else to suggest that we deprecate using "" as a 
boolean false.

Something like this (completely untested) :

diff --git a/config.c b/config.c
index 526a3f4..44afeaa 100644
--- a/config.c
+++ b/config.c
@@ -300,8 +300,19 @@ int git_config_bool(const char *name, const char 
*value)
 {
        if (!value)
                return 1;
-       if (!*value)
+       if (!*value) {
+               fprintf(stderr,
+                       "Warning: using an empty value for boolean config "
+                       "variables is deprecated.\n"
+                       "An empty value currently means 'false' as a "
+                       "boolean, but may very well means 'true' in the "
+                       "future!\n"
+                       "Please consider using a 'false' value explicitely "
+                       "for variable '%s', so that your config is future "
+                       "proof. You can do that using:\n"
+                       "\tgit config %s false\n", name, name)
                return 0;
+       }
        if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
                return 1;
        if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))

So that in a future version we can use "" internaly for both no value and 
empty value variables and consider them meaning the same.

> The fact that you
> passed all the tests suggests that we have a gap in the test
> coverage for these two, so you are inviting more tests from
> others?

I just sent a patch to close this gap.

[...]

> Your "builtin-config.c" patch looks better than before (which
> would segfault), but I think
>
> 	$ git config --bool --list
>
> could pay attention to the "type" thing set earlier, just like
> show_config() does.

I didn't try this, but I will.

Thanks,
Christian.