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

STALE3726d

6 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Patryk Obara <hidden>
Date: 2016-06-15 23:04:55

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 2/2] commit: fix ending newline for template files

From: Patryk Obara <hidden>
Date: 2016-06-15 23:04:55

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 <redacted>
---
 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

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

From: Patryk Obara <hidden>
Date: 2016-06-15 23:04:55

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 <redacted>
---
 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

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

From: Patryk Obara <hidden>
Date: 2016-06-15 23:05:02

On Tue, May 26, 2015 at 8:15 AM, Patryk Obara [off-list ref] 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

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:05:02

On Tue, May 26, 2015 at 2:15 AM, Patryk Obara [off-list ref] 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 <redacted>

More below...

quoted hunk

---

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"

quoted hunk

+       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

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:05:02

On Tue, May 26, 2015 at 2:15 AM, Patryk Obara [off-list ref] 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)

quoted hunk

Signed-off-by: Patryk Obara <redacted>
---

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.

quoted hunk

        /* 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).

quoted hunk

                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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help