{"thread":{"id":"18201","subject":"[PATCH 0/3] config --bool-or-int fixes","startedAt":"2009-03-07T17:14:03Z","lastAt":"2009-03-10T17:59:04Z","messageCount":9,"participants":["Jeff King","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"107311","messageId":"1236446046-18319-1-git-send-email-peff@peff.net","threadId":"18201","inReplyTo":null,"subject":"[PATCH 0/3] config --bool-or-int fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-07T17:14:03Z","receivedAt":"2009-03-07T17:14:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The interesting one is 2/3, which fixes a bogus usage message due to the\nrecent parseopt conversion. The others are cleanups that I noticed while\nfixing it.\n\n[PATCH 1/3] t1300: use test_must_fail as appropriate\n[PATCH 2/3] config: set help text for --bool-or-int\n[PATCH 3/3] document config --bool-or-int\n"},{"id":"107312","messageId":"1236446046-18319-2-git-send-email-peff@peff.net","threadId":"18201","inReplyTo":"1236446046-18319-1-git-send-email-peff@peff.net","subject":"[PATCH 1/3] t1300: use test_must_fail as appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-07T17:14:04Z","receivedAt":"2009-03-07T17:14:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some of the tests checked the exit code manually, even going\nso far as to run git outside of the test_expect harness.\n\nSigned-off-by: Jeff King \n---\nJust a cleanup I noticed when one of the tests failed for me.\n\n t/t1300-repo-config.sh | 10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 11b82f4..3c06842 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -336,10 +336,10 @@ test_expect_success 'get bool variable with empty value' \\\n \t'git config --bool emptyvalue.variable > output &&\n \t cmp output expect'\n \n-git config > output 2>&1\n-\n-test_expect_success 'no arguments, but no crash' \\\n-\t\"test $? = 129 && grep usage output\"\n+test_expect_success 'no arguments, but no crash' '\n+\ttest_must_fail git config >output 2>&1 &&\n+\tgrep usage output\n+'\n \n cat > .git/config << EOF\n [a.b]\n@@ -373,7 +373,7 @@ EOF\n test_expect_success 'new variable inserts into proper section' 'cmp .git/config expect'\n \n test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' \\\n-\t'git config --file non-existing-config -l; test $? != 0'\n+\t'test_must_fail git config --file non-existing-config -l'\n \n cat > other-config << EOF\n [ein]\n-- \n1.6.2.195.g44251.dirty\n"},{"id":"107313","messageId":"1236446046-18319-3-git-send-email-peff@peff.net","threadId":"18201","inReplyTo":"1236446046-18319-1-git-send-email-peff@peff.net","subject":"[PATCH 2/3] config: set help text for --bool-or-int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-07T17:14:05Z","receivedAt":"2009-03-07T17:14:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The conversion to parse_opt left this as NULL; on glibc\nsystems, the usage message prints\n\n --bool-or-int (null)\n\nand on other ones, segfaults.\n\nSigned-off-by: Jeff King \n---\nI found this by running t1300 on a non-glibc system. I guess printing\n(null) is friendlier for real code, but for the test suite, it would be\nmuch more convenient to have it segfault to detect the bug. This is not\nthe first such error we've had that I've caught in the same way.\n\n builtin-config.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex b11a096..1a3baa1 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -68,7 +68,7 @@ static struct option builtin_config_options[] = {\n \tOPT_GROUP(\"Type\"),\n \tOPT_BIT(0, \"bool\", &types, \"value is \\\"true\\\" or \\\"false\\\"\", TYPE_BOOL),\n \tOPT_BIT(0, \"int\", &types, \"value is decimal number\", TYPE_INT),\n-\tOPT_BIT(0, \"bool-or-int\", &types, NULL, TYPE_BOOL_OR_INT),\n+\tOPT_BIT(0, \"bool-or-int\", &types, \"value is --bool or --int\", TYPE_BOOL_OR_INT),\n \tOPT_GROUP(\"Other\"),\n \tOPT_BOOLEAN('z', \"null\", &end_null, \"terminate values with NUL byte\"),\n \tOPT_END(),\n-- \n1.6.2.195.g44251.dirty\n"},{"id":"107314","messageId":"1236446046-18319-4-git-send-email-peff@peff.net","threadId":"18201","inReplyTo":"1236446046-18319-1-git-send-email-peff@peff.net","subject":"[PATCH 3/3] document config --bool-or-int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-07T17:14:06Z","receivedAt":"2009-03-07T17:14:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The documentation is just a pointer to the --bool and --int\noptions, but it makes sense to at least mention that it\nexists.\n\nSigned-off-by: Jeff King \n---\nMaybe it is pointless to document this; it seems to have been added by\nc35b0b5 purely as a way for t1300 to test the git_config_bool_or_int\nfeature.\n\nIn that case, 2/3 should probably be hiding it instead of describing it,\nas well.\n\n Documentation/git-config.txt | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 7d14007..82ce89e 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -131,6 +131,10 @@ See also <>.\n \tin the config file will cause the value to be multiplied\n \tby 1024, 1048576, or 1073741824 prior to output.\n \n+--bool-or-int::\n+\t'git-config' will ensure that the output matches the format of\n+\teither --bool or --int, as described above.\n+\n -z::\n --null::\n \tFor all options that output values and/or keys, always\n-- \n1.6.2.195.g44251.dirty\n"},{"id":"107315","messageId":"20090307171640.GA18398@coredump.intra.peff.net","threadId":"18201","inReplyTo":"1236446046-18319-1-git-send-email-peff@peff.net","subject":"Re: [PATCH 0/3] config --bool-or-int fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-07T17:16:40Z","receivedAt":"2009-03-07T17:16:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 07, 2009 at 12:14:03PM -0500, Jeff King wrote:\n\n> The interesting one is 2/3, which fixes a bogus usage message due to the\n> recent parseopt conversion. The others are cleanups that I noticed while\n> fixing it.\n> \n> [PATCH 1/3] t1300: use test_must_fail as appropriate\n> [PATCH 2/3] config: set help text for --bool-or-int\n> [PATCH 3/3] document config --bool-or-int\n\nI failed to mention, but it may have been obvious: these should go on\ntop of the fc/parseopt-config topic in next.\n\n-Peff\n"},{"id":"107346","messageId":"94a0d4530903071307p46092810rb1637bfc853ee4d1@mail.gmail.com","threadId":"18201","inReplyTo":"1236446046-18319-3-git-send-email-peff@peff.net","subject":"Re: [PATCH 2/3] config: set help text for --bool-or-int","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-03-07T21:07:46Z","receivedAt":"2009-03-07T21:07:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Mar 7, 2009 at 7:14 PM, Jeff King wrote:\n> The conversion to parse_opt left this as NULL; on glibc\n> systems, the usage message prints\n>\n>   --bool-or-int   (null)\n>\n> and on other ones, segfaults.\n\nShouldn't then OPT_BIT make sure there is no crash?\n\nI was surprised when it didn't complain. I thought on making it \"\" but\nI wanted to make it visible that there was no documentation for that,\nwhich is the reason I left it that way.\n\n-- \nFelipe Contreras\n"},{"id":"107353","messageId":"20090307224807.GA18548@coredump.intra.peff.net","threadId":"18201","inReplyTo":"94a0d4530903071307p46092810rb1637bfc853ee4d1@mail.gmail.com","subject":"Re: [PATCH 2/3] config: set help text for --bool-or-int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-07T22:48:07Z","receivedAt":"2009-03-07T22:48:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 07, 2009 at 11:07:46PM +0200, Felipe Contreras wrote:\n\n> On Sat, Mar 7, 2009 at 7:14 PM, Jeff King wrote:\n> > The conversion to parse_opt left this as NULL; on glibc\n> > systems, the usage message prints\n> >\n> >   --bool-or-int   (null)\n> >\n> > and on other ones, segfaults.\n> \n> Shouldn't then OPT_BIT make sure there is no crash?\n\nPerhaps, but it doesn't (and I assume you mean usage_with_help, as\nOPT_BIT is just filling in the struct). It's not clear what a NULL help\nparameter should do, though. Hide the option? Show no help description?\nThere are already ways to accomplish both of those.\n\n> I was surprised when it didn't complain. I thought on making it \"\" but\n> I wanted to make it visible that there was no documentation for that,\n> which is the reason I left it that way.\n\nOK. I think there are really valid options:\n\n 1. it's there with a description (which is what my patch does)\n\n 2. it's there without a description, because it's obvious what it does\n coming after --bool and --int\n\n 3. it's hidden\n\nI really don't care which. But what is there now is broken.\n\n-Peff\n"},{"id":"107509","messageId":"94a0d4530903091450gdcad625g95cd9550cfb724fa@mail.gmail.com","threadId":"18201","inReplyTo":"20090307224807.GA18548@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] config: set help text for --bool-or-int","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-03-09T21:50:09Z","receivedAt":"2009-03-09T21:50:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Mar 8, 2009 at 12:48 AM, Jeff King wrote:\n> On Sat, Mar 07, 2009 at 11:07:46PM +0200, Felipe Contreras wrote:\n>\n>> On Sat, Mar 7, 2009 at 7:14 PM, Jeff King wrote:\n>> > The conversion to parse_opt left this as NULL; on glibc\n>> > systems, the usage message prints\n>> >\n>> >   --bool-or-int   (null)\n>> >\n>> > and on other ones, segfaults.\n>>\n>> Shouldn't then OPT_BIT make sure there is no crash?\n>\n> Perhaps, but it doesn't (and I assume you mean usage_with_help, as\n> OPT_BIT is just filling in the struct). It's not clear what a NULL help\n> parameter should do, though. Hide the option? Show no help description?\n> There are already ways to accomplish both of those.\n\nYeah, I meant usage_with_help. I don't know what should be done, but I\nthink two things should be achieved:\n\na) don't crash\nb) encourage the options to always have a description\n\nPerhaps not showing the option at all, or perhaps showing \"**EMPTY**\".\n\n>> I was surprised when it didn't complain. I thought on making it \"\" but\n>> I wanted to make it visible that there was no documentation for that,\n>> which is the reason I left it that way.\n>\n> OK. I think there are really valid options:\n>\n>  1. it's there with a description (which is what my patch does)\n>\n>  2. it's there without a description, because it's obvious what it does\n>     coming after --bool and --int\n\nI don't think it's obvious, that partly why I didn't fill the description.\n\n>  3. it's hidden\n>\n> I really don't care which. But what is there now is broken.\n\nDefinitely, your patch must be applied ASAP.\n\nMinor nitpick: \"value is interpreted either as bool or int\"\n\nThe value is what it is, the --boo-or-int option doesn't change the\nvalue, just how it is interpreted.\n\n-- \nFelipe Contreras\n"},{"id":"107598","messageId":"20090310175904.GB26351@sigill.intra.peff.net","threadId":"18201","inReplyTo":"94a0d4530903091450gdcad625g95cd9550cfb724fa@mail.gmail.com","subject":"Re: [PATCH 2/3] config: set help text for --bool-or-int","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-03-10T17:59:04Z","receivedAt":"2009-03-10T17:59:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 09, 2009 at 11:50:09PM +0200, Felipe Contreras wrote:\n\n> Yeah, I meant usage_with_help. I don't know what should be done, but I\n> think two things should be achieved:\n> \n> a) don't crash\n> b) encourage the options to always have a description\n\nI don't know of a good way to check at compile time that the struct\nmember is non-NULL. So I think we are stuck with parse_options dying, or\ndoing something to cover it up, like:\n\n> Perhaps not showing the option at all, or perhaps showing \"**EMPTY**\".\n\nBut the problem there is that if you _did_ mean for it to be shown, such\na bug can easily escape notice. Tests will generally still pass, so\nunless you are scanning the output manually, nothing will tell you what\nhappened.\n\nI think our best bet is to simply try to catch such things in code\nreview. This one slipped through, but it was still caught in 'next',\nmost because it _did_ crash on a non-glibc platform.\n\n> Minor nitpick: \"value is interpreted either as bool or int\"\n> \n> The value is what it is, the --boo-or-int option doesn't change the\n> value, just how it is interpreted.\n\nFair enough. My patch is is in 'next' now, so please go ahead and submit\na new patch with your suggested change. You might want to change --bool\nand --int at the same time, though, as they use the same wording.\n\n-Peff\n"}]}