[PATCH 0/2] commit -t appends newline after template file

[PATCH 0/2] commit -t appends newline after template file

[Patryk Obara]

These are my first patches to git, so be extra pedantic during review, please.

I noticed, that newline is appended, when I try to use template file - which
is annoying if template ends with comment. I digged a bit and it turned out
that:

* my editor (vim) was appending newline before eof in template, (I forgot
  about this); most editors, that I tested appends newline before eof by
  default. Emacs was exception here.

* commit --status appends newline unconditionally before placing first
  comment line - it needs to do this or comment might be appended to last line
  of template file. Usually, in result two newlines are appended after
  template file content - and unexpected empty line appears in editor.

* tests for git-commit do not verify newlines at all

I fixed tests and wrote few more (patch 1/2) - after applying this patch
some tests won't pass (they shouldn't be passing in the first place imo).
Patch 2 fixes all broken tests.

More detailed description is in mails to follow.

[PATCH 1/2] t750*: make tests for commit messages more pedantic

[Patryk Obara]

Currently messages are compared with --pretty=format:%s%b which does
not retain raw format of commit message. In result it's not clear what
part of expected commit msg is subject and what part is body. Also, it's
impossible to test if messages with multiple lines are handled
correctly, which may be significant when using nondefault --cleanup.

Change "commit_msg_is" function to use raw message format in log and
interpret escaped sequences in expected message. This way it's possible
to test exactly what commit message text was saved.

Add test to verify, that no additional content is appended to template
message, which uncovers tiny "bug" in --status handling - new line is
always appended before status message. If template file ended with
newline (which is default for many popular text editors, e.g. vim)
then blank line appears before status (which is very annoying when
template ends with line starting with '#'). On the other hand, this
newline needs to be appended if template file didn't end with newline
(which is default for e.g. emacs) - otherwise first line of status
message may be not cleaned up.

Add explicit test to verify if \n is kept unexpanded in commit message -
this used to be part of unrelated template test.

Modify add-content-and-comment fake editor to include both comments and
whitespace, so --cleanup=whitespace is now actually tested.

Modify expected value of test "cleanup commit messages" (t7502), which
shouldn't be passing, because template and logfiles are unnecessarily
stripped before placing them into editor.

Signed-off-by: Patryk Obara <patryk.obara@gmail.com>
---
 t/t7500-commit.sh               | 91 ++++++++++++++++++++++++++++++-----------
 t/t7500/add-content-and-comment |  4 ++
 t/t7502-commit.sh               | 29 +++++++------
 t/t7504-commit-msg-hook.sh      | 15 ++++---
 4 files changed, 97 insertions(+), 42 deletions(-)

diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 116885a..fd1bf71 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -13,8 +13,8 @@ commit_msg_is () {
 	expect=commit_msg_is.expect
 	actual=commit_msg_is.actual
 
-	printf "%s" "$(git log --pretty=format:%s%b -1)" >"$actual" &&
-	printf "%s" "$1" >"$expect" &&
+	git log --pretty=format:%B -1 >"$actual" &&
+	printf "%b" "$1" >"$expect" &&
 	test_i18ncmp "$expect" "$actual"
 }
 
@@ -76,7 +76,7 @@ test_expect_success 'adding real content to a template should commit' '
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
 		git commit --template "$TEMPLATE"
 	) &&
-	commit_msg_is "template linecommit message"
+	commit_msg_is "template line\ncommit message\n"
 '
 
 test_expect_success '-t option should be short for --template' '
@@ -87,7 +87,7 @@ test_expect_success '-t option should be short for --template' '
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
 		git commit -t "$TEMPLATE"
 	) &&
-	commit_msg_is "short templatecommit message"
+	commit_msg_is "short template\ncommit message\n"
 '
 
 test_expect_success 'config-specified template should commit' '
@@ -99,7 +99,7 @@ test_expect_success 'config-specified template should commit' '
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
 		git commit
 	) &&
-	commit_msg_is "new templatecommit message"
+	commit_msg_is "new template\ncommit message\n"
 '
 
 test_expect_success 'explicit commit message should override template' '
@@ -107,7 +107,7 @@ test_expect_success 'explicit commit message should override template' '
 	git add foo &&
 	GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-content git commit --template "$TEMPLATE" \
 		-m "command line msg" &&
-	commit_msg_is "command line msg"
+	commit_msg_is "command line msg\n"
 '
 
 test_expect_success 'commit message from file should override template' '
@@ -118,7 +118,7 @@ test_expect_success 'commit message from file should override template' '
 		test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
 		git commit --template "$TEMPLATE" --file -
 	) &&
-	commit_msg_is "standard input msg"
+	commit_msg_is "standard input msg\n"
 '
 
 cat >"$TEMPLATE" <<\EOF
@@ -132,7 +132,7 @@ test_expect_success 'commit message from template with whitespace issue' '
 	git add foo &&
 	GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-whitespaced-content git commit \
 		--template "$TEMPLATE" &&
