{"thread":{"id":"41835","subject":"[PATCH v10 3/3] commit: add a commit.verbose config variable","startedAt":"2016-03-26T19:48:49Z","lastAt":"2016-03-31T20:45:38Z","messageCount":35,"participants":["Pranit Bauva","Eric Sunshine","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":10,"patchTotal":3},"messages":[{"id":"281910","messageId":"01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000@eu-west-1.amazonses.com","threadId":"41835","inReplyTo":"01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com","subject":"[PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-26T19:48:49Z","receivedAt":"2016-03-26T19:48:49Z","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 taking care of multiple levels of verbosity.\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 - [v9] $gmane/288820\n - [v8] $gmane/288820\n - [v7] $gmane/288820\n - [v6] $gmane/288728\n - [v5] $gmane/288728\n - [v4] $gmane/288652\n - [v3] $gmane/288634\n - [v2] $gmane/288569\n - [v1] $gmane/287540\n\nChanges with respect to the previous patch:\n - Fixed a status related bug pointed out by SZEDER\n - Change some tests\n---\n Documentation/config.txt | 4 ++++\n Documentation/git-commit.txt | 3 ++-\n builtin/commit.c | 16 ++++++++++++++-\n t/t7507-commit-verbose.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++\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..6a80a38 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@@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tfinalize_colopts(&s.colopts, -1);\n \tfinalize_deferred_config(&s);\n \n+\tif (verbose == -1)\n+\t\tverbose = 0;\n+\n \thandle_untracked_files_arg(&s);\n \tif (show_ignored_in_status)\n \t\ts.show_ignored_files = 1;\n@@ -1505,6 +1510,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 +1664,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 b40eb3c..569fd8b 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -101,4 +101,52 @@ 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+\techo content >file2 &&\n+\techo content >>file &&\n+\tgit add file2 &&\n+\tgit -c commit.verbose=true commit -F message &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose true and --verbose' '\n+\tgit -c commit.verbose=true commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose true and -v -v' '\n+\tgit -c commit.verbose=true commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose true and --no-verbose' '\n+\tgit -c commit.verbose=true commit --amend --no-verbose &&\n+\t! test -s out\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose' '\n+\tgit -c commit.verbose=false commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose false and -v -v' '\n+\tgit -c commit.verbose=false commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose omitted' '\n+\tgit -c commit.verbose=false commit --amend &&\n+\t! test -s out\n+'\n+\n+test_expect_success 'commit.verbose false and --no-verbose' '\n+\tgit -c commit.verbose=false commit --amend --no-verbose &&\n+\t! test -s out\n+'\n+\n+test_expect_success 'status ignores commit.verbose=true' '\n+\tgit -c commit.verbose=true status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n test_done\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"281911","messageId":"01020153b478cf9b-1417ac08-005c-4d72-91f1-0b07f00a24d5-000000@eu-west-1.amazonses.com","threadId":"41835","inReplyTo":"01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com","subject":"[PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-26T19:48:49Z","receivedAt":"2016-03-26T19:48:49Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"So that we can see how many diffs were contained in the message and use\nthem in individual tests where ever it is required. Also use\nwrite_script() to create the fake \"editor\".\n\nHelped-by: Eric Sunshine \nSigned-off-by: Pranit Bauva \n---\n t/t7507-commit-verbose.sh | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..b40eb3c 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -3,9 +3,11 @@\n test_description='verbose commit template'\n . ./test-lib.sh\n \n-cat >check-for-diff <out\n EOF\n chmod +x check-for-diff\n test_set_editor \"$PWD/check-for-diff\"\n@@ -23,7 +25,8 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'initial commit shows verbose diff' '\n-\tgit commit --amend -v\n+\tgit commit --amend -v &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'second commit' '\n@@ -39,13 +42,15 @@ check_message() {\n \n test_expect_success 'verbose diff is stripped out' '\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n \tgit config diff.mnemonicprefix true &&\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n cat >diff <<'EOF'\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"281912","messageId":"01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com","threadId":"41835","inReplyTo":"01020153a7ba4e07-f326395b-63df-4f8a-b378-8f387b02d0bd-000000@eu-west-1.amazonses.com","subject":"[PATCH v10 1/3] parse-options.c: make OPTION__COUNTUP understand \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-26T19:48:49Z","receivedAt":"2016-03-26T19:48:49Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"The reason to make it understand negative values or more specifically\n\"unspecified\" values is to give the ability to differentiate whether\n`--option` or `--no-option` was specified at all.\n\nMany uses of COUNTUP have now been replaced with BOOL and what remains\nare verbose/quiet/force. This change will not affect existing users of\nCOUNTUP at all, as long as they use the initial value of 0 (or more), as\nthere is no mechanism to decrement. The only thing the command line can\ndo is to reset it to zero with \"--no-foo\".\n\nVerbose or quiet don't use negative values before this commit but force\nuses it in a different way to handle its config and then munges the \"-1\"\ninto 0 before we get to parse_options.\n\nThere are uses of COUNTUP in cmd_hash_object() and test-parse-options.c\nand they are both fine.\n\nHelped-by: Jeff King \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nSigned-off-by: Pranit Bauva \n\n---\nThe discussion about this patch:\n[1] : http://thread.gmane.org/gmane.comp.version-control.git/289027\n\nPrevious version of the patch:\n[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. Also added some points as to where this patch could\nbreak but it doesn't.\n---\n Documentation/technical/api-parse-options.txt | 8 ++++++--\n parse-options.c | 8 +++++++-\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 5f0757d..8908bf7 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -144,8 +144,12 @@ There are some macros to easily define options:\n \n `OPT_COUNTUP(short, long, &int_var, description)`::\n \tIntroduce a count-up option.\n-\t`int_var` is incremented on each use of `--option`, and\n-\treset to zero with `--no-option`.\n+\tEach use of `--option` increments `int_var`, starting from zero\n+\t(even if initially negative), and `--no-option` resets it to\n+\tzero. To determine if `--option` or `--no-option` was set at\n+\tall, set `int_var` to a negative value, and if it is still\n+\tnegative after parse_options(), then neither `--option` nor\n+\t`--no-option` was seen.\n \n `OPT_BIT(short, long, &int_var, description, mask)`::\n \tIntroduce a boolean option.\ndiff --git a/parse-options.c b/parse-options.c\nindex 47a9192..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":"281915","messageId":"CAPig+cTB+xZmwALTY2Oy2_dVXknSEjaBiQ8NNr-qOVOZKjHvXw@mail.gmail.com","threadId":"41835","inReplyTo":"01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v10 1/3] parse-options.c: make OPTION__COUNTUP understand \"unspecified\" values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T02:45:41Z","receivedAt":"2016-03-27T02:45:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n> parse-options.c: make OPTION__COUNTUP understand \"unspecified\" values\n\nA bit clearer: s/understand/respect/\nAlso: s/__/_/\n\n> The reason to make it understand negative values or more specifically\n> \"unspecified\" values is to give the ability to differentiate whether\n> `--option` or `--no-option` was specified at all.\n\nThe word \"negative\" shows up far too early in this paragraph, even\nbefore \"unspecified\". It also fails utterly to explain what\n\"unspecified\" means and how to use it. It does vaguely explain why\nrespecting \"unspecified\" is desirable (to know if --[no-]option was\nseen), but otherwise leaves the reader quite clueless.\n\n> Many uses of COUNTUP have now been replaced with BOOL and what remains\n> are verbose/quiet/force. This change will not affect existing users of\n> COUNTUP at all, as long as they use the initial value of 0 (or more), as\n> there is no mechanism to decrement. The only thing the command line can\n> do is to reset it to zero with \"--no-foo\".\n\nCopying this paragraph verbatim from Junio's email[1] isn't\nnecessarily the best way to compose a commit message. Junio was\n\"thinking out loud\" while justifying the change to himself as being\nsafe, but you ought to reframe this reasoning into a more concise form\nwhich flows with the rest of the commit message, keeping the important\nbits and dropping others.\n\n> Verbose or quiet don't use negative values before this commit but force\n> uses it in a different way to handle its config and then munges the \"-1\"\n> into 0 before we get to parse_options.\n\nPresumably, you're talking about a very specific instance of -1 in\nconjunction with OPT__FORCE in builtin/clean.c, but that information\nis entirely missing from this paragraph, thus this explanation serves\nonly to confuse rather than clarify. It is a good idea to cite this\nspecific unusual case when justifying that this change is safe, but it\nmust be accompanied by sufficient context for the reader to understand\nwhat is being said.\n\n> There are uses of COUNTUP in cmd_hash_object() and test-parse-options.c\n> and they are both fine.\n\nWhat does \"fine\" mean? I know from reading the code that \"fine\" means\nthat these clients initialize the values to 0, thus should see no\nbehavioral difference. But why are these two cases called out\nspecially when they are already covered by the above \"as long as they\nuse initial value of 0\" explanation? As a reader, having these two\ncases mentioned specially makes me wonder if I'm missing something\nnon-obvious about them.\n\nIt would probably be better to avoid mentioning cmd_hash_object() and\ntest-parse-options.c at all, and instead explain generally that, with\none exception (builtin/clean.c), all current clients of OPTION_COUNTUP\nuse an initial value of zero, thus are not impacted by this change.\nAnd, then go on to explain builtin/clean.c's special case and why it\nis safe, as well.\n\n\nSorry for seeming to be very picky, but if I hadn't already been\nfollowing this topic (and hadn't in fact suggested this particular\nchange), as a reader, I think I would find this commit message utterly\nbaffling, and wouldn't have a clue what this change is about or why it\nis desirable.\n\nPerhaps it would be a good idea to re-read \"(2) Describe your changes\nwell\" in Documentation/SubmittingPatches for hints about writing a\ngood commit message, as well as Junio's recent re-stating[2] of those\nhints.\n\nTry to write the commit message as if you were speaking to someone who\nwasn't already familiar with the issue you're trying to solve.\nSpecifically, explain the problem (\"no way to distinguish between\n--[no-]option being seen or not\"); explain the solution (\"introduce an\n'unspecified' value\"); the implementation of the \"unspecified\" value\n(\"any negative number\" plus an example of how to make use of it);\njustify that the change is safe (\"existing clients of OPTION_COUNTUP\nare not impacted because ...\").\n\n> Signed-off-by: Pranit Bauva \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\nUnless I'm mistaken, the previous version was v9[3], not v1.\n\n> Changes introduced:\n> Use a different language in commit message to make the change and its\n> utility more clear. Also added some points as to where this patch could\n> break but it doesn't.\n\nThis version forgets to add the new tests to t0040-parse-options.sh\nwhich SZEDER requested[4] to ensure that the new behavior works as\nexpected.\n\n> ---\n> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n> @@ -144,8 +144,12 @@ There are some macros to easily define options:\n>\n> `OPT_COUNTUP(short, long, &int_var, description)`::\n> Introduce a count-up option.\n> - `int_var` is incremented on each use of `--option`, and\n> - reset to zero with `--no-option`.\n> + Each use of `--option` increments `int_var`, starting from zero\n> + (even if initially negative), and `--no-option` resets it to\n> + zero. To determine if `--option` or `--no-option` was set at\n> + all, set `int_var` to a negative value, and if it is still\n\nI realize that you copied this text verbatim from the example I\ngave[5], but in retrospect, I think s/set/initialize/ would be a bit\nmore clear:\n\n all, initialize `int_var` to a negative value, ...\n\n> + negative after parse_options(), then neither `--option` nor\n> + `--no-option` was seen.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289264/\n[2]: http://article.gmane.org/gmane.comp.version-control.git/289757/\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289724\n[4]: http://article.gmane.org/gmane.comp.version-control.git/289733\n[5]: http://article.gmane.org/gmane.comp.version-control.git/289822\n"},{"id":"281916","messageId":"CAPig+cRQ5hOLi3539Emq=YexHRXqdesf+Yo2cY+NkiPO_9Vhyg@mail.gmail.com","threadId":"41835","inReplyTo":"01020153b478cf9b-1417ac08-005c-4d72-91f1-0b07f00a24d5-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T03:07:18Z","receivedAt":"2016-03-27T03:07:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n> So that we can see how many diffs were contained in the message and use\n> them in individual tests where ever it is required. Also use\n> write_script() to create the fake \"editor\".\n\nIt is important to explain *why* you want to be able to count how many\ndiffs were in the editor message. In particular, you will be adding\nnew tests in a subsequent patch which will expect a specific number of\ndiffs (rather than any number of diffs)\n\nAlso, you need to explain why you're changing the existing tests to\ncount the number of diffs. Is it simply \"because you can\" or is it\nbecause you suspect that a change you're making in a subsequent patch\nmight accidentally cause too many diffs to show up in the existing\ntest cases?\n\n> Signed-off-by: Pranit Bauva \n> ---\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> @@ -3,9 +3,11 @@\n> -cat >check-for-diff < -#!$SHELL_PATH\n> -exec grep '^diff --git' \"\\$1\"\n> +write_script \"check-for-diff\" <<'EOF' &&\n\nThe 'EOF' is more commonly written as \\EOF in Git test scripts.\n\n> +! test -s out ||\n> +rm out &&\n\nWhy not just \"rm -f out\"? But, more importantly, why do you need to\nremove the file at all? The '>' redirection operator (used below) will\noverwrite the file, so no need to remove it beforehand.\n\n> +! grep '^diff --git' \"$1\" ||\n> +grep '^diff --git' \"$1\" >out\n\nUm, what? Why two greps? I would have expected you to simply re-use\nthe existing grep (minus the backslash) while adding the redirection:\n\n -exec grep '^diff --git' \"\\$1\"\n +exec grep '^diff --git' \"$1\" >out\n\nAm I missing something obvious?\n\n> EOF\n> chmod +x check-for-diff\n\nMentioned previously[1]: Drop the 'chmod' since write_script() does it for you.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289832\n\n> test_set_editor \"$PWD/check-for-diff\"\n> @@ -23,7 +25,8 @@ test_expect_success 'setup' '\n> test_expect_success 'initial commit shows verbose diff' '\n> - git commit --amend -v\n> + git commit --amend -v &&\n> + test_line_count = 1 out\n> '\n>\n> test_expect_success 'second commit' '\n> @@ -39,13 +42,15 @@ check_message() {\n>\n> test_expect_success 'verbose diff is stripped out' '\n> git commit --amend -v &&\n> - check_message message\n> + check_message message &&\n> + test_line_count = 1 out\n> '\n>\n> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n> git config diff.mnemonicprefix true &&\n> git commit --amend -v &&\n> - check_message message\n> + check_message message &&\n> + test_line_count = 1 out\n> '\n"},{"id":"281917","messageId":"CAPig+cTBxH7BAs2M+PBwTHfVJvH0ADL1OSEKdHQY59L-WiYosA@mail.gmail.com","threadId":"41835","inReplyTo":"01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T03:34:49Z","receivedAt":"2016-03-27T03:34:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n> Add commit.verbose configuration variable as a convenience for those\n> who always prefer --verbose taking care of multiple levels of verbosity.\n\nWhat does \"taking care of multiple levels of verbosity\" mean? I\nsuppose you mean that commit.verbose specifies a verbosity level\n(rather than being merely boolean), but that phrase is difficult to\ndecipher. And, since it's obvious from the patch itself that verbosity\nis not a mere boolean, there isn't really a need to mention it here.\nThe commit message would be perfectly fine without that bit, so\nperhaps just drop it.\n\n> Signed-off-by: Pranit Bauva \n>\n> ---\n> Changes with respect to the previous patch:\n> - Fixed a status related bug pointed out by SZEDER\n> - Change some tests\n\nPlease help reviewers out by being a bit more verbose when describing\nthe changes. For instance, what bug did you fix pointed out by SZEDER?\nAnd, \"change some tests\" says nothing useful. What did you change in\nthe tests?\n\n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> finalize_colopts(&s.colopts, -1);\n> finalize_deferred_config(&s);\n>\n> + if (verbose == -1)\n> + verbose = 0;\n> +\n\nNit: It might be good to drop the blank line above this new\nconditional to make it clear that it is part of the\ninit_config/parse_options processing (the tail of which is visible in\nthe context above).\n\n> handle_untracked_files_arg(&s);\n> if (show_ignored_in_status)\n> s.show_ignored_files = 1;\n> @@ -1654,6 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> argc = parse_and_validate_options(argc, argv, builtin_commit_options,\n> builtin_commit_usage,\n> prefix, current_head, &s);\n> +\n> + if (verbose == -1)\n> + verbose = (config_verbose == -1) ? 0 : config_verbose;\n> +\n\nNit: For consistency, dropping the blank line above this new\nconditional wouldn't hurt either.\n\n> if (dry_run)\n> return dry_run_commit(argc, argv, prefix, current_head, &s);\n> index_file = prepare_index(argc, argv, prefix, current_head, 0);\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n> +test_expect_success 'commit.verbose true and --verbose omitted' '\n> + echo content >file2 &&\n> + echo content >>file &&\n> + git add file2 &&\n> + git -c commit.verbose=true commit -F message &&\n> + test_line_count = 1 out\n> +'\n\nWhy is this test so utterly different than it was in v9 (even though\nthe title is the same), and why is it so different from other tests\nbelow?\n\nMore below...\n\n> +test_expect_success 'commit.verbose true and --verbose' '\n> + git -c commit.verbose=true commit --amend --verbose &&\n> + test_line_count = 1 out\n> +'\n> +\n> +test_expect_success 'commit.verbose true and -v -v' '\n> + git -c commit.verbose=true commit --amend -v -v &&\n> + test_line_count = 2 out\n> +'\n> +\n> +test_expect_success 'commit.verbose true and --no-verbose' '\n> + git -c commit.verbose=true commit --amend --no-verbose &&\n> + ! test -s out\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --verbose' '\n> + git -c commit.verbose=false commit --amend --verbose &&\n> + test_line_count = 1 out\n> +'\n> +\n> +test_expect_success 'commit.verbose false and -v -v' '\n> + git -c commit.verbose=false commit --amend -v -v &&\n> + test_line_count = 2 out\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --verbose omitted' '\n> + git -c commit.verbose=false commit --amend &&\n> + ! test -s out\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --no-verbose' '\n> + git -c commit.verbose=false commit --amend --no-verbose &&\n> + ! test -s out\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\nThe fact that v9 broke a number of tests in other scripts which use\ngit-status (as SZEDER pointed out[1]), due to initializing 'verbose'\nto -1 in builtin/commit.c, suggests that you probably ought to add\nanother test here to protect against that sort of breakage, as well.\nSpecifically, git-status should remain non-verbose when neither\ncommit.verbose nor --verbose is specified.\n\n> +\n> test_done\n"},{"id":"281919","messageId":"CAFZEwPNGVihCctwq+HGnEsAitn7J6jwm7mm7bOQEuhrENKzuuw@mail.gmail.com","threadId":"41835","inReplyTo":"CAPig+cTB+xZmwALTY2Oy2_dVXknSEjaBiQ8NNr-qOVOZKjHvXw@mail.gmail.com","subject":"Re: [PATCH v10 1/3] parse-options.c: make OPTION__COUNTUP understand \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-27T06:10:49Z","receivedAt":"2016-03-27T06:10:49Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 27, 2016 at 8:15 AM, Eric Sunshine wrote:\n> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n>> parse-options.c: make OPTION__COUNTUP understand \"unspecified\" values\n>\n> A bit clearer: s/understand/respect/\n> Also: s/__/_/\n\nSure.\n\n>> The reason to make it understand negative values or more specifically\n>> \"unspecified\" values is to give the ability to differentiate whether\n>> `--option` or `--no-option` was specified at all.\n>\n> The word \"negative\" shows up far too early in this paragraph, even\n> before \"unspecified\". It also fails utterly to explain what\n> \"unspecified\" means and how to use it. It does vaguely explain why\n> respecting \"unspecified\" is desirable (to know if --[no-]option was\n> seen), but otherwise leaves the reader quite clueless.\n>\n>> Many uses of COUNTUP have now been replaced with BOOL and what remains\n>> are verbose/quiet/force. This change will not affect existing users of\n>> COUNTUP at all, as long as they use the initial value of 0 (or more), as\n>> there is no mechanism to decrement. The only thing the command line can\n>> do is to reset it to zero with \"--no-foo\".\n>\n> Copying this paragraph verbatim from Junio's email[1] isn't\n> necessarily the best way to compose a commit message. Junio was\n> \"thinking out loud\" while justifying the change to himself as being\n> safe, but you ought to reframe this reasoning into a more concise form\n> which flows with the rest of the commit message, keeping the important\n> bits and dropping others.\n\nI will drop off some initial parts.\n\n>> Verbose or quiet don't use negative values before this commit but force\n>> uses it in a different way to handle its config and then munges the \"-1\"\n>> into 0 before we get to parse_options.\n>\n> Presumably, you're talking about a very specific instance of -1 in\n> conjunction with OPT__FORCE in builtin/clean.c, but that information\n> is entirely missing from this paragraph, thus this explanation serves\n> only to confuse rather than clarify. It is a good idea to cite this\n> specific unusual case when justifying that this change is safe, but it\n> must be accompanied by sufficient context for the reader to understand\n> what is being said.\n\nI will mention about OPT__FORCE in builtin/clean.c clearly.\n\n>> There are uses of COUNTUP in cmd_hash_object() and test-parse-options.c\n>> and they are both fine.\n>\n> What does \"fine\" mean? I know from reading the code that \"fine\" means\n> that these clients initialize the values to 0, thus should see no\n> behavioral difference. But why are these two cases called out\n> specially when they are already covered by the above \"as long as they\n> use initial value of 0\" explanation? As a reader, having these two\n> cases mentioned specially makes me wonder if I'm missing something\n> non-obvious about them.\n\nIts good to drop off this paragraph\n\n> It would probably be better to avoid mentioning cmd_hash_object() and\n> test-parse-options.c at all, and instead explain generally that, with\n> one exception (builtin/clean.c), all current clients of OPTION_COUNTUP\n> use an initial value of zero, thus are not impacted by this change.\n> And, then go on to explain builtin/clean.c's special case and why it\n> is safe, as well.\n>\n>\n> Sorry for seeming to be very picky, but if I hadn't already been\n> following this topic (and hadn't in fact suggested this particular\n> change), as a reader, I think I would find this commit message utterly\n> baffling, and wouldn't have a clue what this change is about or why it\n> is desirable.\n>\n> Perhaps it would be a good idea to re-read \"(2) Describe your changes\n> well\" in Documentation/SubmittingPatches for hints about writing a\n> good commit message, as well as Junio's recent re-stating[2] of those\n> hints.\n\nI don't disagree to your point and it is a bit difficult for me to\nexplain stuff to other people especially when I know what's going on\nbut other might not.\n\n> Try to write the commit message as if you were speaking to someone who\n> wasn't already familiar with the issue you're trying to solve.\n> Specifically, explain the problem (\"no way to distinguish between\n> --[no-]option being seen or not\"); explain the solution (\"introduce an\n> 'unspecified' value\"); the implementation of the \"unspecified\" value\n> (\"any negative number\" plus an example of how to make use of it);\n> justify that the change is safe (\"existing clients of OPTION_COUNTUP\n> are not impacted because ...\").\n\nI will try and do this in the commit message.\n\n>> Signed-off-by: Pranit Bauva \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> Unless I'm mistaken, the previous version was v9[3], not v1.\n>\n>> Changes introduced:\n>> Use a different language in commit message to make the change and its\n>> utility more clear. Also added some points as to where this patch could\n>> break but it doesn't.\n>\n> This version forgets to add the new tests to t0040-parse-options.sh\n> which SZEDER requested[4] to ensure that the new behavior works as\n> expected.\n>\n>> ---\n>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\n>> @@ -144,8 +144,12 @@ There are some macros to easily define options:\n>>\n>> `OPT_COUNTUP(short, long, &int_var, description)`::\n>> Introduce a count-up option.\n>> - `int_var` is incremented on each use of `--option`, and\n>> - reset to zero with `--no-option`.\n>> + Each use of `--option` increments `int_var`, starting from zero\n>> + (even if initially negative), and `--no-option` resets it to\n>> + zero. To determine if `--option` or `--no-option` was set at\n>> + all, set `int_var` to a negative value, and if it is still\n>\n> I realize that you copied this text verbatim from the example I\n> gave[5], but in retrospect, I think s/set/initialize/ would be a bit\n> more clear:\n>\n> all, initialize `int_var` to a negative value, ...\n>\n>> + negative after parse_options(), then neither `--option` nor\n>> + `--no-option` was seen.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/289264/\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/289757/\n> [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289724\n> [4]: http://article.gmane.org/gmane.comp.version-control.git/289733\n> [5]: http://article.gmane.org/gmane.comp.version-control.git/289822\n"},{"id":"281920","messageId":"CAFZEwPPNMtopk9a3nVaCuujWoyeA5+39vEj3Mct1YckCXLh1-A@mail.gmail.com","threadId":"41835","inReplyTo":"CAPig+cTBxH7BAs2M+PBwTHfVJvH0ADL1OSEKdHQY59L-WiYosA@mail.gmail.com","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-27T07:00:02Z","receivedAt":"2016-03-27T07:00:02Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine wrote:\n> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n>> Add commit.verbose configuration variable as a convenience for those\n>> who always prefer --verbose taking care of multiple levels of verbosity.\n>\n> What does \"taking care of multiple levels of verbosity\" mean? I\n> suppose you mean that commit.verbose specifies a verbosity level\n> (rather than being merely boolean), but that phrase is difficult to\n> decipher. And, since it's obvious from the patch itself that verbosity\n> is not a mere boolean, there isn't really a need to mention it here.\n> The commit message would be perfectly fine without that bit, so\n> perhaps just drop it.\n\nI will drop it.\n\n>> Signed-off-by: Pranit Bauva \n>>\n>> ---\n>> Changes with respect to the previous patch:\n>> - Fixed a status related bug pointed out by SZEDER\n>> - Change some tests\n>\n> Please help reviewers out by being a bit more verbose when describing\n> the changes. For instance, what bug did you fix pointed out by SZEDER?\n> And, \"change some tests\" says nothing useful. What did you change in\n> the tests?\n\nI will try and be more specific henceforth.\n\n>> ---\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> @@ -1355,6 +1357,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n>> finalize_colopts(&s.colopts, -1);\n>> finalize_deferred_config(&s);\n>>\n>> + if (verbose == -1)\n>> + verbose = 0;\n>> +\n>\n> Nit: It might be good to drop the blank line above this new\n> conditional to make it clear that it is part of the\n> init_config/parse_options processing (the tail of which is visible in\n> the context above).\n\nSure.\n\n>> handle_untracked_files_arg(&s);\n>> if (show_ignored_in_status)\n>> s.show_ignored_files = 1;\n>> @@ -1654,6 +1664,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>> argc = parse_and_validate_options(argc, argv, builtin_commit_options,\n>> builtin_commit_usage,\n>> prefix, current_head, &s);\n>> +\n>> + if (verbose == -1)\n>> + verbose = (config_verbose == -1) ? 0 : config_verbose;\n>> +\n>\n> Nit: For consistency, dropping the blank line above this new\n> conditional wouldn't hurt either.\n>\n>> if (dry_run)\n>> return dry_run_commit(argc, argv, prefix, current_head, &s);\n>> index_file = prepare_index(argc, argv, prefix, current_head, 0);\n>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>> @@ -101,4 +101,52 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n>> +test_expect_success 'commit.verbose true and --verbose omitted' '\n>> + echo content >file2 &&\n>> + echo content >>file &&\n>> + git add file2 &&\n>> + git -c commit.verbose=true commit -F message &&\n>> + test_line_count = 1 out\n>> +'\n>\n> Why is this test so utterly different than it was in v9 (even though\n> the title is the same), and why is it so different from other tests\n> below?\n\nThis is because the \"editor\" in v9 checked for \"# Changes\"... While\nthis \"editor\" checks for 'diff --git'. And submodules don't give a\nproper diff to verify (I tried this out and noticed this behavior by\ntweaking some parts). In fact submodules don't give diff at all. But\nthey do give \"# Changes\"... So its important to setup up a little\nbefore getting started. If this seems unnecessary, then should I move\nall the tests which were introduced here above the submodule test?\n\n> More below...\n>\n>> +test_expect_success 'commit.verbose true and --verbose' '\n>> + git -c commit.verbose=true commit --amend --verbose &&\n>> + test_line_count = 1 out\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose true and -v -v' '\n>> + git -c commit.verbose=true commit --amend -v -v &&\n>> + test_line_count = 2 out\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose true and --no-verbose' '\n>> + git -c commit.verbose=true commit --amend --no-verbose &&\n>> + ! test -s out\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and --verbose' '\n>> + git -c commit.verbose=false commit --amend --verbose &&\n>> + test_line_count = 1 out\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and -v -v' '\n>> + git -c commit.verbose=false commit --amend -v -v &&\n>> + test_line_count = 2 out\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and --verbose omitted' '\n>> + git -c commit.verbose=false commit --amend &&\n>> + ! test -s out\n>> +'\n>> +\n>> +test_expect_success 'commit.verbose false and --no-verbose' '\n>> + git -c commit.verbose=false commit --amend --no-verbose &&\n>> + ! test -s out\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> The fact that v9 broke a number of tests in other scripts which use\n> git-status (as SZEDER pointed out[1]), due to initializing 'verbose'\n> to -1 in builtin/commit.c, suggests that you probably ought to add\n> another test here to protect against that sort of breakage, as well.\n> Specifically, git-status should remain non-verbose when neither\n> commit.verbose nor --verbose is specified.\n\nYes, I should. I will include that.\n>> +\n>> test_done\n"},{"id":"293766","messageId":"CAPig+cStkyOE4MwmFpq9V8h5M=6j=FWbDTkyMGXZsf8sBH=9pg@mail.gmail.com","threadId":"41835","inReplyTo":"CAFZEwPPNMtopk9a3nVaCuujWoyeA5+39vEj3Mct1YckCXLh1-A@mail.gmail.com","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T08:17:31Z","receivedAt":"2016-03-27T08:17:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 27, 2016 at 3:00 AM, Pranit Bauva wrote:\n> On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine wrote:\n>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n>>> +test_expect_success 'commit.verbose true and --verbose omitted' '\n>>> + echo content >file2 &&\n>>> + echo content >>file &&\n>>> + git add file2 &&\n>>> + git -c commit.verbose=true commit -F message &&\n>>> + test_line_count = 1 out\n>>> +'\n>>\n>> Why is this test so utterly different than it was in v9 (even though\n>> the title is the same), and why is it so different from other tests\n>> below?\n>\n> This is because the \"editor\" in v9 checked for \"# Changes\"... While\n> this \"editor\" checks for 'diff --git'. And submodules don't give a\n> proper diff to verify (I tried this out and noticed this behavior by\n> tweaking some parts). In fact submodules don't give diff at all. But\n> they do give \"# Changes\"... So its important to setup up a little\n> before getting started. If this seems unnecessary, then should I move\n> all the tests which were introduced here above the submodule test?\n\nLet's ignore submodules when discussing this since they don't need to\nfactor into the issue. What you are actually saying (and what took me\na while to understand due to the \"submodules\" misdirection) is that\nyou need to do some additional setup to test the \"-v -v\" cases. In\nparticular, you need to introduce some change to the worktree which is\nnot in the index.\n\nThe typical way to satisfy this requirement (which doesn't require\nrelocating tests) is to add a \"setup\" test before the tests which\ndepend upon that additional setup, rather than adding that setup to\nthe first test which needs it. Just about the simplest setup test\nwhich satisfies your needs is the following (inserted just before the\nfirst of the new tests):\n\n test_expect_success 'setup -v -v' '\n echo dirty >file\n '\n\nAnd, then you can restore the \"commit.verbose true and --verbose\nomitted\" test to its simple form:\n\n test_expect_success 'commit.verbose true and --verbose omitted' '\n git -c commit.verbose=true commit --amend &&\n test_line_count = 1 out\n '\n\n\nBy the way, now that commit.verbose is no longer a mere boolean,\nyou're going to need some additional tests beyond the\ncommit.verbose={true,false} ones you've already added. In particular,\nyou should be testing commit.verbose with several numeric values to\nverify that it works as expected. For instance:\n\n commit.verbose=-2\n commit.verbose=-1\n commit.verbose=0\n commit.verbose=1\n commit.verbose=2\n commit.verbose=3\n\nThe -2 case is interesting; I'm pretty sure the current implementation\nof this patch will misbehave since the only negative value it's\n"},{"id":"281924","messageId":"CAFZEwPMSpJGgTeHm41FYXiXgx7whFT0BsqUEBJ2ZJOsfydKxVw@mail.gmail.com","threadId":"41835","inReplyTo":"CAPig+cStkyOE4MwmFpq9V8h5M=6j=FWbDTkyMGXZsf8sBH=9pg@mail.gmail.com","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-27T08:40:19Z","receivedAt":"2016-03-27T08:40:19Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 27, 2016 at 1:47 PM, Eric Sunshine wrote:\n> On Sun, Mar 27, 2016 at 3:00 AM, Pranit Bauva wrote:\n>> On Sun, Mar 27, 2016 at 9:04 AM, Eric Sunshine wrote:\n>>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n>>>> +test_expect_success 'commit.verbose true and --verbose omitted' '\n>>>> + echo content >file2 &&\n>>>> + echo content >>file &&\n>>>> + git add file2 &&\n>>>> + git -c commit.verbose=true commit -F message &&\n>>>> + test_line_count = 1 out\n>>>> +'\n>>>\n>>> Why is this test so utterly different than it was in v9 (even though\n>>> the title is the same), and why is it so different from other tests\n>>> below?\n>>\n>> This is because the \"editor\" in v9 checked for \"# Changes\"... While\n>> this \"editor\" checks for 'diff --git'. And submodules don't give a\n>> proper diff to verify (I tried this out and noticed this behavior by\n>> tweaking some parts). In fact submodules don't give diff at all. But\n>> they do give \"# Changes\"... So its important to setup up a little\n>> before getting started. If this seems unnecessary, then should I move\n>> all the tests which were introduced here above the submodule test?\n>\n> Let's ignore submodules when discussing this since they don't need to\n> factor into the issue. What you are actually saying (and what took me\n> a while to understand due to the \"submodules\" misdirection) is that\n> you need to do some additional setup to test the \"-v -v\" cases. In\n> particular, you need to introduce some change to the worktree which is\n> not in the index.\n\nSorry for the misdirection. And you understood correctly. I do need to\nintroduce some changes in worktree.\n\n> The typical way to satisfy this requirement (which doesn't require\n> relocating tests) is to add a \"setup\" test before the tests which\n> depend upon that additional setup, rather than adding that setup to\n> the first test which needs it. Just about the simplest setup test\n> which satisfies your needs is the following (inserted just before the\n> first of the new tests):\n>\n> test_expect_success 'setup -v -v' '\n> echo dirty >file\n> '\n>\n> And, then you can restore the \"commit.verbose true and --verbose\n> omitted\" test to its simple form:\n>\n> test_expect_success 'commit.verbose true and --verbose omitted' '\n> git -c commit.verbose=true commit --amend &&\n> test_line_count = 1 out\n> '\n\nHaving a additional setup test seems a nice way to go about.\n\n\n> By the way, now that commit.verbose is no longer a mere boolean,\n> you're going to need some additional tests beyond the\n> commit.verbose={true,false} ones you've already added. In particular,\n> you should be testing commit.verbose with several numeric values to\n> verify that it works as expected. For instance:\n>\n> commit.verbose=-2\n> commit.verbose=-1\n> commit.verbose=0\n> commit.verbose=1\n> commit.verbose=2\n> commit.verbose=3\n>\n> The -2 case is interesting; I'm pretty sure the current implementation\n> of this patch will misbehave since the only negative value it's\n> expecting 'config_verbose' to be is -1.\n\n-2 case will fail. I should probably expect 'config_verbose' to be\nnegative instead.\n"},{"id":"281925","messageId":"1459079497-19121-1-git-send-email-szeder@ira.uka.de","threadId":"41835","inReplyTo":"01020153b478cfab-5309f834-7617-4a7f-a3e9-93f0dcf4a6be-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-27T11:51:37Z","receivedAt":"2016-03-27T11:51:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> +test_expect_success 'commit.verbose true and --no-verbose' '\n> +\tgit -c commit.verbose=true commit --amend --no-verbose &&\n> +\t! test -s out\n\nPlease use the test_must_be_empty helper instead, because it has a\nnice, human-readable name and it complains with a helpful error\nmessage if something goes wrong, whereas 'test -s' just fails\nsilently.\n\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --verbose' '\n> +\tgit -c commit.verbose=false commit --amend --verbose &&\n> +\ttest_line_count = 1 out\n> +'\n> +\n> +test_expect_success 'commit.verbose false and -v -v' '\n> +\tgit -c commit.verbose=false commit --amend -v -v &&\n> +\ttest_line_count = 2 out\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --verbose omitted' '\n> +\tgit -c commit.verbose=false commit --amend &&\n> +\t! test -s out\n> +'\n> +\n> +test_expect_success 'commit.verbose false and --no-verbose' '\n> +\tgit -c commit.verbose=false commit --amend --no-verbose &&\n> +\t! test -s out\n> +'\n> +\n> +test_expect_success 'status ignores commit.verbose=true' '\n> +\tgit -c commit.verbose=true status >actual &&\n> +\t! grep \"^diff --git\" actual\n> +'\n> +\n> test_done\n> \n> --\n> https://github.com/git/git/pull/218\n> \n> \n"},{"id":"281926","messageId":"CAFZEwPOhTC_LjUPHMN0PJsc94gvmHgmQL2Xe63oRGOiEMR2jjQ@mail.gmail.com","threadId":"41835","inReplyTo":"1459079497-19121-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-27T11:59:47Z","receivedAt":"2016-03-27T11:59:47Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor wrote:\n>> +test_expect_success 'commit.verbose true and --no-verbose' '\n>> + git -c commit.verbose=true commit --amend --no-verbose &&\n>> + ! test -s out\n>\n> Please use the test_must_be_empty helper instead, because it has a\n> nice, human-readable name and it complains with a helpful error\n> message if something goes wrong, whereas 'test -s' just fails\n> silently.\n\nThanks for pointing it out. I was unsure whether 'test -s' is a good\nchoice but used it since I did not know any other alternative.\n"},{"id":"281927","messageId":"20160327140733.Horde.hQiEcpos_hNprvjPS1zIbi2@webmail.informatik.kit.edu","threadId":"41835","inReplyTo":"CAFZEwPOhTC_LjUPHMN0PJsc94gvmHgmQL2Xe63oRGOiEMR2jjQ@mail.gmail.com","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-27T12:07:33Z","receivedAt":"2016-03-27T12:07:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Pranit Bauva :\n\n> On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor wrote:\n>>> +test_expect_success 'commit.verbose true and --no-verbose' '\n>>> + git -c commit.verbose=true commit --amend --no-verbose &&\n>>> + ! test -s out\n>>\n>> Please use the test_must_be_empty helper instead, because it has a\n>> nice, human-readable name and it complains with a helpful error\n>> message if something goes wrong, whereas 'test -s' just fails\n>> silently.\n>\n> Thanks for pointing it out. I was unsure whether 'test -s' is a good\n> choice but used it since I did not know any other alternative.\n\nt/test-lib-functions.sh contains all our test helpers functions, that's\nwhere you can look for a suitable helper, should it be necessary.\n"},{"id":"281928","messageId":"CAFZEwPPwz5Woz-TXiDQz43VChueDGqzOPB0VQQiefH7igWNiQQ@mail.gmail.com","threadId":"41835","inReplyTo":"20160327140733.Horde.hQiEcpos_hNprvjPS1zIbi2@webmail.informatik.kit.edu","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-27T12:11:48Z","receivedAt":"2016-03-27T12:11:48Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 27, 2016 at 5:37 PM, SZEDER Gábor wrote:\n> t/test-lib-functions.sh contains all our test helpers functions, that's\n> where you can look for a suitable helper, should it be necessary.\nThanks. I will surely look into it.\n"},{"id":"281929","messageId":"1459085245-20658-1-git-send-email-szeder@ira.uka.de","threadId":"41835","inReplyTo":"CAPig+cRQ5hOLi3539Emq=YexHRXqdesf+Yo2cY+NkiPO_9Vhyg@mail.gmail.com","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-03-27T13:27:25Z","receivedAt":"2016-03-27T13:27:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> > +! test -s out ||\n> > +rm out &&\n> \n> Why not just \"rm -f out\"? But, more importantly, why do you need to\n> remove the file at all? The '>' redirection operator (used below) will\n> overwrite the file, so no need to remove it beforehand.\n> \n> > +! grep '^diff --git' \"$1\" ||\n> > +grep '^diff --git' \"$1\" >out\n> \n> Um, what? Why two greps? I would have expected you to simply re-use\n> the existing grep (minus the backslash) while adding the redirection:\n> \n> -exec grep '^diff --git' \"\\$1\"\n> +exec grep '^diff --git' \"$1\" >out\n> \n> Am I missing something obvious?\n\nIn the non-verbose cases no diff is included in the commit message\ntemplate, thus the pattern looking for it doesn't match anything, grep\nexits with error code, which in turn becomes the editor's exit\ncode, ultimately making 'git commit' fail. Not good.\n\nI suppose both the explicit 'rm out' and the '! grep ... || ...' is\nthere to deal with this situation.\n\nHmph.\n\nI think we could:\n\n - either revive the idea of two editor scripts: one for the\n non-verbose case checking with '! grep ...' that there are no\n diffs in the commit message template, and one for all verbose\n cases storing those diff lines in a file to be counted later.\n\n - or use a fake editor that merely copies the whole commit message\n template to a separate file, and we do the greping in the tests\n themselves as well.\n\n - or simply stick a 'true' at the end of the editor script ensuring\n that it returns success even when grep can't find the pattern, but\n I kind of feel ashamed of myself for even mentioning this\n possibility ;)\n\nI would go for the second possibility, but don't feel strong about it.\n"},{"id":"281930","messageId":"CAFZEwPN_sL7_yj=FcHynK2L5DhuSD5XSGHnGk73yAcH=+MdY3w@mail.gmail.com","threadId":"41835","inReplyTo":"1459085245-20658-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-27T13:43:41Z","receivedAt":"2016-03-27T13:43:41Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 27, 2016 at 6:57 PM, SZEDER Gábor wrote:\n>> > +! test -s out ||\n>> > +rm out &&\n>>\n>> Why not just \"rm -f out\"? But, more importantly, why do you need to\n>> remove the file at all? The '>' redirection operator (used below) will\n>> overwrite the file, so no need to remove it beforehand.\n>>\n>> > +! grep '^diff --git' \"$1\" ||\n>> > +grep '^diff --git' \"$1\" >out\n>>\n>> Um, what? Why two greps? I would have expected you to simply re-use\n>> the existing grep (minus the backslash) while adding the redirection:\n>>\n>> -exec grep '^diff --git' \"\\$1\"\n>> +exec grep '^diff --git' \"$1\" >out\n>>\n>> Am I missing something obvious?\n>\n> In the non-verbose cases no diff is included in the commit message\n> template, thus the pattern looking for it doesn't match anything, grep\n> exits with error code, which in turn becomes the editor's exit\n> code, ultimately making 'git commit' fail. Not good.\n>\n> I suppose both the explicit 'rm out' and the '! grep ... || ...' is\n> there to deal with this situation.\n\nYes. In fact, I did this as a last resort after trying a lot of other\nstuff which didn't work.\n\n> Hmph.\n>\n> I think we could:\n>\n> - either revive the idea of two editor scripts: one for the\n> non-verbose case checking with '! grep ...' that there are no\n> diffs in the commit message template, and one for all verbose\n> cases storing those diff lines in a file to be counted later.\n>\n> - or use a fake editor that merely copies the whole commit message\n> template to a separate file, and we do the greping in the tests\n> themselves as well.\n>\n> - or simply stick a 'true' at the end of the editor script ensuring\n> that it returns success even when grep can't find the pattern, but\n> I kind of feel ashamed of myself for even mentioning this\n> possibility ;)\n>\n> I would go for the second possibility, but don't feel strong about it.\n\nThere is one more possibility, that we could use 'test_must_fail' (as\ngrep exits with error code with no diff) but this will send a very\nwrong interpretation as \"This type of scenario is not meant to work\".\nOr to put it in better words, \"You cannot have no diff with commit\".\nThere is a difference between two negatives and one positive.\n"},{"id":"281937","messageId":"CAPig+cRU+JJabUdNTVDtZsrmWgn0TWetxhCFpyR+=5LK-w-7XA@mail.gmail.com","threadId":"41835","inReplyTo":"CAFZEwPOhTC_LjUPHMN0PJsc94gvmHgmQL2Xe63oRGOiEMR2jjQ@mail.gmail.com","subject":"Re: [PATCH v10 3/3] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T16:48:05Z","receivedAt":"2016-03-27T16:48:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 27, 2016 at 7:59 AM, Pranit Bauva wrote:\n> On Sun, Mar 27, 2016 at 5:21 PM, SZEDER Gábor wrote:\n>>> +test_expect_success 'commit.verbose true and --no-verbose' '\n>>> + git -c commit.verbose=true commit --amend --no-verbose &&\n>>> + ! test -s out\n>>\n>> Please use the test_must_be_empty helper instead, because it has a\n>> nice, human-readable name and it complains with a helpful error\n>> message if something goes wrong, whereas 'test -s' just fails\n>> silently.\n>\n> Thanks for pointing it out. I was unsure whether 'test -s' is a good\n> choice but used it since I did not know any other alternative.\n\nFor these particular tests, it would be more stylized to use:\n\n test_line_count = 0 out\n\nwhich would keep them consistent with other tests in the same file which use:\n\n test_line_count = 1 out\n ...\n test_line_count = 2 out\n\nhowever, that's a minor point.\n"},{"id":"281938","messageId":"CAPig+cQucLoednXHatos0yF66JGN+njhLarK=WYam=qtwV5y=Q@mail.gmail.com","threadId":"41835","inReplyTo":"CAFZEwPMaZkUi+DvohhVrc_dW_8cdfJsZX-YA_SRWDp021UvDLQ@mail.gmail.com","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T17:03:53Z","receivedAt":"2016-03-27T17:03:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[forwarding this to the list since Pranit (presumably) accidentally\nreplied only to me but it's relevant to the ongoing discussion]\n\nOn Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva wrote:\n> On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine wrote:\n>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n>>> So that we can see how many diffs were contained in the message and use\n>>> them in individual tests where ever it is required. Also use\n>>> write_script() to create the fake \"editor\".\n>>\n>> It is important to explain *why* you want to be able to count how many\n>> diffs were in the editor message. In particular, you will be adding\n>> new tests in a subsequent patch which will expect a specific number of\n>> diffs (rather than any number of diffs)\n>>\n>> Also, you need to explain why you're changing the existing tests to\n>> count the number of diffs. Is it simply \"because you can\" or is it\n>> because you suspect that a change you're making in a subsequent patch\n>> might accidentally cause too many diffs to show up in the existing\n>> test cases?\n>\n> Sorry for not writing commit messages properly. It is a part I am working on.\n> How does this paragraph sound to you in addition to the previous commit message?\n> \"A subsequent commit will introduce a scenario where it is important\n> to be able to exactly determine how many diffs were present.\"\n>\n>>> Signed-off-by: Pranit Bauva \n>>> ---\n>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>>> @@ -3,9 +3,11 @@\n>>> -cat >check-for-diff <>> -#!$SHELL_PATH\n>>> -exec grep '^diff --git' \"\\$1\"\n>>> +write_script \"check-for-diff\" <<'EOF' &&\n>>\n>> The 'EOF' is more commonly written as \\EOF in Git test scripts.\n>>\n>>> +! test -s out ||\n>>> +rm out &&\n>>\n>> Why not just \"rm -f out\"? But, more importantly, why do you need to\n>> remove the file at all? The '>' redirection operator (used below) will\n>> overwrite the file, so no need to remove it beforehand.\n>\n> I wasn't aware about '-f' option. It is important to remove the file.\n> I initially tried without removing the file and it caused problems.\n> This is because let's say the previous test had 1 diff which was\n> stored in out. Now, if in the next test, it is expected to have 0\n> diffs then grep would fail to execute and nothing will be re-written\n> to out and out will contain the 1 diff from previous test. An\n> explanation for this should be mentioned in the commit message?\n>\n>>> +! grep '^diff --git' \"$1\" ||\n>>> +grep '^diff --git' \"$1\" >out\n>>\n>> Um, what? Why two greps? I would have expected you to simply re-use\n>> the existing grep (minus the backslash) while adding the redirection:\n>>\n>> -exec grep '^diff --git' \"\\$1\"\n>> +exec grep '^diff --git' \"$1\" >out\n>\n> The reason of two greps in that if in some test it fails to find any\n> diffs, the editor will return an error. Of course we could test this\n> scenario by using test_must_fail as in previous patches, but I think\n> it would be more clearer if we could test for the existence of out\n> which is done in patch 3/3. I will explanation for this in commit\n> message.\n>\n>>> EOF\n>>> chmod +x check-for-diff\n>>\n>> Mentioned previously[1]: Drop the 'chmod' since write_script() does it for you.\n>>\n>> [1]: http://article.gmane.org/gmane.comp.version-control.git/289832\n>\n> Then you mentioned that you were talking[1] about some other review.\n> So I thought you mean to keep as it is. I guess I misinterpreted it.\n> And I did not test that before. Now, I have tested that it works if we\n> remove chmod.\n> [1] : http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289832\n>\n>>> test_set_editor \"$PWD/check-for-diff\"\n>>> @@ -23,7 +25,8 @@ test_expect_success 'setup' '\n>>> test_expect_success 'initial commit shows verbose diff' '\n>>> - git commit --amend -v\n>>> + git commit --amend -v &&\n>>> + test_line_count = 1 out\n>>> '\n>>>\n>>> test_expect_success 'second commit' '\n>>> @@ -39,13 +42,15 @@ check_message() {\n>>>\n>>> test_expect_success 'verbose diff is stripped out' '\n>>> git commit --amend -v &&\n>>> - check_message message\n>>> + check_message message &&\n>>> + test_line_count = 1 out\n>>> '\n>>>\n>>> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n>>> git config diff.mnemonicprefix true &&\n>>> git commit --amend -v &&\n>>> - check_message message\n>>> + check_message message &&\n>>> + test_line_count = 1 out\n>>> '\n"},{"id":"281939","messageId":"CAPig+cQoRW3_0HPv7aUZFgAu41vkR73Z7Z+hpfrXZymEAvDFMg@mail.gmail.com","threadId":"41835","inReplyTo":"CAPig+cTFK=HPAtk7MeMQSTccmiaai++3sVn6J_pRcSi+w_4Lng@mail.gmail.com","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T17:05:20Z","receivedAt":"2016-03-27T17:05:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[also forwarding this to the list since it's relevant to the ongoing\ndiscussion, and I hadn't noticed when replying that Pranit had\n(presumably) accidentally dropped the git list as a recipient]\n\nOn Sun, Mar 27, 2016 at 3:10 AM, Eric Sunshine wrote:\n> On Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva wrote:\n>> On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine wrote:\n>>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva wrote:\n>>> It is important to explain *why* you want to be able to count how many\n>>> diffs were in the editor message. In particular, you will be adding\n>>> new tests in a subsequent patch which will expect a specific number of\n>>> diffs (rather than any number of diffs)\n>>>\n>>> Also, you need to explain why you're changing the existing tests to\n>>> count the number of diffs. Is it simply \"because you can\" or is it\n>>> because you suspect that a change you're making in a subsequent patch\n>>> might accidentally cause too many diffs to show up in the existing\n>>> test cases?\n>>\n>> Sorry for not writing commit messages properly. It is a part I am working on.\n>> How does this paragraph sound to you in addition to the previous commit message?\n>> \"A subsequent commit will introduce a scenario where it is important\n>> to be able to exactly determine how many diffs were present.\"\n>\n> That's fine for explaining why 'check-for-diff' is being updated to\n> store the output, but you still need to explain why you're changing\n> the existing tests.\n>\n>>>> -cat >check-for-diff <>>> -#!$SHELL_PATH\n>>>> -exec grep '^diff --git' \"\\$1\"\n>>>> +write_script \"check-for-diff\" <<'EOF' &&\n>>>> +! test -s out ||\n>>>> +rm out &&\n>>>\n>>> Why not just \"rm -f out\"? But, more importantly, why do you need to\n>>> remove the file at all? The '>' redirection operator (used below) will\n>>> overwrite the file, so no need to remove it beforehand.\n>>\n>> I wasn't aware about '-f' option. It is important to remove the file.\n>> I initially tried without removing the file and it caused problems.\n>> This is because let's say the previous test had 1 diff which was\n>> stored in out. Now, if in the next test, it is expected to have 0\n>> diffs then grep would fail to execute and nothing will be re-written\n>> to out and out will contain the 1 diff from previous test. An\n>> explanation for this should be mentioned in the commit message?\n>\n> No. Rather than trying to explain all this in the commit message, it\n> would be better to retain a simple implementation of 'check-for-diff'\n> rather than adding several levels of complication. More about this\n> below...\n>\n>>>> +! grep '^diff --git' \"$1\" ||\n>>>> +grep '^diff --git' \"$1\" >out\n>>>\n>>> Um, what? Why two greps? I would have expected you to simply re-use\n>>> the existing grep (minus the backslash) while adding the redirection:\n>>>\n>>> -exec grep '^diff --git' \"\\$1\"\n>>> +exec grep '^diff --git' \"$1\" >out\n>>\n>> The reason of two greps in that if in some test it fails to find any\n>> diffs, the editor will return an error. Of course we could test this\n>> scenario by using test_must_fail as in previous patches, but I think\n>> it would be more clearer if we could test for the existence of out\n>> which is done in patch 3/3. I will explanation for this in commit\n>> message.\n>\n> This sounds unnecessarily complicated. It's not too difficult to\n> reason about a script named 'check-for-diff' actually \"checking for\n> presence of diffs\" and failing if no diff is present, from which it\n> follows naturally that new tests which don't expect any diffs would\n> use test_must_fail. Keeping it simple like this also makes this patch\n> much less noisy since it doesn't require changing existing tests.\n>\n> Likewise, it keeps most of the new tests in 3/3 small because the bulk\n> of them only want to know that a diff was present, but don't care\n> about the number of diffs. However, it's not necessarily a bad thing\n> to ensure that you got the number of diffs you expected (for instance,\n> that a single -v really behaved as -v, and not as -v -v), so that can\n> be used as an argument for overhauling the old tests, as well as using\n> counting in all new tests.\n>\n> However, even if you take the approach of making 'check-for-diff'\n> succeed unconditionally and always count diffs, the current\n> implementation is still overly complicated. It would be much simpler\n> to let 'check-for-diff' always create the output file, and then use\n> \"test_line_count = 0 out\" when expecting no diffs than to sometimes\n> create the output file and sometimes not. The shell '>' operator will\n> truncate the file to zero size even before grep is invoked, so you\n> don't need to worry that results from an earlier test will pollute\n> 'out' for a subsequent test, even if grep finds no matches. Thus,\n> 'check-for-diff' collapses to this tiny implementation:\n>\n> grep '^diff --git' \"$1\" >out\n> exit 0\n>\n> Or, if you want to be terse:\n>\n> grep '^diff --git' \"$1\" || exit 0 >out\n>\n> A couple notes: First, \"out\" isn't a great name for this file; perhaps\n> come up with a better name. Second, under this scenario,\n> \"check-for-diff\" isn't the most accurate name; it's now simply\n> collecting diffs rather than checking for presence.\n>\n> An alternative would be for the output file to contain the actual\n> count of diffs, rather than the diff lines themselves. For instance:\n>\n> write_script count-diffs <<\\EOF &&\n> grep -c '^diff --git' \"$1\" >out\n> exit 0\n> EOF\n>\n> Whether that's actually better is subjective.\n>\n> So, what's my final word? I quite like the idea of keeping everything\n> as simple as possible, thus not altering existing tests and only doing\n> line count in the two or three new tests which actually care about the\n> number of diffs. However, I can also be sold on retrofitting existing\n> tests and having new tests do line counting as a way to improve test\n> coverage by catching cases where \"-v\" incorrectly behaves as \"-v -v\".\n> If you go that route, then the commit message of this patch needs to\n> sell it as such (an improvement in test coverage).\n"},{"id":"281940","messageId":"CAPig+cQyXi3_EwZakwrL3JahqZPFsJEifaTOB-KrZLtkxn5Oew@mail.gmail.com","threadId":"41835","inReplyTo":"CAPig+cS3usDDeTMzjqbxqi+k+XbmjawZ0TF20s9-vM6o=Fm=OQ@mail.gmail.com","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T17:08:54Z","receivedAt":"2016-03-27T17:08:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[forwarding this to the list, as well, since I again didn't notice\nwhen replying that Pranit had accidentally dropped the mailing list as\na recipient]\n\nOn Sun, Mar 27, 2016 at 12:59 PM, Eric Sunshine wrote:\n> On Sun, Mar 27, 2016 at 5:05 AM, Pranit Bauva wrote:\n>> On Sun, Mar 27, 2016 at 12:40 PM, Eric Sunshine wrote:\n>>> However, even if you take the approach of making 'check-for-diff'\n>>> succeed unconditionally and always count diffs, the current\n>>> implementation is still overly complicated. It would be much simpler\n>>> to let 'check-for-diff' always create the output file, and then use\n>>> \"test_line_count = 0 out\" when expecting no diffs than to sometimes\n>>> create the output file and sometimes not. The shell '>' operator will\n>>> truncate the file to zero size even before grep is invoked, so you\n>>> don't need to worry that results from an earlier test will pollute\n>>> 'out' for a subsequent test, even if grep finds no matches. Thus,\n>>> 'check-for-diff' collapses to this tiny implementation:\n>>>\n>>> grep '^diff --git' \"$1\" >out\n>>> exit 0\n>>>\n>>> Or, if you want to be terse:\n>>>\n>>> grep '^diff --git' \"$1\" || exit 0 >out\n>>\n>> I tried using both of this and it gives an error\n>\n> These worked fine for me when I rewrote the test script before making\n> the suggestion. You'll need to provide more information if you want to\n> get to the bottom of the problem you experienced (for instance, show\n> the exact code you used and the actual error message).\n"},{"id":"281941","messageId":"20160327172741.GA4005@flurp.local","threadId":"41835","inReplyTo":"1459085245-20658-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-27T17:27:41Z","receivedAt":"2016-03-27T17:27:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 27, 2016 at 03:27:25PM +0200, SZEDER Gábor wrote:\n> > > +! test -s out ||\n> > > +rm out &&\n> > \n> > Why not just \"rm -f out\"? But, more importantly, why do you need to\n> > remove the file at all? The '>' redirection operator (used below) will\n> > overwrite the file, so no need to remove it beforehand.\n> > \n> > > +! grep '^diff --git' \"$1\" ||\n> > > +grep '^diff --git' \"$1\" >out\n> > \n> > Um, what? Why two greps? I would have expected you to simply re-use\n> > the existing grep (minus the backslash) while adding the redirection:\n> > \n> > -exec grep '^diff --git' \"\\$1\"\n> > +exec grep '^diff --git' \"$1\" >out\n> > \n> > Am I missing something obvious?\n> \n> In the non-verbose cases no diff is included in the commit message\n> template, thus the pattern looking for it doesn't match anything, grep\n> exits with error code, which in turn becomes the editor's exit\n> code, ultimately making 'git commit' fail. Not good.\n> \n> I suppose both the explicit 'rm out' and the '! grep ... || ...' is\n> there to deal with this situation.\n\nSure, I understand that, but that's not what I meant. What I wanted\nto know was why Pranit didn't just use the simpler:\n\n grep '^diff --git' \"$1\" >out\n exit 0\n\nand then, in tests:\n\n test_line_count = n out\n\nwhere 'n' is 0, 1, or 2 as expected by the test.\n\nUnfortunately, you missed the rest of the discussion since Pranit\n(presumably) accidentally dropped the mailing list when he replied,\nand I didn't notice the omission when replying to him with the above\nsuggestion, which would have saved you the bother of going through\nthis, as well.\n\n> I think we could:\n> \n> - either revive the idea of two editor scripts: one for the\n> non-verbose case checking with '! grep ...' that there are no\n> diffs in the commit message template, and one for all verbose\n> cases storing those diff lines in a file to be counted later.\n> \n> - or use a fake editor that merely copies the whole commit message\n> template to a separate file, and we do the greping in the tests\n> themselves as well.\n> \n> - or simply stick a 'true' at the end of the editor script ensuring\n> that it returns success even when grep can't find the pattern, but\n> I kind of feel ashamed of myself for even mentioning this\n> possibility ;)\n> \n> I would go for the second possibility, but don't feel strong about it.\n\nYour #3 is effectively what I had suggested, as well (which is\nreproduced above). I had already made this change locally along with\nsome other changes I suggested in other responses, and those changes\nlook like this (atop Pranit's two patches), which isn't too bad:\n\n--- 8< ---\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 569fd8b..ea26b57 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -4,12 +4,9 @@ test_description='verbose commit template'\n . ./test-lib.sh\n \n write_script \"check-for-diff\" <<'EOF' &&\n-! test -s out ||\n-rm out &&\n-! grep '^diff --git' \"$1\" ||\n grep '^diff --git' \"$1\" >out\n+exit 0\n EOF\n-chmod +x check-for-diff\n test_set_editor \"$PWD/check-for-diff\"\n \n cat >message <<'EOF'\n@@ -101,11 +98,12 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n '\n \n+test_expect_success 'setup -v -v' '\n+\techo dirty >file\n+'\n+\n test_expect_success 'commit.verbose true and --verbose omitted' '\n-\techo content >file2 &&\n-\techo content >>file &&\n-\tgit add file2 &&\n-\tgit -c commit.verbose=true commit -F message &&\n+\tgit -c commit.verbose=true commit --amend &&\n \ttest_line_count = 1 out\n '\n \n@@ -121,7 +119,7 @@ test_expect_success 'commit.verbose true and -v -v' '\n \n test_expect_success 'commit.verbose true and --no-verbose' '\n \tgit -c commit.verbose=true commit --amend --no-verbose &&\n-\t! test -s out\n+\ttest_line_count = 0 out\n '\n \n test_expect_success 'commit.verbose false and --verbose' '\n@@ -136,12 +134,12 @@ test_expect_success 'commit.verbose false and -v -v' '\n \n test_expect_success 'commit.verbose false and --verbose omitted' '\n \tgit -c commit.verbose=false commit --amend &&\n-\t! test -s out\n+\ttest_line_count = 0 out\n '\n \n test_expect_success 'commit.verbose false and --no-verbose' '\n \tgit -c commit.verbose=false commit --amend --no-verbose &&\n-\t! test -s out\n+\ttest_line_count = 0 out\n '\n \n test_expect_success 'status ignores commit.verbose=true' '\n-- \n"},{"id":"281943","messageId":"CAFZEwPOuWGoVj1s4aqoNy6eLAKHEdcH=7=o3gPxQ_eQvpp8N8Q@mail.gmail.com","threadId":"41835","inReplyTo":"20160327172741.GA4005@flurp.local","subject":"Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-27T18:31:16Z","receivedAt":"2016-03-27T18:31:16Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Sun, Mar 27, 2016 at 10:57 PM, Eric Sunshine wrote:\n> On Sun, Mar 27, 2016 at 03:27:25PM +0200, SZEDER Gábor wrote:\n>> > > +! test -s out ||\n>> > > +rm out &&\n>> >\n>> > Why not just \"rm -f out\"? But, more importantly, why do you need to\n>> > remove the file at all? The '>' redirection operator (used below) will\n>> > overwrite the file, so no need to remove it beforehand.\n>> >\n>> > > +! grep '^diff --git' \"$1\" ||\n>> > > +grep '^diff --git' \"$1\" >out\n>> >\n>> > Um, what? Why two greps? I would have expected you to simply re-use\n>> > the existing grep (minus the backslash) while adding the redirection:\n>> >\n>> > -exec grep '^diff --git' \"\\$1\"\n>> > +exec grep '^diff --git' \"$1\" >out\n>> >\n>> > Am I missing something obvious?\n>>\n>> In the non-verbose cases no diff is included in the commit message\n>> template, thus the pattern looking for it doesn't match anything, grep\n>> exits with error code, which in turn becomes the editor's exit\n>> code, ultimately making 'git commit' fail. Not good.\n>>\n>> I suppose both the explicit 'rm out' and the '! grep ... || ...' is\n>> there to deal with this situation.\n>\n> Sure, I understand that, but that's not what I meant. What I wanted\n> to know was why Pranit didn't just use the simpler:\n>\n> grep '^diff --git' \"$1\" >out\n> exit 0\n>\n> and then, in tests:\n>\n> test_line_count = n out\n>\n> where 'n' is 0, 1, or 2 as expected by the test.\n\nSorry to create extra noise. This works perfectly fine. I had put an\n'exec' locally. But now I have tested this and it works fine..\n\n>\n> Unfortunately, you missed the rest of the discussion since Pranit\n> (presumably) accidentally dropped the mailing list when he replied,\n> and I didn't notice the omission when replying to him with the above\n> suggestion, which would have saved you the bother of going through\n> this, as well.\n\nSorry for this. I might have just started typing and forgot to click reply all.\n>> I think we could:\n>>\n>> - either revive the idea of two editor scripts: one for the\n>> non-verbose case checking with '! grep ...' that there are no\n>> diffs in the commit message template, and one for all verbose\n>> cases storing those diff lines in a file to be counted later.\n>>\n>> - or use a fake editor that merely copies the whole commit message\n>> template to a separate file, and we do the greping in the tests\n>> themselves as well.\n>>\n>> - or simply stick a 'true' at the end of the editor script ensuring\n>> that it returns success even when grep can't find the pattern, but\n>> I kind of feel ashamed of myself for even mentioning this\n>> possibility ;)\n>>\n>> I would go for the second possibility, but don't feel strong about it.\n>\n> Your #3 is effectively what I had suggested, as well (which is\n> reproduced above). I had already made this change locally along with\n> some other changes I suggested in other responses, and those changes\n> look like this (atop Pranit's two patches), which isn't too bad:\n>\n> --- 8< ---\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 569fd8b..ea26b57 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -4,12 +4,9 @@ test_description='verbose commit template'\n> . ./test-lib.sh\n>\n> write_script \"check-for-diff\" <<'EOF' &&\n> -! test -s out ||\n> -rm out &&\n> -! grep '^diff --git' \"$1\" ||\n> grep '^diff --git' \"$1\" >out\n> +exit 0\n> EOF\n> -chmod +x check-for-diff\n> test_set_editor \"$PWD/check-for-diff\"\n>\n> cat >message <<'EOF'\n> @@ -101,11 +98,12 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n> test_i18ngrep \"Aborting commit due to empty commit message.\" err\n> '\n>\n> +test_expect_success 'setup -v -v' '\n> + echo dirty >file\n> +'\n> +\n> test_expect_success 'commit.verbose true and --verbose omitted' '\n> - echo content >file2 &&\n> - echo content >>file &&\n> - git add file2 &&\n> - git -c commit.verbose=true commit -F message &&\n> + git -c commit.verbose=true commit --amend &&\n> test_line_count = 1 out\n> '\n>\n> @@ -121,7 +119,7 @@ test_expect_success 'commit.verbose true and -v -v' '\n>\n> test_expect_success 'commit.verbose true and --no-verbose' '\n> git -c commit.verbose=true commit --amend --no-verbose &&\n> - ! test -s out\n> + test_line_count = 0 out\n> '\n>\n> test_expect_success 'commit.verbose false and --verbose' '\n> @@ -136,12 +134,12 @@ test_expect_success 'commit.verbose false and -v -v' '\n>\n> test_expect_success 'commit.verbose false and --verbose omitted' '\n> git -c commit.verbose=false commit --amend &&\n> - ! test -s out\n> + test_line_count = 0 out\n> '\n>\n> test_expect_success 'commit.verbose false and --no-verbose' '\n> git -c commit.verbose=false commit --amend --no-verbose &&\n> - ! test -s out\n> + test_line_count = 0 out\n> '\n>\n> test_expect_success 'status ignores commit.verbose=true' '\n> --\n"},{"id":"282317","messageId":"01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com","threadId":"41835","inReplyTo":"01020153b478cf07-758c7f14-33a1-4a67-9bc9-4688de3d6742-000000@eu-west-1.amazonses.com","subject":"[PATCH v11 1/4] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T14:45:53Z","receivedAt":"2016-03-31T14:45:53Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Current implementation of parse-options.c treats OPT__QUIET() as integer\nand not boolean and thus it is more appropriate to print it as integer\nto avoid confusion.\n\nWhile at it, fix some style issues.\n---\n t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------\n test-parse-options.c | 2 +-\n 2 files changed, 48 insertions(+), 48 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 9be6411..2dcbdc0 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -7,7 +7,7 @@ test_description='our own option parser'\n \n . ./test-lib.sh\n \n-cat > expect << EOF\n+cat >expect <\n \n --yes get a boolean\n@@ -49,7 +49,7 @@ Standard options\n EOF\n \n test_expect_success 'test help' '\n-\ttest_must_fail test-parse-options -h > output 2> output.err &&\n+\ttest_must_fail test-parse-options -h >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_i18ncmp expect output\n '\n@@ -64,7 +64,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' '\n \tcheck magnitude: 3221225472 -m 3g\n '\n \n-cat > expect << EOF\n+cat >expect < expect << EOF\n+cat >expect < expect << EOF\n+cat >expect < output 2> output.err &&\n+\t\t>output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect << EOF\n+cat >expect < output 2> output.err &&\n+\ttest-parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'unambiguously abbreviated option with \"=\"' '\n-\ttest-parse-options --int=2 > output 2> output.err &&\n+\ttest-parse-options --int=2 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n@@ -256,7 +256,7 @@ test_expect_success 'ambiguously abbreviated option' '\n \ttest_expect_code 129 test-parse-options --strin 123\n '\n \n-cat > expect << EOF\n+cat >expect < output 2> output.err &&\n+\ttest-parse-options --st 123 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > typo.err << EOF\n+cat >typo.err < output 2> output.err &&\n+\ttest_must_fail test-parse-options -boolean >output 2>output.err &&\n \ttest_must_be_empty output &&\n \ttest_cmp typo.err output.err\n '\n \n-cat > typo.err << EOF\n+cat >typo.err < output 2> output.err &&\n+\ttest_must_fail test-parse-options -ambiguous >output 2>output.err &&\n \ttest_must_be_empty output &&\n \ttest_cmp typo.err output.err\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options --quux >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n- test_cmp expect output\n+\ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\t\tfoo -q >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options --length=four -b -4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest_must_fail test-parse-options --no-length >output 2>output.err &&\n \ttest_i18ncmp expect output &&\n \ttest_i18ncmp expect.err output.err\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options --set23 -bbbbb --no-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' '\n-\ttest-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err &&\n+\ttest-parse-options --set23 -bbbbb --neg-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options -bb --or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_NEGBIT() works' '\n-\ttest-parse-options -bb --no-neg-or4 > output 2> output.err &&\n+\ttest-parse-options -bb --no-neg-or4 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '\n-\ttest-parse-options + + + + + + > output 2> output.err &&\n+\ttest-parse-options + + + + + + >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n-cat > expect <expect < output 2> output.err &&\n+\ttest-parse-options -12345 >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n@@ -449,7 +449,7 @@ timestamp: 0\n string: (not set)\n abbrev: 7\n verbose: 0\n-quiet: no\n+quiet: 0\n dry run: no\n file: (not set)\n EOF\n@@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n \ttest_cmp expect output\n '\n \n-cat >>expect <<'EOF'\n+cat >>expect <\nSigned-off-by: Pranit Bauva \n\nPrevious version of this patch:\n - [v10]: $gmane/288820\n\nChanges this version wrt previous one:\nI decided to include no of diffs in every test and rewrote the commit\nmessage so as to sell this idea. This was given as an option to me by\nEric and the other option being to drop unnecessary testing of lines\nwhere it isn't required. Also this patch uses a suggestion given by Eric\nto make the \"editor\" look more clean as compared to the editor in my\nprevious version.\n---\n t/t7507-commit-verbose.sh | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..0f28a86 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -3,11 +3,10 @@\n test_description='verbose commit template'\n . ./test-lib.sh\n \n-cat >check-for-diff <out\n+exit 0\n EOF\n-chmod +x check-for-diff\n test_set_editor \"$PWD/check-for-diff\"\n \n cat >message <<'EOF'\n@@ -23,7 +22,8 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'initial commit shows verbose diff' '\n-\tgit commit --amend -v\n+\tgit commit --amend -v &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'second commit' '\n@@ -39,13 +39,15 @@ check_message() {\n \n test_expect_success 'verbose diff is stripped out' '\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n \tgit config diff.mnemonicprefix true &&\n \tgit commit --amend -v &&\n-\tcheck_message message\n+\tcheck_message message &&\n+\ttest_line_count = 1 out\n '\n \n cat >diff <<'EOF'\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"282321","messageId":"01020153cd2341e2-2616ba0b-9271-4a97-87d1-b6dedbf31828-000000@eu-west-1.amazonses.com","threadId":"41835","inReplyTo":"01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com","subject":"[PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T14:45:53Z","receivedAt":"2016-03-31T14:45:53Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"The reason to make it respect \"unspecified\" values is to give the\nability to differentiate whether `--option` or `--no-option` was\nspecified at all. \"unspecified\" values should be in the form of negative\nvalues. If initial value is set to negative and `--option` specified\nthen it will reflect the number of occurrences (counting done from 0),\nif `--no-option` is specified then it will reflect 0 and if nothing at\nall is given then it will retain its negative value.\n\nThis change will not affect existing users of COUNTUP at all, as long as\nthey use the initial value of 0 (or more).\n\nForce uses the \"unspecified\" value in conjunction with OPT__FORCE in\nbuiltin/clean.c in a different way to handle its config which\nmunges the \"-1\" into 0 before we get to parse_options.\n\nTo test this behavior \"verbose\" is set to \"unspecified\" while quiet is\nset to 0 which will test the new behavior with all sets of values.\n\nHelped-by: Jeff King \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nSigned-off-by: Pranit Bauva \n\n---\nThe discussion about this patch:\n[1] : http://thread.gmane.org/gmane.comp.version-control.git/289027\n\nPrevious version of the patch:\n[v10] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v9] : http://thread.gmane.org/gmane.comp.version-control.git/288820\n[v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061\n\nChanges introduced:\nUse a different language in commit message to make the change and its\nutility more clear. Also added some points as to where this patch could\nbreak but it doesn't. Include tests to test this behavior by modifying\ntest-parse-options.c and t/t0040-parse-options.sh as requested by SZEDER\nand reminded by Eric.\n\nPlease Note: The diff might seem improper especially the part where I\nhave introduced some continuous lines but this is a logical error by git\ndiff (nothing could be done about it) and thus the changes will be\nclearly visible with the original file itself.\n---\n Documentation/technical/api-parse-options.txt | 8 ++++--\n parse-options.c | 8 +++++-\n t/t0040-parse-options.sh | 39 ++++++++++++++++++++-------\n test-parse-options.c | 3 ++-\n 4 files changed, 44 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt\nindex 5f0757d..8908bf7 100644\n--- a/Documentation/technical/api-parse-options.txt\n+++ b/Documentation/technical/api-parse-options.txt\n@@ -144,8 +144,12 @@ There are some macros to easily define options:\n \n `OPT_COUNTUP(short, long, &int_var, description)`::\n \tIntroduce a count-up option.\n-\t`int_var` is incremented on each use of `--option`, and\n-\treset to zero with `--no-option`.\n+\tEach use of `--option` increments `int_var`, starting from zero\n+\t(even if initially negative), and `--no-option` resets it to\n+\tzero. To determine if `--option` or `--no-option` was set at\n+\tall, set `int_var` to a negative value, and if it is still\n+\tnegative after parse_options(), then neither `--option` nor\n+\t`--no-option` was seen.\n \n `OPT_BIT(short, long, &int_var, description, mask)`::\n \tIntroduce a boolean option.\ndiff --git a/parse-options.c b/parse-options.c\nindex 47a9192..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:\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 2dcbdc0..275bb64 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -63,7 +63,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -211,7 +211,7 @@ magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -234,7 +234,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -263,7 +263,7 @@ magnitude: 0\n timestamp: 0\n string: 123\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -302,7 +302,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -322,7 +322,7 @@ magnitude: 0\n timestamp: 1\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 1\n dry run: no\n file: (not set)\n@@ -344,7 +344,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -373,7 +373,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -398,7 +398,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -429,7 +429,7 @@ magnitude: 0\n timestamp: 0\n string: (not set)\n abbrev: 7\n-verbose: 0\n+verbose: -1\n quiet: 0\n dry run: no\n file: (not set)\n@@ -454,6 +454,25 @@ dry run: no\n file: (not set)\n EOF\n \n+test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '\n+\ttest-parse-options --no-verbose >output 2>output.err &&\n+\ttest_must_be_empty output.err &&\n+\ttest_cmp expect output\n+'\n+\n+cat >expect <output 2>output.err &&\n \ttest_must_be_empty output.err &&\ndiff --git a/test-parse-options.c b/test-parse-options.c\nindex 86afa98..f02c275 100644\n--- a/test-parse-options.c\n+++ b/test-parse-options.c\n@@ -7,7 +7,8 @@ static int integer = 0;\n static unsigned long magnitude = 0;\n static unsigned long timestamp;\n static int abbrev = 7;\n-static int verbose = 0, dry_run = 0, quiet = 0;\n+static int verbose = -1; /* unspecified */\n+static int dry_run = 0, quiet = 0;\n static char *string = NULL;\n static char *file = NULL;\n static int ambiguous;\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"282323","messageId":"01020153cd2341ec-341900e5-d186-401f-8cd2-dd213303ae4f-000000@eu-west-1.amazonses.com","threadId":"41835","inReplyTo":"01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com","subject":"[PATCH v11 4/4] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T14:45:53Z","receivedAt":"2016-03-31T14:45:53Z","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 - [v10] $gmane/288820\n - [v9] $gmane/288820\n - [v8] $gmane/288820\n - [v7] $gmane/288820\n - [v6] $gmane/288728\n - [v5] $gmane/288728\n - [v4] $gmane/288652\n - [v3] $gmane/288634\n - [v2] $gmane/288569\n - [v1] $gmane/287540\n\nChanges with respect to the previous patch:\n - Before config_verbose reacted to only -1 but henceforth it will react\n to every negative integer.\n - Use test_line_count for consistency\n - Add 1 more test for to check whether git status breaks\n - Add more tests to test whether commit.verbose works fine with all\n integers suggested by Eric Sunshine.\n Note: One might think some tests are extra but I think that it will\n be better to include them as they \"complete the continuity\" thus\n generalising the series which will make the patch even more clearer.\n\nSZEDER pointed out a bug in v9 which broke status. verbose being set to\n-1, returns true in the `if(verbose)` statement which gives a verbose\noutput. So thus, it is required to reset it to 0 if it was -1. This will\nwork fine when status is passed with --verbose and --no-verbose as they\nboth reflect non-negative values.\n---\n Documentation/config.txt | 4 +\n Documentation/git-commit.txt | 3 +-\n builtin/commit.c | 14 +++-\n t/t7507-commit-verbose.sh | 175 +++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 194 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2cd6bdd..1d0ec2e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1110,6 +1110,10 @@ commit.template::\n \t\"`~/`\" is expanded to the value of `$HOME` and \"`~user/`\" to the\n \tspecified user's home directory.\n \n+commit.verbose::\n+\tA boolean or int to specify the level of verbose with `git commit`.\n+\tSee linkgit:git-commit[1].\n+\n credential.helper::\n \tSpecify an external helper to be called when a username or\n \tpassword credential is needed; the helper may consult external\ndiff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\nindex 9ec6b3c..d474226 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].\n \twhat changes the commit has.\n \tNote that this diff output doesn't have its\n \tlines prefixed with '#'. This diff will not be a part\n-\tof the commit message.\n+\tof the commit message. See the `commit.verbose` configuration\n+\tvariable in linkgit:git-config[1].\n +\n If specified twice, show in addition the unified diff between\n what would be committed and the worktree files, i.e. the unstaged\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b3bd2d4..96e6190 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -113,7 +113,9 @@ static char *edit_message, *use_message;\n static char *fixup_message, *squash_message;\n static int all, also, interactive, patch_interactive, only, amend, signoff;\n static int edit_flag = -1; /* unspecified */\n-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;\n+static int config_verbose = -1; /* unspecified */\n+static int verbose = -1; /* unspecified */\n+static int quiet, no_verify, allow_empty, dry_run, renew_authorship;\n static int no_post_rewrite, allow_empty_message;\n static char *untracked_files_arg, *force_date, *ignore_submodule_arg;\n static char *sign_commit;\n@@ -1354,6 +1356,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_status_usage, 0);\n \tfinalize_colopts(&s.colopts, -1);\n \tfinalize_deferred_config(&s);\n+\tif (verbose == -1)\n+\t\tverbose = 0;\n \n \thandle_untracked_files_arg(&s);\n \tif (show_ignored_in_status)\n@@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n \t\tsign_commit = git_config_bool(k, v) ? \"\" : NULL;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"commit.verbose\")) {\n+\t\tint is_bool;\n+\t\tconfig_verbose = git_config_bool_or_int(k, v, &is_bool);\n+\t\treturn 0;\n+\t}\n \n \tstatus = git_gpg_config(k, v, NULL);\n \tif (status)\n@@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \targc = parse_and_validate_options(argc, argv, builtin_commit_options,\n \t\t\t\t\t builtin_commit_usage,\n \t\t\t\t\t prefix, current_head, &s);\n+\tif (verbose == -1)\n+\t\tverbose = (config_verbose < 0) ? 0 : config_verbose;\n+\n \tif (dry_run)\n \t\treturn dry_run_commit(argc, argv, prefix, current_head, &s);\n \tindex_file = prepare_index(argc, argv, prefix, current_head, 0);\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 0f28a86..7c79484 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -98,4 +98,179 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n '\n \n+test_expect_success 'set up -v -v' '\n+\techo dirty >file &&\n+\techo dirty >file2 &&\n+\tgit add file2\n+'\n+test_expect_success 'commit.verbose true and --verbose omitted' '\n+\tgit -c commit.verbose=true commit -F message &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose true and --verbose' '\n+\tgit -c commit.verbose=true commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose true and -v -v' '\n+\tgit -c commit.verbose=true commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose true and --no-verbose' '\n+\tgit -c commit.verbose=true commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose' '\n+\tgit -c commit.verbose=false commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose false and -v -v' '\n+\tgit -c commit.verbose=false commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose false and --verbose omitted' '\n+\tgit -c commit.verbose=false commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose false and --no-verbose' '\n+\tgit -c commit.verbose=false commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and --verbose omitted' '\n+\tgit -c commit.verbose=-2 commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and --verbose omitted' '\n+\tgit -c commit.verbose=-1 commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and --verbose omitted' '\n+\tgit -c commit.verbose=0 commit --amend &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and --verbose omitted' '\n+\tgit -c commit.verbose=1 commit --amend &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and --verbose omitted' '\n+\tgit -c commit.verbose=2 commit --amend &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit-verbose=3 and --verbose omitted' '\n+\tgit -c commit.verbose=3 commit --amend &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and --verbose' '\n+\tgit -c commit.verbose=-2 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and --verbose' '\n+\tgit -c commit.verbose=-1 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and --verbose' '\n+\tgit -c commit.verbose=0 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and --verbose' '\n+\tgit -c commit.verbose=1 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and --verbose' '\n+\tgit -c commit.verbose=2 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=3 and --verbose' '\n+\tgit -c commit.verbose=3 commit --amend --verbose &&\n+\ttest_line_count = 1 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and -v -v' '\n+\tgit -c commit.verbose=-2 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and -v -v' '\n+\tgit -c commit.verbose=-1 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and -v -v' '\n+\tgit -c commit.verbose=0 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and -v -v' '\n+\tgit -c commit.verbose=1 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and -v -v' '\n+\tgit -c commit.verbose=2 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=3 and -v -v' '\n+\tgit -c commit.verbose=3 commit --amend -v -v &&\n+\ttest_line_count = 2 out\n+'\n+\n+test_expect_success 'commit.verbose=-2 and --no-verbose' '\n+\tgit -c commit.verbose=-2 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=-1 and --no-verbose' '\n+\tgit -c commit.verbose=-1 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=0 and --no-verbose' '\n+\tgit -c commit.verbose=0 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=1 and --no-verbose' '\n+\tgit -c commit.verbose=1 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=2 and --no-verbose' '\n+\tgit -c commit.verbose=2 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'commit.verbose=3 and --no-verbose' '\n+\tgit -c commit.verbose=3 commit --amend --no-verbose &&\n+\ttest_line_count = 0 out\n+'\n+\n+test_expect_success 'status ignores commit.verbose=true' '\n+\tgit -c commit.verbose=true status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n+test_expect_success 'status does not verbose without --verbose' '\n+\tgit status >actual &&\n+\t! grep \"^diff --git\" actual\n+'\n+\n test_done\n\n--\nhttps://github.com/git/git/pull/218\n"},{"id":"282362","messageId":"xmqqegaqo6gg.fsf@gitster.mtv.corp.google.com","threadId":"41835","inReplyTo":"01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v11 1/4] test-parse-options: print quiet as integer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T18:19:59Z","receivedAt":"2016-03-31T18:19:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Current implementation of parse-options.c treats OPT__QUIET() as integer\n> and not boolean and thus it is more appropriate to print it as integer\n> to avoid confusion.\n>\n> While at it, fix some style issues.\n\nI counted the changes in t0040 and you have _more_ style fixes than\nthe real change. That is not \"while at it\".\n\nWhile I welcome the style fix, it is better done as a preparatory\nclean-up step before the real change.\n\n> ---\n\nMissing sign-off.\n\n> -cat > typo.err << EOF\n> +cat >typo.err < error: did you mean \\`--boolean\\` (with two dashes ?)\n> EOF\n\nIf your \"style cleanup\" patch were separate, you could fix this (and\nother that have backslash escape inside the here-document) further\nto something like this:\n\n\tcat >type.err <<\\EOF\n\terror: did you mean `--boolean` (with two dashes ?)\n EOF\n\nThanks.\n"},{"id":"282363","messageId":"xmqqa8leo6a6.fsf@gitster.mtv.corp.google.com","threadId":"41835","inReplyTo":"01020153cd2341ef-eed17644-85ee-4d1d-8051-b5d56bd9ae52-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v11 3/4] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T18:23:45Z","receivedAt":"2016-03-31T18:23:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Make the fake \"editor\" store output of grep in a file so that we can\n> see how many diffs were contained in the message and use them in\n> individual tests where ever it is required. Also use write_script()\n> to create the fake \"editor\".\n>\n> A subsequent commit will introduce scenarios where it is important to be\n> able to exactly determine how many diffs were present.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Pranit Bauva \n>\n> Previous version of this patch:\n> - [v10]: $gmane/288820\n>\n> Changes this version wrt previous one:\n> I decided to include no of diffs in every test and rewrote the commit\n> message so as to sell this idea. This was given as an option to me by\n> Eric and the other option being to drop unnecessary testing of lines\n> where it isn't required. Also this patch uses a suggestion given by Eric\n> to make the \"editor\" look more clean as compared to the editor in my\n> previous version.\n> ---\n\nOK, by always exiting 0 from the editor, you do not interfere with\nthe \"git commit\" that invoked it, and you inspect the editor's\nfinding after \"git commit\" returns. The approach taken by this\npatch looks a lot more sensible than the previous one.\n\nYou'd need the three-dash right before \"Previous version of...\"\nline, though.\n\n> t/t7507-commit-verbose.sh | 16 +++++++++-------\n> 1 file changed, 9 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 2ddf28c..0f28a86 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -3,11 +3,10 @@\n> test_description='verbose commit template'\n> . ./test-lib.sh\n> \n> -cat >check-for-diff < -#!$SHELL_PATH\n> -exec grep '^diff --git' \"\\$1\"\n> +write_script \"check-for-diff\" <<\\EOF &&\n> +grep '^diff --git' \"$1\" >out\n> +exit 0\n> EOF\n> -chmod +x check-for-diff\n> test_set_editor \"$PWD/check-for-diff\"\n> \n> cat >message <<'EOF'\n> @@ -23,7 +22,8 @@ test_expect_success 'setup' '\n> '\n> \n> test_expect_success 'initial commit shows verbose diff' '\n> -\tgit commit --amend -v\n> +\tgit commit --amend -v &&\n> +\ttest_line_count = 1 out\n> '\n> \n> test_expect_success 'second commit' '\n> @@ -39,13 +39,15 @@ check_message() {\n> \n> test_expect_success 'verbose diff is stripped out' '\n> \tgit commit --amend -v &&\n> -\tcheck_message message\n> +\tcheck_message message &&\n> +\ttest_line_count = 1 out\n> '\n> \n> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n> \tgit config diff.mnemonicprefix true &&\n> \tgit commit --amend -v &&\n> -\tcheck_message message\n> +\tcheck_message message &&\n> +\ttest_line_count = 1 out\n> '\n> \n> cat >diff <<'EOF'\n>\n> --\n> https://github.com/git/git/pull/218\n"},{"id":"282364","messageId":"CAFZEwPMyVftmW8ZCCyFMUMOegqimpFxjLrHfjMKc88w+HDvG+A@mail.gmail.com","threadId":"41835","inReplyTo":"xmqqegaqo6gg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v11 1/4] test-parse-options: print quiet as integer","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T18:40:35Z","receivedAt":"2016-03-31T18:40:35Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Thu, Mar 31, 2016 at 11:49 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> Current implementation of parse-options.c treats OPT__QUIET() as integer\n>> and not boolean and thus it is more appropriate to print it as integer\n>> to avoid confusion.\n>>\n>> While at it, fix some style issues.\n>\n> I counted the changes in t0040 and you have _more_ style fixes than\n> the real change. That is not \"while at it\".\n>\n> While I welcome the style fix, it is better done as a preparatory\n> clean-up step before the real change.\n\nOkay. I thought this was a minor change so I squashed it together. I\nwill separate it though.\n\n> Missing sign-off.\n\nWill include this\n\n>> -cat > typo.err << EOF\n>> +cat >typo.err <> error: did you mean \\`--boolean\\` (with two dashes ?)\n>> EOF\n>\n> If your \"style cleanup\" patch were separate, you could fix this (and\n> other that have backslash escape inside the here-document) further\n> to something like this:\n>\n> cat >type.err <<\\EOF\n> error: did you mean `--boolean` (with two dashes ?)\n> EOF\n>\n> Thanks.\n\nWill include this.\n"},{"id":"282365","messageId":"xmqq60w2o5g9.fsf@gitster.mtv.corp.google.com","threadId":"41835","inReplyTo":"01020153cd2341e2-2616ba0b-9271-4a97-87d1-b6dedbf31828-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T18:41:42Z","receivedAt":"2016-03-31T18:41:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> This change will not affect existing users of COUNTUP at all, as long as\n> they use the initial value of 0 (or more).\n>\n> Force uses the \"unspecified\" value in conjunction with OPT__FORCE in\n> builtin/clean.c in a different way to handle its config which\n> munges the \"-1\" into 0 before we get to parse_options.\n\nThese two paragraphs leave the readers wondering what the conclusion\nis. \"it is OK as long as...\" makes us wonder \"so are there users\nthat do not satisfy that condition?\" \"in a different way\" makes us\nwonder \"Does this change break builtin/clean.c because COUNTUP is\nused in a different way?\".\n\n\tThis change does not affect existing users of COUNTUP,\n\tbecause they all use the initial value of 0 (or more).\n\n\tNote that builtin/clean.c initializes the variable used with\n\tOPT__FORCE (which uses OPT_COUNTUP()) to a negative value,\n\tbut it is set to either 0 or 1 by reading the configuration\n\tbefore the code calls parse_options(), i.e. as far as\n\tparse_options() is concerned, the initial value of the\n\tvariable is not negative.\n\nperhaps?\n\n> `OPT_COUNTUP(short, long, &int_var, description)`::\n> \tIntroduce a count-up option.\n> -\t`int_var` is incremented on each use of `--option`, and\n> -\treset to zero with `--no-option`.\n> +\tEach use of `--option` increments `int_var`, starting from zero\n> +\t(even if initially negative), and `--no-option` resets it to\n> +\tzero. To determine if `--option` or `--no-option` was set at\n> +\tall, set `int_var` to a negative value, and if it is still\n> +\tnegative after parse_options(), then neither `--option` nor\n> +\t`--no-option` was seen.\n> \n> `OPT_BIT(short, long, &int_var, description, mask)`::\n> \tIntroduce a boolean option.\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\nThe new behaviour given by the patch looks very sensible, but I\nwonder if this is easier to reason about:\n\n\tcase OPTION_COUNTUP:\n+\t\tif (*(int *)opt->value < 0)\n+\t\t\t*(int *)opt->value = 0;\n\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n\nThat is, upon hitting this case arm, we know that an explicit option\nwas given from the command line, so the \"unspecified\" initial value,\nif it is still there, gets reset to 0, and after doing that, we just\ndo the usual thing.\n"},{"id":"282367","messageId":"CAFZEwPP1iQQq5WXsBa-4vZ3wXH-DeNUw5JtqsgYmeUSo-XPYxQ@mail.gmail.com","threadId":"41835","inReplyTo":"xmqqa8leo6a6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v11 3/4] t7507-commit-verbose: improve test coverage by testing number of diffs","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T18:42:20Z","receivedAt":"2016-03-31T18:42:20Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Thu, Mar 31, 2016 at 11:53 PM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> Make the fake \"editor\" store output of grep in a file so that we can\n>> see how many diffs were contained in the message and use them in\n>> individual tests where ever it is required. Also use write_script()\n>> to create the fake \"editor\".\n>>\n>> A subsequent commit will introduce scenarios where it is important to be\n>> able to exactly determine how many diffs were present.\n>>\n>> Helped-by: Eric Sunshine \n>> Signed-off-by: Pranit Bauva \n>>\n>> Previous version of this patch:\n>> - [v10]: $gmane/288820\n>>\n>> Changes this version wrt previous one:\n>> I decided to include no of diffs in every test and rewrote the commit\n>> message so as to sell this idea. This was given as an option to me by\n>> Eric and the other option being to drop unnecessary testing of lines\n>> where it isn't required. Also this patch uses a suggestion given by Eric\n>> to make the \"editor\" look more clean as compared to the editor in my\n>> previous version.\n>> ---\n>\n> OK, by always exiting 0 from the editor, you do not interfere with\n> the \"git commit\" that invoked it, and you inspect the editor's\n> finding after \"git commit\" returns. The approach taken by this\n> patch looks a lot more sensible than the previous one.\n>\n> You'd need the three-dash right before \"Previous version of...\"\n> line, though.\n\nThat's silly of me to forget this. Will do it.\n\n>> t/t7507-commit-verbose.sh | 16 +++++++++-------\n>> 1 file changed, 9 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>> index 2ddf28c..0f28a86 100755\n>> --- a/t/t7507-commit-verbose.sh\n>> +++ b/t/t7507-commit-verbose.sh\n>> @@ -3,11 +3,10 @@\n>> test_description='verbose commit template'\n>> . ./test-lib.sh\n>>\n>> -cat >check-for-diff <> -#!$SHELL_PATH\n>> -exec grep '^diff --git' \"\\$1\"\n>> +write_script \"check-for-diff\" <<\\EOF &&\n>> +grep '^diff --git' \"$1\" >out\n>> +exit 0\n>> EOF\n>> -chmod +x check-for-diff\n>> test_set_editor \"$PWD/check-for-diff\"\n>>\n>> cat >message <<'EOF'\n>> @@ -23,7 +22,8 @@ test_expect_success 'setup' '\n>> '\n>>\n>> test_expect_success 'initial commit shows verbose diff' '\n>> - git commit --amend -v\n>> + git commit --amend -v &&\n>> + test_line_count = 1 out\n>> '\n>>\n>> test_expect_success 'second commit' '\n>> @@ -39,13 +39,15 @@ check_message() {\n>>\n>> test_expect_success 'verbose diff is stripped out' '\n>> git commit --amend -v &&\n>> - check_message message\n>> + check_message message &&\n>> + test_line_count = 1 out\n>> '\n>>\n>> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n>> git config diff.mnemonicprefix true &&\n>> git commit --amend -v &&\n>> - check_message message\n>> + check_message message &&\n>> + test_line_count = 1 out\n>> '\n>>\n>> cat >diff <<'EOF'\n>>\n>> --\n>> https://github.com/git/git/pull/218\n"},{"id":"282380","messageId":"CAFZEwPO4CJ-Q=52Es14CdD0mjG-qgy4=X2kU71npi=8x7FsSQw@mail.gmail.com","threadId":"41835","inReplyTo":"xmqq60w2o5g9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T19:34:03Z","receivedAt":"2016-03-31T19:34:03Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> This change will not affect existing users of COUNTUP at all, as long as\n>> they use the initial value of 0 (or more).\n>>\n>> Force uses the \"unspecified\" value in conjunction with OPT__FORCE in\n>> builtin/clean.c in a different way to handle its config which\n>> munges the \"-1\" into 0 before we get to parse_options.\n>\n> These two paragraphs leave the readers wondering what the conclusion\n> is. \"it is OK as long as...\" makes us wonder \"so are there users\n> that do not satisfy that condition?\" \"in a different way\" makes us\n> wonder \"Does this change break builtin/clean.c because COUNTUP is\n> used in a different way?\".\n>\n> This change does not affect existing users of COUNTUP,\n> because they all use the initial value of 0 (or more).\n>\n> Note that builtin/clean.c initializes the variable used with\n> OPT__FORCE (which uses OPT_COUNTUP()) to a negative value,\n> but it is set to either 0 or 1 by reading the configuration\n> before the code calls parse_options(), i.e. as far as\n> parse_options() is concerned, the initial value of the\n> variable is not negative.\n>\n> perhaps?\n\nThanks again for the help with the comit message. I sometimes fail to\nsee how first time readers will infer from the message.\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>> + Each use of `--option` increments `int_var`, starting from zero\n>> + (even if initially negative), and `--no-option` resets it to\n>> + zero. To determine if `--option` or `--no-option` was set at\n>> + 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>>\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> The new behaviour given by the patch looks very sensible, but I\n> wonder if this is easier to reason about:\n>\n> case OPTION_COUNTUP:\n> + if (*(int *)opt->value < 0)\n> + *(int *)opt->value = 0;\n> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n>\n> That is, upon hitting this case arm, we know that an explicit option\n> was given from the command line, so the \"unspecified\" initial value,\n> if it is still there, gets reset to 0, and after doing that, we just\n> do the usual thing.\n\nThis does look cleaner. Nice thinking that there is no need to\nactually specify when it gets 0. It gets 0 no matter what as long as\nOPTION_COUNTUP is speficied in any format (with or without --no) and\nvariable is \"unspecified\".\n"},{"id":"282381","messageId":"xmqqegaqmmyr.fsf@gitster.mtv.corp.google.com","threadId":"41835","inReplyTo":"CAFZEwPO4CJ-Q=52Es14CdD0mjG-qgy4=X2kU71npi=8x7FsSQw@mail.gmail.com","subject":"Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T20:06:20Z","receivedAt":"2016-03-31T20:06:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano wrote:\n>>\n>> case OPTION_COUNTUP:\n>> + if (*(int *)opt->value < 0)\n>> + *(int *)opt->value = 0;\n>> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n>>\n>> That is, upon hitting this case arm, we know that an explicit option\n>> was given from the command line, so the \"unspecified\" initial value,\n>> if it is still there, gets reset to 0, and after doing that, we just\n>> do the usual thing.\n>\n> This does look cleaner. Nice thinking that there is no need to\n> actually specify when it gets 0. It gets 0 no matter what as long as\n> OPTION_COUNTUP is speficied in any format (with or without --no) and\n> variable is \"unspecified\".\n\nI do not think there is any planned users of such an enhancement,\nbut the above points us into a future possibility to be able to do\nthis:\n\n case OPTION_COUNTUP:\n+ if (*(int *)opt->value < 0)\n+ *(int *)opt->value = -*(int *)opt->value - 1;\n *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n\nThat is, by using \"-2\" as the \"unspecified\" value, you can start\ncounting up from 2 (i.e. the presence of the option resets the\nvariable to 1 and then the option being not \"unset\" increments it)\nif your application wants to.\n"},{"id":"282387","messageId":"CAFZEwPN_=mLXf+8JfMhwf_5fviG-rjHp07b+41HXs2NLO6BnxA@mail.gmail.com","threadId":"41835","inReplyTo":"xmqqegaqmmyr.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-31T20:41:16Z","receivedAt":"2016-03-31T20:41:16Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Apr 1, 2016 at 1:36 AM, Junio C Hamano wrote:\n> Pranit Bauva writes:\n>\n>> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano wrote:\n>>>\n>>> case OPTION_COUNTUP:\n>>> + if (*(int *)opt->value < 0)\n>>> + *(int *)opt->value = 0;\n>>> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n>>>\n>>> That is, upon hitting this case arm, we know that an explicit option\n>>> was given from the command line, so the \"unspecified\" initial value,\n>>> if it is still there, gets reset to 0, and after doing that, we just\n>>> do the usual thing.\n>>\n>> This does look cleaner. Nice thinking that there is no need to\n>> actually specify when it gets 0. It gets 0 no matter what as long as\n>> OPTION_COUNTUP is speficied in any format (with or without --no) and\n>> variable is \"unspecified\".\n>\n> I do not think there is any planned users of such an enhancement,\n> but the above points us into a future possibility to be able to do\n> this:\n>\n> case OPTION_COUNTUP:\n> + if (*(int *)opt->value < 0)\n> + *(int *)opt->value = -*(int *)opt->value - 1;\n> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n>\n> That is, by using \"-2\" as the \"unspecified\" value, you can start\n> counting up from 2 (i.e. the presence of the option resets the\n> variable to 1 and then the option being not \"unset\" increments it)\n> if your application wants to.\n\nI am not very familiar with Git community but I think including a new\nfeature to use our existing library (who's functionality isn't\nrequired as for now) can trigger some creative thoughts in minds of\nother developers which will make them think \"How could this be used?\".\nThis could lead to some exciting ideas on improving the UI of git.\nHaving something actually in hand gives you a confidence, rather than\nknowing that you could grab it whenever it is needed.\n\nAlso, when such an idea for new feature comes up, it is better to\nimplement it because let's say some developer is stuck in future and\nthis new feature could help him but he doesn't have a clue that such a\ndiscussion happened some time ago. Thus saving him time and further\neffort.\n\nAnyways, How is the convention in git for these type of futuristic ideas?\n"},{"id":"282388","messageId":"xmqqshz6l6kt.fsf@gitster.mtv.corp.google.com","threadId":"41835","inReplyTo":"CAFZEwPN_=mLXf+8JfMhwf_5fviG-rjHp07b+41HXs2NLO6BnxA@mail.gmail.com","subject":"Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect \"unspecified\" values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T20:45:38Z","receivedAt":"2016-03-31T20:45:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva writes:\n\n> Also, when such an idea for new feature comes up, it is better to\n> implement it because let's say some developer is stuck in future and\n> this new feature could help him but he doesn't have a clue that such a\n> discussion happened some time ago. Thus saving him time and further\n> effort.\n>\n> Anyways, How is the convention in git for these type of futuristic ideas?\n\nJust keep it in mind without complicating the code, unless the idea\ntruly makes sense and has immediate use.\n\nFor example, I do not know how it would be useful to be able to\ncount up starting from 2 with an option --more that is COUNTUP, if\nyour --no-more would reset it to 0.\n\n git cmd --more ;# sets more=2\n git cmd --more --more ;# sets more=3\n git cmd --no-more --more ;# sets more=1, not more=2\n"}]}