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

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

[Pranit Bauva]

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
Changes wrt previous version (v12):
 - Use '\' when interpolation isn't required

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t0040-parse-options.sh | 76 ++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

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

[PATCH v15 2/7] test-parse-options: print quiet as integer

[Pranit Bauva]

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(-)

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)");
 
-- 
2.8.1

[PATCH v15 3/7] t0040-parse-options: improve test coverage

[Pranit Bauva]

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>

---
Link to v14:
 - $gmane/288880

Changes wrt v14:
 - Change the test to use '-q -q -q --no-quiet' instead of just '--no-quiet'
 - Move the test for '--no-verbose' from OPT_COUNTUP patch to this one.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t0040-parse-options.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 450da45..57fc2a1 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -476,4 +476,61 @@ 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 -q -q -q --no-quiet >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-verbose sets verbose to 0' '
+	test-parse-options --no-verbose >output 2> output.err &&
+	test_must_be_empty output.err &&
+	test_cmp expect output
+'
+
 test_done
-- 
2.8.1

Re: [PATCH v15 3/7] t0040-parse-options: improve test coverage

[Eric Sunshine]

On Sat, Apr 30, 2016 at 4:03 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.

As this patch is also adding a test of --[no-]verbose, the commit
message should mention it.

More below...

> 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,61 @@ test_expect_success '--no-list resets list' '
> +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
> +'
> +
> +test_expect_success '--no-quiet sets quiet to 0' '
> +       test-parse-options -q -q -q --no-quiet >output 2>output.err &&
> +       test_must_be_empty output.err &&
> +       test_cmp expect output
> +'

It wouldn't hurt to have two tests for --no-quiet: one which tests
--no-quiet alone to ensure that 'quiet' *remains* at 0, and one which
tests --no-quiet in combination with some --quiet's to ensure that
'quiet' is *reset* to 0. These tests would give you good coverage for
changes by subsequent patches, such as the OPTION_COUNTUP patch which
flips the initial value to -1.

> +
> +test_expect_success '--no-verbose sets verbose to 0' '
> +       test-parse-options --no-verbose >output 2> output.err &&
> +       test_must_be_empty output.err &&
> +       test_cmp expect output
> +'

One would expect to see 'verbose' get the same treatment of having a
test invoke --verbose multiple times. (Yes, I realize that the "long
options" test does just this, but testing multiple --verbose's is not
its primary purpose, so having a test which does test multiple
--verbose's as its primary purpose can be beneficial and is less
likely to be broken by someone in the future.)

Re: [PATCH v15 3/7] t0040-parse-options: improve test coverage

[Pranit Bauva]

On Wed, May 4, 2016 at 2:06 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Apr 30, 2016 at 4:03 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.
>
> As this patch is also adding a test of --[no-]verbose, the commit
> message should mention it.

Will include this in commit message.

>
> More below...
>
>> 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,61 @@ test_expect_success '--no-list resets list' '
>> +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
>> +'
>> +
>> +test_expect_success '--no-quiet sets quiet to 0' '
>> +       test-parse-options -q -q -q --no-quiet >output 2>output.err &&
>> +       test_must_be_empty output.err &&
>> +       test_cmp expect output
>> +'
>
> It wouldn't hurt to have two tests for --no-quiet: one which tests
> --no-quiet alone to ensure that 'quiet' *remains* at 0, and one which
> tests --no-quiet in combination with some --quiet's to ensure that
> 'quiet' is *reset* to 0. These tests would give you good coverage for
> changes by subsequent patches, such as the OPTION_COUNTUP patch which
> flips the initial value to -1.

Will add them

>> +
>> +test_expect_success '--no-verbose sets verbose to 0' '
>> +       test-parse-options --no-verbose >output 2> output.err &&
>> +       test_must_be_empty output.err &&
>> +       test_cmp expect output
>> +'
>
> One would expect to see 'verbose' get the same treatment of having a
> test invoke --verbose multiple times. (Yes, I realize that the "long
> options" test does just this, but testing multiple --verbose's is not
> its primary purpose, so having a test which does test multiple
> --verbose's as its primary purpose can be beneficial and is less
> likely to be broken by someone in the future.)

Sure. Having another test dedicated wouldn't hurt.

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

[Pranit Bauva]

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:
[v14] : http://thread.gmane.org/gmane.comp.version-control.git/288820

