threads / patch / 42007

v14, 6 partstest-parse-options: print quiet as integer

Subject: [PATCH v14 2/6] test-parse-options: print quiet as integer

## tl;dr

22 messages between Apr 12, 2016 and Apr 27, 2016. Diffs are folded; open one to read it.

replies: 21people: 2as markdown or json

Pranit Bauva· Apr 12, 2016, 23:02 UTC · lore

[PATCH v14 1/6] t0040-test-parse-options.sh: fix style issues

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
Changes wrt previous version (v12):
 - Use '\' when interpolation isn't required
---
 t/t0040-parse-options.sh | 76 ++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 38 deletions(-)
Show changes to t/t0040-parse-options.sh +38 −39
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 9be6411..477fcff 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -7,7 +7,7 @@ test_description='our own option parser'
 
 . ./test-lib.sh
 
-cat > expect << EOF
+cat >expect <<\EOF
 usage: test-parse-options <options>
 
     --yes                 get a boolean
@@ -49,14 +49,14 @@ Standard options
 EOF
 
 test_expect_success 'test help' '
-	test_must_fail test-parse-options -h > output 2> output.err &&
+	test_must_fail test-parse-options -h >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_i18ncmp expect output
 '
 
 mv expect expect.err
 
-cat >expect.template <<EOF
+cat >expect.template <<\EOF
 boolean: 0
 integer: 0
 magnitude: 0
@@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '
 	check magnitude: 3221225472 -m 3g
 '
 
-cat > expect << EOF
+cat >expect <<\EOF
 boolean: 2
 integer: 1729
 magnitude: 16384
@@ -176,7 +176,7 @@ test_expect_success 'short options' '
 	test_must_be_empty output.err
 '
 
-cat > expect << EOF
+cat >expect <<\EOF
 boolean: 2
 integer: 1729
 magnitude: 16384
@@ -204,7 +204,7 @@ test_expect_success 'missing required value' '
 	test_expect_code 129 test-parse-options --file
 '
 
-cat > expect << EOF
+cat >expect <<\EOF
 boolean: 1
 integer: 13
 magnitude: 0
@@ -222,12 +222,12 @@ EOF
 
 test_expect_success 'intermingled arguments' '
 	test-parse-options a1 --string 123 b1 --boolean -j 13 -- --boolean \
-		> output 2> output.err &&
+		>output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
-cat > expect << EOF
+cat >expect <<\EOF
 boolean: 0
 integer: 2
 magnitude: 0
@@ -241,13 +241,13 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
-	test-parse-options --int 2 --boolean --no-bo > output 2> output.err &&
+	test-parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
-	test-parse-options --int=2 > output 2> output.err &&
+	test-parse-options --int=2 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
@@ -256,7 +256,7 @@ test_expect_success 'ambiguously abbreviated option' '
 	test_expect_code 129 test-parse-options --strin 123
 '
 
-cat > expect << EOF
+cat >expect <<\EOF
 boolean: 0
 integer: 0
 magnitude: 0