-	commit_msg_is "commit message"
+	commit_msg_is "commit message\n"
 '
 
 test_expect_success 'using alternate GIT_INDEX_FILE (1)' '
@@ -187,7 +187,7 @@ test_expect_success 'commit message from file (1)' '
 		cd subdir &&
 		git commit --allow-empty -F log
 	) &&
-	commit_msg_is "Log in sub directory"
+	commit_msg_is "Log in sub directory\n"
 '
 
 test_expect_success 'commit message from file (2)' '
@@ -197,7 +197,7 @@ test_expect_success 'commit message from file (2)' '
 		cd subdir &&
 		git commit --allow-empty -F log
 	) &&
-	commit_msg_is "Log in sub directory"
+	commit_msg_is "Log in sub directory\n"
 '
 
 test_expect_success 'commit message from stdin' '
@@ -205,17 +205,17 @@ test_expect_success 'commit message from stdin' '
 		cd subdir &&
 		echo "Log with foo word" | git commit --allow-empty -F -
 	) &&
-	commit_msg_is "Log with foo word"
+	commit_msg_is "Log with foo word\n"
 '
 
 test_expect_success 'commit -F overrides -t' '
 	(
 		cd subdir &&
-		echo "-F log" > f.log &&
-		echo "-t template" > t.template &&
+		echo "log content" > f.log &&
+		echo "template content" > t.template &&
 		git commit --allow-empty -F f.log -t t.template
 	) &&
-	commit_msg_is "-F log"
+	commit_msg_is "log content\n"
 '
 
 test_expect_success 'Commit without message is allowed with --allow-empty-message' '
@@ -238,7 +238,7 @@ test_expect_success 'Commit a message with --allow-empty-message' '
 	echo "even more content" >>foo &&
 	git add foo &&
 	git commit --allow-empty-message -m"hello there" &&
-	commit_msg_is "hello there"
+	commit_msg_is "hello there\n"
 '
 
 test_expect_success 'commit -C empty respects --allow-empty-message' '
@@ -269,53 +269,53 @@ EOF
 test_expect_success 'commit --fixup provides correct one-line commit message' '
 	commit_for_rebase_autosquash_setup &&
 	git commit --fixup HEAD~1 &&
-	commit_msg_is "fixup! target message subject line"
+	commit_msg_is "fixup! target message subject line\n"
 '
 
 test_expect_success 'commit --squash works with -F' '
 	commit_for_rebase_autosquash_setup &&
 	echo "log message from file" >msgfile &&
 	git commit --squash HEAD~1 -F msgfile  &&
-	commit_msg_is "squash! target message subject linelog message from file"
+	commit_msg_is "squash! target message subject line\n\nlog message from file\n"
 '
 
 test_expect_success 'commit --squash works with -m' '
 	commit_for_rebase_autosquash_setup &&
-	git commit --squash HEAD~1 -m "foo bar\nbaz" &&
-	commit_msg_is "squash! target message subject linefoo bar\nbaz"
+	git commit --squash HEAD~1 -m "foo bar baz" &&
+	commit_msg_is "squash! target message subject line\n\nfoo bar baz\n"
 '
 
 test_expect_success 'commit --squash works with -C' '
 	commit_for_rebase_autosquash_setup &&
 	git commit --squash HEAD~1 -C HEAD &&
-	commit_msg_is "squash! target message subject lineintermediate commit"
+	commit_msg_is "squash! target message subject line\n\nintermediate commit\n"
 '
 
 test_expect_success 'commit --squash works with -c' '
 	commit_for_rebase_autosquash_setup &&
 	test_set_editor "$TEST_DIRECTORY"/t7500/edit-content &&
 	git commit --squash HEAD~1 -c HEAD &&
-	commit_msg_is "squash! target message subject lineedited commit"
+	commit_msg_is "squash! target message subject line\n\nedited commit\n"
 '
 
 test_expect_success 'commit --squash works with -C for same commit' '
 	commit_for_rebase_autosquash_setup &&
 	git commit --squash HEAD -C HEAD &&
-	commit_msg_is "squash! intermediate commit"
+	commit_msg_is "squash! intermediate commit\n"
 '
 
 test_expect_success 'commit --squash works with -c for same commit' '
 	commit_for_rebase_autosquash_setup &&
 	test_set_editor "$TEST_DIRECTORY"/t7500/edit-content &&
 	git commit --squash HEAD -c HEAD &&
-	commit_msg_is "squash! edited commit"
+	commit_msg_is "squash! edited commit\n"
 '
 
 test_expect_success 'commit --squash works with editor' '
 	commit_for_rebase_autosquash_setup &&
 	test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
 	git commit --squash HEAD~1 &&
-	commit_msg_is "squash! target message subject linecommit message"
+	commit_msg_is "squash! target message subject line\n\ncommit message\n"
 '
 
 test_expect_success 'invalid message options when using --fixup' '
