{"thread":{"id":"41743","subject":"[PATCH v8 2/2] commit: add a commit.verbose config variable","startedAt":"2016-03-18T21:19:10Z","lastAt":"2016-03-23T20:43:31Z","messageCount":11,"participants":["Pranit Bauva","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":8,"patchTotal":2},"messages":[{"id":"281180","messageId":"010201538b98a7e5-ee604368-5a27-4884-b01e-027fa02bf1c6-000000@eu-west-1.amazonses.com","threadId":"41743","inReplyTo":"010201537710be08-f31428b3-5df3-4694-8c4a-0e8f058131b3-000000@eu-west-1.amazonses.com","subject":"[PATCH v8 1/2] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-18T21:19:10Z","receivedAt":"2016-03-18T21:19:10Z","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 \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/213\n"},{"id":"281179","messageId":"010201538b98aaa4-ad08c159-4935-48f1-acdb-7c376da2c368-000000@eu-west-1.amazonses.com","threadId":"41743","inReplyTo":"010201538b98a7e5-ee604368-5a27-4884-b01e-027fa02bf1c6-000000@eu-west-1.amazonses.com","subject":"[PATCH v8 2/2] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-18T21:19:11Z","receivedAt":"2016-03-18T21:19:11Z","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\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 - Consider multiple verbosity as mentioned by SZEDER.\n - To implement this, change the way in which OPTION_COUNTUP() works.\n\nApproach used: (Suggested by Junio Hamano and Eric Sunshine)\n - initialize verbose to \"unspecified\"\n - initialize config_verbose to \"unspecified\"\n - let git_config() update config_verbose\n - let parse_options() update verbose\n\n Then:\n - If verbose is still \"unspecified\", then overwrite it with\n config_verbose.\n - If verbose is still \"unspecified\" after that, then neither the\n command line nor the configuration gave you verbosity.\n - Otherwise verbose at this point has the verbosity level to use so set\n it to 0.\n---\n Documentation/config.txt | 4 ++++\n Documentation/git-commit.txt | 3 ++-\n builtin/commit.c | 17 +++++++++++++-\n t/t7507-commit-verbose.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 77 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..ad85be0 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,14 @@ 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 < 0) {\n+\t\tif (config_verbose > -1)\n+\t\t\tverbose = config_verbose;\n+\t\telse\n+\t\t\tverbose = 0;\n+\t}\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 2ddf28c..dda674e 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -96,4 +96,59 @@ 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+\tgit -c commit.verbose=true commit --amend\n+'\n+\n+test_expect_success 'commit.verbose true and --verbose' '\n+\t(\n+\t\tGIT_EDITOR=cat &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit -c commit.verbose=true commit --amend --verbose\n+\t) &&\n+\tgrep \"^diff --git\" .git/COMMIT_EDITMSG >out &&\n+\twc -l out | grep \"1\"\n+'\n+\n+test_expect_success 'commit.verbose true and -v -v' '\n+\t(\n+\t\tGIT_EDITOR=cat &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit -c commit.verbose=true commit --amend -v -v\n+\t) &&\n+\tgrep \"# Changes not staged for commit\" .git/COMMIT_EDITMSG >out &&\n+\twc -l out | grep \"2\"\n+'\n+\n+test_expect_success 'commit.verbose true and --no-verbose' '\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+\tgit -c commit.verbose=false commit --amend --verbose\n+'\n+\n+test_expect_success 'commit.verbose false and -v -v' '\n+\t(\n+\t\tGIT_EDITOR=cat &&\n+\t\texport GIT_EDITOR &&\n+\t\tgit -c commit.verbose=false commit --amend -v -v\n+\t) &&\n+\tgrep \"# Changes not staged for commit\" .git/COMMIT_EDITMSG >out &&\n+\twc -l out | grep \"2\"\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose omitted' '\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_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/213\n"},{"id":"281188","messageId":"xmqq4mc3pf3y.fsf@gitster.mtv.corp.google.com","threadId":"41743","inReplyTo":"010201538b98a7e5-ee604368-5a27-4884-b01e-027fa02bf1c6-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v8 1/2] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-18T22:59:13Z","receivedAt":"2016-03-18T22:59:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\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\nA few more things I noticed about this are:\n\n - Many uses of COUNTUP has now been replaced with BOOL and what\n remains are verbose/quiet/force.\n\n - This change will not affect existing users of COUNTUP at all, as\n long as they use the initial value of 0 (or more), as there is no\n mechanism to decrement. The only thing the command line can do\n is to reset it to zero with \"--no-foo\".\n\nSo it seems a safe and sensible change. Even though I suspect that\nthe justification can be written more clearly, I am not sure if it\nworth the extra effort.\n\n>\n> Signed-off-by: Pranit Bauva \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> Documentation/technical/api-parse-options.txt | 6 ++++--\n> parse-options.c | 8 +++++++-\n> 2 files changed, 11 insertions(+), 3 deletions(-)\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/213\n"},{"id":"281210","messageId":"CAFZEwPNkWYC4nj6bjQd+HYxSVjm_UQwGF5UQwOxziA8K6iuszg@mail.gmail.com","threadId":"41743","inReplyTo":"xmqq4mc3pf3y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v8 1/2] parse-options.c: make OPTION__COUNTUP consider negative values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-19T04:55:09Z","receivedAt":"2016-03-19T04:55:09Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sat, Mar 19, 2016 at 4:29 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\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> A few more things I noticed about this are:\n>\n> - Many uses of COUNTUP has now been replaced with BOOL and what\n> remains are verbose/quiet/force.\n>\n> - This change will not affect existing users of COUNTUP at all, as\n> long as they use the initial value of 0 (or more), as there is no\n> mechanism to decrement. The only thing the command line can do\n> is to reset it to zero with \"--no-foo\".\n>\n> So it seems a safe and sensible change. Even though I suspect that\n> the justification can be written more clearly, I am not sure if it\n> worth the extra effort.\n\nI can include this paragraph. Some people might have a bit difficulty\nin knowing the importance as they might not have followed my earlier\npatches. To bridge this gap, I can include your paragraph in the\ncommit message. Any other particular things you want me to justify? I\nwill be glad to send another re-roll of this.\n\nRegards,\nPranit Bauva\n"},{"id":"281253","messageId":"CAPig+cShT2aPkoRK0pXPzbRbQqCU+O5uEqEr4oMkZ8LTwREaAA@mail.gmail.com","threadId":"41743","inReplyTo":"010201538b98aaa4-ad08c159-4935-48f1-acdb-7c376da2c368-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v8 2/2] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-20T03:56:48Z","receivedAt":"2016-03-20T03:56:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 18, 2016 at 5:19 PM, Pranit Bauva wrote:\n> Add commit.verbose configuration variable as a convenience for those\n> who always prefer --verbose.\n>\n> Signed-off-by: Pranit Bauva \n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -1654,6 +1661,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> + if (verbose < 0) {\n> + if (config_verbose > -1)\n> + verbose = config_verbose;\n> + else\n> + verbose = 0;\n> + }\n\nI think it's more common in this codebase to compare against -1\ndirectly rather than <0, so:\n\n if (verbose == -1) {\n if (config_verbose != -1)\n verbose = config_verbose;\n else\n verbose = 0;\n }\n\nOr, this might be easier to read:\n\n if (verbose == -1)\n verbose = config_verbose;\n\n if (verbose == -1)\n verbose = 0;\n\nBut, this likely isn't better:\n\n if (verbose == -1)\n verbose = config_verbose == -1 ? 0 : config_verbose;\n\nAnyhow, probably not worth a re-roll.\n\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> @@ -96,4 +96,59 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n> +test_expect_success 'commit.verbose true and --verbose' '\n> + (\n> + GIT_EDITOR=cat &&\n> + export GIT_EDITOR &&\n> + git -c commit.verbose=true commit --amend --verbose\n\nEasier would be to write this as:\n\n GIT_EDITOR=cat git -c commit.verbose=true commit --amend --verbose\n\nand then you wouldn't need the subhsell.\n\nHowever, more intuitive would probably be to create another \"editor\"\nsimilar to the 'check-for-diff' editor this script already uses. (The\n'check-for-diff' editor is an obvious example about how to go about\nsuch an undertaking.) You would need to invoke 'test_set_editor' in a\nsubshell for this particular test in order to avoid clobbering the\nglobal editor used by this script. Or, have a preparatory patch which\nditches the global setting of the editor and has each test invoke\n'test_set_editor' as needed (and only if needed).\n\nSame comments apply to the other new tests which use a custom \"editor\".\n\n> + ) &&\n> + grep \"^diff --git\" .git/COMMIT_EDITMSG >out &&\n> + wc -l out | grep \"1\"\n> +'\n> +\n> +test_expect_success 'commit.verbose true and -v -v' '\n> + (\n> + GIT_EDITOR=cat &&\n> + export GIT_EDITOR &&\n> + git -c commit.verbose=true commit --amend -v -v\n> + ) &&\n> + grep \"# Changes not staged for commit\" .git/COMMIT_EDITMSG >out &&\n> + wc -l out | grep \"2\"\n> +'\n> +\n> +test_expect_success 'commit.verbose true and --no-verbose' '\n> + test_must_fail git -c commit.verbose=true commit --amend --no-verbose\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --verbose' '\n> + git -c commit.verbose=false commit --amend --verbose\n> +'\n> +\n> +test_expect_success 'commit.verbose false and -v -v' '\n> + (\n> + GIT_EDITOR=cat &&\n> + export GIT_EDITOR &&\n> + git -c commit.verbose=false commit --amend -v -v\n> + ) &&\n> + grep \"# Changes not staged for commit\" .git/COMMIT_EDITMSG >out &&\n> + wc -l out | grep \"2\"\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --verbose omitted' '\n> + test_must_fail git -c commit.verbose=false commit --amend\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --no-verbose' '\n> + test_must_fail git -c commit.verbose=false commit --amend --no-verbose\n> +'\n> +\n> +test_expect_success 'status ignores commit.verbose=true' '\n> + git -c commit.verbose=true status >actual &&\n> + ! grep \"^diff --git\" actual\n> +'\n> +\n> test_done\n"},{"id":"281269","messageId":"CAFZEwPMZk9jDpi4F9WiYhv4aTqFqn3127i1AUKtNJXwG-11tyw@mail.gmail.com","threadId":"41743","inReplyTo":"CAPig+cShT2aPkoRK0pXPzbRbQqCU+O5uEqEr4oMkZ8LTwREaAA@mail.gmail.com","subject":"Re: [PATCH v8 2/2] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-20T11:05:31Z","receivedAt":"2016-03-20T11:05:31Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine wrote:\n> On Fri, Mar 18, 2016 at 5:19 PM, Pranit Bauva wrote:\n>> Add commit.verbose configuration variable as a convenience for those\n>> who always prefer --verbose.\n>>\n>> Signed-off-by: Pranit Bauva \n>> ---\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> @@ -1654,6 +1661,14 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>> + if (verbose < 0) {\n>> + if (config_verbose > -1)\n>> + verbose = config_verbose;\n>> + else\n>> + verbose = 0;\n>> + }\n>\n> I think it's more common in this codebase to compare against -1\n> directly rather than <0, so:\n>\n> if (verbose == -1) {\n> if (config_verbose != -1)\n> verbose = config_verbose;\n> else\n> verbose = 0;\n> }\n>\n> Or, this might be easier to read:\n>\n> if (verbose == -1)\n> verbose = config_verbose;\n>\n> if (verbose == -1)\n> verbose = 0;\n>\n> But, this likely isn't better:\n>\n> if (verbose == -1)\n> verbose = config_verbose == -1 ? 0 : config_verbose;\n>\n> Anyhow, probably not worth a re-roll.\n\nI will note this for future patches.\n\n>\n>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>> @@ -96,4 +96,59 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n>> +test_expect_success 'commit.verbose true and --verbose' '\n>> + (\n>> + GIT_EDITOR=cat &&\n>> + export GIT_EDITOR &&\n>> + git -c commit.verbose=true commit --amend --verbose\n>\n> Easier would be to write this as:\n>\n> GIT_EDITOR=cat git -c commit.verbose=true commit --amend --verbose\n>\n> and then you wouldn't need the subhsell.\n\nTrue. I will update this.\n\n> However, more intuitive would probably be to create another \"editor\"\n> similar to the 'check-for-diff' editor this script already uses. (The\n> 'check-for-diff' editor is an obvious example about how to go about\n> such an undertaking.) You would need to invoke 'test_set_editor' in a\n> subshell for this particular test in order to avoid clobbering the\n> global editor used by this script. Or, have a preparatory patch which\n> ditches the global setting of the editor and has each test invoke\n> 'test_set_editor' as needed (and only if needed).\n\nI guess it would complicate things as sometimes I need to check\nwhether it has 1 line and sometimes 2 lines.\n\n> Same comments apply to the other new tests which use a custom \"editor\".\n>\n>> + ) &&\n>> + grep \"^diff --git\" .git/COMMIT_EDITMSG >out &&\n>> + wc -l out | grep \"1\"\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose true and -v -v' '\n>> + (\n>> + GIT_EDITOR=cat &&\n>> + export GIT_EDITOR &&\n>> + git -c commit.verbose=true commit --amend -v -v\n>> + ) &&\n>> + grep \"# Changes not staged for commit\" .git/COMMIT_EDITMSG >out &&\n>> + wc -l out | grep \"2\"\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose true and --no-verbose' '\n>> + test_must_fail git -c commit.verbose=true commit --amend --no-verbose\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and --verbose' '\n>> + git -c commit.verbose=false commit --amend --verbose\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and -v -v' '\n>> + (\n>> + GIT_EDITOR=cat &&\n>> + export GIT_EDITOR &&\n>> + git -c commit.verbose=false commit --amend -v -v\n>> + ) &&\n>> + grep \"# Changes not staged for commit\" .git/COMMIT_EDITMSG >out &&\n>> + wc -l out | grep \"2\"\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and --verbose omitted' '\n>> + test_must_fail git -c commit.verbose=false commit --amend\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and --no-verbose' '\n>> + test_must_fail git -c commit.verbose=false commit --amend --no-verbose\n>> +'\n>> +\n>> +test_expect_success 'status ignores commit.verbose=true' '\n>> + git -c commit.verbose=true status >actual &&\n>> + ! grep \"^diff --git\" actual\n>> +'\n>> +\n>> test_done\n"},{"id":"281294","messageId":"CAPig+cSCrdxYb=0WOiAARrrPK07PeyTKsopPfwDOLAO4m3scnQ@mail.gmail.com","threadId":"41743","inReplyTo":"CAFZEwPMZk9jDpi4F9WiYhv4aTqFqn3127i1AUKtNJXwG-11tyw@mail.gmail.com","subject":"Re: [PATCH v8 2/2] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-20T17:34:57Z","receivedAt":"2016-03-20T17:34:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva wrote:\n> On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine wrote:\n>> However, more intuitive would probably be to create another \"editor\"\n>> similar to the 'check-for-diff' editor this script already uses. (The\n>> 'check-for-diff' editor is an obvious example about how to go about\n>> such an undertaking.) You would need to invoke 'test_set_editor' in a\n>> subshell for this particular test in order to avoid clobbering the\n>> global editor used by this script. Or, have a preparatory patch which\n>> ditches the global setting of the editor and has each test invoke\n>> 'test_set_editor' as needed (and only if needed).\n>\n> I guess it would complicate things as sometimes I need to check\n> whether it has 1 line and sometimes 2 lines.\n\nIt's not really a big complication. If I'm reading the patch\ncorrectly, you should be able to re-use the existing check-for-diff\n\"editor\" for the first of the new tests for which you're currently\nsetting a custom GIT_EDITOR. To do so, you will need to modify\ncheck-for-diff to also count lines and assert that only one was found,\nwhich should work for the new test and continue working for the\nexisting tests.\n\nThen, you just need to create one more \"editor\" for the two tests\nwhere you set custom GIT_EDITOR and expect two lines.\n\nBy the way, I forgot to mention in the review that, rather than:\n\n wc -l out | grep \"1\"\n\nfor counting lines in a test script, you'd use:\n\n test_line_count = 1 out\n\nHowever, if you're doing it in an \"editor\" (which I recommend), then you'd use:\n\n test $(wc -l wrote:\n> On Sun, Mar 20, 2016 at 7:05 AM, Pranit Bauva wrote:\n>> On Sun, Mar 20, 2016 at 9:26 AM, Eric Sunshine wrote:\n>>> However, more intuitive would probably be to create another \"editor\"\n>>> similar to the 'check-for-diff' editor this script already uses. (The\n>>> 'check-for-diff' editor is an obvious example about how to go about\n>>> such an undertaking.) You would need to invoke 'test_set_editor' in a\n>>> subshell for this particular test in order to avoid clobbering the\n>>> global editor used by this script. Or, have a preparatory patch which\n>>> ditches the global setting of the editor and has each test invoke\n>>> 'test_set_editor' as needed (and only if needed).\n>>\n>> I guess it would complicate things as sometimes I need to check\n>> whether it has 1 line and sometimes 2 lines.\n>\n> It's not really a big complication. If I'm reading the patch\n> correctly, you should be able to re-use the existing check-for-diff\n> \"editor\" for the first of the new tests for which you're currently\n> setting a custom GIT_EDITOR. To do so, you will need to modify\n> check-for-diff to also count lines and assert that only one was found,\n> which should work for the new test and continue working for the\n> existing tests.\n>\n> Then, you just need to create one more \"editor\" for the two tests\n> where you set custom GIT_EDITOR and expect two lines.\n>\n> By the way, I forgot to mention in the review that, rather than:\n>\n> wc -l out | grep \"1\"\n>\n> for counting lines in a test script, you'd use:\n>\n> test_line_count = 1 out\n>\n> However, if you're doing it in an \"editor\" (which I recommend), then you'd use:\n>\n> test $(wc -l \n> And, another \"by the way\": You can use the write_script() function to\n> simplify creation of the new \"editor(s)\".\n\nThanks for a detailed explanation.\n\n> In fact, it would be nice to convert creation of the check-for-diff\n> \"editor\" to use write_script, as well. So, basically, I'm suggesting\n> splitting the current patch into three, where the first two are\n> preparatory cleanups:\n>\n> 1. use write_script() to create the check-for-diff \"editor\" rather\n> than creating it manually\n>\n> 2. drop the global test_set_editor() and instead have each test invoke\n> it as needed (and only if needed)\n>\n> 3. the current patch which adds new tests along with a new \"editor\"\n>\n> Alternatively, combine #1 and #2 into a single patch which drops the\n> global test_set_editor() and, as an aside, also does \"while here,\n> let's use write_script() to create 'check-for'diff' rather than doing\n> so manually\".\n\nThese changes seem nice. I will update and send the patch.\n"},{"id":"281601","messageId":"xmqq7fgtc88i.fsf@gitster.mtv.corp.google.com","threadId":"41743","inReplyTo":"CAFZEwPNKozf+owtJtaE27MqQb3koL0L=HDnErVoWrTnQpieGgw@mail.gmail.com","subject":"Re: [PATCH v8 2/2] commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-23T19:19:41Z","receivedAt":"2016-03-23T19:19:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine wrote:\n> ...\n>> Alternatively, combine #1 and #2 into a single patch which drops the\n>> global test_set_editor() and, as an aside, also does \"while here,\n>> let's use write_script() to create 'check-for'diff' rather than doing\n>> so manually\".\n>\n> These changes seem nice. I will update and send the patch.\n\nSo, has anything happened to this topic or has it been abandoned?\n\nI am not in a hurry, just wanted to see if I need to keep the old\none in my tree as a reminder to myself.\n"},{"id":"281602","messageId":"CAFZEwPM1tukRCewKCgvYMQNDRMWh4MXcrXAbdn4S3Hg8c1R+hA@mail.gmail.com","threadId":"41743","inReplyTo":"xmqq7fgtc88i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v8 2/2] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-23T19:23:19Z","receivedAt":"2016-03-23T19:23:19Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine wrote:\n>> ...\n>>> Alternatively, combine #1 and #2 into a single patch which drops the\n>>> global test_set_editor() and, as an aside, also does \"while here,\n>>> let's use write_script() to create 'check-for'diff' rather than doing\n>>> so manually\".\n>>\n>> These changes seem nice. I will update and send the patch.\n>\n> So, has anything happened to this topic or has it been abandoned?\n>\n> I am not in a hurry, just wanted to see if I need to keep the old\n> one in my tree as a reminder to myself.\n\nSorry for that! Actually I am bit caught up with writing my proposal\nfor GSoC 2016. I would be able to complete that in around an hour.\nThen will work on this. Then on the shell function -> C function\nporting patch. Please bear with me for a little while.\n"},{"id":"281604","messageId":"xmqq37rhc4cs.fsf@gitster.mtv.corp.google.com","threadId":"41743","inReplyTo":"CAFZEwPM1tukRCewKCgvYMQNDRMWh4MXcrXAbdn4S3Hg8c1R+hA@mail.gmail.com","subject":"Re: [PATCH v8 2/2] commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-23T20:43:31Z","receivedAt":"2016-03-23T20:43:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> On Thu, Mar 24, 2016 at 12:49 AM, Junio C Hamano wrote:\n>> Pranit Bauva writes:\n>>\n>>> On Sun, Mar 20, 2016 at 11:04 PM, Eric Sunshine wrote:\n>>> ...\n>>>> Alternatively, combine #1 and #2 into a single patch which drops the\n>>>> global test_set_editor() and, as an aside, also does \"while here,\n>>>> let's use write_script() to create 'check-for'diff' rather than doing\n>>>> so manually\".\n>>>\n>>> These changes seem nice. I will update and send the patch.\n>>\n>> So, has anything happened to this topic or has it been abandoned?\n>>\n>> I am not in a hurry, just wanted to see if I need to keep the old\n>> one in my tree as a reminder to myself.\n>\n> Sorry for that! Actually I am bit caught up with writing my proposal\n> for GSoC 2016. I would be able to complete that in around an hour.\n> Then will work on this. Then on the shell function -> C function\n> porting patch. Please bear with me for a little while.\n\nOh, no worries. Naturally the application/proposal should take\nhigher priority.\n\nThanks.\n"}]}