* [bug] generic issue with git_config handlers
@ 2008-01-31 9:16 Pierre Habouzit
2008-01-31 9:25 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Pierre Habouzit @ 2008-01-31 9:16 UTC (permalink / raw
To: Git ML
[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]
One of my co-workers stumbled upon a misfeature of the git config
parser. The following syntax is allowed:
[section]
foo
I saw that this is a feature, though as a consequence, the "value"
passed to git_config handlers may be NULL, and a _lot_ of git config
handlers don't know this could happen. This becomes an issue when you do
something like:
[user]
name
--> every git command segfaults basically
[alias]
foo
--> `git foo` segfaults
I wanted to fix that, and generate nicer errors than a crash, changing
git_config to also take a boolean argument telling if the caller expects
"value" to be NULL, or would like to reject it, though the code has so
many callbacks to fix, and I have too little time right now, that I just
drop the thing on the list, hoping that some nice soul will take care of
the issue.
Cheers,
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug] generic issue with git_config handlers
2008-01-31 9:16 [bug] generic issue with git_config handlers Pierre Habouzit
@ 2008-01-31 9:25 ` Junio C Hamano
2008-01-31 10:10 ` Pierre Habouzit
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2008-01-31 9:25 UTC (permalink / raw
To: Pierre Habouzit; +Cc: Git ML
Pierre Habouzit <madcoder@debian.org> writes:
> One of my co-workers stumbled upon a misfeature of the git config
> parser. The following syntax is allowed:
>
> [section]
> foo
Yeah, that is how "truth" value of boolean is spelled.
> [user]
> name
That's very unfortunate. Whatever is expecting string value
should check for NULL. Fix should probably be easy enough for
any git-hacker-wannabe to tackle ;-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug] generic issue with git_config handlers
2008-01-31 9:25 ` Junio C Hamano
@ 2008-01-31 10:10 ` Pierre Habouzit
2008-02-04 6:27 ` Christian Couder
0 siblings, 1 reply; 8+ messages in thread
From: Pierre Habouzit @ 2008-01-31 10:10 UTC (permalink / raw
To: Junio C Hamano; +Cc: Git ML
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
> > One of my co-workers stumbled upon a misfeature of the git config
> > parser. The following syntax is allowed:
> >
> > [section]
> > foo
>
> Yeah, that is how "truth" value of boolean is spelled.
>
> > [user]
> > name
>
> That's very unfortunate. Whatever is expecting string value
> should check for NULL. Fix should probably be easy enough for
> any git-hacker-wannabe to tackle ;-)
I think so too, though my count is something like 40 functions to
investigate (the 40 handlers) and where it recurses into ;) Too much
work for the time I have right now.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug] generic issue with git_config handlers
2008-01-31 10:10 ` Pierre Habouzit
@ 2008-02-04 6:27 ` Christian Couder
2008-02-04 17:01 ` Johannes Sixt
0 siblings, 1 reply; 8+ messages in thread
From: Christian Couder @ 2008-02-04 6:27 UTC (permalink / raw
To: Pierre Habouzit; +Cc: Junio C Hamano, Git ML
Le jeudi 31 janvier 2008, Pierre Habouzit a écrit :
> On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:
> > Pierre Habouzit <madcoder@debian.org> writes:
> > > One of my co-workers stumbled upon a misfeature of the git config
> > > parser. The following syntax is allowed:
> > >
> > > [section]
> > > foo
> >
> > Yeah, that is how "truth" value of boolean is spelled.
> >
> > > [user]
> > > name
> >
> > That's very unfortunate. Whatever is expecting string value
> > should check for NULL. Fix should probably be easy enough for
> > any git-hacker-wannabe to tackle ;-)
>
> I think so too, though my count is something like 40 functions to
> investigate (the 40 handlers) and where it recurses into ;) Too much
> work for the time I have right now.
I would suggest this patch:
---8<---
diff --git a/config.c b/config.c
index 526a3f4..92613c5 100644
--- a/config.c
+++ b/config.c
@@ -139,7 +139,7 @@ static int get_value(config_fn_t fn, char *name,
unsigned in
if (!value)
return -1;
}
- return fn(name, value);
+ return fn(name, value ? value : "");
}
static int get_extended_base_var(char *name, int baselen, int c)
---8<---
but it breaks some test cases.
$ ./t1300-repo-config.sh -d -i -v
[...]
* expecting success: git config --get-regexp novalue > output &&
cmp output expect
output expect differ: char 17, line 1
* FAIL 34: get-regexp variable with no value
git config --get-regexp novalue > output &&
cmp output expect
$ cat output | hexdump -C
00000000 6e 6f 76 61 6c 75 65 2e 76 61 72 69 61 62 6c 65 |
novalue.variable|
00000010 20 0a | .|
00000012
$ cat expect | hexdump -C
00000000 6e 6f 76 61 6c 75 65 2e 76 61 72 69 61 62 6c 65 |
novalue.variable|
00000010 0a |.|
00000011
I don't know if the added space is a big problem.
It comes from the following code in builtin-config.c:44
if (show_keys) {
if (value_)
printf("%s%c", key_, key_delim);
else
printf("%s", key_);
where "value_" is now "" instead of NULL.
At this point, as I don't know much the code in these files, I think I could
very well use some advice from people more familiar with this.
Thanks in advance,
Christian.
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [bug] generic issue with git_config handlers
2008-02-04 6:27 ` Christian Couder
@ 2008-02-04 17:01 ` Johannes Sixt
2008-02-04 23:13 ` Christian Couder
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Sixt @ 2008-02-04 17:01 UTC (permalink / raw
To: Christian Couder; +Cc: Pierre Habouzit, Junio C Hamano, Git ML
Christian Couder schrieb:
> Le jeudi 31 janvier 2008, Pierre Habouzit a écrit :
>> On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:
>>> Pierre Habouzit <madcoder@debian.org> writes:
>>>> One of my co-workers stumbled upon a misfeature of the git config
>>>> parser. The following syntax is allowed:
>>>>
>>>> [section]
>>>> foo
>>> Yeah, that is how "truth" value of boolean is spelled.
>>>
>>>> [user]
>>>> name
>>> That's very unfortunate. Whatever is expecting string value
>>> should check for NULL. Fix should probably be easy enough for
>>> any git-hacker-wannabe to tackle ;-)
>> I think so too, though my count is something like 40 functions to
>> investigate (the 40 handlers) and where it recurses into ;) Too much
>> work for the time I have right now.
>
> I would suggest this patch:
>
> ---8<---
> diff --git a/config.c b/config.c
> index 526a3f4..92613c5 100644
> --- a/config.c
> +++ b/config.c
> @@ -139,7 +139,7 @@ static int get_value(config_fn_t fn, char *name,
> unsigned in
> if (!value)
> return -1;
> }
> - return fn(name, value);
> + return fn(name, value ? value : "");
> }
You can't. The reason is that get_config_bool() treats value == NULL and
*value == '\0' differently. *That's* the most unfortunate part of it. :-(
-- Hannes
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug] generic issue with git_config handlers
2008-02-04 17:01 ` Johannes Sixt
@ 2008-02-04 23:13 ` Christian Couder
2008-02-05 0:03 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Christian Couder @ 2008-02-04 23:13 UTC (permalink / raw
To: Johannes Sixt; +Cc: Pierre Habouzit, Junio C Hamano, Git ML
Le lundi 4 février 2008, Johannes Sixt a écrit :
> Christian Couder schrieb:
> > return -1;
> > }
> > - return fn(name, value);
> > + return fn(name, value ? value : "");
> > }
>
> You can't. The reason is that get_config_bool() treats value == NULL and
> *value == '\0' differently. *That's* the most unfortunate part of it. :-(
You are right. We have this (in config.c:299):
int git_config_bool(const char *name, const char *value)
{
if (!value)
return 1;
if (!*value)
return 0;
if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
return 1;
if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))
return 0;
return git_config_int(name, value) != 0;
}
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!
Thanks,
Christian.
---8<---
diff --git a/builtin-config.c b/builtin-config.c
index e4a12e3..b92cf4b 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -20,7 +20,7 @@ static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;
static int show_all_config(const char *key_, const char *value_)
{
- if (value_)
+ if (value_ && *value_)
printf("%s%c%s%c", key_, delim, value_, term);
else
printf("%s%c", key_, term);
@@ -42,7 +42,7 @@ static int show_config(const char* key_, const char*
value_)
return 0;
if (show_keys) {
- if (value_)
+ if (value_ && *value_)
printf("%s%c", key_, key_delim);
else
printf("%s", key_);
diff --git a/config.c b/config.c
index 526a3f4..a2c7214 100644
--- a/config.c
+++ b/config.c
@@ -131,7 +131,7 @@ static int get_value(config_fn_t fn, char *name,
unsigned in
while (c == ' ' || c == '\t')
c = get_next_char();
- value = NULL;
+ value = "";
if (c != '\n') {
if (c != '=')
return -1;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index a786c5c..deb11dc 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -611,8 +611,7 @@ foo
barQsection.sub=section.val3
-Qsection.sub=section.val4
-Qsection.sub=section.val5Q
+Qsection.sub=section.val4Qsection.sub=section.val5Q
EOF
git config --null --list | tr '\000' 'Q' > result
---8<---
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [bug] generic issue with git_config handlers
2008-02-04 23:13 ` Christian Couder
@ 2008-02-05 0:03 ` Junio C Hamano
2008-02-07 5:45 ` Christian Couder
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2008-02-05 0:03 UTC (permalink / raw
To: Christian Couder; +Cc: Johannes Sixt, Pierre Habouzit, Git ML
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. 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?
> diff --git a/config.c b/config.c
> index 526a3f4..a2c7214 100644
> --- a/config.c
> +++ b/config.c
> @@ -131,7 +131,7 @@ static int get_value(config_fn_t fn, char *name,
> unsigned in
> while (c == ' ' || c == '\t')
> c = get_next_char();
>
> - value = NULL;
> + value = "";
> if (c != '\n') {
> if (c != '=')
> return -1;
As long as you have this, I do not think you can avoid breaking
existing repositories that have:
[core]
autocrlf
filemode =
and expect git to say "Ah, core.autocrlf is set to true, and
filemode is not trustable, so I need to do a MS-DOG".
$ git config --bool core.autocrlf
true
$ git config --bool core.filemode
false
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug] generic issue with git_config handlers
2008-02-05 0:03 ` Junio C Hamano
@ 2008-02-07 5:45 ` Christian Couder
0 siblings, 0 replies; 8+ messages in thread
From: Christian Couder @ 2008-02-07 5:45 UTC (permalink / raw
To: Junio C Hamano; +Cc: Johannes Sixt, Pierre Habouzit, Git ML
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.
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-02-07 5:39 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-01-31 9:16 [bug] generic issue with git_config handlers Pierre Habouzit
2008-01-31 9:25 ` Junio C Hamano
2008-01-31 10:10 ` Pierre Habouzit
2008-02-04 6:27 ` Christian Couder
2008-02-04 17:01 ` Johannes Sixt
2008-02-04 23:13 ` Christian Couder
2008-02-05 0:03 ` Junio C Hamano
2008-02-07 5:45 ` Christian Couder
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).