@@ -329,4 +329,47 @@ test_expect_success 'invalid message options when using --fixup' '
 	test_must_fail git commit --fixup HEAD~1 -F log
 '
 
+test_expect_success 'no blank lines appended after template with --status' '
+	echo "template line" > "$TEMPLATE" &&
+	echo changes >>foo &&
+	git add foo &&
+	test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+	git commit -e -t "$TEMPLATE" --status &&
+	commit_msg_is "template line\ncommit message\n"
+'
+
+test_expect_success 'template without newline before eof should work with --status' '
+	printf "%s" "template line" > "$TEMPLATE" &&
+	echo changes >>foo &&
+	git add foo &&
+	test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+	git commit -e -t "$TEMPLATE" --status &&
+	commit_msg_is "template line\ncommit message\n"
+'
+
+test_expect_success 'no blank lines appended after -F text with --status' '
+	echo "log line" >log-file &&
+	echo changes >>foo &&
+	git add foo &&
+	test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+	git commit -e -F log-file --status &&
+	commit_msg_is "log line\ncommit message\n"
+'
+
+test_expect_success 'logfile without newline before eof should work with --status' '
+	printf "%s" "log line" >log-file &&
+	echo changes >>foo &&
+	git add foo &&
+	test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
+	git commit -e -F log-file --status &&
+	commit_msg_is "log line\ncommit message\n"
+'
+
+test_expect_success 'commit does not expand \n in message' '
+	echo changes >>foo &&
+	git add foo &&
+	git commit -m "foo\nbar" &&
+	commit_msg_is "foo\\\\nbar\n"
+'
+
 test_done
diff --git a/t/t7500/add-content-and-comment b/t/t7500/add-content-and-comment
index c4dccff..2a1fc22 100755
--- a/t/t7500/add-content-and-comment
+++ b/t/t7500/add-content-and-comment
@@ -1,5 +1,9 @@
 #!/bin/sh
 echo "commit message" >> "$1"
+# add multiple newlines to verify if --cleanup=whitespace will remove them
+echo >> "$1"
+echo >> "$1"
+echo >> "$1"
 echo "# comment" >> "$1"
 exit 0
 
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 051489e..d2203ed 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -8,11 +8,12 @@ commit_msg_is () {
 	expect=commit_msg_is.expect
 	actual=commit_msg_is.actual
 
-	printf "%s" "$(git log --pretty=format:%s%b -1)" >$actual &&
-	printf "%s" "$1" >$expect &&
-	test_i18ncmp $expect $actual
+	git log --pretty=format:%B -1 >"$actual" &&
+	printf "%b" "$1" >"$expect" &&
+	test_i18ncmp "$expect" "$actual"
 }
 
+
 # Arguments: [<prefix] [<commit message>] [<commit options>]
 check_summary_oneline() {
 	test_tick &&
@@ -255,15 +256,17 @@ test_expect_success 'cleanup commit messages (strip option,-F,-e)' '
 	echo >>negative &&
 	{ echo;echo sample;echo; } >text &&
 	git commit -e -F text -a &&
-	head -n 4 .git/COMMIT_EDITMSG >actual
+	head -n 5 .git/COMMIT_EDITMSG >actual &&
+	commit_msg_is "sample\n"
 '
 
-echo "sample
+echo "
+sample
 
 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit." >expect
 
-test_expect_success 'cleanup commit messages (strip option,-F,-e): output' '
+test_expect_success 'editor view before cleanup commit messages (strip option,-F,-e)' '
 	test_i18ncmp expect actual
 '
 
@@ -282,7 +285,7 @@ test_expect_success 'cleanup commit message (no config and no option uses defaul
 	  test_set_editor "$TEST_DIRECTORY"/t7500/add-content-and-comment &&
 	  git commit --no-status
 	) &&
-	commit_msg_is "commit message"
+	commit_msg_is "commit message\n"
 '
 
 test_expect_success 'cleanup commit message (option overrides default)' '
@@ -292,7 +295,7 @@ test_expect_success 'cleanup commit message (option overrides default)' '
 	  test_set_editor "$TEST_DIRECTORY"/t7500/add-content-and-comment &&
 	  git commit --cleanup=whitespace --no-status
 	) &&
-	commit_msg_is "commit message # comment"
+	commit_msg_is "commit message\n\n# comment\n"
 '
 
 test_expect_success 'cleanup commit message (config overrides default)' '
@@ -302,7 +305,7 @@ test_expect_success 'cleanup commit message (config overrides default)' '
 	  test_set_editor "$TEST_DIRECTORY"/t7500/add-content-and-comment &&
 	  git -c commit.cleanup=whitespace commit --no-status
 	) &&
-	commit_msg_is "commit message # comment"
+	commit_msg_is "commit message\n\n# comment\n"
 '
 
 test_expect_success 'cleanup commit message (option overrides config)' '
