{"thread":{"id":"41906","subject":"[PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","startedAt":"2016-04-02T23:33:11Z","lastAt":"2016-04-12T23:11:45Z","messageCount":36,"participants":["Pranit Bauva","Eric Sunshine","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":12,"patchTotal":5},"messages":[{"id":"282536","messageId":"01020153d952be68-ee055619-664d-4502-92e9-6aa524e2354b-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com","subject":"[PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-02T23:33:11Z","receivedAt":"2016-04-02T23:33:11Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"The reason to make it respect \"unspecified\" values is to give the\nability to differentiate whether `--option` or `--no-option` was\nspecified at all. \"unspecified\" values should be in the form of negative\nvalues. If initial value is set to negative and `--option` specified\nthen it will reflect the number of occurrences (counting done from 0),\nif `--no-option` is specified then it will reflect 0 and if nothing at\nall is given then it will retain its negative value.\n\nThis change will not affect existing users of COUNTUP, because they all\nuse the initial value of 0 (or more).\n\nNote that builtin/clean.c initializes the variable used with\nOPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set\nto either 0 or 1 by reading the configuration before the code calls\nparse_options(), i.e. as far as parse_options() is concerned, the\ninitial value of the variable is not negative.\n\nTo test this behavior \"verbose\" is set to \"unspecified\" while quiet is\nset to 0 which will test the new behavior with all sets of values.\n\nHelped-by: Jeff King \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nSigned-off-by: Pranit Bauva \n\n---\nThe discussion about this patch:\n[1] : http://thread.gmane.org/gmane.comp.version-control.git/289027\n\nPrevious version of the patch:\n[v11] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v10] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v9] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061\n\nChanges wrt previous version (v11):\n - Use bits of commit message provided by Junio.\n\nPlease Note: The diff might seem improper especially the part where I\nhave introduced some continuous lines but this is a logical error by git\ndiff (nothing could be done about it) and thus the changes will be\nclearly visible with the original file itself.\n---\n Documentation/technical/api-parse-options.txt | 8 ++++--\n parse-options.c | 2 ++\n t/t0040-parse-options.sh | 39 ++++++++++++++++++++-------\n test-parse-options.c | 3 ++-\n 4 files changed, 39 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 5f0757d..8908bf7 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -144,8 +144,12 @@ There are some macros to easily define options:\n \n `OPT_COUNTUP(short, long, &int_var, description)`::\n \tIntroduce a count-up option.\n-\t`int_var` is incremented on each use of `--option`, and\n-\treset to zero with `--no-option`.\n+\tEach use of `--option` increments `int_var`, starting from zero\n+\t(even if initially negative), and `--no-option` resets it to\n+\tzero. To determine if `--option` or `--no-option` was set at\n+\tall, set `int_var` to a negative value, and if it is still\n+\tnegative after parse_options(), then neither `--option` nor\n+\t`--no-option` was seen.\n \n `OPT_BIT(short, long, &int_var, description, mask)`::\n \tIntroduce a boolean option.\ndiff --git a/parse-options.c b/parse-options.c\nindex 47a9192..312a85d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\treturn 0;\n \n \tcase OPTION_COUNTUP:\n+\t\tif (*(int *)opt->value < 0)\n+\t\t\t*(int *)opt->value = 0;\n \t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n \t\treturn 0;\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 302c315..bfd8dea 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -63,7 +63,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -211,7 +211,7 @@ magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -234,7 +234,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -263,7 +263,7 @@ magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -302,7 +302,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -322,7 +322,7 @@ magnitude: 0\n timestamp: 1\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 1\n dry run: no\n file: (not set)\n@@ -344,7 +344,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -373,7 +373,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -398,7 +398,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -429,7 +429,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -454,6 +454,25 @@ dry run: no\n file: (not set)\n EOF\n \n+test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '\n+\ttest-parse-options --no-verbose >output 2>output.err &&\n+\ttest_must_be_empty output.err &&\n+\ttest_cmp expect output\n+'\n+\n+cat >expect <output 2>output.err &&\n \ttest_must_be_empty output.err &&\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 86afa98..f02c275 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -7,7 +7,8 @@ static int integer = 0;\n static unsigned long magnitude = 0;\n static unsigned long timestamp;\n static int abbrev = 7;\n-static int verbose = 0, dry_run = 0, quiet = 0;\n+static int verbose = -1; /* unspecified */\n+static int dry_run = 0, quiet = 0;\n static char *string = NULL;\n static char *file = NULL;\n static int ambiguous;\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"282537","messageId":"01020153d952be64-9ca893f1-5a7b-4a2c-a0bd-98abf93a17ac-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com","subject":"[PATCH v12 2/5] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-02T23:33:11Z","receivedAt":"2016-04-02T23:33:11Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Current implementation of parse-options.c treats OPT__QUIET() as integer\nand not boolean and thus it is more appropriate to print it as integer\nto avoid confusion.\n\nSigned-off-by: Pranit Bauva \n---\n t/t0040-parse-options.sh | 26 +++++++++++++-------------\n test-parse-options.c | 2 +-\n 2 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex c6f205b..302c315 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -64,7 +64,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -164,7 +164,7 @@ timestamp: 0\n string: 123\n abbrev: 7\n verbose: 2\n-quiet: no\n+quiet: 0\n dry run: yes\n file: prefix/my.file\n EOF\n@@ -184,7 +184,7 @@ timestamp: 0\n string: 321\n abbrev: 10\n verbose: 2\n-quiet: no\n+quiet: 0\n dry run: no\n file: prefix/fi.le\n EOF\n@@ -212,7 +212,7 @@ timestamp: 0\n string: 123\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n arg 00: a1\n@@ -235,7 +235,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -264,7 +264,7 @@ timestamp: 0\n string: 123\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -303,7 +303,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n arg 00: --quux\n@@ -323,7 +323,7 @@ timestamp: 1\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: yes\n+quiet: 1\n dry run: no\n file: (not set)\n arg 00: foo\n@@ -345,7 +345,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -374,7 +374,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -399,7 +399,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -430,7 +430,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -449,7 +449,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 2c8c8f1..86afa98 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -90,7 +90,7 @@ int main(int argc, char **argv)\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n \tprintf(\"abbrev: %d\\n\", abbrev);\n \tprintf(\"verbose: %d\\n\", verbose);\n-\tprintf(\"quiet: %s\\n\", quiet ? \"yes\" : \"no\");\n+\tprintf(\"quiet: %d\\n\", quiet);\n \tprintf(\"dry run: %s\\n\", dry_run ? \"yes\" : \"no\");\n \tprintf(\"file: %s\\n\", file ? file : \"(not set)\");\n \n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"282538","messageId":"01020153d952be6c-04f90bd2-291e-4eca-99fd-e897d8601132-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com","subject":"[PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-02T23:33:11Z","receivedAt":"2016-04-02T23:33:11Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Make the fake \"editor\" store output of grep in a file so that we can\nsee how many diffs were contained in the message and use them in\nindividual tests where ever it is required. Also use write_script()\nto create the fake \"editor\".\n\nA subsequent commit will introduce scenarios where it is important to be\nable to exactly determine how many diffs were present.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Pranit Bauva \n\n---\nPrevious version of this patch:\n - [v11] : $gmane/28820\n - [v10]: $gmane/288820\n\nChanges this version wrt previous one:\nInclude the triple dash which I previously forgot, pointed out by Junio.\n---\n t/t7507-commit-verbose.sh | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..0f28a86 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -3,11 +3,10 @@\n test_description='verbose commit template'\n . ./test-lib.sh\n \n-cat >check-for-diff <out\n+exit 0\n EOF\n-chmod +x check-for-diff\n test_set_editor \"$PWD/check-for-diff\"\n \n cat >message <<'EOF'\n@@ -23,7 +22,8 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'initial commit shows verbose diff' '\n-\tgit commit --amend -v\n+\tgit commit --amend -v &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'second commit' '\n@@ -39,13 +39,15 @@ check_message() {\n \n test_expect_success 'verbose diff is stripped out' '\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n \tgit config diff.mnemonicprefix true &&\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n cat >diff <<'EOF'\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"282539","messageId":"01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com","subject":"[PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-02T23:33:11Z","receivedAt":"2016-04-02T23:33:11Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Signed-off-by: Pranit Bauva \n\n---\nChanges wrt previous version (v11):\n - This patch is a split up of patch 1/4 v11 as requested by Junio.\n - This patch uses the backslash with EOF as suggested by Junio\n for 2 tests namely \"detect possible typos\"\n---\n t/t0040-parse-options.sh | 72 ++++++++++++++++++++++++------------------------\n 1 file changed, 36 insertions(+), 36 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 9be6411..c6f205b 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -7,7 +7,7 @@ test_description='our own option parser'\n \n . ./test-lib.sh\n \n-cat > expect << EOF\n+cat >expect <\n \n --yes get a boolean\n@@ -49,7 +49,7 @@ Standard options\n EOF\n \n test_expect_success 'test help' '\n-\ttest_must_fail test-parse-options -h > output 2> output.err &&\n+\ttest_must_fail test-parse-options -h >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_i18ncmp expect output\n '\n@@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '\n \tcheck magnitude: 3221225472 -m 3g\n '\n \n-cat > expect << EOF\n+cat >expect < expect << EOF\n+cat >expect < expect << EOF\n+cat >expect < output 2> output.err &&\n+\t\t>output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect << EOF\n+cat >expect < output 2> output.err &&\n+\ttest-parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'unambiguously abbreviated option with \"=\"' '\n-\ttest-parse-options --int=2 > output 2> output.err &&\n+\ttest-parse-options --int=2 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n@@ -256,7 +256,7 @@ test_expect_success 'ambiguously abbreviated option' '\n \ttest_expect_code 129 test-parse-options --strin 123\n '\n \n-cat > expect << EOF\n+cat >expect < output 2> output.err &&\n+\ttest-parse-options --st 123 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > typo.err << EOF\n-error: did you mean \\`--boolean\\` (with two dashes ?)\n+cat >typo.err <<\\EOF\n+error: did you mean `--boolean` (with two dashes ?)\n EOF\n \n test_expect_success 'detect possible typos' '\n-\ttest_must_fail test-parse-options -boolean > output 2> output.err &&\n+\ttest_must_fail test-parse-options -boolean >output 2>output.err &&\n \ttest_must_be_empty output &&\n \ttest_cmp typo.err output.err\n '\n \n-cat > typo.err << EOF\n-error: did you mean \\`--ambiguous\\` (with two dashes ?)\n+cat >typo.err <<\\EOF\n+error: did you mean `--ambiguous` (with two dashes ?)\n EOF\n \n test_expect_success 'detect possible typos' '\n-\ttest_must_fail test-parse-options -ambiguous > output 2> output.err &&\n+\ttest_must_fail test-parse-options -ambiguous >output 2>output.err &&\n \ttest_must_be_empty output &&\n \ttest_cmp typo.err output.err\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options --quux >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n- test_cmp expect output\n+\ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\t\tfoo -q >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options --length=four -b -4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest_must_fail test-parse-options --no-length >output 2>output.err &&\n \ttest_i18ncmp expect output &&\n \ttest_i18ncmp expect.err output.err\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options --set23 -bbbbb --no-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '\n-\ttest-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err &&\n+\ttest-parse-options --set23 -bbbbb --neg-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options -bb --or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_NEGBIT() works' '\n-\ttest-parse-options -bb --no-neg-or4 > output 2> output.err &&\n+\ttest-parse-options -bb --no-neg-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n-\ttest-parse-options + + + + + + > output 2> output.err &&\n+\ttest-parse-options + + + + + + >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options -12345 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n@@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n \ttest_cmp expect output\n '\n \n-cat >>expect <<'EOF'\n+cat >>expect <\nHelped-by: Eric Sunshine \nSigned-off-by: Pranit Bauva \n\n---\nThe previous version of the patch are:\n - [v11] $gmane/288820\n - [v10] $gmane/288820\n - [v9] $gmane/288820\n - [v8] $gmane/288820\n - [v7] $gmane/288820\n - [v6] $gmane/288728\n - [v5] $gmane/288728\n - [v4] $gmane/288652\n - [v3] $gmane/288634\n - [v2] $gmane/288569\n - [v1] $gmane/287540\n\n Note: One might think some tests are extra but I think that it will\n be better to include them as they \"complete the continuity\" thus\n generalising the series which will make the patch even more clearer.\n---\n Documentation/config.txt | 4 +\n Documentation/git-commit.txt | 3 +-\n builtin/commit.c | 14 +++-\n t/t7507-commit-verbose.sh | 175 +++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 194 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2cd6bdd..1d0ec2e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1110,6 +1110,10 @@ commit.template::\n \t\"`~/`\" is expanded to the value of `$HOME` and \"`~user/`\" to the\n \tspecified user's home directory.\n \n+commit.verbose::\n+\tA boolean or int to specify the level of verbose with `git commit`.\n+\tSee linkgit:git-commit[1].\n+\n credential.helper::\n \tSpecify an external helper to be called when a username or\n \tpassword credential is needed; the helper may consult external\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 9ec6b3c..d474226 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].\n \twhat changes the commit has.\n \tNote that this diff output doesn't have its\n \tlines prefixed with '#'. This diff will not be a part\n-\tof the commit message.\n+\tof the commit message. See the `commit.verbose` configuration\n+\tvariable in linkgit:git-config[1].\n +\n If specified twice, show in addition the unified diff between\n what would be committed and the worktree files, i.e. the unstaged\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b3bd2d4..96e6190 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -113,7 +113,9 @@ static char *edit_message, *use_message;\n static char *fixup_message, *squash_message;\n static int all, also, interactive, patch_interactive, only, amend, signoff;\n static int edit_flag = -1; /* unspecified */\n-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n+static int config_verbose = -1; /* unspecified */\n+static int verbose = -1; /* unspecified */\n+static int quiet, no_verify, allow_empty, dry_run, renew_authorship;\n static int no_post_rewrite, allow_empty_message;\n static char *untracked_files_arg, *force_date, *ignore_submodule_arg;\n static char *sign_commit;\n@@ -1354,6 +1356,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_status_usage, 0);\n \tfinalize_colopts(&s.colopts, -1);\n \tfinalize_deferred_config(&s);\n+\tif (verbose == -1)\n+\t\tverbose = 0;\n \n \thandle_untracked_files_arg(&s);\n \tif (show_ignored_in_status)\n@@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n \t\tsign_commit = git_config_bool(k, v) ? \"\" : NULL;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"commit.verbose\")) {\n+\t\tint is_bool;\n+\t\tconfig_verbose = git_config_bool_or_int(k, v, &is_bool);\n+\t\treturn 0;\n+\t}\n \n \tstatus = git_gpg_config(k, v, NULL);\n \tif (status)\n@@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \targc = parse_and_validate_options(argc, argv, builtin_commit_options,\n \t\t\t\t\t builtin_commit_usage,\n \t\t\t\t\t prefix, current_head, &s);\n+\tif (verbose == -1)\n+\t\tverbose = (config_verbose < 0) ? 0 : config_verbose;\n+\n \tif (dry_run)\n \t\treturn dry_run_commit(argc, argv, prefix, current_head, &s);\n \tindex_file = prepare_index(argc, argv, prefix, current_head, 0);\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 0f28a86..7c79484 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -98,4 +98,179 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n '\n \n+test_expect_success 'set up -v -v' '\n+\techo dirty >file &&\n+\techo dirty >file2 &&\n+\tgit add file2\n+'\n+test_expect_success 'commit.verbose true and --verbose omitted' '\n+\tgit -c commit.verbose=true commit -F message &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose true and --verbose' '\n+\tgit -c commit.verbose=true commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose true and -v -v' '\n+\tgit -c commit.verbose=true commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose true and --no-verbose' '\n+\tgit -c commit.verbose=true commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose' '\n+\tgit -c commit.verbose=false commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose false and -v -v' '\n+\tgit -c commit.verbose=false commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose omitted' '\n+\tgit -c commit.verbose=false commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose false and --no-verbose' '\n+\tgit -c commit.verbose=false commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and --verbose omitted' '\n+\tgit -c commit.verbose=-2 commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and --verbose omitted' '\n+\tgit -c commit.verbose=-1 commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and --verbose omitted' '\n+\tgit -c commit.verbose=0 commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and --verbose omitted' '\n+\tgit -c commit.verbose=1 commit --amend &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and --verbose omitted' '\n+\tgit -c commit.verbose=2 commit --amend &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit-verbose=3 and --verbose omitted' '\n+\tgit -c commit.verbose=3 commit --amend &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and --verbose' '\n+\tgit -c commit.verbose=-2 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and --verbose' '\n+\tgit -c commit.verbose=-1 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and --verbose' '\n+\tgit -c commit.verbose=0 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and --verbose' '\n+\tgit -c commit.verbose=1 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and --verbose' '\n+\tgit -c commit.verbose=2 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=3 and --verbose' '\n+\tgit -c commit.verbose=3 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and -v -v' '\n+\tgit -c commit.verbose=-2 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and -v -v' '\n+\tgit -c commit.verbose=-1 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and -v -v' '\n+\tgit -c commit.verbose=0 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and -v -v' '\n+\tgit -c commit.verbose=1 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and -v -v' '\n+\tgit -c commit.verbose=2 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=3 and -v -v' '\n+\tgit -c commit.verbose=3 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and --no-verbose' '\n+\tgit -c commit.verbose=-2 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and --no-verbose' '\n+\tgit -c commit.verbose=-1 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and --no-verbose' '\n+\tgit -c commit.verbose=0 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and --no-verbose' '\n+\tgit -c commit.verbose=1 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and --no-verbose' '\n+\tgit -c commit.verbose=2 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=3 and --no-verbose' '\n+\tgit -c commit.verbose=3 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'status ignores commit.verbose=true' '\n+\tgit -c commit.verbose=true status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n+test_expect_success 'status does not verbose without --verbose' '\n+\tgit status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n test_done\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"282577","messageId":"CAPig+cQ+89MZSyHJhCrZsGMc7mM_uorE5j9GXk7kCTT1OTXV6Q@mail.gmail.com","threadId":"41906","inReplyTo":"01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-03T21:00:22Z","receivedAt":"2016-04-03T21:00:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n> Signed-off-by: Pranit Bauva \n> ---\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> @@ -7,7 +7,7 @@ test_description='our own option parser'\n>\n> . ./test-lib.sh\n>\n> -cat > expect << EOF\n> +cat >expect < usage: test-parse-options \n> --yes get a boolean\n\nIt would be better to use <<\\EOF for this one to make it clear that no\ninterpolation is desired in the heredoc.\n\n> @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '\n> check magnitude: 3221225472 -m 3g\n> '\n>\n> -cat > expect << EOF\n> +cat >expect < boolean: 2\n> integer: 1729\n> magnitude: 16384\n> @@ -310,12 +310,12 @@ arg 00: --quux\n> EOF\n>\n> test_expect_success 'keep some options as arguments' '\n> - test-parse-options --quux > output 2> output.err &&\n> + test-parse-options --quux >output 2>output.err &&\n> test_must_be_empty output.err &&\n> - test_cmp expect output\n> + test_cmp expect output\n\nOkay, this is a whitespace change (spaces -> tab).\n\n> '\n> @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n> -cat >>expect <<'EOF'\n> +cat >>expect < list: foo\n> list: bar\n> list: baz\n"},{"id":"282578","messageId":"CAPig+cQpdr7A7GxKUN3tDs2rQ7z_S5b1WVsk01=BwkCwB2mp0A@mail.gmail.com","threadId":"41906","inReplyTo":"01020153d952be64-9ca893f1-5a7b-4a2c-a0bd-98abf93a17ac-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v12 2/5] test-parse-options: print quiet as integer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-03T21:30:38Z","receivedAt":"2016-04-03T21:30:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n> Current implementation of parse-options.c treats OPT__QUIET() as integer\n> and not boolean and thus it is more appropriate to print it as integer\n> to avoid confusion.\n\nI can buy this line of reasoning, however, it would be even easier to\nsell the change if you cited an existing client (a git command) which\nactually respects multiple quiet levels. Are there any?\n\nMore importantly, though, this change implies that you should also add\ntests to ensure that the quiet level is indeed incremented with each\n--quiet, just as \"-vv\" and \"--verbose --verbose\" are already tested.\nYou might be able to include such new tests directly in this patch as\nlong as the commit message is clear about it, or add them in a\nseparate patch.\n\nBy the way, I don't see any tests to ensure that --no-verbose and\n--no-quiet reset those respective values to 0. A separate patch which\nadds such tests would be nice (unless such tests already exist and I\nmerely missed them).\n\n> Signed-off-by: Pranit Bauva \n> ---\n> t/t0040-parse-options.sh | 26 +++++++++++++-------------\n> test-parse-options.c | 2 +-\n> 2 files changed, 14 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index c6f205b..302c315 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -64,7 +64,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -164,7 +164,7 @@ timestamp: 0\n> string: 123\n> abbrev: 7\n> verbose: 2\n> -quiet: no\n> +quiet: 0\n> dry run: yes\n> file: prefix/my.file\n> EOF\n> @@ -184,7 +184,7 @@ timestamp: 0\n> string: 321\n> abbrev: 10\n> verbose: 2\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: prefix/fi.le\n> EOF\n> @@ -212,7 +212,7 @@ timestamp: 0\n> string: 123\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> arg 00: a1\n> @@ -235,7 +235,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -264,7 +264,7 @@ timestamp: 0\n> string: 123\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -303,7 +303,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> arg 00: --quux\n> @@ -323,7 +323,7 @@ timestamp: 1\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: yes\n> +quiet: 1\n> dry run: no\n> file: (not set)\n> arg 00: foo\n> @@ -345,7 +345,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -374,7 +374,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -399,7 +399,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -430,7 +430,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -449,7 +449,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 2c8c8f1..86afa98 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -90,7 +90,7 @@ int main(int argc, char **argv)\n> printf(\"string: %s\\n\", string ? string : \"(not set)\");\n> printf(\"abbrev: %d\\n\", abbrev);\n> printf(\"verbose: %d\\n\", verbose);\n> - printf(\"quiet: %s\\n\", quiet ? \"yes\" : \"no\");\n> + printf(\"quiet: %d\\n\", quiet);\n> printf(\"dry run: %s\\n\", dry_run ? \"yes\" : \"no\");\n> printf(\"file: %s\\n\", file ? file : \"(not set)\");\n"},{"id":"282585","messageId":"CAPig+cS23i9XFTntLTHRbNf21DxhLAcw+9ent_ZtWj=1aO7JwQ@mail.gmail.com","threadId":"41906","inReplyTo":"01020153d952be68-ee055619-664d-4502-92e9-6aa524e2354b-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-03T23:10:01Z","receivedAt":"2016-04-03T23:10:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n> The reason to make it respect \"unspecified\" values is to give the\n> ability to differentiate whether `--option` or `--no-option` was\n> specified at all. \"unspecified\" values should be in the form of negative\n> values. If initial value is set to negative and `--option` specified\n> then it will reflect the number of occurrences (counting done from 0),\n> if `--no-option` is specified then it will reflect 0 and if nothing at\n> all is given then it will retain its negative value.\n\nThanks, this rewrite does a better job of explaining the aim of the\nchange and how a client can take advantage of it. However, with my\n\"first-time reader\" hat on, I still have some trouble digesting it as\na coherent whole. I wonder if a rewrite like this would help?\n\n OPT_COUNTUP() merely increments the counter upon --option, and\n resets it to 0 upon --no-option, which means that there is no\n \"unspecified\" value with which a client can initialize the\n counter to determine whether or not --[no-]option was seen at\n all.\n\n Make OPT_COUNTUP() treat any negative number as an \"unspecified\"\n value to address this shortcoming. In particular, if a client\n initializes the counter to -1, then if it is still -1 after\n parse_options(), then neither --option nor --no-option was seen;\n if it is 0, then --no-option was seen last, and if it is 1 or\n greater, than --option was seen last.\n\n> This change will not affect existing users of COUNTUP, because they all\n> use the initial value of 0 (or more).\n\n \"This change does not affect behavior of existing clients of...\"\n\n> Note that builtin/clean.c initializes the variable used with\n> OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set\n> to either 0 or 1 by reading the configuration before the code calls\n> parse_options(), i.e. as far as parse_options() is concerned, the\n> initial value of the variable is not negative.\n>\n> To test this behavior \"verbose\" is set to \"unspecified\" while quiet is\n> set to 0 which will test the new behavior with all sets of values.\n\nI think you need to mention here that you're talking about test-parse-options.c\n(and indirectly t0040) since it's otherwise too easy for the reader to\nthink that this paragraph is a continuation of the discussion about\nOPT_COUNTUP()'s new behavior and how it won't impact existing tests,\nrather than a new topic of its own (testing the behavior). Maybe say\nsomething like this:\n\n To test the new behavior, augment the initial \"verbose\" setting\n of test-parse-options.c to be unspecified...\n\nand then go on to say that, because \"quiet\" is still initialized to 0,\nyou have complete coverage. An alternative would be to split off the\nnew testing into its own patch, which would make this patch (which is\nthe real meat of the change) less noisy.\n\nI actually expected you to add new functionality to\ntest-parse-options.c specifically to test OPT_COUNTUP() directly\nrather than indirectly through --verbose and --quiet, however, I think\nI can be sold on the approach you took for a couple reasons. First,\nyou have a genuine use-case for an \"unspecified\" --verbose value, so\nchanging test-parse-options.c's --verbose to start at -1 tests what\nyou ultimately care about. Second, since you retained 0-initialization\nof --quiet, that case of OPT_COUNTUP() is still being tested.\n\nWhat I find a bit disturbing about this approach is that you are\ngetting \"full coverage\" only because of current *implementation*, not\nbecause you are explicitly testing for *expected* behavior. That is,\nyou get that coverage only while both OPT__VERBOSE() and OPT__QUIET()\nare built atop OPT_COUNTUP(); if OPT__QUIET() is ever rewritten so it\nno longer uses OPT_COUNTUP(), then the tests silently lose full\ncoverage. However, I may be over-worrying about the situation...\n\n> Signed-off-by: Pranit Bauva \n> ---\n> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n> @@ -144,8 +144,12 @@ There are some macros to easily define options:\n> `OPT_COUNTUP(short, long, &int_var, description)`::\n> Introduce a count-up option.\n> - `int_var` is incremented on each use of `--option`, and\n> - reset to zero with `--no-option`.\n> + Each use of `--option` increments `int_var`, starting from zero\n> + (even if initially negative), and `--no-option` resets it to\n> + zero. To determine if `--option` or `--no-option` was set at\n\ns/was set/was encountered/\n\n> + all, set `int_var` to a negative value, and if it is still\n\ns/set `int_var`/initialize `int_var`/\n\n> + negative after parse_options(), then neither `--option` nor\n> + `--no-option` was seen.\n> diff --git a/parse-options.c b/parse-options.c\n> @@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n> case OPTION_COUNTUP:\n> + if (*(int *)opt->value < 0)\n> + *(int *)opt->value = 0;\n> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n\nThis is nicer.\n\n> return 0;\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> @@ -454,6 +454,25 @@ dry run: no\n> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '\n> + test-parse-options --no-verbose >output 2>output.err &&\n> + test_must_be_empty output.err &&\n> + test_cmp expect output\n> +'\n\nIf you take the advice of my 2/5 review and add new tests (in a new\npatch) which test --no-verbose and --no-quiet, then I think this new\ntest here can just go away since the case it cares about will already\nbe covered.\n"},{"id":"282586","messageId":"CAPig+cRBwOjxA2cB=ZR__o7LsHxk9Rbzfv7ngRxv8h6=koQSRw@mail.gmail.com","threadId":"41906","inReplyTo":"01020153d952be6c-04f90bd2-291e-4eca-99fd-e897d8601132-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T00:02:56Z","receivedAt":"2016-04-04T00:02:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n> Make the fake \"editor\" store output of grep in a file so that we can\n> see how many diffs were contained in the message and use them in\n> individual tests where ever it is required. Also use write_script()\n> to create the fake \"editor\".\n>\n> A subsequent commit will introduce scenarios where it is important to be\n> able to exactly determine how many diffs were present.\n\nThese two sentences are backward. The \"subsequent commit\" bit is\njustification for why you are making the \"editor\" store the output,\nthus it belongs with the first paragraph. The bit about write_script()\nis just a minor aside which can go in its own paragraph.\n\nI think it's also important to explain that you're changing the\nbehavior of write_script() so that it always succeeds, regardless of\nwhether grep found diff headers or not, and to give the reason for\nmaking this change (\"so that you don't have to use 'test_must_fail'\nfor cases when no diff headers are expected and can instead easily use\n'test_line_count = 0'\").\n\nThe patch itself looks fine and is easy enough to read.\n\n> Helped-by: Eric Sunshine \n> Signed-off-by: Pranit Bauva \n> ---\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 2ddf28c..0f28a86 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -3,11 +3,10 @@\n> test_description='verbose commit template'\n> . ./test-lib.sh\n>\n> -cat >check-for-diff < -#!$SHELL_PATH\n> -exec grep '^diff --git' \"\\$1\"\n> +write_script \"check-for-diff\" <<\\EOF &&\n> +grep '^diff --git' \"$1\" >out\n> +exit 0\n> EOF\n> -chmod +x check-for-diff\n> test_set_editor \"$PWD/check-for-diff\"\n>\n> cat >message <<'EOF'\n> @@ -23,7 +22,8 @@ test_expect_success 'setup' '\n> '\n>\n> test_expect_success 'initial commit shows verbose diff' '\n> - git commit --amend -v\n> + git commit --amend -v &&\n> + test_line_count = 1 out\n> '\n>\n> test_expect_success 'second commit' '\n> @@ -39,13 +39,15 @@ check_message() {\n>\n> test_expect_success 'verbose diff is stripped out' '\n> git commit --amend -v &&\n> - check_message message\n> + check_message message &&\n> + test_line_count = 1 out\n> '\n>\n> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n> git config diff.mnemonicprefix true &&\n> git commit --amend -v &&\n> - check_message message\n> + check_message message &&\n> + test_line_count = 1 out\n> '\n"},{"id":"282587","messageId":"CAPig+cTYHFyDXJ2ZECtje2OG-ObT04gnQw+0+LbMMVO51bx2fg@mail.gmail.com","threadId":"41906","inReplyTo":"01020153d952be81-1859e6e4-81e4-473f-a2fa-7e27305b4e3e-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v12 5/5] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T00:58:12Z","receivedAt":"2016-04-04T00:58:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n> Add commit.verbose configuration variable as a convenience for those\n> who always prefer --verbose.\n>\n> Signed-off-by: Pranit Bauva \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -1110,6 +1110,10 @@ commit.template::\n> +commit.verbose::\n> + A boolean or int to specify the level of verbose with `git commit`.\n> + See linkgit:git-commit[1].\n\ns/level of verbose with/verbosity level of/\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n> + if (!strcmp(k, \"commit.verbose\")) {\n> + int is_bool;\n> + config_verbose = git_config_bool_or_int(k, v, &is_bool);\n> + return 0;\n> + }\n> @@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> argc = parse_and_validate_options(argc, argv, builtin_commit_options,\n> builtin_commit_usage,\n> prefix, current_head, &s);\n> + if (verbose == -1)\n> + verbose = (config_verbose < 0) ? 0 : config_verbose;\n\nOkay, so this version compares 'config_verbose' with \"< 0\" rather than\n\"== -1\" so it won't misbehave when someone sets config.verbose=-2 as\npointed out in [1]. Good.\n\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> @@ -98,4 +98,179 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n> +test_expect_success 'set up -v -v' '\n\nStyle: \"setup -v -v\", not \"set up -v -v\"\n\n> + echo dirty >file &&\n> + echo dirty >file2 &&\n> + git add file2\n> +'\n\nWhy this extra complexity when all you need for the \"-v -v\" case is:\n\n test_expect_success 'setup -v -v' '\n echo dirty >file\n '\n\nas suggested by [1]? Try to keep unnecessary gunk out of the tests\n(and code, in general) to avoid misleading readers into wondering if\nsomething unusual is going on. Which leads to...\n\n> +test_expect_success 'commit.verbose true and --verbose omitted' '\n> + git -c commit.verbose=true commit -F message &&\n> + test_line_count = 1 out\n> +'\n\nWhy is this new test different (by using \"-F message\") from all other\nnew tests which simply use \"--amend\"? The answer is that it is\nperforming a \"setup\" action which should have been done in the \"setup\"\ntest just above. But, as noted, this extra setup is unnecessary, thus\nalso misleading and confusing for readers. Better would be to use the\nminimal \"setup\" shown above (from [1]), and restore this test to use\n\"--amend\" like all other tests.\n\nMore below...\n\n> +test_expect_success 'commit.verbose true and --verbose' '\n> + git -c commit.verbose=true commit --amend --verbose &&\n> + test_line_count = 1 out\n> +'\n> +\n> +test_expect_success 'commit.verbose true and -v -v' '\n> + git -c commit.verbose=true commit --amend -v -v &&\n> + test_line_count = 2 out\n> +'\n> [...large number of almost identical tests omitted...]\n> +\n> +test_expect_success 'commit.verbose=-2 and --no-verbose' '\n> + git -c commit.verbose=-2 commit --amend --no-verbose &&\n> + test_line_count = 0 out\n> +'\n> +\n> +test_expect_success 'commit.verbose=-1 and --no-verbose' '\n> + git -c commit.verbose=-1 commit --amend --no-verbose &&\n> + test_line_count = 0 out\n> +'\n> +\n> +test_expect_success 'commit.verbose=0 and --no-verbose' '\n> + git -c commit.verbose=0 commit --amend --no-verbose &&\n> + test_line_count = 0 out\n> +'\n> +\n> +test_expect_success 'commit.verbose=1 and --no-verbose' '\n> + git -c commit.verbose=1 commit --amend --no-verbose &&\n> + test_line_count = 0 out\n> +'\n\nThe fact that the 32 new tests are nearly identical suggests strongly\nthat the testing should instead either be table-driven or be done via\nfor-loops to systematically cover all cases. Not only would either of\nthese approaches be easier to digest, but they would make it easy to\ntell at a glance if any cases were missing. See [2] for an example of\nhow the tests can be table-driven, and see the bottom of [3] for an\nexample of using for-loops to test systematically (though you'd need\nto use nested for-loops rather than just the one in the example).\n\nI'm leaning toward systematic testing via nested for-loops as the more\nsuitable of the two approach for this particular application.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/290000\n[2]: http://article.gmane.org/gmane.comp.version-control.git/290344\n[3]: http://article.gmane.org/gmane.comp.version-control.git/289860\n"},{"id":"282588","messageId":"CAPig+cQGeqvmTz1SdpJG137PgR=bC0d_Nu_q_ofhZM1BCYoxjQ@mail.gmail.com","threadId":"41906","inReplyTo":"CAPig+cRBwOjxA2cB=ZR__o7LsHxk9Rbzfv7ngRxv8h6=koQSRw@mail.gmail.com","subject":"Re: [PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T01:05:09Z","receivedAt":"2016-04-04T01:05:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 3, 2016 at 8:02 PM, Eric Sunshine wrote:\n> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>> Make the fake \"editor\" store output of grep in a file so that we can\n>> see how many diffs were contained in the message and use them in\n>> individual tests where ever it is required. Also use write_script()\n>> to create the fake \"editor\".\n>>\n>> A subsequent commit will introduce scenarios where it is important to be\n>> able to exactly determine how many diffs were present.\n>\n> These two sentences are backward. The \"subsequent commit\" bit is\n> justification for why you are making the \"editor\" store the output,\n> thus it belongs with the first paragraph. The bit about write_script()\n> is just a minor aside which can go in its own paragraph.\n>\n> I think it's also important to explain that you're changing the\n> behavior of write_script() so that it always succeeds, regardless of\n\ns/write_script()/the fake \"editor\"/\n\n> whether grep found diff headers or not, and to give the reason for\n> making this change (\"so that you don't have to use 'test_must_fail'\n> for cases when no diff headers are expected and can instead easily use\n> 'test_line_count = 0'\").\n"},{"id":"282611","messageId":"CAFZEwPNaxFyFmCkvPjRZQ0grOm1oWB1+5QhkpOMyEKW7j_99qQ@mail.gmail.com","threadId":"41906","inReplyTo":"CAPig+cQ+89MZSyHJhCrZsGMc7mM_uorE5j9GXk7kCTT1OTXV6Q@mail.gmail.com","subject":"Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-04T12:45:12Z","receivedAt":"2016-04-04T12:45:12Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Mon, Apr 4, 2016 at 2:30 AM, Eric Sunshine wrote:\n> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>> Signed-off-by: Pranit Bauva \n>> ---\n>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n>> @@ -7,7 +7,7 @@ test_description='our own option parser'\n>>\n>> . ./test-lib.sh\n>>\n>> -cat > expect << EOF\n>> +cat >expect <> usage: test-parse-options \n>> --yes get a boolean\n>\n> It would be better to use <<\\EOF for this one to make it clear that no\n> interpolation is desired in the heredoc.\n>\n>> @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '\n>> check magnitude: 3221225472 -m 3g\n>> '\n>>\n>> -cat > expect << EOF\n>> +cat >expect <\n> Ditto: <<\\EOF\n>\n> Same applies to all similar heredocs in subsequent tests where\n> interpolation is not desired.\n>\n>> boolean: 2\n>> integer: 1729\n>> magnitude: 16384\n>> @@ -310,12 +310,12 @@ arg 00: --quux\n>> EOF\n>>\n>> test_expect_success 'keep some options as arguments' '\n>> - test-parse-options --quux > output 2> output.err &&\n>> + test-parse-options --quux >output 2>output.err &&\n>> test_must_be_empty output.err &&\n>> - test_cmp expect output\n>> + test_cmp expect output\n>\n> Okay, this is a whitespace change (spaces -> tab).\n>\n>> '\n>> @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n>> -cat >>expect <<'EOF'\n>> +cat >>expect <\n> This is not a desirable change. This heredoc does not require\n> interpolation, so you don't want to turn it into a form which does\n> interpolate. For style consistency, therefore, you should change 'EOF'\n> to \\EOF.\n>\n>> list: foo\n>> list: bar\n>> list: baz\n\nOkay I will do the change. I was previously unaware about the use of\n'\\' before EOF. I googled it now. But I am still confused about its\nuse in this scenario. Upto what I understood, it is used where you\nwant to expand a variable, substitute a command, arithmethic\nexpansion. The use of '\\' in the tests I have changed in v12 wrt 11 is\nunderstood by me as you want to remove the use of escape sequences\nwhich is justified. But this seems a bit vague. Is it some convention\nin git?\n"},{"id":"282626","messageId":"CAPig+cQw=aG0NOgEsmdTGc7v3iBA-qG9_C4EdSm=6PcE6Qn-uQ@mail.gmail.com","threadId":"41906","inReplyTo":"CAFZEwPNaxFyFmCkvPjRZQ0grOm1oWB1+5QhkpOMyEKW7j_99qQ@mail.gmail.com","subject":"Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-04T17:30:36Z","receivedAt":"2016-04-04T17:30:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 4, 2016 at 8:45 AM, Pranit Bauva wrote:\n> Okay I will do the change. I was previously unaware about the use of\n> '\\' before EOF. I googled it now. But I am still confused about its\n> use in this scenario. Upto what I understood, it is used where you\n> want to expand a variable, substitute a command, arithmethic\n> expansion. The use of '\\' in the tests I have changed in v12 wrt 11 is\n> understood by me as you want to remove the use of escape sequences\n> which is justified. But this seems a bit vague. Is it some convention\n> in git?\n\nBoth 'EOF' and \\EOF suppress interpolation and other transformations\nin the heredoc content which would otherwise occur with plain EOF. The\n'EOF' form is well documented; \\EOF not so much, but is used heavily\nin git test scripts. So:\n\n x=flormp\n echo < wrote:\n> The fact that the 32 new tests are nearly identical suggests strongly\n> that the testing should instead either be table-driven or be done via\n> for-loops to systematically cover all cases. Not only would either of\n> these approaches be easier to digest, but they would make it easy to\n> tell at a glance if any cases were missing. See [2] for an example of\n> how the tests can be table-driven, and see the bottom of [3] for an\n> example of using for-loops to test systematically (though you'd need\n> to use nested for-loops rather than just the one in the example).\n>\n> I'm leaning toward systematic testing via nested for-loops as the more\n> suitable of the two approach for this particular application.\n\nBy the way, while this would be a nice change, it doesn't necessarily\nhave to be part of this series, and could be done as a follow-up by\nyou or someone else.\n\n(The other changes suggested in the same review, however, ought to be\nfixed in this series; in particular, simplifying the \"setup\" test and\nmaking the first test after \"setup\" consistent with the remaining\ntests.)\n"},{"id":"282682","messageId":"CAFZEwPPPf3OUb8rL-juL3dMXZamwsTO2e+pLg3y8grdJf6W6YA@mail.gmail.com","threadId":"41906","inReplyTo":"CAPig+cQw=aG0NOgEsmdTGc7v3iBA-qG9_C4EdSm=6PcE6Qn-uQ@mail.gmail.com","subject":"Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-05T05:08:09Z","receivedAt":"2016-04-05T05:08:09Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Mon, Apr 4, 2016 at 11:00 PM, Eric Sunshine wrote:\n> On Mon, Apr 4, 2016 at 8:45 AM, Pranit Bauva wrote:\n>> Okay I will do the change. I was previously unaware about the use of\n>> '\\' before EOF. I googled it now. But I am still confused about its\n>> use in this scenario. Upto what I understood, it is used where you\n>> want to expand a variable, substitute a command, arithmethic\n>> expansion. The use of '\\' in the tests I have changed in v12 wrt 11 is\n>> understood by me as you want to remove the use of escape sequences\n>> which is justified. But this seems a bit vague. Is it some convention\n>> in git?\n>\n> Both 'EOF' and \\EOF suppress interpolation and other transformations\n> in the heredoc content which would otherwise occur with plain EOF. The\n> 'EOF' form is well documented; \\EOF not so much, but is used heavily\n> in git test scripts. So:\n>\n> x=flormp\n> echo < Hello, $x\n> EOF\n>\n> prints \"Hello, flormp\", whereas:\n>\n> echo <<\\EOF\n> Hello, $x\n> EOF\n>\n> prints \"Hello, $x\".\n>\n> While test scripts sometimes use \\EOF to explicitly suppress variable\n> expansion, it's also quite common to use it even when there is nothing\n> which could be expanded in the heredoc content, in which case it\n> signals to the reader that the author doesn't expect the content to\n> undergo expansion or interpolation. It's also a bit of future-proofing\n> in case some later change to the heredoc content inserts something\n> which might otherwise be expanded.\n\nThanks for taking out your time to explain this clearly. I will do the\nchanges in the tests as suggested by your review. :)\n"},{"id":"282708","messageId":"CAFZEwPORAer3=WmUqqhztv4kAOHdv+yB12tRyUYh_YTSmtcQsQ@mail.gmail.com","threadId":"41906","inReplyTo":"CAPig+cQpdr7A7GxKUN3tDs2rQ7z_S5b1WVsk01=BwkCwB2mp0A@mail.gmail.com","subject":"Re: [PATCH v12 2/5] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-05T15:39:57Z","receivedAt":"2016-04-05T15:39:57Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"[+cc:Duy Nguyen, Jonathan Nieder]\n\nOn Mon, Apr 4, 2016 at 3:00 AM, Eric Sunshine wrote:\n> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>> Current implementation of parse-options.c treats OPT__QUIET() as integer\n>> and not boolean and thus it is more appropriate to print it as integer\n>> to avoid confusion.\n>\n> I can buy this line of reasoning, however, it would be even easier to\n> sell the change if you cited an existing client (a git command) which\n> actually respects multiple quiet levels. Are there any?\n\nI investigated into this. But I was unable to find any git commit\nwhich actually respects mulitple quiet levels. I first did a 'git grep\nOPT__QUIET' to find the commands which use this. Then I went through\nthe documentation which covers it. None of them have any such mention\nof multiple quiet levels. But still I dug further and and went through\nall the individual source files. I followed the corresponding C source\ncode for the header file included and also searched there for any\ntrace of quiet. But I still didn't find any such use of multiple quiet\nlevels. I have found that the commit id 212c0a6f (Duy Ngyuyen; 7 Dec,\n2013; parse-options: remove OPT__BOOLEAN). Maybe he has something to\nsay as to why this was introduced and OPT__QUIET which previously used\nOPT__BOOLEAN, now uses OPT_COUNTUP rather than OPT_BOOL. This commit\nIn fact git-repack command has quiet but this is not mentioned in the\ndocumentation. If someone could include this it in the documentation.\nI would do it but I am not quite familiar with git-repack and haven't\nreally used it anytime.\n\n> More importantly, though, this change implies that you should also add\n> tests to ensure that the quiet level is indeed incremented with each\n> --quiet, just as \"-vv\" and \"--verbose --verbose\" are already tested.\n> You might be able to include such new tests directly in this patch as\n> long as the commit message is clear about it, or add them in a\n> separate patch.\n\nI will include the tests for multiple level of quiet in the patch.\n\n> By the way, I don't see any tests to ensure that --no-verbose and\n> --no-quiet reset those respective values to 0. A separate patch which\n> adds such tests would be nice (unless such tests already exist and I\n> merely missed them).\nThere are no tests to ensure that --no-verbose and --no-quiet reset\nthose respective values to 0 just before this patch. But I have\ncovered the --no-verbose tests in the upcoming patch 3/5. I will\ninclude the tests for --no-quiet in this patch.\n"},{"id":"282710","messageId":"CAFZEwPPvwiw4E78vYA_jpLAfASS-M0fxPopnbFSLd4yomzVciA@mail.gmail.com","threadId":"41906","inReplyTo":"CAPig+cS23i9XFTntLTHRbNf21DxhLAcw+9ent_ZtWj=1aO7JwQ@mail.gmail.com","subject":"Re: [PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-05T15:51:32Z","receivedAt":"2016-04-05T15:51:32Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Mon, Apr 4, 2016 at 4:40 AM, Eric Sunshine wrote:\n> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>> The reason to make it respect \"unspecified\" values is to give the\n>> ability to differentiate whether `--option` or `--no-option` was\n>> specified at all. \"unspecified\" values should be in the form of negative\n>> values. If initial value is set to negative and `--option` specified\n>> then it will reflect the number of occurrences (counting done from 0),\n>> if `--no-option` is specified then it will reflect 0 and if nothing at\n>> all is given then it will retain its negative value.\n>\n> Thanks, this rewrite does a better job of explaining the aim of the\n> change and how a client can take advantage of it. However, with my\n> \"first-time reader\" hat on, I still have some trouble digesting it as\n> a coherent whole. I wonder if a rewrite like this would help?\n>\n> OPT_COUNTUP() merely increments the counter upon --option, and\n> resets it to 0 upon --no-option, which means that there is no\n> \"unspecified\" value with which a client can initialize the\n> counter to determine whether or not --[no-]option was seen at\n> all.\n>\n> Make OPT_COUNTUP() treat any negative number as an \"unspecified\"\n> value to address this shortcoming. In particular, if a client\n> initializes the counter to -1, then if it is still -1 after\n> parse_options(), then neither --option nor --no-option was seen;\n> if it is 0, then --no-option was seen last, and if it is 1 or\n> greater, than --option was seen last.\n>\n>> This change will not affect existing users of COUNTUP, because they all\n>> use the initial value of 0 (or more).\n>\n> \"This change does not affect behavior of existing clients of...\"\n>\n\nSure I could do the change.\n\n>> Note that builtin/clean.c initializes the variable used with\n>> OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set\n>> to either 0 or 1 by reading the configuration before the code calls\n>> parse_options(), i.e. as far as parse_options() is concerned, the\n>> initial value of the variable is not negative.\n>>\n>> To test this behavior \"verbose\" is set to \"unspecified\" while quiet is\n>> set to 0 which will test the new behavior with all sets of values.\n>\n> I think you need to mention here that you're talking about test-parse-options.c\n> (and indirectly t0040) since it's otherwise too easy for the reader to\n> think that this paragraph is a continuation of the discussion about\n> OPT_COUNTUP()'s new behavior and how it won't impact existing tests,\n> rather than a new topic of its own (testing the behavior). Maybe say\n> something like this:\n>\n> To test the new behavior, augment the initial \"verbose\" setting\n> of test-parse-options.c to be unspecified...\n>\n> and then go on to say that, because \"quiet\" is still initialized to 0,\n> you have complete coverage. An alternative would be to split off the\n> new testing into its own patch, which would make this patch (which is\n> the real meat of the change) less noisy.\n\nI do like including test-parse-options.c in commit message. My main\nmotive behind including the test with this patch was to show the\n\"first-time\" reader how to use the changes introduced in this patch.\nThis would also set a complete picture of this commit. And I kind of\nbelieve it is much effort for the reader to find the commit whose\nparent will be this (if it exists at all, as the reader won't know\nabout it) which will give a kind of demonstration to utilizing this\nchange.\n\n>\n> I actually expected you to add new functionality to\n> test-parse-options.c specifically to test OPT_COUNTUP() directly\n> rather than indirectly through --verbose and --quiet, however, I think\n> I can be sold on the approach you took for a couple reasons. First,\n> you have a genuine use-case for an \"unspecified\" --verbose value, so\n> changing test-parse-options.c's --verbose to start at -1 tests what\n> you ultimately care about. Second, since you retained 0-initialization\n> of --quiet, that case of OPT_COUNTUP() is still being tested.\n>\n> What I find a bit disturbing about this approach is that you are\n> getting \"full coverage\" only because of current *implementation*, not\n> because you are explicitly testing for *expected* behavior. That is,\n> you get that coverage only while both OPT__VERBOSE() and OPT__QUIET()\n> are built atop OPT_COUNTUP(); if OPT__QUIET() is ever rewritten so it\n> no longer uses OPT_COUNTUP(), then the tests silently lose full\n> coverage. However, I may be over-worrying about the situation...\n\nThe main reason as I mentioned above was to give a demonstration of\nhow to utilize the change introduced.\n\n>\n>> Signed-off-by: Pranit Bauva \n>> ---\n>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n>> @@ -144,8 +144,12 @@ There are some macros to easily define options:\n>> `OPT_COUNTUP(short, long, &int_var, description)`::\n>> Introduce a count-up option.\n>> - `int_var` is incremented on each use of `--option`, and\n>> - reset to zero with `--no-option`.\n>> + Each use of `--option` increments `int_var`, starting from zero\n>> + (even if initially negative), and `--no-option` resets it to\n>> + zero. To determine if `--option` or `--no-option` was set at\n>\n> s/was set/was encountered/\n>\n>> + all, set `int_var` to a negative value, and if it is still\n>\n> s/set `int_var`/initialize `int_var`/\n>\n>> + negative after parse_options(), then neither `--option` nor\n>> + `--no-option` was seen.\n>> diff --git a/parse-options.c b/parse-options.c\n>> @@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n>> case OPTION_COUNTUP:\n>> + if (*(int *)opt->value < 0)\n>> + *(int *)opt->value = 0;\n>> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n>\n> This is nicer.\n>\n>> return 0;\n>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n>> @@ -454,6 +454,25 @@ dry run: no\n>> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '\n>> + test-parse-options --no-verbose >output 2>output.err &&\n>> + test_must_be_empty output.err &&\n>> + test_cmp expect output\n>> +'\n>\n> If you take the advice of my 2/5 review and add new tests (in a new\n> patch) which test --no-verbose and --no-quiet, then I think this new\n> test here can just go away since the case it cares about will already\n> be covered.\n\nAdding a new patch between 2/5 and 3/5 would be a better choice.\n"},{"id":"282711","messageId":"CAFZEwPO_Y6-jEGXDspExhmaWO36QWhaK813TcEUWSqB_byZngQ@mail.gmail.com","threadId":"41906","inReplyTo":"CAPig+cRBwOjxA2cB=ZR__o7LsHxk9Rbzfv7ngRxv8h6=koQSRw@mail.gmail.com","subject":"Re: [PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-05T15:54:02Z","receivedAt":"2016-04-05T15:54:02Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Mon, Apr 4, 2016 at 5:32 AM, Eric Sunshine wrote:\n> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>> Make the fake \"editor\" store output of grep in a file so that we can\n>> see how many diffs were contained in the message and use them in\n>> individual tests where ever it is required. Also use write_script()\n>> to create the fake \"editor\".\n>>\n>> A subsequent commit will introduce scenarios where it is important to be\n>> able to exactly determine how many diffs were present.\n>\n> These two sentences are backward. The \"subsequent commit\" bit is\n> justification for why you are making the \"editor\" store the output,\n> thus it belongs with the first paragraph. The bit about write_script()\n> is just a minor aside which can go in its own paragraph.\n>\n> I think it's also important to explain that you're changing the\n> behavior of write_script() so that it always succeeds, regardless of\n> whether grep found diff headers or not, and to give the reason for\n> making this change (\"so that you don't have to use 'test_must_fail'\n> for cases when no diff headers are expected and can instead easily use\n> 'test_line_count = 0'\").\n>\n> The patch itself looks fine and is easy enough to read.\n\nI will include some more explanation as you suggest.\n\n>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Pranit Bauva \n>> ---\n>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>> index 2ddf28c..0f28a86 100755\n>> --- a/t/t7507-commit-verbose.sh\n>> +++ b/t/t7507-commit-verbose.sh\n>> @@ -3,11 +3,10 @@\n>> test_description='verbose commit template'\n>> . ./test-lib.sh\n>>\n>> -cat >check-for-diff <> -#!$SHELL_PATH\n>> -exec grep '^diff --git' \"\\$1\"\n>> +write_script \"check-for-diff\" <<\\EOF &&\n>> +grep '^diff --git' \"$1\" >out\n>> +exit 0\n>> EOF\n>> -chmod +x check-for-diff\n>> test_set_editor \"$PWD/check-for-diff\"\n>>\n>> cat >message <<'EOF'\n>> @@ -23,7 +22,8 @@ test_expect_success 'setup' '\n>> '\n>>\n>> test_expect_success 'initial commit shows verbose diff' '\n>> - git commit --amend -v\n>> + git commit --amend -v &&\n>> + test_line_count = 1 out\n>> '\n>>\n>> test_expect_success 'second commit' '\n>> @@ -39,13 +39,15 @@ check_message() {\n>>\n>> test_expect_success 'verbose diff is stripped out' '\n>> git commit --amend -v &&\n>> - check_message message\n>> + check_message message &&\n>> + test_line_count = 1 out\n>> '\n>>\n>> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n>> git config diff.mnemonicprefix true &&\n>> git commit --amend -v &&\n>> - check_message message\n>> + check_message message &&\n>> + test_line_count = 1 out\n>> '\n"},{"id":"282712","messageId":"CAFZEwPNQE82_A39sO7z--3sR74GwF-HEgHTePmN_DbFgFmgKgQ@mail.gmail.com","threadId":"41906","inReplyTo":"CAPig+cQdAbweFF7NWJhNjALXN5cdpE-gtvZ_LB0tVmLJ2XGLiA@mail.gmail.com","subject":"Re: [PATCH v12 5/5] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-05T15:58:40Z","receivedAt":"2016-04-05T15:58:40Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Tue, Apr 5, 2016 at 4:59 AM, Eric Sunshine wrote:\n> On Sun, Apr 3, 2016 at 8:58 PM, Eric Sunshine wrote:\n>> The fact that the 32 new tests are nearly identical suggests strongly\n>> that the testing should instead either be table-driven or be done via\n>> for-loops to systematically cover all cases. Not only would either of\n>> these approaches be easier to digest, but they would make it easy to\n>> tell at a glance if any cases were missing. See [2] for an example of\n>> how the tests can be table-driven, and see the bottom of [3] for an\n>> example of using for-loops to test systematically (though you'd need\n>> to use nested for-loops rather than just the one in the example).\n>>\n>> I'm leaning toward systematic testing via nested for-loops as the more\n>> suitable of the two approach for this particular application.\n>\nI hadn't thought of this before. I also believe using for-loops will\nmake it more clear, crisp and will avoid the effort of going through\nthe whole patch to find out if some test is missing.\n\n> By the way, while this would be a nice change, it doesn't necessarily\n> have to be part of this series, and could be done as a follow-up by\n> you or someone else.\n>\n> (The other changes suggested in the same review, however, ought to be\n> fixed in this series; in particular, simplifying the \"setup\" test and\n> making the first test after \"setup\" consistent with the remaining\n> tests.)\n\nI will include it this series only as it will be a bit easier for me\nto keep a track of the previous reviews.\n"},{"id":"282744","messageId":"CAPig+cTynD42tDe9bQDvKEb7ACFpWwwq85AT8byb8u79G0iVHA@mail.gmail.com","threadId":"41906","inReplyTo":"CAFZEwPORAer3=WmUqqhztv4kAOHdv+yB12tRyUYh_YTSmtcQsQ@mail.gmail.com","subject":"Re: [PATCH v12 2/5] test-parse-options: print quiet as integer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-06T05:56:51Z","receivedAt":"2016-04-06T05:56:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 5, 2016 at 11:39 AM, Pranit Bauva wrote:\n> On Mon, Apr 4, 2016 at 3:00 AM, Eric Sunshine wrote:\n>> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>>> Current implementation of parse-options.c treats OPT__QUIET() as integer\n>>> and not boolean and thus it is more appropriate to print it as integer\n>>> to avoid confusion.\n>>\n>> I can buy this line of reasoning, however, it would be even easier to\n>> sell the change if you cited an existing client (a git command) which\n>> actually respects multiple quiet levels. Are there any?\n>\n> I investigated into this. But I was unable to find any git commit\n> which actually respects mulitple quiet levels. I first did a 'git grep\n> OPT__QUIET' to find the commands which use this. Then I went through\n> the documentation which covers it. None of them have any such mention\n> of multiple quiet levels. But still I dug further and and went through\n> all the individual source files. I followed the corresponding C source\n> code for the header file included and also searched there for any\n> trace of quiet. But I still didn't find any such use of multiple quiet\n> levels. I have found that the commit id 212c0a6f (Duy Ngyuyen; 7 Dec,\n> 2013; parse-options: remove OPT__BOOLEAN). Maybe he has something to\n> say as to why this was introduced and OPT__QUIET which previously used\n> OPT__BOOLEAN, now uses OPT_COUNTUP rather than OPT_BOOL. This commit\n> In fact git-repack command has quiet but this is not mentioned in the\n> documentation. If someone could include this it in the documentation.\n> I would do it but I am not quite familiar with git-repack and haven't\n> really used it anytime.\n\nI didn't find any existing Git command recognizing multiple quiet\nlevels either, however, I think I can still buy the change considering\nthat there is existing precedent in other Unix commands.\n"},{"id":"282947","messageId":"CACsJy8Bru3ZW_g2HA3yNt_yc6fV9DfWLY1BHWzVWWxxrtLp-9A@mail.gmail.com","threadId":"41906","inReplyTo":"CAFZEwPORAer3=WmUqqhztv4kAOHdv+yB12tRyUYh_YTSmtcQsQ@mail.gmail.com","subject":"Re: [PATCH v12 2/5] test-parse-options: print quiet as integer","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-04-08T11:33:10Z","receivedAt":"2016-04-08T11:33:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Apr 5, 2016 at 10:39 PM, Pranit Bauva wrote:\n> [+cc:Duy Nguyen, Jonathan Nieder]\n>\n> On Mon, Apr 4, 2016 at 3:00 AM, Eric Sunshine wrote:\n>> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>>> Current implementation of parse-options.c treats OPT__QUIET() as integer\n>>> and not boolean and thus it is more appropriate to print it as integer\n>>> to avoid confusion.\n>>\n>> I can buy this line of reasoning, however, it would be even easier to\n>> sell the change if you cited an existing client (a git command) which\n>> actually respects multiple quiet levels. Are there any?\n>\n> I investigated into this. But I was unable to find any git commit\n> which actually respects mulitple quiet levels. I first did a 'git grep\n> OPT__QUIET' to find the commands which use this. Then I went through\n> the documentation which covers it. None of them have any such mention\n> of multiple quiet levels. But still I dug further and and went through\n> all the individual source files. I followed the corresponding C source\n> code for the header file included and also searched there for any\n> trace of quiet. But I still didn't find any such use of multiple quiet\n> levels. I have found that the commit id 212c0a6f (Duy Ngyuyen; 7 Dec,\n> 2013; parse-options: remove OPT__BOOLEAN). Maybe he has something to\n> say as to why this was introduced and OPT__QUIET which previously used\n> OPT__BOOLEAN, now uses OPT_COUNTUP rather than OPT_BOOL.\n\nI don't have much to say because my commit is a harmless conversion :)\nOPT_BOOLEAN _is_ OPT_COUNTUP with a misleading name, see b04ba2b\n(parse-options: deprecate OPT_BOOLEAN - 2011-09-27). If you dig\nfurther back, both OPT_VERBOSE and OPT_QUIET are introduced at the\nsame time in 0ce865b (Add shortcuts for very often used options. -\n2007-10-14) and they both are defined with OPT_BOOLEAN. I guess it was\njust an oversight that OPT_QUIET is defined as OPT_BOOLEAN instead of\nOPT_BOOL.\n-- \nDuy\n"},{"id":"282967","messageId":"xmqq4mbcxctd.fsf@gitster.mtv.corp.google.com","threadId":"41906","inReplyTo":"CAPig+cQpdr7A7GxKUN3tDs2rQ7z_S5b1WVsk01=BwkCwB2mp0A@mail.gmail.com","subject":"Re: [PATCH v12 2/5] test-parse-options: print quiet as integer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-08T16:52:46Z","receivedAt":"2016-04-08T16:52:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote:\n>> Current implementation of parse-options.c treats OPT__QUIET() as integer\n>> and not boolean and thus it is more appropriate to print it as integer\n>> to avoid confusion.\n>\n> I can buy this line of reasoning, however, it would be even easier to\n> sell the change if you cited an existing client (a git command) which\n> actually respects multiple quiet levels. Are there any?\n\nThis is the only one I found in Documentation/\n\n -q::\n --quiet::\n Make 'git svn' less verbose. Specify a second time to make it\n even less verbose.\n\nIt probably is not a good idea to use OPT__QUIET() and try to do\nmultiple classes of quietness in the longer term anyway.\n\nNew work should be done to improve OPT_VERBOSITY() that already can\ndo \"even more verbose\" and \"even more quiet\" with a single variable;\ncurrently there is no way to ask \"has any verbosity ever been given\nfrom the command line?\", and we may want to support that.\n"},{"id":"283014","messageId":"01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com","subject":"[PATCH v13 1/6] t0040-test-parse-options.sh: fix style issues","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-09T12:23:20Z","receivedAt":"2016-04-09T12:23:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Signed-off-by: Pranit Bauva \n\n---\nChanges wrt previous version (v12):\n - Use '\\' when interpolation isn't required\n---\n t/t0040-parse-options.sh | 76 ++++++++++++++++++++++++------------------------\n 1 file changed, 38 insertions(+), 38 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 9be6411..477fcff 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -7,7 +7,7 @@ test_description='our own option parser'\n \n . ./test-lib.sh\n \n-cat > expect << EOF\n+cat >expect <<\\EOF\n usage: test-parse-options \n \n --yes get a boolean\n@@ -49,14 +49,14 @@ Standard options\n EOF\n \n test_expect_success 'test help' '\n-\ttest_must_fail test-parse-options -h > output 2> output.err &&\n+\ttest_must_fail test-parse-options -h >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_i18ncmp expect output\n '\n \n mv expect expect.err\n \n-cat >expect.template <expect.template <<\\EOF\n boolean: 0\n integer: 0\n magnitude: 0\n@@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '\n \tcheck magnitude: 3221225472 -m 3g\n '\n \n-cat > expect << EOF\n+cat >expect <<\\EOF\n boolean: 2\n integer: 1729\n magnitude: 16384\n@@ -176,7 +176,7 @@ test_expect_success 'short options' '\n \ttest_must_be_empty output.err\n '\n \n-cat > expect << EOF\n+cat >expect <<\\EOF\n boolean: 2\n integer: 1729\n magnitude: 16384\n@@ -204,7 +204,7 @@ test_expect_success 'missing required value' '\n \ttest_expect_code 129 test-parse-options --file\n '\n \n-cat > expect << EOF\n+cat >expect <<\\EOF\n boolean: 1\n integer: 13\n magnitude: 0\n@@ -222,12 +222,12 @@ EOF\n \n test_expect_success 'intermingled arguments' '\n \ttest-parse-options a1 --string 123 b1 --boolean -j 13 -- --boolean \\\n-\t\t> output 2> output.err &&\n+\t\t>output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect << EOF\n+cat >expect <<\\EOF\n boolean: 0\n integer: 2\n magnitude: 0\n@@ -241,13 +241,13 @@ file: (not set)\n EOF\n \n test_expect_success 'unambiguously abbreviated option' '\n-\ttest-parse-options --int 2 --boolean --no-bo > output 2> output.err &&\n+\ttest-parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'unambiguously abbreviated option with \"=\"' '\n-\ttest-parse-options --int=2 > output 2> output.err &&\n+\ttest-parse-options --int=2 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n@@ -256,7 +256,7 @@ test_expect_success 'ambiguously abbreviated option' '\n \ttest_expect_code 129 test-parse-options --strin 123\n '\n \n-cat > expect << EOF\n+cat >expect <<\\EOF\n boolean: 0\n integer: 0\n magnitude: 0\n@@ -270,32 +270,32 @@ file: (not set)\n EOF\n \n test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n-\ttest-parse-options --st 123 > output 2> output.err &&\n+\ttest-parse-options --st 123 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > typo.err << EOF\n-error: did you mean \\`--boolean\\` (with two dashes ?)\n+cat >typo.err <<\\EOF\n+error: did you mean `--boolean` (with two dashes ?)\n EOF\n \n test_expect_success 'detect possible typos' '\n-\ttest_must_fail test-parse-options -boolean > output 2> output.err &&\n+\ttest_must_fail test-parse-options -boolean >output 2>output.err &&\n \ttest_must_be_empty output &&\n \ttest_cmp typo.err output.err\n '\n \n-cat > typo.err << EOF\n-error: did you mean \\`--ambiguous\\` (with two dashes ?)\n+cat >typo.err <<\\EOF\n+error: did you mean `--ambiguous` (with two dashes ?)\n EOF\n \n test_expect_success 'detect possible typos' '\n-\ttest_must_fail test-parse-options -ambiguous > output 2> output.err &&\n+\ttest_must_fail test-parse-options -ambiguous >output 2>output.err &&\n \ttest_must_be_empty output &&\n \ttest_cmp typo.err output.err\n '\n \n-cat > expect <expect <<\\EOF\n boolean: 0\n integer: 0\n magnitude: 0\n@@ -310,12 +310,12 @@ arg 00: --quux\n EOF\n \n test_expect_success 'keep some options as arguments' '\n-\ttest-parse-options --quux > output 2> output.err &&\n+\ttest-parse-options --quux >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n- test_cmp expect output\n+\ttest_cmp expect output\n '\n \n-cat > expect <expect <<\\EOF\n boolean: 0\n integer: 0\n magnitude: 0\n@@ -331,12 +331,12 @@ EOF\n \n test_expect_success 'OPT_DATE() works' '\n \ttest-parse-options -t \"1970-01-01 00:00:01 +0000\" \\\n-\t\tfoo -q > output 2> output.err &&\n+\t\tfoo -q >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect <<\\EOF\n Callback: \"four\", 0\n boolean: 5\n integer: 4\n@@ -351,22 +351,22 @@ file: (not set)\n EOF\n \n test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '\n-\ttest-parse-options --length=four -b -4 > output 2> output.err &&\n+\ttest-parse-options --length=four -b -4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect <<\\EOF\n Callback: \"not set\", 1\n EOF\n \n test_expect_success 'OPT_CALLBACK() and callback errors work' '\n-\ttest_must_fail test-parse-options --no-length > output 2> output.err &&\n+\ttest_must_fail test-parse-options --no-length >output 2>output.err &&\n \ttest_i18ncmp expect output &&\n \ttest_i18ncmp expect.err output.err\n '\n \n-cat > expect <expect <<\\EOF\n boolean: 1\n integer: 23\n magnitude: 0\n@@ -380,18 +380,18 @@ file: (not set)\n EOF\n \n test_expect_success 'OPT_BIT() and OPT_SET_INT() work' '\n-\ttest-parse-options --set23 -bbbbb --no-or4 > output 2> output.err &&\n+\ttest-parse-options --set23 -bbbbb --no-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '\n-\ttest-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err &&\n+\ttest-parse-options --set23 -bbbbb --neg-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect <<\\EOF\n boolean: 6\n integer: 0\n magnitude: 0\n@@ -405,24 +405,24 @@ file: (not set)\n EOF\n \n test_expect_success 'OPT_BIT() works' '\n-\ttest-parse-options -bb --or4 > output 2> output.err &&\n+\ttest-parse-options -bb --or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_NEGBIT() works' '\n-\ttest-parse-options -bb --no-neg-or4 > output 2> output.err &&\n+\ttest-parse-options -bb --no-neg-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n-\ttest-parse-options + + + + + + > output 2> output.err &&\n+\ttest-parse-options + + + + + + >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect <<\\EOF\n boolean: 0\n integer: 12345\n magnitude: 0\n@@ -436,12 +436,12 @@ file: (not set)\n EOF\n \n test_expect_success 'OPT_NUMBER_CALLBACK() works' '\n-\ttest-parse-options -12345 > output 2> output.err &&\n+\ttest-parse-options -12345 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat >expect <expect <<\\EOF\n boolean: 0\n integer: 0\n magnitude: 0\n@@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n \ttest_cmp expect output\n '\n \n-cat >>expect <<'EOF'\n+cat >>expect <<\\EOF\n list: foo\n list: bar\n list: baz\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"283015","messageId":"01020153faf9fcd0-879c8263-6143-4ad2-a3b3-deb78c93246a-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com","subject":"[PATCH v13 6/6] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-09T12:23:20Z","receivedAt":"2016-04-09T12:23:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Add commit.verbose configuration variable as a convenience for those\nwho always prefer --verbose.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Pranit Bauva \n\n---\nThe previous version of the patch are:\n - [v12] $gmane/288820\n - [v11] $gmane/288820\n - [v10] $gmane/288820\n - [v9] $gmane/288820\n - [v8] $gmane/288820\n - [v7] $gmane/288820\n - [v6] $gmane/288728\n - [v5] $gmane/288728\n - [v4] $gmane/288652\n - [v3] $gmane/288634\n - [v2] $gmane/288569\n - [v1] $gmane/287540\n\n Note: One might think some tests are extra but I think that it will\n be better to include them as they \"complete the continuity\" thus\n generalising the series which will make the patch even more clearer.\n\nChanges wrt v12:\n - Change the setup test as suggested by Eric.\n - Use for loops for systematic testing. I have avoided nested-for loops\n or using a function to test because I believe it will just add to the\n complexity rather than simplifying it. Just by using for loops the\n tests look quite systematic and easy to digest.\n---\n Documentation/config.txt | 4 ++++\n Documentation/git-commit.txt | 3 ++-\n builtin/commit.c | 14 ++++++++++-\n t/t7507-commit-verbose.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 75 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2cd6bdd..1d0ec2e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1110,6 +1110,10 @@ commit.template::\n \t\"`~/`\" is expanded to the value of `$HOME` and \"`~user/`\" to the\n \tspecified user's home directory.\n \n+commit.verbose::\n+\tA boolean or int to specify the level of verbose with `git commit`.\n+\tSee linkgit:git-commit[1].\n+\n credential.helper::\n \tSpecify an external helper to be called when a username or\n \tpassword credential is needed; the helper may consult external\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 9ec6b3c..d474226 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].\n \twhat changes the commit has.\n \tNote that this diff output doesn't have its\n \tlines prefixed with '#'. This diff will not be a part\n-\tof the commit message.\n+\tof the commit message. See the `commit.verbose` configuration\n+\tvariable in linkgit:git-config[1].\n +\n If specified twice, show in addition the unified diff between\n what would be committed and the worktree files, i.e. the unstaged\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b3bd2d4..96e6190 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -113,7 +113,9 @@ static char *edit_message, *use_message;\n static char *fixup_message, *squash_message;\n static int all, also, interactive, patch_interactive, only, amend, signoff;\n static int edit_flag = -1; /* unspecified */\n-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n+static int config_verbose = -1; /* unspecified */\n+static int verbose = -1; /* unspecified */\n+static int quiet, no_verify, allow_empty, dry_run, renew_authorship;\n static int no_post_rewrite, allow_empty_message;\n static char *untracked_files_arg, *force_date, *ignore_submodule_arg;\n static char *sign_commit;\n@@ -1354,6 +1356,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_status_usage, 0);\n \tfinalize_colopts(&s.colopts, -1);\n \tfinalize_deferred_config(&s);\n+\tif (verbose == -1)\n+\t\tverbose = 0;\n \n \thandle_untracked_files_arg(&s);\n \tif (show_ignored_in_status)\n@@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n \t\tsign_commit = git_config_bool(k, v) ? \"\" : NULL;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"commit.verbose\")) {\n+\t\tint is_bool;\n+\t\tconfig_verbose = git_config_bool_or_int(k, v, &is_bool);\n+\t\treturn 0;\n+\t}\n \n \tstatus = git_gpg_config(k, v, NULL);\n \tif (status)\n@@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \targc = parse_and_validate_options(argc, argv, builtin_commit_options,\n \t\t\t\t\t builtin_commit_usage,\n \t\t\t\t\t prefix, current_head, &s);\n+\tif (verbose == -1)\n+\t\tverbose = (config_verbose < 0) ? 0 : config_verbose;\n+\n \tif (dry_run)\n \t\treturn dry_run_commit(argc, argv, prefix, current_head, &s);\n \tindex_file = prepare_index(argc, argv, prefix, current_head, 0);\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 0f28a86..00e0c3d 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -98,4 +98,60 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n '\n \n+test_expect_success 'setup -v -v' '\n+\techo dirty >file\n+'\n+\n+for i in true 1\n+do\n+\ttest_expect_success \"commit.verbose=$i and --verbose omitted\" \"\n+\t\tgit -c commit.verbose=$i commit --amend &&\n+\t\ttest_line_count = 1 out\n+\t\"\n+done\n+\n+for i in false -2 -1 0\n+do\n+\ttest_expect_success \"commit.verbose=$i and --verbose omitted\" \"\n+\t\tgit -c commit.verbose=$i commit --amend &&\n+\t\ttest_line_count = 0 out\n+\t\"\n+done\n+\n+for i in 2 3\n+do\n+\ttest_expect_success \"commit.verbose=$i and --verbose omitted\" \"\n+\t\tgit -c commit.verbose=$i commit --amend &&\n+\t\ttest_line_count = 2 out\n+\t\"\n+done\n+\n+for i in true false -2 -1 0 1 2 3\n+do\n+\ttest_expect_success \"commit.verbose=$i and --verbose\" \"\n+\t\tgit -c commit.verbose=$i commit --amend --verbose &&\n+\t\ttest_line_count = 1 out\n+\t\"\n+\n+\ttest_expect_success \"commit.verbose=$i and --no-verbose\" \"\n+\t\tgit -c commit.verbose=$i commit --amend --no-verbose &&\n+\t\ttest_line_count = 0 out\n+\t\"\n+\n+\ttest_expect_success \"commit.verbose=$i and -v -v\" \"\n+\t\tgit -c commit.verbose=$i commit --amend -v -v &&\n+\t\ttest_line_count = 2 out\n+\t\"\n+done\n+\n+test_expect_success 'status ignores commit.verbose=true' '\n+\tgit -c commit.verbose=true status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n+test_expect_success 'status does not verbose without --verbose' '\n+\tgit status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n test_done\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"283016","messageId":"01020153faf9fcc9-b883c097-517a-449f-914e-f3f378d3e6f8-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com","subject":"[PATCH v13 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-09T12:23:20Z","receivedAt":"2016-04-09T12:23:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Make the fake \"editor\" store output of grep in a file so that we can\nsee how many diffs were contained in the message and use them in\nindividual tests where ever it is required. A subsequent commit will\nintroduce scenarios where it is important to be able to exactly\ndetermine how many diffs were present.\n\nAlso use write_script() to create the fake \"editor\".\n\nThe fake \"editor\" is always made to succeed regardless of whether grep\nfound diff headers or not so that we don't have to use 'test_must_fail'\nfor which 'test_line_count = 0' is an easy substitute.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Pranit Bauva \n\n---\nPrevious version of this patch:\n - [v12] : $gmane/288820\n - [v11] : $gmane/288820\n - [v10]: $gmane/288820\n\nChanges this version wrt previous one:\nChange the commit message as suggested by Eric\n---\n t/t7507-commit-verbose.sh | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..0f28a86 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -3,11 +3,10 @@\n test_description='verbose commit template'\n . ./test-lib.sh\n \n-cat >check-for-diff <out\n+exit 0\n EOF\n-chmod +x check-for-diff\n test_set_editor \"$PWD/check-for-diff\"\n \n cat >message <<'EOF'\n@@ -23,7 +22,8 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'initial commit shows verbose diff' '\n-\tgit commit --amend -v\n+\tgit commit --amend -v &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'second commit' '\n@@ -39,13 +39,15 @@ check_message() {\n \n test_expect_success 'verbose diff is stripped out' '\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n \tgit config diff.mnemonicprefix true &&\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n cat >diff <<'EOF'\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"283017","messageId":"01020153faf9fcbb-adadd774-db3a-43ce-b908-d08ac1312b31-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com","subject":"[PATCH v13 2/6] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-09T12:23:20Z","receivedAt":"2016-04-09T12:23:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Current implementation of parse-options.c treats OPT__QUIET() as integer\nand not boolean and thus it is more appropriate to print it as integer\nto avoid confusion.\n\nSigned-off-by: Pranit Bauva \n---\n t/t0040-parse-options.sh | 26 +++++++++++++-------------\n test-parse-options.c | 2 +-\n 2 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 477fcff..450da45 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -64,7 +64,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -164,7 +164,7 @@ timestamp: 0\n string: 123\n abbrev: 7\n verbose: 2\n-quiet: no\n+quiet: 0\n dry run: yes\n file: prefix/my.file\n EOF\n@@ -184,7 +184,7 @@ timestamp: 0\n string: 321\n abbrev: 10\n verbose: 2\n-quiet: no\n+quiet: 0\n dry run: no\n file: prefix/fi.le\n EOF\n@@ -212,7 +212,7 @@ timestamp: 0\n string: 123\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n arg 00: a1\n@@ -235,7 +235,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -264,7 +264,7 @@ timestamp: 0\n string: 123\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -303,7 +303,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n arg 00: --quux\n@@ -323,7 +323,7 @@ timestamp: 1\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: yes\n+quiet: 1\n dry run: no\n file: (not set)\n arg 00: foo\n@@ -345,7 +345,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -374,7 +374,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -399,7 +399,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -430,7 +430,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -449,7 +449,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 2c8c8f1..86afa98 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -90,7 +90,7 @@ int main(int argc, char **argv)\n \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n \tprintf(\"abbrev: %d\\n\", abbrev);\n \tprintf(\"verbose: %d\\n\", verbose);\n-\tprintf(\"quiet: %s\\n\", quiet ? \"yes\" : \"no\");\n+\tprintf(\"quiet: %d\\n\", quiet);\n \tprintf(\"dry run: %s\\n\", dry_run ? \"yes\" : \"no\");\n \tprintf(\"file: %s\\n\", file ? file : \"(not set)\");\n \n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"283018","messageId":"01020153faf9fcbf-e99e4a5b-b892-4e4a-8de5-84ee4315f01d-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com","subject":"[PATCH v13 3/6] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-09T12:23:20Z","receivedAt":"2016-04-09T12:23:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Include tests to check for multiple levels of quiet and to check if the\n'--no-quiet' option sets it to 0.\n\nSigned-off-by: Pranit Bauva \n---\n t/t0040-parse-options.sh | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 450da45..c913d1c 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -476,4 +476,41 @@ test_expect_success '--no-list resets list' '\n \ttest_cmp expect output\n '\n \n+cat >expect <<\\EOF\n+boolean: 0\n+integer: 0\n+magnitude: 0\n+timestamp: 0\n+string: (not set)\n+abbrev: 7\n+verbose: 0\n+quiet: 3\n+dry run: no\n+file: (not set)\n+EOF\n+\n+test_expect_success 'multiple quiet levels' '\n+\ttest-parse-options -q -q -q >output 2>output.err &&\n+\ttest_must_be_empty output.err &&\n+\ttest_cmp expect output\n+'\n+\n+cat >expect <<\\EOF\n+boolean: 0\n+integer: 0\n+magnitude: 0\n+timestamp: 0\n+string: (not set)\n+abbrev: 7\n+verbose: 0\n+quiet: 0\n+dry run: no\n+file: (not set)\n+EOF\n+\n+test_expect_success '--no-quiet sets quiet to 0' '\n+\ttest-parse-options --no-quiet >output 2>output.err &&\n+\ttest_must_be_empty output.err &&\n+\ttest_cmp expect output\n+'\n test_done\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"283019","messageId":"01020153faf9fcca-f9fc4f38-7432-4f5c-831d-0274eb5ec992-000000@eu-west-1.amazonses.com","threadId":"41906","inReplyTo":"01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com","subject":"[PATCH v13 4/6] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-09T12:23:20Z","receivedAt":"2016-04-09T12:23:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"OPT_COUNTUP() merely increments the counter upon --option, and resets it\nto 0 upon --no-option, which means that there is no \"unspecified\" value\nwith which a client can initialize the counter to determine whether or\nnot --[no]-option was seen at all.\n\nMake OPT_COUNTUP() treat any negative number as an \"unspecified\" value\nto address this shortcoming. In particular, if a client initializes the\ncounter to -1, then if it is still -1 after parse_options(), then\nneither --option nor --no-option was seen; If it is 0, then --no-option\nwas seen last, and if it is 1 or greater, than --option was seen last.\n\nThis change does not affect the behavior of existing clients because\nthey all use the initial value of 0 (or more).\n\nNote that builtin/clean.c initializes the variable used with\nOPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set\nto either 0 or 1 by reading the configuration before the code calls\nparse_options(), i.e. as far as parse_options() is concerned, the\ninitial value of the variable is not negative.\n\nTo test this behavior, in test-parse-options.c, \"verbose\" is set to\n\"unspecified\" while quiet is set to 0 which will test the new behavior\nwith all sets of values.\n\nHelped-by: Jeff King \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nSigned-off-by: Pranit Bauva \n\n---\nThe discussion about this patch:\n[1] : http://thread.gmane.org/gmane.comp.version-control.git/289027\n\nPrevious version of the patch:\n[v11] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v10] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v9] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061\n\nChanges wrt previous version (v12):\n - Use bits of commit message provided by Eric Sunshine.\n\nPlease Note: The diff might seem improper especially the part where I\nhave introduced some continuous lines but this is a logical error by git\ndiff (nothing could be done about it) and thus the changes will be\nclearly visible with the original file itself.\n---\n Documentation/technical/api-parse-options.txt | 8 ++++--\n parse-options.c | 2 ++\n t/t0040-parse-options.sh | 39 ++++++++++++++++++++-------\n test-parse-options.c | 3 ++-\n 4 files changed, 39 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 5f0757d..8908bf7 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -144,8 +144,12 @@ There are some macros to easily define options:\n \n `OPT_COUNTUP(short, long, &int_var, description)`::\n \tIntroduce a count-up option.\n-\t`int_var` is incremented on each use of `--option`, and\n-\treset to zero with `--no-option`.\n+\tEach use of `--option` increments `int_var`, starting from zero\n+\t(even if initially negative), and `--no-option` resets it to\n+\tzero. To determine if `--option` or `--no-option` was set at\n+\tall, set `int_var` to a negative value, and if it is still\n+\tnegative after parse_options(), then neither `--option` nor\n+\t`--no-option` was seen.\n \n `OPT_BIT(short, long, &int_var, description, mask)`::\n \tIntroduce a boolean option.\ndiff --git a/parse-options.c b/parse-options.c\nindex 47a9192..312a85d 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\treturn 0;\n \n \tcase OPTION_COUNTUP:\n+\t\tif (*(int *)opt->value < 0)\n+\t\t\t*(int *)opt->value = 0;\n \t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n \t\treturn 0;\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex c913d1c..efcd844 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -63,7 +63,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -211,7 +211,7 @@ magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -234,7 +234,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -263,7 +263,7 @@ magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -302,7 +302,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -322,7 +322,7 @@ magnitude: 0\n timestamp: 1\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 1\n dry run: no\n file: (not set)\n@@ -344,7 +344,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -373,7 +373,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -398,7 +398,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -429,7 +429,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -454,6 +454,25 @@ dry run: no\n file: (not set)\n EOF\n \n+test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '\n+\ttest-parse-options --no-verbose >output 2>output.err &&\n+\ttest_must_be_empty output.err &&\n+\ttest_cmp expect output\n+'\n+\n+cat >expect <output 2>output.err &&\n \ttest_must_be_empty output.err &&\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 86afa98..f02c275 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -7,7 +7,8 @@ static int integer = 0;\n static unsigned long magnitude = 0;\n static unsigned long timestamp;\n static int abbrev = 7;\n-static int verbose = 0, dry_run = 0, quiet = 0;\n+static int verbose = -1; /* unspecified */\n+static int dry_run = 0, quiet = 0;\n static char *string = NULL;\n static char *file = NULL;\n static int ambiguous;\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"283220","messageId":"xmqqa8kyplkp.fsf@gitster.mtv.corp.google.com","threadId":"41906","inReplyTo":"01020153faf9fcd0-879c8263-6143-4ad2-a3b3-deb78c93246a-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v13 6/6] commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T21:24:22Z","receivedAt":"2016-04-12T21:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hmph, isn't this already in 'next', hence we cannot accept a\nreplacement patch?\n"},{"id":"283221","messageId":"CAFZEwPO5uYxh=9FetsNqsDxfGw6HcBWYZg76BcFL4J_2fKAXEw@mail.gmail.com","threadId":"41906","inReplyTo":"xmqqa8kyplkp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v13 6/6] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T21:28:40Z","receivedAt":"2016-04-12T21:28:40Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 2:54 AM, Junio C Hamano wrote:\n> Hmph, isn't this already in 'next', hence we cannot accept a\n> replacement patch?\n\nYes, this is already in 'next'. This was going to be merged in the\nthird cycle after 2.8 but on my request, you delayed it. So this is an\nupdate on the patch. Is it that once a patch is in 'next', it cannot\nbe replaced with a new updated one? There are 5 other patches along\nwith this which are a requirement for this patch.\n"},{"id":"283222","messageId":"xmqq60vmpl5c.fsf@gitster.mtv.corp.google.com","threadId":"41906","inReplyTo":"01020153faf9fcbb-adadd774-db3a-43ce-b908-d08ac1312b31-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v13 2/6] test-parse-options: print quiet as integer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T21:33:35Z","receivedAt":"2016-04-12T21:33:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Current implementation of parse-options.c treats OPT__QUIET() as integer\n> and not boolean and thus it is more appropriate to print it as integer\n> to avoid confusion.\n\nThere is no \"confusion\" factor involved, as we do not use native\n\"boolean\" type in our C code. IIUC, the reason why we want to do\nthis is because we may want to see how it would affect the value of\nthe underlying variable to give multiple --quiet options from the\ncommand line, which is a policy issue (i.e. we want to allow\ncommands to react to multiple quiet options differently), not an\nimplementation one (i.e. \"current implementation happens to use\ninteger\").\n\n\tWe would want to see how multiple --quiet options affect the\n\tvalue of the underlying variable (we may want \"--quiet --quiet\"\n\tto still be 1, or we may want to see the value incremented\n\tto 2). Show the value as integer to allow us to inspect it.\n\nperhaps?\n\n>\n> Signed-off-by: Pranit Bauva \n> ---\n> t/t0040-parse-options.sh | 26 +++++++++++++-------------\n> test-parse-options.c | 2 +-\n> 2 files changed, 14 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index 477fcff..450da45 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -64,7 +64,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -164,7 +164,7 @@ timestamp: 0\n> string: 123\n> abbrev: 7\n> verbose: 2\n> -quiet: no\n> +quiet: 0\n> dry run: yes\n> file: prefix/my.file\n> EOF\n> @@ -184,7 +184,7 @@ timestamp: 0\n> string: 321\n> abbrev: 10\n> verbose: 2\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: prefix/fi.le\n> EOF\n> @@ -212,7 +212,7 @@ timestamp: 0\n> string: 123\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> arg 00: a1\n> @@ -235,7 +235,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -264,7 +264,7 @@ timestamp: 0\n> string: 123\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -303,7 +303,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> arg 00: --quux\n> @@ -323,7 +323,7 @@ timestamp: 1\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: yes\n> +quiet: 1\n> dry run: no\n> file: (not set)\n> arg 00: foo\n> @@ -345,7 +345,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -374,7 +374,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -399,7 +399,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -430,7 +430,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> @@ -449,7 +449,7 @@ timestamp: 0\n> string: (not set)\n> abbrev: 7\n> verbose: 0\n> -quiet: no\n> +quiet: 0\n> dry run: no\n> file: (not set)\n> EOF\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 2c8c8f1..86afa98 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -90,7 +90,7 @@ int main(int argc, char **argv)\n> \tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n> \tprintf(\"abbrev: %d\\n\", abbrev);\n> \tprintf(\"verbose: %d\\n\", verbose);\n> -\tprintf(\"quiet: %s\\n\", quiet ? \"yes\" : \"no\");\n> +\tprintf(\"quiet: %d\\n\", quiet);\n> \tprintf(\"dry run: %s\\n\", dry_run ? \"yes\" : \"no\");\n> \tprintf(\"file: %s\\n\", file ? file : \"(not set)\");\n> \n>\n> --\n> https://github.com/git/git/pull/218\n"},{"id":"283223","messageId":"xmqqy48io6bh.fsf@gitster.mtv.corp.google.com","threadId":"41906","inReplyTo":"xmqqa8kyplkp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v13 6/6] commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T21:39:14Z","receivedAt":"2016-04-12T21:39:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Hmph, isn't this already in 'next', hence we cannot accept a\n> replacement patch?\n\nAs a one-time measure, I'll revert the previous one\n\n50f0d20d (commit: add a commit.verbose config variable, 2016-03-14)\n\nout of 'next' and queue this one instead on 'pu'.\n"},{"id":"283224","messageId":"CAFZEwPPSY+k9qc8hZ685o9_U3UktbgK-e00zrG2+7GehG-47dQ@mail.gmail.com","threadId":"41906","inReplyTo":"xmqq60vmpl5c.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v13 2/6] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T22:16:28Z","receivedAt":"2016-04-12T22:16:28Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 3:03 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> Current implementation of parse-options.c treats OPT__QUIET() as integer\n>> and not boolean and thus it is more appropriate to print it as integer\n>> to avoid confusion.\n>\n> There is no \"confusion\" factor involved, as we do not use native\n> \"boolean\" type in our C code. IIUC, the reason why we want to do\n> this is because we may want to see how it would affect the value of\n> the underlying variable to give multiple --quiet options from the\n> command line, which is a policy issue (i.e. we want to allow\n> commands to react to multiple quiet options differently), not an\n> implementation one (i.e. \"current implementation happens to use\n> integer\").\n>\n> We would want to see how multiple --quiet options affect the\n> value of the underlying variable (we may want \"--quiet --quiet\"\n> to still be 1, or we may want to see the value incremented\n> to 2). Show the value as integer to allow us to inspect it.\n>\n> perhaps?\n\nThis commit message does look a lot better. I will re-roll this.\nShould I just send an update of this patch or the whole series?\n"},{"id":"283225","messageId":"xmqqtwj6o4ip.fsf@gitster.mtv.corp.google.com","threadId":"41906","inReplyTo":"xmqqy48io6bh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v13 6/6] commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T22:18:06Z","receivedAt":"2016-04-12T22:18:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Hmph, isn't this already in 'next', hence we cannot accept a\n>> replacement patch?\n>\n> As a one-time measure, I'll revert the previous one\n>\n> 50f0d20d (commit: add a commit.verbose config variable, 2016-03-14)\n>\n> out of 'next' and queue this one instead on 'pu'.\n\nI queued these 6 patches on 'master' and merged to 'pu', but it\nseems the series breaks t0040, so in the meantime, I ejected the\nwhole thing.\n"},{"id":"283226","messageId":"CAFZEwPMvBBjp2=ydq1oRcdc-d7oT68mYWC=PFqKe4fv7brGkvw@mail.gmail.com","threadId":"41906","inReplyTo":"xmqqtwj6o4ip.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v13 6/6] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T22:25:12Z","receivedAt":"2016-04-12T22:25:12Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 3:48 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Junio C Hamano writes:\n>>\n>>> Hmph, isn't this already in 'next', hence we cannot accept a\n>>> replacement patch?\n>>\n>> As a one-time measure, I'll revert the previous one\n>>\n>> 50f0d20d (commit: add a commit.verbose config variable, 2016-03-14)\n>>\n>> out of 'next' and queue this one instead on 'pu'.\n>\n> I queued these 6 patches on 'master' and merged to 'pu', but it\n> seems the series breaks t0040, so in the meantime, I ejected the\n> whole thing.\n\nI forgot to update the newly introduced tests in this series for the\nnew behaviour. I will send an re-roll\n"},{"id":"283242","messageId":"xmqqd1puo21a.fsf@gitster.mtv.corp.google.com","threadId":"41906","inReplyTo":"CAFZEwPPSY+k9qc8hZ685o9_U3UktbgK-e00zrG2+7GehG-47dQ@mail.gmail.com","subject":"Re: [PATCH v13 2/6] test-parse-options: print quiet as integer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T23:11:45Z","receivedAt":"2016-04-12T23:11:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> On Wed, Apr 13, 2016 at 3:03 AM, Junio C Hamano wrote:\n>> Pranit Bauva writes:\n>>\n>>> Current implementation of parse-options.c treats OPT__QUIET() as integer\n>>> and not boolean and thus it is more appropriate to print it as integer\n>>> to avoid confusion.\n>>\n>> There is no \"confusion\" factor involved, as we do not use native\n>> \"boolean\" type in our C code. IIUC, the reason why we want to do\n>> this is because we may want to see how it would affect the value of\n>> the underlying variable to give multiple --quiet options from the\n>> command line, which is a policy issue (i.e. we want to allow\n>> commands to react to multiple quiet options differently), not an\n>> implementation one (i.e. \"current implementation happens to use\n>> integer\").\n>>\n>> We would want to see how multiple --quiet options affect the\n>> value of the underlying variable (we may want \"--quiet --quiet\"\n>> to still be 1, or we may want to see the value incremented\n>> to 2). Show the value as integer to allow us to inspect it.\n>>\n>> perhaps?\n>\n> This commit message does look a lot better. I will re-roll this.\n\nYou need to pick which one you want inside the (parentheses),\nthough. After reading 3/6 I am guessing that that you wanted the\nlatter.\n\n> Should I just send an update of this patch or the whole series?\n\nThe latter, for a series this small.\n\nThanks.\n"}]}