Changes wrt previous version (v14):
 - Remove a test and move it to a preparatory 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.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 Documentation/technical/api-parse-options.txt |  8 ++++++--
 parse-options.c                               |  2 ++
 t/t0040-parse-options.sh                      | 26 +++++++++++++-------------
 test-parse-options.c                          |  3 ++-
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 695bd4b..27bd701 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 encountered at
+	all, initialize `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 57fc2a1..9638ca0 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)
@@ -448,7 +448,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -483,7 +483,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 3
 dry run: no
 file: (not set)
@@ -502,7 +502,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;
-- 
2.8.1

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

[Pranit Bauva]

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.

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.

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

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

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t7507-commit-verbose.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

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'
-- 
2.8.1

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

[Pranit Bauva]

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 v14:
 - Add the status related tests in a different patch after this patch.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 Documentation/config.txt     |  4 ++++
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c             | 14 +++++++++++++-
 t/t7507-commit-verbose.sh    | 46 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..8bf6040 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 391126e..114ffc9 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;
@@ -1364,6 +1366,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)
@@ -1515,6 +1519,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)
@@ -1664,6 +1673,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..2bb6d8d 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -98,4 +98,50 @@ 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_done
-- 
2.8.1

[PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Pranit Bauva]

Variable named 'verbose' in builtin/commit.c is consumed by git-status
and git-commit so if a new verbose related behavior is introduced in
git-commit, then it should not affect the behavior of git-status.

One previous commit (title: commit: add a commit.verbose config
variable) introduced a new config variable named commit.verbose,
so care should be taken that it would not affect the behavior of
status.

Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
respect "unspecified" values") changes the initial value of verbose
from 0 to -1. This can cause git-status to display a verbose output even
when it isn't supposed to.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
This is a split off from the previous patch 6/6 as suggested by Eric
Sunshine.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t7507-commit-verbose.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 2bb6d8d..00e0c3d 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -144,4 +144,14 @@ do
 	"
 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
-- 
2.8.1

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> Variable named 'verbose' in builtin/commit.c is consumed by git-status
> and git-commit so if a new verbose related behavior is introduced in
> git-commit, then it should not affect the behavior of git-status.
>
> One previous commit (title: commit: add a commit.verbose config
> variable) introduced a new config variable named commit.verbose,
> so care should be taken that it would not affect the behavior of
> status.
>
> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
> respect "unspecified" values") changes the initial value of verbose
> from 0 to -1. This can cause git-status to display a verbose output even
> when it isn't supposed to.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> ---
> This is a split off from the previous patch 6/6 as suggested by Eric
> Sunshine.

If these are documenting what your previous patches broke, then
there test body should describe what should happen, and then if it
is broken, use test_expect_failure, no?

Your first test does "run status with commit.verbose is set, and
make sure the "diff --git" does not appear", which is correct, so if
it does not work, test_expect_failure would be the right thing to
use.

These, especially the latter, look rather unpleasant regressions to
me, and the main commit.verbose change would need to be held back
before they are fixed.

> ---
>  t/t7507-commit-verbose.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 2bb6d8d..00e0c3d 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -144,4 +144,14 @@ do
>  	"
>  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

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Pranit Bauva]

On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>> and git-commit so if a new verbose related behavior is introduced in
>> git-commit, then it should not affect the behavior of git-status.
>>
>> One previous commit (title: commit: add a commit.verbose config
>> variable) introduced a new config variable named commit.verbose,
>> so care should be taken that it would not affect the behavior of
>> status.
>>
>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>> respect "unspecified" values") changes the initial value of verbose
>> from 0 to -1. This can cause git-status to display a verbose output even
>> when it isn't supposed to.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> ---
>> This is a split off from the previous patch 6/6 as suggested by Eric
>> Sunshine.
>
> If these are documenting what your previous patches broke, then
> there test body should describe what should happen, and then if it
> is broken, use test_expect_failure, no?
>
> Your first test does "run status with commit.verbose is set, and
> make sure the "diff --git" does not appear", which is correct, so if
> it does not work, test_expect_failure would be the right thing to
> use.
>
> These, especially the latter, look rather unpleasant regressions to
> me, and the main commit.verbose change would need to be held back
> before they are fixed.

I agree that using test_expect_failure would be a better way of going
with this thing. Thanks. Will send an updated patch for this.

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Eric Sunshine]

On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>>> and git-commit so if a new verbose related behavior is introduced in
>>> git-commit, then it should not affect the behavior of git-status.
>>>
>>> One previous commit (title: commit: add a commit.verbose config
>>> variable) introduced a new config variable named commit.verbose,
>>> so care should be taken that it would not affect the behavior of
>>> status.
>>>
>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>> respect "unspecified" values") changes the initial value of verbose
>>> from 0 to -1. This can cause git-status to display a verbose output even
>>> when it isn't supposed to.
>>>
>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> If these are documenting what your previous patches broke, then
>> there test body should describe what should happen, and then if it
>> is broken, use test_expect_failure, no?
>>
>> Your first test does "run status with commit.verbose is set, and
>> make sure the "diff --git" does not appear", which is correct, so if
>> it does not work, test_expect_failure would be the right thing to
>> use.
>>
>> These, especially the latter, look rather unpleasant regressions to
>> me, and the main commit.verbose change would need to be held back
>> before they are fixed.
>
> I agree that using test_expect_failure would be a better way of going
> with this thing. Thanks. Will send an updated patch for this.

Please don't. test_expect_failure() is not warranted.

Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.

The problems which caused the regressions were addressed in later
versions of the series, thus using test_expect_success() is indeed
correct, whereas test_expect_failure(), which illustrates broken
behavior, would be the wrong choice.

The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.

This patch's commit message is perhaps a bit too detailed about what
could have gone wrong in earlier patches in this series; indeed, it
misled Junio into thinking that patches in this series did break
behavior, when in fact, it was instead previous rounds of this series
which were buggy. If you instead make this a preparatory patch[3],
then you can sell it more simply by explaining that git-commit and
git-status share implementation (without necessarily going into detail
about exactly what is shared), and that you're improving test coverage
to ensure that changes specific to git-commit don't accidentally
impact git-status, as well.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648
[2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730
[3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Pranit Bauva]

On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>>>> and git-commit so if a new verbose related behavior is introduced in
>>>> git-commit, then it should not affect the behavior of git-status.
>>>>
>>>> One previous commit (title: commit: add a commit.verbose config
>>>> variable) introduced a new config variable named commit.verbose,
>>>> so care should be taken that it would not affect the behavior of
>>>> status.
>>>>
>>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>>> respect "unspecified" values") changes the initial value of verbose
>>>> from 0 to -1. This can cause git-status to display a verbose output even
>>>> when it isn't supposed to.
>>>>
>>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>>
>>> If these are documenting what your previous patches broke, then
>>> there test body should describe what should happen, and then if it
>>> is broken, use test_expect_failure, no?
>>>
>>> Your first test does "run status with commit.verbose is set, and
>>> make sure the "diff --git" does not appear", which is correct, so if
>>> it does not work, test_expect_failure would be the right thing to
>>> use.
>>>
>>> These, especially the latter, look rather unpleasant regressions to
>>> me, and the main commit.verbose change would need to be held back
>>> before they are fixed.
>>
>> I agree that using test_expect_failure would be a better way of going
>> with this thing. Thanks. Will send an updated patch for this.
>
> Please don't. test_expect_failure() is not warranted.

I got confused between test_must_fail and test_expect_failure. I
thought Junio mentioned to use test_must_fail and remove the " ! "
sign.

> Step back a moment and recall why these tests were added. Earlier
> rounds of this series were buggy and caused regressions in git-status.
> As a consequence, reviewers suggested[1,2] that you improve test
> coverage to ensure that such breakage is caught early.
>
> The problems which caused the regressions were addressed in later
> versions of the series, thus using test_expect_success() is indeed
> correct, whereas test_expect_failure(), which illustrates broken
> behavior, would be the wrong choice.
>
> The point of these new tests is to prevent regressions caused by
> *subsequent* changes, which is why it was suggested that these tests
> be added early (as a "preparatory patch"[3]), not at the very end of
> the series as done here in v15.
>
> This patch's commit message is perhaps a bit too detailed about what
> could have gone wrong in earlier patches in this series; indeed, it
> misled Junio into thinking that patches in this series did break
> behavior, when in fact, it was instead previous rounds of this series
> which were buggy. If you instead make this a preparatory patch[3],
> then you can sell it more simply by explaining that git-commit and
> git-status share implementation (without necessarily going into detail
> about exactly what is shared), and that you're improving test coverage
> to ensure that changes specific to git-commit don't accidentally
> impact git-status, as well.

Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.

>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730
> [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Eric Sunshine]

On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> I agree that using test_expect_failure would be a better way of going
>>> with this thing. Thanks. Will send an updated patch for this.
>>
>> Please don't. test_expect_failure() is not warranted.
>
> I got confused between test_must_fail and test_expect_failure. I
> thought Junio mentioned to use test_must_fail and remove the " ! "
> sign.
>
>> Step back a moment and recall why these tests were added. Earlier
>> rounds of this series were buggy and caused regressions in git-status.
>> As a consequence, reviewers suggested[1,2] that you improve test
>> coverage to ensure that such breakage is caught early.
>>
>> The problems which caused the regressions were addressed in later
>> versions of the series, thus using test_expect_success() is indeed
>> correct, whereas test_expect_failure(), which illustrates broken
>> behavior, would be the wrong choice.
>>
>> The point of these new tests is to prevent regressions caused by
>> *subsequent* changes, which is why it was suggested that these tests
>> be added early (as a "preparatory patch"[3]), not at the very end of
>> the series as done here in v15.
>>
>> This patch's commit message is perhaps a bit too detailed about what
>> could have gone wrong in earlier patches in this series; indeed, it
>> misled Junio into thinking that patches in this series did break
>> behavior, when in fact, it was instead previous rounds of this series
>> which were buggy. If you instead make this a preparatory patch[3],
>> then you can sell it more simply by explaining that git-commit and
>> git-status share implementation (without necessarily going into detail
>> about exactly what is shared), and that you're improving test coverage
>> to ensure that changes specific to git-commit don't accidentally
>> impact git-status, as well.
>
> Sure! I just wanted the commit message to be detailed as per the
> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
> patch 7/7 changing the commit message. Also I will make the commit
> message less detailed.

This patch should be inserted before 4/7 since it needs to protect
against breakage which might occur when 4/7 changes the behavior of
OPTION_COUNTUP.

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Pranit Bauva]

On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>>> I agree that using test_expect_failure would be a better way of going
>>>> with this thing. Thanks. Will send an updated patch for this.
>>>
>>> Please don't. test_expect_failure() is not warranted.
>>
>> I got confused between test_must_fail and test_expect_failure. I
>> thought Junio mentioned to use test_must_fail and remove the " ! "
>> sign.
>>
>>> Step back a moment and recall why these tests were added. Earlier
>>> rounds of this series were buggy and caused regressions in git-status.
>>> As a consequence, reviewers suggested[1,2] that you improve test
>>> coverage to ensure that such breakage is caught early.
>>>
>>> The problems which caused the regressions were addressed in later
>>> versions of the series, thus using test_expect_success() is indeed
>>> correct, whereas test_expect_failure(), which illustrates broken
>>> behavior, would be the wrong choice.
>>>
>>> The point of these new tests is to prevent regressions caused by
>>> *subsequent* changes, which is why it was suggested that these tests
>>> be added early (as a "preparatory patch"[3]), not at the very end of
>>> the series as done here in v15.
>>>
>>> This patch's commit message is perhaps a bit too detailed about what
>>> could have gone wrong in earlier patches in this series; indeed, it
>>> misled Junio into thinking that patches in this series did break
>>> behavior, when in fact, it was instead previous rounds of this series
>>> which were buggy. If you instead make this a preparatory patch[3],
>>> then you can sell it more simply by explaining that git-commit and
>>> git-status share implementation (without necessarily going into detail
>>> about exactly what is shared), and that you're improving test coverage
>>> to ensure that changes specific to git-commit don't accidentally
>>> impact git-status, as well.
>>
>> Sure! I just wanted the commit message to be detailed as per the
>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
>> patch 7/7 changing the commit message. Also I will make the commit
>> message less detailed.
>
> This patch should be inserted before 4/7 since it needs to protect
> against breakage which might occur when 4/7 changes the behavior of
> OPTION_COUNTUP.

I forgot to mention about this earlier. When I was rebasing, this stroked me.
I guess making any changes in ordering the commits will make one of
the test as absurd. One of the test uses a configuration variable
'commit.verbose' will won't be effective before the patch 6/7. So I
guess I will have to only change the commit message to reflect as
"improving test coverage".

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Eric Sunshine]

On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> Step back a moment and recall why these tests were added. Earlier
>>>> rounds of this series were buggy and caused regressions in git-status.
>>>> As a consequence, reviewers suggested[1,2] that you improve test
>>>> coverage to ensure that such breakage is caught early.
>>>>
>>>> The point of these new tests is to prevent regressions caused by
>>>> *subsequent* changes, which is why it was suggested that these tests
>>>> be added early (as a "preparatory patch"[3]), not at the very end of
>>>> the series as done here in v15.
>>>
>>> Sure! I just wanted the commit message to be detailed as per the
>>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
>>> patch 7/7 changing the commit message. Also I will make the commit
>>> message less detailed.
>>
>> This patch should be inserted before 4/7 since it needs to protect
>> against breakage which might occur when 4/7 changes the behavior of
>> OPTION_COUNTUP.
>
> I forgot to mention about this earlier. When I was rebasing, this stroked me.
> I guess making any changes in ordering the commits will make one of
> the test as absurd. One of the test uses a configuration variable
> 'commit.verbose' will won't be effective before the patch 6/7. So I
> guess I will have to only change the commit message to reflect as
> "improving test coverage".

I also had intended to talk about this but forgot. What would be quite
logical is to introduce only the "git-status without --verbose" test
in this new "improve coverage" patch before 4/7. The other test, which
ensures that git-status doesn't regress with commit.verbose, would
then very naturally be included in the patch which adds the
commit.verbose functionality (currently patch 6/7).

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Pranit Bauva]

On Tue, May 3, 2016 at 9:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>>> Step back a moment and recall why these tests were added. Earlier
>>>>> rounds of this series were buggy and caused regressions in git-status.
>>>>> As a consequence, reviewers suggested[1,2] that you improve test
>>>>> coverage to ensure that such breakage is caught early.
>>>>>
>>>>> The point of these new tests is to prevent regressions caused by
>>>>> *subsequent* changes, which is why it was suggested that these tests
>>>>> be added early (as a "preparatory patch"[3]), not at the very end of
>>>>> the series as done here in v15.
>>>>
>>>> Sure! I just wanted the commit message to be detailed as per the
>>>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
>>>> patch 7/7 changing the commit message. Also I will make the commit
>>>> message less detailed.
>>>
>>> This patch should be inserted before 4/7 since it needs to protect
>>> against breakage which might occur when 4/7 changes the behavior of
>>> OPTION_COUNTUP.
>>
>> I forgot to mention about this earlier. When I was rebasing, this stroked me.
>> I guess making any changes in ordering the commits will make one of
>> the test as absurd. One of the test uses a configuration variable
>> 'commit.verbose' will won't be effective before the patch 6/7. So I
>> guess I will have to only change the commit message to reflect as
>> "improving test coverage".
>
> I also had intended to talk about this but forgot. What would be quite
> logical is to introduce only the "git-status without --verbose" test
> in this new "improve coverage" patch before 4/7. The other test, which
> ensures that git-status doesn't regress with commit.verbose, would
> then very naturally be included in the patch which adds the
> commit.verbose functionality (currently patch 6/7).

Sure. Will do. Thanks!

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>>> One previous commit (title: commit: add a commit.verbose config
>>>> variable) introduced a new config variable named commit.verbose,
>>>> so care should be taken that it would not affect the behavior of
>>>> status.
>>>>
>>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>>> respect "unspecified" values") changes the initial value of verbose
>>>> from 0 to -1. This can cause git-status to display a verbose output even
>>>> when it isn't supposed to.
>>>> ...
>
> This patch's commit message is perhaps a bit too detailed about what
> could have gone wrong in earlier patches in this series; indeed, it
> misled Junio into thinking that patches in this series did break
> behavior, when in fact, it was instead previous rounds of this series
> which were buggy.

Indeed.  Please forget everything I said about expect-failure, if
the top two paragraphs are describing breakages that this series
does *NOT* introduce.  I was misled by them--and others will, too.
These two paragraphs do not belong to the log message.

Thanks for clarifying.

[PATCH v16 0/7] config commit verbose

[Pranit Bauva]

This series of patches add a configuration variable for verbose in
git-commit.

Link to v15:
http://thread.gmane.org/gmane.comp.version-control.git/293127

Changes wrt v15:
 * Remove the previous patch 7/7 and split the tests. Include one in
   initial patch 6/7. The other one is introduced in a separate commit
   after 4/7.
 * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,
   --no-verbose with -v as suggested by Eric Sunshine

Pranit Bauva (7):
  t0040-test-parse-options.sh: fix style issues
  test-parse-options: print quiet as integer
  t0040-parse-options: improve test coverage
  t/t7507: improve test coverage
  parse-options.c: make OPTION_COUNTUP respect "unspecified" values
  t7507-commit-verbose: improve test coverage by testing number of diffs
  commit: add a commit.verbose config variable

 Documentation/config.txt                      |   4 +
 Documentation/git-commit.txt                  |   3 +-
 Documentation/technical/api-parse-options.txt |   8 +-
 builtin/commit.c                              |  14 +-
 parse-options.c                               |   2 +
 t/t0040-parse-options.sh                      | 238 +++++++++++++++++++-------
 t/t7507-commit-verbose.sh                     |  72 +++++++-
 test-parse-options.c                          |   5 +-
 8 files changed, 271 insertions(+), 75 deletions(-)

-- 
2.8.1

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

[Pranit Bauva]

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---

 t/t0040-parse-options.sh | 76 ++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

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

[PATCH v16 2/7] test-parse-options: print quiet as integer

[Pranit Bauva]

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(-)

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)");
 
-- 
2.8.1

[PATCH v16 3/7] t0040-parse-options: improve test coverage

[Pranit Bauva]

Include tests to check for multiple levels of quiet and to check the
behavior of '--no-quiet'.

Include tests to check for multiple levels of verbose and to check the
behavior of '--no-verbose'.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
 t/t0040-parse-options.sh | 114 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 450da45..717a514 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -476,4 +476,118 @@ 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: 3
+quiet: 0
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success 'multiple verbose levels' '
+	test-parse-options -v -v -v >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
+'
+
+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 resets multiple -q to 0' '
+	test-parse-options -q -q -q --no-quiet >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-verbose sets verbose to 0' '
+	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: 0
+quiet: 0
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success '--no-verbose resets multiple verbose to 0' '
+	test-parse-options -v -v -v --no-verbose >output 2>output.err &&
+	test_must_be_empty output.err &&
+	test_cmp expect output
+'
+
 test_done
-- 
2.8.1

[PATCH v16 4/7] t/t7507: improve test coverage

[Pranit Bauva]

git-commit and git-status share the same implementation thus it is
necessary to ensure that changes specific to git-commit don't
accidentally impact git-status.

This test verifies that changes made to verbose in git-commit does not
impact git-status.

Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 t/t7507-commit-verbose.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 2ddf28c..a3c8582 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -96,4 +96,9 @@ 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 'status does not verbose without --verbose' '
+	git status >actual &&
+	! grep "^diff --git" actual
+'
+
 test_done
-- 
2.8.1

[PATCH v16 5/7] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Pranit Bauva]

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

---
 Documentation/technical/api-parse-options.txt |  8 ++++++--
 parse-options.c                               |  2 ++
 t/t0040-parse-options.sh                      | 28 +++++++++++++--------------
 test-parse-options.c                          |  3 ++-
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 695bd4b..27bd701 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 encountered at
+	all, initialize `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 717a514..fec3fef 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)
@@ -448,7 +448,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -483,7 +483,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 3
 dry run: no
 file: (not set)
