{"thread":{"id":"42007","subject":"[PATCH v14 2/6] test-parse-options: print quiet as integer","startedAt":"2016-04-12T23:02:17Z","lastAt":"2016-04-27T18:16:41Z","messageCount":22,"participants":["Pranit Bauva","Eric Sunshine"],"isPatch":true,"patchVersion":14,"patchTotal":6},"messages":[{"id":"283232","messageId":"010201540cb60955-e7575250-a84d-40a7-baea-d004b92579de-000000@eu-west-1.amazonses.com","threadId":"42007","inReplyTo":"010201540cb60832-9402a692-3caa-47a1-9e8e-ae5a1bc7eb2f-000000@eu-west-1.amazonses.com","subject":"[PATCH v14 2/6] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T23:02:17Z","receivedAt":"2016-04-12T23:02:17Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"We would want to see how multiple --quiet options affect the value of\nthe underlying variable (we may want \"--quiet --quiet\" to still be 1, or\nwe may want to see the value incremented to 2). Show the value as\ninteger to allow us to inspect it.\n\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\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":"283233","messageId":"010201540cb60965-887d5e4b-b12d-4477-8271-eefa349ceddd-000000@eu-west-1.amazonses.com","threadId":"42007","inReplyTo":"010201540cb60832-9402a692-3caa-47a1-9e8e-ae5a1bc7eb2f-000000@eu-west-1.amazonses.com","subject":"[PATCH v14 3/6] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T23:02:17Z","receivedAt":"2016-04-12T23:02:17Z","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 <pranit.bauva@gmail.com>\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":"283234","messageId":"010201540cb60832-9402a692-3caa-47a1-9e8e-ae5a1bc7eb2f-000000@eu-west-1.amazonses.com","threadId":"42007","inReplyTo":"01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com","subject":"[PATCH v14 1/6] t0040-test-parse-options.sh: fix style issues","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T23:02:17Z","receivedAt":"2016-04-12T23:02:17Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\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 <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 <<EOF\n+cat >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 <<EOF\n+cat >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 <<EOF\n+cat >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 <<EOF\n+cat >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 <<EOF\n+cat >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 <<EOF\n+cat >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 <<EOF\n+cat >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 <<EOF\n+cat >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 <<EOF\n+cat >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":"283235","messageId":"010201540cb60966-e9711378-3b22-4fbb-a2c7-f6876a6fb3c6-000000@eu-west-1.amazonses.com","threadId":"42007","inReplyTo":"010201540cb60832-9402a692-3caa-47a1-9e8e-ae5a1bc7eb2f-000000@eu-west-1.amazonses.com","subject":"[PATCH v14 6/6] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T23:02:17Z","receivedAt":"2016-04-12T23:02:17Z","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 <gitster@pobox.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\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":"283236","messageId":"010201540cb60971-0bf3b786-4acd-4317-bd3c-3397dc2b51f9-000000@eu-west-1.amazonses.com","threadId":"42007","inReplyTo":"010201540cb60832-9402a692-3caa-47a1-9e8e-ae5a1bc7eb2f-000000@eu-west-1.amazonses.com","subject":"[PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T23:02:17Z","receivedAt":"2016-04-12T23:02:17Z","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 <peff@peff.net>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\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 (v13):\n - Some tests failed in v12 as the newly introduced tests weren't\n   updated by the behaviour of this patch.\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                      | 43 +++++++++++++++++++--------\n test-parse-options.c                          |  3 +-\n 4 files changed, 41 insertions(+), 15 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..d3f52f6 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 <<EOF\n+boolean: 0\n+integer: 0\n+magnitude: 0\n+timestamp: 0\n+string: (not set)\n+abbrev: 7\n+verbose: -1\n+quiet: 0\n+dry run: no\n+file: (not set)\n+EOF\n+\n test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n \ttest-parse-options --no-ambig >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n@@ -483,7 +502,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 3\n dry run: no\n file: (not set)\n@@ -502,7 +521,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)\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":"283237","messageId":"010201540cb6096f-5d2150af-6595-4d88-85e5-18eaeb699fb7-000000@eu-west-1.amazonses.com","threadId":"42007","inReplyTo":"010201540cb60832-9402a692-3caa-47a1-9e8e-ae5a1bc7eb2f-000000@eu-west-1.amazonses.com","subject":"[PATCH v14 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-12T23:02:17Z","receivedAt":"2016-04-12T23:02:17Z","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 and also helps in\nmaintaining the consistency.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\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 <<EOF\n-#!$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-\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":"283292","messageId":"CAPig+cTB=bYNxR8yN2CGvkmtCZKomnbdNnZon9HA5uE9aivW=Q@mail.gmail.com","threadId":"42007","inReplyTo":"010201540cb60965-887d5e4b-b12d-4477-8271-eefa349ceddd-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T05:26:51Z","receivedAt":"2016-04-13T05:26:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> Include tests to check for multiple levels of quiet and to check if the\n> '--no-quiet' option sets it to 0.\n>\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> ---\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> @@ -476,4 +476,41 @@ test_expect_success '--no-list resets list' '\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> +       test-parse-options --no-quiet >output 2>output.err &&\n\nMeh, as implemented, this isn't a very interesting test, is it?\n'quiet' started at 0, so all this shows is that --no-quiet didn't\ndisturb the 0. To really test that it resets it to 0, you'd want:\n\n    test-parse-options --quiet --no-quiet >... 2>... &&\n\n> +       test_must_be_empty output.err &&\n> +       test_cmp expect output\n> +'\n>  test_done\n"},{"id":"283295","messageId":"CAPig+cQM_E9VgsxifBcT_XGyTCCaanNcfQEm6rU85pSNaQG7MQ@mail.gmail.com","threadId":"42007","inReplyTo":"010201540cb60971-0bf3b786-4acd-4317-bd3c-3397dc2b51f9-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T05:56:29Z","receivedAt":"2016-04-13T05:56:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> OPT_COUNTUP() merely increments the counter upon --option, and resets it\n> to 0 upon --no-option, which means that there is no \"unspecified\" value\n> with which a client can initialize the counter to determine whether or\n> not --[no]-option was seen at all.\n>\n> Make OPT_COUNTUP() treat any negative number as an \"unspecified\" value\n> to address this shortcoming. In particular, if a client initializes the\n> counter to -1, then if it is still -1 after parse_options(), then\n> neither --option nor --no-option was seen; If it is 0, then --no-option\n> was seen last, and if it is 1 or greater, than --option was seen last.\n\nNit: I'm pretty sure that when I wrote this commit message for you[1]\nthe \"if\" following the semicolon was correctly lowercase. It's not\nclear why it got incorrectly capitalized here.\n\nMore below...\n\n> This change does not affect the behavior of existing clients because\n> they all use the initial value of 0 (or more).\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, in test-parse-options.c, \"verbose\" is set to\n> \"unspecified\" while quiet is set to 0 which will test the new behavior\n> with all sets of values.\n>\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\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\nRepeating from [1]: s/was set/was encountered/\n\n> +       all, set `int_var` to a negative value, and if it is still\n\nRepeating from [1] and [2]: 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/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\nWhat is \"--no- flag\"?\n\n> +       test-parse-options --no-verbose >output 2>output.err &&\n> +       test_must_be_empty output.err &&\n> +       test_cmp expect output\n> +'\n\nIn my v12 review, I noticed that neither --no-verbose nor --no-quiet\nwas being tested by t0040 (which is conceptually independent of the\nOPT__COUNTUP change) and suggested[3] that you add a new patch to\naddress that shortcoming. This idea was followed up by [1] saying that\nthis test (here) could then be dropped since the case it checks would\nalready be covered by the new patch. My impression was that you\nagreed[4] that that made sense, however, this test is still here. Did\nI misunderstand your response[4]?\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/290662\n[2]: http://article.gmane.org/gmane.comp.version-control.git/289991\n[3]: http://article.gmane.org/gmane.comp.version-control.git/290655\n[4]: http://article.gmane.org/gmane.comp.version-control.git/290787\n"},{"id":"283296","messageId":"CAPig+cS17RF6f6fuTLpn-E551_NkWjmNPbXh8EFQ_Sp5L9Xb1A@mail.gmail.com","threadId":"42007","inReplyTo":"010201540cb6096f-5d2150af-6595-4d88-85e5-18eaeb699fb7-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v14 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T06:03:21Z","receivedAt":"2016-04-13T06:03:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> 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. A subsequent commit will\n> introduce scenarios where it is important to be able to exactly\n> determine how many diffs were present.\n>\n> Also use write_script() to create the fake \"editor\".\n>\n> The fake \"editor\" is always made to succeed regardless of whether grep\n> found diff headers or not so that we don't have to use 'test_must_fail'\n> for which 'test_line_count = 0' is an easy substitute and also helps in\n> maintaining the consistency.\n\nAs mentioned by [1], the change to write_script() is a minor aside; it\nis less important than the explanation of how and why the return value\nof the fake \"editor\" changed, thus the order of the 2nd and 3rd\nparagraphs should be swapped.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/290663\n\n> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n"},{"id":"283298","messageId":"CAPig+cTcCyGu5Y8aHbE3i6fXh3T_mD0ZiuxPFh=DVHOE38pE5w@mail.gmail.com","threadId":"42007","inReplyTo":"010201540cb60966-e9711378-3b22-4fbb-a2c7-f6876a6fb3c6-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v14 6/6] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T06:14:23Z","receivedAt":"2016-04-13T06:14:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> Add commit.verbose configuration variable as a convenience for those\n> who always prefer --verbose.\n>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> ---\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> @@ -98,4 +98,60 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n> +test_expect_success 'status ignores commit.verbose=true' '\n> +       git -c commit.verbose=true status >actual &&\n> +       ! grep \"^diff --git\" actual\n> +'\n\nI understand what this test is checking, as it is in response to\nJunio's suggestion[1]...\n\n> +test_expect_success 'status does not verbose without --verbose' '\n> +       git status >actual &&\n> +       ! grep \"^diff --git\" actual\n> +'\n\nBut what is this test checking?\n\n> +\n>  test_done\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/288648\n"},{"id":"283303","messageId":"CAFZEwPMjDxFHOdbOgqXmBoud=2gmrpfiR4Aa83_7+bjka1bk4g@mail.gmail.com","threadId":"42007","inReplyTo":"CAPig+cQM_E9VgsxifBcT_XGyTCCaanNcfQEm6rU85pSNaQG7MQ@mail.gmail.com","subject":"Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-13T08:39:39Z","receivedAt":"2016-04-13T08:39:39Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> OPT_COUNTUP() merely increments the counter upon --option, and resets it\n>> to 0 upon --no-option, which means that there is no \"unspecified\" value\n>> with which a client can initialize the counter to determine whether or\n>> not --[no]-option was seen at all.\n>>\n>> Make OPT_COUNTUP() treat any negative number as an \"unspecified\" value\n>> to address this shortcoming. In particular, if a client initializes the\n>> counter to -1, then if it is still -1 after parse_options(), then\n>> neither --option nor --no-option was seen; If it is 0, then --no-option\n>> was seen last, and if it is 1 or greater, than --option was seen last.\n>\n> Nit: I'm pretty sure that when I wrote this commit message for you[1]\n> the \"if\" following the semicolon was correctly lowercase. It's not\n> clear why it got incorrectly capitalized here.\n\nA typo. Will fix it if re-rolled\n\n> More below...\n>\n>> This change does not affect the behavior of existing clients because\n>> they all use the initial value of 0 (or more).\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, in test-parse-options.c, \"verbose\" is set to\n>> \"unspecified\" while quiet is set to 0 which will test the new behavior\n>> with all sets of values.\n>>\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\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> Repeating from [1]: s/was set/was encountered/\n>\n>> +       all, set `int_var` to a negative value, and if it is still\n>\n> Repeating from [1] and [2]: s/set `int_var`/initialize `int_var`/\n\nWill include this if re-rolled.\n>\n>> +       negative after parse_options(), then neither `--option` nor\n>> +       `--no-option` was seen.\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>\n> What is \"--no- flag\"?\n>\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> In my v12 review, I noticed that neither --no-verbose nor --no-quiet\n> was being tested by t0040 (which is conceptually independent of the\n> OPT__COUNTUP change) and suggested[3] that you add a new patch to\n> address that shortcoming. This idea was followed up by [1] saying that\n> this test (here) could then be dropped since the case it checks would\n> already be covered by the new patch. My impression was that you\n> agreed[4] that that made sense, however, this test is still here. Did\n> I misunderstand your response[4]?\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991\n> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655\n> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787\n\nI actually did include the tests in the patch 3/6[1]. I mentioned in\nmy response[2] that I will include the tests between 2/5 and 3/5.\nSince, after that no email was exchanged, I thought you agreed.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/291310\n[2]:http://article.gmane.org/gmane.comp.version-control.git/290787\n"},{"id":"283304","messageId":"CAFZEwPMU5KSoBJ0kHGnnPCq0zsoj8ROAXhJ9HFn66fqDehvWGw@mail.gmail.com","threadId":"42007","inReplyTo":"CAPig+cTB=bYNxR8yN2CGvkmtCZKomnbdNnZon9HA5uE9aivW=Q@mail.gmail.com","subject":"Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-13T08:59:48Z","receivedAt":"2016-04-13T08:59:48Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 10:56 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> Include tests to check for multiple levels of quiet and to check if the\n>> '--no-quiet' option sets it to 0.\n>>\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>> ---\n>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n>> @@ -476,4 +476,41 @@ test_expect_success '--no-list resets list' '\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>> +       test-parse-options --no-quiet >output 2>output.err &&\n>\n> Meh, as implemented, this isn't a very interesting test, is it?\n> 'quiet' started at 0, so all this shows is that --no-quiet didn't\n> disturb the 0. To really test that it resets it to 0, you'd want:\n>\n>     test-parse-options --quiet --no-quiet >... 2>... &&\n>\n>> +       test_must_be_empty output.err &&\n>> +       test_cmp expect output\n>> +'\n>>  test_done\n\nThis is to test whether the 0 of quiet remains 0 if --no-quiet is\nincluded. This test \"defines\" the current behavior. Then when I change\nOPT_COUNTUP(), this test will ensure that this behavior is not\ninterrupted as promised by the commit message of that patch[1]. I\nguess this also describe why I choose to include these tests between\n2/5 and 3/5 rather than 3/5 and 4/5. And also see the extended\ndiscussion[2] for this. If I do a re-roll then I include `--quiet`\nbefore `--no-quiet`\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/291313\n"},{"id":"283305","messageId":"CAFZEwPMiYCd1Ase5r1XtOo4k3cONvryp_Kw-wpm1SVsBs37=BQ@mail.gmail.com","threadId":"42007","inReplyTo":"CAPig+cS17RF6f6fuTLpn-E551_NkWjmNPbXh8EFQ_Sp5L9Xb1A@mail.gmail.com","subject":"Re: [PATCH v14 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-13T09:00:33Z","receivedAt":"2016-04-13T09:00:33Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 11:33 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> 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. A subsequent commit will\n>> introduce scenarios where it is important to be able to exactly\n>> determine how many diffs were present.\n>>\n>> Also use write_script() to create the fake \"editor\".\n>>\n>> The fake \"editor\" is always made to succeed regardless of whether grep\n>> found diff headers or not so that we don't have to use 'test_must_fail'\n>> for which 'test_line_count = 0' is an easy substitute and also helps in\n>> maintaining the consistency.\n>\n> As mentioned by [1], the change to write_script() is a minor aside; it\n> is less important than the explanation of how and why the return value\n> of the fake \"editor\" changed, thus the order of the 2nd and 3rd\n> paragraphs should be swapped.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/290663\n\nI will include this if I do a re-roll\n>\n>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n"},{"id":"283306","messageId":"CAFZEwPN+N+21CN+P3oegaxEve59BKHabdfyM_c7QdjV+bonpQA@mail.gmail.com","threadId":"42007","inReplyTo":"CAPig+cTcCyGu5Y8aHbE3i6fXh3T_mD0ZiuxPFh=DVHOE38pE5w@mail.gmail.com","subject":"Re: [PATCH v14 6/6] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-13T09:15:31Z","receivedAt":"2016-04-13T09:15:31Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 11:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>\n>> +test_expect_success 'status does not verbose without --verbose' '\n>> +       git status >actual &&\n>> +       ! grep \"^diff --git\" actual\n>> +'\n>\n> But what is this test checking?\n\nstatus is also a consumer of the verbose whose initial value is set to\n-1. This makes it include verbose in status output. This bug was fixed\nby explicitly initializing verbose to 0 if -1. SZEDER pointed out a\nbug[1] which broke some tests in and then when I fixed it, you\nrequested me to include tests even in this patch[2] which I found\nconvincing enough.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289730\n[2]: http://article.gmane.org/gmane.comp.version-control.git/289993\n"},{"id":"283386","messageId":"CAPig+cSvvAJT6yb2h6B1S5LDr8H03t-b66rmbKWpo88Fw-ehmg@mail.gmail.com","threadId":"42007","inReplyTo":"CAFZEwPMU5KSoBJ0kHGnnPCq0zsoj8ROAXhJ9HFn66fqDehvWGw@mail.gmail.com","subject":"Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T17:27:46Z","receivedAt":"2016-04-13T17:27:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 13, 2016 at 4:59 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Wed, Apr 13, 2016 at 10:56 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>>> +test_expect_success '--no-quiet sets quiet to 0' '\n>>> +       test-parse-options --no-quiet >output 2>output.err &&\n>>\n>> Meh, as implemented, this isn't a very interesting test, is it?\n>> 'quiet' started at 0, so all this shows is that --no-quiet didn't\n>> disturb the 0. To really test that it resets it to 0, you'd want:\n>>\n>>     test-parse-options --quiet --no-quiet >... 2>... &&\n>>\n>>> +       test_must_be_empty output.err &&\n>>> +       test_cmp expect output\n>>> +'\n>>>  test_done\n>\n> This is to test whether the 0 of quiet remains 0 if --no-quiet is\n> included. This test \"defines\" the current behavior. Then when I change\n> OPT_COUNTUP(), this test will ensure that this behavior is not\n> interrupted as promised by the commit message of that patch[1]. I\n> guess this also describe why I choose to include these tests between\n> 2/5 and 3/5 rather than 3/5 and 4/5. And also see the extended\n> discussion[2] for this. If I do a re-roll then I include `--quiet`\n> before `--no-quiet`\n\nEach of these patches should have a single conceptual purpose. It\nseems, from the above explanation, that you're mixing and mismatching\nbits of such changes between patches.\n\n* The two new tests for --no-verbose and --no-quiet should be together\nand check that they correctly reverse --verbose and --quiet,\nrespectively.\n\n* The test you describe above which ensures that --no-quiet leaves\n'quiet' at 0 should be bundled with the change that might break that\nbehavior, namely, the OPT__COUNTUP() change.\n"},{"id":"283388","messageId":"CAPig+cRUr2sgvQimGWy1WhRZ160d_RyafO-69q5hDrPCy4EMXw@mail.gmail.com","threadId":"42007","inReplyTo":"CAFZEwPMjDxFHOdbOgqXmBoud=2gmrpfiR4Aa83_7+bjka1bk4g@mail.gmail.com","subject":"Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T17:33:14Z","receivedAt":"2016-04-13T17:33:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\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>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet\n>> was being tested by t0040 (which is conceptually independent of the\n>> OPT__COUNTUP change) and suggested[3] that you add a new patch to\n>> address that shortcoming. This idea was followed up by [1] saying that\n>> this test (here) could then be dropped since the case it checks would\n>> already be covered by the new patch. My impression was that you\n>> agreed[4] that that made sense, however, this test is still here. Did\n>> I misunderstand your response[4]?\n>>\n>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662\n>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991\n>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655\n>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787\n>\n> I actually did include the tests in the patch 3/6[1]. I mentioned in\n> my response[2] that I will include the tests between 2/5 and 3/5.\n> Since, after that no email was exchanged, I thought you agreed.\n\nI'm not sure that I understand what you are saying since the\n--no-verbose test does not seem to be included in the patch you cite\n(it is instead in the present patch under discussion).\n\nPerhaps there is some miscommunication and misunderstanding.\n\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/291310\n> [2]:http://article.gmane.org/gmane.comp.version-control.git/290787\n"},{"id":"283390","messageId":"CAFZEwPPoNz0W8kqva1_nudvK2ZVjuC1zk0xzM8f6bzL1efU4OA@mail.gmail.com","threadId":"42007","inReplyTo":"CAPig+cRUr2sgvQimGWy1WhRZ160d_RyafO-69q5hDrPCy4EMXw@mail.gmail.com","subject":"Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-13T17:43:56Z","receivedAt":"2016-04-13T17:43:56Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 11:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\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>>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet\n>>> was being tested by t0040 (which is conceptually independent of the\n>>> OPT__COUNTUP change) and suggested[3] that you add a new patch to\n>>> address that shortcoming. This idea was followed up by [1] saying that\n>>> this test (here) could then be dropped since the case it checks would\n>>> already be covered by the new patch. My impression was that you\n>>> agreed[4] that that made sense, however, this test is still here. Did\n>>> I misunderstand your response[4]?\n>>>\n>>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662\n>>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991\n>>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655\n>>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787\n>>\n>> I actually did include the tests in the patch 3/6[1]. I mentioned in\n>> my response[2] that I will include the tests between 2/5 and 3/5.\n>> Since, after that no email was exchanged, I thought you agreed.\n>\n> I'm not sure that I understand what you are saying since the\n> --no-verbose test does not seem to be included in the patch you cite\n> (it is instead in the present patch under discussion).\n>\n> Perhaps there is some miscommunication and misunderstanding.\n\nSorry for being a bit unclear.\nI will explain this. The patch 3/6 contains the test which tests the\nquiet option thus in turn testing whether the variable quiet becomes 0\nwith --no flag. This patch ie. 4/6 contains the test which tests the\nverbose options thus in turn testing whether the variable verbose\nbecomes 0 with no flag. Both of them test different behavior as quiet\nis initially 0 while verbose is initially -1.\n\nSo finally what I wanted to achieve is that I should test --no-quiet\nin 3/6 as till then this new behavior is not yet introduced. Thus, it\nwill confirm the wanted behavior which exists before 4/6.\n\nThis patch introduces a test to verify whether --no-verbose makes the\nvariable 0. This patch introduces a new \"unspecified\" behavior. Thus\nwe can test this new behavior with this.\n\nI hope now it is a bit clear on what I want to do.\n\n>> [1]: http://article.gmane.org/gmane.comp.version-control.git/291310\n>> [2]:http://article.gmane.org/gmane.comp.version-control.git/290787\n"},{"id":"283391","messageId":"CAPig+cQjOq_cKCa7_9CVG0tzd7x1aW9Wg4tZ+UvQUAvkPRWWHA@mail.gmail.com","threadId":"42007","inReplyTo":"CAFZEwPN+N+21CN+P3oegaxEve59BKHabdfyM_c7QdjV+bonpQA@mail.gmail.com","subject":"Re: [PATCH v14 6/6] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T17:44:43Z","receivedAt":"2016-04-13T17:44:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 13, 2016 at 5:15 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Wed, Apr 13, 2016 at 11:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>>> +test_expect_success 'status does not verbose without --verbose' '\n>>> +       git status >actual &&\n>>> +       ! grep \"^diff --git\" actual\n>>> +'\n>>\n>> But what is this test checking?\n>\n> status is also a consumer of the verbose whose initial value is set to\n> -1. This makes it include verbose in status output. This bug was fixed\n> by explicitly initializing verbose to 0 if -1. SZEDER pointed out a\n> bug[1] which broke some tests in and then when I fixed it, you\n> requested me to include tests even in this patch[2] which I found\n> convincing enough.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/289730\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/289993\n\nOkay, makes sense, but it's not at all obvious from the context of\nthis patch or its commit message. It probably would have been clearer\nhad the two git-status tests been added in a separate preparatory test\nwith a commit message explaining that the tests are to ensure that a\nsubsequent patch (adding commit.verbose) won't break the existing\nbehavior of git-status. Having a separate commit explaining that would\nalso help future readers of the test script who wonder what this test\nis doing (since it's not obvious and it's not explained by the current\ncommit message) when they use git-blame to try to figure out its\npurpose. If you do re-roll, you might consider breaking them out to a\nnew patch or, at the very least, document their purpose in the commit\nmessage of this patch.\n"},{"id":"283394","messageId":"CAPig+cQ9DvpJBBqwngfweX2KwC7mTG3TO+M+B84Rcrhw9JH-Mw@mail.gmail.com","threadId":"42007","inReplyTo":"CAFZEwPPoNz0W8kqva1_nudvK2ZVjuC1zk0xzM8f6bzL1efU4OA@mail.gmail.com","subject":"Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-13T17:50:26Z","receivedAt":"2016-04-13T17:50:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 13, 2016 at 1:43 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Wed, Apr 13, 2016 at 11:03 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>>> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\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>>>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet\n>>>> was being tested by t0040 (which is conceptually independent of the\n>>>> OPT__COUNTUP change) and suggested[3] that you add a new patch to\n>>>> address that shortcoming. This idea was followed up by [1] saying that\n>>>> this test (here) could then be dropped since the case it checks would\n>>>> already be covered by the new patch. My impression was that you\n>>>> agreed[4] that that made sense, however, this test is still here. Did\n>>>> I misunderstand your response[4]?\n>>>>\n>>>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662\n>>>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991\n>>>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655\n>>>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787\n>>>\n>>> I actually did include the tests in the patch 3/6[1]. I mentioned in\n>>> my response[2] that I will include the tests between 2/5 and 3/5.\n>>> Since, after that no email was exchanged, I thought you agreed.\n>>\n>> I'm not sure that I understand what you are saying since the\n>> --no-verbose test does not seem to be included in the patch you cite\n>> (it is instead in the present patch under discussion).\n>>\n>> Perhaps there is some miscommunication and misunderstanding.\n>\n> Sorry for being a bit unclear.\n> I will explain this. The patch 3/6 contains the test which tests the\n> quiet option thus in turn testing whether the variable quiet becomes 0\n> with --no flag. This patch ie. 4/6 contains the test which tests the\n> verbose options thus in turn testing whether the variable verbose\n> becomes 0 with no flag. Both of them test different behavior as quiet\n> is initially 0 while verbose is initially -1.\n>\n> So finally what I wanted to achieve is that I should test --no-quiet\n> in 3/6 as till then this new behavior is not yet introduced. Thus, it\n> will confirm the wanted behavior which exists before 4/6.\n>\n> This patch introduces a test to verify whether --no-verbose makes the\n> variable 0. This patch introduces a new \"unspecified\" behavior. Thus\n> we can test this new behavior with this.\n>\n> I hope now it is a bit clear on what I want to do.\n\nSorry, it's not clearer. I understand what you're trying to do, but it\nstill seems to be the a less desirable (and more complex) approach\nsince it's mixing conceptually distinct notions and mismatching them\nwith changes in the wrong patches. But, perhaps my notion of\n\"conceptually distinct\" is different from yours and vice-versa.\n"},{"id":"284465","messageId":"CAFZEwPMsmay+tv48p=zh3r4L1d38tNU0Z6zyn3Op_pd-nsv3Aw@mail.gmail.com","threadId":"42007","inReplyTo":"CAPig+cSvvAJT6yb2h6B1S5LDr8H03t-b66rmbKWpo88Fw-ehmg@mail.gmail.com","subject":"Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-25T18:40:16Z","receivedAt":"2016-04-25T18:40:16Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Apr 13, 2016 at 4:59 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> On Wed, Apr 13, 2016 at 10:56 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>>>> +test_expect_success '--no-quiet sets quiet to 0' '\n>>>> +       test-parse-options --no-quiet >output 2>output.err &&\n>>>\n>>> Meh, as implemented, this isn't a very interesting test, is it?\n>>> 'quiet' started at 0, so all this shows is that --no-quiet didn't\n>>> disturb the 0. To really test that it resets it to 0, you'd want:\n>>>\n>>>     test-parse-options --quiet --no-quiet >... 2>... &&\n>>>\n>>>> +       test_must_be_empty output.err &&\n>>>> +       test_cmp expect output\n>>>> +'\n>>>>  test_done\n>>\n>> This is to test whether the 0 of quiet remains 0 if --no-quiet is\n>> included. This test \"defines\" the current behavior. Then when I change\n>> OPT_COUNTUP(), this test will ensure that this behavior is not\n>> interrupted as promised by the commit message of that patch[1]. I\n>> guess this also describe why I choose to include these tests between\n>> 2/5 and 3/5 rather than 3/5 and 4/5. And also see the extended\n>> discussion[2] for this. If I do a re-roll then I include `--quiet`\n>> before `--no-quiet`\n>\n> Each of these patches should have a single conceptual purpose. It\n> seems, from the above explanation, that you're mixing and mismatching\n> bits of such changes between patches.\n>\n> * The two new tests for --no-verbose and --no-quiet should be together\n> and check that they correctly reverse --verbose and --quiet,\n> respectively.\n>\n> * The test you describe above which ensures that --no-quiet leaves\n> 'quiet' at 0 should be bundled with the change that might break that\n> behavior, namely, the OPT__COUNTUP() change.\n\nI am planning to re-roll this.\nSo, I am just confirming whether I understood properly.\n\n * I will add the tests for check for '-q --no-quiet' instead of just\n'--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch\nwhich improves test coverage which will be before the OPT_COUNTUP()\nchange.\n\n * I will then add the test for '--no-quiet' sets to 0 in the separate\npatch after OPT_COUNTUP() change.\n\nIs there something else or something different that you are suggesting?\n"},{"id":"284698","messageId":"CAPig+cQJok5HnFYqqhvDxAKnoEjiwrzGuc04EWrnYBYcUfQpLA@mail.gmail.com","threadId":"42007","inReplyTo":"CAFZEwPMsmay+tv48p=zh3r4L1d38tNU0Z6zyn3Op_pd-nsv3Aw@mail.gmail.com","subject":"Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-27T17:55:36Z","receivedAt":"2016-04-27T17:55:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 25, 2016 at 2:40 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> Each of these patches should have a single conceptual purpose. It\n>> seems, from the above explanation, that you're mixing and mismatching\n>> bits of such changes between patches.\n>>\n>> * The two new tests for --no-verbose and --no-quiet should be together\n>> and check that they correctly reverse --verbose and --quiet,\n>> respectively.\n>>\n>> * The test you describe above which ensures that --no-quiet leaves\n>> 'quiet' at 0 should be bundled with the change that might break that\n>> behavior, namely, the OPT__COUNTUP() change.\n>\n> I am planning to re-roll this.\n> So, I am just confirming whether I understood properly.\n>\n>  * I will add the tests for check for '-q --no-quiet' instead of just\n> '--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch\n> which improves test coverage which will be before the OPT_COUNTUP()\n> change.\n\nThese tests would be even a bit more interesting if you tested \"-q -q\n--no-quiet\" and \"-v -v --no-verbose\", respectively, to ensure that the\n\"no\" options actually reset to 0 rather than merely decrementing by 1.\n\nI recall also suggesting adding a new test checking that \"-q -q\"\nincrements the quiet count to 2 (which could be done in the patch\nwhich makes 'quiet' print as a number instead of a boolean or in the\nsame \"improve test coverage\" patch).\n\n>  * I will then add the test for '--no-quiet' sets to 0 in the separate\n> patch after OPT_COUNTUP() change.\n\nNo, this and \"--no-verbose sets to 0\" are logically related to the\nOPT__COUNTUP() change, thus would be incorporated into that patch.\nAlternately, these two tests could just be part of \"improve test\ncoverage\" patch, establishing base behavior which the OPT__COUNTUP()\npatch shouldn't break.\n"},{"id":"284705","messageId":"CAFZEwPMDuiDcxLWya_U=LxhpQCA5+yYgHE9Q8MFarB59fTUGiQ@mail.gmail.com","threadId":"42007","inReplyTo":"CAPig+cQJok5HnFYqqhvDxAKnoEjiwrzGuc04EWrnYBYcUfQpLA@mail.gmail.com","subject":"Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-27T18:16:41Z","receivedAt":"2016-04-27T18:16:41Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, Apr 27, 2016 at 11:25 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Mon, Apr 25, 2016 at 2:40 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> Each of these patches should have a single conceptual purpose. It\n>>> seems, from the above explanation, that you're mixing and mismatching\n>>> bits of such changes between patches.\n>>>\n>>> * The two new tests for --no-verbose and --no-quiet should be together\n>>> and check that they correctly reverse --verbose and --quiet,\n>>> respectively.\n>>>\n>>> * The test you describe above which ensures that --no-quiet leaves\n>>> 'quiet' at 0 should be bundled with the change that might break that\n>>> behavior, namely, the OPT__COUNTUP() change.\n>>\n>> I am planning to re-roll this.\n>> So, I am just confirming whether I understood properly.\n>>\n>>  * I will add the tests for check for '-q --no-quiet' instead of just\n>> '--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch\n>> which improves test coverage which will be before the OPT_COUNTUP()\n>> change.\n>\n> These tests would be even a bit more interesting if you tested \"-q -q\n> --no-quiet\" and \"-v -v --no-verbose\", respectively, to ensure that the\n> \"no\" options actually reset to 0 rather than merely decrementing by 1.\n\nThis seems a better choice.\n\n> I recall also suggesting adding a new test checking that \"-q -q\"\n> increments the quiet count to 2 (which could be done in the patch\n> which makes 'quiet' print as a number instead of a boolean or in the\n> same \"improve test coverage\" patch).\n\nWill include this.\n\n>>  * I will then add the test for '--no-quiet' sets to 0 in the separate\n>> patch after OPT_COUNTUP() change.\n>\n> No, this and \"--no-verbose sets to 0\" are logically related to the\n> OPT__COUNTUP() change, thus would be incorporated into that patch.\n> Alternately, these two tests could just be part of \"improve test\n> coverage\" patch, establishing base behavior which the OPT__COUNTUP()\n> patch shouldn't break.\n\nI would prefer including it in \"improve test coverage\" patch to\nestablish the base behavior. This seems more natural to me.\n\nYou can expect this series from me within 2 days.\n\nThanks.\n"}]}