@@ -312,28 +315,28 @@ test_expect_success 'cleanup commit message (option overrides config)' '
 	  test_set_editor "$TEST_DIRECTORY"/t7500/add-content-and-comment &&
 	  git -c commit.cleanup=whitespace commit --cleanup=default
 	) &&
-	commit_msg_is "commit message"
+	commit_msg_is "commit message\n"
 '
 
 test_expect_success 'cleanup commit message (default, -m)' '
 	echo content >>file &&
 	git add file &&
 	git commit -m "message #comment " &&
-	commit_msg_is "message #comment"
+	commit_msg_is "message #comment\n"
 '
 
 test_expect_success 'cleanup commit message (whitespace option, -m)' '
 	echo content >>file &&
 	git add file &&
 	git commit --cleanup=whitespace --no-status -m "message #comment " &&
-	commit_msg_is "message #comment"
+	commit_msg_is "message #comment\n"
 '
 
 test_expect_success 'cleanup commit message (whitespace config, -m)' '
 	echo content >>file &&
 	git add file &&
 	git -c commit.cleanup=whitespace commit --no-status -m "message #comment " &&
-	commit_msg_is "message #comment"
+	commit_msg_is "message #comment\n"
 '
 
 test_expect_success 'message shows author when it is not equal to committer' '
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 1f53ea8..2c03a86 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -179,7 +179,12 @@ EOF
 chmod +x "$HOOK"
 
 commit_msg_is () {
-	test "`git log --pretty=format:%s%b -1`" = "$1"
+	expect=commit_msg_is.expect
+	actual=commit_msg_is.actual
+
+	git log --pretty=format:%B -1 >"$actual" &&
+	printf "%b" "$1" >"$expect" &&
+	test_i18ncmp "$expect" "$actual"
 }
 
 test_expect_success 'hook edits commit message' '
@@ -187,7 +192,7 @@ test_expect_success 'hook edits commit message' '
 	echo "additional" >> file &&
 	git add file &&
 	git commit -m "additional" &&
-	commit_msg_is "new message"
+	commit_msg_is "new message\n"
 
 '
 
@@ -197,7 +202,7 @@ test_expect_success 'hook edits commit message (editor)' '
 	git add file &&
 	echo "additional content" > FAKE_MSG &&
 	GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit &&
-	commit_msg_is "new message"
+	commit_msg_is "new message\n"
 
 '
 
@@ -206,7 +211,7 @@ test_expect_success "hook doesn't edit commit message" '
 	echo "plus" >> file &&
 	git add file &&
 	git commit --no-verify -m "plus" &&
-	commit_msg_is "plus"
+	commit_msg_is "plus\n"
 
 '
 
@@ -216,7 +221,7 @@ test_expect_success "hook doesn't edit commit message (editor)" '
 	git add file &&
 	echo "more plus" > FAKE_MSG &&
 	GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify &&
-	commit_msg_is "more plus"
+	commit_msg_is "more plus\n"
 
 '
 
-- 
2.4.1

[PATCH 2/2] commit: fix ending newline for template files

[Patryk Obara]

git-commit with -t or -F -e uses content of user-supplied file as
initial value for commit msg in editor. There is no guarantee, that this
file ends with newline - it depends on file content and editor used to
create file (some editors append and hide last newline from user while
others do not).

When --status (default) is supplied, additional comment is placed after
template content. If template file ended with newline this results in
additional line being appended (which may be unexpected e.g. when last
line of template is a comment). On the other hand, first line of status
should never be concatenated to last line of template file.

Append newline before status _only_ if template/logfile didn't end with
one already. This way content of template is exactly the way user intended
and there's no chance, that line of status will merge with last line of
template.

Remove unnecessary premature cleanup of commit message, which was
implemented for -F, but not for -t.

Signed-off-by: Patryk Obara <patryk.obara@gmail.com>
---
 builtin/commit.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index da79ac4..eb41e05 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -666,8 +666,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	struct strbuf sb = STRBUF_INIT;
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
-	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 	int old_display_comment_prefix;
+	int sb_ends_with_newline = 0;
 
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
@@ -737,7 +737,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (strbuf_read_file(&sb, template_file, 0) < 0)
 			die_errno(_("could not read '%s'"), template_file);
 		hook_arg1 = "template";