@@ -521,7 +521,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -540,7 +540,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;
-- 
2.8.1

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

[Pranit Bauva]

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.

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.

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

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
 t/t7507-commit-verbose.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index a3c8582..5a81181 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'
-- 
2.8.1

[PATCH v16 7/7] commit: add a commit.verbose config variable

[Pranit Bauva]

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

Add tests to check the behavior introduced by this commit and also to
verify that behavior of status doesn't break because of this commit.

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

---
 Documentation/config.txt     |  4 ++++
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c             | 14 +++++++++++-
 t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..8bf6040 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 391126e..114ffc9 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;
@@ -1364,6 +1366,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)
@@ -1515,6 +1519,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)
@@ -1664,6 +1673,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 5a81181..ed2653d 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -103,4 +103,55 @@ test_expect_success 'status does not verbose without --verbose' '
 	! grep "^diff --git" actual
 '
 
+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_done
-- 
2.8.1

Re: [PATCH v16 7/7] commit: add a commit.verbose config variable

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 391126e..114ffc9 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;

The name does not make it clear that config_verbose is only for
"commit" and not relevant to "status".

> @@ -1364,6 +1366,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;

Mental note: cmd_status() does not use git_commit_config() but uses
git_status_config(), hence config_verbose is not affected.  But
because verbose is initialised to -1, the code needs to turn it off
like this.

> @@ -1664,6 +1673,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;
> +

cmd_commit() does use git_commit_config(), and verbose is
initialised -1, so without command line option, we fall back to
config_verbose if it is set from the configuration.

I wonder if the attached patch squashed into this commit makes
things easier to understand, though.  The points are:

 - We rename the configuration to make it clear that it is about
   "commit" and does not apply to "status".

 - We initialize verbose to 0 as before.  The only thing "git
   status" cares about is if "--verbose" was given.  Giving it
   "--no-verbose" or nothing should not make any difference.

 - But we do need to stuff -1 to verbose in "git commit" before
   handling the command line options, because the distinction
   between having "--no-verbose" and not having any matter there,
   and we do so in cmd_commit(), i.e. only place where it matters.

 builtin/commit.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 583d1e3..a486620 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -113,9 +113,8 @@ 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 config_verbose = -1; /* unspecified */
-static int verbose = -1; /* unspecified */
-static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
+static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
+static int config_commit_verbose = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
 static char *sign_commit;
@@ -1366,8 +1365,6 @@ 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)
@@ -1521,7 +1518,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 	}
 	if (!strcmp(k, "commit.verbose")) {
 		int is_bool;
-		config_verbose = git_config_bool_or_int(k, v, &is_bool);
+		config_commit_verbose = git_config_bool_or_int(k, v, &is_bool);
 		return 0;
 	}
 
@@ -1670,11 +1667,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (parse_commit(current_head))
 			die(_("could not parse HEAD commit"));
 	}
+	verbose = -1; /* unspecified */
 	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;
+		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
 
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);

Re: [PATCH v16 7/7] commit: add a commit.verbose config variable

[Pranit Bauva]

On Fri, May 6, 2016 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 391126e..114ffc9 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;
>
> The name does not make it clear that config_verbose is only for
> "commit" and not relevant to "status".

True.

>> @@ -1364,6 +1366,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;
>
> Mental note: cmd_status() does not use git_commit_config() but uses
> git_status_config(), hence config_verbose is not affected.  But
> because verbose is initialised to -1, the code needs to turn it off
> like this.

Yes

>> @@ -1664,6 +1673,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;
>> +
>
> cmd_commit() does use git_commit_config(), and verbose is
> initialised -1, so without command line option, we fall back to
> config_verbose if it is set from the configuration.
>
> I wonder if the attached patch squashed into this commit makes
> things easier to understand, though.  The points are:
>
>  - We rename the configuration to make it clear that it is about
>    "commit" and does not apply to "status".
>
>  - We initialize verbose to 0 as before.  The only thing "git
>    status" cares about is if "--verbose" was given.  Giving it
>    "--no-verbose" or nothing should not make any difference.
>
>  - But we do need to stuff -1 to verbose in "git commit" before
>    handling the command line options, because the distinction
>    between having "--no-verbose" and not having any matter there,
>    and we do so in cmd_commit(), i.e. only place where it matters.

Awesome work by addressing these points. I hadn't thought of these earlier.

>  builtin/commit.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 583d1e3..a486620 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -113,9 +113,8 @@ 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 config_verbose = -1; /* unspecified */
> -static int verbose = -1; /* unspecified */
> -static int quiet, no_verify, allow_empty, dry_run, renew_authorship;
> +static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
> +static int config_commit_verbose = -1; /* unspecified */
>  static int no_post_rewrite, allow_empty_message;
>  static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
>  static char *sign_commit;
> @@ -1366,8 +1365,6 @@ 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)
> @@ -1521,7 +1518,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)
>         }
>         if (!strcmp(k, "commit.verbose")) {
>                 int is_bool;
> -               config_verbose = git_config_bool_or_int(k, v, &is_bool);
> +               config_commit_verbose = git_config_bool_or_int(k, v, &is_bool);
>                 return 0;
>         }
>
> @@ -1670,11 +1667,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>                 if (parse_commit(current_head))
>                         die(_("could not parse HEAD commit"));
>         }
> +       verbose = -1; /* unspecified */
>         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;
> +               verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
>
>         if (dry_run)
>                 return dry_run_commit(argc, argv, prefix, current_head, &s);

