{"thread":{"id":"27294","subject":"[PATCH 1/2] commit: do not cleanup template","startedAt":"2011-05-07T15:53:43Z","lastAt":"2011-05-08T18:55:22Z","messageCount":11,"participants":["Boris Faure","Junio C Hamano","Boris 'billiob' Faure","Sebastian Schuberth"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"167324","messageId":"1304783624-9919-1-git-send-email-billiob@gmail.com","threadId":"27294","inReplyTo":null,"subject":"[PATCH 1/2] commit: do not cleanup template","fromName":"Boris Faure","fromEmail":"billiob@gmail.com","sentAt":"2011-05-07T15:53:43Z","receivedAt":"2011-05-07T15:53:43Z","isPatch":true,"sender":{"key":"billiob@gmail.com","avatar":"https://avatars.githubusercontent.com/u/27988?v=4"},"body":"The template can begin with a new line (to insert commit title) that\nshould not be cleaned up.\n\nSigned-off-by: Boris Faure \n---\n builtin/commit.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 67757e9..f0e880b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -708,7 +708,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (s->fp == NULL)\n \t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n \n-\tif (cleanup_mode != CLEANUP_NONE)\n+\tif (!template_file && cleanup_mode != CLEANUP_NONE)\n \t\tstripspace(&sb, 0);\n \n \tif (signoff) {\n-- \n1.7.5.1\n"},{"id":"167325","messageId":"1304783624-9919-2-git-send-email-billiob@gmail.com","threadId":"27294","inReplyTo":"1304783624-9919-1-git-send-email-billiob@gmail.com","subject":"[PATCH 2/2] commit: do not add a newline after a template","fromName":"Boris Faure","fromEmail":"billiob@gmail.com","sentAt":"2011-05-07T15:53:44Z","receivedAt":"2011-05-07T15:53:44Z","isPatch":true,"sender":{"key":"billiob@gmail.com","avatar":"https://avatars.githubusercontent.com/u/27988?v=4"},"body":"Signed-off-by: Boris Faure \n---\n builtin/commit.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex f0e880b..2209924 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -754,7 +754,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\t\t ? \"MERGE_HEAD\"\n \t\t\t\t\t : \"CHERRY_PICK_HEAD\"));\n \n-\t\tfprintf(s->fp, \"\\n\");\n+\t\tif (!template_file)\n+\t\t\tfprintf(s->fp, \"\\n\");\n \t\tstatus_printf(s, GIT_COLOR_NORMAL,\n \t\t\t_(\"Please enter the commit message for your changes.\"));\n \t\tif (cleanup_mode == CLEANUP_ALL)\n-- \n1.7.5.1\n"},{"id":"167335","messageId":"7vwri2uv4m.fsf@alter.siamese.dyndns.org","threadId":"27294","inReplyTo":"1304783624-9919-1-git-send-email-billiob@gmail.com","subject":"Re: [PATCH 1/2] commit: do not cleanup template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-07T20:13:29Z","receivedAt":"2011-05-07T20:13:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These two commits change the longstanding behaviour. It is hard to tell if\nthe existing behaviour is something people who use commit log message\ntemplates have relied on (in which case your patch will be a regression to\nthem) or they all shared the sense of annoyance and have been wishing to\nbe improved (in which case nobody will be inconvenienced with your patch).\nThey need to be explained a bit better why these changes are needed.\n\nMy gut feeling is that it is relatively easy to justify [1/2]. We prepare\nthe message file to be edited by the committer in sb by reading from\ndifferent message sources, and usually we would want to clean up what is\nin sb before writing it out, but you may want to enforce a particular\nformat by using the template (such as having a leading whitespace) and the\nway to tweak and fix that appearance should be by editing the template,\nnot by automatically running stripspace() here [*1*] when the message came\nfrom the template.\n\nI cannot tell offhand how you justify [2/2], though. What motivated you to\nremove this blank line? At this point in the codepath, it does not look\nlike it should matter if the original message came from your template or\nfrom somewhere else.\n\nIf the blank line is unneeded after \"# You may be committing a merge...\"\nfor readability, wouldn't that blank line be unneeded when you took the\nmessage from other places, no?\n\nIt might make sense to move that newline before \"if (whence != FROM_COMMIT)\"\nblock, though, to make the logic easier to follow, regardless of the use\nof the template file.\n\n[Footnote]\n\n*1* I however have a feeling that this call is here because other people\nwanted to clean it up here because otherwise mistakes like trailing\nwhitespaces or excess blank lines and whatnot made in the template will\npropagate down to the actual commit log message. But we run stripspace\nagain after the editor returns, so I think that worry is unfounded if we\nare using an editor.\n"},{"id":"167337","messageId":"BANLkTinvwjoa4z2JEiuPD44BV2V_m50VYQ@mail.gmail.com","threadId":"27294","inReplyTo":"7vwri2uv4m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] commit: do not cleanup template","fromName":"Boris 'billiob' Faure","fromEmail":"billiob@gmail.com","sentAt":"2011-05-07T21:17:03Z","receivedAt":"2011-05-07T21:17:03Z","isPatch":true,"sender":{"key":"billiob@gmail.com","avatar":"https://avatars.githubusercontent.com/u/27988?v=4"},"body":"On Sat, May 7, 2011 at 22:13, Junio C Hamano wrote:\n> […]\n> My gut feeling is that it is relatively easy to justify [1/2]. We prepare\n> the message file to be edited by the committer in sb by reading from\n> different message sources, and usually we would want to clean up what is\n> in sb before writing it out, but you may want to enforce a particular\n> format by using the template (such as having a leading whitespace) and the\n> way to tweak and fix that appearance should be by editing the template,\n> not by automatically running stripspace() here [*1*] when the message came\n> from the template.\n\nThat's exactly my point of view.\n\nI use the commit template feature mostly to show additional pieces of\ninformation I'd use in my editor when writing the log.\nIt had scratched an itch because I have to insert manually a newline\nto write the commit title whenever I use a commit log template.\nIt's a bit strange for me to have that different behavior when writing\na commit log whether the commit log template feature is used or not.\nI consider the newline being part of the template since sometimes the\ntemplate could even pre-fill the commit log title.\nThat's the main reason I disabled the cleanup when using a template.\n\n> I cannot tell offhand how you justify [2/2], though. What motivated you to\n> remove this blank line? At this point in the codepath, it does not look\n> like it should matter if the original message came from your template or\n> from somewhere else.\n\nIn the way I use the commit log template, I consider that new line\ninserted there as useless.\n\n> If the blank line is unneeded after \"# You may be committing a merge...\"\n> for readability, wouldn't that blank line be unneeded when you took the\n> message from other places, no?\n\nMaybe.\n\n> It might make sense to move that newline before \"if (whence != FROM_COMMIT)\"\n> block, though, to make the logic easier to follow, regardless of the use\n> of the template file.\n\nMoving the newline there will not change my point of view on the\nusefulness of it when using a template.\n\n-- \nBoris Faure\n"},{"id":"167340","messageId":"7vei4auoqb.fsf@alter.siamese.dyndns.org","threadId":"27294","inReplyTo":"BANLkTinvwjoa4z2JEiuPD44BV2V_m50VYQ@mail.gmail.com","subject":"Re: [PATCH 1/2] commit: do not cleanup template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-07T22:31:40Z","receivedAt":"2011-05-07T22:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Boris 'billiob' Faure\" writes:\n\n> On Sat, May 7, 2011 at 22:13, Junio C Hamano wrote:\n>> […]\n>> My gut feeling is that it is relatively easy to justify [1/2]. We prepare\n>> the message file to be edited by the committer in sb by reading from\n>> different message sources, and usually we would want to clean up what is\n>> in sb before writing it out, but you may want to enforce a particular\n>> format by using the template (such as having a leading whitespace) and the\n>> way to tweak and fix that appearance should be by editing the template,\n>> not by automatically running stripspace() here [*1*] when the message came\n>> from the template.\n>\n> That's exactly my point of view.\n\nPlease describe these things in the proposed commit log message. A\nsubmitted patch should not force reviewers and future readers of \"git log\"\nto guess why this change was made.\n\nWhile reviewing, reviewers can ask what problem you are trying to solve,\nwhy you did things in one particular way, etc., and they may be able to\nextract the necessary information from you like I just did during this\nexchange.\n\nBut people who are reading \"git log\" in the future and bisecting the code\nto see why this change was made 6 months ago will not have that luxury,\nand even if they manage to find you and asked, you may not remember why.\n\nWithout having enough clue to understand what you wanted to achieve, they\nmay make changes that breaks your unstated expectation, and they would not\neven know they broke things.\n"},{"id":"167349","messageId":"4DC65684.1050606@gmail.com","threadId":"27294","inReplyTo":"7vwri2uv4m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] commit: do not cleanup template","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-05-08T08:38:28Z","receivedAt":"2011-05-08T08:38:28Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 07.05.2011 22:13, Junio C Hamano wrote:\n\n> My gut feeling is that it is relatively easy to justify [1/2]. We prepare\n> the message file to be edited by the committer in sb by reading from\n> different message sources, and usually we would want to clean up what is\n> in sb before writing it out, but you may want to enforce a particular\n> format by using the template (such as having a leading whitespace) and the\n> way to tweak and fix that appearance should be by editing the template,\n> not by automatically running stripspace() here [*1*] when the message came\n> from the template.\n\nFYI, Boris' patch pretty much matches the one I proposed back in [1] (or the slightly improved version in [2]). My commit message gives additional reasoning why whitespace in general should not be stripped from templates. IIRC, the only reason my patch was not accepted back then was that I was too lazy to write a test against it.\n\n[1] http://kerneltrap.org/mailarchive/git/2010/3/10/25306\n[2] http://repo.or.cz/w/git/mingw/4msysgit.git/commitdiff/39228f1b8af3eb6a6108c4fabf398d23a97a5993\n\n-- \nSebastian Schuberth\n"},{"id":"167350","messageId":"4DC65888.7090309@gmail.com","threadId":"27294","inReplyTo":"7vwri2uv4m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] commit: do not cleanup template","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-05-08T08:47:04Z","receivedAt":"2011-05-08T08:47:04Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 07.05.2011 22:13, Junio C Hamano wrote:\n\n> These two commits change the longstanding behaviour. It is hard to tell if\n> the existing behaviour is something people who use commit log message\n> templates have relied on (in which case your patch will be a regression to\n> them) or they all shared the sense of annoyance and have been wishing to\n> be improved (in which case nobody will be inconvenienced with your patch).\n> They need to be explained a bit better why these changes are needed.\n\nI forgot this in my first mail: Being one of the poeple that in general \nshares Boris' sense of annoyance, I'm voting in favor of his patch [1/2] \n(or mine, if it is any better), but not [2/2].\n\nFor any poeple that might be inconvenienced by the patch there is a \ntrivial one-time fix: Strip whitespaces from their template. Because of \nthis, I believe it is OK to change the long-standing behavior in this case.\n\n-- \nSebastian Schuberth\n"},{"id":"167358","messageId":"1304850662-6424-1-git-send-email-billiob@gmail.com","threadId":"27294","inReplyTo":"4DC65888.7090309@gmail.com","subject":"[PATCH/RFC v2] Do not strip empty lines / trailing spaces from a commit message template","fromName":"Boris Faure","fromEmail":"billiob@gmail.com","sentAt":"2011-05-08T10:31:02Z","receivedAt":"2011-05-08T10:31:02Z","isPatch":true,"sender":{"key":"billiob@gmail.com","avatar":"https://avatars.githubusercontent.com/u/27988?v=4"},"body":"Templates should be just that: A form that the user fills out, and forms\nhave blanks. If people are attached to not having extra whitespace in the\neditor, they can simply clean up their templates.\n\nAdded test with editor adding even more whitespace.\n\nSigned-off-by: Boris Faure \nBased-on-patch-by:Sebastian Schuberth \n---\n builtin/commit.c | 4 +++-\n t/t7500-commit.sh | 14 ++++++++++++++\n t/t7500/add-whitespaced-content | 3 +++\n 3 files changed, 20 insertions(+), 1 deletions(-)\n create mode 100755 t/t7500/add-whitespaced-content\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 67757e9..411d5e4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -615,6 +615,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tconst char *hook_arg1 = NULL;\n \tconst char *hook_arg2 = NULL;\n \tint ident_shown = 0;\n+\tint clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n \n \tif (!no_verify && run_hook(index_file, \"pre-commit\", NULL))\n \t\treturn 0;\n@@ -681,6 +682,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tif (strbuf_read_file(&sb, template_file, 0) < 0)\n \t\t\tdie_errno(_(\"could not read '%s'\"), template_file);\n \t\thook_arg1 = \"template\";\n+\t\tclean_message_contents = 0;\n \t}\n \n \t/*\n@@ -708,7 +710,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (s->fp == NULL)\n \t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n \n-\tif (cleanup_mode != CLEANUP_NONE)\n+\tif (clean_message_contents)\n \t\tstripspace(&sb, 0);\n \n \tif (signoff) {\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex 47096f9..dedbc0d 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -123,6 +123,20 @@ test_expect_success 'commit message from file should override template' '\n \tcommit_msg_is \"standard input msg\"\n '\n \n+cat > \"$TEMPLATE\" << EOF\n+\n+\n+### template\n+\n+EOF\n+test_expect_success 'commit message from template with whitespace issue' '\n+\techo \"content galore\" >> foo &&\n+\tgit add foo &&\n+\tGIT_EDITOR=\"$TEST_DIRECTORY\"/t7500/add-whitespaced-content git commit \\\n+\t\t--template \"$TEMPLATE\" &&\n+\tcommit_msg_is \"commit message\"\n+'\n+\n test_expect_success 'using alternate GIT_INDEX_FILE (1)' '\n \n \tcp .git/index saved-index &&\ndiff --git a/t/t7500/add-whitespaced-content b/t/t7500/add-whitespaced-content\nnew file mode 100755\nindex 0000000..3d71c68\n--- /dev/null\n+++ b/t/t7500/add-whitespaced-content\n@@ -0,0 +1,3 @@\n+#!/bin/sh\n+echo -e \"\\n \\ncommit message \t \\n\\n\" >> \"$1\"\n+exit 0\n-- \n1.7.5.1.217.g4e3aa.dirty\n"},{"id":"167423","messageId":"4DC68C88.7030604@gmail.com","threadId":"27294","inReplyTo":"1304850662-6424-1-git-send-email-billiob@gmail.com","subject":"Re: [PATCH/RFC v2] Do not strip empty lines / trailing spaces from a commit message template","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-05-08T12:28:56Z","receivedAt":"2011-05-08T12:28:56Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 08.05.2011 12:31, Boris Faure wrote:\n\n> Templates should be just that: A form that the user fills out, and forms\n> have blanks. If people are attached to not having extra whitespace in the\n> editor, they can simply clean up their templates.\n>\n> Added test with editor adding even more whitespace.\n>\n> Signed-off-by: Boris Faure\n> Based-on-patch-by:Sebastian Schuberth\n\nThanks for rebasing the patch and adding the test! I've run the whole \ntest suite on Linux, and all tests pass for me (including the added one).\n\n-- \nSebastian Schuberth\n"},{"id":"167438","messageId":"7vy62hrrf3.fsf@alter.siamese.dyndns.org","threadId":"27294","inReplyTo":"4DC68C88.7030604@gmail.com","subject":"Re: [PATCH/RFC v2] Do not strip empty lines / trailing spaces from a commit message template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-08T18:14:08Z","receivedAt":"2011-05-08T18:14:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On 08.05.2011 12:31, Boris Faure wrote:\n>\n>> Templates should be just that: A form that the user fills out, and forms\n>> have blanks. If people are attached to not having extra whitespace in the\n>> editor, they can simply clean up their templates.\n>>\n>> Added test with editor adding even more whitespace.\n>>\n>> Signed-off-by: Boris Faure\n>> Based-on-patch-by:Sebastian Schuberth\n>\n> Thanks for rebasing the patch and adding the test! I've run the whole\n> test suite on Linux, and all tests pass for me (including the added\n> one).\n\nThanks, both. The updated log message explains _why_ this change is a\ngood thing very well.\n\nI'd avoid \"echo -e\" which is not portable, though.\n"},{"id":"167439","messageId":"1304880922-19879-1-git-send-email-billiob@gmail.com","threadId":"27294","inReplyTo":"7vy62hrrf3.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC v3] Do not strip empty lines / trailing spaces from a commit message template","fromName":"Boris Faure","fromEmail":"billiob@gmail.com","sentAt":"2011-05-08T18:55:22Z","receivedAt":"2011-05-08T18:55:22Z","isPatch":true,"sender":{"key":"billiob@gmail.com","avatar":"https://avatars.githubusercontent.com/u/27988?v=4"},"body":"Templates should be just that: A form that the user fills out, and forms\nhave blanks. If people are attached to not having extra whitespace in the\neditor, they can simply clean up their templates.\n\nAdded test with editor adding even more whitespace.\n\nSigned-off-by: Boris Faure \nBased-on-patch-by:Sebastian Schuberth \n---\n builtin/commit.c | 4 +++-\n t/t7500-commit.sh | 14 ++++++++++++++\n t/t7500/add-whitespaced-content | 8 ++++++++\n 3 files changed, 25 insertions(+), 1 deletions(-)\n create mode 100755 t/t7500/add-whitespaced-content\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 67757e9..411d5e4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -615,6 +615,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tconst char *hook_arg1 = NULL;\n \tconst char *hook_arg2 = NULL;\n \tint ident_shown = 0;\n+\tint clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n \n \tif (!no_verify && run_hook(index_file, \"pre-commit\", NULL))\n \t\treturn 0;\n@@ -681,6 +682,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tif (strbuf_read_file(&sb, template_file, 0) < 0)\n \t\t\tdie_errno(_(\"could not read '%s'\"), template_file);\n \t\thook_arg1 = \"template\";\n+\t\tclean_message_contents = 0;\n \t}\n \n \t/*\n@@ -708,7 +710,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (s->fp == NULL)\n \t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n \n-\tif (cleanup_mode != CLEANUP_NONE)\n+\tif (clean_message_contents)\n \t\tstripspace(&sb, 0);\n \n \tif (signoff) {\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex 47096f9..dedbc0d 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -123,6 +123,20 @@ test_expect_success 'commit message from file should override template' '\n \tcommit_msg_is \"standard input msg\"\n '\n \n+cat > \"$TEMPLATE\" << EOF\n+\n+\n+### template\n+\n+EOF\n+test_expect_success 'commit message from template with whitespace issue' '\n+\techo \"content galore\" >> foo &&\n+\tgit add foo &&\n+\tGIT_EDITOR=\"$TEST_DIRECTORY\"/t7500/add-whitespaced-content git commit \\\n+\t\t--template \"$TEMPLATE\" &&\n+\tcommit_msg_is \"commit message\"\n+'\n+\n test_expect_success 'using alternate GIT_INDEX_FILE (1)' '\n \n \tcp .git/index saved-index &&\ndiff --git a/t/t7500/add-whitespaced-content b/t/t7500/add-whitespaced-content\nnew file mode 100755\nindex 0000000..9cb5860\n--- /dev/null\n+++ b/t/t7500/add-whitespaced-content\n@@ -0,0 +1,8 @@\n+#!/bin/sh\n+cat >> \"$1\" << EOF\n+ \n+\n+commit message \t \n+\n+EOF\n+exit 0\n-- \n1.7.5.1.217.g4e3aa.dirty\n"}]}