-		clean_message_contents = 0;
 	}
 
 	/*
@@ -775,9 +774,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	s->hints = 0;
 
-	if (clean_message_contents)
-		stripspace(&sb, 0);
-
 	if (signoff)
 		append_signoff(&sb, ignore_non_trailer(&sb), 0);
 
@@ -786,6 +782,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	if (auto_comment_line_char)
 		adjust_comment_line_char(&sb);
+
+	sb_ends_with_newline = ends_with(sb.buf, "\n");
+
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
@@ -794,6 +793,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		int ident_shown = 0;
 		int saved_color_setting;
 		struct ident_split ci, ai;
+		int append_newline = (template_file || logfile) ? !sb_ends_with_newline : 1;
+
+		if (append_newline)
+			fprintf(s->fp, "\n");
 
 		if (whence != FROM_COMMIT) {
 			if (cleanup_mode == CLEANUP_SCISSORS)
@@ -815,7 +818,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 					 : "CHERRY_PICK_HEAD"));
 		}
 
-		fprintf(s->fp, "\n");
 		if (cleanup_mode == CLEANUP_ALL)
 			status_printf(s, GIT_COLOR_NORMAL,
 				_("Please enter the commit message for your changes."
-- 
2.4.1

Re: [PATCH 0/2] commit -t appends newline after template file

[Patryk Obara]

On Tue, May 26, 2015 at 8:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:
>
> These are my first patches to git, so be extra pedantic during review, please.
>
> I noticed, that newline is appended, when I try to use template file - which
> is annoying if template ends with comment. I digged a bit and it turned out
> that:

Hey, can anyone go through my commit and tell me if I need to improve anything
or (maybe) accept it?

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic

[Eric Sunshine]

On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:
> Currently messages are compared with --pretty=format:%s%b which does
> not retain raw format of commit message. In result it's not clear what
> part of expected commit msg is subject and what part is body. Also, it's
> impossible to test if messages with multiple lines are handled
> correctly, which may be significant when using nondefault --cleanup.

Makes sense.

> Change "commit_msg_is" function to use raw message format in log and
> interpret escaped sequences in expected message. This way it's possible
> to test exactly what commit message text was saved.

These changes would be less daunting to review if split into multiple
patches; one per logical change. So, for instance, patch 1 would make
this change and adjust tests accordingly.

> Add test to verify, that no additional content is appended to template
> message, which uncovers tiny "bug" in --status handling - new line is
> always appended before status message. If template file ended with
> newline (which is default for many popular text editors, e.g. vim)
> then blank line appears before status (which is very annoying when
> template ends with line starting with '#'). On the other hand, this
> newline needs to be appended if template file didn't end with newline
> (which is default for e.g. emacs) - otherwise first line of status
> message may be not cleaned up.

This could be patch 2.

> Add explicit test to verify if \n is kept unexpanded in commit message -
> this used to be part of unrelated template test.

And patch 3, and so on...

> Modify add-content-and-comment fake editor to include both comments and
> whitespace, so --cleanup=whitespace is now actually tested.
>
> Modify expected value of test "cleanup commit messages" (t7502), which
> shouldn't be passing, because template and logfiles are unnecessarily
> stripped before placing them into editor.

Your cover letter correctly states that with this patch is applied, a
number of tests fail. Tests which are expected to fail should be
declared test_expect_failure rather than test_expect_success. The
patch which fixes the failures should flip them to
test_expect_success.

> Signed-off-by: Patryk Obara <patryk.obara@gmail.com>

More below...

> ---
> diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
> index 116885a..fd1bf71 100755
> --- a/t/t7500-commit.sh
> +++ b/t/t7500-commit.sh
> @@ -13,8 +13,8 @@ commit_msg_is () {
>         expect=commit_msg_is.expect
>         actual=commit_msg_is.actual
>
> -       printf "%s" "$(git log --pretty=format:%s%b -1)" >"$actual" &&
> -       printf "%s" "$1" >"$expect" &&
> +       git log --pretty=format:%B -1 >"$actual" &&
> +       printf "%b" "$1" >"$expect" &&
>         test_i18ncmp "$expect" "$actual"
>  }
>
> @@ -329,4 +329,47 @@ test_expect_success 'invalid message options when using --fixup' '
>         test_must_fail git commit --fixup HEAD~1 -F log
>  '
>
> +test_expect_success 'no blank lines appended after template with --status' '
> +       echo "template line" > "$TEMPLATE" &&

Style: Modern code omits the space after the redirection operator
(>"$TEMPLATE"), however, it's also important to match existing style.
Unfortunately, this file has an equal mixture of both '>blap' and '>
blap', so it's difficult to know which style to match. As this is new
code, it'd probably be best to omit the space.

> +       echo changes >>foo &&
> +       git add foo &&
> +       test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
> +       git commit -e -t "$TEMPLATE" --status &&
> +       commit_msg_is "template line\ncommit message\n"
> +'
> +
> +test_expect_success 'template without newline before eof should work with --status' '

It's not clear what "should work" means. I suppose you mean that the
end result should have exactly one newline after the template. Perhaps
the test title could indicate the intent more clearly.

> +       printf "%s" "template line" > "$TEMPLATE" &&
> +       echo changes >>foo &&
> +       git add foo &&
> +       test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
> +       git commit -e -t "$TEMPLATE" --status &&
> +       commit_msg_is "template line\ncommit message\n"
> +'
> +
> +test_expect_success 'logfile without newline before eof should work with --status' '

Ditto: Unclear "should work"

> +       printf "%s" "log line" >log-file &&
> +       echo changes >>foo &&
> +       git add foo &&
> +       test_set_editor "$TEST_DIRECTORY"/t7500/add-content &&
> +       git commit -e -F log-file --status &&
> +       commit_msg_is "log line\ncommit message\n"
> +'
>  test_done
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 051489e..d2203ed 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -8,11 +8,12 @@ commit_msg_is () {
>         expect=commit_msg_is.expect
>         actual=commit_msg_is.actual
>
> -       printf "%s" "$(git log --pretty=format:%s%b -1)" >$actual &&
> -       printf "%s" "$1" >$expect &&
> -       test_i18ncmp $expect $actual
> +       git log --pretty=format:%B -1 >"$actual" &&
> +       printf "%b" "$1" >"$expect" &&
> +       test_i18ncmp "$expect" "$actual"
>  }
>
> +

Sneaking in unnecessary whitespace change.

>  # Arguments: [<prefix] [<commit message>] [<commit options>]
>  check_summary_oneline() {
>         test_tick &&

Re: [PATCH 2/2] commit: fix ending newline for template files

[Eric Sunshine]

On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:
> git-commit with -t or -F -e uses content of user-supplied file as
> initial value for commit msg in editor. There is no guarantee, that this
> file ends with newline - it depends on file content and editor used to
> create file (some editors append and hide last newline from user while
> others do not).
>
> When --status (default) is supplied, additional comment is placed after
> template content. If template file ended with newline this results in
> additional line being appended (which may be unexpected e.g. when last
> line of template is a comment). On the other hand, first line of status
> should never be concatenated to last line of template file.
>
> Append newline before status _only_ if template/logfile didn't end with
> one already. This way content of template is exactly the way user intended
> and there's no chance, that line of status will merge with last line of
> template.

There is also interaction with --signoff (which does its own handling
of present or missing newline)...

> Remove unnecessary premature cleanup of commit message, which was
> implemented for -F, but not for -t.

Is this change distinct from the rest of the patch? If so, it may
deserve its own patch.

Moreover, it lacks justification and explanation of why you consider
the cleanup unnecessary. History [1] indicates that its application to
-F but not -t was intentional.

[1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)

> Signed-off-by: Patryk Obara <patryk.obara@gmail.com>
> ---
> diff --git a/builtin/commit.c b/builtin/commit.c
> index da79ac4..eb41e05 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -666,8 +666,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>         struct strbuf sb = STRBUF_INIT;
>         const char *hook_arg1 = NULL;
>         const char *hook_arg2 = NULL;
> -       int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
>         int old_display_comment_prefix;
> +       int sb_ends_with_newline = 0;

What does 'sb' mean in sb_ends_with_newline? Is it a reference to
strbuf? If so, it doesn't make the variable name any more meaningful.

>         /* This checks and barfs if author is badly specified */
>         determine_author_info(author_ident);
> @@ -786,6 +782,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>
>         if (auto_comment_line_char)
>                 adjust_comment_line_char(&sb);
> +
> +       sb_ends_with_newline = ends_with(sb.buf, "\n");
> +
>         strbuf_release(&sb);
>
>         /* This checks if committer ident is explicitly given */
> @@ -794,6 +793,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>                 int ident_shown = 0;
>                 int saved_color_setting;
>                 struct ident_split ci, ai;
> +               int append_newline = (template_file || logfile) ? !sb_ends_with_newline : 1;
> +
> +               if (append_newline)
> +                       fprintf(s->fp, "\n");