This makes things quite easy to understand.
Very simple speaking:
 * Rename config_verbose => config_commit_verbose
 * initialize verbose to -1 only in cmd_commit()

I checked out your branch gitster/pb/commit-verbose-config and tests
from t0040 seem to be failing. Don't worry I will handle those, I will
squash your patch in mine and re-roll it again. I am still unsure how
those tests broke. I will figure it out.

Thanks for your help! :)

Re: [PATCH v16 7/7] commit: add a commit.verbose config variable

[Pranit Bauva]

On Fri, May 6, 2016 at 10:35 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

> I checked out your branch gitster/pb/commit-verbose-config and tests
> from t0040 seem to be failing. Don't worry I will handle those, I will
> squash your patch in mine and re-roll it again. I am still unsure how
> those tests broke. I will figure it out.
>
> Thanks for your help! :)

False alarm. I had a dirty build. The test suite passes perfectly.
Feel free to squash your patch in locally.

Re: [PATCH v16 7/7] commit: add a commit.verbose config variable

[Eric Sunshine]

On Thu, May 5, 2016 at 3:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>> +static int config_verbose = -1; /* unspecified */
>
> The name does not make it clear that config_verbose is only for
> "commit" and not relevant to "status".
>
>> @@ -1364,6 +1366,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;
>
> Mental note: cmd_status() does not use git_commit_config() but uses
> git_status_config(), hence config_verbose is not affected.  But
> because verbose is initialised to -1, the code needs to turn it off
> like this.
>
>> @@ -1664,6 +1673,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;
>> +
>
> cmd_commit() does use git_commit_config(), and verbose is
> initialised -1, so without command line option, we fall back to
> config_verbose if it is set from the configuration.
>
> I wonder if the attached patch squashed into this commit makes
> things easier to understand, though.  The points are:
>
>  - We rename the configuration to make it clear that it is about
>    "commit" and does not apply to "status".
>
>  - We initialize verbose to 0 as before.  The only thing "git
>    status" cares about is if "--verbose" was given.  Giving it
>    "--no-verbose" or nothing should not make any difference.
>
>  - But we do need to stuff -1 to verbose in "git commit" before
>    handling the command line options, because the distinction
>    between having "--no-verbose" and not having any matter there,
>    and we do so in cmd_commit(), i.e. only place where it matters.

Hmm... if someday someone wants git-status to support a status.verbose
config variable, with Pranit's current implementation, it's a pretty
simple change: Just add to git_status_config():

    if (!strcmp(k, "status.verbose")) {
        int is_bool;
        config_verbose = git_config_bool_or_int(k, v, &is_bool);
        return 0;
    }

and in cmd_status() change:

    if (verbose == -1)
        verbose = 0;

to:

    if (verbose == -1)
        verbose = (config_verbose < 0) ? 0 : config_verbose;

It wouldn't be too hard with your proposal either: Either add a
'config_status_verbose' variable or rename 'config_commit_verbose'
back to 'config_verbose', initialize the global 'verbose' to -1, drop
the explicit 'verbose = -1' from cmd_commit(), and make the same
changes shown for Pranit's version. The diff would be a bit noisier.

I do like that your proposal makes it more difficult for
commit.verbose to break git-status, but otherwise don't feel that it
is significantly better. So, I dunno...

Re: [PATCH v16 0/7] config commit verbose

[Junio C Hamano]

Pranit Bauva <pranit.bauva@gmail.com> writes:

> This series of patches add a configuration variable for verbose in
> git-commit.
>
> Link to v15:
> http://thread.gmane.org/gmane.comp.version-control.git/293127
>
> Changes wrt v15:
>  * Remove the previous patch 7/7 and split the tests. Include one in
>    initial patch 6/7. The other one is introduced in a separate commit
>    after 4/7.
>  * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,
>    --no-verbose with -v as suggested by Eric Sunshine

Thanks for a pleasant read.  Modulo minor readability nits I sent
separately on 7/7, this looked good.

A tangent that we may want to think about after this series lands
and dust settles is to make test-parse-options simpler to use.  I
see many instances of this repeated:

        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
        '

But the only thing this test cares about is if "quiet: 3" is in the
output.  I think we should be able to write the above 18 lines with
just four lines, like this:

	test_expect_success 'multiple quiet levels' '
		test-parse-options --expect="quiet: 3" -q -q -q
	'

There may be a handful of tests that care about more than one
variable, and the current output format must be used when the
new --expect option is not given, but I suspect that the majority of
tests would want the concise form.

[PATCH 0/3] test-parse-options update

[Junio C Hamano]

During the review of Pranit's "commit.verbose" series, I noticed an
overly verbose input and output used to drive test-parse-options
helper in t0040.  Here is a patch to teach the program to allow us
to write the test in a more concise way.

I'll leave it as an exercise to the readers to actually use this to
convert tests in t0040.  That needs to wait until Pranit's series
is merged and the dust settles.

Junio C Hamano (3):
  test-parse-options: fix output when callback option fails
  test-parse-options: hold output in a strbuf
  test-parse-options: --expect=<string> option to simplify tests

 t/t0040-parse-options.sh |   5 +--
 test-parse-options.c     | 110 ++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 97 insertions(+), 18 deletions(-)

-- 
2.8.2-505-gdbd0e1d

[PATCH 1/3] test-parse-options: fix output when callback option fails

[Junio C Hamano]

When test-parse-options detects an error on the command line, it
gives the usage string just like any parse-options API users do,
without showing any "variable dump".  An exception is the callback
test, where a "variable dump" for the option is done before the
command line options are fully parsed.

Do not expose this implementation detail by separating the handling
of callback test into two phases, one to capture the fact that an
option was given during the option parsing phase, and the other to
show that fact as a part of normal "variable dump".

The effect of this fix is seen in the patch to t/t0040 where it
tried "test-parse-options --no-length" where "--length" is a callback
that does not take a negative form.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0040-parse-options.sh |  4 +---
 test-parse-options.c     | 18 ++++++++++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index fec3fef..dbaee55 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -356,9 +356,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 	test_cmp expect output
 '
 
-cat >expect <<\EOF
-Callback: "not set", 1
-EOF
+>expect
 
 test_expect_success 'OPT_CALLBACK() and callback errors work' '
 	test_must_fail test-parse-options --no-length >output 2>output.err &&
diff --git a/test-parse-options.c b/test-parse-options.c
index f02c275..b5f4e90 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -14,10 +14,18 @@ static char *file = NULL;
 static int ambiguous;
 static struct string_list list;
 
+static struct {
+	int called;
+	const char *arg;
+	int unset;
+} length_cb;
+
 static int length_callback(const struct option *opt, const char *arg, int unset)
 {
-	printf("Callback: \"%s\", %d\n",
-		(arg ? arg : "not set"), unset);
+	length_cb.called = 1;
+	length_cb.arg = arg;
+	length_cb.unset = unset;
+
 	if (unset)
 		return 1; /* do not support unset */
 
@@ -84,6 +92,12 @@ int main(int argc, char **argv)
 
 	argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);
 
+	if (length_cb.called) {
+		const char *arg = length_cb.arg;
+		int unset = length_cb.unset;
+		printf("Callback: \"%s\", %d\n",
+		       (arg ? arg : "not set"), unset);
+	}
 	printf("boolean: %d\n", boolean);
 	printf("integer: %d\n", integer);
 	printf("magnitude: %lu\n", magnitude);
-- 
2.8.2-505-gdbd0e1d

[PATCH 2/3] test-parse-options: hold output in a strbuf

[Junio C Hamano]

In this step, all the output is held in a strbuf and unconditionally
dumped at the end, so there is no behaviour change (other than that
the processing may be a bit slower, now we do the buffering stdio
has been doing for us).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 test-parse-options.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/test-parse-options.c b/test-parse-options.c
index b5f4e90..3db4332 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "string-list.h"
+#include "strbuf.h"
 
 static int boolean = 0;
 static int integer = 0;
@@ -89,31 +90,34 @@ int main(int argc, char **argv)
 		OPT_END(),
 	};
 	int i;
+	struct strbuf output = STRBUF_INIT;
 
 	argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);
 
 	if (length_cb.called) {
 		const char *arg = length_cb.arg;
 		int unset = length_cb.unset;
-		printf("Callback: \"%s\", %d\n",
+		strbuf_addf(&output, "Callback: \"%s\", %d\n",
 		       (arg ? arg : "not set"), unset);
 	}
-	printf("boolean: %d\n", boolean);
-	printf("integer: %d\n", integer);
-	printf("magnitude: %lu\n", magnitude);
-	printf("timestamp: %lu\n", timestamp);
-	printf("string: %s\n", string ? string : "(not set)");
-	printf("abbrev: %d\n", abbrev);
-	printf("verbose: %d\n", verbose);
-	printf("quiet: %d\n", quiet);
-	printf("dry run: %s\n", dry_run ? "yes" : "no");
-	printf("file: %s\n", file ? file : "(not set)");
+	strbuf_addf(&output, "boolean: %d\n", boolean);
+	strbuf_addf(&output, "integer: %d\n", integer);
+	strbuf_addf(&output, "magnitude: %lu\n", magnitude);
+	strbuf_addf(&output, "timestamp: %lu\n", timestamp);
+	strbuf_addf(&output, "string: %s\n", string ? string : "(not set)");
+	strbuf_addf(&output, "abbrev: %d\n", abbrev);
+	strbuf_addf(&output, "verbose: %d\n", verbose);
+	strbuf_addf(&output, "quiet: %d\n", quiet);
+	strbuf_addf(&output, "dry run: %s\n", dry_run ? "yes" : "no");
+	strbuf_addf(&output, "file: %s\n", file ? file : "(not set)");
 
 	for (i = 0; i < list.nr; i++)
-		printf("list: %s\n", list.items[i].string);
+		strbuf_addf(&output, "list: %s\n", list.items[i].string);
 
 	for (i = 0; i < argc; i++)
-		printf("arg %02d: %s\n", i, argv[i]);
+		strbuf_addf(&output, "arg %02d: %s\n", i, argv[i]);
+
+	printf("%s", output.buf);
 
 	return 0;
 }
