{"thread":{"id":"42187","subject":"[PATCH v15 1/7] t0040-test-parse-options.sh: fix style issues","startedAt":"2016-04-30T20:03:30Z","lastAt":"2016-05-09T16:01:55Z","messageCount":53,"participants":["Pranit Bauva","Junio C Hamano","Eric Sunshine","Stefan Beller","SZEDER Gábor","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":15,"patchTotal":7},"messages":[{"id":"285049","messageId":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":null,"subject":"[PATCH v15 1/7] t0040-test-parse-options.sh: fix style issues","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-30T20:03:30Z","receivedAt":"2016-04-30T20:03:30Z","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\nSigned-off-by: Pranit Bauva \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-- \n2.8.1\n"},{"id":"285050","messageId":"1462046616-2582-2-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v15 2/7] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-30T20:03:31Z","receivedAt":"2016-04-30T20:03:31Z","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 \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-- \n2.8.1\n"},{"id":"285051","messageId":"1462046616-2582-3-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v15 3/7] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-30T20:03:32Z","receivedAt":"2016-04-30T20:03:32Z","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---\nLink to v14:\n - $gmane/288880\n\nChanges wrt v14:\n - Change the test to use '-q -q -q --no-quiet' instead of just '--no-quiet'\n - Move the test for '--no-verbose' from OPT_COUNTUP patch to this one.\n\nSigned-off-by: Pranit Bauva \n---\n t/t0040-parse-options.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 57 insertions(+)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 450da45..57fc2a1 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -476,4 +476,61 @@ 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 -q -q -q --no-quiet >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-verbose sets verbose to 0' '\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 test_done\n-- \n2.8.1\n"},{"id":"285052","messageId":"1462046616-2582-4-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v15 4/7] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-30T20:03:33Z","receivedAt":"2016-04-30T20:03:33Z","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[v14] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n\nChanges wrt previous version (v14):\n - Remove a test and move it to a preparatory 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\nSigned-off-by: Pranit Bauva \n---\n Documentation/technical/api-parse-options.txt | 8 ++++++--\n parse-options.c | 2 ++\n t/t0040-parse-options.sh | 26 +++++++++++++-------------\n test-parse-options.c | 3 ++-\n 4 files changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 695bd4b..27bd701 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 encountered at\n+\tall, initialize `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 57fc2a1..9638ca0 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@@ -448,7 +448,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@@ -483,7 +483,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 +502,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-- \n2.8.1\n"},{"id":"285053","messageId":"1462046616-2582-5-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v15 5/7] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-30T20:03:34Z","receivedAt":"2016-04-30T20:03:34Z","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\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\nAlso use write_script() to create the fake \"editor\".\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\nSigned-off-by: Pranit Bauva \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-- \n2.8.1\n"},{"id":"285054","messageId":"1462046616-2582-6-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v15 6/7] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-30T20:03:35Z","receivedAt":"2016-04-30T20:03:35Z","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 v14:\n - Add the status related tests in a different patch after this patch.\n\nSigned-off-by: Pranit Bauva \n---\n Documentation/config.txt | 4 ++++\n Documentation/git-commit.txt | 3 ++-\n builtin/commit.c | 14 +++++++++++++-\n t/t7507-commit-verbose.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 65 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 42d2b50..8bf6040 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 391126e..114ffc9 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@@ -1364,6 +1366,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@@ -1515,6 +1519,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@@ -1664,6 +1673,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..2bb6d8d 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -98,4 +98,50 @@ 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_done\n-- \n2.8.1\n"},{"id":"285055","messageId":"1462046616-2582-7-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-04-30T20:03:36Z","receivedAt":"2016-04-30T20:03:36Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Variable named 'verbose' in builtin/commit.c is consumed by git-status\nand git-commit so if a new verbose related behavior is introduced in\ngit-commit, then it should not affect the behavior of git-status.\n\nOne previous commit (title: commit: add a commit.verbose config\nvariable) introduced a new config variable named commit.verbose,\nso care should be taken that it would not affect the behavior of\nstatus.\n\nAnother previous commit (title: \"parse-options.c: make OPTION_COUNTUP\nrespect \"unspecified\" values\") changes the initial value of verbose\nfrom 0 to -1. This can cause git-status to display a verbose output even\nwhen it isn't supposed to.\n\nSigned-off-by: Pranit Bauva \n\n---\nThis is a split off from the previous patch 6/6 as suggested by Eric\nSunshine.\n\nSigned-off-by: Pranit Bauva \n---\n t/t7507-commit-verbose.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2bb6d8d..00e0c3d 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -144,4 +144,14 @@ do\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-- \n2.8.1\n"},{"id":"285222","messageId":"xmqq7ffcqct1.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"1462046616-2582-7-git-send-email-pranit.bauva@gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T23:07:22Z","receivedAt":"2016-05-02T23:07:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Variable named 'verbose' in builtin/commit.c is consumed by git-status\n> and git-commit so if a new verbose related behavior is introduced in\n> git-commit, then it should not affect the behavior of git-status.\n>\n> One previous commit (title: commit: add a commit.verbose config\n> variable) introduced a new config variable named commit.verbose,\n> so care should be taken that it would not affect the behavior of\n> status.\n>\n> Another previous commit (title: \"parse-options.c: make OPTION_COUNTUP\n> respect \"unspecified\" values\") changes the initial value of verbose\n> from 0 to -1. This can cause git-status to display a verbose output even\n> when it isn't supposed to.\n>\n> Signed-off-by: Pranit Bauva \n>\n> ---\n> This is a split off from the previous patch 6/6 as suggested by Eric\n> Sunshine.\n\nIf these are documenting what your previous patches broke, then\nthere test body should describe what should happen, and then if it\nis broken, use test_expect_failure, no?\n\nYour first test does \"run status with commit.verbose is set, and\nmake sure the \"diff --git\" does not appear\", which is correct, so if\nit does not work, test_expect_failure would be the right thing to\nuse.\n\nThese, especially the latter, look rather unpleasant regressions to\nme, and the main commit.verbose change would need to be held back\nbefore they are fixed.\n\n> ---\n> t/t7507-commit-verbose.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 2bb6d8d..00e0c3d 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -144,4 +144,14 @@ do\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"},{"id":"285239","messageId":"CAFZEwPOAWh48YCxA3B+kRxVpkwN32OHW7Qrb9ajs2Cy0S8sjLw@mail.gmail.com","threadId":"42187","inReplyTo":"xmqq7ffcqct1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-03T03:39:20Z","receivedAt":"2016-05-03T03:39:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> Variable named 'verbose' in builtin/commit.c is consumed by git-status\n>> and git-commit so if a new verbose related behavior is introduced in\n>> git-commit, then it should not affect the behavior of git-status.\n>>\n>> One previous commit (title: commit: add a commit.verbose config\n>> variable) introduced a new config variable named commit.verbose,\n>> so care should be taken that it would not affect the behavior of\n>> status.\n>>\n>> Another previous commit (title: \"parse-options.c: make OPTION_COUNTUP\n>> respect \"unspecified\" values\") changes the initial value of verbose\n>> from 0 to -1. This can cause git-status to display a verbose output even\n>> when it isn't supposed to.\n>>\n>> Signed-off-by: Pranit Bauva \n>>\n>> ---\n>> This is a split off from the previous patch 6/6 as suggested by Eric\n>> Sunshine.\n>\n> If these are documenting what your previous patches broke, then\n> there test body should describe what should happen, and then if it\n> is broken, use test_expect_failure, no?\n>\n> Your first test does \"run status with commit.verbose is set, and\n> make sure the \"diff --git\" does not appear\", which is correct, so if\n> it does not work, test_expect_failure would be the right thing to\n> use.\n>\n> These, especially the latter, look rather unpleasant regressions to\n> me, and the main commit.verbose change would need to be held back\n> before they are fixed.\n\nI agree that using test_expect_failure would be a better way of going\nwith this thing. Thanks. Will send an updated patch for this.\n"},{"id":"285249","messageId":"CAPig+cR7pPHZv_z3G+BsLPqP7WYSVUb_7c2qmM+0y-TFeWjaSg@mail.gmail.com","threadId":"42187","inReplyTo":"CAFZEwPOAWh48YCxA3B+kRxVpkwN32OHW7Qrb9ajs2Cy0S8sjLw@mail.gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-03T05:12:32Z","receivedAt":"2016-05-03T05:12:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva wrote:\n> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano wrote:\n>> Pranit Bauva writes:\n>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status\n>>> and git-commit so if a new verbose related behavior is introduced in\n>>> git-commit, then it should not affect the behavior of git-status.\n>>>\n>>> One previous commit (title: commit: add a commit.verbose config\n>>> variable) introduced a new config variable named commit.verbose,\n>>> so care should be taken that it would not affect the behavior of\n>>> status.\n>>>\n>>> Another previous commit (title: \"parse-options.c: make OPTION_COUNTUP\n>>> respect \"unspecified\" values\") changes the initial value of verbose\n>>> from 0 to -1. This can cause git-status to display a verbose output even\n>>> when it isn't supposed to.\n>>>\n>>> Signed-off-by: Pranit Bauva \n>>\n>> If these are documenting what your previous patches broke, then\n>> there test body should describe what should happen, and then if it\n>> is broken, use test_expect_failure, no?\n>>\n>> Your first test does \"run status with commit.verbose is set, and\n>> make sure the \"diff --git\" does not appear\", which is correct, so if\n>> it does not work, test_expect_failure would be the right thing to\n>> use.\n>>\n>> These, especially the latter, look rather unpleasant regressions to\n>> me, and the main commit.verbose change would need to be held back\n>> before they are fixed.\n>\n> I agree that using test_expect_failure would be a better way of going\n> with this thing. Thanks. Will send an updated patch for this.\n\nPlease don't. test_expect_failure() is not warranted.\n\nStep back a moment and recall why these tests were added. Earlier\nrounds of this series were buggy and caused regressions in git-status.\nAs a consequence, reviewers suggested[1,2] that you improve test\ncoverage to ensure that such breakage is caught early.\n\nThe problems which caused the regressions were addressed in later\nversions of the series, thus using test_expect_success() is indeed\ncorrect, whereas test_expect_failure(), which illustrates broken\nbehavior, would be the wrong choice.\n\nThe point of these new tests is to prevent regressions caused by\n*subsequent* changes, which is why it was suggested that these tests\nbe added early (as a \"preparatory patch\"[3]), not at the very end of\nthe series as done here in v15.\n\nThis patch's commit message is perhaps a bit too detailed about what\ncould have gone wrong in earlier patches in this series; indeed, it\nmisled Junio into thinking that patches in this series did break\nbehavior, when in fact, it was instead previous rounds of this series\nwhich were buggy. If you instead make this a preparatory patch[3],\nthen you can sell it more simply by explaining that git-commit and\ngit-status share implementation (without necessarily going into detail\nabout exactly what is shared), and that you're improving test coverage\nto ensure that changes specific to git-commit don't accidentally\nimpact git-status, as well.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468\n"},{"id":"285251","messageId":"CAFZEwPMLcyAu67MrVWKpN2ytAFaB6rOj4ASUi3VG81DSS0Euiw@mail.gmail.com","threadId":"42187","inReplyTo":"CAPig+cR7pPHZv_z3G+BsLPqP7WYSVUb_7c2qmM+0y-TFeWjaSg@mail.gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-03T06:42:38Z","receivedAt":"2016-05-03T06:42:38Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine wrote:\n> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva wrote:\n>> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano wrote:\n>>> Pranit Bauva writes:\n>>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status\n>>>> and git-commit so if a new verbose related behavior is introduced in\n>>>> git-commit, then it should not affect the behavior of git-status.\n>>>>\n>>>> One previous commit (title: commit: add a commit.verbose config\n>>>> variable) introduced a new config variable named commit.verbose,\n>>>> so care should be taken that it would not affect the behavior of\n>>>> status.\n>>>>\n>>>> Another previous commit (title: \"parse-options.c: make OPTION_COUNTUP\n>>>> respect \"unspecified\" values\") changes the initial value of verbose\n>>>> from 0 to -1. This can cause git-status to display a verbose output even\n>>>> when it isn't supposed to.\n>>>>\n>>>> Signed-off-by: Pranit Bauva \n>>>\n>>> If these are documenting what your previous patches broke, then\n>>> there test body should describe what should happen, and then if it\n>>> is broken, use test_expect_failure, no?\n>>>\n>>> Your first test does \"run status with commit.verbose is set, and\n>>> make sure the \"diff --git\" does not appear\", which is correct, so if\n>>> it does not work, test_expect_failure would be the right thing to\n>>> use.\n>>>\n>>> These, especially the latter, look rather unpleasant regressions to\n>>> me, and the main commit.verbose change would need to be held back\n>>> before they are fixed.\n>>\n>> I agree that using test_expect_failure would be a better way of going\n>> with this thing. Thanks. Will send an updated patch for this.\n>\n> Please don't. test_expect_failure() is not warranted.\n\nI got confused between test_must_fail and test_expect_failure. I\nthought Junio mentioned to use test_must_fail and remove the \" ! \"\nsign.\n\n> Step back a moment and recall why these tests were added. Earlier\n> rounds of this series were buggy and caused regressions in git-status.\n> As a consequence, reviewers suggested[1,2] that you improve test\n> coverage to ensure that such breakage is caught early.\n>\n> The problems which caused the regressions were addressed in later\n> versions of the series, thus using test_expect_success() is indeed\n> correct, whereas test_expect_failure(), which illustrates broken\n> behavior, would be the wrong choice.\n>\n> The point of these new tests is to prevent regressions caused by\n> *subsequent* changes, which is why it was suggested that these tests\n> be added early (as a \"preparatory patch\"[3]), not at the very end of\n> the series as done here in v15.\n>\n> This patch's commit message is perhaps a bit too detailed about what\n> could have gone wrong in earlier patches in this series; indeed, it\n> misled Junio into thinking that patches in this series did break\n> behavior, when in fact, it was instead previous rounds of this series\n> which were buggy. If you instead make this a preparatory patch[3],\n> then you can sell it more simply by explaining that git-commit and\n> git-status share implementation (without necessarily going into detail\n> about exactly what is shared), and that you're improving test coverage\n> to ensure that changes specific to git-commit don't accidentally\n> impact git-status, as well.\n\nSure! I just wanted the commit message to be detailed as per the\nguidelines given by SubmittingPatches. I will swap the patch 6/7 and\npatch 7/7 changing the commit message. Also I will make the commit\nmessage less detailed.\n\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648\n> [2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730\n> [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468\n"},{"id":"285252","messageId":"CAPig+cQC0r6Lm9kOFQ2xukN-GiU0iTV5BNc7W8t4f0trkdtHsQ@mail.gmail.com","threadId":"42187","inReplyTo":"CAFZEwPMLcyAu67MrVWKpN2ytAFaB6rOj4ASUi3VG81DSS0Euiw@mail.gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-03T06:49:25Z","receivedAt":"2016-05-03T06:49:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva wrote:\n> On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine wrote:\n>> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva wrote:\n>>> I agree that using test_expect_failure would be a better way of going\n>>> with this thing. Thanks. Will send an updated patch for this.\n>>\n>> Please don't. test_expect_failure() is not warranted.\n>\n> I got confused between test_must_fail and test_expect_failure. I\n> thought Junio mentioned to use test_must_fail and remove the \" ! \"\n> sign.\n>\n>> Step back a moment and recall why these tests were added. Earlier\n>> rounds of this series were buggy and caused regressions in git-status.\n>> As a consequence, reviewers suggested[1,2] that you improve test\n>> coverage to ensure that such breakage is caught early.\n>>\n>> The problems which caused the regressions were addressed in later\n>> versions of the series, thus using test_expect_success() is indeed\n>> correct, whereas test_expect_failure(), which illustrates broken\n>> behavior, would be the wrong choice.\n>>\n>> The point of these new tests is to prevent regressions caused by\n>> *subsequent* changes, which is why it was suggested that these tests\n>> be added early (as a \"preparatory patch\"[3]), not at the very end of\n>> the series as done here in v15.\n>>\n>> This patch's commit message is perhaps a bit too detailed about what\n>> could have gone wrong in earlier patches in this series; indeed, it\n>> misled Junio into thinking that patches in this series did break\n>> behavior, when in fact, it was instead previous rounds of this series\n>> which were buggy. If you instead make this a preparatory patch[3],\n>> then you can sell it more simply by explaining that git-commit and\n>> git-status share implementation (without necessarily going into detail\n>> about exactly what is shared), and that you're improving test coverage\n>> to ensure that changes specific to git-commit don't accidentally\n>> impact git-status, as well.\n>\n> Sure! I just wanted the commit message to be detailed as per the\n> guidelines given by SubmittingPatches. I will swap the patch 6/7 and\n> patch 7/7 changing the commit message. Also I will make the commit\n> message less detailed.\n\nThis patch should be inserted before 4/7 since it needs to protect\nagainst breakage which might occur when 4/7 changes the behavior of\nOPTION_COUNTUP.\n"},{"id":"285269","messageId":"CAFZEwPOYi0rv-WhVuV5ALwd=2_w2F2aeKN61EoZxswQQRGqcnA@mail.gmail.com","threadId":"42187","inReplyTo":"CAPig+cQC0r6Lm9kOFQ2xukN-GiU0iTV5BNc7W8t4f0trkdtHsQ@mail.gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-03T09:18:24Z","receivedAt":"2016-05-03T09:18:24Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine wrote:\n> On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva wrote:\n>> On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine wrote:\n>>> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva wrote:\n>>>> I agree that using test_expect_failure would be a better way of going\n>>>> with this thing. Thanks. Will send an updated patch for this.\n>>>\n>>> Please don't. test_expect_failure() is not warranted.\n>>\n>> I got confused between test_must_fail and test_expect_failure. I\n>> thought Junio mentioned to use test_must_fail and remove the \" ! \"\n>> sign.\n>>\n>>> Step back a moment and recall why these tests were added. Earlier\n>>> rounds of this series were buggy and caused regressions in git-status.\n>>> As a consequence, reviewers suggested[1,2] that you improve test\n>>> coverage to ensure that such breakage is caught early.\n>>>\n>>> The problems which caused the regressions were addressed in later\n>>> versions of the series, thus using test_expect_success() is indeed\n>>> correct, whereas test_expect_failure(), which illustrates broken\n>>> behavior, would be the wrong choice.\n>>>\n>>> The point of these new tests is to prevent regressions caused by\n>>> *subsequent* changes, which is why it was suggested that these tests\n>>> be added early (as a \"preparatory patch\"[3]), not at the very end of\n>>> the series as done here in v15.\n>>>\n>>> This patch's commit message is perhaps a bit too detailed about what\n>>> could have gone wrong in earlier patches in this series; indeed, it\n>>> misled Junio into thinking that patches in this series did break\n>>> behavior, when in fact, it was instead previous rounds of this series\n>>> which were buggy. If you instead make this a preparatory patch[3],\n>>> then you can sell it more simply by explaining that git-commit and\n>>> git-status share implementation (without necessarily going into detail\n>>> about exactly what is shared), and that you're improving test coverage\n>>> to ensure that changes specific to git-commit don't accidentally\n>>> impact git-status, as well.\n>>\n>> Sure! I just wanted the commit message to be detailed as per the\n>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and\n>> patch 7/7 changing the commit message. Also I will make the commit\n>> message less detailed.\n>\n> This patch should be inserted before 4/7 since it needs to protect\n> against breakage which might occur when 4/7 changes the behavior of\n> OPTION_COUNTUP.\n\nI forgot to mention about this earlier. When I was rebasing, this stroked me.\nI guess making any changes in ordering the commits will make one of\nthe test as absurd. One of the test uses a configuration variable\n'commit.verbose' will won't be effective before the patch 6/7. So I\nguess I will have to only change the commit message to reflect as\n\"improving test coverage\".\n"},{"id":"285327","messageId":"xmqq4mafp2hg.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"CAPig+cR7pPHZv_z3G+BsLPqP7WYSVUb_7c2qmM+0y-TFeWjaSg@mail.gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-03T15:47:55Z","receivedAt":"2016-05-03T15:47:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>>>> One previous commit (title: commit: add a commit.verbose config\n>>>> variable) introduced a new config variable named commit.verbose,\n>>>> so care should be taken that it would not affect the behavior of\n>>>> status.\n>>>>\n>>>> Another previous commit (title: \"parse-options.c: make OPTION_COUNTUP\n>>>> respect \"unspecified\" values\") changes the initial value of verbose\n>>>> from 0 to -1. This can cause git-status to display a verbose output even\n>>>> when it isn't supposed to.\n>>>> ...\n>\n> This patch's commit message is perhaps a bit too detailed about what\n> could have gone wrong in earlier patches in this series; indeed, it\n> misled Junio into thinking that patches in this series did break\n> behavior, when in fact, it was instead previous rounds of this series\n> which were buggy.\n\nIndeed. Please forget everything I said about expect-failure, if\nthe top two paragraphs are describing breakages that this series\ndoes *NOT* introduce. I was misled by them--and others will, too.\nThese two paragraphs do not belong to the log message.\n\nThanks for clarifying.\n"},{"id":"285331","messageId":"CAPig+cRa4z2ZUsQ7a-w1MT8S=haaqFeELK8fjwp62BZLBfnnQQ@mail.gmail.com","threadId":"42187","inReplyTo":"CAFZEwPOYi0rv-WhVuV5ALwd=2_w2F2aeKN61EoZxswQQRGqcnA@mail.gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-03T16:17:00Z","receivedAt":"2016-05-03T16:17:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva wrote:\n> On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine wrote:\n>>>> Step back a moment and recall why these tests were added. Earlier\n>>>> rounds of this series were buggy and caused regressions in git-status.\n>>>> As a consequence, reviewers suggested[1,2] that you improve test\n>>>> coverage to ensure that such breakage is caught early.\n>>>>\n>>>> The point of these new tests is to prevent regressions caused by\n>>>> *subsequent* changes, which is why it was suggested that these tests\n>>>> be added early (as a \"preparatory patch\"[3]), not at the very end of\n>>>> the series as done here in v15.\n>>>\n>>> Sure! I just wanted the commit message to be detailed as per the\n>>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and\n>>> patch 7/7 changing the commit message. Also I will make the commit\n>>> message less detailed.\n>>\n>> This patch should be inserted before 4/7 since it needs to protect\n>> against breakage which might occur when 4/7 changes the behavior of\n>> OPTION_COUNTUP.\n>\n> I forgot to mention about this earlier. When I was rebasing, this stroked me.\n> I guess making any changes in ordering the commits will make one of\n> the test as absurd. One of the test uses a configuration variable\n> 'commit.verbose' will won't be effective before the patch 6/7. So I\n> guess I will have to only change the commit message to reflect as\n> \"improving test coverage\".\n\nI also had intended to talk about this but forgot. What would be quite\nlogical is to introduce only the \"git-status without --verbose\" test\nin this new \"improve coverage\" patch before 4/7. The other test, which\nensures that git-status doesn't regress with commit.verbose, would\nthen very naturally be included in the patch which adds the\ncommit.verbose functionality (currently patch 6/7).\n"},{"id":"285332","messageId":"CAFZEwPMef2TnFS9yT6Gh-L8MwnWogzsNthktAN4CxjOxbY5JFw@mail.gmail.com","threadId":"42187","inReplyTo":"CAPig+cRa4z2ZUsQ7a-w1MT8S=haaqFeELK8fjwp62BZLBfnnQQ@mail.gmail.com","subject":"Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-03T16:18:17Z","receivedAt":"2016-05-03T16:18:17Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Tue, May 3, 2016 at 9:47 PM, Eric Sunshine wrote:\n> On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva wrote:\n>> On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine wrote:\n>>>>> Step back a moment and recall why these tests were added. Earlier\n>>>>> rounds of this series were buggy and caused regressions in git-status.\n>>>>> As a consequence, reviewers suggested[1,2] that you improve test\n>>>>> coverage to ensure that such breakage is caught early.\n>>>>>\n>>>>> The point of these new tests is to prevent regressions caused by\n>>>>> *subsequent* changes, which is why it was suggested that these tests\n>>>>> be added early (as a \"preparatory patch\"[3]), not at the very end of\n>>>>> the series as done here in v15.\n>>>>\n>>>> Sure! I just wanted the commit message to be detailed as per the\n>>>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and\n>>>> patch 7/7 changing the commit message. Also I will make the commit\n>>>> message less detailed.\n>>>\n>>> This patch should be inserted before 4/7 since it needs to protect\n>>> against breakage which might occur when 4/7 changes the behavior of\n>>> OPTION_COUNTUP.\n>>\n>> I forgot to mention about this earlier. When I was rebasing, this stroked me.\n>> I guess making any changes in ordering the commits will make one of\n>> the test as absurd. One of the test uses a configuration variable\n>> 'commit.verbose' will won't be effective before the patch 6/7. So I\n>> guess I will have to only change the commit message to reflect as\n>> \"improving test coverage\".\n>\n> I also had intended to talk about this but forgot. What would be quite\n> logical is to introduce only the \"git-status without --verbose\" test\n> in this new \"improve coverage\" patch before 4/7. The other test, which\n> ensures that git-status doesn't regress with commit.verbose, would\n> then very naturally be included in the patch which adds the\n> commit.verbose functionality (currently patch 6/7).\n\nSure. Will do. Thanks!\n"},{"id":"285441","messageId":"CAPig+cTW9=w8fMTwcF-Z6DPmXMM4qiWZA=7S1GcFEoGG8hPUtw@mail.gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-3-git-send-email-pranit.bauva@gmail.com","subject":"Re: [PATCH v15 3/7] t0040-parse-options: improve test coverage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-04T08:36:54Z","receivedAt":"2016-05-04T08:36:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 30, 2016 at 4:03 PM, Pranit Bauva 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\nAs this patch is also adding a test of --[no-]verbose, the commit\nmessage should mention it.\n\nMore below...\n\n> Signed-off-by: Pranit Bauva \n> ---\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> @@ -476,4 +476,61 @@ test_expect_success '--no-list resets list' '\n> +test_expect_success 'multiple quiet levels' '\n> + test-parse-options -q -q -q >output 2>output.err &&\n> + test_must_be_empty output.err &&\n> + test_cmp expect output\n> +'\n> +\n> +test_expect_success '--no-quiet sets quiet to 0' '\n> + test-parse-options -q -q -q --no-quiet >output 2>output.err &&\n> + test_must_be_empty output.err &&\n> + test_cmp expect output\n> +'\n\nIt wouldn't hurt to have two tests for --no-quiet: one which tests\n--no-quiet alone to ensure that 'quiet' *remains* at 0, and one which\ntests --no-quiet in combination with some --quiet's to ensure that\n'quiet' is *reset* to 0. These tests would give you good coverage for\nchanges by subsequent patches, such as the OPTION_COUNTUP patch which\nflips the initial value to -1.\n\n> +\n> +test_expect_success '--no-verbose sets verbose to 0' '\n> + test-parse-options --no-verbose >output 2> output.err &&\n> + test_must_be_empty output.err &&\n> + test_cmp expect output\n> +'\n\nOne would expect to see 'verbose' get the same treatment of having a\ntest invoke --verbose multiple times. (Yes, I realize that the \"long\noptions\" test does just this, but testing multiple --verbose's is not\nits primary purpose, so having a test which does test multiple\n--verbose's as its primary purpose can be beneficial and is less\nlikely to be broken by someone in the future.)\n"},{"id":"285554","messageId":"CAFZEwPN84Qr2z4M7DQFWda3JO7Ut-4FWO5G8+ExwSobwcyM6Fw@mail.gmail.com","threadId":"42187","inReplyTo":"CAPig+cTW9=w8fMTwcF-Z6DPmXMM4qiWZA=7S1GcFEoGG8hPUtw@mail.gmail.com","subject":"Re: [PATCH v15 3/7] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T04:46:03Z","receivedAt":"2016-05-05T04:46:03Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Wed, May 4, 2016 at 2:06 PM, Eric Sunshine wrote:\n> On Sat, Apr 30, 2016 at 4:03 PM, Pranit Bauva 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> As this patch is also adding a test of --[no-]verbose, the commit\n> message should mention it.\n\nWill include this in commit message.\n\n>\n> More below...\n>\n>> Signed-off-by: Pranit Bauva \n>> ---\n>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n>> @@ -476,4 +476,61 @@ test_expect_success '--no-list resets list' '\n>> +test_expect_success 'multiple quiet levels' '\n>> + test-parse-options -q -q -q >output 2>output.err &&\n>> + test_must_be_empty output.err &&\n>> + test_cmp expect output\n>> +'\n>> +\n>> +test_expect_success '--no-quiet sets quiet to 0' '\n>> + test-parse-options -q -q -q --no-quiet >output 2>output.err &&\n>> + test_must_be_empty output.err &&\n>> + test_cmp expect output\n>> +'\n>\n> It wouldn't hurt to have two tests for --no-quiet: one which tests\n> --no-quiet alone to ensure that 'quiet' *remains* at 0, and one which\n> tests --no-quiet in combination with some --quiet's to ensure that\n> 'quiet' is *reset* to 0. These tests would give you good coverage for\n> changes by subsequent patches, such as the OPTION_COUNTUP patch which\n> flips the initial value to -1.\n\nWill add them\n\n>> +\n>> +test_expect_success '--no-verbose sets verbose to 0' '\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> One would expect to see 'verbose' get the same treatment of having a\n> test invoke --verbose multiple times. (Yes, I realize that the \"long\n> options\" test does just this, but testing multiple --verbose's is not\n> its primary purpose, so having a test which does test multiple\n> --verbose's as its primary purpose can be beneficial and is less\n> likely to be broken by someone in the future.)\n\nSure. Having another test dedicated wouldn't hurt.\n"},{"id":"285557","messageId":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462046616-2582-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 0/7] config commit verbose","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:49:55Z","receivedAt":"2016-05-05T09:49:55Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"This series of patches add a configuration variable for verbose in\ngit-commit.\n\nLink to v15:\nhttp://thread.gmane.org/gmane.comp.version-control.git/293127\n\nChanges wrt v15:\n * Remove the previous patch 7/7 and split the tests. Include one in\n initial patch 6/7. The other one is introduced in a separate commit\n after 4/7.\n * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,\n --no-verbose with -v as suggested by Eric Sunshine\n\nPranit Bauva (7):\n t0040-test-parse-options.sh: fix style issues\n test-parse-options: print quiet as integer\n t0040-parse-options: improve test coverage\n t/t7507: improve test coverage\n parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values\n t7507-commit-verbose: improve test coverage by testing number of diffs\n commit: add a commit.verbose config variable\n\n Documentation/config.txt | 4 +\n Documentation/git-commit.txt | 3 +-\n Documentation/technical/api-parse-options.txt | 8 +-\n builtin/commit.c | 14 +-\n parse-options.c | 2 +\n t/t0040-parse-options.sh | 238 +++++++++++++++++++-------\n t/t7507-commit-verbose.sh | 72 +++++++-\n test-parse-options.c | 5 +-\n 8 files changed, 271 insertions(+), 75 deletions(-)\n\n-- \n2.8.1\n"},{"id":"285561","messageId":"1462441802-4768-2-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 1/7] t0040-test-parse-options.sh: fix style issues","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:49:56Z","receivedAt":"2016-05-05T09:49:56Z","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---\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-- \n2.8.1\n"},{"id":"285558","messageId":"1462441802-4768-3-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 2/7] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:49:57Z","receivedAt":"2016-05-05T09:49:57Z","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 \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-- \n2.8.1\n"},{"id":"285560","messageId":"1462441802-4768-4-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 3/7] t0040-parse-options: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:49:58Z","receivedAt":"2016-05-05T09:49:58Z","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 the\nbehavior of '--no-quiet'.\n\nInclude tests to check for multiple levels of verbose and to check the\nbehavior of '--no-verbose'.\n\nSigned-off-by: Pranit Bauva \n\n---\n t/t0040-parse-options.sh | 114 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 114 insertions(+)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 450da45..717a514 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -476,4 +476,118 @@ 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: 3\n+quiet: 0\n+dry run: no\n+file: (not set)\n+EOF\n+\n+test_expect_success 'multiple verbose levels' '\n+\ttest-parse-options -v -v -v >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+\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 resets multiple -q to 0' '\n+\ttest-parse-options -q -q -q --no-quiet >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-verbose sets verbose to 0' '\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: 0\n+quiet: 0\n+dry run: no\n+file: (not set)\n+EOF\n+\n+test_expect_success '--no-verbose resets multiple verbose to 0' '\n+\ttest-parse-options -v -v -v --no-verbose >output 2>output.err &&\n+\ttest_must_be_empty output.err &&\n+\ttest_cmp expect output\n+'\n+\n test_done\n-- \n2.8.1\n"},{"id":"285562","messageId":"1462441802-4768-5-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 4/7] t/t7507: improve test coverage","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:49:59Z","receivedAt":"2016-05-05T09:49:59Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"git-commit and git-status share the same implementation thus it is\nnecessary to ensure that changes specific to git-commit don't\naccidentally impact git-status.\n\nThis test verifies that changes made to verbose in git-commit does not\nimpact git-status.\n\nSigned-off-by: Pranit Bauva \n---\n t/t7507-commit-verbose.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..a3c8582 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -96,4 +96,9 @@ 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 'status does not verbose without --verbose' '\n+\tgit status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n test_done\n-- \n2.8.1\n"},{"id":"285559","messageId":"1462441802-4768-6-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 5/7] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:50:00Z","receivedAt":"2016-05-05T09:50:00Z","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\n---\n Documentation/technical/api-parse-options.txt | 8 ++++++--\n parse-options.c | 2 ++\n t/t0040-parse-options.sh | 28 +++++++++++++--------------\n test-parse-options.c | 3 ++-\n 4 files changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 695bd4b..27bd701 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 encountered at\n+\tall, initialize `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 717a514..fec3fef 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@@ -448,7 +448,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@@ -483,7 +483,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@@ -521,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)\n@@ -540,7 +540,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-- \n2.8.1\n"},{"id":"285564","messageId":"1462441802-4768-7-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 6/7] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:50:01Z","receivedAt":"2016-05-05T09:50:01Z","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\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\nAlso use write_script() to create the fake \"editor\".\n\nHelped-by: Eric Sunshine \nSigned-off-by: Pranit Bauva \n\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 a3c8582..5a81181 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-- \n2.8.1\n"},{"id":"285563","messageId":"1462441802-4768-8-git-send-email-pranit.bauva@gmail.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"[PATCH v16 7/7] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-05T09:50:02Z","receivedAt":"2016-05-05T09:50:02Z","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\nAdd tests to check the behavior introduced by this commit and also to\nverify that behavior of status doesn't break because of this commit.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Pranit Bauva \n\n---\n Documentation/config.txt | 4 ++++\n Documentation/git-commit.txt | 3 ++-\n builtin/commit.c | 14 +++++++++++-\n t/t7507-commit-verbose.sh | 51 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 70 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 42d2b50..8bf6040 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 391126e..114ffc9 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@@ -1364,6 +1366,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@@ -1515,6 +1519,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@@ -1664,6 +1673,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 5a81181..ed2653d 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -103,4 +103,55 @@ test_expect_success 'status does not verbose without --verbose' '\n \t! grep \"^diff --git\" actual\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_done\n-- \n2.8.1\n"},{"id":"285585","messageId":"xmqqbn4kb9m1.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"1462441802-4768-8-git-send-email-pranit.bauva@gmail.com","subject":"Re: [PATCH v16 7/7] commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T19:14:30Z","receivedAt":"2016-05-05T19:14:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 391126e..114ffc9 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\nThe name does not make it clear that config_verbose is only for\n\"commit\" and not relevant to \"status\".\n\n> @@ -1364,6 +1366,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\nMental note: cmd_status() does not use git_commit_config() but uses\ngit_status_config(), hence config_verbose is not affected. But\nbecause verbose is initialised to -1, the code needs to turn it off\nlike this.\n\n> @@ -1664,6 +1673,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\ncmd_commit() does use git_commit_config(), and verbose is\ninitialised -1, so without command line option, we fall back to\nconfig_verbose if it is set from the configuration.\n\nI wonder if the attached patch squashed into this commit makes\nthings easier to understand, though. The points are:\n\n - We rename the configuration to make it clear that it is about\n \"commit\" and does not apply to \"status\".\n\n - We initialize verbose to 0 as before. The only thing \"git\n status\" cares about is if \"--verbose\" was given. Giving it\n \"--no-verbose\" or nothing should not make any difference.\n\n - But we do need to stuff -1 to verbose in \"git commit\" before\n handling the command line options, because the distinction\n between having \"--no-verbose\" and not having any matter there,\n and we do so in cmd_commit(), i.e. only place where it matters.\n\n builtin/commit.c | 12 +++++-------\n 1 file changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 583d1e3..a486620 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -113,9 +113,8 @@ 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 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 quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n+static int config_commit_verbose = -1; /* unspecified */\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@@ -1366,8 +1365,6 @@ 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@@ -1521,7 +1518,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)\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\tconfig_commit_verbose = git_config_bool_or_int(k, v, &is_bool);\n \t\treturn 0;\n \t}\n \n@@ -1670,11 +1667,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tif (parse_commit(current_head))\n \t\t\tdie(_(\"could not parse HEAD commit\"));\n \t}\n+\tverbose = -1; /* unspecified */\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+\t\tverbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;\n \n \tif (dry_run)\n \t\treturn dry_run_commit(argc, argv, prefix, current_head, &s);\n"},{"id":"285588","messageId":"xmqq7ff8b99q.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"1462441802-4768-1-git-send-email-pranit.bauva@gmail.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T19:21:53Z","receivedAt":"2016-05-05T19:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> This series of patches add a configuration variable for verbose in\n> git-commit.\n>\n> Link to v15:\n> http://thread.gmane.org/gmane.comp.version-control.git/293127\n>\n> Changes wrt v15:\n> * Remove the previous patch 7/7 and split the tests. Include one in\n> initial patch 6/7. The other one is introduced in a separate commit\n> after 4/7.\n> * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,\n> --no-verbose with -v as suggested by Eric Sunshine\n\nThanks for a pleasant read. Modulo minor readability nits I sent\nseparately on 7/7, this looked good.\n\nA tangent that we may want to think about after this series lands\nand dust settles is to make test-parse-options simpler to use. I\nsee many instances of this repeated:\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 test-parse-options -q -q -q >output 2>output.err &&\n test_must_be_empty output.err &&\n test_cmp expect output\n '\n\nBut the only thing this test cares about is if \"quiet: 3\" is in the\noutput. I think we should be able to write the above 18 lines with\njust four lines, like this:\n\n\ttest_expect_success 'multiple quiet levels' '\n\t\ttest-parse-options --expect=\"quiet: 3\" -q -q -q\n\t'\n\nThere may be a handful of tests that care about more than one\nvariable, and the current output format must be used when the\nnew --expect option is not given, but I suspect that the majority of\ntests would want the concise form.\n"},{"id":"285628","messageId":"20160505215056.28224-1-gitster@pobox.com","threadId":"42187","inReplyTo":"xmqq7ff8b99q.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/3] test-parse-options update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T21:50:53Z","receivedAt":"2016-05-05T21:50:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"During the review of Pranit's \"commit.verbose\" series, I noticed an\noverly verbose input and output used to drive test-parse-options\nhelper in t0040. Here is a patch to teach the program to allow us\nto write the test in a more concise way.\n\nI'll leave it as an exercise to the readers to actually use this to\nconvert tests in t0040. That needs to wait until Pranit's series\nis merged and the dust settles.\n\nJunio C Hamano (3):\n test-parse-options: fix output when callback option fails\n test-parse-options: hold output in a strbuf\n test-parse-options: --expect= option to simplify tests\n\n t/t0040-parse-options.sh | 5 +--\n test-parse-options.c | 110 ++++++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 97 insertions(+), 18 deletions(-)\n\n-- \n2.8.2-505-gdbd0e1d\n"},{"id":"285629","messageId":"20160505215056.28224-2-gitster@pobox.com","threadId":"42187","inReplyTo":"20160505215056.28224-1-gitster@pobox.com","subject":"[PATCH 1/3] test-parse-options: fix output when callback option fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T21:50:54Z","receivedAt":"2016-05-05T21:50:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When test-parse-options detects an error on the command line, it\ngives the usage string just like any parse-options API users do,\nwithout showing any \"variable dump\". An exception is the callback\ntest, where a \"variable dump\" for the option is done before the\ncommand line options are fully parsed.\n\nDo not expose this implementation detail by separating the handling\nof callback test into two phases, one to capture the fact that an\noption was given during the option parsing phase, and the other to\nshow that fact as a part of normal \"variable dump\".\n\nThe effect of this fix is seen in the patch to t/t0040 where it\ntried \"test-parse-options --no-length\" where \"--length\" is a callback\nthat does not take a negative form.\n\nSigned-off-by: Junio C Hamano \n---\n t/t0040-parse-options.sh | 4 +---\n test-parse-options.c | 18 ++++++++++++++++--\n 2 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex fec3fef..dbaee55 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -356,9 +356,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '\n \ttest_cmp expect output\n '\n \n-cat >expect <<\\EOF\n-Callback: \"not set\", 1\n-EOF\n+>expect\n \n test_expect_success 'OPT_CALLBACK() and callback errors work' '\n \ttest_must_fail test-parse-options --no-length >output 2>output.err &&\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex f02c275..b5f4e90 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -14,10 +14,18 @@ static char *file = NULL;\n static int ambiguous;\n static struct string_list list;\n \n+static struct {\n+\tint called;\n+\tconst char *arg;\n+\tint unset;\n+} length_cb;\n+\n static int length_callback(const struct option *opt, const char *arg, int unset)\n {\n-\tprintf(\"Callback: \\\"%s\\\", %d\\n\",\n-\t\t(arg ? arg : \"not set\"), unset);\n+\tlength_cb.called = 1;\n+\tlength_cb.arg = arg;\n+\tlength_cb.unset = unset;\n+\n \tif (unset)\n \t\treturn 1; /* do not support unset */\n \n@@ -84,6 +92,12 @@ int main(int argc, char **argv)\n \n \targc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);\n \n+\tif (length_cb.called) {\n+\t\tconst char *arg = length_cb.arg;\n+\t\tint unset = length_cb.unset;\n+\t\tprintf(\"Callback: \\\"%s\\\", %d\\n\",\n+\t\t (arg ? arg : \"not set\"), unset);\n+\t}\n \tprintf(\"boolean: %d\\n\", boolean);\n \tprintf(\"integer: %d\\n\", integer);\n \tprintf(\"magnitude: %lu\\n\", magnitude);\n-- \n2.8.2-505-gdbd0e1d\n"},{"id":"285630","messageId":"20160505215056.28224-3-gitster@pobox.com","threadId":"42187","inReplyTo":"20160505215056.28224-1-gitster@pobox.com","subject":"[PATCH 2/3] test-parse-options: hold output in a strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T21:50:55Z","receivedAt":"2016-05-05T21:50:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In this step, all the output is held in a strbuf and unconditionally\ndumped at the end, so there is no behaviour change (other than that\nthe processing may be a bit slower, now we do the buffering stdio\nhas been doing for us).\n\nSigned-off-by: Junio C Hamano \n---\n test-parse-options.c | 30 +++++++++++++++++-------------\n 1 file changed, 17 insertions(+), 13 deletions(-)\n\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex b5f4e90..3db4332 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"parse-options.h\"\n #include \"string-list.h\"\n+#include \"strbuf.h\"\n \n static int boolean = 0;\n static int integer = 0;\n@@ -89,31 +90,34 @@ int main(int argc, char **argv)\n \t\tOPT_END(),\n \t};\n \tint i;\n+\tstruct strbuf output = STRBUF_INIT;\n \n \targc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);\n \n \tif (length_cb.called) {\n \t\tconst char *arg = length_cb.arg;\n \t\tint unset = length_cb.unset;\n-\t\tprintf(\"Callback: \\\"%s\\\", %d\\n\",\n+\t\tstrbuf_addf(&output, \"Callback: \\\"%s\\\", %d\\n\",\n \t\t (arg ? arg : \"not set\"), unset);\n \t}\n-\tprintf(\"boolean: %d\\n\", boolean);\n-\tprintf(\"integer: %d\\n\", integer);\n-\tprintf(\"magnitude: %lu\\n\", magnitude);\n-\tprintf(\"timestamp: %lu\\n\", timestamp);\n-\tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n-\tprintf(\"abbrev: %d\\n\", abbrev);\n-\tprintf(\"verbose: %d\\n\", verbose);\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+\tstrbuf_addf(&output, \"boolean: %d\\n\", boolean);\n+\tstrbuf_addf(&output, \"integer: %d\\n\", integer);\n+\tstrbuf_addf(&output, \"magnitude: %lu\\n\", magnitude);\n+\tstrbuf_addf(&output, \"timestamp: %lu\\n\", timestamp);\n+\tstrbuf_addf(&output, \"string: %s\\n\", string ? string : \"(not set)\");\n+\tstrbuf_addf(&output, \"abbrev: %d\\n\", abbrev);\n+\tstrbuf_addf(&output, \"verbose: %d\\n\", verbose);\n+\tstrbuf_addf(&output, \"quiet: %d\\n\", quiet);\n+\tstrbuf_addf(&output, \"dry run: %s\\n\", dry_run ? \"yes\" : \"no\");\n+\tstrbuf_addf(&output, \"file: %s\\n\", file ? file : \"(not set)\");\n \n \tfor (i = 0; i < list.nr; i++)\n-\t\tprintf(\"list: %s\\n\", list.items[i].string);\n+\t\tstrbuf_addf(&output, \"list: %s\\n\", list.items[i].string);\n \n \tfor (i = 0; i < argc; i++)\n-\t\tprintf(\"arg %02d: %s\\n\", i, argv[i]);\n+\t\tstrbuf_addf(&output, \"arg %02d: %s\\n\", i, argv[i]);\n+\n+\tprintf(\"%s\", output.buf);\n \n \treturn 0;\n }\n-- \n2.8.2-505-gdbd0e1d\n"},{"id":"285631","messageId":"20160505215056.28224-4-gitster@pobox.com","threadId":"42187","inReplyTo":"20160505215056.28224-1-gitster@pobox.com","subject":"[PATCH 3/3] test-parse-options: --expect= option to simplify tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-05T21:50:56Z","receivedAt":"2016-05-05T21:50:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Existing tests in t0040 follow a rather verbose pattern:\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 test-parse-options -q -q -q >output 2>output.err &&\n test_must_be_empty output.err &&\n test_cmp expect output\n '\n\nBut the only thing this test cares about is if \"quiet: 3\" is in the\noutput. We should be able to write the above 18 lines with just\nfour lines, like this:\n\n\ttest_expect_success 'multiple quiet levels' '\n\t\ttest-parse-options --expect=\"quiet: 3\" -q -q -q\n\t'\n\nTeach the new --expect= option to test-parse-options helper.\n\nSigned-off-by: Junio C Hamano \n---\n t/t0040-parse-options.sh | 1 +\n test-parse-options.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 66 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex dbaee55..d678fbf 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -45,6 +45,7 @@ Standard options\n -v, --verbose be verbose\n -n, --dry-run dry run\n -q, --quiet be quiet\n+ --expect expected output in the variable dump\n \n EOF\n \ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 3db4332..010f3b2 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -14,6 +14,7 @@ static char *string = NULL;\n static char *file = NULL;\n static int ambiguous;\n static struct string_list list;\n+static struct string_list expect;\n \n static struct {\n \tint called;\n@@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+/*\n+ * See if expect->string (\"label: value\") has a line in output that\n+ * begins with \"label:\", and if the line in output matches it.\n+ */\n+static int match_line(struct string_list_item *expect, struct strbuf *output)\n+{\n+\tconst char *label = expect->string;\n+\tconst char *colon = strchr(label, ':');\n+\tconst char *scan = output->buf;\n+\tsize_t label_len, expect_len;\n+\n+\tif (!colon)\n+\t\tdie(\"Malformed --expect value: %s\", label);\n+\tlabel_len = colon - label;\n+\n+\twhile (scan < output->buf + output->len) {\n+\t\tconst char *next;\n+\t\tscan = memmem(scan, output->buf + output->len - scan,\n+\t\t\t label, label_len);\n+\t\tif (!scan)\n+\t\t\treturn 0;\n+\t\tif (scan == output->buf || scan[-1] == '\\n')\n+\t\t\tbreak;\n+\t\tnext = strchr(scan + label_len, '\\n');\n+\t\tif (!next)\n+\t\t\treturn 0;\n+\t\tscan = next + 1;\n+\t}\n+\n+\t/*\n+\t * scan points at a line that begins with the label we are\n+\t * looking for. Does it match?\n+\t */\n+\texpect_len = strlen(expect->string);\n+\n+\tif (output->buf + output->len <= scan + expect_len)\n+\t\treturn 0; /* value not long enough */\n+\tif (memcmp(scan, expect->string, expect_len))\n+\t\treturn 0; /* does not match */\n+\n+\treturn (scan + expect_len < output->buf + output->len &&\n+\t\tscan[expect_len] == '\\n');\n+}\n+\n+static int show_expected(struct string_list *list, struct strbuf *output)\n+{\n+\tstruct string_list_item *expect;\n+\tint found_mismatch = 0;\n+\n+\tfor_each_string_list_item(expect, list) {\n+\t\tif (!match_line(expect, output))\n+\t\t\tfound_mismatch = 1;\n+\t}\n+\treturn found_mismatch;\n+}\n+\n int main(int argc, char **argv)\n {\n \tconst char *prefix = \"prefix/\";\n@@ -87,6 +144,8 @@ int main(int argc, char **argv)\n \t\tOPT__VERBOSE(&verbose, \"be verbose\"),\n \t\tOPT__DRY_RUN(&dry_run, \"dry run\"),\n \t\tOPT__QUIET(&quiet, \"be quiet\"),\n+\t\tOPT_STRING_LIST(0, \"expect\", &expect, \"string\",\n+\t\t\t\t\"expected output in the variable dump\"),\n \t\tOPT_END(),\n \t};\n \tint i;\n@@ -117,7 +176,10 @@ int main(int argc, char **argv)\n \tfor (i = 0; i < argc; i++)\n \t\tstrbuf_addf(&output, \"arg %02d: %s\\n\", i, argv[i]);\n \n-\tprintf(\"%s\", output.buf);\n-\n-\treturn 0;\n+\tif (expect.nr)\n+\t\treturn show_expected(&expect, &output);\n+\telse {\n+\t\tprintf(\"%s\", output.buf);\n+\t\treturn 0;\n+\t}\n }\n-- \n2.8.2-505-gdbd0e1d\n"},{"id":"285654","messageId":"CAGZ79kY+9BUjcbpSA8sAqd=qZ5niZ2CDsPeGuXhK+yqZY4hL9Q@mail.gmail.com","threadId":"42187","inReplyTo":"20160505215056.28224-4-gitster@pobox.com","subject":"Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-06T00:41:03Z","receivedAt":"2016-05-06T00:41:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano wrote:\n> Existing tests in t0040 follow a rather verbose pattern:\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> test-parse-options -q -q -q >output 2>output.err &&\n> test_must_be_empty output.err &&\n> test_cmp expect output\n> '\n>\n> But the only thing this test cares about is if \"quiet: 3\" is in the\n> output. We should be able to write the above 18 lines with just\n> four lines, like this:\n>\n> test_expect_success 'multiple quiet levels' '\n> test-parse-options --expect=\"quiet: 3\" -q -q -q\n> '\n>\n> Teach the new --expect= option to test-parse-options helper.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> t/t0040-parse-options.sh | 1 +\n> test-parse-options.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---\n> 2 files changed, 66 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index dbaee55..d678fbf 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -45,6 +45,7 @@ Standard options\n> -v, --verbose be verbose\n> -n, --dry-run dry run\n> -q, --quiet be quiet\n> + --expect expected output in the variable dump\n>\n> EOF\n>\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index 3db4332..010f3b2 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -14,6 +14,7 @@ static char *string = NULL;\n> static char *file = NULL;\n> static int ambiguous;\n> static struct string_list list;\n> +static struct string_list expect;\n>\n> static struct {\n> int called;\n> @@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const char *arg, int unset)\n> return 0;\n> }\n>\n> +/*\n> + * See if expect->string (\"label: value\") has a line in output that\n> + * begins with \"label:\", and if the line in output matches it.\n> + */\n> +static int match_line(struct string_list_item *expect, struct strbuf *output)\n> +{\n> + const char *label = expect->string;\n> + const char *colon = strchr(label, ':');\n> + const char *scan = output->buf;\n> + size_t label_len, expect_len;\n> +\n> + if (!colon)\n> + die(\"Malformed --expect value: %s\", label);\n> + label_len = colon - label;\n> +\n> + while (scan < output->buf + output->len) {\n> + const char *next;\n> + scan = memmem(scan, output->buf + output->len - scan,\n> + label, label_len);\n> + if (!scan)\n> + return 0;\n> + if (scan == output->buf || scan[-1] == '\\n')\n\nDoes scan[-1] work for the first line?\n\n> + break;\n> + next = strchr(scan + label_len, '\\n');\n> + if (!next)\n> + return 0;\n> + scan = next + 1;\n> + }\n> +\n> + /*\n> + * scan points at a line that begins with the label we are\n> + * looking for. Does it match?\n> + */\n> + expect_len = strlen(expect->string);\n> +\n> + if (output->buf + output->len <= scan + expect_len)\n> + return 0; /* value not long enough */\n> + if (memcmp(scan, expect->string, expect_len))\n> + return 0; /* does not match */\n> +\n> + return (scan + expect_len < output->buf + output->len &&\n> + scan[expect_len] == '\\n');\n> +}\n> +\n> +static int show_expected(struct string_list *list, struct strbuf *output)\n> +{\n> + struct string_list_item *expect;\n> + int found_mismatch = 0;\n> +\n> + for_each_string_list_item(expect, list) {\n> + if (!match_line(expect, output))\n> + found_mismatch = 1;\n> + }\n> + return found_mismatch;\n> +}\n> +\n> int main(int argc, char **argv)\n> {\n> const char *prefix = \"prefix/\";\n> @@ -87,6 +144,8 @@ int main(int argc, char **argv)\n> OPT__VERBOSE(&verbose, \"be verbose\"),\n> OPT__DRY_RUN(&dry_run, \"dry run\"),\n> OPT__QUIET(&quiet, \"be quiet\"),\n> + OPT_STRING_LIST(0, \"expect\", &expect, \"string\",\n> + \"expected output in the variable dump\"),\n> OPT_END(),\n> };\n> int i;\n> @@ -117,7 +176,10 @@ int main(int argc, char **argv)\n> for (i = 0; i < argc; i++)\n> strbuf_addf(&output, \"arg %02d: %s\\n\", i, argv[i]);\n>\n> - printf(\"%s\", output.buf);\n> -\n> - return 0;\n> + if (expect.nr)\n> + return show_expected(&expect, &output);\n\nOn a philosophical level this patch series is adding a\ntrailing \"|grep $X\" for the test-parse-options.\nI think such a grep pattern is a good thing because it is\ncheap to implement in unix like environments.\n\nThis however is a lot of C code for finding specific subsets\nin the output, so it is not quite cheap. Then we could also go\nthe non-wasteful way and instead check what to add to the strbuf\ninstead of filtering afterwards, i.e. each strbuf_add is guarded by\nan\n\n if (is_interesting_output(...))\n strbuf_add(...)\n\n> + else {\n> + printf(\"%s\", output.buf);\n> + return 0;\n> + }\n> }\n> --\n> 2.8.2-505-gdbd0e1d\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"285656","messageId":"CAPig+cQO_N=AM+YniXMKHOzvGy4JU=Sqxn+dGWuuqmc62s-qyA@mail.gmail.com","threadId":"42187","inReplyTo":"CAGZ79kY+9BUjcbpSA8sAqd=qZ5niZ2CDsPeGuXhK+yqZY4hL9Q@mail.gmail.com","subject":"Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-06T01:27:47Z","receivedAt":"2016-05-06T01:27:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 5, 2016 at 8:41 PM, Stefan Beller wrote:\n> On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano wrote:\n>> [...]\n>> But the only thing this test cares about is if \"quiet: 3\" is in the\n>> output. We should be able to write the above 18 lines with just\n>> four lines, like this:\n>>\n>> test_expect_success 'multiple quiet levels' '\n>> test-parse-options --expect=\"quiet: 3\" -q -q -q\n>> '\n>>\n>> Teach the new --expect= option to test-parse-options helper.\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n>> +/*\n>> + * See if expect->string (\"label: value\") has a line in output that\n>> + * begins with \"label:\", and if the line in output matches it.\n>> + */\n>> +static int match_line(struct string_list_item *expect, struct strbuf *output)\n>> +{\n>> + [...]\n>> + const char *scan = output->buf;\n>> + [...]\n>> + while (scan < output->buf + output->len) {\n>> + const char *next;\n>> + scan = memmem(scan, output->buf + output->len - scan,\n>> + label, label_len);\n>> + if (!scan)\n>> + return 0;\n>> + if (scan == output->buf || scan[-1] == '\\n')\n>\n> Does scan[-1] work for the first line?\n\nTake note of the short-circuiting '||' operator.\n\n> On a philosophical level this patch series is adding a\n> trailing \"|grep $X\" for the test-parse-options.\n> I think such a grep pattern is a good thing because it is\n> cheap to implement in unix like environments.\n>\n> This however is a lot of C code for finding specific subsets\n> in the output, so it is not quite cheap. Then we could also go\n> the non-wasteful way and instead check what to add to the strbuf\n> instead of filtering afterwards, i.e. each strbuf_add is guarded by\n> an\n>\n> if (is_interesting_output(...))\n> strbuf_add(...)\n\nI agree that this is adds far more complexity than I had expected upon\nreading Junio's suggestion about simplifying the t0040 tests. Patch 1\naside (which seems a desirable change), rather than patches 2 and 3, I\nhad expected to see only introduction of a minor helper function in\nt0040; perhaps something like this:\n\n options_expect () {\n expect=\"$1\" &&\n shift &&\n test-parse-options \"$@\" >actual &&\n grep \"$expect\" actual\n }\n\nand tests updated like this:\n\n options_expect \"quiet: 3\" -q -q -q\n"},{"id":"285657","messageId":"xmqqeg9f7v1l.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"CAGZ79kY+9BUjcbpSA8sAqd=qZ5niZ2CDsPeGuXhK+yqZY4hL9Q@mail.gmail.com","subject":"Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T02:57:26Z","receivedAt":"2016-05-06T02:57:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> instead of filtering afterwards, i.e. each strbuf_add is guarded by\n> an\n>\n> if (is_interesting_output(...))\n> strbuf_add(...)\n\nThat's a good approach.\n\nThe implementation gets a bit trickier than the previous one, but it\nwould look like this. Discard 2/3 and 3/3 and replace them with\nthis one.\n\nThe external interface on the input side is no different, but on the\noutput side, this version has \"expected '%s', got '%s'\" error, in\nthe same spirit as the output from \"test_cmp\", added in.\n\nInstead of checking the entire output line-by-line for each expected\noutput (in case you did not notice, you can give --expect='quiet: 3'\n--expect='abbrev: 7' and both must match), this one will check each\noutput line against each expected pattern. We wouldn't have too\nmany entries in the variable dump and we wouldn't be taking too many\n--expect options, so the matching performance would not matter,\nthough.\n\n\n t/t0040-parse-options.sh | 1 +\n test-parse-options.c | 88 ++++++++++++++++++++++++++++++++++++++++--------\n 2 files changed, 75 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex dbaee55..d678fbf 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -45,6 +45,7 @@ Standard options\n -v, --verbose be verbose\n -n, --dry-run dry run\n -q, --quiet be quiet\n+ --expect expected output in the variable dump\n \n EOF\n \ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex b5f4e90..e3f25df 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int collect_expect(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct string_list *expect;\n+\tstruct string_list_item *item;\n+\tstruct strbuf label = STRBUF_INIT;\n+\tconst char *colon;\n+\n+\tif (!arg || unset)\n+\t\tdie(\"malformed --expect option\");\n+\n+\texpect = (struct string_list *)opt->value;\n+\tcolon = strchr(arg, ':');\n+\tif (!colon)\n+\t\tdie(\"malformed --expect option, lacking a colon\");\n+\tstrbuf_add(&label, arg, colon - arg);\n+\titem = string_list_insert(expect, strbuf_detach(&label, NULL));\n+\tif (item->util)\n+\t\tdie(\"malformed --expect option, duplicate %s\", label.buf);\n+\titem->util = (void *)arg;\n+\treturn 0;\n+}\n+\n+__attribute__((format (printf,3,4)))\n+static void show(struct string_list *expect, int *status, const char *fmt, ...)\n+{\n+\tstruct string_list_item *item;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tva_list args;\n+\n+\tva_start(args, fmt);\n+\tstrbuf_vaddf(&buf, fmt, args);\n+\tva_end(args);\n+\n+\tif (!expect->nr)\n+\t\tprintf(\"%s\\n\", buf.buf);\n+\telse {\n+\t\tchar *colon = strchr(buf.buf, ':');\n+\t\tif (!colon)\n+\t\t\tdie(\"malformed output format, output lacking colon: %s\", fmt);\n+\t\t*colon = '\\0';\n+\t\titem = string_list_lookup(expect, buf.buf);\n+\t\t*colon = ':';\n+\t\tif (!item)\n+\t\t\t; /* not among entries being checked */\n+\t\telse {\n+\t\t\tif (strcmp((const char *)item->util, buf.buf)) {\n+\t\t\t\tprintf(\"expected '%s', got '%s'\\n\",\n+\t\t\t\t (char *)item->util, buf.buf);\n+\t\t\t\t*status = 1;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tstrbuf_reset(&buf);\n+}\n+\n int main(int argc, char **argv)\n {\n \tconst char *prefix = \"prefix/\";\n@@ -46,6 +101,7 @@ int main(int argc, char **argv)\n \t\t\"test-parse-options \",\n \t\tNULL\n \t};\n+\tstruct string_list expect = STRING_LIST_INIT_NODUP;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"yes\", &boolean, \"get a boolean\"),\n \t\tOPT_BOOL('D', \"no-doubt\", &boolean, \"begins with 'no-'\"),\n@@ -86,34 +142,38 @@ int main(int argc, char **argv)\n \t\tOPT__VERBOSE(&verbose, \"be verbose\"),\n \t\tOPT__DRY_RUN(&dry_run, \"dry run\"),\n \t\tOPT__QUIET(&quiet, \"be quiet\"),\n+\t\tOPT_CALLBACK(0, \"expect\", &expect, \"string\",\n+\t\t\t \"expected output in the variable dump\",\n+\t\t\t collect_expect),\n \t\tOPT_END(),\n \t};\n \tint i;\n+\tint ret = 0;\n \n \targc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);\n \n \tif (length_cb.called) {\n \t\tconst char *arg = length_cb.arg;\n \t\tint unset = length_cb.unset;\n-\t\tprintf(\"Callback: \\\"%s\\\", %d\\n\",\n-\t\t (arg ? arg : \"not set\"), unset);\n+\t\tshow(&expect, &ret, \"Callback: \\\"%s\\\", %d\",\n+\t\t (arg ? arg : \"not set\"), unset);\n \t}\n-\tprintf(\"boolean: %d\\n\", boolean);\n-\tprintf(\"integer: %d\\n\", integer);\n-\tprintf(\"magnitude: %lu\\n\", magnitude);\n-\tprintf(\"timestamp: %lu\\n\", timestamp);\n-\tprintf(\"string: %s\\n\", string ? string : \"(not set)\");\n-\tprintf(\"abbrev: %d\\n\", abbrev);\n-\tprintf(\"verbose: %d\\n\", verbose);\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+\tshow(&expect, &ret, \"boolean: %d\", boolean);\n+\tshow(&expect, &ret, \"integer: %d\", integer);\n+\tshow(&expect, &ret, \"magnitude: %lu\", magnitude);\n+\tshow(&expect, &ret, \"timestamp: %lu\", timestamp);\n+\tshow(&expect, &ret, \"string: %s\", string ? string : \"(not set)\");\n+\tshow(&expect, &ret, \"abbrev: %d\", abbrev);\n+\tshow(&expect, &ret, \"verbose: %d\", verbose);\n+\tshow(&expect, &ret, \"quiet: %d\", quiet);\n+\tshow(&expect, &ret, \"dry run: %s\", dry_run ? \"yes\" : \"no\");\n+\tshow(&expect, &ret, \"file: %s\", file ? file : \"(not set)\");\n \n \tfor (i = 0; i < list.nr; i++)\n-\t\tprintf(\"list: %s\\n\", list.items[i].string);\n+\t\tshow(&expect, &ret, \"list: %s\", list.items[i].string);\n \n \tfor (i = 0; i < argc; i++)\n-\t\tprintf(\"arg %02d: %s\\n\", i, argv[i]);\n+\t\tshow(&expect, &ret, \"arg %02d: %s\", i, argv[i]);\n \n \treturn 0;\n }\n"},{"id":"285660","messageId":"CAFZEwPMoqK0jLgvb9rm5j_0PHFO6Qg9OU1V_jxkFvL+wGGaNng@mail.gmail.com","threadId":"42187","inReplyTo":"xmqqbn4kb9m1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v16 7/7] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-06T05:05:46Z","receivedAt":"2016-05-06T05:05:46Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, May 6, 2016 at 12:44 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 391126e..114ffc9 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>\n> The name does not make it clear that config_verbose is only for\n> \"commit\" and not relevant to \"status\".\n\nTrue.\n\n>> @@ -1364,6 +1366,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n>> builtin_status_usage, 0);\n>> finalize_colopts(&s.colopts, -1);\n>> finalize_deferred_config(&s);\n>> + if (verbose == -1)\n>> + verbose = 0;\n>\n> Mental note: cmd_status() does not use git_commit_config() but uses\n> git_status_config(), hence config_verbose is not affected. But\n> because verbose is initialised to -1, the code needs to turn it off\n> like this.\n\nYes\n\n>> @@ -1664,6 +1673,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>> +\n>\n> cmd_commit() does use git_commit_config(), and verbose is\n> initialised -1, so without command line option, we fall back to\n> config_verbose if it is set from the configuration.\n>\n> I wonder if the attached patch squashed into this commit makes\n> things easier to understand, though. The points are:\n>\n> - We rename the configuration to make it clear that it is about\n> \"commit\" and does not apply to \"status\".\n>\n> - We initialize verbose to 0 as before. The only thing \"git\n> status\" cares about is if \"--verbose\" was given. Giving it\n> \"--no-verbose\" or nothing should not make any difference.\n>\n> - But we do need to stuff -1 to verbose in \"git commit\" before\n> handling the command line options, because the distinction\n> between having \"--no-verbose\" and not having any matter there,\n> and we do so in cmd_commit(), i.e. only place where it matters.\n\nAwesome work by addressing these points. I hadn't thought of these earlier.\n\n> builtin/commit.c | 12 +++++-------\n> 1 file changed, 5 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 583d1e3..a486620 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -113,9 +113,8 @@ 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 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 quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n> +static int config_commit_verbose = -1; /* unspecified */\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> @@ -1366,8 +1365,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> builtin_status_usage, 0);\n> finalize_colopts(&s.colopts, -1);\n> finalize_deferred_config(&s);\n> - if (verbose == -1)\n> - verbose = 0;\n>\n> handle_untracked_files_arg(&s);\n> if (show_ignored_in_status)\n> @@ -1521,7 +1518,7 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n> }\n> if (!strcmp(k, \"commit.verbose\")) {\n> int is_bool;\n> - config_verbose = git_config_bool_or_int(k, v, &is_bool);\n> + config_commit_verbose = git_config_bool_or_int(k, v, &is_bool);\n> return 0;\n> }\n>\n> @@ -1670,11 +1667,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> if (parse_commit(current_head))\n> die(_(\"could not parse HEAD commit\"));\n> }\n> + verbose = -1; /* unspecified */\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> + verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;\n>\n> if (dry_run)\n> return dry_run_commit(argc, argv, prefix, current_head, &s);\n\nThis makes things quite easy to understand.\nVery simple speaking:\n * Rename config_verbose => config_commit_verbose\n * initialize verbose to -1 only in cmd_commit()\n\nI checked out your branch gitster/pb/commit-verbose-config and tests\nfrom t0040 seem to be failing. Don't worry I will handle those, I will\nsquash your patch in mine and re-roll it again. I am still unsure how\nthose tests broke. I will figure it out.\n\nThanks for your help! :)\n"},{"id":"285661","messageId":"CAPig+cR=W3z-7RgWJ5QF8gNMdR_XA8q++K9MDdfeL+BePoOwCw@mail.gmail.com","threadId":"42187","inReplyTo":"xmqqbn4kb9m1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v16 7/7] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-06T05:07:40Z","receivedAt":"2016-05-06T05:07:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 5, 2016 at 3:14 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>> +static int config_verbose = -1; /* unspecified */\n>\n> The name does not make it clear that config_verbose is only for\n> \"commit\" and not relevant to \"status\".\n>\n>> @@ -1364,6 +1366,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n>> builtin_status_usage, 0);\n>> finalize_colopts(&s.colopts, -1);\n>> finalize_deferred_config(&s);\n>> + if (verbose == -1)\n>> + verbose = 0;\n>\n> Mental note: cmd_status() does not use git_commit_config() but uses\n> git_status_config(), hence config_verbose is not affected. But\n> because verbose is initialised to -1, the code needs to turn it off\n> like this.\n>\n>> @@ -1664,6 +1673,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>> +\n>\n> cmd_commit() does use git_commit_config(), and verbose is\n> initialised -1, so without command line option, we fall back to\n> config_verbose if it is set from the configuration.\n>\n> I wonder if the attached patch squashed into this commit makes\n> things easier to understand, though. The points are:\n>\n> - We rename the configuration to make it clear that it is about\n> \"commit\" and does not apply to \"status\".\n>\n> - We initialize verbose to 0 as before. The only thing \"git\n> status\" cares about is if \"--verbose\" was given. Giving it\n> \"--no-verbose\" or nothing should not make any difference.\n>\n> - But we do need to stuff -1 to verbose in \"git commit\" before\n> handling the command line options, because the distinction\n> between having \"--no-verbose\" and not having any matter there,\n> and we do so in cmd_commit(), i.e. only place where it matters.\n\nHmm... if someday someone wants git-status to support a status.verbose\nconfig variable, with Pranit's current implementation, it's a pretty\nsimple change: Just add to git_status_config():\n\n if (!strcmp(k, \"status.verbose\")) {\n int is_bool;\n config_verbose = git_config_bool_or_int(k, v, &is_bool);\n return 0;\n }\n\nand in cmd_status() change:\n\n if (verbose == -1)\n verbose = 0;\n\nto:\n\n if (verbose == -1)\n verbose = (config_verbose < 0) ? 0 : config_verbose;\n\nIt wouldn't be too hard with your proposal either: Either add a\n'config_status_verbose' variable or rename 'config_commit_verbose'\nback to 'config_verbose', initialize the global 'verbose' to -1, drop\nthe explicit 'verbose = -1' from cmd_commit(), and make the same\nchanges shown for Pranit's version. The diff would be a bit noisier.\n\nI do like that your proposal makes it more difficult for\ncommit.verbose to break git-status, but otherwise don't feel that it\nis significantly better. So, I dunno...\n"},{"id":"285662","messageId":"CAPig+cQO3W4WthHstrVFWziU2RAuNyEzeQwBEyDXG8dghRjECQ@mail.gmail.com","threadId":"42187","inReplyTo":"xmqq7ff8b99q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-06T05:30:45Z","receivedAt":"2016-05-06T05:30:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 5, 2016 at 3:21 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>> This series of patches add a configuration variable for verbose in\n>> git-commit.\n>>\n>> Changes wrt v15:\n>> * Remove the previous patch 7/7 and split the tests. Include one in\n>> initial patch 6/7. The other one is introduced in a separate commit\n>> after 4/7.\n>> * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,\n>> --no-verbose with -v as suggested by Eric Sunshine\n>\n> Thanks for a pleasant read. Modulo minor readability nits I sent\n> separately on 7/7, this looked good.\n\nAgreed, this version was a more pleasant and coherent read than previous ones.\n\nConsidering that this series is already at v16 and the 7/7 review\ncomments were very minor, I'd be fine seeing this series land as-is,\nrather than expecting v17.\n"},{"id":"285663","messageId":"CAGZ79kZ59K5BoSVsbt4YM-Try9Q1CVdFeBW8GE5E1dJpSBWzVA@mail.gmail.com","threadId":"42187","inReplyTo":"xmqqeg9f7v1l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-05-06T05:51:50Z","receivedAt":"2016-05-06T05:51:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, May 5, 2016 at 7:57 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> instead of filtering afterwards, i.e. each strbuf_add is guarded by\n>> an\n>>\n>> if (is_interesting_output(...))\n>> strbuf_add(...)\n>\n> That's a good approach.\n>\n> The implementation gets a bit trickier than the previous one, but it\n> would look like this. Discard 2/3 and 3/3 and replace them with\n> this one.\n>\n> The external interface on the input side is no different, but on the\n> output side, this version has \"expected '%s', got '%s'\" error, in\n> the same spirit as the output from \"test_cmp\", added in.\n>\n> Instead of checking the entire output line-by-line for each expected\n> output (in case you did not notice, you can give --expect='quiet: 3'\n> --expect='abbrev: 7' and both must match), this one will check each\n> output line against each expected pattern. We wouldn't have too\n> many entries in the variable dump and we wouldn't be taking too many\n> --expect options, so the matching performance would not matter,\n> though.\n>\n>\n> t/t0040-parse-options.sh | 1 +\n> test-parse-options.c | 88 ++++++++++++++++++++++++++++++++++++++++--------\n> 2 files changed, 75 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index dbaee55..d678fbf 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -45,6 +45,7 @@ Standard options\n> -v, --verbose be verbose\n> -n, --dry-run dry run\n> -q, --quiet be quiet\n> + --expect expected output in the variable dump\n>\n> EOF\n>\n> diff --git a/test-parse-options.c b/test-parse-options.c\n> index b5f4e90..e3f25df 100644\n> --- a/test-parse-options.c\n> +++ b/test-parse-options.c\n> @@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const char *arg, int unset)\n> return 0;\n> }\n>\n> +static int collect_expect(const struct option *opt, const char *arg, int unset)\n> +{\n> + struct string_list *expect;\n> + struct string_list_item *item;\n> + struct strbuf label = STRBUF_INIT;\n> + const char *colon;\n> +\n> + if (!arg || unset)\n> + die(\"malformed --expect option\");\n> +\n> + expect = (struct string_list *)opt->value;\n> + colon = strchr(arg, ':');\n> + if (!colon)\n> + die(\"malformed --expect option, lacking a colon\");\n> + strbuf_add(&label, arg, colon - arg);\n> + item = string_list_insert(expect, strbuf_detach(&label, NULL));\n> + if (item->util)\n> + die(\"malformed --expect option, duplicate %s\", label.buf);\n> + item->util = (void *)arg;\n> + return 0;\n> +}\n> +\n> +__attribute__((format (printf,3,4)))\n> +static void show(struct string_list *expect, int *status, const char *fmt, ...)\n> +{\n> + struct string_list_item *item;\n> + struct strbuf buf = STRBUF_INIT;\n> + va_list args;\n> +\n> + va_start(args, fmt);\n> + strbuf_vaddf(&buf, fmt, args);\n> + va_end(args);\n> +\n> + if (!expect->nr)\n> + printf(\"%s\\n\", buf.buf);\n> + else {\n> + char *colon = strchr(buf.buf, ':');\n> + if (!colon)\n> + die(\"malformed output format, output lacking colon: %s\", fmt);\n> + *colon = '\\0';\n> + item = string_list_lookup(expect, buf.buf);\n> + *colon = ':';\n\nI have been staring at this for a good couple of minutes and wondered if this\nlow level string manipulation is really the best way to do it.\n\n(It feels very C idiomatic, not using a lot of Gits own data\nstructures. I would have\nexpected some sort of skip_prefix just with partial regular expression or a\nstring_list_split_in_place for the splitting. But this \"set and reset *colon\"\nseems to be optimal here)\n\n> + if (!item)\n> + ; /* not among entries being checked */\n> + else {\n> + if (strcmp((const char *)item->util, buf.buf)) {\n> + printf(\"expected '%s', got '%s'\\n\",\n> + (char *)item->util, buf.buf);\n> + *status = 1;\n> + }\n> + }\n> + }\n> + strbuf_reset(&buf);\n\nstrbuf_release ?\n\n> +}\n> +\n> int main(int argc, char **argv)\n> {\n> const char *prefix = \"prefix/\";\n> @@ -46,6 +101,7 @@ int main(int argc, char **argv)\n> \"test-parse-options \",\n> NULL\n> };\n> + struct string_list expect = STRING_LIST_INIT_NODUP;\n> struct option options[] = {\n> OPT_BOOL(0, \"yes\", &boolean, \"get a boolean\"),\n> OPT_BOOL('D', \"no-doubt\", &boolean, \"begins with 'no-'\"),\n> @@ -86,34 +142,38 @@ int main(int argc, char **argv)\n> OPT__VERBOSE(&verbose, \"be verbose\"),\n> OPT__DRY_RUN(&dry_run, \"dry run\"),\n> OPT__QUIET(&quiet, \"be quiet\"),\n> + OPT_CALLBACK(0, \"expect\", &expect, \"string\",\n> + \"expected output in the variable dump\",\n> + collect_expect),\n> OPT_END(),\n> };\n> int i;\n> + int ret = 0;\n>\n> argc = parse_options(argc, (const char **)argv, prefix, options, usage, 0);\n>\n> if (length_cb.called) {\n> const char *arg = length_cb.arg;\n> int unset = length_cb.unset;\n> - printf(\"Callback: \\\"%s\\\", %d\\n\",\n> - (arg ? arg : \"not set\"), unset);\n> + show(&expect, &ret, \"Callback: \\\"%s\\\", %d\",\n> + (arg ? arg : \"not set\"), unset);\n> }\n> - printf(\"boolean: %d\\n\", boolean);\n> - printf(\"integer: %d\\n\", integer);\n> - printf(\"magnitude: %lu\\n\", magnitude);\n> - printf(\"timestamp: %lu\\n\", timestamp);\n> - printf(\"string: %s\\n\", string ? string : \"(not set)\");\n> - printf(\"abbrev: %d\\n\", abbrev);\n> - printf(\"verbose: %d\\n\", verbose);\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> + show(&expect, &ret, \"boolean: %d\", boolean);\n> + show(&expect, &ret, \"integer: %d\", integer);\n> + show(&expect, &ret, \"magnitude: %lu\", magnitude);\n> + show(&expect, &ret, \"timestamp: %lu\", timestamp);\n> + show(&expect, &ret, \"string: %s\", string ? string : \"(not set)\");\n> + show(&expect, &ret, \"abbrev: %d\", abbrev);\n> + show(&expect, &ret, \"verbose: %d\", verbose);\n> + show(&expect, &ret, \"quiet: %d\", quiet);\n> + show(&expect, &ret, \"dry run: %s\", dry_run ? \"yes\" : \"no\");\n> + show(&expect, &ret, \"file: %s\", file ? file : \"(not set)\");\n>\n> for (i = 0; i < list.nr; i++)\n> - printf(\"list: %s\\n\", list.items[i].string);\n> + show(&expect, &ret, \"list: %s\", list.items[i].string);\n>\n> for (i = 0; i < argc; i++)\n> - printf(\"arg %02d: %s\\n\", i, argv[i]);\n> + show(&expect, &ret, \"arg %02d: %s\", i, argv[i]);\n>\n> return 0;\n\n return ret; ? Otherwise `ret` is unused.\n\n> }\n"},{"id":"285668","messageId":"CAFZEwPOeZBRVqctNYuiBYC=SaDtU7jHqmgMNWLUayfTUqC1xnw@mail.gmail.com","threadId":"42187","inReplyTo":"CAFZEwPMoqK0jLgvb9rm5j_0PHFO6Qg9OU1V_jxkFvL+wGGaNng@mail.gmail.com","subject":"Re: [PATCH v16 7/7] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-06T06:40:55Z","receivedAt":"2016-05-06T06:40:55Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, May 6, 2016 at 10:35 AM, Pranit Bauva wrote:\n\n> I checked out your branch gitster/pb/commit-verbose-config and tests\n> from t0040 seem to be failing. Don't worry I will handle those, I will\n> squash your patch in mine and re-roll it again. I am still unsure how\n> those tests broke. I will figure it out.\n>\n> Thanks for your help! :)\n\nFalse alarm. I had a dirty build. The test suite passes perfectly.\nFeel free to squash your patch in locally.\n"},{"id":"285671","messageId":"xmqqwpn764ej.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"CAGZ79kZ59K5BoSVsbt4YM-Try9Q1CVdFeBW8GE5E1dJpSBWzVA@mail.gmail.com","subject":"Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T07:18:12Z","receivedAt":"2016-05-06T07:18:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> + *colon = '\\0';\n>> + item = string_list_lookup(expect, buf.buf);\n>> + *colon = ':';\n>\n> I have been staring at this for a good couple of minutes and wondered if this\n> low level string manipulation is really the best way to do it.\n\nIt just shows that string_list API was not designed as richly as\nothers, compared to say the more complete API like strbuf. If it\nhad a variant, I wouldn't have needed the \"temporary\ntermination to get a string\" hack.\n"},{"id":"285706","messageId":"20160506162058.Horde.toAFyoD2uVNcv2x2Ssx_9zt@webmail.informatik.kit.edu","threadId":"42187","inReplyTo":"CAPig+cQO3W4WthHstrVFWziU2RAuNyEzeQwBEyDXG8dghRjECQ@mail.gmail.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-06T14:20:58Z","receivedAt":"2016-05-06T14:20:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Eric Sunshine :\n\n> On Thu, May 5, 2016 at 3:21 PM, Junio C Hamano wrote:\n>> Pranit Bauva writes:\n>>> This series of patches add a configuration variable for verbose in\n>>> git-commit.\n>>>\n>>> Changes wrt v15:\n>>> * Remove the previous patch 7/7 and split the tests. Include one in\n>>> initial patch 6/7. The other one is introduced in a separate commit\n>>> after 4/7.\n>>> * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,\n>>> --no-verbose with -v as suggested by Eric Sunshine\n>>\n>> Thanks for a pleasant read. Modulo minor readability nits I sent\n>> separately on 7/7, this looked good.\n>\n> Agreed, this version was a more pleasant and coherent read than \n> previous ones.\n>\n> Considering that this series is already at v16 and the 7/7 review\n> comments were very minor, I'd be fine seeing this series land as-is,\n> rather than expecting v17.\n\nv16, wow, I totally lost track of this series, sorry.\n\nAnd I hate to bring this up this late again... at v16 of a now 7 patch\nseries, when all this started out like two months ago as a GSoC mini\nproject...\nBut I do it anyway. Oh well.\n\nA while ago in a related thread Peff remarked about 'git commit's\n'--quiet' and '--verbose' options:\n\n I think that is a UX mistake, and we would not do\n it that way if designing from scratch. But we're stuck with it for\n historical reasons (I'd probably name \"--verbose\" as \"--show-diff\" or\n something if writing it today).\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289069\n\nThen I replied:\n\n However, that doesn't mean that we have to spread this badly chosen\n name from options to config variables, does it? I think that if we\n are going to define a new config variable today, then it should be\n named properly, and it's better not to call it 'commit.verbose', but\n 'commit.showDiff' or something.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303\n\nAny thoughts on this? Before a poorly named config variable enters to\nthe codebase and we'll have to maintain it \"forever\"...\n"},{"id":"285712","messageId":"xmqqshxv5hhg.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"20160506162058.Horde.toAFyoD2uVNcv2x2Ssx_9zt@webmail.informatik.kit.edu","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T15:33:15Z","receivedAt":"2016-05-06T15:33:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> A while ago in a related thread Peff remarked about 'git commit's\n> '--quiet' and '--verbose' options:\n>\n> I think that is a UX mistake, and we would not do\n> it that way if designing from scratch. But we're stuck with it for\n> historical reasons (I'd probably name \"--verbose\" as \"--show-diff\" or\n> something if writing it today).\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289069\n>\n> Then I replied:\n>\n> However, that doesn't mean that we have to spread this badly chosen\n> name from options to config variables, does it? I think that if we\n> are going to define a new config variable today, then it should be\n> named properly, and it's better not to call it 'commit.verbose', but\n> 'commit.showDiff' or something.\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303\n>\n> Any thoughts on this? Before a poorly named config variable enters to\n> the codebase and we'll have to maintain it \"forever\"...\n\nMy thoughts are --show-diff would probably be a UI mistake of a\ndifferent sort, if you are anticipating that the different kinds of\ninformation to be shown in verbose modes would proliferate and that\nyou would want to give the user flexibility to pick and choose to\nuse some while not using some other among them. You would end up\nhaving --show-xyzzy --show-frotz --show-nitfol ... options.\n\nI am not convinced that we would want such a degree of flexibility\nin the first place, but even if we did, we'd be better off giving\nthat as \"--verbose=diff,xyzzy,frotz...\", I would think.\n\nAnd commit.verbose that begins its life as a simple boolean, which\ncan be extended to become bool-or-string if needed, is better than\nhaving commit.showDiff, commit.showXyzzy, commit.showFrotz, etc.\n"},{"id":"285748","messageId":"CAFZEwPPkBcdupLktJ=ystnx_1y7Mv+U436Jn9JBUCrvkt+t8tQ@mail.gmail.com","threadId":"42187","inReplyTo":"CACBZZX5ssO2EiuxR7wotGowMaPhtioaJVSDpQDUwUkv1rLJJWw@mail.gmail.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-05-06T16:16:18Z","receivedAt":"2016-05-06T16:16:18Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"[+cc:git@vger.kernel.org] Because its an interesting fact to be shared\nwhich isn't covered elsewhere.\n\nOn Fri, May 6, 2016 at 2:53 AM, Ævar Arnfjörð Bjarmason\n wrote:\n> Sending this privately since it's probably covered elsewhere. With\n> this, if I set the option will \"reword\" in git rebase -i show me the\n> patch?\n>\n> If so: awesome.\n\nYes, git rebase -i will show the diff in 'reword' if commit.verbose is\nset to true or a value greater than 0.\n\nI dug further in git-rebase--interactive.sh\nI could find appearances of \"git commit --amend\" but I was unable to\nfind appearances of \"COMMIT_EDITMSG\". If COMMIT_EDITMSG was coming\ninto picture, the commit.verbose could not affect it. And that is not\nthe case.\n\nI guess this would be a desirable trait for most of the consumers of\ncommit.verbose (like Ævar) so there would not be a need to suppress.\n\nRegards,\nPranit Bauva\n"},{"id":"285761","messageId":"xmqqinyr3xbj.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"CAGZ79kZ59K5BoSVsbt4YM-Try9Q1CVdFeBW8GE5E1dJpSBWzVA@mail.gmail.com","subject":"Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T17:34:08Z","receivedAt":"2016-05-06T17:34:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> + if (!item)\n>> + ; /* not among entries being checked */\n>> + else {\n>> + if (strcmp((const char *)item->util, buf.buf)) {\n>> + printf(\"expected '%s', got '%s'\\n\",\n>> + (char *)item->util, buf.buf);\n>> + *status = 1;\n>> + }\n>> + }\n>> + }\n>> + strbuf_reset(&buf);\n>\n> strbuf_release ?\n\nThanks for spotting a leak.\n\nI originally had the buf as static, as all generated strings are\nshort and of similar length, in an attempt to reuse the already\nallocated storage instead of allocating it from scratch every call.\n\n>>\n>> return 0;\n>\n> return ret; ? Otherwise `ret` is unused.\n\nThis, too. Thanks.\n"},{"id":"285763","messageId":"xmqqeg9f3w39.fsf_-_@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"20160505215056.28224-1-gitster@pobox.com","subject":"[PATCH] t0040: remove unused test helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T18:00:42Z","receivedAt":"2016-05-06T18:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"9a001381 (Fix tests under GETTEXT_POISON on parseopt, 2012-08-27)\nintroduced check_i18n, but the helper was never used from the\nbeginning.\n\nThe same commit also introduced check_unknown_i18n to replace the\nhelper check_unknown and changed all users of the latter to use the\nformer, but failed to remove check_unknown itself.\n\nSigned-off-by: Junio C Hamano \n---\n t/t0040-parse-options.sh | 24 ------------------------\n 1 file changed, 24 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex d678fbf..5c8c72a 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -81,30 +81,6 @@ check() {\n \ttest_cmp expect output\n }\n \n-check_i18n() {\n-\twhat=\"$1\" &&\n-\tshift &&\n-\texpect=\"$1\" &&\n-\tshift &&\n-\tsed \"s/^$what .*/$what $expect/\" expect &&\n-\ttest-parse-options $* >output 2>output.err &&\n-\ttest_must_be_empty output.err &&\n-\ttest_i18ncmp expect output\n-}\n-\n-check_unknown() {\n-\tcase \"$1\" in\n-\t--*)\n-\t\techo error: unknown option \\`${1#--}\\' >expect ;;\n-\t-*)\n-\t\techo error: unknown switch \\`${1#-}\\' >expect ;;\n-\tesac &&\n-\tcat expect.err >>expect &&\n-\ttest_must_fail test-parse-options $* >output 2>output.err &&\n-\ttest_must_be_empty output &&\n-\ttest_cmp expect output.err\n-}\n-\n check_unknown_i18n() {\n \tcase \"$1\" in\n \t--*)\n-- \n2.8.2-507-g43e827d\n"},{"id":"285774","messageId":"CACBZZX6XThQ9Ns4YMd_jC2jmNHhWg7QgXsn9_Ejy_8itToJQug@mail.gmail.com","threadId":"42187","inReplyTo":"CAFZEwPPkBcdupLktJ=ystnx_1y7Mv+U436Jn9JBUCrvkt+t8tQ@mail.gmail.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-05-06T19:47:42Z","receivedAt":"2016-05-06T19:47:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, May 6, 2016 at 6:16 PM, Pranit Bauva wrote:\n> [+cc:git@vger.kernel.org] Because its an interesting fact to be shared\n> which isn't covered elsewhere.\n>\n> On Fri, May 6, 2016 at 2:53 AM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> Sending this privately since it's probably covered elsewhere. With\n>> this, if I set the option will \"reword\" in git rebase -i show me the\n>> patch?\n>>\n>> If so: awesome.\n>\n> Yes, git rebase -i will show the diff in 'reword' if commit.verbose is\n> set to true or a value greater than 0.\n>\n> I dug further in git-rebase--interactive.sh\n> I could find appearances of \"git commit --amend\" but I was unable to\n> find appearances of \"COMMIT_EDITMSG\". If COMMIT_EDITMSG was coming\n> into picture, the commit.verbose could not affect it. And that is not\n> the case.\n>\n> I guess this would be a desirable trait for most of the consumers of\n> commit.verbose (like Ævar) so there would not be a need to suppress.\n\nYeah it's great, it's something I've wanted from interactive rebase\nfor a while now.\n"},{"id":"285782","messageId":"xmqq8tzm3o76.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"CACBZZX6XThQ9Ns4YMd_jC2jmNHhWg7QgXsn9_Ejy_8itToJQug@mail.gmail.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T20:51:09Z","receivedAt":"2016-05-06T20:51:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Fri, May 6, 2016 at 6:16 PM, Pranit Bauva wrote:\n>> [+cc:git@vger.kernel.org] Because its an interesting fact to be shared\n>> which isn't covered elsewhere.\n>>\n>> On Fri, May 6, 2016 at 2:53 AM, Ævar Arnfjörð Bjarmason\n>> wrote:\n>>> Sending this privately since it's probably covered elsewhere. With\n>>> this, if I set the option will \"reword\" in git rebase -i show me the\n>>> patch?\n>>>\n>>> If so: awesome.\n>>\n>> Yes, git rebase -i will show the diff in 'reword' if commit.verbose is\n>> set to true or a value greater than 0.\n>>\n>> I dug further in git-rebase--interactive.sh\n>> I could find appearances of \"git commit --amend\" but I was unable to\n>> find appearances of \"COMMIT_EDITMSG\". If COMMIT_EDITMSG was coming\n>> into picture, the commit.verbose could not affect it. And that is not\n>> the case.\n>>\n>> I guess this would be a desirable trait for most of the consumers of\n>> commit.verbose (like Ævar) so there would not be a need to suppress.\n>\n> Yeah it's great, it's something I've wanted from interactive rebase\n> for a while now.\n\nI can see why \"commit -v\" may be useful during \"rebase -i\", but we\nshould also have rebase.verbose and \"rebase -v\". I do not want to\nmake all my commits with -v, and I suspect I want to do \"commit -v\"\nmore often during \"rebase -i\" than regular commit, for example.\n"},{"id":"285795","messageId":"20160507053209.GA1704@sigill.intra.peff.net","threadId":"42187","inReplyTo":"xmqqshxv5hhg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-07T05:32:09Z","receivedAt":"2016-05-07T05:32:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2016 at 08:33:15AM -0700, Junio C Hamano wrote:\n\n> > Then I replied:\n> >\n> > However, that doesn't mean that we have to spread this badly chosen\n> > name from options to config variables, does it? I think that if we\n> > are going to define a new config variable today, then it should be\n> > named properly, and it's better not to call it 'commit.verbose', but\n> > 'commit.showDiff' or something.\n> >\n> > http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303\n> >\n> > Any thoughts on this? Before a poorly named config variable enters to\n> > the codebase and we'll have to maintain it \"forever\"...\n> \n> My thoughts are --show-diff would probably be a UI mistake of a\n> different sort, if you are anticipating that the different kinds of\n> information to be shown in verbose modes would proliferate and that\n> you would want to give the user flexibility to pick and choose to\n> use some while not using some other among them. You would end up\n> having --show-xyzzy --show-frotz --show-nitfol ... options.\n> \n> I am not convinced that we would want such a degree of flexibility\n> in the first place, but even if we did, we'd be better off giving\n> that as \"--verbose=diff,xyzzy,frotz...\", I would think.\n> \n> And commit.verbose that begins its life as a simple boolean, which\n> can be extended to become bool-or-string if needed, is better than\n> having commit.showDiff, commit.showXyzzy, commit.showFrotz, etc.\n\nI don't think anyone is anticipating more \"--show-\" options. It is just\nthat \"--verbose\" is the opposite of \"--quiet\" in most other commands,\nand pertains to chattiness on the terminal about what is going on.\n\nWhereas in git-commit, is about sticking some data in the commit message\ntemplate. Naively I'd expect it to cause commit to spew more data to\nstderr about what's being committed, ident info, etc.\n\nIf you are thinking that there could be something like \"--show-ident\" to\nreplace that, I do not mind that too much. But IMHO that does not\naddress the root problem that commit's \"--verbose\" is not very much like\nthe same option in other commands. And something like\n\"--verbose=diff,ident\" just seems to make that worse by coupling options\nthat otherwise don't have anything to do with each other.\n\n-Peff\n"},{"id":"285827","messageId":"CACBZZX5FHBG8xXc4wKUyW90FianJB1PT2FyByqYVqccb2ef2eg@mail.gmail.com","threadId":"42187","inReplyTo":"20160507053209.GA1704@sigill.intra.peff.net","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-05-07T19:28:53Z","receivedAt":"2016-05-07T19:28:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, May 7, 2016 at 7:32 AM, Jeff King wrote:\n> On Fri, May 06, 2016 at 08:33:15AM -0700, Junio C Hamano wrote:\n>\n>> > Then I replied:\n>> >\n>> > However, that doesn't mean that we have to spread this badly chosen\n>> > name from options to config variables, does it? I think that if we\n>> > are going to define a new config variable today, then it should be\n>> > named properly, and it's better not to call it 'commit.verbose', but\n>> > 'commit.showDiff' or something.\n>> >\n>> > http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289303\n>> >\n>> > Any thoughts on this? Before a poorly named config variable enters to\n>> > the codebase and we'll have to maintain it \"forever\"...\n>>\n>> My thoughts are --show-diff would probably be a UI mistake of a\n>> different sort, if you are anticipating that the different kinds of\n>> information to be shown in verbose modes would proliferate and that\n>> you would want to give the user flexibility to pick and choose to\n>> use some while not using some other among them. You would end up\n>> having --show-xyzzy --show-frotz --show-nitfol ... options.\n>>\n>> I am not convinced that we would want such a degree of flexibility\n>> in the first place, but even if we did, we'd be better off giving\n>> that as \"--verbose=diff,xyzzy,frotz...\", I would think.\n>>\n>> And commit.verbose that begins its life as a simple boolean, which\n>> can be extended to become bool-or-string if needed, is better than\n>> having commit.showDiff, commit.showXyzzy, commit.showFrotz, etc.\n>\n> I don't think anyone is anticipating more \"--show-\" options. It is just\n> that \"--verbose\" is the opposite of \"--quiet\" in most other commands,\n> and pertains to chattiness on the terminal about what is going on.\n>\n> Whereas in git-commit, is about sticking some data in the commit message\n> template. Naively I'd expect it to cause commit to spew more data to\n> stderr about what's being committed, ident info, etc.\n>\n> If you are thinking that there could be something like \"--show-ident\" to\n> replace that, I do not mind that too much. But IMHO that does not\n> address the root problem that commit's \"--verbose\" is not very much like\n> the same option in other commands. And something like\n> \"--verbose=diff,ident\" just seems to make that worse by coupling options\n> that otherwise don't have anything to do with each other.\n\nI can see how it looks out of place looked at like that, but for me as\na long-time user (aren't we all?) it never felt out of place because\nit's a more verbose version of the output that's brought up when I'm\nmodifying it.\n\nI.e. I'm modifying the commit message, so the message is brought up,\noptionally and more verbosely I can ask for the whole commit\n(including diff) to amend the commit message.\n\nI.e. I really expect --verbose to be a more verbose version of the\nprimary thing a command is doing, which in the case of \"commit\n--amend\" is giving me info I need to modify the commit.\n\nIt also fits nicely with \"status --verbose\" showing a diff of staged\nchanges, similar to how --verbose for commit shows the diff for a\ncommit being amended.\n"},{"id":"285898","messageId":"xmqqy47kxu68.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"CACBZZX5FHBG8xXc4wKUyW90FianJB1PT2FyByqYVqccb2ef2eg@mail.gmail.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-08T18:48:31Z","receivedAt":"2016-05-08T18:48:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I.e. I really expect --verbose to be a more verbose version of the\n> primary thing a command is doing, which in the case of \"commit\n> --amend\" is giving me info I need to modify the commit.\n\nThat summarises what I wanted to say very well. Thanks.\n"},{"id":"285918","messageId":"20160509142825.GB9552@sigill.intra.peff.net","threadId":"42187","inReplyTo":"xmqqy47kxu68.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-09T14:28:25Z","receivedAt":"2016-05-09T14:28:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 08, 2016 at 11:48:31AM -0700, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > I.e. I really expect --verbose to be a more verbose version of the\n> > primary thing a command is doing, which in the case of \"commit\n> > --amend\" is giving me info I need to modify the commit.\n> \n> That summarises what I wanted to say very well. Thanks.\n\nI guess I do not really consider the template content to be the primary\nthing the command is doing. It is subjective, though. I don't feel\nstrongly enough to keep discussing it if other people don't agree.\n\n-Peff\n"},{"id":"285929","messageId":"xmqqtwi7xlsc.fsf@gitster.mtv.corp.google.com","threadId":"42187","inReplyTo":"20160509142825.GB9552@sigill.intra.peff.net","subject":"Re: [PATCH v16 0/7] config commit verbose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-09T16:01:55Z","receivedAt":"2016-05-09T16:01:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I guess I do not really consider the template content to be the primary\n> thing the command is doing. It is subjective, though. I don't feel\n> strongly enough to keep discussing it if other people don't agree.\n\nI just see the primary thing of what \"commit -e\" does is to help\nusers edit their log message (and view \"-v\" as giving more helping),\nbut I do agree with you that this is very subjective.\n\nIf we had these as either in broken-down form (\"--show-diff\",\n\"--show-diffstat\", and \"--show-untracked\") or just a single\n\"--show-extra-info\" option when we did the feature in the very\nbeginning, I do not think I'd feel that \"--show-*\" option(s) should\nbe renamed/redone to \"--verbose\". So personally, my subjective\njudgment is \"'--verbose' and '--show-diff' would have been equally\nvalid, and it is OK to let whichever came first squat on the\nfeature.\"\n"}]}