Did you consider the alternate approach of handling newline processing
immediately upon loading 'logfile' and 'template_file', rather than
delaying processing until this point? Doing it that way would involve
a bit of code repetition but might be easier to reason about since it
would occur before possible interactions in following code (such as
--signoff handling).

>                 if (whence != FROM_COMMIT) {
>                         if (cleanup_mode == CLEANUP_SCISSORS)
> @@ -815,7 +818,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>                                          : "CHERRY_PICK_HEAD"));
>                 }
>
> -               fprintf(s->fp, "\n");
>                 if (cleanup_mode == CLEANUP_ALL)
>                         status_printf(s, GIT_COLOR_NORMAL,
>                                 _("Please enter the commit message for your changes."
> --
> 2.4.1

Re: [PATCH 2/2] commit: fix ending newline for template files

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Moreover, it lacks justification and explanation of why you consider
> the cleanup unnecessary. History [1] indicates that its application to
> -F but not -t was intentional.
>
> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)

Sorry, but the date of that commit seems to be too new to be
considered "history"; I do not seem to have it, either.

But I agree with you that I too failed to see why this change is
necessary or desirable in the explanation in the proposed log
message.

Re: [PATCH 2/2] commit: fix ending newline for template files

[Eric Sunshine]

On Thu, May 28, 2015 at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Moreover, it lacks justification and explanation of why you consider
>> the cleanup unnecessary. History [1] indicates that its application to
>> -F but not -t was intentional.
>>
>> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)
>
> Sorry, but the date of that commit seems to be too new to be
> considered "history"; I do not seem to have it, either.

