{"thread":{"id":"36902","subject":"[PATCH v5 0/4] commit: Add commit.verbose configuration","startedAt":"2014-06-12T19:38:58Z","lastAt":"2014-06-16T22:25:22Z","messageCount":28,"participants":["Caleb Thompson","Jeremiah Mahler","Jeff King","Jakub Narębski","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":4},"messages":[{"id":"244024","messageId":"1402601942-45553-1-git-send-email-caleb@calebthompson.io","threadId":"36902","inReplyTo":null,"subject":"[PATCH v5 0/4] commit: Add commit.verbose configuration","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-12T19:38:58Z","receivedAt":"2014-06-12T19:38:58Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"This patch allows people to set commit.verbose to implicitly send\n--verbose to git-commit.\n\nThis version incorporates changes suggested by Eric Sunshine, Duy\nNguyen, and Jeremiah Mahler.\n\nIt introduces several cleanup patches to t/t7505-commit-verbose.sh to\nbring it closer to the current state of the tests as Eric has explained\nthem to me, then adds the verbose config and --no-verbose flag.\n\nSince the last version of this patch\n(http://marc.info/?l=git&m=140251155830422&w=2), I've made the following\nchanges:\n\n* Revert change to flags, as --no-verbose already existed and worked as\n expected with the commit.verbose configuration. Thanks to René Scharfe.\n* Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.\n\nAdditionally, this set of patches was generated by format-patch, so it\nshould work correctly with git-am.\n\n------------------------------------------------------\n\nCaleb Thompson (4):\n commit test: Use test_config instead of git-config\n commit test: Use write_script\n commit test: test_set_editor in each test\n commit: add commit.verbose configuration\n\n Documentation/config.txt | 5 +++\n Documentation/git-commit.txt | 8 ++++-\n builtin/commit.c | 4 +++\n contrib/completion/git-completion.bash | 1 +\n t/t7507-commit-verbose.sh | 64 +++++++++++++++++++++++++---------\n 5 files changed, 64 insertions(+), 18 deletions(-)\n\n--\n2.0.0\n"},{"id":"244027","messageId":"1402601942-45553-2-git-send-email-caleb@calebthompson.io","threadId":"36902","inReplyTo":"1402601942-45553-1-git-send-email-caleb@calebthompson.io","subject":"[PATCH v5 1/4] commit test: Use test_config instead of git-config","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-12T19:38:59Z","receivedAt":"2014-06-12T19:38:59Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"Some of the tests in t/t7507-commit-verbose.sh were still using\ngit-config to set configuration. Change them to use the test_config\nhelper.\n\nSigned-off-by: Caleb Thompson \n---\n t/t7507-commit-verbose.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..6d778ed 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -43,7 +43,7 @@ test_expect_success 'verbose diff is stripped out' '\n '\n \n test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n-\tgit config diff.mnemonicprefix true &&\n+\ttest_config diff.mnemonicprefix true &&\n \tgit commit --amend -v &&\n \tcheck_message message\n '\n@@ -71,7 +71,7 @@ test_expect_success 'diff in message is retained with -v' '\n '\n \n test_expect_success 'submodule log is stripped out too with -v' '\n-\tgit config diff.submodule log &&\n+\ttest_config diff.submodule log &&\n \tgit submodule add ./. sub &&\n \tgit commit -m \"sub added\" &&\n \t(\n-- \n2.0.0\n"},{"id":"244025","messageId":"1402601942-45553-3-git-send-email-caleb@calebthompson.io","threadId":"36902","inReplyTo":"1402601942-45553-1-git-send-email-caleb@calebthompson.io","subject":"[PATCH v5 2/4] commit test: Use write_script","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-12T19:39:00Z","receivedAt":"2014-06-12T19:39:00Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"Use write_script from t/test-lib-functions.sh instead of cat, shebang,\nand chmod. This protects us from potential shell meta-characters in the\nname of our trash directory, which would be interpreted if we set\n$EDITOR directly.\n\nSigned-off-by: Caleb Thompson \n---\n t/t7507-commit-verbose.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 6d778ed..db09107 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -3,11 +3,9 @@\n test_description='verbose commit template'\n . ./test-lib.sh\n \n-cat >check-for-diff <message <<'EOF'\n-- \n2.0.0\n"},{"id":"244026","messageId":"1402601942-45553-4-git-send-email-caleb@calebthompson.io","threadId":"36902","inReplyTo":"1402601942-45553-1-git-send-email-caleb@calebthompson.io","subject":"[PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-12T19:39:01Z","receivedAt":"2014-06-12T19:39:01Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"t/t7507-commit-verbose.sh was using a global test_set_editor call to\nbuild its environment.\n\nImprove robustness against global state changes by having only tests\nwhich intend to use the $EDITOR to check for presence of a diff in the\neditor set up the test-editor to use check-for-diff rather than relying\nupon the editor set once at script start.\n\nBesides being in line with current practices, it also allows the tests\nwhich set GIT_EDITOR=cat manually to avoid using a subshell and simplify\ntheir logic.\n\nSigned-off-by: Caleb Thompson \n---\n t/t7507-commit-verbose.sh | 18 +++++++-----------\n 1 file changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex db09107..35a4d06 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -6,7 +6,6 @@ test_description='verbose commit template'\n write_script check-for-diff <<-'EOF'\n \texec grep '^diff --git' \"$1\"\n EOF\n-test_set_editor \"$PWD/check-for-diff\"\n \n cat >message <<'EOF'\n subject\n@@ -21,6 +20,7 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'initial commit shows verbose diff' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \tgit commit --amend -v\n '\n \n@@ -36,11 +36,13 @@ check_message() {\n }\n \n test_expect_success 'verbose diff is stripped out' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \tgit commit --amend -v &&\n \tcheck_message message\n '\n \n test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n \ttest_config diff.mnemonicprefix true &&\n \tgit commit --amend -v &&\n \tcheck_message message\n@@ -77,20 +79,14 @@ test_expect_success 'submodule log is stripped out too with -v' '\n \t\techo \"more\" >>file &&\n \t\tgit commit -a -m \"submodule commit\"\n \t) &&\n-\t(\n-\t\tGIT_EDITOR=cat &&\n-\t\texport GIT_EDITOR &&\n-\t\ttest_must_fail git commit -a -v 2>err\n-\t) &&\n+\ttest_set_editor cat &&\n+\ttest_must_fail git commit -a -v 2>err &&\n \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n '\n \n test_expect_success 'verbose diff is stripped out with set core.commentChar' '\n-\t(\n-\t\tGIT_EDITOR=cat &&\n-\t\texport GIT_EDITOR &&\n-\t\ttest_must_fail git -c core.commentchar=\";\" commit -a -v 2>err\n-\t) &&\n+\ttest_set_editor cat &&\n+\ttest_must_fail git -c core.commentchar=\";\" commit -a -v 2>err &&\n \ttest_i18ngrep \"Aborting commit due to empty commit message.\" err\n '\n \n-- \n2.0.0\n"},{"id":"244030","messageId":"1402603225-46240-1-git-send-email-caleb@calebthompson.io","threadId":"36902","inReplyTo":"1402601942-45553-1-git-send-email-caleb@calebthompson.io","subject":"[PATCH v5 4/4] commit: Add commit.verbose configuration","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-12T20:00:25Z","receivedAt":"2014-06-12T20:00:25Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"Add a new configuration variable commit.verbose to implicitly pass\n--verbose to git-commit. Ensure that --no-verbose to git-commit\nnegates that setting.\n\nSigned-off-by: Caleb Thompson \n---\n Documentation/config.txt | 5 +++++\n Documentation/git-commit.txt | 8 +++++++-\n builtin/commit.c | 4 ++++\n contrib/completion/git-completion.bash | 1 +\n t/t7507-commit-verbose.sh | 36 ++++++++++++++++++++++++++++++++++\n 5 files changed, 53 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex cd2d651..ec51e1c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1017,6 +1017,11 @@ 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 to enable/disable inclusion of diff information in the\n+\tcommit message template when using an editor to prepare the commit\n+\tmessage. Defaults to false.\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 0bbc8f5..8cb3439 100644\n--- a/Documentation/git-commit.txt\n+++ b/Documentation/git-commit.txt\n@@ -282,7 +282,13 @@ configuration variable documented in linkgit:git-config[1].\n\tShow unified diff between the HEAD commit and what\n\twould be committed at the bottom of the commit message\n\ttemplate. Note that this diff output doesn't have its\n-\tlines prefixed with '#'.\n+\tlines prefixed with '#'. The `commit.verbose` configuration\n+\tvariable can be set to true to implicitly send this option.\n+\n+--no-verbose::\n+\tDo not show the unified diff at the bottom of the commit message\n+\ttemplate. This is the default behavior, but can be used to override\n+\tthe `commit.verbose` configuration variable.\n\n -q::\n --quiet::\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 99c2044..c782388 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1489,6 +1489,10 @@ 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\tverbose = git_config_bool(k, v);\n+\t\treturn 0;\n+\t}\n\n\tstatus = git_gpg_config(k, v, NULL);\n\tif (status)\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 2c59a76..b8f4b94 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1976,6 +1976,7 @@ _git_config ()\n\t\tcolor.ui\n\t\tcommit.status\n\t\tcommit.template\n+\t\tcommit.verbose\n\t\tcore.abbrev\n\t\tcore.askpass\n\t\tcore.attributesfile\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 35a4d06..402d6a1 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'\n\texec grep '^diff --git' \"$1\"\n EOF\n\n+write_script check-for-no-diff <<-'EOF'\n+\texec grep -v '^diff --git' \"$1\"\n+EOF\n+\n cat >message <<'EOF'\n subject\n\n@@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n\tcheck_message message\n '\n\n+test_expect_success 'commit shows verbose diff with commit.verbose true' '\n+\techo morecontent >>file &&\n+\tgit add file &&\n+\ttest_config commit.verbose true &&\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\tgit commit --amend\n+'\n+\n+test_expect_success 'commit --verbose overrides commit.verbose false' '\n+\techo evenmorecontent >>file &&\n+\tgit add file &&\n+\ttest_config commit.verbose false &&\n+\ttest_set_editor \"$PWD/check-for-diff\" &&\n+\tgit commit --amend --verbose\n+'\n+\n+test_expect_success 'commit does not show verbose diff with commit.verbose false' '\n+\techo evenmorecontent >>file &&\n+\tgit add file &&\n+\ttest_config commit.verbose false &&\n+\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n+\tgit commit --amend\n+'\n+\n+test_expect_success 'commit --no-verbose overrides commit.verbose true' '\n+\techo evenmorecontent >>file &&\n+\tgit add file &&\n+\ttest_config commit.verbose true &&\n+\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n+\tgit commit --amend --no-verbose\n+'\n+\n cat >diff <<'EOF'\n This is an example commit message that contains a diff.\n\n--\n2.0.0\n"},{"id":"244032","messageId":"20140612203010.GA17761@hudson.localdomain","threadId":"36902","inReplyTo":"1402601942-45553-1-git-send-email-caleb@calebthompson.io","subject":"Re: [PATCH v5 0/4] commit: Add commit.verbose configuration","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T20:30:10Z","receivedAt":"2014-06-12T20:30:10Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Thu, Jun 12, 2014 at 02:38:58PM -0500, Caleb Thompson wrote:\n> This patch allows people to set commit.verbose to implicitly send\n> --verbose to git-commit.\n> \n> This version incorporates changes suggested by Eric Sunshine, Duy\n> Nguyen, and Jeremiah Mahler.\n> \n> It introduces several cleanup patches to t/t7505-commit-verbose.sh to\n> bring it closer to the current state of the tests as Eric has explained\n> them to me, then adds the verbose config and --no-verbose flag.\n> \n> Since the last version of this patch\n> (http://marc.info/?l=git&m=140251155830422&w=2), I've made the following\n> changes:\n> \n> * Revert change to flags, as --no-verbose already existed and worked as\n> expected with the commit.verbose configuration. Thanks to René Scharfe.\n> * Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.\n> \n> Additionally, this set of patches was generated by format-patch, so it\n> should work correctly with git-am.\n> \n> ------------------------------------------------------\n> \n> Caleb Thompson (4):\n> commit test: Use test_config instead of git-config\n> commit test: Use write_script\n> commit test: test_set_editor in each test\n> commit: add commit.verbose configuration\n> \n> Documentation/config.txt | 5 +++\n> Documentation/git-commit.txt | 8 ++++-\n> builtin/commit.c | 4 +++\n> contrib/completion/git-completion.bash | 1 +\n> t/t7507-commit-verbose.sh | 64 +++++++++++++++++++++++++---------\n> 5 files changed, 64 insertions(+), 18 deletions(-)\n> \n> --\n> 2.0.0\n> \n\nThe patches look good, they apply clean ('git am'), and all tests pass.\n\nReviewed-by: Jeremiah Mahler \n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244057","messageId":"20140613065037.GA7908@sigill.intra.peff.net","threadId":"36902","inReplyTo":"1402601942-45553-3-git-send-email-caleb@calebthompson.io","subject":"Re: [PATCH v5 2/4] commit test: Use write_script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T06:50:37Z","receivedAt":"2014-06-13T06:50:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 12, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:\n\n> Use write_script from t/test-lib-functions.sh instead of cat, shebang,\n> and chmod. This protects us from potential shell meta-characters in the\n> name of our trash directory, which would be interpreted if we set\n> $EDITOR directly.\n\nI'm not sure about this last sentence; isn't that what test_set_editor\nis doing, which was already there? I think the real rationale is\nreadability: since $SHELL_PATH is handled for us, you can turn off\ninterpolation in the here-doc containing the helper script. That avoids\nan extra layer of quoting.\n\n-Peff\n"},{"id":"244058","messageId":"20140613065942.GB7908@sigill.intra.peff.net","threadId":"36902","inReplyTo":"1402601942-45553-4-git-send-email-caleb@calebthompson.io","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T06:59:42Z","receivedAt":"2014-06-13T06:59:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 12, 2014 at 02:39:01PM -0500, Caleb Thompson wrote:\n\n> t/t7507-commit-verbose.sh was using a global test_set_editor call to\n> build its environment.\n> \n> Improve robustness against global state changes by having only tests\n> which intend to use the $EDITOR to check for presence of a diff in the\n> editor set up the test-editor to use check-for-diff rather than relying\n> upon the editor set once at script start.\n\nThis implies to me that EDITOR is unset after leaving these tests. I\ndon't think that is how it works, though. The tests themselves run in\nthe main environment of the test script. A call to test_set_editor from\none of them will still affect the other tests[1].\n\nI think it works anyway because every subsequent test that cares\nactually sets the editor itself.\n\nOr did you just mean that the new rule is \"every test sets the editor as\nthey need\", which means that we do not have to worry anymore about\npolluting the environment for other tests?\n\n-Peff\n\n[1] It might make sense for test_set_editor, when run from within a\n test, to behave more like test_config, and do:\n\n test_when_finished '\n sane_unset FAKE_EDITOR &&\n sane_unset EDITOR\n '\n\n I don't know if there would be fallouts with other test scripts,\n though.\n"},{"id":"244144","messageId":"20140613162607.GA85151@sirius.local","threadId":"36902","inReplyTo":"20140613065037.GA7908@sigill.intra.peff.net","subject":"Re: [PATCH v5 2/4] commit test: Use write_script","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-13T16:26:07Z","receivedAt":"2014-06-13T16:26:07Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"You're very right - I may have confused this commit message and the one\nto switch to test_set_editor. I'll rewrite this commit message.\n\nWhat do you think of something like this for the description:\n\n Use write_script from t/test-lib-functions.sh instead of cat,\n shebang, and chmod. This aids in readability for creating the script\n by using the named function and allows us to turn off interpolation\n in the heredoc of the script body to avoid extra escaping, since\n $SHELL_PATH is handled for us.\n\nOn Fri, Jun 13, 2014 at 02:50:37AM -0400, Jeff King wrote:\n> On Thu, Jun 12, 2014 at 02:39:00PM -0500, Caleb Thompson wrote:\n>\n> > Use write_script from t/test-lib-functions.sh instead of cat, shebang,\n> > and chmod. This protects us from potential shell meta-characters in the\n> > name of our trash directory, which would be interpreted if we set\n> > $EDITOR directly.\n>\n> I'm not sure about this last sentence; isn't that what test_set_editor\n> is doing, which was already there? I think the real rationale is\n> readability: since $SHELL_PATH is handled for us, you can turn off\n> interpolation in the here-doc containing the helper script. That avoids\n> an extra layer of quoting.\n>\n> -Peff\n"},{"id":"244146","messageId":"20140613163644.GB85151@sirius.local","threadId":"36902","inReplyTo":"20140613065942.GB7908@sigill.intra.peff.net","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-13T16:36:44Z","receivedAt":"2014-06-13T16:36:44Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:\n> On Thu, Jun 12, 2014 at 02:39:01PM -0500, Caleb Thompson wrote:\n>\n> > t/t7507-commit-verbose.sh was using a global test_set_editor call to\n> > build its environment.\n> >\n> > Improve robustness against global state changes by having only tests\n> > which intend to use the $EDITOR to check for presence of a diff in the\n> > editor set up the test-editor to use check-for-diff rather than relying\n> > upon the editor set once at script start.\n>\n> This implies to me that EDITOR is unset after leaving these tests. I\n> don't think that is how it works, though. The tests themselves run in\n> the main environment of the test script. A call to test_set_editor from\n> one of them will still affect the other tests[1].\n>\n> I think it works anyway because every subsequent test that cares\n> actually sets the editor itself.\n>\n> Or did you just mean that the new rule is \"every test sets the editor as\n> they need\", which means that we do not have to worry anymore about\n> polluting the environment for other tests?\n\nThat's exactly what I meant. We can stop relying on the global state *as\nit is initially set* and instead move the setup into the tests which\nrely on it.\n\n>\n> -Peff\n>\n> [1] It might make sense for test_set_editor, when run from within a\n> test, to behave more like test_config, and do:\n>\n> test_when_finished '\n> sane_unset FAKE_EDITOR &&\n> sane_unset EDITOR\n> '\n\nIt might, but it's a little out of scope in addition to your concern\nabout other test scripts.\n\n>\n> I don't know if there would be fallouts with other test scripts,\n> though.\n\nHow is this for a reword of that commit description:\n\n t/t7507-commit-verbose.sh was using a global test_set_editor call to\n build its environment. The $EDITOR being used was not necessary for\n all tests, and was in fact circumvented using subshells in some\n cases.\n\n To improve robustness against global state changes and avoid the\n use of subshells to temporarily switch the editor, set the editor\n explicitly wherever it will be important.\n\n Specifically, in tests that need to check for the presence of a diff in the\n editor, make calls to set_test_editor to set $EDITOR to check-for-diff\n rather than relying on that editor being configured globally. This also\n helps readers grok the tests as the setup is closer to the verification.\n\nCaleb Thompson\n"},{"id":"244147","messageId":"20140613164910.GA87252@sirius.local","threadId":"36902","inReplyTo":"20140612203010.GA17761@hudson.localdomain","subject":"Re: [PATCH v5 0/4] commit: Add commit.verbose configuration","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-13T16:49:10Z","receivedAt":"2014-06-13T16:49:10Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"On Thu, Jun 12, 2014 at 01:30:10PM -0700, Jeremiah Mahler wrote:\n> On Thu, Jun 12, 2014 at 02:38:58PM -0500, Caleb Thompson wrote:\n> > This patch allows people to set commit.verbose to implicitly send\n> > --verbose to git-commit.\n> >\n> > This version incorporates changes suggested by Eric Sunshine, Duy\n> > Nguyen, and Jeremiah Mahler.\n> >\n> > It introduces several cleanup patches to t/t7505-commit-verbose.sh to\n> > bring it closer to the current state of the tests as Eric has explained\n> > them to me, then adds the verbose config and --no-verbose flag.\n> >\n> > Since the last version of this patch\n> > (http://marc.info/?l=git&m=140251155830422&w=2), I've made the following\n> > changes:\n> >\n> > * Revert change to flags, as --no-verbose already existed and worked as\n> > expected with the commit.verbose configuration. Thanks to René Scharfe.\n> > * Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.\n> >\n> > Additionally, this set of patches was generated by format-patch, so it\n> > should work correctly with git-am.\n> >\n> > ------------------------------------------------------\n> >\n> > Caleb Thompson (4):\n> > commit test: Use test_config instead of git-config\n> > commit test: Use write_script\n> > commit test: test_set_editor in each test\n> > commit: add commit.verbose configuration\n> >\n> > Documentation/config.txt | 5 +++\n> > Documentation/git-commit.txt | 8 ++++-\n> > builtin/commit.c | 4 +++\n> > contrib/completion/git-completion.bash | 1 +\n> > t/t7507-commit-verbose.sh | 64 +++++++++++++++++++++++++---------\n> > 5 files changed, 64 insertions(+), 18 deletions(-)\n> >\n> > --\n> > 2.0.0\n> >\n>\n> The patches look good, they apply clean ('git am'), and all tests pass.\n>\n> Reviewed-by: Jeremiah Mahler \n\nSo that I'm clear on the etiquitte, is it appropriate for me to add this\nReviewed-by line to the commit messages at this point, provided that the\npatches don't change?\n\n> --\n> Jeremiah Mahler\n> jmmahler@gmail.com\n> http://github.com/jmahler\n"},{"id":"244153","messageId":"539B3205.5010208@gmail.com","threadId":"36902","inReplyTo":"20140613163644.GB85151@sirius.local","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2014-06-13T17:16:53Z","receivedAt":"2014-06-13T17:16:53Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2014-06-13 18:36, Caleb Thompson pisze:\n> On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:\n\n>> [1] It might make sense for test_set_editor, when run from within a\n>> test, to behave more like test_config, and do:\n>>\n>> test_when_finished '\n>> sane_unset FAKE_EDITOR &&\n>> sane_unset EDITOR\n>> '\n>\n> It might, but it's a little out of scope in addition to your concern\n> about other test scripts.\n>\n>>\n>> I don't know if there would be fallouts with other test scripts,\n>> though.\n>\n> How is this for a reword of that commit description:\n>\n> t/t7507-commit-verbose.sh was using a global test_set_editor call to\n> build its environment. The $EDITOR being used was not necessary for\n> all tests, and was in fact circumvented using subshells in some\n> cases.\n>\n> To improve robustness against global state changes and avoid the\n> use of subshells to temporarily switch the editor, set the editor\n> explicitly wherever it will be important.\n>\n> Specifically, in tests that need to check for the presence of a diff in the\n> editor, make calls to set_test_editor to set $EDITOR to check-for-diff\n> rather than relying on that editor being configured globally. This also\n> helps readers grok the tests as the setup is closer to the verification.\n\nThis also allows to run only specified subset of tests\nwith TEST_SKIP without requiring to remember which tests\nare setup tests and have to be not skipped, isn't it?\n\n-- \nJakub Narębski\n"},{"id":"244156","messageId":"xmqqtx7o3dvh.fsf@gitster.dls.corp.google.com","threadId":"36902","inReplyTo":"20140613065942.GB7908@sigill.intra.peff.net","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-13T17:42:26Z","receivedAt":"2014-06-13T17:42:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [1] It might make sense for test_set_editor, when run from within a\n> test, to behave more like test_config, and do:\n>\n> test_when_finished '\n> sane_unset FAKE_EDITOR &&\n> sane_unset EDITOR\n> '\n>\n> I don't know if there would be fallouts with other test scripts,\n> though.\n\nThe default environment for tests is to set EDITOR=: to avoid\naccidentally triggering interactive cruft and interfering with\nautomated tests, I thought.\n\nIf the above sane-unset is changed to EDITOR=: then I think that is\nprobably sensible.\n"},{"id":"244157","messageId":"20140613174745.GA88614@sirius.local","threadId":"36902","inReplyTo":"539B3205.5010208@gmail.com","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-13T17:47:45Z","receivedAt":"2014-06-13T17:47:45Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"On Fri, Jun 13, 2014 at 07:16:53PM +0200, Jakub Narębski wrote:\n> W dniu 2014-06-13 18:36, Caleb Thompson pisze:\n> >On Fri, Jun 13, 2014 at 02:59:42AM -0400, Jeff King wrote:\n>\n> >>[1] It might make sense for test_set_editor, when run from within a\n> >> test, to behave more like test_config, and do:\n> >>\n> >> test_when_finished '\n> >> sane_unset FAKE_EDITOR &&\n> >> sane_unset EDITOR\n> >> '\n> >\n> >It might, but it's a little out of scope in addition to your concern\n> >about other test scripts.\n> >\n> >>\n> >> I don't know if there would be fallouts with other test scripts,\n> >> though.\n> >\n> >How is this for a reword of that commit description:\n> >\n> > t/t7507-commit-verbose.sh was using a global test_set_editor call to\n> > build its environment. The $EDITOR being used was not necessary for\n> > all tests, and was in fact circumvented using subshells in some\n> > cases.\n> >\n> > To improve robustness against global state changes and avoid the\n> > use of subshells to temporarily switch the editor, set the editor\n> > explicitly wherever it will be important.\n> >\n> > Specifically, in tests that need to check for the presence of a diff in the\n> > editor, make calls to set_test_editor to set $EDITOR to check-for-diff\n> > rather than relying on that editor being configured globally. This also\n> > helps readers grok the tests as the setup is closer to the verification.\n>\n> This also allows to run only specified subset of tests\n> with TEST_SKIP without requiring to remember which tests\n> are setup tests and have to be not skipped, isn't it?\n\nI don't see any references to TEST_SKIP in the code. Do you mean\ntest_skip() from t/test_lib.sh? If so, it isn't clear to me what the use\ncase would be for that, so I'd have to take your word.\n\nCaleb Thompson\n"},{"id":"244158","messageId":"xmqqppic3dko.fsf@gitster.dls.corp.google.com","threadId":"36902","inReplyTo":"1402603225-46240-1-git-send-email-caleb@calebthompson.io","subject":"Re: [PATCH v5 4/4] commit: Add commit.verbose configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-13T17:48:55Z","receivedAt":"2014-06-13T17:48:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb Thompson writes:\n\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 35a4d06..402d6a1 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'\n> \texec grep '^diff --git' \"$1\"\n> EOF\n>\n> +write_script check-for-no-diff <<-'EOF'\n> +\texec grep -v '^diff --git' \"$1\"\n> +EOF\n\nThis lets grep show all lines that are not \"diff --git\" in the\ninput, and as usual grep exits success if it has any line in the\noutput.\n\n $ grep -v '^diff --git' <<\\EOF ; echo $?\n diff --git\n a\n EOF\n a\n 0\n $ exit\n\nWhat are we testing, exactly?\n\n> @@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n> \tcheck_message message\n> '\n>\n> +test_expect_success 'commit shows verbose diff with commit.verbose true' '\n> +\techo morecontent >>file &&\n> +\tgit add file &&\n> +\ttest_config commit.verbose true &&\n> +\ttest_set_editor \"$PWD/check-for-diff\" &&\n> +\tgit commit --amend\n> +'\n> +\n> +test_expect_success 'commit --verbose overrides commit.verbose false' '\n> +\techo evenmorecontent >>file &&\n> +\tgit add file &&\n> +\ttest_config commit.verbose false &&\n> +\ttest_set_editor \"$PWD/check-for-diff\" &&\n> +\tgit commit --amend --verbose\n> +'\n> +\n> +test_expect_success 'commit does not show verbose diff with commit.verbose false' '\n> +\techo evenmorecontent >>file &&\n> +\tgit add file &&\n> +\ttest_config commit.verbose false &&\n> +\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n> +\tgit commit --amend\n> +'\n> +\n> +test_expect_success 'commit --no-verbose overrides commit.verbose true' '\n> +\techo evenmorecontent >>file &&\n> +\tgit add file &&\n> +\ttest_config commit.verbose true &&\n> +\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n> +\tgit commit --amend --no-verbose\n> +'\n> +\n> cat >diff <<'EOF'\n> This is an example commit message that contains a diff.\n>\n> --\n> 2.0.0\n"},{"id":"244164","messageId":"539B487F.4030104@gmail.com","threadId":"36902","inReplyTo":"20140613174745.GA88614@sirius.local","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2014-06-13T18:52:47Z","receivedAt":"2014-06-13T18:52:47Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 2014-06-13 19:47, Caleb Thompson pisze:\n> On Fri, Jun 13, 2014 at 07:16:53PM +0200, Jakub Narębski wrote:\n>> W dniu 2014-06-13 18:36, Caleb Thompson pisze:\n\n>>> t/t7507-commit-verbose.sh was using a global test_set_editor call to\n>>> build its environment. The $EDITOR being used was not necessary for\n>>> all tests, and was in fact circumvented using subshells in some\n>>> cases.\n>>>\n>>> To improve robustness against global state changes and avoid the\n>>> use of subshells to temporarily switch the editor, set the editor\n>>> explicitly wherever it will be important.\n>>>\n>>> Specifically, in tests that need to check for the presence of a diff in the\n>>> editor, make calls to set_test_editor to set $EDITOR to check-for-diff\n>>> rather than relying on that editor being configured globally. This also\n>>> helps readers grok the tests as the setup is closer to the verification.\n>>\n>> This also allows to run only specified subset of tests\n>> with TEST_SKIP without requiring to remember which tests\n>> are setup tests and have to be not skipped, isn't it?\n>\n> I don't see any references to TEST_SKIP in the code. Do you mean\n> test_skip() from t/test_lib.sh? If so, it isn't clear to me what the use\n> case would be for that, so I'd have to take your word.\n\nI meant here GIT_SKIP_TESTS, but I see that test_set_editor was not in \nfirst test i.e. test_expect_success 'setup', so it wouldn't matter.\nBefore and after both work correctly with GIT_SKIP_TESTS, before because\nof global setup.\n\nI'm sorry for the noise, then.\n\n-- \nJakub Narębski\n"},{"id":"244186","messageId":"20140613232841.GC23078@sigill","threadId":"36902","inReplyTo":"20140613162607.GA85151@sirius.local","subject":"Re: [PATCH v5 2/4] commit test: Use write_script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T23:28:42Z","receivedAt":"2014-06-13T23:28:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 11:26:07AM -0500, Caleb Thompson wrote:\n\n> You're very right - I may have confused this commit message and the one\n> to switch to test_set_editor. I'll rewrite this commit message.\n> \n> What do you think of something like this for the description:\n> \n> Use write_script from t/test-lib-functions.sh instead of cat,\n> shebang, and chmod. This aids in readability for creating the script\n> by using the named function and allows us to turn off interpolation\n> in the heredoc of the script body to avoid extra escaping, since\n> $SHELL_PATH is handled for us.\n\nThat looks fine to me. Thanks.\n\n-Peff\n"},{"id":"244187","messageId":"20140613233958.GD23078@sigill","threadId":"36902","inReplyTo":"20140613163644.GB85151@sirius.local","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T23:39:59Z","receivedAt":"2014-06-13T23:39:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 11:36:44AM -0500, Caleb Thompson wrote:\n\n> > Or did you just mean that the new rule is \"every test sets the editor as\n> > they need\", which means that we do not have to worry anymore about\n> > polluting the environment for other tests?\n> \n> That's exactly what I meant. We can stop relying on the global state *as\n> it is initially set* and instead move the setup into the tests which\n> rely on it.\n\nAh, OK, it was just me mis-reading, then.\n\nThe rewording you included below is clearer to me. Thanks.\n\n> > [1] It might make sense for test_set_editor, when run from within a\n> > test, to behave more like test_config, and do:\n> >\n> > test_when_finished '\n> > sane_unset FAKE_EDITOR &&\n> > sane_unset EDITOR\n> > '\n> \n> It might, but it's a little out of scope in addition to your concern\n> about other test scripts.\n\nYeah, I agree it does not need to block this series.\n\n-Peff\n"},{"id":"244188","messageId":"20140613234128.GE23078@sigill","threadId":"36902","inReplyTo":"xmqqtx7o3dvh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T23:41:29Z","receivedAt":"2014-06-13T23:41:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > [1] It might make sense for test_set_editor, when run from within a\n> > test, to behave more like test_config, and do:\n> >\n> > test_when_finished '\n> > sane_unset FAKE_EDITOR &&\n> > sane_unset EDITOR\n> > '\n> >\n> > I don't know if there would be fallouts with other test scripts,\n> > though.\n> \n> The default environment for tests is to set EDITOR=: to avoid\n> accidentally triggering interactive cruft and interfering with\n> automated tests, I thought.\n\nAh, yeah, that would make more sense.\n\n> If the above sane-unset is changed to EDITOR=: then I think that is\n> probably sensible.\n\nI think the trick is that other scripts may be relying on the global\nside-effect, and would need to be fixed up (and it is not always obvious\nwhich spots will need it; they might fail the tests, or they might start\nsilently passing for the wrong reason).\n\n-Peff\n"},{"id":"244190","messageId":"20140614041452.GA1375@hudson.localdomain","threadId":"36902","inReplyTo":"20140613164910.GA87252@sirius.local","subject":"Re: [PATCH v5 0/4] commit: Add commit.verbose configuration","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-14T04:14:52Z","receivedAt":"2014-06-14T04:14:52Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Fri, Jun 13, 2014 at 11:49:10AM -0500, Caleb Thompson wrote:\n> On Thu, Jun 12, 2014 at 01:30:10PM -0700, Jeremiah Mahler wrote:\n> > On Thu, Jun 12, 2014 at 02:38:58PM -0500, Caleb Thompson wrote:\n> > > This patch allows people to set commit.verbose to implicitly send\n> > > --verbose to git-commit.\n> > >\n> > > This version incorporates changes suggested by Eric Sunshine, Duy\n> > > Nguyen, and Jeremiah Mahler.\n> > >\n> > > It introduces several cleanup patches to t/t7505-commit-verbose.sh to\n> > > bring it closer to the current state of the tests as Eric has explained\n> > > them to me, then adds the verbose config and --no-verbose flag.\n> > >\n> > > Since the last version of this patch\n> > > (http://marc.info/?l=git&m=140251155830422&w=2), I've made the following\n> > > changes:\n> > >\n> > > * Revert change to flags, as --no-verbose already existed and worked as\n> > > expected with the commit.verbose configuration. Thanks to René Scharfe.\n> > > * Fix <<-'EOS' style for check-for-no-diff script. Thanks to Mike Burns.\n> > >\n> > > Additionally, this set of patches was generated by format-patch, so it\n> > > should work correctly with git-am.\n> > >\n> > > ------------------------------------------------------\n> > >\n> > > Caleb Thompson (4):\n> > > commit test: Use test_config instead of git-config\n> > > commit test: Use write_script\n> > > commit test: test_set_editor in each test\n> > > commit: add commit.verbose configuration\n> > >\n> > > Documentation/config.txt | 5 +++\n> > > Documentation/git-commit.txt | 8 ++++-\n> > > builtin/commit.c | 4 +++\n> > > contrib/completion/git-completion.bash | 1 +\n> > > t/t7507-commit-verbose.sh | 64 +++++++++++++++++++++++++---------\n> > > 5 files changed, 64 insertions(+), 18 deletions(-)\n> > >\n> > > --\n> > > 2.0.0\n> > >\n> >\n> > The patches look good, they apply clean ('git am'), and all tests pass.\n> >\n> > Reviewed-by: Jeremiah Mahler \n> \n> So that I'm clear on the etiquitte, is it appropriate for me to add this\n> Reviewed-by line to the commit messages at this point, provided that the\n> patches don't change?\n> \n\nI am not sure of the etiquette either. But personally, since I hardly\ncontributed anything, I don't think it is necessary to put my tag in\nyour patches.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244272","messageId":"20140616174640.GA28126@sirius.local","threadId":"36902","inReplyTo":"20140613234128.GE23078@sigill","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-16T17:46:40Z","receivedAt":"2014-06-16T17:46:40Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"On Fri, Jun 13, 2014 at 07:41:29PM -0400, Jeff King wrote:\n> On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > > [1] It might make sense for test_set_editor, when run from within a\n> > > test, to behave more like test_config, and do:\n> > >\n> > > test_when_finished '\n> > > sane_unset FAKE_EDITOR &&\n> > > sane_unset EDITOR\n> > > '\n> > >\n> > > I don't know if there would be fallouts with other test scripts,\n> > > though.\n> >\n> > The default environment for tests is to set EDITOR=: to avoid\n> > accidentally triggering interactive cruft and interfering with\n> > automated tests, I thought.\n>\n> Ah, yeah, that would make more sense.\n>\n> > If the above sane-unset is changed to EDITOR=: then I think that is\n> > probably sensible.\n>\n> I think the trick is that other scripts may be relying on the global\n> side-effect, and would need to be fixed up (and it is not always obvious\n> which spots will need it; they might fail the tests, or they might start\n> silently passing for the wrong reason).\n\nFor this reason, and that the scope of this change has already ballooned, I'd\nrather not make this change in this patch if that's alright.\n\nCaleb Thompson\n"},{"id":"244332","messageId":"xmqq4mzkr89c.fsf@gitster.dls.corp.google.com","threadId":"36902","inReplyTo":"20140616174640.GA28126@sirius.local","subject":"Re: [PATCH v5 3/4] commit test: test_set_editor in each test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T18:58:55Z","receivedAt":"2014-06-16T18:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb Thompson writes:\n\n> On Fri, Jun 13, 2014 at 07:41:29PM -0400, Jeff King wrote:\n>> On Fri, Jun 13, 2014 at 10:42:26AM -0700, Junio C Hamano wrote:\n>>\n>> > Jeff King writes:\n>> >\n>> > > [1] It might make sense for test_set_editor, when run from within a\n>> > > test, to behave more like test_config, and do:\n>> > >\n>> > > test_when_finished '\n>> > > sane_unset FAKE_EDITOR &&\n>> > > sane_unset EDITOR\n>> > > '\n>> > >\n>> > > I don't know if there would be fallouts with other test scripts,\n>> > > though.\n>> >\n>> > The default environment for tests is to set EDITOR=: to avoid\n>> > accidentally triggering interactive cruft and interfering with\n>> > automated tests, I thought.\n>>\n>> Ah, yeah, that would make more sense.\n>>\n>> > If the above sane-unset is changed to EDITOR=: then I think that is\n>> > probably sensible.\n>>\n>> I think the trick is that other scripts may be relying on the global\n>> side-effect, and would need to be fixed up (and it is not always obvious\n>> which spots will need it; they might fail the tests, or they might start\n>> silently passing for the wrong reason).\n>\n> For this reason, and that the scope of this change has already ballooned, I'd\n> rather not make this change in this patch if that's alright.\n>\n> Caleb Thompson\n\nMy comment was not about your series, but \"if we were to update\ntest_set_editor, unsetting EDITOR is not the right thing to do\".\n\nI do not think it is reasonable to include such a change to\ntest_set_editor in this series.\n"},{"id":"244342","messageId":"20140616195057.GB28126@sirius.local","threadId":"36902","inReplyTo":"xmqqppic3dko.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 4/4] commit: Add commit.verbose configuration","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-16T19:50:57Z","receivedAt":"2014-06-16T19:50:57Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:\n> Caleb Thompson writes:\n>\n> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> > index 35a4d06..402d6a1 100755\n> > --- a/t/t7507-commit-verbose.sh\n> > +++ b/t/t7507-commit-verbose.sh\n> > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'\n> >\t\texec grep '^diff --git' \"$1\"\n> > EOF\n> >\n> > +write_script check-for-no-diff <<-'EOF'\n> > +\texec grep -v '^diff --git' \"$1\"\n> > +EOF\n>\n> This lets grep show all lines that are not \"diff --git\" in the\n> input, and as usual grep exits success if it has any line in the\n> output.\n>\n> $ grep -v '^diff --git' <<\\EOF ; echo $?\n> diff --git\n> a\n> EOF\n> a\n> 0\n> $ exit\n>\n> What are we testing, exactly?\n\nGood catch. It worked when I switched check-for-diff from\ncheck-for-no-diff, but I didn't try to make check-for-no-diff fail\nindependently, so I apologize.\n\nThis version removes the the beginning of a line starting with\n\"diff --git\" from the string, then checks that the result and the\noriginal string are not the same. Switching the != logic to = makes the\ntests using check-for-no-diff fail.\n\n\twrite_script check-for-no-diff <<-'EOF'\n\t\texec test \"${1#*^diff --git} != $1\n\tEOF\n\nAnother option is to replace the parameter substitution with a call to\ngrep:\n\n\twrite_script check-for-no-diff <<-'EOF'\n\t\texec test \"`grep -v '^diff --git' \\\"$1\\\"` != \"$1\n\tEOF\n\nI think that the former reads nicer, and requires less escaping, but I'm\nopen to feedback.\n\n\n> > @@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n> >\t\tcheck_message message\n> > '\n> >\n> > +test_expect_success 'commit shows verbose diff with commit.verbose true' '\n> > +\techo morecontent >>file &&\n> > +\tgit add file &&\n> > +\ttest_config commit.verbose true &&\n> > +\ttest_set_editor \"$PWD/check-for-diff\" &&\n> > +\tgit commit --amend\n> > +'\n> > +\n> > +test_expect_success 'commit --verbose overrides commit.verbose false' '\n> > +\techo evenmorecontent >>file &&\n> > +\tgit add file &&\n> > +\ttest_config commit.verbose false &&\n> > +\ttest_set_editor \"$PWD/check-for-diff\" &&\n> > +\tgit commit --amend --verbose\n> > +'\n> > +\n> > +test_expect_success 'commit does not show verbose diff with commit.verbose false' '\n> > +\techo evenmorecontent >>file &&\n> > +\tgit add file &&\n> > +\ttest_config commit.verbose false &&\n> > +\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n> > +\tgit commit --amend\n> > +'\n> > +\n> > +test_expect_success 'commit --no-verbose overrides commit.verbose true' '\n> > +\techo evenmorecontent >>file &&\n> > +\tgit add file &&\n> > +\ttest_config commit.verbose true &&\n> > +\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n> > +\tgit commit --amend --no-verbose\n> > +'\n> > +\n> > cat >diff <<'EOF'\n> > This is an example commit message that contains a diff.\n> >\n> > --\n> > 2.0.0\n\nCaleb Thompson\n"},{"id":"244345","messageId":"20140616200558.GA37769@sirius.local","threadId":"36902","inReplyTo":"20140616195057.GB28126@sirius.local","subject":"Re: [PATCH v5 4/4] commit: Add commit.verbose configuration","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-16T20:05:58Z","receivedAt":"2014-06-16T20:05:58Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"On Mon, Jun 16, 2014 at 02:50:57PM -0500, Caleb Thompson wrote:\n> On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:\n> > Caleb Thompson writes:\n> >\n> > > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> > > index 35a4d06..402d6a1 100755\n> > > --- a/t/t7507-commit-verbose.sh\n> > > +++ b/t/t7507-commit-verbose.sh\n> > > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'\n> > >\t\texec grep '^diff --git' \"$1\"\n> > > EOF\n> > >\n> > > +write_script check-for-no-diff <<-'EOF'\n> > > +\texec grep -v '^diff --git' \"$1\"\n> > > +EOF\n> >\n> > This lets grep show all lines that are not \"diff --git\" in the\n> > input, and as usual grep exits success if it has any line in the\n> > output.\n> >\n> > $ grep -v '^diff --git' <<\\EOF ; echo $?\n> > diff --git\n> > a\n> > EOF\n> > a\n> > 0\n> > $ exit\n> >\n> > What are we testing, exactly?\n>\n> Good catch. It worked when I switched check-for-diff from\n> check-for-no-diff, but I didn't try to make check-for-no-diff fail\n> independently, so I apologize.\n>\n> This version removes the the beginning of a line starting with\n> \"diff --git\" from the string, then checks that the result and the\n> original string are not the same. Switching the != logic to = makes the\n> tests using check-for-no-diff fail.\n>\n>\twrite_script check-for-no-diff <<-'EOF'\n>\t\texec test \"${1#*^diff --git} != $1\n>\tEOF\n>\n> Another option is to replace the parameter substitution with a call to\n> grep:\n>\n>\twrite_script check-for-no-diff <<-'EOF'\n>\t\texec test \"`grep -v '^diff --git' \\\"$1\\\"` != \"$1\n>\tEOF\n>\n> I think that the former reads nicer, and requires less escaping, but I'm\n> open to feedback.\n\nCorrection: the first variant does not work, only the second. Sorry for the\nconfustion.\n\n\twrite_script check-for-no-diff <<-'EOF'\n\t\texec test \"`grep -v '^diff --git' \\\"$1\\\"`\" != \"$1\"\n\tEOF\n\n> > > @@ -48,6 +52,38 @@ test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '\n> > >\t\tcheck_message message\n> > > '\n> > >\n> > > +test_expect_success 'commit shows verbose diff with commit.verbose true' '\n> > > +\techo morecontent >>file &&\n> > > +\tgit add file &&\n> > > +\ttest_config commit.verbose true &&\n> > > +\ttest_set_editor \"$PWD/check-for-diff\" &&\n> > > +\tgit commit --amend\n> > > +'\n> > > +\n> > > +test_expect_success 'commit --verbose overrides commit.verbose false' '\n> > > +\techo evenmorecontent >>file &&\n> > > +\tgit add file &&\n> > > +\ttest_config commit.verbose false &&\n> > > +\ttest_set_editor \"$PWD/check-for-diff\" &&\n> > > +\tgit commit --amend --verbose\n> > > +'\n> > > +\n> > > +test_expect_success 'commit does not show verbose diff with commit.verbose false' '\n> > > +\techo evenmorecontent >>file &&\n> > > +\tgit add file &&\n> > > +\ttest_config commit.verbose false &&\n> > > +\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n> > > +\tgit commit --amend\n> > > +'\n> > > +\n> > > +test_expect_success 'commit --no-verbose overrides commit.verbose true' '\n> > > +\techo evenmorecontent >>file &&\n> > > +\tgit add file &&\n> > > +\ttest_config commit.verbose true &&\n> > > +\ttest_set_editor \"$PWD/check-for-no-diff\" &&\n> > > +\tgit commit --amend --no-verbose\n> > > +'\n> > > +\n> > > cat >diff <<'EOF'\n> > > This is an example commit message that contains a diff.\n> > >\n> > > --\n> > > 2.0.0\n>\n> Caleb Thompson\n\n\n"},{"id":"244346","messageId":"xmqqzjhcpqju.fsf@gitster.dls.corp.google.com","threadId":"36902","inReplyTo":"20140616195057.GB28126@sirius.local","subject":"Re: [PATCH v5 4/4] commit: Add commit.verbose configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T20:06:45Z","receivedAt":"2014-06-16T20:06:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb Thompson writes:\n\n> On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:\n>> Caleb Thompson writes:\n>>\n>> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n>> > index 35a4d06..402d6a1 100755\n>> > --- a/t/t7507-commit-verbose.sh\n>> > +++ b/t/t7507-commit-verbose.sh\n>> > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'\n>> >\t\texec grep '^diff --git' \"$1\"\n>> > EOF\n>> >\n>> > +write_script check-for-no-diff <<-'EOF'\n>> > +\texec grep -v '^diff --git' \"$1\"\n>> > +EOF\n>>\n>> This lets grep show all lines that are not \"diff --git\" in the\n>> input, and as usual grep exits success if it has any line in the\n>> output.\n>>\n>> $ grep -v '^diff --git' <<\\EOF ; echo $?\n>> diff --git\n>> a\n>> EOF\n>> a\n>> 0\n>> $ exit\n>>\n>> What are we testing, exactly?\n>\n> Good catch. It worked when I switched check-for-diff from\n> check-for-no-diff, but I didn't try to make check-for-no-diff fail\n> independently, so I apologize.\n\nNo need to apologize at all. None of us (including this reviewer)\nis perfect and that is why we review patches by each other.\n\n> This version removes the the beginning of a line starting with\n> \"diff --git\" from the string,...\n\nAgain, what are we testing, exactly?\n\nWe do not want to see \"^diff --git\" in the output file, in other\nwords, we want to make sure \"^diff --git\" does not appear in the\noutput.\n\nSo\n\n write_script check-for-no-diff <<-\\EOF\n ! grep '^diff --git' \"$@\"\n\tEOF\n\nshould be the most natural way to express what we are testing, no?\n"},{"id":"244347","messageId":"20140616201037.GA37953@sirius.local","threadId":"36902","inReplyTo":"xmqqzjhcpqju.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 4/4] commit: Add commit.verbose configuration","fromName":"Caleb Thompson","fromEmail":"caleb@calebthompson.io","sentAt":"2014-06-16T20:10:37Z","receivedAt":"2014-06-16T20:10:37Z","isPatch":true,"sender":{"key":"caleb@calebthompson.io","avatar":"https://gravatar.com/avatar/0440c826b3dfa9dd7f768e4b661935a3a2584612a2fcd261f6914ced0457ab4d?d=mp&s=160"},"body":"On Mon, Jun 16, 2014 at 01:06:45PM -0700, Junio C Hamano wrote:\n> Caleb Thompson writes:\n>\n> > On Fri, Jun 13, 2014 at 10:48:55AM -0700, Junio C Hamano wrote:\n> >> Caleb Thompson writes:\n> >>\n> >> > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> >> > index 35a4d06..402d6a1 100755\n> >> > --- a/t/t7507-commit-verbose.sh\n> >> > +++ b/t/t7507-commit-verbose.sh\n> >> > @@ -7,6 +7,10 @@ write_script check-for-diff <<-'EOF'\n> >> >\t\texec grep '^diff --git' \"$1\"\n> >> > EOF\n> >> >\n> >> > +write_script check-for-no-diff <<-'EOF'\n> >> > +\texec grep -v '^diff --git' \"$1\"\n> >> > +EOF\n> >>\n> >> This lets grep show all lines that are not \"diff --git\" in the\n> >> input, and as usual grep exits success if it has any line in the\n> >> output.\n> >>\n> >> $ grep -v '^diff --git' <<\\EOF ; echo $?\n> >> diff --git\n> >> a\n> >> EOF\n> >> a\n> >> 0\n> >> $ exit\n> >>\n> >> What are we testing, exactly?\n> >\n> > Good catch. It worked when I switched check-for-diff from\n> > check-for-no-diff, but I didn't try to make check-for-no-diff fail\n> > independently, so I apologize.\n>\n> No need to apologize at all. None of us (including this reviewer)\n> is perfect and that is why we review patches by each other.\n>\n> > This version removes the the beginning of a line starting with\n> > \"diff --git\" from the string,...\n>\n> Again, what are we testing, exactly?\n>\n> We do not want to see \"^diff --git\" in the output file, in other\n> words, we want to make sure \"^diff --git\" does not appear in the\n> output.\n>\n> So\n>\n> write_script check-for-no-diff <<-\\EOF\n> ! grep '^diff --git' \"$@\"\n>\tEOF\n>\n> should be the most natural way to express what we are testing, no?\n\nI did consider that. The reason I didn't propose that is that it doesn't catch\nthe unlikely case that the $1 only contains a \"diff --git\" line or that $1 is\nempty.\n\nThose are rather unreasonable concerns, so I'm happy to use the much more\nreadable version as you propose.\n\nCaleb Thompson\n"},{"id":"244353","messageId":"xmqqsin4ppjw.fsf@gitster.dls.corp.google.com","threadId":"36902","inReplyTo":"20140614041452.GA1375@hudson.localdomain","subject":"Re: [PATCH v5 0/4] commit: Add commit.verbose configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T20:28:19Z","receivedAt":"2014-06-16T20:28:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler writes:\n\n> On Fri, Jun 13, 2014 at 11:49:10AM -0500, Caleb Thompson wrote:\n> ...\n>> > The patches look good, they apply clean ('git am'), and all tests pass.\n>> >\n>> > Reviewed-by: Jeremiah Mahler \n>> \n>> So that I'm clear on the etiquitte, is it appropriate for me to add this\n>> Reviewed-by line to the commit messages at this point, provided that the\n>> patches don't change?\n>\n> I am not sure of the etiquette either. But personally, since I hardly\n> contributed anything, I don't think it is necessary to put my tag in\n> your patches.\n\nIt is correct that it is not necessary for Caleb to resend these\npatches if it is done only to add \"Reviewed-by:\" from you (but it\ndoes not hurt to do so, either).\n\nIn the review process, \"Reviewed-by:\" sent for a patch by one of the\ntrusted reviewers will reduce workload from other reviewers because\nthere will be one less reason to read such a patch [*1*]. A\ncorollary to this is that other reviewers will ignore an extra\n\"Reviewed-by:\" by somebody whose review quality is still unknown, so\nit will not hurt.\n\nYour reading others patches and commenting on is appreciated very\nmuch, and sending a \"Reviewed-by:\" is perfectly fine. As you gain\nexperience and as others see your review comments more and more,\npeople will start trusting your reviews. Everybody begins at a\n\"novice\" state, after all ;-)\n\nBy the way, speaking of netiquette, please refrain from using\n\"Mail-Followup-To:\" while working on this list [*2*].\n\nThanks.\n\n[Footnotes]\n\n*1* We usually read a patch for one of the two reasons: either the\nreviewer personally finds what the patch wants to do interesting and\nworthwhile, and wants to make sure it is done in the right way. Or\nthe reviewer thinks that applying the patch is detrimental to the\noverall project, perhaps the design and/or the implementation is\nwrong, and point the problems out. A \"Reviewed-by\" from a trusted\nreviewer will allow other reviewers to simply skip/ignore the patch\nif what it does is \"Meh\" to them, saying \"I am not particularly\ninterested, but as long as it does not hurt, I would not be opposed,\nand the other guy reviewed and says it would not hurt, and I tend to\ntrust his judgment.\" The maintainer does not have the luxury of\nskip/ignore such a patch because he needs to at least apply and test\nthe integration result, though ;-).\n\nNow, who are the \"trusted reviewers\"? Anybody who is knows s/he is\none of them ;-)\n\n*2* http://thread.gmane.org/gmane.comp.version-control.git/165477/focus=165549\n"},{"id":"244360","messageId":"xmqq38f4pk4t.fsf@gitster.dls.corp.google.com","threadId":"36902","inReplyTo":"20140616201037.GA37953@sirius.local","subject":"Re: [PATCH v5 4/4] commit: Add commit.verbose configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T22:25:22Z","receivedAt":"2014-06-16T22:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Caleb Thompson writes:\n\n>> Again, what are we testing, exactly?\n>>\n>> We do not want to see \"^diff --git\" in the output file, in other\n>> words, we want to make sure \"^diff --git\" does not appear in the\n>> output.\n>>\n>> So\n>>\n>> write_script check-for-no-diff <<-\\EOF\n>> ! grep '^diff --git' \"$@\"\n>>\tEOF\n>>\n>> should be the most natural way to express what we are testing, no?\n>\n> I did consider that. The reason I didn't propose that is that it doesn't catch\n> the unlikely case that the $1 only contains a \"diff --git\" line or that $1 is\n> empty.\n>\n> Those are rather unreasonable concerns, so I'm happy to use the much more\n> readable version as you propose.\n\nIf it only has \"diff --git\", then the grep will find a hit, exits\nwith success, the script yields the opposite and \"git commit\" will\nfail, which is what we want, so that is OK. \"$1 is empty\" may or\nmay not be an error, depending on your settings, I guess (i.e. can't\nwe squelch the \"# helpful instruction\" lines altogether?)?\n\nIf the editor input is expected to be very stable, we could even do\nsomething like:\n\n\twrite_script check-editor-input <<-\\EOF\n\tdiff expect \"$1\" >&2\n EOF\n\nand then catch any deviation from the norm with something like:\n\n\tcat >expect <<-\\EOF &&\n ... expected editor input comes here ...\n EOF\n test_set_editor \"$PWD/check-editor-input &&\n\tgit commit --amend\n\nbut if the editor input may easily be affected by volatile things\nlike blob object names given in the diff output by \"commit -v\" or\nuntracked cruft in the working tree listed in the status output,\nthen it would add unnecessary maintenance burden (every time earlier\nparts of the test scripts are updated, the expected output may have\nto change to adjust to these non-essential details), so it is a\njudgment call.\n\nThanks.\n"}]}