@@ -270,32 +270,32 @@ file: (not set)
 EOF
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
-	test-parse-options --st 123 > output 2> output.err &&
+	test-parse-options --st 123 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
-cat > typo.err << EOF
-error: did you mean \`--boolean\` (with two dashes ?)
+cat >typo.err <<\EOF
+error: did you mean `--boolean` (with two dashes ?)
 EOF
 
 test_expect_success 'detect possible typos' '
-	test_must_fail test-parse-options -boolean > output 2> output.err &&
+	test_must_fail test-parse-options -boolean >output 2>output.err &&
 	test_must_be_empty output &&
 	test_cmp typo.err output.err
 '
 
-cat > typo.err << EOF
-error: did you mean \`--ambiguous\` (with two dashes ?)
+cat >typo.err <<\EOF
+error: did you mean `--ambiguous` (with two dashes ?)
 EOF
 
 test_expect_success 'detect possible typos' '
-	test_must_fail test-parse-options -ambiguous > output 2> output.err &&
+	test_must_fail test-parse-options -ambiguous >output 2>output.err &&
 	test_must_be_empty output &&
 	test_cmp typo.err output.err
 '
 
-cat > expect <<EOF
+cat >expect <<\EOF
 boolean: 0
 integer: 0
 magnitude: 0
@@ -310,12 +310,12 @@ arg 00: --quux
 EOF
 
 test_expect_success 'keep some options as arguments' '
-	test-parse-options --quux > output 2> output.err &&
+	test-parse-options --quux >output 2>output.err &&
 	test_must_be_empty output.err &&
-        test_cmp expect output
+	test_cmp expect output
 '
 
-cat > expect <<EOF
+cat >expect <<\EOF
 boolean: 0
 integer: 0
 magnitude: 0
@@ -331,12 +331,12 @@ EOF
 
 test_expect_success 'OPT_DATE() works' '
 	test-parse-options -t "1970-01-01 00:00:01 +0000" \
-		foo -q > output 2> output.err &&
+		foo -q >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
-cat > expect <<EOF
+cat >expect <<\EOF
 Callback: "four", 0
 boolean: 5
 integer: 4
@@ -351,22 +351,22 @@ file: (not set)
 EOF
 
 test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
-	test-parse-options --length=four -b -4 > output 2> output.err &&
+	test-parse-options --length=four -b -4 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
-cat > expect <<EOF
+cat >expect <<\EOF
 Callback: "not set", 1
 EOF
 
 test_expect_success 'OPT_CALLBACK() and callback errors work' '
-	test_must_fail test-parse-options --no-length > output 2> output.err &&
+	test_must_fail test-parse-options --no-length >output 2>output.err &&
 	test_i18ncmp expect output &&
 	test_i18ncmp expect.err output.err
 '
 
-cat > expect <<EOF
+cat >expect <<\EOF
 boolean: 1
 integer: 23
 magnitude: 0
@@ -380,18 +380,18 @@ file: (not set)
 EOF
 
 test_expect_success 'OPT_BIT() and OPT_SET_INT() work' '
-	test-parse-options --set23 -bbbbb --no-or4 > output 2> output.err &&
+	test-parse-options --set23 -bbbbb --no-or4 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '
-	test-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err &&
+	test-parse-options --set23 -bbbbb --neg-or4 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
-cat > expect <<EOF
+cat >expect <<\EOF
 boolean: 6
 integer: 0
 magnitude: 0
@@ -405,24 +405,24 @@ file: (not set)
 EOF
 
 test_expect_success 'OPT_BIT() works' '
-	test-parse-options -bb --or4 > output 2> output.err &&
+	test-parse-options -bb --or4 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'OPT_NEGBIT() works' '
-	test-parse-options -bb --no-neg-or4 > output 2> output.err &&
+	test-parse-options -bb --no-neg-or4 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '
-	test-parse-options + + + + + + > output 2> output.err &&
+	test-parse-options + + + + + + >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
-cat > expect <<EOF
+cat >expect <<\EOF
 boolean: 0
 integer: 12345
 magnitude: 0
@@ -436,12 +436,12 @@ file: (not set)
 EOF
 
 test_expect_success 'OPT_NUMBER_CALLBACK() works' '
-	test-parse-options -12345 > output 2> output.err &&
+	test-parse-options -12345 >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
-cat >expect <<EOF
+cat >expect <<\EOF
 boolean: 0
 integer: 0
 magnitude: 0
@@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
 	test_cmp expect output
 '
 
-cat >>expect <<'EOF'
+cat >>expect <<\EOF
 list: foo
 list: bar
 list: baz

--
https://github.com/git/git/pull/218
Pranit Bauva· Apr 12, 2016, 23:02 UTC · re: Pranit Bauva · lore

We would want to see how multiple --quiet options affect the value of the underlying variable (we may want "--quiet --quiet" to still be 1, or we may want to see the value incremented to 2). Show the value as integer to allow us to inspect it.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t0040-parse-options.sh | 26 +++++++++++++-------------
 test-parse-options.c     |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)
Show changes to 2 files +14 −15

t/t0040-parse-options.sh, test-parse-options.c

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 477fcff..450da45 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -64,7 +64,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
@@ -164,7 +164,7 @@ timestamp: 0
 string: 123
 abbrev: 7
 verbose: 2
-quiet: no
+quiet: 0
 dry run: yes
 file: prefix/my.file
 EOF
@@ -184,7 +184,7 @@ timestamp: 0
 string: 321
 abbrev: 10
 verbose: 2
-quiet: no
+quiet: 0
 dry run: no
 file: prefix/fi.le
 EOF
@@ -212,7 +212,7 @@ timestamp: 0
 string: 123
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 arg 00: a1
@@ -235,7 +235,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
@@ -264,7 +264,7 @@ timestamp: 0
 string: 123
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
@@ -303,7 +303,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 arg 00: --quux
@@ -323,7 +323,7 @@ timestamp: 1
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: yes
+quiet: 1
 dry run: no
 file: (not set)
 arg 00: foo
@@ -345,7 +345,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
@@ -374,7 +374,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
@@ -399,7 +399,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
@@ -430,7 +430,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
@@ -449,7 +449,7 @@ timestamp: 0
 string: (not set)
 abbrev: 7
 verbose: 0
-quiet: no
+quiet: 0
 dry run: no
 file: (not set)
 EOF
diff --git a/test-parse-options.c b/test-parse-options.c
index 2c8c8f1..86afa98 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -90,7 +90,7 @@ int main(int argc, char **argv)
 	printf("string: %s\n", string ? string : "(not set)");
 	printf("abbrev: %d\n", abbrev);
 	printf("verbose: %d\n", verbose);
-	printf("quiet: %s\n", quiet ? "yes" : "no");
+	printf("quiet: %d\n", quiet);
 	printf("dry run: %s\n", dry_run ? "yes" : "no");
 	printf("file: %s\n", file ? file : "(not set)");
 

--
https://github.com/git/git/pull/218
Pranit Bauva· Apr 12, 2016, 23:02 UTC · re: Pranit Bauva · lore

[PATCH v14 3/6] t0040-parse-options: improve test coverage

Include tests to check for multiple levels of quiet and to check if the '--no-quiet' option sets it to 0.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t0040-parse-options.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
Show changes to t/t0040-parse-options.sh +37 −1
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 450da45..c913d1c 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -476,4 +476,41 @@ test_expect_success '--no-list resets list' '
 	test_cmp expect output
 '
 
+cat >expect <<\EOF
+boolean: 0
+integer: 0
+magnitude: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: 0
+quiet: 3
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success 'multiple quiet levels' '
+	test-parse-options -q -q -q >output 2>output.err &&
+	test_must_be_empty output.err &&
+	test_cmp expect output
+'
+
+cat >expect <<\EOF
+boolean: 0
+integer: 0
+magnitude: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: 0
+quiet: 0
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success '--no-quiet sets quiet to 0' '
+	test-parse-options --no-quiet >output 2>output.err &&
+	test_must_be_empty output.err &&
+	test_cmp expect output
+'
 test_done

--
https://github.com/git/git/pull/218
Eric Sunshine· Apr 13, 2016, 05:26 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage

On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 22 quoted lines
> Include tests to check for multiple levels of quiet and to check if the
> '--no-quiet' option sets it to 0.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> @@ -476,4 +476,41 @@ test_expect_success '--no-list resets list' '
> +cat >expect <<\EOF
> +boolean: 0
> +integer: 0
> +magnitude: 0
> +timestamp: 0
> +string: (not set)
> +abbrev: 7
> +verbose: 0
> +quiet: 0
> +dry run: no
> +file: (not set)
> +EOF
> +
> +test_expect_success '--no-quiet sets quiet to 0' '
> +       test-parse-options --no-quiet >output 2>output.err &&

Meh, as implemented, this isn't a very interesting test, is it? 'quiet' started at 0, so all this shows is that --no-quiet didn't disturb the 0. To really test that it resets it to 0, you'd want:

    test-parse-options --quiet --no-quiet >... 2>... &&
> +       test_must_be_empty output.err &&
> +       test_cmp expect output
> +'
>  test_done
Pranit Bauva· Apr 13, 2016, 08:59 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage

On Wed, Apr 13, 2016 at 10:56 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 34 quoted lines
> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Include tests to check for multiple levels of quiet and to check if the
>> '--no-quiet' option sets it to 0.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
>> @@ -476,4 +476,41 @@ test_expect_success '--no-list resets list' '
>> +cat >expect <<\EOF
>> +boolean: 0
>> +integer: 0
>> +magnitude: 0
>> +timestamp: 0
>> +string: (not set)
>> +abbrev: 7
>> +verbose: 0
>> +quiet: 0
>> +dry run: no
>> +file: (not set)
>> +EOF
>> +
>> +test_expect_success '--no-quiet sets quiet to 0' '
>> +       test-parse-options --no-quiet >output 2>output.err &&
>
> Meh, as implemented, this isn't a very interesting test, is it?
> 'quiet' started at 0, so all this shows is that --no-quiet didn't
> disturb the 0. To really test that it resets it to 0, you'd want:
>
>     test-parse-options --quiet --no-quiet >... 2>... &&
>
>> +       test_must_be_empty output.err &&
>> +       test_cmp expect output
>> +'
>>  test_done

This is to test whether the 0 of quiet remains 0 if --no-quiet is included. This test "defines" the current behavior. Then when I change OPT_COUNTUP(), this test will ensure that this behavior is not interrupted as promised by the commit message of that patch[1]. I guess this also describe why I choose to include these tests between 2/5 and 3/5 rather than 3/5 and 4/5. And also see the extended discussion[2] for this. If I do a re-roll then I include `--quiet` before `--no-quiet`

[1]: http://article.gmane.org/gmane.comp.version-control.git/291313
Eric Sunshine· Apr 13, 2016, 17:27 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage

On Wed, Apr 13, 2016 at 4:59 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 24 quoted lines
> On Wed, Apr 13, 2016 at 10:56 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +test_expect_success '--no-quiet sets quiet to 0' '
>>> +       test-parse-options --no-quiet >output 2>output.err &&
>>
>> Meh, as implemented, this isn't a very interesting test, is it?
>> 'quiet' started at 0, so all this shows is that --no-quiet didn't
>> disturb the 0. To really test that it resets it to 0, you'd want:
>>
>>     test-parse-options --quiet --no-quiet >... 2>... &&
>>
>>> +       test_must_be_empty output.err &&
>>> +       test_cmp expect output
>>> +'
>>>  test_done
>
> This is to test whether the 0 of quiet remains 0 if --no-quiet is
> included. This test "defines" the current behavior. Then when I change
> OPT_COUNTUP(), this test will ensure that this behavior is not
> interrupted as promised by the commit message of that patch[1]. I
> guess this also describe why I choose to include these tests between
> 2/5 and 3/5 rather than 3/5 and 4/5. And also see the extended
> discussion[2] for this. If I do a re-roll then I include `--quiet`
> before `--no-quiet`

Each of these patches should have a single conceptual purpose. It seems, from the above explanation, that you're mixing and mismatching bits of such changes between patches.

* The two new tests for --no-verbose and --no-quiet should be together
and check that they correctly reverse --verbose and --quiet,
respectively.
* The test you describe above which ensures that --no-quiet leaves
'quiet' at 0 should be bundled with the change that might break that
behavior, namely, the OPT__COUNTUP() change.
Pranit Bauva· Apr 25, 2016, 18:40 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage

On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 37 quoted lines
> On Wed, Apr 13, 2016 at 4:59 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Apr 13, 2016 at 10:56 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> +test_expect_success '--no-quiet sets quiet to 0' '
>>>> +       test-parse-options --no-quiet >output 2>output.err &&
>>>
>>> Meh, as implemented, this isn't a very interesting test, is it?
>>> 'quiet' started at 0, so all this shows is that --no-quiet didn't
>>> disturb the 0. To really test that it resets it to 0, you'd want:
>>>
>>>     test-parse-options --quiet --no-quiet >... 2>... &&
>>>
>>>> +       test_must_be_empty output.err &&
>>>> +       test_cmp expect output
>>>> +'
>>>>  test_done
>>
>> This is to test whether the 0 of quiet remains 0 if --no-quiet is
>> included. This test "defines" the current behavior. Then when I change
>> OPT_COUNTUP(), this test will ensure that this behavior is not
>> interrupted as promised by the commit message of that patch[1]. I
>> guess this also describe why I choose to include these tests between
>> 2/5 and 3/5 rather than 3/5 and 4/5. And also see the extended
>> discussion[2] for this. If I do a re-roll then I include `--quiet`
>> before `--no-quiet`
>
> Each of these patches should have a single conceptual purpose. It
> seems, from the above explanation, that you're mixing and mismatching
> bits of such changes between patches.
>
> * The two new tests for --no-verbose and --no-quiet should be together
> and check that they correctly reverse --verbose and --quiet,
> respectively.
>
> * The test you describe above which ensures that --no-quiet leaves
> 'quiet' at 0 should be bundled with the change that might break that
> behavior, namely, the OPT__COUNTUP() change.

I am planning to re-roll this. So, I am just confirming whether I understood properly.

 * I will add the tests for check for '-q --no-quiet' instead of just
'--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch
which improves test coverage which will be before the OPT_COUNTUP()
change.
 * I will then add the test for '--no-quiet' sets to 0 in the separate
patch after OPT_COUNTUP() change.
Is there something else or something different that you are suggesting?
Eric Sunshine· Apr 27, 2016, 17:55 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage

On Mon, Apr 25, 2016 at 2:40 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 20 quoted lines
> On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Each of these patches should have a single conceptual purpose. It
>> seems, from the above explanation, that you're mixing and mismatching
>> bits of such changes between patches.
>>
>> * The two new tests for --no-verbose and --no-quiet should be together
>> and check that they correctly reverse --verbose and --quiet,
>> respectively.
>>
>> * The test you describe above which ensures that --no-quiet leaves
>> 'quiet' at 0 should be bundled with the change that might break that
>> behavior, namely, the OPT__COUNTUP() change.
>
> I am planning to re-roll this.
> So, I am just confirming whether I understood properly.
>
>  * I will add the tests for check for '-q --no-quiet' instead of just
> '--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch
> which improves test coverage which will be before the OPT_COUNTUP()
> change.

These tests would be even a bit more interesting if you tested "-q -q --no-quiet" and "-v -v --no-verbose", respectively, to ensure that the "no" options actually reset to 0 rather than merely decrementing by 1.

I recall also suggesting adding a new test checking that "-q -q" increments the quiet count to 2 (which could be done in the patch which makes 'quiet' print as a number instead of a boolean or in the same "improve test coverage" patch).

>  * I will then add the test for '--no-quiet' sets to 0 in the separate
> patch after OPT_COUNTUP() change.

No, this and "--no-verbose sets to 0" are logically related to the OPT__COUNTUP() change, thus would be incorporated into that patch. Alternately, these two tests could just be part of "improve test coverage" patch, establishing base behavior which the OPT__COUNTUP() patch shouldn't break.

Pranit Bauva· Apr 27, 2016, 18:16 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage

On Wed, Apr 27, 2016 at 11:25 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 25 quoted lines
> On Mon, Apr 25, 2016 at 2:40 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> Each of these patches should have a single conceptual purpose. It
>>> seems, from the above explanation, that you're mixing and mismatching
>>> bits of such changes between patches.
>>>
>>> * The two new tests for --no-verbose and --no-quiet should be together
>>> and check that they correctly reverse --verbose and --quiet,
>>> respectively.
>>>
>>> * The test you describe above which ensures that --no-quiet leaves
>>> 'quiet' at 0 should be bundled with the change that might break that
>>> behavior, namely, the OPT__COUNTUP() change.
>>
>> I am planning to re-roll this.
>> So, I am just confirming whether I understood properly.
>>
>>  * I will add the tests for check for '-q --no-quiet' instead of just
>> '--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch
>> which improves test coverage which will be before the OPT_COUNTUP()
>> change.
>
> These tests would be even a bit more interesting if you tested "-q -q
> --no-quiet" and "-v -v --no-verbose", respectively, to ensure that the
> "no" options actually reset to 0 rather than merely decrementing by 1.
This seems a better choice.
> I recall also suggesting adding a new test checking that "-q -q"
> increments the quiet count to 2 (which could be done in the patch
> which makes 'quiet' print as a number instead of a boolean or in the
> same "improve test coverage" patch).
Will include this.
Show 8 quoted lines
>>  * I will then add the test for '--no-quiet' sets to 0 in the separate
>> patch after OPT_COUNTUP() change.
>
> No, this and "--no-verbose sets to 0" are logically related to the
> OPT__COUNTUP() change, thus would be incorporated into that patch.
> Alternately, these two tests could just be part of "improve test
> coverage" patch, establishing base behavior which the OPT__COUNTUP()
> patch shouldn't break.

I would prefer including it in "improve test coverage" patch to establish the base behavior. This seems more natural to me.

You can expect this series from me within 2 days.
Thanks.
Pranit Bauva· Apr 12, 2016, 23:02 UTC · re: Pranit Bauva · lore

[PATCH v14 6/6] commit: add a commit.verbose config variable

Add commit.verbose configuration variable as a convenience for those who always prefer --verbose.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
The previous version of the patch are:
 - [v12] $gmane/288820
 - [v11] $gmane/288820
 - [v10] $gmane/288820
 - [v9] $gmane/288820
 - [v8] $gmane/288820
 - [v7] $gmane/288820
 - [v6] $gmane/288728
 - [v5] $gmane/288728
 - [v4] $gmane/288652
 - [v3] $gmane/288634
 - [v2] $gmane/288569
 - [v1] $gmane/287540
   Note: One might think some tests are extra but I think that it will
   be better to include them as they "complete the continuity" thus
   generalising the series which will make the patch even more clearer.
Changes wrt v12:
 - Change the setup test as suggested by Eric.
 - Use for loops for systematic testing. I have avoided nested-for loops
   or using a function to test because I believe it will just add to the
   complexity rather than simplifying it. Just by using for loops the
   tests look quite systematic and easy to digest.
---
 Documentation/config.txt     |  4 ++++
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c             | 14 ++++++++++-
 t/t7507-commit-verbose.sh    | 56 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 2 deletions(-)
Show changes to 4 files +75 −3

Documentation/config.txt, Documentation/git-commit.txt, builtin/commit.c, t/t7507-commit-verbose.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..1d0ec2e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1110,6 +1110,10 @@ commit.template::
 	"`~/`" is expanded to the value of `$HOME` and "`~user/`" to the
 	specified user's home directory.
 
+commit.verbose::
+	A boolean or int to specify the level of verbose with `git commit`.
+	See linkgit:git-commit[1].
+
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 9ec6b3c..d474226 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].
 	what changes the commit has.
 	Note that this diff output doesn't have its
 	lines prefixed with '#'. This diff will not be a part
-	of the commit message.
+	of the commit message. See the `commit.verbose` configuration
+	variable in linkgit:git-config[1].
 +
 If specified twice, show in addition the unified diff between
 what would be committed and the worktree files, i.e. the unstaged
diff --git a/builtin/commit.c b/builtin/commit.c
index b3bd2d4..96e6190 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -113,7 +113,9 @@ static char *edit_message, *use_message;
 static char *fixup_message, *squash_message;
 static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
+static int config_verbose = -1; /* unspecified */
+static int verbose = -1; /* unspecified */
+static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
 static char *sign_commit;
@@ -1354,6 +1356,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			     builtin_status_usage, 0);
 	finalize_colopts(&s.colopts, -1);
 	finalize_deferred_config(&s);
+	if (verbose == -1)
+		verbose = 0;
 
 	handle_untracked_files_arg(&s);
 	if (show_ignored_in_status)
@@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
 	}
+	if (!strcmp(k, "commit.verbose")) {
+		int is_bool;
+		config_verbose = git_config_bool_or_int(k, v, &is_bool);
+		return 0;
+	}
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
@@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	argc = parse_and_validate_options(argc, argv, builtin_commit_options,
 					  builtin_commit_usage,
 					  prefix, current_head, &s);
+	if (verbose == -1)
+		verbose = (config_verbose < 0) ? 0 : config_verbose;
+
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 0f28a86..00e0c3d 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -98,4 +98,60 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
 	test_i18ngrep "Aborting commit due to empty commit message." err
 '
 
+test_expect_success 'setup -v -v' '
+	echo dirty >file
+'
+
+for i in true 1
+do
+	test_expect_success "commit.verbose=$i and --verbose omitted" "
+		git -c commit.verbose=$i commit --amend &&
+		test_line_count = 1 out
+	"
+done
+
+for i in false -2 -1 0
+do
+	test_expect_success "commit.verbose=$i and --verbose omitted" "
+		git -c commit.verbose=$i commit --amend &&
+		test_line_count = 0 out
+	"
+done
+
+for i in 2 3
+do
+	test_expect_success "commit.verbose=$i and --verbose omitted" "
+		git -c commit.verbose=$i commit --amend &&
+		test_line_count = 2 out
+	"
+done
+
+for i in true false -2 -1 0 1 2 3
+do
+	test_expect_success "commit.verbose=$i and --verbose" "
+		git -c commit.verbose=$i commit --amend --verbose &&
+		test_line_count = 1 out
+	"
+
+	test_expect_success "commit.verbose=$i and --no-verbose" "
+		git -c commit.verbose=$i commit --amend --no-verbose &&
+		test_line_count = 0 out
+	"
+
+	test_expect_success "commit.verbose=$i and -v -v" "
+		git -c commit.verbose=$i commit --amend -v -v &&
+		test_line_count = 2 out
+	"
+done
+
+test_expect_success 'status ignores commit.verbose=true' '
+	git -c commit.verbose=true status >actual &&
+	! grep "^diff --git" actual
+'
+
+test_expect_success 'status does not verbose without --verbose' '
+	git status >actual &&
+	! grep "^diff --git" actual
+'
+
 test_done

--
https://github.com/git/git/pull/218
Eric Sunshine· Apr 13, 2016, 06:14 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 6/6] commit: add a commit.verbose config variable

On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 13 quoted lines
> Add commit.verbose configuration variable as a convenience for those
> who always prefer --verbose.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> @@ -98,4 +98,60 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '
> +test_expect_success 'status ignores commit.verbose=true' '
> +       git -c commit.verbose=true status >actual &&
> +       ! grep "^diff --git" actual
> +'

I understand what this test is checking, as it is in response to Junio's suggestion[1]...

> +test_expect_success 'status does not verbose without --verbose' '
> +       git status >actual &&
> +       ! grep "^diff --git" actual
> +'
But what is this test checking?
> +
>  test_done
[1]: http://article.gmane.org/gmane.comp.version-control.git/288648
Pranit Bauva· Apr 13, 2016, 09:15 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 6/6] commit: add a commit.verbose config variable

On Wed, Apr 13, 2016 at 11:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 8 quoted lines
> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>
>> +test_expect_success 'status does not verbose without --verbose' '
>> +       git status >actual &&
>> +       ! grep "^diff --git" actual
>> +'
>
> But what is this test checking?

status is also a consumer of the verbose whose initial value is set to -1. This makes it include verbose in status output. This bug was fixed by explicitly initializing verbose to 0 if -1. SZEDER pointed out a bug[1] which broke some tests in and then when I fixed it, you requested me to include tests even in this patch[2] which I found convincing enough.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289730 [2]: http://article.gmane.org/gmane.comp.version-control.git/289993

Eric Sunshine· Apr 13, 2016, 17:44 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 6/6] commit: add a commit.verbose config variable

On Wed, Apr 13, 2016 at 5:15 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 18 quoted lines
> On Wed, Apr 13, 2016 at 11:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +test_expect_success 'status does not verbose without --verbose' '
>>> +       git status >actual &&
>>> +       ! grep "^diff --git" actual
>>> +'
>>
>> But what is this test checking?
>
> status is also a consumer of the verbose whose initial value is set to
> -1. This makes it include verbose in status output. This bug was fixed
> by explicitly initializing verbose to 0 if -1. SZEDER pointed out a
> bug[1] which broke some tests in and then when I fixed it, you
> requested me to include tests even in this patch[2] which I found
> convincing enough.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289730
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289993

Okay, makes sense, but it's not at all obvious from the context of this patch or its commit message. It probably would have been clearer had the two git-status tests been added in a separate preparatory test with a commit message explaining that the tests are to ensure that a subsequent patch (adding commit.verbose) won't break the existing behavior of git-status. Having a separate commit explaining that would also help future readers of the test script who wonder what this test is doing (since it's not obvious and it's not explained by the current commit message) when they use git-blame to try to figure out its purpose. If you do re-roll, you might consider breaking them out to a new patch or, at the very least, document their purpose in the commit message of this patch.

Pranit Bauva· Apr 12, 2016, 23:02 UTC · re: Pranit Bauva · lore

[PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

OPT_COUNTUP() merely increments the counter upon --option, and resets it to 0 upon --no-option, which means that there is no "unspecified" value with which a client can initialize the counter to determine whether or not --[no]-option was seen at all.

Make OPT_COUNTUP() treat any negative number as an "unspecified" value to address this shortcoming. In particular, if a client initializes the counter to -1, then if it is still -1 after parse_options(), then neither --option nor --no-option was seen; If it is 0, then --no-option was seen last, and if it is 1 or greater, than --option was seen last.

This change does not affect the behavior of existing clients because they all use the initial value of 0 (or more).

Note that builtin/clean.c initializes the variable used with OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set to either 0 or 1 by reading the configuration before the code calls parse_options(), i.e. as far as parse_options() is concerned, the initial value of the variable is not negative.

To test this behavior, in test-parse-options.c, "verbose" is set to "unspecified" while quiet is set to 0 which will test the new behavior with all sets of values.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

--- The discussion about this patch: [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027

Previous version of the patch: [v11] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v10] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v9] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061

Changes wrt previous version (v13):
 - Some tests failed in v12 as the newly introduced tests weren't
   updated by the behaviour of this patch.
Please Note: The diff might seem improper especially the part where I
have introduced some continuous lines but this is a logical error by git
diff (nothing could be done about it) and thus the changes will be
clearly visible with the original file itself.
---
 Documentation/technical/api-parse-options.txt |  8 +++--
 parse-options.c                               |  2 ++
 t/t0040-parse-options.sh                      | 43 +++++++++++++++++++--------
 test-parse-options.c                          |  3 +-
 4 files changed, 41 insertions(+), 15 deletions(-)
Show changes to 4 files +41 −16

Documentation/technical/api-parse-options.txt, parse-options.c, t/t0040-parse-options.sh, test-parse-options.c

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 5f0757d..8908bf7 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -144,8 +144,12 @@ There are some macros to easily define options:
 
 `OPT_COUNTUP(short, long, &int_var, description)`::
 	Introduce a count-up option.
-	`int_var` is incremented on each use of `--option`, and
-	reset to zero with `--no-option`.
+	Each use of `--option` increments `int_var`, starting from zero
+	(even if initially negative), and `--no-option` resets it to
+	zero. To determine if `--option` or `--no-option` was set at
+	all, set `int_var` to a negative value, and if it is still
+	negative after parse_options(), then neither `--option` nor
+	`--no-option` was seen.
 
 `OPT_BIT(short, long, &int_var, description, mask)`::
 	Introduce a boolean option.
diff --git a/parse-options.c b/parse-options.c
index 47a9192..312a85d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_COUNTUP:
+		if (*(int *)opt->value < 0)
+			*(int *)opt->value = 0;
 		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
 		return 0;
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index c913d1c..d3f52f6 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -63,7 +63,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -211,7 +211,7 @@ magnitude: 0
 timestamp: 0
 string: 123
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -234,7 +234,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -263,7 +263,7 @@ magnitude: 0
 timestamp: 0
 string: 123
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -302,7 +302,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -322,7 +322,7 @@ magnitude: 0
 timestamp: 1
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 1
 dry run: no
 file: (not set)
@@ -344,7 +344,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -373,7 +373,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -398,7 +398,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -429,7 +429,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -454,6 +454,25 @@ dry run: no
 file: (not set)
 EOF
 
+test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
+	test-parse-options --no-verbose >output 2>output.err &&
+	test_must_be_empty output.err &&
+	test_cmp expect output
+'
+
+cat >expect <<EOF
+boolean: 0
+integer: 0
+magnitude: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: -1
+quiet: 0
+dry run: no
+file: (not set)
+EOF
+
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
 	test-parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
@@ -483,7 +502,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 3
 dry run: no
 file: (not set)
@@ -502,7 +521,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
diff --git a/test-parse-options.c b/test-parse-options.c
index 86afa98..f02c275 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -7,7 +7,8 @@ static int integer = 0;
 static unsigned long magnitude = 0;
 static unsigned long timestamp;
 static int abbrev = 7;
-static int verbose = 0, dry_run = 0, quiet = 0;
+static int verbose = -1; /* unspecified */
+static int dry_run = 0, quiet = 0;
 static char *string = NULL;
 static char *file = NULL;
 static int ambiguous;

--
https://github.com/git/git/pull/218
Eric Sunshine· Apr 13, 2016, 05:56 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 10 quoted lines
> OPT_COUNTUP() merely increments the counter upon --option, and resets it
> to 0 upon --no-option, which means that there is no "unspecified" value
> with which a client can initialize the counter to determine whether or
> not --[no]-option was seen at all.
>
> Make OPT_COUNTUP() treat any negative number as an "unspecified" value
> to address this shortcoming. In particular, if a client initializes the
> counter to -1, then if it is still -1 after parse_options(), then
> neither --option nor --no-option was seen; If it is 0, then --no-option
> was seen last, and if it is 1 or greater, than --option was seen last.
Nit: I'm pretty sure that when I wrote this commit message for you[1]
the "if" following the semicolon was correctly lowercase. It's not
clear why it got incorrectly capitalized here.
More below...
Show 24 quoted lines
> This change does not affect the behavior of existing clients because
> they all use the initial value of 0 (or more).
>
> Note that builtin/clean.c initializes the variable used with
> OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set
> to either 0 or 1 by reading the configuration before the code calls
> parse_options(), i.e. as far as parse_options() is concerned, the
> initial value of the variable is not negative.
>
> To test this behavior, in test-parse-options.c, "verbose" is set to
> "unspecified" while quiet is set to 0 which will test the new behavior
> with all sets of values.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> @@ -144,8 +144,12 @@ There are some macros to easily define options:
>  `OPT_COUNTUP(short, long, &int_var, description)`::
>         Introduce a count-up option.
> -       `int_var` is incremented on each use of `--option`, and
> -       reset to zero with `--no-option`.
> +       Each use of `--option` increments `int_var`, starting from zero
> +       (even if initially negative), and `--no-option` resets it to
> +       zero. To determine if `--option` or `--no-option` was set at
Repeating from [1]: s/was set/was encountered/
> +       all, set `int_var` to a negative value, and if it is still
Repeating from [1] and [2]: s/set `int_var`/initialize `int_var`/
Show 5 quoted lines
> +       negative after parse_options(), then neither `--option` nor
> +       `--no-option` was seen.
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> @@ -454,6 +454,25 @@ dry run: no
> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
What is "--no- flag"?
> +       test-parse-options --no-verbose >output 2>output.err &&
> +       test_must_be_empty output.err &&
> +       test_cmp expect output
> +'

In my v12 review, I noticed that neither --no-verbose nor --no-quiet was being tested by t0040 (which is conceptually independent of the OPT__COUNTUP change) and suggested[3] that you add a new patch to address that shortcoming. This idea was followed up by [1] saying that this test (here) could then be dropped since the case it checks would already be covered by the new patch. My impression was that you agreed[4] that that made sense, however, this test is still here. Did I misunderstand your response[4]?

[1]: http://article.gmane.org/gmane.comp.version-control.git/290662 [2]: http://article.gmane.org/gmane.comp.version-control.git/289991 [3]: http://article.gmane.org/gmane.comp.version-control.git/290655 [4]: http://article.gmane.org/gmane.comp.version-control.git/290787

Pranit Bauva· Apr 13, 2016, 08:39 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 15 quoted lines
> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> OPT_COUNTUP() merely increments the counter upon --option, and resets it
>> to 0 upon --no-option, which means that there is no "unspecified" value
>> with which a client can initialize the counter to determine whether or
>> not --[no]-option was seen at all.
>>
>> Make OPT_COUNTUP() treat any negative number as an "unspecified" value
>> to address this shortcoming. In particular, if a client initializes the
>> counter to -1, then if it is still -1 after parse_options(), then
>> neither --option nor --no-option was seen; If it is 0, then --no-option
>> was seen last, and if it is 1 or greater, than --option was seen last.
>
> Nit: I'm pretty sure that when I wrote this commit message for you[1]
> the "if" following the semicolon was correctly lowercase. It's not
> clear why it got incorrectly capitalized here.
A typo. Will fix it if re-rolled
Show 32 quoted lines
> More below...
>
>> This change does not affect the behavior of existing clients because
>> they all use the initial value of 0 (or more).
>>
>> Note that builtin/clean.c initializes the variable used with
>> OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set
>> to either 0 or 1 by reading the configuration before the code calls
>> parse_options(), i.e. as far as parse_options() is concerned, the
>> initial value of the variable is not negative.
>>
>> To test this behavior, in test-parse-options.c, "verbose" is set to
>> "unspecified" while quiet is set to 0 which will test the new behavior
>> with all sets of values.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> @@ -144,8 +144,12 @@ There are some macros to easily define options:
>>  `OPT_COUNTUP(short, long, &int_var, description)`::
>>         Introduce a count-up option.
>> -       `int_var` is incremented on each use of `--option`, and
>> -       reset to zero with `--no-option`.
>> +       Each use of `--option` increments `int_var`, starting from zero
>> +       (even if initially negative), and `--no-option` resets it to
>> +       zero. To determine if `--option` or `--no-option` was set at
>
> Repeating from [1]: s/was set/was encountered/
>
>> +       all, set `int_var` to a negative value, and if it is still
>
> Repeating from [1] and [2]: s/set `int_var`/initialize `int_var`/
Will include this if re-rolled.
Show 27 quoted lines
>
>> +       negative after parse_options(), then neither `--option` nor
>> +       `--no-option` was seen.
>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
>> @@ -454,6 +454,25 @@ dry run: no
>> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
>
> What is "--no- flag"?
>
>> +       test-parse-options --no-verbose >output 2>output.err &&
>> +       test_must_be_empty output.err &&
>> +       test_cmp expect output
>> +'
>
> In my v12 review, I noticed that neither --no-verbose nor --no-quiet
> was being tested by t0040 (which is conceptually independent of the
> OPT__COUNTUP change) and suggested[3] that you add a new patch to
> address that shortcoming. This idea was followed up by [1] saying that
> this test (here) could then be dropped since the case it checks would
> already be covered by the new patch. My impression was that you
> agreed[4] that that made sense, however, this test is still here. Did
> I misunderstand your response[4]?
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991
> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655
> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787

I actually did include the tests in the patch 3/6[1]. I mentioned in my response[2] that I will include the tests between 2/5 and 3/5. Since, after that no email was exchanged, I thought you agreed.

[1]: http://article.gmane.org/gmane.comp.version-control.git/291310 [2]:http://article.gmane.org/gmane.comp.version-control.git/290787

Eric Sunshine· Apr 13, 2016, 17:33 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 25 quoted lines
> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
>>> +       test-parse-options --no-verbose >output 2>output.err &&
>>> +       test_must_be_empty output.err &&
>>> +       test_cmp expect output
>>> +'
>>
>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet
>> was being tested by t0040 (which is conceptually independent of the
>> OPT__COUNTUP change) and suggested[3] that you add a new patch to
>> address that shortcoming. This idea was followed up by [1] saying that
>> this test (here) could then be dropped since the case it checks would
>> already be covered by the new patch. My impression was that you
>> agreed[4] that that made sense, however, this test is still here. Did
>> I misunderstand your response[4]?
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662
>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991
>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655
>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787
>
> I actually did include the tests in the patch 3/6[1]. I mentioned in
> my response[2] that I will include the tests between 2/5 and 3/5.
> Since, after that no email was exchanged, I thought you agreed.

I'm not sure that I understand what you are saying since the --no-verbose test does not seem to be included in the patch you cite (it is instead in the present patch under discussion).

Perhaps there is some miscommunication and misunderstanding.
> [1]: http://article.gmane.org/gmane.comp.version-control.git/291310
> [2]:http://article.gmane.org/gmane.comp.version-control.git/290787
Pranit Bauva· Apr 13, 2016, 17:43 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

On Wed, Apr 13, 2016 at 11:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 32 quoted lines
> On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
>>>> +       test-parse-options --no-verbose >output 2>output.err &&
>>>> +       test_must_be_empty output.err &&
>>>> +       test_cmp expect output
>>>> +'
>>>
>>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet
>>> was being tested by t0040 (which is conceptually independent of the
>>> OPT__COUNTUP change) and suggested[3] that you add a new patch to
>>> address that shortcoming. This idea was followed up by [1] saying that
>>> this test (here) could then be dropped since the case it checks would
>>> already be covered by the new patch. My impression was that you
>>> agreed[4] that that made sense, however, this test is still here. Did
>>> I misunderstand your response[4]?
>>>
>>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662
>>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991
>>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655
>>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787
>>
>> I actually did include the tests in the patch 3/6[1]. I mentioned in
>> my response[2] that I will include the tests between 2/5 and 3/5.
>> Since, after that no email was exchanged, I thought you agreed.
>
> I'm not sure that I understand what you are saying since the
> --no-verbose test does not seem to be included in the patch you cite
> (it is instead in the present patch under discussion).
>
> Perhaps there is some miscommunication and misunderstanding.

Sorry for being a bit unclear. I will explain this. The patch 3/6 contains the test which tests the quiet option thus in turn testing whether the variable quiet becomes 0 with --no flag. This patch ie. 4/6 contains the test which tests the verbose options thus in turn testing whether the variable verbose becomes 0 with no flag. Both of them test different behavior as quiet is initially 0 while verbose is initially -1.

So finally what I wanted to achieve is that I should test --no-quiet in 3/6 as till then this new behavior is not yet introduced. Thus, it will confirm the wanted behavior which exists before 4/6.

This patch introduces a test to verify whether --no-verbose makes the variable 0. This patch introduces a new "unspecified" behavior. Thus we can test this new behavior with this.

I hope now it is a bit clear on what I want to do.
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/291310
>> [2]:http://article.gmane.org/gmane.comp.version-control.git/290787
Eric Sunshine· Apr 13, 2016, 17:50 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

On Wed, Apr 13, 2016 at 1:43 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 51 quoted lines
> On Wed, Apr 13, 2016 at 11:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>>> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
>>>>> +       test-parse-options --no-verbose >output 2>output.err &&
>>>>> +       test_must_be_empty output.err &&
>>>>> +       test_cmp expect output
>>>>> +'
>>>>
>>>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet
>>>> was being tested by t0040 (which is conceptually independent of the
>>>> OPT__COUNTUP change) and suggested[3] that you add a new patch to
>>>> address that shortcoming. This idea was followed up by [1] saying that
>>>> this test (here) could then be dropped since the case it checks would
>>>> already be covered by the new patch. My impression was that you
>>>> agreed[4] that that made sense, however, this test is still here. Did
>>>> I misunderstand your response[4]?
>>>>
>>>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662
>>>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991
>>>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655
>>>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787
>>>
>>> I actually did include the tests in the patch 3/6[1]. I mentioned in
>>> my response[2] that I will include the tests between 2/5 and 3/5.
>>> Since, after that no email was exchanged, I thought you agreed.
>>
>> I'm not sure that I understand what you are saying since the
>> --no-verbose test does not seem to be included in the patch you cite
>> (it is instead in the present patch under discussion).
>>
>> Perhaps there is some miscommunication and misunderstanding.
>
> Sorry for being a bit unclear.
> I will explain this. The patch 3/6 contains the test which tests the
> quiet option thus in turn testing whether the variable quiet becomes 0
> with --no flag. This patch ie. 4/6 contains the test which tests the
> verbose options thus in turn testing whether the variable verbose
> becomes 0 with no flag. Both of them test different behavior as quiet
> is initially 0 while verbose is initially -1.
>
> So finally what I wanted to achieve is that I should test --no-quiet
> in 3/6 as till then this new behavior is not yet introduced. Thus, it
> will confirm the wanted behavior which exists before 4/6.
>
> This patch introduces a test to verify whether --no-verbose makes the
> variable 0. This patch introduces a new "unspecified" behavior. Thus
> we can test this new behavior with this.
>
> I hope now it is a bit clear on what I want to do.

Sorry, it's not clearer. I understand what you're trying to do, but it still seems to be the a less desirable (and more complex) approach since it's mixing conceptually distinct notions and mismatching them with changes in the wrong patches. But, perhaps my notion of "conceptually distinct" is different from yours and vice-versa.

Pranit Bauva· Apr 12, 2016, 23:02 UTC · re: Pranit Bauva · lore

[PATCH v14 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs

Make the fake "editor" store output of grep in a file so that we can see how many diffs were contained in the message and use them in individual tests where ever it is required. A subsequent commit will introduce scenarios where it is important to be able to exactly determine how many diffs were present.

Also use write_script() to create the fake "editor".

The fake "editor" is always made to succeed regardless of whether grep found diff headers or not so that we don't have to use 'test_must_fail' for which 'test_line_count = 0' is an easy substitute and also helps in maintaining the consistency.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
Previous version of this patch:
 - [v12] : $gmane/288820
 - [v11] : $gmane/288820
 - [v10]: $gmane/288820
Changes this version wrt previous one:
Change the commit message as suggested by Eric
---
 t/t7507-commit-verbose.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
Show changes to t/t7507-commit-verbose.sh +9 −8
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 2ddf28c..0f28a86 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -3,11 +3,10 @@
 test_description='verbose commit template'
 . ./test-lib.sh
 
-cat >check-for-diff <<EOF
-#!$SHELL_PATH
-exec grep '^diff --git' "\$1"
+write_script "check-for-diff" <<\EOF &&
+grep '^diff --git' "$1" >out
+exit 0
 EOF
-chmod +x check-for-diff
 test_set_editor "$PWD/check-for-diff"
 
 cat >message <<'EOF'
@@ -23,7 +22,8 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'initial commit shows verbose diff' '
-	git commit --amend -v
+	git commit --amend -v &&
+	test_line_count = 1 out
 '
 
 test_expect_success 'second commit' '
@@ -39,13 +39,15 @@ check_message() {
 
 test_expect_success 'verbose diff is stripped out' '
 	git commit --amend -v &&
-	check_message message
+	check_message message &&
+	test_line_count = 1 out
 '
 
 test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
 	git config diff.mnemonicprefix true &&
 	git commit --amend -v &&
-	check_message message
+	check_message message &&
+	test_line_count = 1 out
 '
 
 cat >diff <<'EOF'

--
https://github.com/git/git/pull/218
Eric Sunshine· Apr 13, 2016, 06:03 UTC · re: Pranit Bauva · lore

Re: [PATCH v14 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs

On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 12 quoted lines
> Make the fake "editor" store output of grep in a file so that we can
> see how many diffs were contained in the message and use them in
> individual tests where ever it is required. A subsequent commit will
> introduce scenarios where it is important to be able to exactly
> determine how many diffs were present.
>
> Also use write_script() to create the fake "editor".
>
> The fake "editor" is always made to succeed regardless of whether grep
> found diff headers or not so that we don't have to use 'test_must_fail'
> for which 'test_line_count = 0' is an easy substitute and also helps in
> maintaining the consistency.

As mentioned by [1], the change to write_script() is a minor aside; it is less important than the explanation of how and why the return value of the fake "editor" changed, thus the order of the 2nd and 3rd paragraphs should be swapped.

[1]: http://article.gmane.org/gmane.comp.version-control.git/290663
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Pranit Bauva· Apr 13, 2016, 09:00 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs

On Wed, Apr 13, 2016 at 11:33 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 20 quoted lines
> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Make the fake "editor" store output of grep in a file so that we can
>> see how many diffs were contained in the message and use them in
>> individual tests where ever it is required. A subsequent commit will
>> introduce scenarios where it is important to be able to exactly
>> determine how many diffs were present.
>>
>> Also use write_script() to create the fake "editor".
>>
>> The fake "editor" is always made to succeed regardless of whether grep
>> found diff headers or not so that we don't have to use 'test_must_fail'
>> for which 'test_line_count = 0' is an easy substitute and also helps in
>> maintaining the consistency.
>
> As mentioned by [1], the change to write_script() is a minor aside; it
> is less important than the explanation of how and why the return value
> of the fake "editor" changed, thus the order of the 2nd and 3rd
> paragraphs should be swapped.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/290663
I will include this if I do a re-roll
>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

← back to recent threads