-- 
2.8.2-505-gdbd0e1d

[PATCH 3/3] test-parse-options: --expect=<string> option to simplify tests

[Junio C Hamano]

Existing tests in t0040 follow a rather verbose pattern:

        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
        '

But the only thing this test cares about is if "quiet: 3" is in the
output.  We should be able to write the above 18 lines with just
four lines, like this:

	test_expect_success 'multiple quiet levels' '
		test-parse-options --expect="quiet: 3" -q -q -q
	'

Teach the new --expect=<string> option to test-parse-options helper.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0040-parse-options.sh |  1 +
 test-parse-options.c     | 68 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index dbaee55..d678fbf 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -45,6 +45,7 @@ Standard options
     -v, --verbose         be verbose
     -n, --dry-run         dry run
     -q, --quiet           be quiet
+    --expect <string>     expected output in the variable dump
 
 EOF
 
diff --git a/test-parse-options.c b/test-parse-options.c
index 3db4332..010f3b2 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -14,6 +14,7 @@ static char *string = NULL;
 static char *file = NULL;
 static int ambiguous;
 static struct string_list list;
+static struct string_list expect;
 
 static struct {
 	int called;
@@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+/*
+ * See if expect->string ("label: value") has a line in output that
+ * begins with "label:", and if the line in output matches it.
+ */
+static int match_line(struct string_list_item *expect, struct strbuf *output)
+{
+	const char *label = expect->string;
+	const char *colon = strchr(label, ':');
+	const char *scan = output->buf;
+	size_t label_len, expect_len;
+
+	if (!colon)
+		die("Malformed --expect value: %s", label);
+	label_len = colon - label;
+
+	while (scan < output->buf + output->len) {
+		const char *next;
+		scan = memmem(scan, output->buf + output->len - scan,
+			      label, label_len);
+		if (!scan)
+			return 0;
+		if (scan == output->buf || scan[-1] == '\n')
+			break;
+		next = strchr(scan + label_len, '\n');
+		if (!next)
+			return 0;
+		scan = next + 1;
+	}
+
+	/*
+	 * scan points at a line that begins with the label we are
+	 * looking for.  Does it match?
+	 */
+	expect_len = strlen(expect->string);
+
+	if (output->buf + output->len <= scan + expect_len)
+		return 0; /* value not long enough */
+	if (memcmp(scan, expect->string, expect_len))
+		return 0; /* does not match */
+
+	return (scan + expect_len < output->buf + output->len &&
+		scan[expect_len] == '\n');
+}
+
+static int show_expected(struct string_list *list, struct strbuf *output)
+{
+	struct string_list_item *expect;
+	int found_mismatch = 0;
+
+	for_each_string_list_item(expect, list) {
+		if (!match_line(expect, output))
+			found_mismatch = 1;
+	}
+	return found_mismatch;
+}
+
 int main(int argc, char **argv)
 {
 	const char *prefix = "prefix/";
@@ -87,6 +144,8 @@ int main(int argc, char **argv)
 		OPT__VERBOSE(&verbose, "be verbose"),
 		OPT__DRY_RUN(&dry_run, "dry run"),
 		OPT__QUIET(&quiet, "be quiet"),
+		OPT_STRING_LIST(0, "expect", &expect, "string",
+				"expected output in the variable dump"),
 		OPT_END(),
 	};
 	int i;
@@ -117,7 +176,10 @@ int main(int argc, char **argv)
 	for (i = 0; i < argc; i++)
 		strbuf_addf(&output, "arg %02d: %s\n", i, argv[i]);
 
-	printf("%s", output.buf);
-
-	return 0;
+	if (expect.nr)
+		return show_expected(&expect, &output);
+	else {
+		printf("%s", output.buf);
+		return 0;
+	}
 }
-- 
2.8.2-505-gdbd0e1d

Re: [PATCH 3/3] test-parse-options: --expect=<string> option to simplify tests

[Stefan Beller]

On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Existing tests in t0040 follow a rather verbose pattern:
>
>         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
>         '
>
> But the only thing this test cares about is if "quiet: 3" is in the
> output.  We should be able to write the above 18 lines with just
> four lines, like this:
>
>         test_expect_success 'multiple quiet levels' '
>                 test-parse-options --expect="quiet: 3" -q -q -q
>         '
>
> Teach the new --expect=<string> option to test-parse-options helper.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t0040-parse-options.sh |  1 +
>  test-parse-options.c     | 68 +++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index dbaee55..d678fbf 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -45,6 +45,7 @@ Standard options
>      -v, --verbose         be verbose
>      -n, --dry-run         dry run
>      -q, --quiet           be quiet
> +    --expect <string>     expected output in the variable dump
>
>  EOF
>
> diff --git a/test-parse-options.c b/test-parse-options.c
> index 3db4332..010f3b2 100644
> --- a/test-parse-options.c
> +++ b/test-parse-options.c
> @@ -14,6 +14,7 @@ static char *string = NULL;
>  static char *file = NULL;
>  static int ambiguous;
>  static struct string_list list;
> +static struct string_list expect;
>
>  static struct {
>         int called;
> @@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const char *arg, int unset)
>         return 0;
>  }
>
> +/*
> + * See if expect->string ("label: value") has a line in output that
> + * begins with "label:", and if the line in output matches it.
> + */
> +static int match_line(struct string_list_item *expect, struct strbuf *output)
> +{
> +       const char *label = expect->string;
> +       const char *colon = strchr(label, ':');
> +       const char *scan = output->buf;
> +       size_t label_len, expect_len;
> +
> +       if (!colon)
> +               die("Malformed --expect value: %s", label);
> +       label_len = colon - label;
> +
> +       while (scan < output->buf + output->len) {
> +               const char *next;
> +               scan = memmem(scan, output->buf + output->len - scan,
> +                             label, label_len);
> +               if (!scan)
> +                       return 0;
> +               if (scan == output->buf || scan[-1] == '\n')

Does scan[-1] work for the first line?

> +                       break;
> +               next = strchr(scan + label_len, '\n');
> +               if (!next)
> +                       return 0;
> +               scan = next + 1;
> +       }
> +
> +       /*
> +        * scan points at a line that begins with the label we are
> +        * looking for.  Does it match?
> +        */
> +       expect_len = strlen(expect->string);
> +
> +       if (output->buf + output->len <= scan + expect_len)
> +               return 0; /* value not long enough */
> +       if (memcmp(scan, expect->string, expect_len))
> +               return 0; /* does not match */
> +
> +       return (scan + expect_len < output->buf + output->len &&
> +               scan[expect_len] == '\n');
> +}
> +
> +static int show_expected(struct string_list *list, struct strbuf *output)
> +{
> +       struct string_list_item *expect;
> +       int found_mismatch = 0;
> +
> +       for_each_string_list_item(expect, list) {
> +               if (!match_line(expect, output))
> +                       found_mismatch = 1;
> +       }
> +       return found_mismatch;
> +}
> +
>  int main(int argc, char **argv)
>  {
>         const char *prefix = "prefix/";
> @@ -87,6 +144,8 @@ int main(int argc, char **argv)
>                 OPT__VERBOSE(&verbose, "be verbose"),
>                 OPT__DRY_RUN(&dry_run, "dry run"),
>                 OPT__QUIET(&quiet, "be quiet"),
> +               OPT_STRING_LIST(0, "expect", &expect, "string",
> +                               "expected output in the variable dump"),
>                 OPT_END(),
>         };
>         int i;
> @@ -117,7 +176,10 @@ int main(int argc, char **argv)
>         for (i = 0; i < argc; i++)
>                 strbuf_addf(&output, "arg %02d: %s\n", i, argv[i]);
>
> -       printf("%s", output.buf);
> -
> -       return 0;
> +       if (expect.nr)
> +               return show_expected(&expect, &output);

On a philosophical level this patch series is adding a
trailing "|grep $X" for the test-parse-options.
I think such a grep pattern is a good thing because it is
cheap to implement in unix like environments.

This however is a lot of C code for finding specific subsets
in the output, so it is not quite cheap. Then we could also go
the non-wasteful way and instead check what to add to the strbuf
instead of filtering afterwards, i.e. each strbuf_add is guarded by
an

     if (is_interesting_output(...))
        strbuf_add(...)

> +       else {
> +               printf("%s", output.buf);
> +               return 0;
> +       }
>  }
> --
> 2.8.2-505-gdbd0e1d
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] test-parse-options: --expect=<string> option to simplify tests

[Eric Sunshine]

