{"thread":{"id":"41667","subject":"[PATCH v4] commit: add a commit.verbose config variable","startedAt":"2016-03-10T22:12:07Z","lastAt":"2016-03-11T18:39:56Z","messageCount":13,"participants":["Pranit Bauva","Junio C Hamano","Eric Sunshine","Roberto Tyley","Philip Oakley"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"280585","messageId":"0102015362963f6a-626ad27e-e705-4a75-93a6-55a5f75d0226-000000@eu-west-1.amazonses.com","threadId":"41667","inReplyTo":null,"subject":"[PATCH v4] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-10T22:12:07Z","receivedAt":"2016-03-10T22:12:07Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Since many people always run the command with this option, it would be\npreferrable to specify it in the configuration file instead of passing\nthe option with `git commit` again and again.\n\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n---\n Documentation/config.txt     |  4 ++++\n Documentation/git-commit.txt |  3 ++-\n builtin/commit.c             |  4 ++++\n t/t7507-commit-verbose.sh    | 48 ++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 01cca0a..9b93f6c 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 to specify whether to always include the verbose option\n+\twith `git commit`. See 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..3dcaac7 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. To activate this option permanently, the\n+\tconfiguration variable `commit.verbose` can be set to true.\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..55e9a82 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1505,6 +1505,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/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex 2ddf28c..4e123a5 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -96,4 +96,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 with commit.verbose true and no arguments' '\n+\techo content >file &&\n+\tgit add file &&\n+\ttest_config commit.verbose true &&\n+\t(\n+\t\tGIT_EDITOR=cat &&\n+\t\texport GIT_EDITOR &&\n+\t\ttest_must_fail git commit >output\n+\t) &&\n+\ttest_i18ngrep \"diff --git\" output\n+'\n+\n+test_expect_success 'commit with commit.verbose true and --no-verbose' '\n+\techo content >file &&\n+\tgit add file &&\n+\ttest_config commit.verbose true &&\n+\t(\n+\t\tGIT_EDITOR=cat &&\n+\t\texport GIT_EDITOR &&\n+\t\ttest_must_fail git commit --no-verbose >output\n+\t) &&\n+\t! test_i18ngrep \"diff --git\" output\n+'\n+\n+test_expect_success 'commit with commit.verbose false and -v' '\n+\techo content >file &&\n+\tgit add file &&\n+\ttest_config commit.verbose false &&\n+\t(\n+\t\tGIT_EDITOR=cat &&\n+\t\texport GIT_EDITOR &&\n+\t\ttest_must_fail git commit -v >output\n+\t) &&\n+\ttest_i18ngrep \"diff --git\" output\n+'\n+\n+test_expect_success 'commit with commit.verbose false no arguments' '\n+\techo content >file &&\n+\tgit add file &&\n+\ttest_config commit.verbose false &&\n+\t(\n+\t\tGIT_EDITOR=cat &&\n+\t\texport GIT_EDITOR &&\n+\t\ttest_must_fail git commit >output\n+\t) &&\n+\t! test_i18ngrep \"diff --git\" output\n+'\n+\n test_done\n\n--\nhttps://github.com/git/git/pull/205\n"},{"id":"280587","messageId":"CAFZEwPOfK4qOQ5+8vVaspx+LM+QcG+gw2uKtyjQEu7jWdHxTGA@mail.gmail.com","threadId":"41667","inReplyTo":"0102015362963f6a-626ad27e-e705-4a75-93a6-55a5f75d0226-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-10T22:39:08Z","receivedAt":"2016-03-10T22:39:08Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Older versions of this patch can be found at :-\n\n[v3] : http://thread.gmane.org/gmane.comp.version-control.git/288634\n\n[v2] : http://thread.gmane.org/gmane.comp.version-control.git/288569\n\n[v1] : http://thread.gmane.org/gmane.comp.version-control.git/287540\n\nThe changes with respect to last version are :\n - put the code in git_commit_config() instead of git_status_config()\n - Add test to cover all possible scenarios\n\nRegards,\nPranit Bauva\nIIT Kharagpur\n"},{"id":"280590","messageId":"xmqq60wuc5ev.fsf@gitster.mtv.corp.google.com","threadId":"41667","inReplyTo":"0102015362963f6a-626ad27e-e705-4a75-93a6-55a5f75d0226-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-10T22:52:08Z","receivedAt":"2016-03-10T22:52:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva <pranit.bauva@gmail.com> writes:\n\n> +\tif (!strcmp(k, \"commit.verbose\")){\n\nv3 did this line correctly but you somehow lost the SP between\n\"){\".  What happened?\n\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> index 2ddf28c..4e123a5 100755\n> --- a/t/t7507-commit-verbose.sh\n> +++ b/t/t7507-commit-verbose.sh\n> @@ -96,4 +96,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 with commit.verbose true and no arguments' '\n> +test_expect_success 'commit with commit.verbose true and --no-verbose' '\n> +test_expect_success 'commit with commit.verbose false and -v' '\n> +test_expect_success 'commit with commit.verbose false no arguments' '\n\nDon't you need a test that status is not broken when the variable is\nset?\n"},{"id":"280591","messageId":"CAPig+cT+dmD2Nxw7z+x0Q2z_aJQFMwRwnb=dn1uH-_Nt9tnk4Q@mail.gmail.com","threadId":"41667","inReplyTo":"0102015362963f6a-626ad27e-e705-4a75-93a6-55a5f75d0226-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-10T23:01:44Z","receivedAt":"2016-03-10T23:01:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 10, 2016 at 5:12 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> Since many people always run the command with this option, it would be\n> preferrable to specify it in the configuration file instead of passing\n> the option with `git commit` again and again.\n\nPerhaps drop the unsubstantiated \"many people always\" and just say:\n\n    Add commit.verbose configuration variable as a convenience\n    for those who always prefer --verbose.\n\nor something.\n\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> ---\n\nAs a convenience to reviewers, please use this area below the \"---\"\nline to provide links and explain what changed since the previous\nround rather than doing so in a separate email.\n\n> diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt\n> @@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1].\n>         what changes the commit has.\n>         Note that this diff output doesn't have its\n>         lines prefixed with '#'. This diff will not be a part\n> -       of the commit message.\n> +       of the commit message. To activate this option permanently, the\n> +       configuration variable `commit.verbose` can be set to true.\n\nThe \"permanently\" bit sounds scary. A more concise way to state this might be:\n\n    See the `commit.verbose` configuration variable in\n    linkgit:git-config[1].\n\nwhich doesn't bother spelling out what the intelligent reader should\ninfer from the reference.\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> @@ -1505,6 +1505,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n>                 sign_commit = git_config_bool(k, v) ? \"\" : NULL;\n>                 return 0;\n>         }\n> +       if (!strcmp(k, \"commit.verbose\")){\n\nStyle: space before {\n\n> +               verbose = git_config_bool(k, v);\n> +               return 0;\n> +       }\n>\n>         status = git_gpg_config(k, v, NULL);\n>         if (status)\n> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\n> @@ -96,4 +96,52 @@ 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 'commit with commit.verbose true and no arguments' '\n\n\"no arguments\" doesn't convey much; how about \"--verbose omitted\" or\nsomething? Ditto for the titles of other tests.\n\n> +       echo content >file &&\n> +       git add file &&\n> +       test_config commit.verbose true &&\n> +       (\n> +               GIT_EDITOR=cat &&\n> +               export GIT_EDITOR &&\n> +               test_must_fail git commit >output\n> +       ) &&\n> +       test_i18ngrep \"diff --git\" output\n> +'\n\nMaking git-commit fail unconditionally with \"aborting due to empty\ncommit message\" is a rather sneaky way to perform this test. I would\nhave expected to see these new tests re-use the existing machinery\nprovided by this script (the check-for-diff \"editor\") rather than\ninventing an entirely new and unintuitive mechanism. Doing so would\nalso reduce the size of each new test.\n\nMore below...\n\n> +test_expect_success 'commit with commit.verbose true and --no-verbose' '\n> +       echo content >file &&\n> +       git add file &&\n> +       test_config commit.verbose true &&\n> +       (\n> +               GIT_EDITOR=cat &&\n> +               export GIT_EDITOR &&\n> +               test_must_fail git commit --no-verbose >output\n> +       ) &&\n> +       ! test_i18ngrep \"diff --git\" output\n> +'\n> +\n> +test_expect_success 'commit with commit.verbose false and -v' '\n> +       echo content >file &&\n> +       git add file &&\n> +       test_config commit.verbose false &&\n> +       (\n> +               GIT_EDITOR=cat &&\n> +               export GIT_EDITOR &&\n> +               test_must_fail git commit -v >output\n> +       ) &&\n> +       test_i18ngrep \"diff --git\" output\n> +'\n> +\n> +test_expect_success 'commit with commit.verbose false no arguments' '\n> +       echo content >file &&\n> +       git add file &&\n> +       test_config commit.verbose false &&\n> +       (\n> +               GIT_EDITOR=cat &&\n> +               export GIT_EDITOR &&\n> +               test_must_fail git commit >output\n> +       ) &&\n> +       ! test_i18ngrep \"diff --git\" output\n> +'\n\nSome additional tests[1][2] are probably warranted.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/288648\n[2]: http://article.gmane.org/gmane.comp.version-control.git/288657\n\n> +\n>  test_done\n"},{"id":"280592","messageId":"CAFZEwPNmMB5ggCFRV913wEhGTkZaGLaJgY9b_Rm_2Ho866fiUA@mail.gmail.com","threadId":"41667","inReplyTo":"xmqq60wuc5ev.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-10T23:02:19Z","receivedAt":"2016-03-10T23:02:19Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"> +     if (!strcmp(k, \"commit.verbose\")){\n\n> v3 did this line correctly but you somehow lost the SP between\n> \"){\".  What happened?\n\nI will include the SP between )) and { .\n\n> Don't you need a test that status is not broken when the variable is\n> set?\n\nI will include the test for status too. But I am a bit confused where\nshould I place them? This patch triggers the possibility of a breakage\nin status related to verbose but other tests related to status are in\ndifferent files. Could you tell me the filename where I should place\nthese test?\n"},{"id":"280598","messageId":"CAFZEwPMznAUBhgJgZ7aRvtH1W8hDYLci6khbw9EsWS9WGhSh=Q@mail.gmail.com","threadId":"41667","inReplyTo":"CAPig+cT+dmD2Nxw7z+x0Q2z_aJQFMwRwnb=dn1uH-_Nt9tnk4Q@mail.gmail.com","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-11T00:15:27Z","receivedAt":"2016-03-11T00:15:27Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n>     Add commit.verbose configuration variable as a convenience\n>     for those who always prefer --verbose.\n>\n> or something.\n\nSure!\n\n> As a convenience to reviewers, please use this area below the \"---\"\n> line to provide links and explain what changed since the previous\n> round rather than doing so in a separate email.\n\nActually I am sending the patches with submitGit herokuapp because my\ninstitute proxy does not allow IMAP/POP3 connections.\n\n\n> The \"permanently\" bit sounds scary. A more concise way to state this might be:\n>\n>     See the `commit.verbose` configuration variable in\n>     linkgit:git-config[1].\n>\n> which doesn't bother spelling out what the intelligent reader should\n> infer from the reference.\n> Style: space before {\n\nSure!\n\n>> +test_expect_success 'commit with commit.verbose true and no arguments' '\n>\n> \"no arguments\" doesn't convey much; how about \"--verbose omitted\" or\n> something? Ditto for the titles of other tests.\n\nWill change the language construct.\n>> +       echo content >file &&\n>> +       git add file &&\n>> +       test_config commit.verbose true &&\n>> +       (\n>> +               GIT_EDITOR=cat &&\n>> +               export GIT_EDITOR &&\n>> +               test_must_fail git commit >output\n>> +       ) &&\n>> +       test_i18ngrep \"diff --git\" output\n>> +'\n>\n> Making git-commit fail unconditionally with \"aborting due to empty\n> commit message\" is a rather sneaky way to perform this test. I would\n> have expected to see these new tests re-use the existing machinery\n> provided by this script (the check-for-diff \"editor\") rather than\n> inventing an entirely new and unintuitive mechanism. Doing so would\n> also reduce the size of each new test.\n\nI agree on the fact that making git-commit fail unconditionally is not\na good way to perform the test. \"check-for-diff\" is not really an\n\"editor\" and it checks for the commit message after it has been\nwritten to the history. The verbose output is stripped when it is\nwritten to the history so we won't be able to test whether this patch\nworks. This is where purposely breaking the code is required as when\nthe commit fails, it gives the output of the contents present at that\ntime (which will contain the verbose output). More over the\n'check-for-diff' uses grep which is not preferred. Many tests are now\nusing test_i18ngrep (eg. f79ce8db). I had planned on using\n'check-for-diff' before but it took me some time to figure out this\nbehavior and thus I began searching for another mechanism (breaking\ncode).\n\n> Some additional tests[1][2] are probably warranted.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/288648\n> [2]: http://article.gmane.org/gmane.comp.version-control.git/288657\n\nI think these tests also are better included in this file as this\npatch triggers it and it would not make much of a difference between\nt7507 and t7502 but in fact improve its readability.\n>>  test_done\n"},{"id":"280601","messageId":"010201536326c56b-cd751f0b-f03f-4c9f-b834-20cee14b6eb5-000000@eu-west-1.amazonses.com","threadId":"41667","inReplyTo":"0102015362963f6a-626ad27e-e705-4a75-93a6-55a5f75d0226-000000@eu-west-1.amazonses.com","subject":"[PATCH v2] t/t7502 : drop duplicate test","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-11T00:49:58Z","receivedAt":"2016-03-11T00:49:58Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"This extra test was introduced erroneously by\nf9c0181 (t7502: test commit.status, --status and\n--no-status, 2010-01-13)\n\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n---\n t/t7502-commit.sh | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex b39e313..725687d 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -527,11 +527,6 @@ try_commit_status_combo () {\n \t\ttest_i18ngrep \"^# Changes to be committed:\" .git/COMMIT_EDITMSG\n \t'\n \n-\ttest_expect_success 'commit' '\n-\t\ttry_commit \"\" &&\n-\t\ttest_i18ngrep \"^# Changes to be committed:\" .git/COMMIT_EDITMSG\n-\t'\n-\n \ttest_expect_success 'commit --status' '\n \t\ttry_commit --status &&\n \t\ttest_i18ngrep \"^# Changes to be committed:\" .git/COMMIT_EDITMSG\n\n--\nhttps://github.com/git/git/pull/208\n"},{"id":"280606","messageId":"CAFZEwPNqzvAMmkhaRUOsp_Hg-2+rG1Mmo0t9_v4x3md-z8mekA@mail.gmail.com","threadId":"41667","inReplyTo":"010201536326c56b-cd751f0b-f03f-4c9f-b834-20cee14b6eb5-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v2] t/t7502 : drop duplicate test","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-11T05:09:15Z","receivedAt":"2016-03-11T05:09:15Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"The older version of this patch :\n - [v1] http://thread.gmane.org/gmane.comp.version-control.git/288662\n\nThe changes between the patches :\n - Improved the language construct of the commit message\n - Provided more details about the cited commit in the commit message\n\nRegards,\nPranit Bauva\nIIT Kharagpur\n"},{"id":"280607","messageId":"20160311054412.GA10841@flurp.local","threadId":"41667","inReplyTo":"CAFZEwPMznAUBhgJgZ7aRvtH1W8hDYLci6khbw9EsWS9WGhSh=Q@mail.gmail.com","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-11T05:44:12Z","receivedAt":"2016-03-11T05:44:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[+cc:Roberto Tyley]\n\nOn Fri, Mar 11, 2016 at 05:45:27AM +0530, Pranit Bauva wrote:\n> On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > As a convenience to reviewers, please use this area below the \"---\"\n> > line to provide links and explain what changed since the previous\n> > round rather than doing so in a separate email.\n> \n> Actually I am sending the patches with submitGit herokuapp because my\n> institute proxy does not allow IMAP/POP3 connections.\n\nThat's unfortunate. Your separate \"cover letter\" often arrives hours\nlater than the patch itself. Perhaps Roberto can comment on submitGit\nand per-patch commentary.\n\n> >> +       echo content >file &&\n> >> +       git add file &&\n> >> +       test_config commit.verbose true &&\n> >> +       (\n> >> +               GIT_EDITOR=cat &&\n> >> +               export GIT_EDITOR &&\n> >> +               test_must_fail git commit >output\n> >> +       ) &&\n> >> +       test_i18ngrep \"diff --git\" output\n> >> +'\n> >\n> > Making git-commit fail unconditionally with \"aborting due to empty\n> > commit message\" is a rather sneaky way to perform this test. I would\n> > have expected to see these new tests re-use the existing machinery\n> > provided by this script (the check-for-diff \"editor\") rather than\n> > inventing an entirely new and unintuitive mechanism. Doing so would\n> > also reduce the size of each new test.\n> \n> I agree on the fact that making git-commit fail unconditionally is not\n> a good way to perform the test. \"check-for-diff\" is not really an\n> \"editor\" and it checks for the commit message after it has been\n> written to the history. The verbose output is stripped when it is\n> written to the history so we won't be able to test whether this patch\n> works.\n\nIt's a bit tricky if you're not used to it, but check-for-diff\nactually does what you want, and does so in a more direct way. While\nit's true that it's not an \"editor\" per se, it does get access to the\nentire block of text that would normally appear in your editor during\nan interactive commit. And, this is happening before the commit has\nbeen written to history. So, check-for-diff gets a chance to look at\nthe full text that would appear in your editor, and can therefore\ncheck if it contains the expected \"diff --git\" string.\n\n> This is where purposely breaking the code is required as when\n> the commit fails, it gives the output of the contents present at that\n> time (which will contain the verbose output). More over the\n> 'check-for-diff' uses grep which is not preferred. Many tests are now\n> using test_i18ngrep (eg. f79ce8db).\n\n'test_i18ngrep' is intended for strings which may be translated,\nhowever, since the expected \"diff --git\" string should never be\ntranslated, check-for-diff's use of 'grep' is correct, whereas\n'test_i18ngrep' would be misleading (if not actively wrong).\n\n> I had planned on using\n> 'check-for-diff' before but it took me some time to figure out this\n> behavior and thus I began searching for another mechanism (breaking\n> code).\n\nAs an experiment, I rewrote the four new tests in terms of\ncheck-for-diff (with \"test_set_editor check-for-diff\" already in\neffect). Here's what they look like, and they function as expected:\n\n    test_expect_success 'commit.verbose true and --verbose omitted' '\n        git -c commit.verbose=true commit --amend\n    '\n\n    test_expect_success 'commit.verbose true and --no-verbose' '\n        test_must_fail git -c commit.verbose=true commit --amend --no-verbose\n    '\n\n    test_expect_success 'commit.verbose false and --verbose' '\n        git -c commit.verbose=false commit --amend --verbose\n    '\n\n    test_expect_success 'commit.verbose false and --verbose omitted' '\n        test_must_fail git -c commit.verbose=false commit --amend\n    '\n\nThese are modeled after the \"initial commit shows verbose diff\" test\nearlier in the script.\n"},{"id":"280609","messageId":"CAFZEwPPbF30fr97WdZdbHwrF-b2VGkYiyTr3dDfdOAXd2SGFkQ@mail.gmail.com","threadId":"41667","inReplyTo":"20160311054412.GA10841@flurp.local","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-03-11T08:00:32Z","receivedAt":"2016-03-11T08:00:32Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"On Fri, Mar 11, 2016 at 11:14 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n> It's a bit tricky if you're not used to it, but check-for-diff\n> actually does what you want, and does so in a more direct way. While\n> it's true that it's not an \"editor\" per se, it does get access to the\n> entire block of text that would normally appear in your editor during\n> an interactive commit. And, this is happening before the commit has\n> been written to history. So, check-for-diff gets a chance to look at\n> the full text that would appear in your editor, and can therefore\n> check if it contains the expected \"diff --git\" string.\n\nYes, this was new to me. Thanks for explaining it in an elaborate\nmanner. It took me some time to actually understand the behavior of\ncheck-for-diff (a tricky one). And it does the task pretty nicely!\n\n> 'test_i18ngrep' is intended for strings which may be translated,\n> however, since the expected \"diff --git\" string should never be\n> translated, check-for-diff's use of 'grep' is correct, whereas\n> 'test_i18ngrep' would be misleading (if not actively wrong).\n\nI should have read the docs before using this method and not just\nblindly using it. I will definitely take care of that next time.\n\n> As an experiment, I rewrote the four new tests in terms of\n> check-for-diff (with \"test_set_editor check-for-diff\" already in\n> effect). Here's what they look like, and they function as expected:\n>\n>     test_expect_success 'commit.verbose true and --verbose omitted' '\n>         git -c commit.verbose=true commit --amend\n>     '\n>\n>     test_expect_success 'commit.verbose true and --no-verbose' '\n>         test_must_fail git -c commit.verbose=true commit --amend --no-verbose\n>     '\n>\n>     test_expect_success 'commit.verbose false and --verbose' '\n>         git -c commit.verbose=false commit --amend --verbose\n>     '\n>\n>     test_expect_success 'commit.verbose false and --verbose omitted' '\n>         test_must_fail git -c commit.verbose=false commit --amend\n>     '\n>\n> These are modeled after the \"initial commit shows verbose diff\" test\n> earlier in the script.\n\nThanks a lot for helping me with the tests. I will add the status\ntests and then resend the patch. This was a nice exercise!\n"},{"id":"280610","messageId":"CAFY1edbeKbswdmjZ-+ioQ1Ns4tQ1tOJPt_dO2G2r+xKL_LeZSQ@mail.gmail.com","threadId":"41667","inReplyTo":"20160311054412.GA10841@flurp.local","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Roberto Tyley","fromEmail":"roberto.tyley@gmail.com","sentAt":"2016-03-11T09:39:19Z","receivedAt":"2016-03-11T09:39:19Z","isPatch":true,"sender":{"key":"roberto.tyley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/52038?v=4"},"body":"On 11 March 2016 at 05:44, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Mar 11, 2016 at 05:45:27AM +0530, Pranit Bauva wrote:\n>> Actually I am sending the patches with submitGit herokuapp because my\n>> institute proxy does not allow IMAP/POP3 connections.\n\nReally glad to hear this is helping you Pranit - I hadn't even thought\nof the blocked IMAP/POP3 connections problem, I'm not sure what other\nmethod you could have easily used to get round this.\n\n> That's unfortunate. Your separate \"cover letter\" often arrives hours\n> later than the patch itself. Perhaps Roberto can comment on submitGit\n> and per-patch commentary.\n\nThis sounds like an improvement I need to make to submitGit, I've\ncreated an issue here:\n\nhttps://github.com/rtyley/submitgit/issues/30\n"},{"id":"280630","messageId":"xmqqvb4samf7.fsf@gitster.mtv.corp.google.com","threadId":"41667","inReplyTo":"010201536326c56b-cd751f0b-f03f-4c9f-b834-20cee14b6eb5-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v2] t/t7502 : drop duplicate test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-11T18:39:56Z","receivedAt":"2016-03-11T18:39:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pranit Bauva <pranit.bauva@gmail.com> writes:\n\n> This extra test was introduced erroneously by\n> f9c0181 (t7502: test commit.status, --status and\n> --no-status, 2010-01-13)\n>\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> ---\n\nThanks.  I briefly thought that this might be checking that doing\nthis twice would give different results, but that is not what is\nhappening.  Also the remainder of this does cover all the\ncombinations, so we are OK after applying this patch.\n\nThanks, will queue.\n\n>  t/t7502-commit.sh | 5 -----\n>  1 file changed, 5 deletions(-)\n>\n> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\n> index b39e313..725687d 100755\n> --- a/t/t7502-commit.sh\n> +++ b/t/t7502-commit.sh\n> @@ -527,11 +527,6 @@ try_commit_status_combo () {\n>  \t\ttest_i18ngrep \"^# Changes to be committed:\" .git/COMMIT_EDITMSG\n>  \t'\n>  \n> -\ttest_expect_success 'commit' '\n> -\t\ttry_commit \"\" &&\n> -\t\ttest_i18ngrep \"^# Changes to be committed:\" .git/COMMIT_EDITMSG\n> -\t'\n> -\n>  \ttest_expect_success 'commit --status' '\n>  \t\ttry_commit --status &&\n>  \t\ttest_i18ngrep \"^# Changes to be committed:\" .git/COMMIT_EDITMSG\n>\n> --\n> https://github.com/git/git/pull/208\n"},{"id":"280631","messageId":"83A7534D5757482089D43FFE96FCA1C9@PhilipOakley","threadId":"41667","inReplyTo":"CAFZEwPMznAUBhgJgZ7aRvtH1W8hDYLci6khbw9EsWS9WGhSh=Q@mail.gmail.com","subject":"Re: [PATCH v4] commit: add a commit.verbose config variable","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2016-03-11T18:39:56Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Pranit Bauva\" <pranit.bauva@gmail.com>\n> On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine <sunshine@sunshineco.com> \n> wrote:\n>\n>>     Add commit.verbose configuration variable as a convenience\n>>     for those who always prefer --verbose.\n>>\n>> or something.\n>\n> Sure!\n>\n>> As a convenience to reviewers, please use this area below the \"---\"\n>> line to provide links and explain what changed since the previous\n>> round rather than doing so in a separate email.\n>\n> Actually I am sending the patches with submitGit herokuapp because my\n> institute proxy does not allow IMAP/POP3 connections.\n\nYou can still include the 'three dashes' and a commentary at the end of your \n(local) regular commit message, and then when it is sent as a patch it will \nhave the right format. There are carried through rebases as well.\n\nThere is a similar feature for attaching notes (though I haven't used it).\n\nEither should get around your institute's proxy issue.\n\n>\n>> The \"permanently\" bit sounds scary. A more concise way to state this \n>> might be:\n>>\n>>     See the `commit.verbose` configuration variable in\n>>     linkgit:git-config[1].\n>>\n>> which doesn't bother spelling out what the intelligent reader should\n>> infer from the reference.\n>> Style: space before {\n>\n> Sure!\n>\n>>> +test_expect_success 'commit with commit.verbose true and no arguments' \n>>> '\n>>\n>> \"no arguments\" doesn't convey much; how about \"--verbose omitted\" or\n>> something? Ditto for the titles of other tests.\n>\n> Will change the language construct.\n>>> +       echo content >file &&\n>>> +       git add file &&\n>>> +       test_config commit.verbose true &&\n>>> +       (\n>>> +               GIT_EDITOR=cat &&\n>>> +               export GIT_EDITOR &&\n>>> +               test_must_fail git commit >output\n>>> +       ) &&\n>>> +       test_i18ngrep \"diff --git\" output\n>>> +'\n>>\n>> Making git-commit fail unconditionally with \"aborting due to empty\n>> commit message\" is a rather sneaky way to perform this test. I would\n>> have expected to see these new tests re-use the existing machinery\n>> provided by this script (the check-for-diff \"editor\") rather than\n>> inventing an entirely new and unintuitive mechanism. Doing so would\n>> also reduce the size of each new test.\n>\n> I agree on the fact that making git-commit fail unconditionally is not\n> a good way to perform the test. \"check-for-diff\" is not really an\n> \"editor\" and it checks for the commit message after it has been\n> written to the history. The verbose output is stripped when it is\n> written to the history so we won't be able to test whether this patch\n> works. This is where purposely breaking the code is required as when\n> the commit fails, it gives the output of the contents present at that\n> time (which will contain the verbose output). More over the\n> 'check-for-diff' uses grep which is not preferred. Many tests are now\n> using test_i18ngrep (eg. f79ce8db). I had planned on using\n> 'check-for-diff' before but it took me some time to figure out this\n> behavior and thus I began searching for another mechanism (breaking\n> code).\n>\n>> Some additional tests[1][2] are probably warranted.\n>>\n>> [1]: http://article.gmane.org/gmane.comp.version-control.git/288648\n>> [2]: http://article.gmane.org/gmane.comp.version-control.git/288657\n>\n> I think these tests also are better included in this file as this\n> patch triggers it and it would not make much of a difference between\n> t7507 and t7502 but in fact improve its readability.\n>>>  test_done\n"}]}