threads / patch / 18201
Subject: [PATCH 0/3] config --bool-or-int fixes
Jeff KingThe interesting one is 2/3, which fixes a bogus usage message due to the recent parseopt conversion. The others are cleanups that I noticed while fixing it.
[PATCH 1/3] t1300: use test_must_fail as appropriate [PATCH 2/3] config: set help text for --bool-or-int [PATCH 3/3] document config --bool-or-int
Jeff King[PATCH 1/3] t1300: use test_must_fail as appropriate
Some of the tests checked the exit code manually, even going so far as to run git outside of the test_expect harness.
Signed-off-by: Jeff King <peff@peff.net> --- Just a cleanup I noticed when one of the tests failed for me.
t/t1300-repo-config.sh | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 11b82f4..3c06842 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -336,10 +336,10 @@ test_expect_success 'get bool variable with empty value' \ 'git config --bool emptyvalue.variable > output && cmp output expect' -git config > output 2>&1 - -test_expect_success 'no arguments, but no crash' \ - "test $? = 129 && grep usage output" +test_expect_success 'no arguments, but no crash' ' + test_must_fail git config >output 2>&1 && + grep usage output +' cat > .git/config << EOF [a.b] @@ -373,7 +373,7 @@ EOF test_expect_success 'new variable inserts into proper section' 'cmp .git/config expect' test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' \ - 'git config --file non-existing-config -l; test $? != 0' + 'test_must_fail git config --file non-existing-config -l' cat > other-config << EOF [ein]
-- 1.6.2.195.g44251.dirty
Jeff King[PATCH 2/3] config: set help text for --bool-or-int
The conversion to parse_opt left this as NULL; on glibc systems, the usage message prints
--bool-or-int (null)
and on other ones, segfaults.
Signed-off-by: Jeff King <peff@peff.net> --- I found this by running t1300 on a non-glibc system. I guess printing (null) is friendlier for real code, but for the test suite, it would be much more convenient to have it segfault to detect the bug. This is not the first such error we've had that I've caught in the same way.
builtin-config.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-config.c b/builtin-config.c index b11a096..1a3baa1 100644 --- a/builtin-config.c +++ b/builtin-config.c @@ -68,7 +68,7 @@ static struct option builtin_config_options[] = { OPT_GROUP("Type"), OPT_BIT(0, "bool", &types, "value is \"true\" or \"false\"", TYPE_BOOL), OPT_BIT(0, "int", &types, "value is decimal number", TYPE_INT), - OPT_BIT(0, "bool-or-int", &types, NULL, TYPE_BOOL_OR_INT), + OPT_BIT(0, "bool-or-int", &types, "value is --bool or --int", TYPE_BOOL_OR_INT), OPT_GROUP("Other"), OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"), OPT_END(),
-- 1.6.2.195.g44251.dirty
Felipe ContrerasRe: [PATCH 2/3] config: set help text for --bool-or-int
On Sat, Mar 7, 2009 at 7:14 PM, Jeff King <peff@peff.net> wrote:
> The conversion to parse_opt left this as NULL; on glibc > systems, the usage message prints > > --bool-or-int (null) > > and on other ones, segfaults.
Shouldn't then OPT_BIT make sure there is no crash?
I was surprised when it didn't complain. I thought on making it "" but I wanted to make it visible that there was no documentation for that, which is the reason I left it that way.
-- Felipe Contreras
Jeff KingRe: [PATCH 2/3] config: set help text for --bool-or-int
On Sat, Mar 07, 2009 at 11:07:46PM +0200, Felipe Contreras wrote:
> On Sat, Mar 7, 2009 at 7:14 PM, Jeff King <peff@peff.net> wrote: > > The conversion to parse_opt left this as NULL; on glibc > > systems, the usage message prints > > > > --bool-or-int (null) > > > > and on other ones, segfaults. > > Shouldn't then OPT_BIT make sure there is no crash?
Perhaps, but it doesn't (and I assume you mean usage_with_help, as OPT_BIT is just filling in the struct). It's not clear what a NULL help parameter should do, though. Hide the option? Show no help description? There are already ways to accomplish both of those.
> I was surprised when it didn't complain. I thought on making it "" but > I wanted to make it visible that there was no documentation for that, > which is the reason I left it that way.
OK. I think there are really valid options:
1. it's there with a description (which is what my patch does)
2. it's there without a description, because it's obvious what it does
coming after --bool and --int3. it's hidden
I really don't care which. But what is there now is broken.
-Peff
Felipe ContrerasRe: [PATCH 2/3] config: set help text for --bool-or-int
On Sun, Mar 8, 2009 at 12:48 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Mar 07, 2009 at 11:07:46PM +0200, Felipe Contreras wrote: > >> On Sat, Mar 7, 2009 at 7:14 PM, Jeff King <peff@peff.net> wrote: >> > The conversion to parse_opt left this as NULL; on glibc >> > systems, the usage message prints >> > >> > --bool-or-int (null) >> > >> > and on other ones, segfaults. >> >> Shouldn't then OPT_BIT make sure there is no crash? > > Perhaps, but it doesn't (and I assume you mean usage_with_help, as > OPT_BIT is just filling in the struct). It's not clear what a NULL help > parameter should do, though. Hide the option? Show no help description? > There are already ways to accomplish both of those.
Yeah, I meant usage_with_help. I don't know what should be done, but I think two things should be achieved:
a) don't crash b) encourage the options to always have a description
Perhaps not showing the option at all, or perhaps showing "**EMPTY**".
>> I was surprised when it didn't complain. I thought on making it "" but >> I wanted to make it visible that there was no documentation for that, >> which is the reason I left it that way. > > OK. I think there are really valid options: > > 1. it's there with a description (which is what my patch does) > > 2. it's there without a description, because it's obvious what it does > coming after --bool and --int
I don't think it's obvious, that partly why I didn't fill the description.
> 3. it's hidden > > I really don't care which. But what is there now is broken.
Definitely, your patch must be applied ASAP.
Minor nitpick: "value is interpreted either as bool or int"
The value is what it is, the --boo-or-int option doesn't change the value, just how it is interpreted.
-- Felipe Contreras
Jeff KingRe: [PATCH 2/3] config: set help text for --bool-or-int
On Mon, Mar 09, 2009 at 11:50:09PM +0200, Felipe Contreras wrote:
> Yeah, I meant usage_with_help. I don't know what should be done, but I > think two things should be achieved: > > a) don't crash > b) encourage the options to always have a description
I don't know of a good way to check at compile time that the struct member is non-NULL. So I think we are stuck with parse_options dying, or doing something to cover it up, like:
> Perhaps not showing the option at all, or perhaps showing "**EMPTY**".
But the problem there is that if you _did_ mean for it to be shown, such a bug can easily escape notice. Tests will generally still pass, so unless you are scanning the output manually, nothing will tell you what happened.
I think our best bet is to simply try to catch such things in code review. This one slipped through, but it was still caught in 'next', most because it _did_ crash on a non-glibc platform.
> Minor nitpick: "value is interpreted either as bool or int" > > The value is what it is, the --boo-or-int option doesn't change the > value, just how it is interpreted.
Fair enough. My patch is is in 'next' now, so please go ahead and submit a new patch with your suggested change. You might want to change --bool and --int at the same time, though, as they use the same wording.
-Peff
Jeff King[PATCH 3/3] document config --bool-or-int
The documentation is just a pointer to the --bool and --int options, but it makes sense to at least mention that it exists.
Signed-off-by: Jeff King <peff@peff.net> --- Maybe it is pointless to document this; it seems to have been added by c35b0b5 purely as a way for t1300 to test the git_config_bool_or_int feature.
In that case, 2/3 should probably be hiding it instead of describing it, as well.
Documentation/git-config.txt | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 7d14007..82ce89e 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -131,6 +131,10 @@ See also <<FILES>>. in the config file will cause the value to be multiplied by 1024, 1048576, or 1073741824 prior to output. +--bool-or-int:: + 'git-config' will ensure that the output matches the format of + either --bool or --int, as described above. + -z:: --null:: For all options that output values and/or keys, always
-- 1.6.2.195.g44251.dirty
Jeff KingRe: [PATCH 0/3] config --bool-or-int fixes
On Sat, Mar 07, 2009 at 12:14:03PM -0500, Jeff King wrote:
> The interesting one is 2/3, which fixes a bogus usage message due to the > recent parseopt conversion. The others are cleanups that I noticed while > fixing it. > > [PATCH 1/3] t1300: use test_must_fail as appropriate > [PATCH 2/3] config: set help text for --bool-or-int > [PATCH 3/3] document config --bool-or-int
I failed to mention, but it may have been obvious: these should go on top of the fc/parseopt-config topic in next.
-Peff