On Thu, May 5, 2016 at 8:41 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> [...]
>> But the only thing this test cares about is if "quiet: 3" is in the
>> output.  We should be able to write the above 18 lines with just
>> four lines, like this:
>>
>>         test_expect_success 'multiple quiet levels' '
>>                 test-parse-options --expect="quiet: 3" -q -q -q
>>         '
>>
>> Teach the new --expect=<string> option to test-parse-options helper.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
>> +/*
>> + * See if expect->string ("label: value") has a line in output that
>> + * begins with "label:", and if the line in output matches it.
>> + */
>> +static int match_line(struct string_list_item *expect, struct strbuf *output)
>> +{
>> +       [...]
>> +       const char *scan = output->buf;
>> +       [...]
>> +       while (scan < output->buf + output->len) {
>> +               const char *next;
>> +               scan = memmem(scan, output->buf + output->len - scan,
>> +                             label, label_len);
>> +               if (!scan)
>> +                       return 0;
>> +               if (scan == output->buf || scan[-1] == '\n')
>
> Does scan[-1] work for the first line?

Take note of the short-circuiting '||' operator.

> On a philosophical level this patch series is adding a
> trailing "|grep $X" for the test-parse-options.
> I think such a grep pattern is a good thing because it is
> cheap to implement in unix like environments.
>
> This however is a lot of C code for finding specific subsets
> in the output, so it is not quite cheap. Then we could also go
> the non-wasteful way and instead check what to add to the strbuf
> instead of filtering afterwards, i.e. each strbuf_add is guarded by
> an
>
>      if (is_interesting_output(...))
>         strbuf_add(...)

I agree that this is adds far more complexity than I had expected upon
reading Junio's suggestion about simplifying the t0040 tests. Patch 1
aside (which seems a desirable change), rather than patches 2 and 3, I
had expected to see only introduction of a minor helper function in
t0040; perhaps something like this:

    options_expect () {
        expect="$1" &&
        shift &&
        test-parse-options "$@" >actual &&
        grep "$expect" actual
    }

and tests updated like this:

    options_expect "quiet: 3" -q -q -q

Re: [PATCH 3/3] test-parse-options: --expect=<string> option to simplify tests

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> instead of filtering afterwards, i.e. each strbuf_add is guarded by
> an
>
>      if (is_interesting_output(...))
>         strbuf_add(...)

That's a good approach.

The implementation gets a bit trickier than the previous one, but it
would look like this.  Discard 2/3 and 3/3 and replace them with
this one.

The external interface on the input side is no different, but on the
output side, this version has "expected '%s', got '%s'" error, in
the same spirit as the output from "test_cmp", added in.

Instead of checking the entire output line-by-line for each expected
output (in case you did not notice, you can give --expect='quiet: 3'
--expect='abbrev: 7' and both must match), this one will check each
output line against each expected pattern.  We wouldn't have too
many entries in the variable dump and we wouldn't be taking too many
--expect options, so the matching performance would not matter,
though.


 t/t0040-parse-options.sh |  1 +
 test-parse-options.c     | 88 ++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index dbaee55..d678fbf 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -45,6 +45,7 @@ Standard options
     -v, --verbose         be verbose
     -n, --dry-run         dry run
     -q, --quiet           be quiet
+    --expect <string>     expected output in the variable dump
 
 EOF
 
diff --git a/test-parse-options.c b/test-parse-options.c
index b5f4e90..e3f25df 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int collect_expect(const struct option *opt, const char *arg, int unset)
+{
+	struct string_list *expect;
+	struct string_list_item *item;
+	struct strbuf label = STRBUF_INIT;
+	const char *colon;
+
+	if (!arg || unset)
+		die("malformed --expect option");
+
+	expect = (struct string_list *)opt->value;
+	colon = strchr(arg, ':');
+	if (!colon)
+		die("malformed --expect option, lacking a colon");
+	strbuf_add(&label, arg, colon - arg);
+	item = string_list_insert(expect, strbuf_detach(&label, NULL));
+	if (item->util)
+		die("malformed --expect option, duplicate %s", label.buf);
+	item->util = (void *)arg;
+	return 0;
+}
+
+__attribute__((format (printf,3,4)))
+static void show(struct string_list *expect, int *status, const char *fmt, ...)
+{
+	struct string_list_item *item;
+	struct strbuf buf = STRBUF_INIT;
+	va_list args;
+
+	va_start(args, fmt);
+	strbuf_vaddf(&buf, fmt, args);
+	va_end(args);
+
+	if (!expect->nr)
+		printf("%s\n", buf.buf);
+	else {
+		char *colon = strchr(buf.buf, ':');
+		if (!colon)
+			die("malformed output format, output lacking colon: %s", fmt);
+		*colon = '\0';
+		item = string_list_lookup(expect, buf.buf);
+		*colon = ':';
+		if (!item)
+			; /* not among entries being checked */
+		else {
+			if (strcmp((const char *)item->util, buf.buf)) {
+				printf("expected '%s', got '%s'\n",
+				       (char *)item->util, buf.buf);
+				*status = 1;
+			}
+		}
+	}
+	strbuf_reset(&buf);
+}
+
 int main(int argc, char **argv)
 {
 	const char *prefix = "prefix/";
@@ -46,6 +101,7 @@ int main(int argc, char **argv)
 		"test-parse-options <options>",
 		NULL
 	};
+	struct string_list expect = STRING_LIST_INIT_NODUP;
 	struct option options[] = {
 		OPT_BOOL(0, "yes", &boolean, "get a boolean"),
 		OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
@@ -86,34 +142,38 @@ int main(int argc, char **argv)
 		OPT__VERBOSE(&verbose, "be verbose"),
 		OPT__DRY_RUN(&dry_run, "dry run"),
 		OPT__QUIET(&quiet, "be quiet"),
+		OPT_CALLBACK(0, "expect", &expect, "string",
+			     "expected output in the variable dump",
+			     collect_expect),
 		OPT_END(),
 	};
 	int i;
+	int ret = 0;
 
 	argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);
 
 	if (length_cb.called) {
 		const char *arg = length_cb.arg;
 		int unset = length_cb.unset;
-		printf("Callback: \"%s\", %d\n",
-		       (arg ? arg : "not set"), unset);
+		show(&expect, &ret, "Callback: \"%s\", %d",
+		     (arg ? arg : "not set"), unset);
 	}
-	printf("boolean: %d\n", boolean);
-	printf("integer: %d\n", integer);
-	printf("magnitude: %lu\n", magnitude);
-	printf("timestamp: %lu\n", timestamp);
-	printf("string: %s\n", string ? string : "(not set)");
-	printf("abbrev: %d\n", abbrev);
-	printf("verbose: %d\n", verbose);
-	printf("quiet: %d\n", quiet);
-	printf("dry run: %s\n", dry_run ? "yes" : "no");
-	printf("file: %s\n", file ? file : "(not set)");
+	show(&expect, &ret, "boolean: %d", boolean);
+	show(&expect, &ret, "integer: %d", integer);
+	show(&expect, &ret, "magnitude: %lu", magnitude);
+	show(&expect, &ret, "timestamp: %lu", timestamp);
+	show(&expect, &ret, "string: %s", string ? string : "(not set)");
+	show(&expect, &ret, "abbrev: %d", abbrev);
+	show(&expect, &ret, "verbose: %d", verbose);
+	show(&expect, &ret, "quiet: %d", quiet);
+	show(&expect, &ret, "dry run: %s", dry_run ? "yes" : "no");
+	show(&expect, &ret, "file: %s", file ? file : "(not set)");
 
 	for (i = 0; i < list.nr; i++)
-		printf("list: %s\n", list.items[i].string);
+		show(&expect, &ret, "list: %s", list.items[i].string);
 
 	for (i = 0; i < argc; i++)
-		printf("arg %02d: %s\n", i, argv[i]);
+		show(&expect, &ret, "arg %02d: %s", i, argv[i]);
 
 	return 0;
 }

Re: [PATCH 3/3] test-parse-options: --expect=<string> option to simplify tests

[Stefan Beller]

