On Mon, 11 Feb 2008, Pierre Habouzit wrote: > > > Having said all that, it might be an option to change your patch > > slightly to say: > > > > const char config_true[] = "true"; > > I was about to suggest the same, and testing against "config_true" just > becomes an optimization, but isn't required. Seems an appropriate hack > to me. Well, I had the thing actually written that way, but it breaks some of the test-suite. Whether the test-suite actually *should* test for what it tests for is obviously debatable, but it does. It does test that when you do git config --get novalue.variable you are expected to get an empty result. Is that good? Probably not. But it's what you get traditionally, and it's what the tests actually test for (t/t1300-repo-config.sh in case you care). But yes, I actually think it might be an improvement and have that thing return "true" (which is what happens if you make the 'config_true' array contain that string). And that allows removal of one test from the "git_config_bool()" function, but on the other hand, it does result in some strange stuff too.. In particular, *if* somebody just takes a string blindly and uses result = xstrdup(value); then with my patch it would then use an empty string for whatever it happened to pick. So having something like [user] name will mean that your name is empty (which will actually trigger an error if you try to commit). In contrast, if you do that 'config_true[] = "true"' thing, then that config file entry will make your name be the string "true", which is just _odd_. So I think an empty config_true actually has a nicer and more easily explainable failure case. It causes empty strings when mis-used, not arbitrary and strange "true" strings. Whatever. Linus - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
