{"thread":{"id":"39425","subject":"[PATCH 0/2] commit -t appends newline after template file","startedAt":"2015-05-26T06:15:06Z","lastAt":"2015-05-31T02:21:53Z","messageCount":14,"participants":["Patryk Obara","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"262115","messageId":"1432620908-16071-1-git-send-email-patryk.obara@gmail.com","threadId":"39425","inReplyTo":null,"subject":"[PATCH 0/2] commit -t appends newline after template file","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2015-05-26T06:15:06Z","receivedAt":"2015-05-26T06:15:06Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"These are my first patches to git, so be extra pedantic during review, please.\n\nI noticed, that newline is appended, when I try to use template file - which\nis annoying if template ends with comment. I digged a bit and it turned out\nthat:\n\n* my editor (vim) was appending newline before eof in template, (I forgot\n  about this); most editors, that I tested appends newline before eof by\n  default. Emacs was exception here.\n\n* commit --status appends newline unconditionally before placing first\n  comment line - it needs to do this or comment might be appended to last line\n  of template file. Usually, in result two newlines are appended after\n  template file content - and unexpected empty line appears in editor.\n\n* tests for git-commit do not verify newlines at all\n\nI fixed tests and wrote few more (patch 1/2) - after applying this patch\nsome tests won't pass (they shouldn't be passing in the first place imo).\nPatch 2 fixes all broken tests.\n\nMore detailed description is in mails to follow.\n"},{"id":"262117","messageId":"1432620908-16071-2-git-send-email-patryk.obara@gmail.com","threadId":"39425","inReplyTo":"1432620908-16071-1-git-send-email-patryk.obara@gmail.com","subject":"[PATCH 1/2] t750*: make tests for commit messages more pedantic","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2015-05-26T06:15:07Z","receivedAt":"2015-05-26T06:15:07Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"Currently messages are compared with --pretty=format:%s%b which does\nnot retain raw format of commit message. In result it's not clear what\npart of expected commit msg is subject and what part is body. Also, it's\nimpossible to test if messages with multiple lines are handled\ncorrectly, which may be significant when using nondefault --cleanup.\n\nChange \"commit_msg_is\" function to use raw message format in log and\ninterpret escaped sequences in expected message. This way it's possible\nto test exactly what commit message text was saved.\n\nAdd test to verify, that no additional content is appended to template\nmessage, which uncovers tiny \"bug\" in --status handling - new line is\nalways appended before status message. If template file ended with\nnewline (which is default for many popular text editors, e.g. vim)\nthen blank line appears before status (which is very annoying when\ntemplate ends with line starting with '#'). On the other hand, this\nnewline needs to be appended if template file didn't end with newline\n(which is default for e.g. emacs) - otherwise first line of status\nmessage may be not cleaned up.\n\nAdd explicit test to verify if \\n is kept unexpanded in commit message -\nthis used to be part of unrelated template test.\n\nModify add-content-and-comment fake editor to include both comments and\nwhitespace, so --cleanup=whitespace is now actually tested.\n\nModify expected value of test \"cleanup commit messages\" (t7502), which\nshouldn't be passing, because template and logfiles are unnecessarily\nstripped before placing them into editor.\n\nSigned-off-by: Patryk Obara <patryk.obara@gmail.com>\n---\n t/t7500-commit.sh               | 91 ++++++++++++++++++++++++++++++-----------\n t/t7500/add-content-and-comment |  4 ++\n t/t7502-commit.sh               | 29 +++++++------\n t/t7504-commit-msg-hook.sh      | 15 ++++---\n 4 files changed, 97 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex 116885a..fd1bf71 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -13,8 +13,8 @@ commit_msg_is () {\n \texpect=commit_msg_is.expect\n \tactual=commit_msg_is.actual\n \n-\tprintf \"%s\" \"$(git log --pretty=format:%s%b -1)\" >\"$actual\" &&\n-\tprintf \"%s\" \"$1\" >\"$expect\" &&\n+\tgit log --pretty=format:%B -1 >\"$actual\" &&\n+\tprintf \"%b\" \"$1\" >\"$expect\" &&\n \ttest_i18ncmp \"$expect\" \"$actual\"\n }\n \n@@ -76,7 +76,7 @@ test_expect_success 'adding real content to a template should commit' '\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n \t\tgit commit --template \"$TEMPLATE\"\n \t) &&\n-\tcommit_msg_is \"template linecommit message\"\n+\tcommit_msg_is \"template line\\ncommit message\\n\"\n '\n \n test_expect_success '-t option should be short for --template' '\n@@ -87,7 +87,7 @@ test_expect_success '-t option should be short for --template' '\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n \t\tgit commit -t \"$TEMPLATE\"\n \t) &&\n-\tcommit_msg_is \"short templatecommit message\"\n+\tcommit_msg_is \"short template\\ncommit message\\n\"\n '\n \n test_expect_success 'config-specified template should commit' '\n@@ -99,7 +99,7 @@ test_expect_success 'config-specified template should commit' '\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n \t\tgit commit\n \t) &&\n-\tcommit_msg_is \"new templatecommit message\"\n+\tcommit_msg_is \"new template\\ncommit message\\n\"\n '\n \n test_expect_success 'explicit commit message should override template' '\n@@ -107,7 +107,7 @@ test_expect_success 'explicit commit message should override template' '\n \tgit add foo &&\n \tGIT_EDITOR=\"$TEST_DIRECTORY\"/t7500/add-content git commit --template \"$TEMPLATE\" \\\n \t\t-m \"command line msg\" &&\n-\tcommit_msg_is \"command line msg\"\n+\tcommit_msg_is \"command line msg\\n\"\n '\n \n test_expect_success 'commit message from file should override template' '\n@@ -118,7 +118,7 @@ test_expect_success 'commit message from file should override template' '\n \t\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n \t\tgit commit --template \"$TEMPLATE\" --file -\n \t) &&\n-\tcommit_msg_is \"standard input msg\"\n+\tcommit_msg_is \"standard input msg\\n\"\n '\n \n cat >\"$TEMPLATE\" <<\\EOF\n@@ -132,7 +132,7 @@ test_expect_success 'commit message from template with whitespace issue' '\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+\tcommit_msg_is \"commit message\\n\"\n '\n \n test_expect_success 'using alternate GIT_INDEX_FILE (1)' '\n@@ -187,7 +187,7 @@ test_expect_success 'commit message from file (1)' '\n \t\tcd subdir &&\n \t\tgit commit --allow-empty -F log\n \t) &&\n-\tcommit_msg_is \"Log in sub directory\"\n+\tcommit_msg_is \"Log in sub directory\\n\"\n '\n \n test_expect_success 'commit message from file (2)' '\n@@ -197,7 +197,7 @@ test_expect_success 'commit message from file (2)' '\n \t\tcd subdir &&\n \t\tgit commit --allow-empty -F log\n \t) &&\n-\tcommit_msg_is \"Log in sub directory\"\n+\tcommit_msg_is \"Log in sub directory\\n\"\n '\n \n test_expect_success 'commit message from stdin' '\n@@ -205,17 +205,17 @@ test_expect_success 'commit message from stdin' '\n \t\tcd subdir &&\n \t\techo \"Log with foo word\" | git commit --allow-empty -F -\n \t) &&\n-\tcommit_msg_is \"Log with foo word\"\n+\tcommit_msg_is \"Log with foo word\\n\"\n '\n \n test_expect_success 'commit -F overrides -t' '\n \t(\n \t\tcd subdir &&\n-\t\techo \"-F log\" > f.log &&\n-\t\techo \"-t template\" > t.template &&\n+\t\techo \"log content\" > f.log &&\n+\t\techo \"template content\" > t.template &&\n \t\tgit commit --allow-empty -F f.log -t t.template\n \t) &&\n-\tcommit_msg_is \"-F log\"\n+\tcommit_msg_is \"log content\\n\"\n '\n \n test_expect_success 'Commit without message is allowed with --allow-empty-message' '\n@@ -238,7 +238,7 @@ test_expect_success 'Commit a message with --allow-empty-message' '\n \techo \"even more content\" >>foo &&\n \tgit add foo &&\n \tgit commit --allow-empty-message -m\"hello there\" &&\n-\tcommit_msg_is \"hello there\"\n+\tcommit_msg_is \"hello there\\n\"\n '\n \n test_expect_success 'commit -C empty respects --allow-empty-message' '\n@@ -269,53 +269,53 @@ EOF\n test_expect_success 'commit --fixup provides correct one-line commit message' '\n \tcommit_for_rebase_autosquash_setup &&\n \tgit commit --fixup HEAD~1 &&\n-\tcommit_msg_is \"fixup! target message subject line\"\n+\tcommit_msg_is \"fixup! target message subject line\\n\"\n '\n \n test_expect_success 'commit --squash works with -F' '\n \tcommit_for_rebase_autosquash_setup &&\n \techo \"log message from file\" >msgfile &&\n \tgit commit --squash HEAD~1 -F msgfile  &&\n-\tcommit_msg_is \"squash! target message subject linelog message from file\"\n+\tcommit_msg_is \"squash! target message subject line\\n\\nlog message from file\\n\"\n '\n \n test_expect_success 'commit --squash works with -m' '\n \tcommit_for_rebase_autosquash_setup &&\n-\tgit commit --squash HEAD~1 -m \"foo bar\\nbaz\" &&\n-\tcommit_msg_is \"squash! target message subject linefoo bar\\nbaz\"\n+\tgit commit --squash HEAD~1 -m \"foo bar baz\" &&\n+\tcommit_msg_is \"squash! target message subject line\\n\\nfoo bar baz\\n\"\n '\n \n test_expect_success 'commit --squash works with -C' '\n \tcommit_for_rebase_autosquash_setup &&\n \tgit commit --squash HEAD~1 -C HEAD &&\n-\tcommit_msg_is \"squash! target message subject lineintermediate commit\"\n+\tcommit_msg_is \"squash! target message subject line\\n\\nintermediate commit\\n\"\n '\n \n test_expect_success 'commit --squash works with -c' '\n \tcommit_for_rebase_autosquash_setup &&\n \ttest_set_editor \"$TEST_DIRECTORY\"/t7500/edit-content &&\n \tgit commit --squash HEAD~1 -c HEAD &&\n-\tcommit_msg_is \"squash! target message subject lineedited commit\"\n+\tcommit_msg_is \"squash! target message subject line\\n\\nedited commit\\n\"\n '\n \n test_expect_success 'commit --squash works with -C for same commit' '\n \tcommit_for_rebase_autosquash_setup &&\n \tgit commit --squash HEAD -C HEAD &&\n-\tcommit_msg_is \"squash! intermediate commit\"\n+\tcommit_msg_is \"squash! intermediate commit\\n\"\n '\n \n test_expect_success 'commit --squash works with -c for same commit' '\n \tcommit_for_rebase_autosquash_setup &&\n \ttest_set_editor \"$TEST_DIRECTORY\"/t7500/edit-content &&\n \tgit commit --squash HEAD -c HEAD &&\n-\tcommit_msg_is \"squash! edited commit\"\n+\tcommit_msg_is \"squash! edited commit\\n\"\n '\n \n test_expect_success 'commit --squash works with editor' '\n \tcommit_for_rebase_autosquash_setup &&\n \ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n \tgit commit --squash HEAD~1 &&\n-\tcommit_msg_is \"squash! target message subject linecommit message\"\n+\tcommit_msg_is \"squash! target message subject line\\n\\ncommit message\\n\"\n '\n \n test_expect_success 'invalid message options when using --fixup' '\n@@ -329,4 +329,47 @@ test_expect_success 'invalid message options when using --fixup' '\n \ttest_must_fail git commit --fixup HEAD~1 -F log\n '\n \n+test_expect_success 'no blank lines appended after template with --status' '\n+\techo \"template line\" > \"$TEMPLATE\" &&\n+\techo changes >>foo &&\n+\tgit add foo &&\n+\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n+\tgit commit -e -t \"$TEMPLATE\" --status &&\n+\tcommit_msg_is \"template line\\ncommit message\\n\"\n+'\n+\n+test_expect_success 'template without newline before eof should work with --status' '\n+\tprintf \"%s\" \"template line\" > \"$TEMPLATE\" &&\n+\techo changes >>foo &&\n+\tgit add foo &&\n+\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n+\tgit commit -e -t \"$TEMPLATE\" --status &&\n+\tcommit_msg_is \"template line\\ncommit message\\n\"\n+'\n+\n+test_expect_success 'no blank lines appended after -F text with --status' '\n+\techo \"log line\" >log-file &&\n+\techo changes >>foo &&\n+\tgit add foo &&\n+\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n+\tgit commit -e -F log-file --status &&\n+\tcommit_msg_is \"log line\\ncommit message\\n\"\n+'\n+\n+test_expect_success 'logfile without newline before eof should work with --status' '\n+\tprintf \"%s\" \"log line\" >log-file &&\n+\techo changes >>foo &&\n+\tgit add foo &&\n+\ttest_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n+\tgit commit -e -F log-file --status &&\n+\tcommit_msg_is \"log line\\ncommit message\\n\"\n+'\n+\n+test_expect_success 'commit does not expand \\n in message' '\n+\techo changes >>foo &&\n+\tgit add foo &&\n+\tgit commit -m \"foo\\nbar\" &&\n+\tcommit_msg_is \"foo\\\\\\\\nbar\\n\"\n+'\n+\n test_done\ndiff --git a/t/t7500/add-content-and-comment b/t/t7500/add-content-and-comment\nindex c4dccff..2a1fc22 100755\n--- a/t/t7500/add-content-and-comment\n+++ b/t/t7500/add-content-and-comment\n@@ -1,5 +1,9 @@\n #!/bin/sh\n echo \"commit message\" >> \"$1\"\n+# add multiple newlines to verify if --cleanup=whitespace will remove them\n+echo >> \"$1\"\n+echo >> \"$1\"\n+echo >> \"$1\"\n echo \"# comment\" >> \"$1\"\n exit 0\n \ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 051489e..d2203ed 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -8,11 +8,12 @@ commit_msg_is () {\n \texpect=commit_msg_is.expect\n \tactual=commit_msg_is.actual\n \n-\tprintf \"%s\" \"$(git log --pretty=format:%s%b -1)\" >$actual &&\n-\tprintf \"%s\" \"$1\" >$expect &&\n-\ttest_i18ncmp $expect $actual\n+\tgit log --pretty=format:%B -1 >\"$actual\" &&\n+\tprintf \"%b\" \"$1\" >\"$expect\" &&\n+\ttest_i18ncmp \"$expect\" \"$actual\"\n }\n \n+\n # Arguments: [<prefix] [<commit message>] [<commit options>]\n check_summary_oneline() {\n \ttest_tick &&\n@@ -255,15 +256,17 @@ test_expect_success 'cleanup commit messages (strip option,-F,-e)' '\n \techo >>negative &&\n \t{ echo;echo sample;echo; } >text &&\n \tgit commit -e -F text -a &&\n-\thead -n 4 .git/COMMIT_EDITMSG >actual\n+\thead -n 5 .git/COMMIT_EDITMSG >actual &&\n+\tcommit_msg_is \"sample\\n\"\n '\n \n-echo \"sample\n+echo \"\n+sample\n \n # Please enter the commit message for your changes. Lines starting\n # with '#' will be ignored, and an empty message aborts the commit.\" >expect\n \n-test_expect_success 'cleanup commit messages (strip option,-F,-e): output' '\n+test_expect_success 'editor view before cleanup commit messages (strip option,-F,-e)' '\n \ttest_i18ncmp expect actual\n '\n \n@@ -282,7 +285,7 @@ test_expect_success 'cleanup commit message (no config and no option uses defaul\n \t  test_set_editor \"$TEST_DIRECTORY\"/t7500/add-content-and-comment &&\n \t  git commit --no-status\n \t) &&\n-\tcommit_msg_is \"commit message\"\n+\tcommit_msg_is \"commit message\\n\"\n '\n \n test_expect_success 'cleanup commit message (option overrides default)' '\n@@ -292,7 +295,7 @@ test_expect_success 'cleanup commit message (option overrides default)' '\n \t  test_set_editor \"$TEST_DIRECTORY\"/t7500/add-content-and-comment &&\n \t  git commit --cleanup=whitespace --no-status\n \t) &&\n-\tcommit_msg_is \"commit message # comment\"\n+\tcommit_msg_is \"commit message\\n\\n# comment\\n\"\n '\n \n test_expect_success 'cleanup commit message (config overrides default)' '\n@@ -302,7 +305,7 @@ test_expect_success 'cleanup commit message (config overrides default)' '\n \t  test_set_editor \"$TEST_DIRECTORY\"/t7500/add-content-and-comment &&\n \t  git -c commit.cleanup=whitespace commit --no-status\n \t) &&\n-\tcommit_msg_is \"commit message # comment\"\n+\tcommit_msg_is \"commit message\\n\\n# comment\\n\"\n '\n \n test_expect_success 'cleanup commit message (option overrides config)' '\n@@ -312,28 +315,28 @@ test_expect_success 'cleanup commit message (option overrides config)' '\n \t  test_set_editor \"$TEST_DIRECTORY\"/t7500/add-content-and-comment &&\n \t  git -c commit.cleanup=whitespace commit --cleanup=default\n \t) &&\n-\tcommit_msg_is \"commit message\"\n+\tcommit_msg_is \"commit message\\n\"\n '\n \n test_expect_success 'cleanup commit message (default, -m)' '\n \techo content >>file &&\n \tgit add file &&\n \tgit commit -m \"message #comment \" &&\n-\tcommit_msg_is \"message #comment\"\n+\tcommit_msg_is \"message #comment\\n\"\n '\n \n test_expect_success 'cleanup commit message (whitespace option, -m)' '\n \techo content >>file &&\n \tgit add file &&\n \tgit commit --cleanup=whitespace --no-status -m \"message #comment \" &&\n-\tcommit_msg_is \"message #comment\"\n+\tcommit_msg_is \"message #comment\\n\"\n '\n \n test_expect_success 'cleanup commit message (whitespace config, -m)' '\n \techo content >>file &&\n \tgit add file &&\n \tgit -c commit.cleanup=whitespace commit --no-status -m \"message #comment \" &&\n-\tcommit_msg_is \"message #comment\"\n+\tcommit_msg_is \"message #comment\\n\"\n '\n \n test_expect_success 'message shows author when it is not equal to committer' '\ndiff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh\nindex 1f53ea8..2c03a86 100755\n--- a/t/t7504-commit-msg-hook.sh\n+++ b/t/t7504-commit-msg-hook.sh\n@@ -179,7 +179,12 @@ EOF\n chmod +x \"$HOOK\"\n \n commit_msg_is () {\n-\ttest \"`git log --pretty=format:%s%b -1`\" = \"$1\"\n+\texpect=commit_msg_is.expect\n+\tactual=commit_msg_is.actual\n+\n+\tgit log --pretty=format:%B -1 >\"$actual\" &&\n+\tprintf \"%b\" \"$1\" >\"$expect\" &&\n+\ttest_i18ncmp \"$expect\" \"$actual\"\n }\n \n test_expect_success 'hook edits commit message' '\n@@ -187,7 +192,7 @@ test_expect_success 'hook edits commit message' '\n \techo \"additional\" >> file &&\n \tgit add file &&\n \tgit commit -m \"additional\" &&\n-\tcommit_msg_is \"new message\"\n+\tcommit_msg_is \"new message\\n\"\n \n '\n \n@@ -197,7 +202,7 @@ test_expect_success 'hook edits commit message (editor)' '\n \tgit add file &&\n \techo \"additional content\" > FAKE_MSG &&\n \tGIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit &&\n-\tcommit_msg_is \"new message\"\n+\tcommit_msg_is \"new message\\n\"\n \n '\n \n@@ -206,7 +211,7 @@ test_expect_success \"hook doesn't edit commit message\" '\n \techo \"plus\" >> file &&\n \tgit add file &&\n \tgit commit --no-verify -m \"plus\" &&\n-\tcommit_msg_is \"plus\"\n+\tcommit_msg_is \"plus\\n\"\n \n '\n \n@@ -216,7 +221,7 @@ test_expect_success \"hook doesn't edit commit message (editor)\" '\n \tgit add file &&\n \techo \"more plus\" > FAKE_MSG &&\n \tGIT_EDITOR=\"\\\"\\$FAKE_EDITOR\\\"\" git commit --no-verify &&\n-\tcommit_msg_is \"more plus\"\n+\tcommit_msg_is \"more plus\\n\"\n \n '\n \n-- \n2.4.1\n"},{"id":"262116","messageId":"1432620908-16071-3-git-send-email-patryk.obara@gmail.com","threadId":"39425","inReplyTo":"1432620908-16071-1-git-send-email-patryk.obara@gmail.com","subject":"[PATCH 2/2] commit: fix ending newline for template files","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2015-05-26T06:15:08Z","receivedAt":"2015-05-26T06:15:08Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"git-commit with -t or -F -e uses content of user-supplied file as\ninitial value for commit msg in editor. There is no guarantee, that this\nfile ends with newline - it depends on file content and editor used to\ncreate file (some editors append and hide last newline from user while\nothers do not).\n\nWhen --status (default) is supplied, additional comment is placed after\ntemplate content. If template file ended with newline this results in\nadditional line being appended (which may be unexpected e.g. when last\nline of template is a comment). On the other hand, first line of status\nshould never be concatenated to last line of template file.\n\nAppend newline before status _only_ if template/logfile didn't end with\none already. This way content of template is exactly the way user intended\nand there's no chance, that line of status will merge with last line of\ntemplate.\n\nRemove unnecessary premature cleanup of commit message, which was\nimplemented for -F, but not for -t.\n\nSigned-off-by: Patryk Obara <patryk.obara@gmail.com>\n---\n builtin/commit.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex da79ac4..eb41e05 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -666,8 +666,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *hook_arg1 = NULL;\n \tconst char *hook_arg2 = NULL;\n-\tint clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n \tint old_display_comment_prefix;\n+\tint sb_ends_with_newline = 0;\n \n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n@@ -737,7 +737,6 @@ 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@@ -775,9 +774,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t */\n \ts->hints = 0;\n \n-\tif (clean_message_contents)\n-\t\tstripspace(&sb, 0);\n-\n \tif (signoff)\n \t\tappend_signoff(&sb, ignore_non_trailer(&sb), 0);\n \n@@ -786,6 +782,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \n \tif (auto_comment_line_char)\n \t\tadjust_comment_line_char(&sb);\n+\n+\tsb_ends_with_newline = ends_with(sb.buf, \"\\n\");\n+\n \tstrbuf_release(&sb);\n \n \t/* This checks if committer ident is explicitly given */\n@@ -794,6 +793,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tint ident_shown = 0;\n \t\tint saved_color_setting;\n \t\tstruct ident_split ci, ai;\n+\t\tint append_newline = (template_file || logfile) ? !sb_ends_with_newline : 1;\n+\n+\t\tif (append_newline)\n+\t\t\tfprintf(s->fp, \"\\n\");\n \n \t\tif (whence != FROM_COMMIT) {\n \t\t\tif (cleanup_mode == CLEANUP_SCISSORS)\n@@ -815,7 +818,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\t\t : \"CHERRY_PICK_HEAD\"));\n \t\t}\n \n-\t\tfprintf(s->fp, \"\\n\");\n \t\tif (cleanup_mode == CLEANUP_ALL)\n \t\t\tstatus_printf(s, GIT_COLOR_NORMAL,\n \t\t\t\t_(\"Please enter the commit message for your changes.\"\n-- \n2.4.1\n"},{"id":"262314","messageId":"CAJfL8+QueOnGwPu0vkpSkWDqPnYYtqdM0mHRWEC8Bad4wuvC4Q@mail.gmail.com","threadId":"39425","inReplyTo":"1432620908-16071-1-git-send-email-patryk.obara@gmail.com","subject":"Re: [PATCH 0/2] commit -t appends newline after template file","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2015-05-28T10:06:34Z","receivedAt":"2015-05-28T10:06:34Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"On Tue, May 26, 2015 at 8:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:\n>\n> These are my first patches to git, so be extra pedantic during review, please.\n>\n> I noticed, that newline is appended, when I try to use template file - which\n> is annoying if template ends with comment. I digged a bit and it turned out\n> that:\n\nHey, can anyone go through my commit and tell me if I need to improve anything\nor (maybe) accept it?\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"262330","messageId":"CAPig+cRHB3Qzm-e1_KROu2RQoW2rftLH=uKrWQBsnW0EYkcLPw@mail.gmail.com","threadId":"39425","inReplyTo":"1432620908-16071-2-git-send-email-patryk.obara@gmail.com","subject":"Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-28T13:34:14Z","receivedAt":"2015-05-28T13:34:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:\n> Currently messages are compared with --pretty=format:%s%b which does\n> not retain raw format of commit message. In result it's not clear what\n> part of expected commit msg is subject and what part is body. Also, it's\n> impossible to test if messages with multiple lines are handled\n> correctly, which may be significant when using nondefault --cleanup.\n\nMakes sense.\n\n> Change \"commit_msg_is\" function to use raw message format in log and\n> interpret escaped sequences in expected message. This way it's possible\n> to test exactly what commit message text was saved.\n\nThese changes would be less daunting to review if split into multiple\npatches; one per logical change. So, for instance, patch 1 would make\nthis change and adjust tests accordingly.\n\n> Add test to verify, that no additional content is appended to template\n> message, which uncovers tiny \"bug\" in --status handling - new line is\n> always appended before status message. If template file ended with\n> newline (which is default for many popular text editors, e.g. vim)\n> then blank line appears before status (which is very annoying when\n> template ends with line starting with '#'). On the other hand, this\n> newline needs to be appended if template file didn't end with newline\n> (which is default for e.g. emacs) - otherwise first line of status\n> message may be not cleaned up.\n\nThis could be patch 2.\n\n> Add explicit test to verify if \\n is kept unexpanded in commit message -\n> this used to be part of unrelated template test.\n\nAnd patch 3, and so on...\n\n> Modify add-content-and-comment fake editor to include both comments and\n> whitespace, so --cleanup=whitespace is now actually tested.\n>\n> Modify expected value of test \"cleanup commit messages\" (t7502), which\n> shouldn't be passing, because template and logfiles are unnecessarily\n> stripped before placing them into editor.\n\nYour cover letter correctly states that with this patch is applied, a\nnumber of tests fail. Tests which are expected to fail should be\ndeclared test_expect_failure rather than test_expect_success. The\npatch which fixes the failures should flip them to\ntest_expect_success.\n\n> Signed-off-by: Patryk Obara <patryk.obara@gmail.com>\n\nMore below...\n\n> ---\n> diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\n> index 116885a..fd1bf71 100755\n> --- a/t/t7500-commit.sh\n> +++ b/t/t7500-commit.sh\n> @@ -13,8 +13,8 @@ commit_msg_is () {\n>         expect=commit_msg_is.expect\n>         actual=commit_msg_is.actual\n>\n> -       printf \"%s\" \"$(git log --pretty=format:%s%b -1)\" >\"$actual\" &&\n> -       printf \"%s\" \"$1\" >\"$expect\" &&\n> +       git log --pretty=format:%B -1 >\"$actual\" &&\n> +       printf \"%b\" \"$1\" >\"$expect\" &&\n>         test_i18ncmp \"$expect\" \"$actual\"\n>  }\n>\n> @@ -329,4 +329,47 @@ test_expect_success 'invalid message options when using --fixup' '\n>         test_must_fail git commit --fixup HEAD~1 -F log\n>  '\n>\n> +test_expect_success 'no blank lines appended after template with --status' '\n> +       echo \"template line\" > \"$TEMPLATE\" &&\n\nStyle: Modern code omits the space after the redirection operator\n(>\"$TEMPLATE\"), however, it's also important to match existing style.\nUnfortunately, this file has an equal mixture of both '>blap' and '>\nblap', so it's difficult to know which style to match. As this is new\ncode, it'd probably be best to omit the space.\n\n> +       echo changes >>foo &&\n> +       git add foo &&\n> +       test_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n> +       git commit -e -t \"$TEMPLATE\" --status &&\n> +       commit_msg_is \"template line\\ncommit message\\n\"\n> +'\n> +\n> +test_expect_success 'template without newline before eof should work with --status' '\n\nIt's not clear what \"should work\" means. I suppose you mean that the\nend result should have exactly one newline after the template. Perhaps\nthe test title could indicate the intent more clearly.\n\n> +       printf \"%s\" \"template line\" > \"$TEMPLATE\" &&\n> +       echo changes >>foo &&\n> +       git add foo &&\n> +       test_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n> +       git commit -e -t \"$TEMPLATE\" --status &&\n> +       commit_msg_is \"template line\\ncommit message\\n\"\n> +'\n> +\n> +test_expect_success 'logfile without newline before eof should work with --status' '\n\nDitto: Unclear \"should work\"\n\n> +       printf \"%s\" \"log line\" >log-file &&\n> +       echo changes >>foo &&\n> +       git add foo &&\n> +       test_set_editor \"$TEST_DIRECTORY\"/t7500/add-content &&\n> +       git commit -e -F log-file --status &&\n> +       commit_msg_is \"log line\\ncommit message\\n\"\n> +'\n>  test_done\n> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\n> index 051489e..d2203ed 100755\n> --- a/t/t7502-commit.sh\n> +++ b/t/t7502-commit.sh\n> @@ -8,11 +8,12 @@ commit_msg_is () {\n>         expect=commit_msg_is.expect\n>         actual=commit_msg_is.actual\n>\n> -       printf \"%s\" \"$(git log --pretty=format:%s%b -1)\" >$actual &&\n> -       printf \"%s\" \"$1\" >$expect &&\n> -       test_i18ncmp $expect $actual\n> +       git log --pretty=format:%B -1 >\"$actual\" &&\n> +       printf \"%b\" \"$1\" >\"$expect\" &&\n> +       test_i18ncmp \"$expect\" \"$actual\"\n>  }\n>\n> +\n\nSneaking in unnecessary whitespace change.\n\n>  # Arguments: [<prefix] [<commit message>] [<commit options>]\n>  check_summary_oneline() {\n>         test_tick &&\n"},{"id":"262333","messageId":"CAPig+cTt5sQ=49qS2+8ZOtiX61kHjAisAvpP7K3XPhtNtCatOg@mail.gmail.com","threadId":"39425","inReplyTo":"1432620908-16071-3-git-send-email-patryk.obara@gmail.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-28T14:29:18Z","receivedAt":"2015-05-28T14:29:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:\n> git-commit with -t or -F -e uses content of user-supplied file as\n> initial value for commit msg in editor. There is no guarantee, that this\n> file ends with newline - it depends on file content and editor used to\n> create file (some editors append and hide last newline from user while\n> others do not).\n>\n> When --status (default) is supplied, additional comment is placed after\n> template content. If template file ended with newline this results in\n> additional line being appended (which may be unexpected e.g. when last\n> line of template is a comment). On the other hand, first line of status\n> should never be concatenated to last line of template file.\n>\n> Append newline before status _only_ if template/logfile didn't end with\n> one already. This way content of template is exactly the way user intended\n> and there's no chance, that line of status will merge with last line of\n> template.\n\nThere is also interaction with --signoff (which does its own handling\nof present or missing newline)...\n\n> Remove unnecessary premature cleanup of commit message, which was\n> implemented for -F, but not for -t.\n\nIs this change distinct from the rest of the patch? If so, it may\ndeserve its own patch.\n\nMoreover, it lacks justification and explanation of why you consider\nthe cleanup unnecessary. History [1] indicates that its application to\n-F but not -t was intentional.\n\n[1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)\n\n> Signed-off-by: Patryk Obara <patryk.obara@gmail.com>\n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index da79ac4..eb41e05 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -666,8 +666,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>         struct strbuf sb = STRBUF_INIT;\n>         const char *hook_arg1 = NULL;\n>         const char *hook_arg2 = NULL;\n> -       int clean_message_contents = (cleanup_mode != CLEANUP_NONE);\n>         int old_display_comment_prefix;\n> +       int sb_ends_with_newline = 0;\n\nWhat does 'sb' mean in sb_ends_with_newline? Is it a reference to\nstrbuf? If so, it doesn't make the variable name any more meaningful.\n\n>         /* This checks and barfs if author is badly specified */\n>         determine_author_info(author_ident);\n> @@ -786,6 +782,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>\n>         if (auto_comment_line_char)\n>                 adjust_comment_line_char(&sb);\n> +\n> +       sb_ends_with_newline = ends_with(sb.buf, \"\\n\");\n> +\n>         strbuf_release(&sb);\n>\n>         /* This checks if committer ident is explicitly given */\n> @@ -794,6 +793,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>                 int ident_shown = 0;\n>                 int saved_color_setting;\n>                 struct ident_split ci, ai;\n> +               int append_newline = (template_file || logfile) ? !sb_ends_with_newline : 1;\n> +\n> +               if (append_newline)\n> +                       fprintf(s->fp, \"\\n\");\n\nDid you consider the alternate approach of handling newline processing\nimmediately upon loading 'logfile' and 'template_file', rather than\ndelaying processing until this point? Doing it that way would involve\na bit of code repetition but might be easier to reason about since it\nwould occur before possible interactions in following code (such as\n--signoff handling).\n\n>                 if (whence != FROM_COMMIT) {\n>                         if (cleanup_mode == CLEANUP_SCISSORS)\n> @@ -815,7 +818,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>                                          : \"CHERRY_PICK_HEAD\"));\n>                 }\n>\n> -               fprintf(s->fp, \"\\n\");\n>                 if (cleanup_mode == CLEANUP_ALL)\n>                         status_printf(s, GIT_COLOR_NORMAL,\n>                                 _(\"Please enter the commit message for your changes.\"\n> --\n> 2.4.1\n"},{"id":"262357","messageId":"xmqqpp5kh8a0.fsf@gitster.dls.corp.google.com","threadId":"39425","inReplyTo":"CAPig+cTt5sQ=49qS2+8ZOtiX61kHjAisAvpP7K3XPhtNtCatOg@mail.gmail.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-28T18:22:15Z","receivedAt":"2015-05-28T18:22:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Moreover, it lacks justification and explanation of why you consider\n> the cleanup unnecessary. History [1] indicates that its application to\n> -F but not -t was intentional.\n>\n> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)\n\nSorry, but the date of that commit seems to be too new to be\nconsidered \"history\"; I do not seem to have it, either.\n\nBut I agree with you that I too failed to see why this change is\nnecessary or desirable in the explanation in the proposed log\nmessage.\n"},{"id":"262358","messageId":"CAPig+cR=Mrgb+-ZZcM6m7AcL25gXYtmEVpO3c23k_UKXPgyQnA@mail.gmail.com","threadId":"39425","inReplyTo":"xmqqpp5kh8a0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-28T18:35:22Z","receivedAt":"2015-05-28T18:35:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 28, 2015 at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n>\n>> Moreover, it lacks justification and explanation of why you consider\n>> the cleanup unnecessary. History [1] indicates that its application to\n>> -F but not -t was intentional.\n>>\n>> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)\n>\n> Sorry, but the date of that commit seems to be too new to be\n> considered \"history\"; I do not seem to have it, either.\n\nIndeed, I somehow botched that. I meant: 8b1ae67 (Do not strip empty\nlines / trailing spaces from a commit message template, 2011-05-08)\n\n> But I agree with you that I too failed to see why this change is\n> necessary or desirable in the explanation in the proposed log\n> message.\n"},{"id":"262360","messageId":"xmqqh9qwh6py.fsf@gitster.dls.corp.google.com","threadId":"39425","inReplyTo":"CAPig+cRHB3Qzm-e1_KROu2RQoW2rftLH=uKrWQBsnW0EYkcLPw@mail.gmail.com","subject":"Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-28T18:55:53Z","receivedAt":"2015-05-28T18:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:\n>> Currently messages are compared with --pretty=format:%s%b which does\n>> not retain raw format of commit message. In result it's not clear what\n>> part of expected commit msg is subject and what part is body. Also, it's\n>> impossible to test if messages with multiple lines are handled\n>> correctly, which may be significant when using nondefault --cleanup.\n>\n> Makes sense.\n> ...\n>> +test_expect_success 'template without newline before eof should work with --status' '\n>\n> It's not clear what \"should work\" means. I suppose you mean that the\n> end result should have exactly one newline after the template. Perhaps\n> the test title could indicate the intent more clearly.\n\nI agree that what \"should work\" in this title is unclear.\n\nBecause there is nothing wrong in the current system, if a follow-up\npatch plans to change the established behaviour, the tests in this\n\"currently we do not test blank lines, so add tests for them\" patch\nshould limit themselves to document the current behaviour.\n\nThen a follow-up patch that modifies the behaviour can show how the\nupdated behaviour is different and illustrate in what way it is\nbetter than the current behaviour.  That would be one way to justify\nthe change.\n"},{"id":"262451","messageId":"xmqqwpzrb0kb.fsf@gitster.dls.corp.google.com","threadId":"39425","inReplyTo":"CAPig+cR=Mrgb+-ZZcM6m7AcL25gXYtmEVpO3c23k_UKXPgyQnA@mail.gmail.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-29T20:17:40Z","receivedAt":"2015-05-29T20:17:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, May 28, 2015 at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Eric Sunshine <sunshine@sunshineco.com> writes:\n>>\n>>> Moreover, it lacks justification and explanation of why you consider\n>>> the cleanup unnecessary. History [1] indicates that its application to\n>>> -F but not -t was intentional.\n>>>\n>>> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)\n>>\n>> Sorry, but the date of that commit seems to be too new to be\n>> considered \"history\"; I do not seem to have it, either.\n>\n> Indeed, I somehow botched that. I meant: 8b1ae67 (Do not strip empty\n> lines / trailing spaces from a commit message template, 2011-05-08)\n\nYeah, that was what I had in mind when I read your response.  And\nthat one is pretty strong in its own opinion on the \"issue\" that was\nbrought up by [PATCH 1/2] being discussed, which was:\n\n    git-commit with -t or -F -e uses content of user-supplied file as\n    initial value for commit msg in editor. There is no guarantee, that this\n    file ends with newline ...\n\nThe log message of 8b1ae67 argues:\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\nin other words, \"if your template ends with an incomplete line and\nit causes you trouble, then do not do that!\".\n\nAs a general principle I am OK with that.\n\nBy default, we should run clean-up after the editor we spawned gives\nus the edited result.  Not adding one more LF after the template\nwhen it already ends with LF would not hurt, but an extra blank\nafter the template material does not hurt, either, so I am honestly\nindifferent.  If the user specified with the --cleanup option not to\nclean-up the result coming back from the editor, then the commented\nmaterial needs to be removed in the editor by the user *anyway*, so\none more LF would not make that much of a difference in that case,\neither.\n\nSo...\n"},{"id":"262458","messageId":"CAPig+cTrW9f1TGvpr4KH+EcOsy=FWvGRj6ZQM6nsFyXc15c4qg@mail.gmail.com","threadId":"39425","inReplyTo":"xmqqwpzrb0kb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-29T22:25:29Z","receivedAt":"2015-05-29T22:25:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> By default, we should run clean-up after the editor we spawned gives\n> us the edited result.  Not adding one more LF after the template\n> when it already ends with LF would not hurt, but an extra blank\n> after the template material does not hurt, either, so I am honestly\n> indifferent.\n\nI had a similar reaction. The one salient bit I picked up was that\nPatryk finds it aesthetically offensive[1] when the template ends with\na comment line, and that comment line does not flow directly into the\ncomment lines provided by --status. That is:\n\n    Template line 1\n    # Template line 2\n\n    # Please enter the commit message...\n    # with '#' will be ignored...\n\n[1]: Quoting from the commit message of patch 1/2: \"...which is very\nannoying when template ends with line starting with '#'\"\n\n> If the user specified with the --cleanup option not to\n> clean-up the result coming back from the editor, then the commented\n> material needs to be removed in the editor by the user *anyway*, so\n> one more LF would not make that much of a difference in that case,\n> either.\n"},{"id":"262465","messageId":"CAJfL8+RtR+w+NQeFGJ7GPsPYgcn59XvWw8eXL12ph9EHwc14ww@mail.gmail.com","threadId":"39425","inReplyTo":"CAPig+cTrW9f1TGvpr4KH+EcOsy=FWvGRj6ZQM6nsFyXc15c4qg@mail.gmail.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Patryk Obara","fromEmail":"patryk.obara@gmail.com","sentAt":"2015-05-30T11:29:16Z","receivedAt":"2015-05-30T11:29:16Z","isPatch":true,"sender":{"key":"patryk.obara@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3967?v=4"},"body":"@Eric, Junio\nThank you a lot for feedback - should I post new set of patches as new thread\nwith new cover letter, or reply to first mail in this thread?\n\n\nOn Thu, May 28, 2015 at 4:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Did you consider the alternate approach of handling newline processing\n> immediately upon loading 'logfile' and 'template_file', rather than\n> delaying processing until this point? Doing it that way would involve\n> a bit of code repetition but might be easier to reason about since it\n> would occur before possible interactions in following code (such as\n> --signoff handling).\n\nYes. I opted to place it in here, because newline was appended previously\nalso in \"if (use_editor)\" block. But I agree, appending this newline after\nloading file will be cleaner - and code repetition may be avoided, if I'll\nseparate file loading code into new function.\n\n\nOn Thu, May 28, 2015 at 4:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Moreover, it lacks justification and explanation of why you consider\n> the cleanup unnecessary. History [1] indicates that its application to\n> -F but not -t was intentional.\n\nThat commit suggests, that cleanup was unintentional in one case, it says\nnothing about it being intentional for -F. Short story: currently cleanup\non commit msg is performed many times (I am not sure if \"only 2 times\" or\nmaybe more. I'll include more detailed analysis with second round of patches :)\n\n\nOn Sat, May 30, 2015 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I had a similar reaction. The one salient bit I picked up was that\n> Patryk finds it aesthetically offensive[1] (...)\n\nYes, that is exactly issue, that I initially wanted to solve. I didn't even\nnotice, that my template had newline appended until I ran git-commit in gdb.\nThen I saw, that I can't actually test changes to newlines and rest followed,\nbecause I didn't want to leave code with more tests disabled.\n\nnano, vim, gedit (and other editors, I guess) append _and_hide_ \\n before\neof from user in default configuration. This newline appended by git before\nstatus is completely unexpected (and unwanted) behaviour IMHO.\n\n\nOn Fri, May 29, 2015 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> in other words, \"if your template ends with an incomplete line and\n> it causes you trouble, then do not do that!\".\n\n1) But problem occurs, when template ends with complete line. To make it\n   disappear, user needs to somehow remove trailing newline from his file.\n   In vim it involves switching to non-default binary mode, in nano or\n   gedit it's impossible. Anwer \"use emacs\" would be a bit disrespectful\n   towards end user ;)\n\n2) That commit addresses different issue - when user intentionally left\n   whitespace in template file, then commit should not clean it up, because\n   it might've beed a \"form\" to be filled.\n\n3) Well, the exact same logic can be applied to logfile - it does not explain\n   why logfiles and template files should be treated differently in this\n   regard. In fact, when looking at 8b1ae67, I think that lack of this cleanup\n   for logfiles might be an unintended ommision. After another look at that\n   commit - included test doesn't actually verify implemented change (commit\n   msg is stripped and \"commit_msg_is\" doesn't verify newlines anyway).\n\nOn Sat, May 30, 2015 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> If the user specified with the --cleanup option not to\n> clean-up the result coming back from the editor, then the commented\n> material needs to be removed in the editor by the user *anyway*.\n\nWhy? Is it not ok to leave lines starting with hash in commit object?\n--cleanup=whitespace|verbatim suggests, that it's a valid usecase.\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"262471","messageId":"xmqqfv6eatmt.fsf@gitster.dls.corp.google.com","threadId":"39425","inReplyTo":"CAPig+cTrW9f1TGvpr4KH+EcOsy=FWvGRj6ZQM6nsFyXc15c4qg@mail.gmail.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-30T16:59:38Z","receivedAt":"2015-05-30T16:59:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> By default, we should run clean-up after the editor we spawned gives\n>> us the edited result.  Not adding one more LF after the template\n>> when it already ends with LF would not hurt, but an extra blank\n>> after the template material does not hurt, either, so I am honestly\n>> indifferent.\n>\n> I had a similar reaction. The one salient bit I picked up was that\n> Patryk finds it aesthetically offensive[1] when the template ends with\n> a comment line, and that comment line does not flow directly into the\n> comment lines provided by --status. That is:\n>\n>     Template line 1\n>     # Template line 2\n>\n>     # Please enter the commit message...\n>     # with '#' will be ignored...\n>\n> [1]: Quoting from the commit message of patch 1/2: \"...which is very\n> annoying when template ends with line starting with '#'\"\n\nAs I said in the message you are responding to, I do not think it\nwould hurt if we stopped adding an LF after a template that already\nends with LF.  I think I am OK with a patch that does so without\ndoing anything else, like changing when clean-up happens, etc.\n"},{"id":"262491","messageId":"CAPig+cTd9OjXkJY3=gQ5b8ZJqLEubhBEN_xm_i1g6CNUxNo1CQ@mail.gmail.com","threadId":"39425","inReplyTo":"CAJfL8+RtR+w+NQeFGJ7GPsPYgcn59XvWw8eXL12ph9EHwc14ww@mail.gmail.com","subject":"Re: [PATCH 2/2] commit: fix ending newline for template files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-31T02:21:53Z","receivedAt":"2015-05-31T02:21:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, May 30, 2015 at 7:29 AM, Patryk Obara <patryk.obara@gmail.com> wrote:\n> On Thu, May 28, 2015 at 4:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> Did you consider the alternate approach of handling newline processing\n>> immediately upon loading 'logfile' and 'template_file', rather than\n>> delaying processing until this point? Doing it that way would involve\n>> a bit of code repetition but might be easier to reason about since it\n>> would occur before possible interactions in following code (such as\n>> --signoff handling).\n>\n> Yes. I opted to place it in here, because newline was appended previously\n> also in \"if (use_editor)\" block. But I agree, appending this newline after\n> loading file will be cleaner - and code repetition may be avoided, if I'll\n> separate file loading code into new function.\n\nA need for this sort of functionality has come up before, so it might\nbe reasonable to introduce a new strbuf function for appending a\ncharacter if missing. In addition to the 'newline' case, appending '/'\nto a pathname is also somewhat common.\n\n> On Sat, May 30, 2015 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> If the user specified with the --cleanup option not to\n>> clean-up the result coming back from the editor, then the commented\n>> material needs to be removed in the editor by the user *anyway*.\n\nYou misattributed this statement. It was from Junio, not I.\n\n> Why? Is it not ok to leave lines starting with hash in commit object?\n> --cleanup=whitespace|verbatim suggests, that it's a valid usecase.\n"}]}