On Thu, May 5, 2016 at 7:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> instead of filtering afterwards, i.e. each strbuf_add is guarded by
>> an
>>
>>      if (is_interesting_output(...))
>>         strbuf_add(...)
>
> That's a good approach.
>
> The implementation gets a bit trickier than the previous one, but it
> would look like this.  Discard 2/3 and 3/3 and replace them with
> this one.
>
> The external interface on the input side is no different, but on the
> output side, this version has "expected '%s', got '%s'" error, in
> the same spirit as the output from "test_cmp", added in.
>
> Instead of checking the entire output line-by-line for each expected
> output (in case you did not notice, you can give --expect='quiet: 3'
> --expect='abbrev: 7' and both must match), this one will check each
> output line against each expected pattern.  We wouldn't have too
> many entries in the variable dump and we wouldn't be taking too many
> --expect options, so the matching performance would not matter,
> though.
>
>
>  t/t0040-parse-options.sh |  1 +
>  test-parse-options.c     | 88 ++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 75 insertions(+), 14 deletions(-)
>
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index dbaee55..d678fbf 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -45,6 +45,7 @@ Standard options
>      -v, --verbose         be verbose
>      -n, --dry-run         dry run
>      -q, --quiet           be quiet
> +    --expect <string>     expected output in the variable dump
>
>  EOF
>
> diff --git a/test-parse-options.c b/test-parse-options.c
> index b5f4e90..e3f25df 100644
> --- a/test-parse-options.c
> +++ b/test-parse-options.c
> @@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const char *arg, int unset)
>         return 0;
>  }
>
> +static int collect_expect(const struct option *opt, const char *arg, int unset)
> +{
> +       struct string_list *expect;
> +       struct string_list_item *item;
> +       struct strbuf label = STRBUF_INIT;
> +       const char *colon;
> +
> +       if (!arg || unset)
> +               die("malformed --expect option");
> +
> +       expect = (struct string_list *)opt->value;
> +       colon = strchr(arg, ':');
> +       if (!colon)
> +               die("malformed --expect option, lacking a colon");
> +       strbuf_add(&label, arg, colon - arg);
> +       item = string_list_insert(expect, strbuf_detach(&label, NULL));
> +       if (item->util)
> +               die("malformed --expect option, duplicate %s", label.buf);
> +       item->util = (void *)arg;
> +       return 0;
> +}
> +
> +__attribute__((format (printf,3,4)))
> +static void show(struct string_list *expect, int *status, const char *fmt, ...)
> +{
> +       struct string_list_item *item;
> +       struct strbuf buf = STRBUF_INIT;
> +       va_list args;
> +
> +       va_start(args, fmt);
> +       strbuf_vaddf(&buf, fmt, args);
> +       va_end(args);
> +
> +       if (!expect->nr)
> +               printf("%s\n", buf.buf);
> +       else {
> +               char *colon = strchr(buf.buf, ':');
> +               if (!colon)
> +                       die("malformed output format, output lacking colon: %s", fmt);
> +               *colon = '\0';
> +               item = string_list_lookup(expect, buf.buf);
> +               *colon = ':';

I have been staring at this for a good couple of minutes and wondered if this
low level string manipulation is really the best way to do it.

(It feels very C idiomatic, not using a lot of Gits own data
structures. I would have
expected some sort of skip_prefix just with partial regular expression or a
string_list_split_in_place for the splitting. But this "set and reset *colon"
seems to be optimal here)

> +               if (!item)
> +                       ; /* not among entries being checked */
> +               else {
> +                       if (strcmp((const char *)item->util, buf.buf)) {
> +                               printf("expected '%s', got '%s'\n",
> +                                      (char *)item->util, buf.buf);
> +                               *status = 1;
> +                       }
> +               }
> +       }
> +       strbuf_reset(&buf);

strbuf_release ?

> +}
> +
>  int main(int argc, char **argv)
>  {
>         const char *prefix = "prefix/";
> @@ -46,6 +101,7 @@ int main(int argc, char **argv)
>                 "test-parse-options <options>",
>                 NULL
>         };
> +       struct string_list expect = STRING_LIST_INIT_NODUP;
>         struct option options[] = {
>                 OPT_BOOL(0, "yes", &boolean, "get a boolean"),
>                 OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
> @@ -86,34 +142,38 @@ int main(int argc, char **argv)
>                 OPT__VERBOSE(&verbose, "be verbose"),
>                 OPT__DRY_RUN(&dry_run, "dry run"),
>                 OPT__QUIET(&quiet, "be quiet"),
> +               OPT_CALLBACK(0, "expect", &expect, "string",
> +                            "expected output in the variable dump",
> +                            collect_expect),
>                 OPT_END(),
>         };
>         int i;
> +       int ret = 0;
>
>         argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);
>
>         if (length_cb.called) {
>                 const char *arg = length_cb.arg;
>                 int unset = length_cb.unset;
> -               printf("Callback: \"%s\", %d\n",
> -                      (arg ? arg : "not set"), unset);
> +               show(&expect, &ret, "Callback: \"%s\", %d",
> +                    (arg ? arg : "not set"), unset);
>         }
> -       printf("boolean: %d\n", boolean);
> -       printf("integer: %d\n", integer);
> -       printf("magnitude: %lu\n", magnitude);
> -       printf("timestamp: %lu\n", timestamp);
> -       printf("string: %s\n", string ? string : "(not set)");
> -       printf("abbrev: %d\n", abbrev);
> -       printf("verbose: %d\n", verbose);
> -       printf("quiet: %d\n", quiet);
> -       printf("dry run: %s\n", dry_run ? "yes" : "no");
> -       printf("file: %s\n", file ? file : "(not set)");
> +       show(&expect, &ret, "boolean: %d", boolean);
> +       show(&expect, &ret, "integer: %d", integer);
> +       show(&expect, &ret, "magnitude: %lu", magnitude);
> +       show(&expect, &ret, "timestamp: %lu", timestamp);
> +       show(&expect, &ret, "string: %s", string ? string : "(not set)");
> +       show(&expect, &ret, "abbrev: %d", abbrev);
> +       show(&expect, &ret, "verbose: %d", verbose);
> +       show(&expect, &ret, "quiet: %d", quiet);
> +       show(&expect, &ret, "dry run: %s", dry_run ? "yes" : "no");
> +       show(&expect, &ret, "file: %s", file ? file : "(not set)");
>
>         for (i = 0; i < list.nr; i++)
> -               printf("list: %s\n", list.items[i].string);
> +               show(&expect, &ret, "list: %s", list.items[i].string);
>
>         for (i = 0; i < argc; i++)
> -               printf("arg %02d: %s\n", i, argv[i]);
> +               show(&expect, &ret, "arg %02d: %s", i, argv[i]);
>
>         return 0;

    return ret; ? Otherwise `ret` is unused.

>  }

Re: [PATCH 3/3] test-parse-options: --expect=<string> option to simplify tests

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> +               *colon = '\0';
>> +               item = string_list_lookup(expect, buf.buf);
>> +               *colon = ':';
>
> I have been staring at this for a good couple of minutes and wondered if this
> low level string manipulation is really the best way to do it.

It just shows that string_list API was not designed as richly as
others, compared to say the more complete API like strbuf.  If it
had a <ptr,len> variant, I wouldn't have needed the "temporary
termination to get a string" hack.

Re: [PATCH 3/3] test-parse-options: --expect=<string> option to simplify tests

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> +               if (!item)
>> +                       ; /* not among entries being checked */
>> +               else {
>> +                       if (strcmp((const char *)item->util, buf.buf)) {
>> +                               printf("expected '%s', got '%s'\n",
>> +                                      (char *)item->util, buf.buf);
>> +                               *status = 1;
>> +                       }
>> +               }
>> +       }
>> +       strbuf_reset(&buf);
>
> strbuf_release ?

Thanks for spotting a leak.

I originally had the buf as static, as all generated strings are
short and of similar length, in an attempt to reuse the already
allocated storage instead of allocating it from scratch every call.

>>
>>         return 0;
>
>     return ret; ? Otherwise `ret` is unused.

This, too.  Thanks.

[PATCH] t0040: remove unused test helpers

[Junio C Hamano]

9a001381 (Fix tests under GETTEXT_POISON on parseopt, 2012-08-27)
introduced check_i18n, but the helper was never used from the
beginning.

The same commit also introduced check_unknown_i18n to replace the
helper check_unknown and changed all users of the latter to use the
former, but failed to remove check_unknown itself.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0040-parse-options.sh | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index d678fbf..5c8c72a 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -81,30 +81,6 @@ check() {
 	test_cmp expect output
 }
 