Indeed, I somehow botched that. I meant: 8b1ae67 (Do not strip empty
lines / trailing spaces from a commit message template, 2011-05-08)

> But I agree with you that I too failed to see why this change is
> necessary or desirable in the explanation in the proposed log
> message.

Re: [PATCH 1/2] t750*: make tests for commit messages more pedantic

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, May 26, 2015 at 2:15 AM, Patryk Obara <patryk.obara@gmail.com> wrote:
>> Currently messages are compared with --pretty=format:%s%b which does
>> not retain raw format of commit message. In result it's not clear what
>> part of expected commit msg is subject and what part is body. Also, it's
>> impossible to test if messages with multiple lines are handled
>> correctly, which may be significant when using nondefault --cleanup.
>
> Makes sense.
> ...
>> +test_expect_success 'template without newline before eof should work with --status' '
>
> It's not clear what "should work" means. I suppose you mean that the
> end result should have exactly one newline after the template. Perhaps
> the test title could indicate the intent more clearly.

I agree that what "should work" in this title is unclear.

Because there is nothing wrong in the current system, if a follow-up
patch plans to change the established behaviour, the tests in this
"currently we do not test blank lines, so add tests for them" patch
should limit themselves to document the current behaviour.

Then a follow-up patch that modifies the behaviour can show how the
updated behaviour is different and illustrate in what way it is
better than the current behaviour.  That would be one way to justify
the change.

Re: [PATCH 2/2] commit: fix ending newline for template files

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, May 28, 2015 at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> Moreover, it lacks justification and explanation of why you consider
>>> the cleanup unnecessary. History [1] indicates that its application to
>>> -F but not -t was intentional.
>>>
>>> [1]: bc92377 (commit: fix ending newline for template files, 2015-05-26)
>>
>> Sorry, but the date of that commit seems to be too new to be
>> considered "history"; I do not seem to have it, either.
>
> Indeed, I somehow botched that. I meant: 8b1ae67 (Do not strip empty
> lines / trailing spaces from a commit message template, 2011-05-08)

Yeah, that was what I had in mind when I read your response.  And
that one is pretty strong in its own opinion on the "issue" that was
brought up by [PATCH 1/2] being discussed, which was:

    git-commit with -t or -F -e uses content of user-supplied file as
    initial value for commit msg in editor. There is no guarantee, that this
    file ends with newline ...

The log message of 8b1ae67 argues:

    Templates should be just that: A form that the user fills out, and forms
    have blanks. If people are attached to not having extra whitespace in the
    editor, they can simply clean up their templates.

in other words, "if your template ends with an incomplete line and
it causes you trouble, then do not do that!".

As a general principle I am OK with that.

By default, we should run clean-up after the editor we spawned gives
us the edited result.  Not adding one more LF after the template
when it already ends with LF would not hurt, but an extra blank
after the template material does not hurt, either, so I am honestly
indifferent.  If the user specified with the --cleanup option not to
clean-up the result coming back from the editor, then the commented
material needs to be removed in the editor by the user *anyway*, so
one more LF would not make that much of a difference in that case,
either.

So...

Re: [PATCH 2/2] commit: fix ending newline for template files

[Eric Sunshine]

On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> By default, we should run clean-up after the editor we spawned gives
> us the edited result.  Not adding one more LF after the template
> when it already ends with LF would not hurt, but an extra blank
> after the template material does not hurt, either, so I am honestly
> indifferent.

I had a similar reaction. The one salient bit I picked up was that
Patryk finds it aesthetically offensive[1] when the template ends with
a comment line, and that comment line does not flow directly into the
comment lines provided by --status. That is:

    Template line 1
    # Template line 2

    # Please enter the commit message...
    # with '#' will be ignored...

[1]: Quoting from the commit message of patch 1/2: "...which is very
annoying when template ends with line starting with '#'"

> If the user specified with the --cleanup option not to
> clean-up the result coming back from the editor, then the commented
> material needs to be removed in the editor by the user *anyway*, so
> one more LF would not make that much of a difference in that case,
> either.

Re: [PATCH 2/2] commit: fix ending newline for template files

[Patryk Obara]

@Eric, Junio
Thank you a lot for feedback - should I post new set of patches as new thread
with new cover letter, or reply to first mail in this thread?


On Thu, May 28, 2015 at 4:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Did you consider the alternate approach of handling newline processing
> immediately upon loading 'logfile' and 'template_file', rather than
> delaying processing until this point? Doing it that way would involve
> a bit of code repetition but might be easier to reason about since it
> would occur before possible interactions in following code (such as
> --signoff handling).

