[PATCH 0/3] config --bool-or-int fixes

[PATCH 0/3] config --bool-or-int fixes

[Jeff King]

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

[PATCH 1/3] t1300: use test_must_fail as appropriate

[Jeff King]

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

[PATCH 2/3] config: set help text for --bool-or-int

[Jeff King]

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

Re: [PATCH 2/3] config: set help text for --bool-or-int

[Felipe Contreras]

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

Re: [PATCH 2/3] config: set help text for --bool-or-int

[Jeff King]

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 --int

  3. it's hidden

I really don't care which. But what is there now is broken.

-Peff

Re: [PATCH 2/3] config: set help text for --bool-or-int

[Felipe Contreras]

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

Re: [PATCH 2/3] config: set help text for --bool-or-int

[Jeff King]

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

[PATCH 3/3] document config --bool-or-int

[Jeff King]

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

Re: [PATCH 0/3] config --bool-or-int fixes

[Jeff King]

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