{"thread":{"id":"41807","subject":"[PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","startedAt":"2016-03-24T08:25:18Z","lastAt":"2016-03-28T19:03:07Z","messageCount":23,"participants":["Pranit Bauva","SZEDER Gábor","Eric Sunshine"],"isPatch":true,"patchVersion":9,"patchTotal":3},"messages":[{"id":"281647","messageId":"01020153a7ba4eae-9b88e119-0505-418f-a137-595250edaa9d-000000@eu-west-1.amazonses.com","threadId":"41807","inReplyTo":"01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com","subject":"[PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-24T08:25:18Z","receivedAt":"2016-03-24T08:25:18Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Also remove test_set_editor from global scope and use it in whichever\ntest it is required.\n---\n t/t7507-commit-verbose.sh | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..cf95efb 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -3,12 +3,11 @@\n test_description='verbose commit template'\n . ./test-lib.sh\n \n-cat >check-for-diff <<EOF\n-#!$SHELL_PATH\n-exec grep '^diff --git' \"\\$1\"\n+write_script \"check-for-diff\" <<-\\EOF &&\n+grep '^diff --git' \"$1\" >out &&\n+test $(wc -l <out) = 1\n EOF\n chmod +x check-for-diff\n-test_set_editor \"$PWD/check-for-diff\"\n \n cat >message <<'EOF'\n subject\n@@ -23,6 +22,7 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'initial commit shows verbose diff' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \tgit commit --amend -v\n '\n \n@@ -38,11 +38,13 @@ check_message() {\n }\n \n test_expect_success 'verbose diff is stripped out' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \tgit commit --amend -v &&\n \tcheck_message message\n '\n \n test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \tgit config diff.mnemonicprefix true &&\n \tgit commit --amend -v &&\n \tcheck_message message\n@@ -66,11 +68,13 @@ test_expect_success 'diff in message is retained without -v' '\n '\n \n test_expect_success 'diff in message is retained with -v' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \tgit commit --amend -F diff -v &&\n \tcheck_message diff\n '\n \n test_expect_success 'submodule log is stripped out too with -v' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \tgit config diff.submodule log &&\n \tgit submodule add ./. sub &&\n \tgit commit -m \"sub added\" &&\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"281648","messageId":"01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com","threadId":"41807","inReplyTo":"010201538b98aaa4-ad08c159-4935-48f1-acdb-7c376da2c368-000000@eu-west-1.amazonses.com","subject":"[PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-24T08:25:18Z","receivedAt":"2016-03-24T08:25:18Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"The reason to make it consider negative values or more specifically\n\"unspecified\" values is to give the ability to differentiate between\nonce, multiple time or with --no-option.\n\nEg. :\ninitialize verbose = -1\n`git commit` => verbose = -1\n`git commit -v` => verbose = 1\n`git commit -v -v` => verbose = 2\n`git commit --no-verbose` => verbose = 0\n\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n\n---\nThe discussion about this patch:\n[1] : http://thread.gmane.org/gmane.comp.version-control.git/289027\n\nPrevious version of the patch:\n[v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061\n\nChanges introduced:\nUse a different language in commit message to make the change and its\nutility more clear.\n---\n Documentation/technical/api-parse-options.txt | 6 ++++--\n parse-options.c                               | 8 +++++++-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 5f0757d..a908d8a 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -144,8 +144,10 @@ 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+\tIf the `int_var` is negative and `--option` is absent,\n+\tthen it will retain its value. Otherwise it will first set\n+\t`int_var` to 0 and then increment it on each use of `--option`,\n+\tand reset to zero with `--no-option`.\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..86d30bd 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\treturn 0;\n \n \tcase OPTION_COUNTUP:\n-\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n+\t\tif (unset)\n+\t\t\t*(int *)opt->value = 0;\n+\t\telse {\n+\t\t\tif (*(int *)opt->value < 0)\n+\t\t\t\t*(int *)opt->value = 0;\n+\t\t\t*(int *)opt->value += 1;\n+\t\t}\n \t\treturn 0;\n \n \tcase OPTION_SET_INT:\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"281649","messageId":"01020153a7ba4ec2-1fb668c3-9dc6-4137-8d92-c6a5825a156c-000000@eu-west-1.amazonses.com","threadId":"41807","inReplyTo":"01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com","subject":"[PATCH v9 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-24T08:25:18Z","receivedAt":"2016-03-24T08:25:18Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Add commit.verbose configuration variable as a convenience for those\nwho always prefer --verbose.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n\n---\nThe previous version of the patch are:\n - [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\nChanges with respect to the previous patch:\n - Compare with -1 as only -1 value is used for unspecified behavior.\n - Write clean tests\n---\n Documentation/config.txt     |  4 ++++\n Documentation/git-commit.txt |  3 ++-\n builtin/commit.c             | 13 ++++++++++-\n t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 69 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2cd6bdd..1d0ec2e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1110,6 +1110,10 @@ commit.template::\n \t\"`~/`\" is expanded to the value of `$HOME` and \"`~user/`\" to the\n \tspecified user's home directory.\n \n+commit.verbose::\n+\tA boolean or int to specify the level of verbose with `git commit`.\n+\tSee linkgit:git-commit[1].\n+\n credential.helper::\n \tSpecify an external helper to be called when a username or\n \tpassword credential is needed; the helper may consult external\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 9ec6b3c..d474226 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].\n \twhat changes the commit has.\n \tNote that this diff output doesn't have its\n \tlines prefixed with '#'. This diff will not be a part\n-\tof the commit message.\n+\tof the commit message. See the `commit.verbose` configuration\n+\tvariable in linkgit:git-config[1].\n +\n If specified twice, show in addition the unified diff between\n what would be committed and the worktree files, i.e. the unstaged\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b3bd2d4..db65c03 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@@ -1505,6 +1507,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n \t\tsign_commit = git_config_bool(k, v) ? \"\" : NULL;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"commit.verbose\")) {\n+\t\tint is_bool;\n+\t\tconfig_verbose = git_config_bool_or_int(k, v, &is_bool);\n+\t\treturn 0;\n+\t}\n \n \tstatus = git_gpg_config(k, v, NULL);\n \tif (status)\n@@ -1654,6 +1661,10 @@ 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+\n+\tif (verbose == -1)\n+\t\tverbose = (config_verbose == -1) ? 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 cf95efb..66deac3 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -9,6 +9,12 @@ test $(wc -l <out) = 1\n EOF\n chmod +x check-for-diff\n \n+write_script \"check-for-double-diff\" <<-\\EOF &&\n+grep '# Changes not staged for commit' \"$1\" >out &&\n+test $(wc -l <out) = 2\n+EOF\n+chmod +x check-for-double-diff\n+\n cat >message <<'EOF'\n subject\n \n@@ -100,4 +106,49 @@ 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 'commit.verbose true and --verbose omitted' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\tgit -c commit.verbose=true commit --amend\n+'\n+\n+test_expect_success 'commit.verbose true and --verbose' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\tgit -c commit.verbose=true commit --amend --verbose\n+'\n+\n+test_expect_success 'commit.verbose true and -v -v' '\n+\ttest_set_editor \"$PWD/check-for-double-diff\" &&\n+\tgit -c commit.verbose=true commit --amend -v -v\n+'\n+\n+test_expect_success 'commit.verbose true and --no-verbose' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\ttest_must_fail git -c commit.verbose=true commit --amend --no-verbose\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\tgit -c commit.verbose=false commit --amend --verbose\n+'\n+\n+test_expect_success 'commit.verbose false and -v -v' '\n+\ttest_set_editor \"$PWD/check-for-double-diff\" &&\n+\tgit -c commit.verbose=false commit --amend -v -v\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose omitted' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\ttest_must_fail git -c commit.verbose=false commit --amend\n+'\n+\n+test_expect_success 'commit.verbose false and --no-verbose' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\ttest_must_fail git -c commit.verbose=false commit --amend --no-verbose\n+'\n+\n+test_expect_success 'status ignores commit.verbose=true' '\n+\tgit -c commit.verbose=true status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n test_done\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"281654","messageId":"1458813876-10591-1-git-send-email-szeder@ira.uka.de","threadId":"41807","inReplyTo":"01020153a7ba4ec2-1fb668c3-9dc6-4137-8d92-c6a5825a156c-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v9 3/3] commit: add a commit.verbose config variable","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-24T10:04:36Z","receivedAt":"2016-03-24T10:04:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> Add commit.verbose configuration variable as a convenience for those\n> who always prefer --verbose.\n> \n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> \n> ---\n> The previous version of the patch are:\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> Changes with respect to the previous patch:\n>  - Compare with -1 as only -1 value is used for unspecified behavior.\n>  - Write clean tests\n> ---\n>  Documentation/config.txt     |  4 ++++\n>  Documentation/git-commit.txt |  3 ++-\n>  builtin/commit.c             | 13 ++++++++++-\n>  t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++\n>  4 files changed, 69 insertions(+), 2 deletions(-)\n\nPlease always run the full test suite before submitting patches to\nmake sure that your changes do not inadvertently break something.\nThis patch breaks several tests in t7512-status-help.sh,\nt7508-status.sh and t7060-wtstatus.sh.\n"},{"id":"281655","messageId":"CAFZEwPN2+RwrE583XHOv9=05XQaWZnL=cK6wa0rMC-3AU275yg@mail.gmail.com","threadId":"41807","inReplyTo":"1458813876-10591-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v9 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-24T10:22:54Z","receivedAt":"2016-03-24T10:22:54Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Thu, Mar 24, 2016 at 3:34 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>> Add commit.verbose configuration variable as a convenience for those\n>> who always prefer --verbose.\n>>\n>> Helped-by: Junio C Hamano <gitster@pobox.com>\n>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>>\n>> ---\n>> The previous version of the patch are:\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>> Changes with respect to the previous patch:\n>>  - Compare with -1 as only -1 value is used for unspecified behavior.\n>>  - Write clean tests\n>> ---\n>>  Documentation/config.txt     |  4 ++++\n>>  Documentation/git-commit.txt |  3 ++-\n>>  builtin/commit.c             | 13 ++++++++++-\n>>  t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++\n>>  4 files changed, 69 insertions(+), 2 deletions(-)\n>\n> Please always run the full test suite before submitting patches to\n> make sure that your changes do not inadvertently break something.\n> This patch breaks several tests in t7512-status-help.sh,\n> t7508-status.sh and t7060-wtstatus.sh.\n\nSorry for that. I will make sure I do run the complete test suite. I\ncurrently ran only commit based tests. But now that I think about it\nthat since status and commit share a lot of things, it might be\npossible to break parts of status. I will investigate further as to\nwhat cased this problem though I kind of get a hint that it is because\nof verbose being the parent and others consuming it. There are a lot\nof tests failing.\n"},{"id":"281656","messageId":"CAFZEwPNyELB2BZ1q95HroV0a3vfNK0eTtzcy-2aEmUY5QVZe-g@mail.gmail.com","threadId":"41807","inReplyTo":"CAFZEwPN2+RwrE583XHOv9=05XQaWZnL=cK6wa0rMC-3AU275yg@mail.gmail.com","subject":"Re: [PATCH v9 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-24T10:26:56Z","receivedAt":"2016-03-24T10:26:56Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"I got it. The verbose is initialised to -1 before. When cmd_commit\nruns, it changes the value of verbose accordingly to 0 or positive.\nBut when cmd_status runs, it will retain the value -1 and the if\nclause which accepts all values except 0 will execute. I guess a if\nstatement inside cmd_status which reinitialises it to 0 or positive\ndepending on the situation will solve the problem.\n\nOn Thu, Mar 24, 2016 at 3:52 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Thu, Mar 24, 2016 at 3:34 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>>> Add commit.verbose configuration variable as a convenience for those\n>>> who always prefer --verbose.\n>>>\n>>> Helped-by: Junio C Hamano <gitster@pobox.com>\n>>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>>>\n>>> ---\n>>> The previous version of the patch are:\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>>> Changes with respect to the previous patch:\n>>>  - Compare with -1 as only -1 value is used for unspecified behavior.\n>>>  - Write clean tests\n>>> ---\n>>>  Documentation/config.txt     |  4 ++++\n>>>  Documentation/git-commit.txt |  3 ++-\n>>>  builtin/commit.c             | 13 ++++++++++-\n>>>  t/t7507-commit-verbose.sh    | 51 ++++++++++++++++++++++++++++++++++++++++++++\n>>>  4 files changed, 69 insertions(+), 2 deletions(-)\n>>\n>> Please always run the full test suite before submitting patches to\n>> make sure that your changes do not inadvertently break something.\n>> This patch breaks several tests in t7512-status-help.sh,\n>> t7508-status.sh and t7060-wtstatus.sh.\n>\n> Sorry for that. I will make sure I do run the complete test suite. I\n> currently ran only commit based tests. But now that I think about it\n> that since status and commit share a lot of things, it might be\n> possible to break parts of status. I will investigate further as to\n> what cased this problem though I kind of get a hint that it is because\n> of verbose being the parent and others consuming it. There are a lot\n> of tests failing.\n"},{"id":"281657","messageId":"1458815593-11060-1-git-send-email-szeder@ira.uka.de","threadId":"41807","inReplyTo":"01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com","subject":"[PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-24T10:33:13Z","receivedAt":"2016-03-24T10:33:13Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> The reason to make it consider negative values or more specifically\n> \"unspecified\" values is to give the ability to differentiate between\n> once, multiple time or with --no-option.\n> \n> Eg. :\n> initialize verbose = -1\n> `git commit` => verbose = -1\n> `git commit -v` => verbose = 1\n> `git commit -v -v` => verbose = 2\n> `git commit --no-verbose` => verbose = 0\n> \n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> \n> ---\n> The discussion about this patch:\n> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027\n> \n> Previous version of the patch:\n> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061\n> \n> Changes introduced:\n> Use a different language in commit message to make the change and its\n> utility more clear.\n\nI don't see the mentioned change in the commit message.  In\nparticular:\n\n  - As Eric pointed out in the previous round, the commit message\n    misses the single most important point of justification: to\n    determine whether '--option' or '--no-option' was specified at\n    all.  Explaining this would also make the examples unnecessary.\n\n  - OPT_COUNTUP is used in several places, mostly indirecty, but the\n    commit message doesn't explain that possible side-effects to these\n    callers were considered and that they are not harmed by this\n    change.\n\n> ---\n>  Documentation/technical/api-parse-options.txt | 6 ++++--\n>  parse-options.c                               | 8 +++++++-\n>  2 files changed, 11 insertions(+), 3 deletions(-)\n\nA couple of new tests to t0040-parse-options.sh would be great to\nensure that starting from a negative value works as advertised, i.e.\nat least that '--option' jumps to 1 and '--no-option' resets to 0.\n\n> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n> index 5f0757d..a908d8a 100644\n> --- a/Documentation/technical/api-parse-options.txt\n> +++ b/Documentation/technical/api-parse-options.txt\n> @@ -144,8 +144,10 @@ 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> +\tIf the `int_var` is negative and `--option` is absent,\n> +\tthen it will retain its value. Otherwise it will first set\n> +\t`int_var` to 0 and then increment it on each use of `--option`,\n> +\tand reset to zero with `--no-option`.\n>  \n>  `OPT_BIT(short, long, &int_var, description, mask)`::\n>  \tIntroduce a boolean option.\n> diff --git a/parse-options.c b/parse-options.c\n> index 47a9192..86d30bd 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,\n>  \t\treturn 0;\n>  \n>  \tcase OPTION_COUNTUP:\n> -\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n> +\t\tif (unset)\n> +\t\t\t*(int *)opt->value = 0;\n> +\t\telse {\n> +\t\t\tif (*(int *)opt->value < 0)\n> +\t\t\t\t*(int *)opt->value = 0;\n> +\t\t\t*(int *)opt->value += 1;\n> +\t\t}\n>  \t\treturn 0;\n>  \n>  \tcase OPTION_SET_INT:\n> \n> --\n> https://github.com/git/git/pull/218\n> \n> \n"},{"id":"281658","messageId":"CAFZEwPOwmJuLzUsAwqZttYcHaZsekpR_o9bXtXv73JBSFuxNYQ@mail.gmail.com","threadId":"41807","inReplyTo":"1458815593-11060-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-24T10:38:24Z","receivedAt":"2016-03-24T10:38:24Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Thu, Mar 24, 2016 at 4:03 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>> The reason to make it consider negative values or more specifically\n>> \"unspecified\" values is to give the ability to differentiate between\n>> once, multiple time or with --no-option.\n>>\n>> Eg. :\n>> initialize verbose = -1\n>> `git commit` => verbose = -1\n>> `git commit -v` => verbose = 1\n>> `git commit -v -v` => verbose = 2\n>> `git commit --no-verbose` => verbose = 0\n>>\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>>\n>> ---\n>> The discussion about this patch:\n>> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027\n>>\n>> Previous version of the patch:\n>> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061\n>>\n>> Changes introduced:\n>> Use a different language in commit message to make the change and its\n>> utility more clear.\n>\n> I don't see the mentioned change in the commit message.  In\n> particular:\n>\n>   - As Eric pointed out in the previous round, the commit message\n>     misses the single most important point of justification: to\n>     determine whether '--option' or '--no-option' was specified at\n>     all.  Explaining this would also make the examples unnecessary.\n\nI will justify it in the commit message. Also will remove the examples.\n\n>   - OPT_COUNTUP is used in several places, mostly indirecty, but the\n>     commit message doesn't explain that possible side-effects to these\n>     callers were considered and that they are not harmed by this\n>     change.\n\nI will include bits from my and Jeff's conversation into this commit message.\n\n>> ---\n>>  Documentation/technical/api-parse-options.txt | 6 ++++--\n>>  parse-options.c                               | 8 +++++++-\n>>  2 files changed, 11 insertions(+), 3 deletions(-)\n>\n> A couple of new tests to t0040-parse-options.sh would be great to\n> ensure that starting from a negative value works as advertised, i.e.\n> at least that '--option' jumps to 1 and '--no-option' resets to 0.\n>\n>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n>> index 5f0757d..a908d8a 100644\n>> --- a/Documentation/technical/api-parse-options.txt\n>> +++ b/Documentation/technical/api-parse-options.txt\n>> @@ -144,8 +144,10 @@ There are some macros to easily define options:\n>>\n>>  `OPT_COUNTUP(short, long, &int_var, description)`::\n>>       Introduce a count-up option.\n>> -     `int_var` is incremented on each use of `--option`, and\n>> -     reset to zero with `--no-option`.\n>> +     If the `int_var` is negative and `--option` is absent,\n>> +     then it will retain its value. Otherwise it will first set\n>> +     `int_var` to 0 and then increment it on each use of `--option`,\n>> +     and reset to zero with `--no-option`.\n>>\n>>  `OPT_BIT(short, long, &int_var, description, mask)`::\n>>       Introduce a boolean option.\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 47a9192..86d30bd 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,\n>>               return 0;\n>>\n>>       case OPTION_COUNTUP:\n>> -             *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n>> +             if (unset)\n>> +                     *(int *)opt->value = 0;\n>> +             else {\n>> +                     if (*(int *)opt->value < 0)\n>> +                             *(int *)opt->value = 0;\n>> +                     *(int *)opt->value += 1;\n>> +             }\n>>               return 0;\n>>\n>>       case OPTION_SET_INT:\n>>\n>> --\n>> https://github.com/git/git/pull/218\n>>\n>>\n"},{"id":"281660","messageId":"1458817259-11675-1-git-send-email-szeder@ira.uka.de","threadId":"41807","inReplyTo":"01020153a7ba4eae-9b88e119-0505-418f-a137-595250edaa9d-000000@eu-west-1.amazonses.com","subject":"[PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-24T11:00:59Z","receivedAt":"2016-03-24T11:00:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> Also remove test_set_editor from global scope and use it in whichever\n> test it is required.\n\nWhy?\n\ntest_set_editor sets and exports shell variables.  Since you don't\ninvoke test_set_editor in a subshell, after the first invocation the\neditor will be part of the global scope anyway.\n\nAlso missing signoff.\n\n> ---\n>  t/t7507-commit-verbose.sh | 12 ++++++++----\n>  1 file changed, 8 insertions(+), 4 deletions(-)\n> \n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 2ddf28c..cf95efb 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -3,12 +3,11 @@\n>  test_description='verbose commit template'\n>  . ./test-lib.sh\n>  \n> -cat >check-for-diff <<EOF\n> -#!$SHELL_PATH\n> -exec grep '^diff --git' \"\\$1\"\n> +write_script \"check-for-diff\" <<-\\EOF &&\n> +grep '^diff --git' \"$1\" >out &&\n> +test $(wc -l <out) = 1\n\nOur test lib offers the test_line_count helper function, which\noutputs a helpful error message in case the number of lines do not\nmatch.\n\nThe original didn't check the number of lines.  This change is not\nmentioned at all in the commit message.\n\n>  EOF\n>  chmod +x check-for-diff\n> -test_set_editor \"$PWD/check-for-diff\"\n>  \n>  cat >message <<'EOF'\n>  subject\n> @@ -23,6 +22,7 @@ test_expect_success 'setup' '\n>  '\n>  \n>  test_expect_success 'initial commit shows verbose diff' '\n> +\ttest_set_editor \"$PWD/check-for-diff\" &&\n>  \tgit commit --amend -v\n>  '\n>  \n> @@ -38,11 +38,13 @@ check_message() {\n>  }\n>  \n>  test_expect_success 'verbose diff is stripped out' '\n> +\ttest_set_editor \"$PWD/check-for-diff\" &&\n>  \tgit commit --amend -v &&\n>  \tcheck_message message\n>  '\n>  \n>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n> +\ttest_set_editor \"$PWD/check-for-diff\" &&\n>  \tgit config diff.mnemonicprefix true &&\n>  \tgit commit --amend -v &&\n>  \tcheck_message message\n> @@ -66,11 +68,13 @@ test_expect_success 'diff in message is retained without -v' '\n>  '\n>  \n>  test_expect_success 'diff in message is retained with -v' '\n> +\ttest_set_editor \"$PWD/check-for-diff\" &&\n>  \tgit commit --amend -F diff -v &&\n>  \tcheck_message diff\n>  '\n>  \n>  test_expect_success 'submodule log is stripped out too with -v' '\n> +\ttest_set_editor \"$PWD/check-for-diff\" &&\n>  \tgit config diff.submodule log &&\n>  \tgit submodule add ./. sub &&\n>  \tgit commit -m \"sub added\" &&\n> \n> --\n> https://github.com/git/git/pull/218\n> \n"},{"id":"281746","messageId":"CAPig+cRszLsZh1ka7+y3EehQxUK67jMrVJse1EOzwLCcs-7x9Q@mail.gmail.com","threadId":"41807","inReplyTo":"1458815593-11060-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-24T23:34:25Z","receivedAt":"2016-03-24T23:34:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 24, 2016 at 6:33 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>> The reason to make it consider negative values or more specifically\n>> \"unspecified\" values is to give the ability to differentiate between\n>> once, multiple time or with --no-option.\n>>\n>> Eg. :\n>> initialize verbose = -1\n>> `git commit` => verbose = -1\n>> `git commit -v` => verbose = 1\n>> `git commit -v -v` => verbose = 2\n>> `git commit --no-verbose` => verbose = 0\n>>\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>> ---\n>> Changes introduced:\n>> Use a different language in commit message to make the change and its\n>> utility more clear.\n>\n> I don't see the mentioned change in the commit message.  In\n> particular:\n>\n>   - As Eric pointed out in the previous round, the commit message\n>     misses the single most important point of justification: to\n>     determine whether '--option' or '--no-option' was specified at\n>     all.  Explaining this would also make the examples unnecessary.\n\nAgreed. It would be more helpful to explain that you are changing\nOPT_COUNTUP so that --option always counts up from zero, even if the\ninitial value was negative, in order to allow the caller to determine\nwhether --option or --no-option was seen at all, by setting the\ninitial value to -1 and then checking if it still -1 after\nparse_options(). If you explain that well, then the tabular example in\nyour commit message can go away altogether.\n\nThe subject (\"make OPTION__COUNTUP consider negative values\") could\nuse a little work too. OPTION__COUNTUP already works with negative\nvalues, but not in the way you want. Instead, you want it to treat\nnegative values specially as \"unspecified\", and that's the real gist\nof this patch, thus the subject ought, at the very least, have the\nword \"unspecified\" in it.\n\n>   - OPT_COUNTUP is used in several places, mostly indirecty, but the\n>     commit message doesn't explain that possible side-effects to these\n>     callers were considered and that they are not harmed by this\n>     change.\n>\n>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n>> @@ -144,8 +144,10 @@ There are some macros to easily define options:\n>>  `OPT_COUNTUP(short, long, &int_var, description)`::\n>>       Introduce a count-up option.\n>> -     `int_var` is incremented on each use of `--option`, and\n>> -     reset to zero with `--no-option`.\n>> +     If the `int_var` is negative and `--option` is absent,\n>> +     then it will retain its value. Otherwise it will first set\n>> +     `int_var` to 0 and then increment it on each use of `--option`,\n>> +     and reset to zero with `--no-option`.\n\nThis reads a little bit backward, but, more importantly, doesn't do a\ngood job of conveying to the reader that a negative value can\nrepresent \"unspecified\". The best way to convey that would probably be\nvia example. For instance, something like this:\n\n    Each use of `--option` increments `int_var`, starting from zero\n    (even if initially negative), and `--no-option` resets it to\n    zero. To determine if `--option` or `--no-option` was seen at\n    all, set `int_var` to a negative value, and if it is still\n    negative after parse_options(), then neither `--option` nor\n    `--no-option` was seen.\n"},{"id":"281756","messageId":"CAPig+cQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg@mail.gmail.com","threadId":"41807","inReplyTo":"1458817259-11675-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-24T23:57:11Z","receivedAt":"2016-03-24T23:57:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>> Also remove test_set_editor from global scope and use it in whichever\n>> test it is required.\n>\n> Why?\n>\n> test_set_editor sets and exports shell variables.  Since you don't\n> invoke test_set_editor in a subshell, after the first invocation the\n> editor will be part of the global scope anyway.\n\nAgreed that this needs proper justification in the commit message.\nAnd, the justification is to make each test more self-contained,\nparticularly because a subsequent patch will introduce a second fake\n\"editor\", and by making tests responsible for setting the editor they\nneed, they don't have to worry about bad interactions from \"editors\"\nset by earlier tests[1][2].\n\nAnother issue is that the commit message is backward. The subject\n(\"t7507-commit-verbose: make test suite use write_script\") tries to\nsell this as primarily being about write_script(), but the real gist\nof the patch is that it is making each test more self-contained, and\nthe subject should say so. The write_script() bit is just a minor\naside which can be introduced with a \"While here let's use\nwrite_script to...\" at the end of the commit message.\n\n>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>> index 2ddf28c..cf95efb 100755\n>> --- a/t/t7507-commit-verbose.sh\n>> +++ b/t/t7507-commit-verbose.sh\n>> @@ -3,12 +3,11 @@\n>>  test_description='verbose commit template'\n>>  . ./test-lib.sh\n>>\n>> -cat >check-for-diff <<EOF\n>> -#!$SHELL_PATH\n>> -exec grep '^diff --git' \"\\$1\"\n>> +write_script \"check-for-diff\" <<-\\EOF &&\n>> +grep '^diff --git' \"$1\" >out &&\n>> +test $(wc -l <out) = 1\n>\n> Our test lib offers the test_line_count helper function, which\n> outputs a helpful error message in case the number of lines do not\n> match.\n\ntest_line_count() was mentioned in [2], however, this line counting is\ndone in the fake \"editor\" script, not in the test script proper, so\nthe spelled-out form $(wc ...) was proposed[2].\n\n> The original didn't check the number of lines.  This change is not\n> mentioned at all in the commit message.\n\nRight, and this particular change really belongs in patch 3/3 where\nit's needed[2], and should be properly explained by the 3/3 commit\nmessage.\n\n>>  EOF\n>>  chmod +x check-for-diff\n\nDrop the 'chmod' line; write_script() does this for you.\n\n>> -test_set_editor \"$PWD/check-for-diff\"\n>>\n>>  cat >message <<'EOF'\n>>  subject\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289329\n[2]: http://article.gmane.org/gmane.comp.version-control.git/289370\n"},{"id":"281757","messageId":"CAPig+cQ3TnKV1GcViq9Qwtq8bkp19jqvQRzBT_v5-MSfSfXgZA@mail.gmail.com","threadId":"41807","inReplyTo":"01020153a7ba4ec2-1fb668c3-9dc6-4137-8d92-c6a5825a156c-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v9 3/3] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T00:05:55Z","receivedAt":"2016-03-25T00:05:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 24, 2016 at 4:25 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> Add commit.verbose configuration variable as a convenience for those\n> who always prefer --verbose.\n\nThe implementation looks better in this version. A couple comments\nbelow about the test script...\n\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> ---\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> @@ -9,6 +9,12 @@ test $(wc -l <out) = 1\n>  EOF\n>  chmod +x check-for-diff\n\nMentioned in patch 2/3 review, this patch (3/3) is where you should\nupdate 'check-for-diff' to also check the line count, and the commit\nmessage should explain the reason for doing so (and don't forget to\nmention that it won't harm existing clients of 'check-for-diff').\n\n> +write_script \"check-for-double-diff\" <<-\\EOF &&\n> +grep '# Changes not staged for commit' \"$1\" >out &&\n> +test $(wc -l <out) = 2\n> +EOF\n> +chmod +x check-for-double-diff\n\nAlso mentioned in patch 2/3 review: drop 'chmod'; write_script() does\nit for you.\n"},{"id":"281773","messageId":"CAFZEwPNMB2Ti2d=qdZKyoYzneHxUPATiFWd_fSPxktZuF3Q+pw@mail.gmail.com","threadId":"41807","inReplyTo":"CAPig+cQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg@mail.gmail.com","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-25T06:06:10Z","receivedAt":"2016-03-25T06:06:10Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Mar 25, 2016 at 5:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>>> Also remove test_set_editor from global scope and use it in whichever\n>>> test it is required.\n>>\n>> Why?\n>>\n>> test_set_editor sets and exports shell variables.  Since you don't\n>> invoke test_set_editor in a subshell, after the first invocation the\n>> editor will be part of the global scope anyway.\n>\n> Agreed that this needs proper justification in the commit message.\n> And, the justification is to make each test more self-contained,\n> particularly because a subsequent patch will introduce a second fake\n> \"editor\", and by making tests responsible for setting the editor they\n> need, they don't have to worry about bad interactions from \"editors\"\n> set by earlier tests[1][2].\n\nThis shou  cadve  mbe ave be ave be ave be ave be ave be ave be ave\n> Another issue is that the commit message is backward. The subject\n> (\"t7507-commit-verbose: make test suite use write_script\") tries to\n> sell this as primarily being about write_script(), but the real gist\n> of the patch is that it is making each test more self-contained, and\n> the subject should say so. The write_script() bit is just a minor\n> aside which can be introduced with a \"While here let's use\n> write_script to...\" at the end of the commit message.\n>\n>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>>> index 2ddf28c..cf95efb 100755\n>>> --- a/t/t7507-commit-verbose.sh\n>>> +++ b/t/t7507-commit-verbose.sh\n>>> @@ -3,12 +3,11 @@\n>>>  test_description='verbose commit template'\n>>>  . ./test-lib.sh\n>>>\n>>> -cat >check-for-diff <<EOF\n>>> -#!$SHELL_PATH\n>>> -exec grep '^diff --git' \"\\$1\"\n>>> +write_script \"check-for-diff\" <<-\\EOF &&\n>>> +grep '^diff --git' \"$1\" >out &&\n>>> +test $(wc -l <out) = 1\n>>\n>> Our test lib offers the test_line_count helper function, which\n>> outputs a helpful error message in case the number of lines do not\n>> match.\n>\n> test_line_count() was mentioned in [2], however, this line counting is\n> done in the fake \"editor\" script, not in the test script proper, so\n> the spelled-out form $(wc ...) was proposed[2].\n\nI have a slight doubt regarding this. Can the functions from test-lib\nwork in such scripts flawlessly? If yes, then its probably better to\nuse them.\n\n>> The original didn't check the number of lines.  This change is not\n>> mentioned at all in the commit message.\n>\n> Right, and this particular change really belongs in patch 3/3 where\n> it's needed[2], and should be properly explained by the 3/3 commit\n> message.\n\nSure! It should have been mentioned.\n\n>>>  EOF\n>>>  chmod +x check-for-diff\n>\n> Drop the 'chmod' line; write_script() does this for you.\n\nI was unaware about this. I will drop it off.\n\n>>> -test_set_editor \"$PWD/check-for-diff\"\n>>>\n>>>  cat >message <<'EOF'\n>>>  subject\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/289329\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/289370\n"},{"id":"281774","messageId":"CAFZEwPNXPDWA2R4p_pUWYEa+CeQDEpQiHz_b+UknfuA4+5KB5g@mail.gmail.com","threadId":"41807","inReplyTo":"CAPig+cQ3TnKV1GcViq9Qwtq8bkp19jqvQRzBT_v5-MSfSfXgZA@mail.gmail.com","subject":"Re: [PATCH v9 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-25T06:15:51Z","receivedAt":"2016-03-25T06:15:51Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Mar 25, 2016 at 5:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Mar 24, 2016 at 4:25 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> Add commit.verbose configuration variable as a convenience for those\n>> who always prefer --verbose.\n>\n> The implementation looks better in this version. A couple comments\n> below about the test script...\n>\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>> ---\n>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>> @@ -9,6 +9,12 @@ test $(wc -l <out) = 1\n>>  EOF\n>>  chmod +x check-for-diff\n>\n> Mentioned in patch 2/3 review, this patch (3/3) is where you should\n> update 'check-for-diff' to also check the line count, and the commit\n> message should explain the reason for doing so (and don't forget to\n> mention that it won't harm existing clients of 'check-for-diff').\n\nAgree that check the line count should belong to this patch. And will\nadd more details in the commit message.\n\n>> +write_script \"check-for-double-diff\" <<-\\EOF &&\n>> +grep '# Changes not staged for commit' \"$1\" >out &&\n>> +test $(wc -l <out) = 2\n>> +EOF\n>> +chmod +x check-for-double-diff\n>\n> Also mentioned in patch 2/3 review: drop 'chmod'; write_script() does\n> it for you.\n"},{"id":"281776","messageId":"CAPig+cRY8P2WtqR4y0zW+netCHa3=9Wnb8LmcB35SqP4OGC6Vg@mail.gmail.com","threadId":"41807","inReplyTo":"CAFZEwPNMB2Ti2d=qdZKyoYzneHxUPATiFWd_fSPxktZuF3Q+pw@mail.gmail.com","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T06:24:31Z","receivedAt":"2016-03-25T06:24:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 2:06 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Fri, Mar 25, 2016 at 5:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> Agreed that this needs proper justification in the commit message.\n>> And, the justification is to make each test more self-contained,\n>> particularly because a subsequent patch will introduce a second fake\n>> \"editor\", and by making tests responsible for setting the editor they\n>> need, they don't have to worry about bad interactions from \"editors\"\n>> set by earlier tests[1][2].\n>\n> This shou  cadve  mbe ave be ave be ave be ave be ave be ave be ave\n\nHmm, yes, what you say makes perfect sense... er, wait...\n\n>>>> -cat >check-for-diff <<EOF\n>>>> -#!$SHELL_PATH\n>>>> -exec grep '^diff --git' \"\\$1\"\n>>>> +write_script \"check-for-diff\" <<-\\EOF &&\n>>>> +grep '^diff --git' \"$1\" >out &&\n>>>> +test $(wc -l <out) = 1\n>>>\n>>> Our test lib offers the test_line_count helper function, which\n>>> outputs a helpful error message in case the number of lines do not\n>>> match.\n>>\n>> test_line_count() was mentioned in [2], however, this line counting is\n>> done in the fake \"editor\" script, not in the test script proper, so\n>> the spelled-out form $(wc ...) was proposed[2].\n>\n> I have a slight doubt regarding this. Can the functions from test-lib\n> work in such scripts flawlessly? If yes, then its probably better to\n> use them.\n\nProbably not easily, and it's not worth complicating the \"editor\"\nscript by trying to import the test_line_count() function.\n\n>>>>  chmod +x check-for-diff\n>>\n>> Drop the 'chmod' line; write_script() does this for you.\n>\n> I was unaware about this. I will drop it off.\n\nI thought I had mentioned it in the review in which I had suggested\nusing write_script() or one of the follow-up emails, but upon looking\nback at those messages, I saw it was not so. It turns out that I was\nprobably thinking about a different patch review in which I had\nmentioned dropping 'chmod'[1].\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/288085/\n"},{"id":"281779","messageId":"CAFZEwPPXPL=d2S+Htc+uFyrUfrWLZRk=7YFDevHNpuEB5ofVDQ@mail.gmail.com","threadId":"41807","inReplyTo":"CAPig+cRY8P2WtqR4y0zW+netCHa3=9Wnb8LmcB35SqP4OGC6Vg@mail.gmail.com","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-25T06:55:04Z","receivedAt":"2016-03-25T06:55:04Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Mar 25, 2016 at 11:54 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Mar 25, 2016 at 2:06 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> On Fri, Mar 25, 2016 at 5:27 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> Agreed that this needs proper justification in the commit message.\n>>> And, the justification is to make each test more self-contained,\n>>> particularly because a subsequent patch will introduce a second fake\n>>> \"editor\", and by making tests responsible for setting the editor they\n>>> need, they don't have to worry about bad interactions from \"editors\"\n>>> set by earlier tests[1][2].\n>>\n>> This shou  cadve  mbe ave be ave be ave be ave be ave be ave be ave\n>\n> Hmm, yes, what you say makes perfect sense... er, wait...\n\nhaha :) Sorry for this. My browser crashed and it sent out some crap\n(don't know how). I mean to say, \"This should have been mentioned in\nthe commit message that scope of editor is reduced so as to not worry\nabout bad interactions from other \"editors\" which may be used after\nwards.\"\n>\n>>>>> -cat >check-for-diff <<EOF\n>>>>> -#!$SHELL_PATH\n>>>>> -exec grep '^diff --git' \"\\$1\"\n>>>>> +write_script \"check-for-diff\" <<-\\EOF &&\n>>>>> +grep '^diff --git' \"$1\" >out &&\n>>>>> +test $(wc -l <out) = 1\n>>>>\n>>>> Our test lib offers the test_line_count helper function, which\n>>>> outputs a helpful error message in case the number of lines do not\n>>>> match.\n>>>\n>>> test_line_count() was mentioned in [2], however, this line counting is\n>>> done in the fake \"editor\" script, not in the test script proper, so\n>>> the spelled-out form $(wc ...) was proposed[2].\n>>\n>> I have a slight doubt regarding this. Can the functions from test-lib\n>> work in such scripts flawlessly? If yes, then its probably better to\n>> use them.\n>\n> Probably not easily, and it's not worth complicating the \"editor\"\n> script by trying to import the test_line_count() function.\n\nSure!\n\n>>>>>  chmod +x check-for-diff\n>>>\n>>> Drop the 'chmod' line; write_script() does this for you.\n>>\n>> I was unaware about this. I will drop it off.\n>\n> I thought I had mentioned it in the review in which I had suggested\n> using write_script() or one of the follow-up emails, but upon looking\n> back at those messages, I saw it was not so. It turns out that I was\n> probably thinking about a different patch review in which I had\n> mentioned dropping 'chmod'[1].\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/288085/\nOh! I hadn't tested by removing chmod. I will try it for fun.\n"},{"id":"281817","messageId":"20160325154638.Horde.LzlsD6cZcQfmt894cll_0Tl@webmail.informatik.kit.edu","threadId":"41807","inReplyTo":"CAPig+cQJ1yK8WhXE0sxSmDenMt1DMZCoJypTu_wkDsDyuBsAWg@mail.gmail.com","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-25T14:46:38Z","receivedAt":"2016-03-25T14:46:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Eric Sunshine <sunshine@sunshineco.com>:\n\n> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n\n>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>>> index 2ddf28c..cf95efb 100755\n>>> --- a/t/t7507-commit-verbose.sh\n>>> +++ b/t/t7507-commit-verbose.sh\n>>> @@ -3,12 +3,11 @@\n>>>  test_description='verbose commit template'\n>>>  . ./test-lib.sh\n>>>\n>>> -cat >check-for-diff <<EOF\n>>> -#!$SHELL_PATH\n>>> -exec grep '^diff --git' \"\\$1\"\n>>> +write_script \"check-for-diff\" <<-\\EOF &&\n>>> +grep '^diff --git' \"$1\" >out &&\n>>> +test $(wc -l <out) = 1\n>>\n>> Our test lib offers the test_line_count helper function, which\n>> outputs a helpful error message in case the number of lines do not\n>> match.\n>\n> test_line_count() was mentioned in [2], however, this line counting is\n> done in the fake \"editor\" script, not in the test script proper, so\n> the spelled-out form $(wc ...) was proposed[2].\n\nAh, yes, of course.\n\nBut then the question is: why is the line counting in the editor script\nin the first place?\n\nBy redirecting grep's output to a file in the editor script, like this\npatch wanted to, we can count the lines in the test script itself after\n'git commit' finished.  This way we could use test_line_count, with\nall its error reporting benefits, and we could use the same editor\nscript for all tests.\n\nAnd if you insist on doing the line counting in the editor script, then\nwhy redirecting grep's output and 'wc -l' separately, why not 'grep -c'?\n"},{"id":"281818","messageId":"CAFZEwPMxESunF2FojXY0AwBQuNqJuRrqWxnDwHESjypi5zJdXA@mail.gmail.com","threadId":"41807","inReplyTo":"20160325154638.Horde.LzlsD6cZcQfmt894cll_0Tl@webmail.informatik.kit.edu","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-25T14:50:49Z","receivedAt":"2016-03-25T14:50:49Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Mar 25, 2016 at 8:16 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n\n> By redirecting grep's output to a file in the editor script, like this\n> patch wanted to, we can count the lines in the test script itself after\n> 'git commit' finished.  This way we could use test_line_count, with\n> all its error reporting benefits, and we could use the same editor\n> script for all tests.\n>\n> And if you insist on doing the line counting in the editor script, then\n> why redirecting grep's output and 'wc -l' separately, why not 'grep -c'?\n\nNice way. Hadn't thought of this before. This will also eliminate\nhaving 2 different scripts to test for 1 line and other to test for 2\nlines.\n"},{"id":"281832","messageId":"CAPig+cQLOwAimmHeUmi5+KScgOP0hr9-MiX8gyGcEk6Zj0U_jQ@mail.gmail.com","threadId":"41807","inReplyTo":"20160325154638.Horde.LzlsD6cZcQfmt894cll_0Tl@webmail.informatik.kit.edu","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T17:04:41Z","receivedAt":"2016-03-25T17:04:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 10:46 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n> Quoting Eric Sunshine <sunshine@sunshineco.com>:\n>> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:\n>>>> -cat >check-for-diff <<EOF\n>>>> -#!$SHELL_PATH\n>>>> -exec grep '^diff --git' \"\\$1\"\n>>>> +write_script \"check-for-diff\" <<-\\EOF &&\n>>>> +grep '^diff --git' \"$1\" >out &&\n>>>> +test $(wc -l <out) = 1\n>>>\n>>> Our test lib offers the test_line_count helper function, which\n>>> outputs a helpful error message in case the number of lines do not\n>>> match.\n>>\n>> test_line_count() was mentioned in [2], however, this line counting is\n>> done in the fake \"editor\" script, not in the test script proper, so\n>> the spelled-out form $(wc ...) was proposed[2].\n>\n> Ah, yes, of course.\n>\n> But then the question is: why is the line counting in the editor script\n> in the first place?\n>\n> By redirecting grep's output to a file in the editor script, like this\n> patch wanted to, we can count the lines in the test script itself after\n> 'git commit' finished.  This way we could use test_line_count, with\n> all its error reporting benefits, and we could use the same editor\n> script for all tests.\n\nThat works too, simplifying the overall implementation, and\neliminating the need for the introductory patch which moves\n'test_set_editor' into each test.\n\n> And if you insist on doing the line counting in the editor script, then\n> why redirecting grep's output and 'wc -l' separately, why not 'grep -c'?\n\nUgh, I utterly forgot about -c.\n"},{"id":"281851","messageId":"CAFZEwPPRYBjdDabLJHsXTtzD2y=vqSZM4OiY4G=BtEF485JP_A@mail.gmail.com","threadId":"41807","inReplyTo":"CAPig+cQLOwAimmHeUmi5+KScgOP0hr9-MiX8gyGcEk6Zj0U_jQ@mail.gmail.com","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-25T18:15:28Z","receivedAt":"2016-03-25T18:15:28Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Mar 25, 2016 at 10:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n> That works too, simplifying the overall implementation, and\n> eliminating the need for the introductory patch which moves\n> 'test_set_editor' into each test.\n\nWouldn't it be cleaner if the introductory patch contain:\n1. using write_script()\n2. grep the output to a file\n3. test for no of lines=1 in required tests\n?\n\nThen the patch 3/3\n>> And if you insist on doing the line counting in the editor script, then\n>> why redirecting grep's output and 'wc -l' separately, why not 'grep -c'?\n>\n> Ugh, I utterly forgot about -c.\n"},{"id":"281889","messageId":"CAPig+cTFCB-FMqKPApJw2z8ZWZaE7zJ70kod_kQHvLv2U4=DXA@mail.gmail.com","threadId":"41807","inReplyTo":"CAFZEwPPRYBjdDabLJHsXTtzD2y=vqSZM4OiY4G=BtEF485JP_A@mail.gmail.com","subject":"Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T23:06:20Z","receivedAt":"2016-03-25T23:06:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 25, 2016 at 2:15 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Fri, Mar 25, 2016 at 10:34 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> That works too, simplifying the overall implementation, and\n>> eliminating the need for the introductory patch which moves\n>> 'test_set_editor' into each test.\n>\n> Wouldn't it be cleaner if the introductory patch contain:\n> 1. using write_script()\n> 2. grep the output to a file\n> 3. test for no of lines=1 in required tests\n> ?\n\nYou could do that.\n"},{"id":"281977","messageId":"CAFZEwPP5Hq0fM3Fhg+JV-aY6bWozjL6m_bNpb=JhjkUjDJu2sw@mail.gmail.com","threadId":"41807","inReplyTo":"1458815593-11060-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-28T18:42:08Z","receivedAt":"2016-03-28T18:42:08Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"> A couple of new tests to t0040-parse-options.sh would be great to\n> ensure that starting from a negative value works as advertised, i.e.\n> at least that '--option' jumps to 1 and '--no-option' resets to 0.\n\nI think adding tests to t0040-parse-options.sh cannot reflect the\nbehavior introduced by this patch.\nThis is because setting the initial value of the variable (which is\ngoing to be modified by the argument) is set in test-parse-options.c .\nA possible solution will be to modify the test-parse-options.c and\ninitialize the required variable (verbose or quiet) to \"unspecified\"\nvalue. But then this will leave one part of the untested ie. when the\ninitial value of the variable is 0. I could do one thing to test these\nboth behaviors which is to set verbose initially to -1 and leave quiet\n= 0. Since verbose and quiet are both consumers of OPT_COUNTUP, this\ncan test both the behaviors.\n\nI tried searching for alternatives but could not find any. Is there\nsomething else which you had thought before that would test this\nbehavior?\n\nIf not, then we are left with 2 options, either modify\ntest-parse-options.c or just hand test it whenever there seems to be a\nproblematic case.\n"},{"id":"281979","messageId":"CAPig+cR63D2L8dXJe9Gx-zFXr4-gNaFC6GdfWNPnt4d=Ds8tWg@mail.gmail.com","threadId":"41807","inReplyTo":"CAFZEwPP5Hq0fM3Fhg+JV-aY6bWozjL6m_bNpb=JhjkUjDJu2sw@mail.gmail.com","subject":"Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-28T19:03:07Z","receivedAt":"2016-03-28T19:03:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 28, 2016 at 2:42 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> A couple of new tests to t0040-parse-options.sh would be great to\n>> ensure that starting from a negative value works as advertised, i.e.\n>> at least that '--option' jumps to 1 and '--no-option' resets to 0.\n>\n> I think adding tests to t0040-parse-options.sh cannot reflect the\n> behavior introduced by this patch.\n> This is because setting the initial value of the variable (which is\n> going to be modified by the argument) is set in test-parse-options.c .\n> A possible solution will be to modify the test-parse-options.c and\n> initialize the required variable (verbose or quiet) to \"unspecified\"\n> value. But then this will leave one part of the untested ie. when the\n> initial value of the variable is 0. I could do one thing to test these\n> both behaviors which is to set verbose initially to -1 and leave quiet\n> = 0. Since verbose and quiet are both consumers of OPT_COUNTUP, this\n> can test both the behaviors.\n>\n> I tried searching for alternatives but could not find any. Is there\n> something else which you had thought before that would test this\n> behavior?\n>\n> If not, then we are left with 2 options, either modify\n> test-parse-options.c or just hand test it whenever there seems to be a\n> problematic case.\n\nModifying test-parse-options.c, if needed, was implied by SZEDER's\nsuggestion to add tests for this new behavior. test-parse-options.c\nexists strictly for testing option parsing, so don't feel constrained\nabout modifying or extending it in order to test the new count-up\nbehavior if the existing implementation doesn't directly support what\nyou want it to do.\n"}]}