Yes. I opted to place it in here, because newline was appended previously
also in "if (use_editor)" block. But I agree, appending this newline after
loading file will be cleaner - and code repetition may be avoided, if I'll
separate file loading code into new function.


On Thu, May 28, 2015 at 4:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Moreover, it lacks justification and explanation of why you consider
> the cleanup unnecessary. History [1] indicates that its application to
> -F but not -t was intentional.

That commit suggests, that cleanup was unintentional in one case, it says
nothing about it being intentional for -F. Short story: currently cleanup
on commit msg is performed many times (I am not sure if "only 2 times" or
maybe more. I'll include more detailed analysis with second round of patches :)


On Sat, May 30, 2015 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> I had a similar reaction. The one salient bit I picked up was that
> Patryk finds it aesthetically offensive[1] (...)

Yes, that is exactly issue, that I initially wanted to solve. I didn't even
notice, that my template had newline appended until I ran git-commit in gdb.
Then I saw, that I can't actually test changes to newlines and rest followed,
because I didn't want to leave code with more tests disabled.

nano, vim, gedit (and other editors, I guess) append _and_hide_ \n before
eof from user in default configuration. This newline appended by git before
status is completely unexpected (and unwanted) behaviour IMHO.


On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> in other words, "if your template ends with an incomplete line and
> it causes you trouble, then do not do that!".

1) But problem occurs, when template ends with complete line. To make it
   disappear, user needs to somehow remove trailing newline from his file.
   In vim it involves switching to non-default binary mode, in nano or
   gedit it's impossible. Anwer "use emacs" would be a bit disrespectful
   towards end user ;)

2) That commit addresses different issue - when user intentionally left
   whitespace in template file, then commit should not clean it up, because
   it might've beed a "form" to be filled.

3) Well, the exact same logic can be applied to logfile - it does not explain
   why logfiles and template files should be treated differently in this
   regard. In fact, when looking at 8b1ae67, I think that lack of this cleanup
   for logfiles might be an unintended ommision. After another look at that
   commit - included test doesn't actually verify implemented change (commit
   msg is stripped and "commit_msg_is" doesn't verify newlines anyway).

On Sat, May 30, 2015 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> If the user specified with the --cleanup option not to
> clean-up the result coming back from the editor, then the commented
> material needs to be removed in the editor by the user *anyway*.

Why? Is it not ok to leave lines starting with hash in commit object?
--cleanup=whitespace|verbatim suggests, that it's a valid usecase.

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

Re: [PATCH 2/2] commit: fix ending newline for template files

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 29, 2015 at 4:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> By default, we should run clean-up after the editor we spawned gives
>> us the edited result.  Not adding one more LF after the template
>> when it already ends with LF would not hurt, but an extra blank
>> after the template material does not hurt, either, so I am honestly
>> indifferent.
>
> I had a similar reaction. The one salient bit I picked up was that
> Patryk finds it aesthetically offensive[1] when the template ends with
> a comment line, and that comment line does not flow directly into the
> comment lines provided by --status. That is:
>
>     Template line 1
>     # Template line 2
>
>     # Please enter the commit message...
>     # with '#' will be ignored...
>
> [1]: Quoting from the commit message of patch 1/2: "...which is very
> annoying when template ends with line starting with '#'"

As I said in the message you are responding to, I do not think it
would hurt if we stopped adding an LF after a template that already
ends with LF.  I think I am OK with a patch that does so without
doing anything else, like changing when clean-up happens, etc.

Re: [PATCH 2/2] commit: fix ending newline for template files

[Eric Sunshine]

On Sat, May 30, 2015 at 7:29 AM, Patryk Obara <patryk.obara@gmail.com> wrote:
> On Thu, May 28, 2015 at 4:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Did you consider the alternate approach of handling newline processing
>> immediately upon loading 'logfile' and 'template_file', rather than
>> delaying processing until this point? Doing it that way would involve
>> a bit of code repetition but might be easier to reason about since it
>> would occur before possible interactions in following code (such as
>> --signoff handling).
>
> Yes. I opted to place it in here, because newline was appended previously
> also in "if (use_editor)" block. But I agree, appending this newline after
> loading file will be cleaner - and code repetition may be avoided, if I'll
> separate file loading code into new function.

A need for this sort of functionality has come up before, so it might
be reasonable to introduce a new strbuf function for appending a
character if missing. In addition to the 'newline' case, appending '/'
to a pathname is also somewhat common.

> On Sat, May 30, 2015 at 12:25 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> If the user specified with the --cleanup option not to
>> clean-up the result coming back from the editor, then the commented
>> material needs to be removed in the editor by the user *anyway*.

You misattributed this statement. It was from Junio, not I.

> Why? Is it not ok to leave lines starting with hash in commit object?
> --cleanup=whitespace|verbatim suggests, that it's a valid usecase.