-check_i18n() {
-	what="$1" &&
-	shift &&
-	expect="$1" &&
-	shift &&
-	sed "s/^$what .*/$what $expect/" <expect.template >expect &&
-	test-parse-options $* >output 2>output.err &&
-	test_must_be_empty output.err &&
-	test_i18ncmp expect output
-}
-
-check_unknown() {
-	case "$1" in
-	--*)
-		echo error: unknown option \`${1#--}\' >expect ;;
-	-*)
-		echo error: unknown switch \`${1#-}\' >expect ;;
-	esac &&
-	cat expect.err >>expect &&
-	test_must_fail test-parse-options $* >output 2>output.err &&
-	test_must_be_empty output &&
-	test_cmp expect output.err
-}
-
 check_unknown_i18n() {
 	case "$1" in
 	--*)
-- 
2.8.2-507-g43e827d

Re: [PATCH v16 0/7] config commit verbose

[Eric Sunshine]

On Thu, May 5, 2016 at 3:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Pranit Bauva <pranit.bauva@gmail.com> writes:
>> This series of patches add a configuration variable for verbose in
>> git-commit.
>>
>> Changes wrt v15:
>>  * Remove the previous patch 7/7 and split the tests. Include one in
>>    initial patch 6/7. The other one is introduced in a separate commit
>>    after 4/7.
>>  * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,
>>    --no-verbose with -v as suggested by Eric Sunshine
>
> Thanks for a pleasant read.  Modulo minor readability nits I sent
> separately on 7/7, this looked good.

Agreed, this version was a more pleasant and coherent read than previous ones.

Considering that this series is already at v16 and the 7/7 review
comments were very minor, I'd be fine seeing this series land as-is,
rather than expecting v17.

Re: [PATCH v16 0/7] config commit verbose

[SZEDER Gábor]

Quoting Eric Sunshine <sunshine@sunshineco.com>:

> On Thu, May 5, 2016 at 3:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>> This series of patches add a configuration variable for verbose in
>>> git-commit.
>>>
>>> Changes wrt v15:
>>> * Remove the previous patch 7/7 and split the tests. Include one in
>>>   initial patch 6/7. The other one is introduced in a separate commit
>>>   after 4/7.
>>> * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,
>>>   --no-verbose with -v as suggested by Eric Sunshine
>>
>> Thanks for a pleasant read.  Modulo minor readability nits I sent
>> separately on 7/7, this looked good.
>
> Agreed, this version was a more pleasant and coherent read than  
> previous ones.
>
> Considering that this series is already at v16 and the 7/7 review
> comments were very minor, I'd be fine seeing this series land as-is,
> rather than expecting v17.

v16, wow, I totally lost track of this series, sorry.

And I hate to bring this up this late again... at v16 of a now 7 patch
series, when all this started out like two months ago as a GSoC mini
project...
But I do it anyway.  Oh well.

A while ago in a related thread Peff remarked about 'git commit's
'--quiet' and '--verbose' options:

    I think that is a UX mistake, and we would not do
    it that way if designing from scratch. But we're stuck with it for
    historical reasons (I'd probably name "--verbose" as "--show-diff" or
    something if writing it today).

http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289069

Then I replied:

    However, that doesn't mean that we have to spread this badly chosen
    name from options to config variables, does it?  I think that if we
    are going to define a new config variable today, then it should be
    named properly, and it's better not to call it 'commit.verbose', but
    'commit.showDiff' or something.

http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303

Any thoughts on this?  Before a poorly named config variable enters to
the codebase and we'll have to maintain it "forever"...

Re: [PATCH v16 0/7] config commit verbose

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> A while ago in a related thread Peff remarked about 'git commit's
> '--quiet' and '--verbose' options:
>
>    I think that is a UX mistake, and we would not do
>    it that way if designing from scratch. But we're stuck with it for
>    historical reasons (I'd probably name "--verbose" as "--show-diff" or
>    something if writing it today).
>
> http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289069
>
> Then I replied:
>
>    However, that doesn't mean that we have to spread this badly chosen
>    name from options to config variables, does it?  I think that if we
>    are going to define a new config variable today, then it should be
>    named properly, and it's better not to call it 'commit.verbose', but
>    'commit.showDiff' or something.
>
> http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303
>
> Any thoughts on this?  Before a poorly named config variable enters to
> the codebase and we'll have to maintain it "forever"...

My thoughts are --show-diff would probably be a UI mistake of a
different sort, if you are anticipating that the different kinds of
information to be shown in verbose modes would proliferate and that
you would want to give the user flexibility to pick and choose to
use some while not using some other among them.  You would end up
having --show-xyzzy --show-frotz --show-nitfol ... options.

I am not convinced that we would want such a degree of flexibility
in the first place, but even if we did, we'd be better off giving
that as "--verbose=diff,xyzzy,frotz...", I would think.

And commit.verbose that begins its life as a simple boolean, which
can be extended to become bool-or-string if needed, is better than
having commit.showDiff, commit.showXyzzy, commit.showFrotz, etc.

Re: [PATCH v16 0/7] config commit verbose

[Jeff King]

On Fri, May 06, 2016 at 08:33:15AM -0700, Junio C Hamano wrote:

> > Then I replied:
> >
> >    However, that doesn't mean that we have to spread this badly chosen
> >    name from options to config variables, does it?  I think that if we
> >    are going to define a new config variable today, then it should be
> >    named properly, and it's better not to call it 'commit.verbose', but
> >    'commit.showDiff' or something.
> >
> > http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303
> >
> > Any thoughts on this?  Before a poorly named config variable enters to
> > the codebase and we'll have to maintain it "forever"...
> 
> My thoughts are --show-diff would probably be a UI mistake of a
> different sort, if you are anticipating that the different kinds of
> information to be shown in verbose modes would proliferate and that
> you would want to give the user flexibility to pick and choose to
> use some while not using some other among them.  You would end up
> having --show-xyzzy --show-frotz --show-nitfol ... options.
> 
> I am not convinced that we would want such a degree of flexibility
> in the first place, but even if we did, we'd be better off giving
> that as "--verbose=diff,xyzzy,frotz...", I would think.
> 
> And commit.verbose that begins its life as a simple boolean, which
> can be extended to become bool-or-string if needed, is better than
> having commit.showDiff, commit.showXyzzy, commit.showFrotz, etc.

I don't think anyone is anticipating more "--show-" options. It is just
that "--verbose" is the opposite of "--quiet" in most other commands,
and pertains to chattiness on the terminal about what is going on.

Whereas in git-commit, is about sticking some data in the commit message
template. Naively I'd expect it to cause commit to spew more data to
stderr about what's being committed, ident info, etc.

If you are thinking that there could be something like "--show-ident" to
replace that, I do not mind that too much. But IMHO that does not
address the root problem that commit's "--verbose" is not very much like
the same option in other commands. And something like
"--verbose=diff,ident" just seems to make that worse by coupling options
that otherwise don't have anything to do with each other.

-Peff

Re: [PATCH v16 0/7] config commit verbose

[Ævar Arnfjörð Bjarmason]

On Sat, May 7, 2016 at 7:32 AM, Jeff King <peff@peff.net> wrote:
> On Fri, May 06, 2016 at 08:33:15AM -0700, Junio C Hamano wrote:
>
>> > Then I replied:
>> >
>> >    However, that doesn't mean that we have to spread this badly chosen
>> >    name from options to config variables, does it?  I think that if we
>> >    are going to define a new config variable today, then it should be
>> >    named properly, and it's better not to call it 'commit.verbose', but
>> >    'commit.showDiff' or something.
>> >
>> > http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303
>> >
>> > Any thoughts on this?  Before a poorly named config variable enters to
>> > the codebase and we'll have to maintain it "forever"...
>>
>> My thoughts are --show-diff would probably be a UI mistake of a
>> different sort, if you are anticipating that the different kinds of
>> information to be shown in verbose modes would proliferate and that
>> you would want to give the user flexibility to pick and choose to
>> use some while not using some other among them.  You would end up
>> having --show-xyzzy --show-frotz --show-nitfol ... options.
>>
>> I am not convinced that we would want such a degree of flexibility
>> in the first place, but even if we did, we'd be better off giving
>> that as "--verbose=diff,xyzzy,frotz...", I would think.
>>
>> And commit.verbose that begins its life as a simple boolean, which
>> can be extended to become bool-or-string if needed, is better than
>> having commit.showDiff, commit.showXyzzy, commit.showFrotz, etc.
>
> I don't think anyone is anticipating more "--show-" options. It is just
> that "--verbose" is the opposite of "--quiet" in most other commands,
> and pertains to chattiness on the terminal about what is going on.
>
> Whereas in git-commit, is about sticking some data in the commit message
> template. Naively I'd expect it to cause commit to spew more data to
> stderr about what's being committed, ident info, etc.
>
> If you are thinking that there could be something like "--show-ident" to
> replace that, I do not mind that too much. But IMHO that does not
> address the root problem that commit's "--verbose" is not very much like
> the same option in other commands. And something like
> "--verbose=diff,ident" just seems to make that worse by coupling options
> that otherwise don't have anything to do with each other.

I can see how it looks out of place looked at like that, but for me as
a long-time user (aren't we all?) it never felt out of place because
it's a more verbose version of the output that's brought up when I'm
modifying it.

I.e. I'm modifying the commit message, so the message is brought up,
optionally and more verbosely I can ask for the whole commit
(including diff) to amend the commit message.

I.e. I really expect --verbose to be a more verbose version of the
primary thing a command is doing, which in the case of "commit
--amend" is giving me info I need to modify the commit.

It also fits nicely with "status --verbose" showing a diff of staged
changes, similar to how --verbose for commit shows the diff for a
commit being amended.

Re: [PATCH v16 0/7] config commit verbose

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I.e. I really expect --verbose to be a more verbose version of the
> primary thing a command is doing, which in the case of "commit
> --amend" is giving me info I need to modify the commit.

That summarises what I wanted to say very well.  Thanks.

Re: [PATCH v16 0/7] config commit verbose

[Jeff King]

On Sun, May 08, 2016 at 11:48:31AM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > I.e. I really expect --verbose to be a more verbose version of the
> > primary thing a command is doing, which in the case of "commit
> > --amend" is giving me info I need to modify the commit.
> 
> That summarises what I wanted to say very well.  Thanks.

I guess I do not really consider the template content to be the primary
thing the command is doing. It is subjective, though. I don't feel
strongly enough to keep discussing it if other people don't agree.

-Peff

Re: [PATCH v16 0/7] config commit verbose

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I guess I do not really consider the template content to be the primary
> thing the command is doing. It is subjective, though. I don't feel
> strongly enough to keep discussing it if other people don't agree.

I just see the primary thing of what "commit -e" does is to help
users edit their log message (and view "-v" as giving more helping),
but I do agree with you that this is very subjective.

If we had these as either in broken-down form ("--show-diff",
"--show-diffstat", and "--show-untracked") or just a single
"--show-extra-info" option when we did the feature in the very
beginning, I do not think I'd feel that "--show-*" option(s) should
be renamed/redone to "--verbose".  So personally, my subjective
judgment is "'--verbose' and '--show-diff' would have been equally
valid, and it is OK to let whichever came first squat on the
feature."

Re: [PATCH v16 0/7] config commit verbose

[Pranit Bauva]

[+cc:git@vger.kernel.org] Because its an interesting fact to be shared
which isn't covered elsewhere.

On Fri, May 6, 2016 at 2:53 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Sending this privately since it's probably covered elsewhere. With
> this, if I set the option will "reword" in git rebase -i show me the
> patch?
>
> If so: awesome.

Yes, git rebase -i will show the diff in 'reword' if commit.verbose is
set to true or a value greater than 0.

I dug further in git-rebase--interactive.sh
I could find appearances of "git commit --amend" but I was unable to
find appearances of "COMMIT_EDITMSG". If COMMIT_EDITMSG was coming
into picture, the commit.verbose could not affect it. And that is not
the case.

I guess this would be a desirable trait for most of the consumers of
commit.verbose (like Ævar) so there would not be a need to suppress.

Regards,
Pranit Bauva

Re: [PATCH v16 0/7] config commit verbose

[Ævar Arnfjörð Bjarmason]

On Fri, May 6, 2016 at 6:16 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> [+cc:git@vger.kernel.org] Because its an interesting fact to be shared
> which isn't covered elsewhere.
>
> On Fri, May 6, 2016 at 2:53 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Sending this privately since it's probably covered elsewhere. With
>> this, if I set the option will "reword" in git rebase -i show me the
>> patch?
>>
>> If so: awesome.
>
> Yes, git rebase -i will show the diff in 'reword' if commit.verbose is
> set to true or a value greater than 0.
>
> I dug further in git-rebase--interactive.sh
> I could find appearances of "git commit --amend" but I was unable to
> find appearances of "COMMIT_EDITMSG". If COMMIT_EDITMSG was coming
> into picture, the commit.verbose could not affect it. And that is not
> the case.
>
> I guess this would be a desirable trait for most of the consumers of
> commit.verbose (like Ævar) so there would not be a need to suppress.

Yeah it's great, it's something I've wanted from interactive rebase
for a while now.

Re: [PATCH v16 0/7] config commit verbose

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, May 6, 2016 at 6:16 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> [+cc:git@vger.kernel.org] Because its an interesting fact to be shared
>> which isn't covered elsewhere.
>>
>> On Fri, May 6, 2016 at 2:53 AM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> Sending this privately since it's probably covered elsewhere. With
>>> this, if I set the option will "reword" in git rebase -i show me the
>>> patch?
>>>
>>> If so: awesome.
>>
>> Yes, git rebase -i will show the diff in 'reword' if commit.verbose is
>> set to true or a value greater than 0.
>>
>> I dug further in git-rebase--interactive.sh
>> I could find appearances of "git commit --amend" but I was unable to
>> find appearances of "COMMIT_EDITMSG". If COMMIT_EDITMSG was coming
>> into picture, the commit.verbose could not affect it. And that is not
>> the case.
>>
>> I guess this would be a desirable trait for most of the consumers of
>> commit.verbose (like Ævar) so there would not be a need to suppress.
>
> Yeah it's great, it's something I've wanted from interactive rebase
> for a while now.

I can see why "commit -v" may be useful during "rebase -i", but we
should also have rebase.verbose and "rebase -v".  I do not want to
make all my commits with -v, and I suspect I want to do "commit -v"
more often during "rebase -i" than regular commit, for example.