[PATCH 1/2] commit: do not cleanup template

[PATCH 1/2] commit: do not cleanup template

[Boris Faure]

The template can begin with a new line (to insert commit title) that
should not be cleaned up.

Signed-off-by: Boris Faure <billiob@gmail.com>
---
 builtin/commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 67757e9..f0e880b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -708,7 +708,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (s->fp == NULL)
 		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
 
-	if (cleanup_mode != CLEANUP_NONE)
+	if (!template_file && cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, 0);
 
 	if (signoff) {
-- 
1.7.5.1

[PATCH 2/2] commit: do not add a newline after a template

[Boris Faure]

Signed-off-by: Boris Faure <billiob@gmail.com>
---
 builtin/commit.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index f0e880b..2209924 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -754,7 +754,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 					 ? "MERGE_HEAD"
 					 : "CHERRY_PICK_HEAD"));
 
-		fprintf(s->fp, "\n");
+		if (!template_file)
+			fprintf(s->fp, "\n");
 		status_printf(s, GIT_COLOR_NORMAL,
 			_("Please enter the commit message for your changes."));
 		if (cleanup_mode == CLEANUP_ALL)
-- 
1.7.5.1

Re: [PATCH 1/2] commit: do not cleanup template

[Junio C Hamano]

These two commits change the longstanding behaviour. It is hard to tell if
the existing behaviour is something people who use commit log message
templates have relied on (in which case your patch will be a regression to
them) or they all shared the sense of annoyance and have been wishing to
be improved (in which case nobody will be inconvenienced with your patch).
They need to be explained a bit better why these changes are needed.

My gut feeling is that it is relatively easy to justify [1/2]. We prepare
the message file to be edited by the committer in sb by reading from
different message sources, and usually we would want to clean up what is
in sb before writing it out, but you may want to enforce a particular
format by using the template (such as having a leading whitespace) and the
way to tweak and fix that appearance should be by editing the template,
not by automatically running stripspace() here [*1*] when the message came
from the template.

I cannot tell offhand how you justify [2/2], though. What motivated you to
remove this blank line? At this point in the codepath, it does not look
like it should matter if the original message came from your template or
from somewhere else.

If the blank line is unneeded after "# You may be committing a merge..."
for readability, wouldn't that blank line be unneeded when you took the
message from other places, no?

It might make sense to move that newline before "if (whence != FROM_COMMIT)"
block, though, to make the logic easier to follow, regardless of the use
of the template file.

[Footnote]

*1* I however have a feeling that this call is here because other people
wanted to clean it up here because otherwise mistakes like trailing
whitespaces or excess blank lines and whatnot made in the template will
propagate down to the actual commit log message. But we run stripspace
again after the editor returns, so I think that worry is unfounded if we
are using an editor.

Re: [PATCH 1/2] commit: do not cleanup template

[Boris 'billiob' Faure]

On Sat, May 7, 2011 at 22:13, Junio C Hamano <gitster@pobox.com> wrote:
> […]
> My gut feeling is that it is relatively easy to justify [1/2]. We prepare
> the message file to be edited by the committer in sb by reading from
> different message sources, and usually we would want to clean up what is
> in sb before writing it out, but you may want to enforce a particular
> format by using the template (such as having a leading whitespace) and the
> way to tweak and fix that appearance should be by editing the template,
> not by automatically running stripspace() here [*1*] when the message came
> from the template.

That's exactly my point of view.

I use the commit template feature mostly to show additional pieces of
information I'd use in my editor when writing the log.
It had scratched an itch because I have to insert manually a newline
to write the commit title whenever I use a commit log template.
It's a bit strange for me to have that different behavior when writing
a commit log whether the commit log template feature is used or not.
I consider the newline being part of the template since sometimes the
template could even pre-fill the commit log title.
That's the main reason I disabled the cleanup when using a template.

> I cannot tell offhand how you justify [2/2], though. What motivated you to
> remove this blank line? At this point in the codepath, it does not look
> like it should matter if the original message came from your template or
> from somewhere else.

In the way I use the commit log template, I consider that new line
inserted there as useless.

> If the blank line is unneeded after "# You may be committing a merge..."
> for readability, wouldn't that blank line be unneeded when you took the
> message from other places, no?

Maybe.

> It might make sense to move that newline before "if (whence != FROM_COMMIT)"
> block, though, to make the logic easier to follow, regardless of the use
> of the template file.

Moving the newline there will not change my point of view on the
usefulness of it when using a template.

-- 
Boris Faure

Re: [PATCH 1/2] commit: do not cleanup template

[Junio C Hamano]

"Boris 'billiob' Faure" <billiob@gmail.com> writes:

> On Sat, May 7, 2011 at 22:13, Junio C Hamano <gitster@pobox.com> wrote:
>> […]
>> My gut feeling is that it is relatively easy to justify [1/2]. We prepare
>> the message file to be edited by the committer in sb by reading from
>> different message sources, and usually we would want to clean up what is
>> in sb before writing it out, but you may want to enforce a particular
>> format by using the template (such as having a leading whitespace) and the
>> way to tweak and fix that appearance should be by editing the template,
>> not by automatically running stripspace() here [*1*] when the message came
>> from the template.
>
> That's exactly my point of view.

Please describe these things in the proposed commit log message.  A
submitted patch should not force reviewers and future readers of "git log"
to guess why this change was made.

While reviewing, reviewers can ask what problem you are trying to solve,
why you did things in one particular way, etc., and they may be able to
extract the necessary information from you like I just did during this
exchange.

But people who are reading "git log" in the future and bisecting the code
to see why this change was made 6 months ago will not have that luxury,
and even if they manage to find you and asked, you may not remember why.

Without having enough clue to understand what you wanted to achieve, they
may make changes that breaks your unstated expectation, and they would not
even know they broke things.

Re: [PATCH 1/2] commit: do not cleanup template

[Sebastian Schuberth]

On 07.05.2011 22:13, Junio C Hamano wrote:

> My gut feeling is that it is relatively easy to justify [1/2]. We prepare
> the message file to be edited by the committer in sb by reading from
> different message sources, and usually we would want to clean up what is
> in sb before writing it out, but you may want to enforce a particular
> format by using the template (such as having a leading whitespace) and the
> way to tweak and fix that appearance should be by editing the template,
> not by automatically running stripspace() here [*1*] when the message came
> from the template.

FYI, Boris' patch pretty much matches the one I proposed back in [1] (or the slightly improved version in [2]). My commit message gives additional reasoning why whitespace in general should not be stripped from templates. IIRC, the only reason my patch was not accepted back then was that I was too lazy to write a test against it.

[1] http://kerneltrap.org/mailarchive/git/2010/3/10/25306
[2] http://repo.or.cz/w/git/mingw/4msysgit.git/commitdiff/39228f1b8af3eb6a6108c4fabf398d23a97a5993

-- 
Sebastian Schuberth

Re: [PATCH 1/2] commit: do not cleanup template

[Sebastian Schuberth]

On 07.05.2011 22:13, Junio C Hamano wrote:

> These two commits change the longstanding behaviour. It is hard to tell if
> the existing behaviour is something people who use commit log message
> templates have relied on (in which case your patch will be a regression to
> them) or they all shared the sense of annoyance and have been wishing to
> be improved (in which case nobody will be inconvenienced with your patch).
> They need to be explained a bit better why these changes are needed.

I forgot this in my first mail: Being one of the poeple that in general 
shares Boris' sense of annoyance, I'm voting in favor of his patch [1/2] 
(or mine, if it is any better), but not [2/2].

For any poeple that might be inconvenienced by the patch there is a 
trivial one-time fix: Strip whitespaces from their template. Because of 
this, I believe it is OK to change the long-standing behavior in this case.

-- 
Sebastian Schuberth

[PATCH/RFC v2] Do not strip empty lines / trailing spaces from a commit message template

[Boris Faure]

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.

Added test with editor adding even more whitespace.

Signed-off-by: Boris Faure <billiob@gmail.com>
Based-on-patch-by:Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/commit.c                |    4 +++-
 t/t7500-commit.sh               |   14 ++++++++++++++
 t/t7500/add-whitespaced-content |    3 +++
 3 files changed, 20 insertions(+), 1 deletions(-)
 create mode 100755 t/t7500/add-whitespaced-content

diff --git a/builtin/commit.c b/builtin/commit.c
index 67757e9..411d5e4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -615,6 +615,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
 	int ident_shown = 0;
+	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
@@ -681,6 +682,7 @@ 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;
 	}
 
 	/*
@@ -708,7 +710,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (s->fp == NULL)
 		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
 
-	if (cleanup_mode != CLEANUP_NONE)
+	if (clean_message_contents)
 		stripspace(&sb, 0);
 
 	if (signoff) {
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 47096f9..dedbc0d 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -123,6 +123,20 @@ test_expect_success 'commit message from file should override template' '
 	commit_msg_is "standard input msg"
 '
 
+cat > "$TEMPLATE" << EOF
+
+
+### template
+
+EOF
+test_expect_success 'commit message from template with whitespace issue' '
+	echo "content galore" >> foo &&
+	git add foo &&
+	GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-whitespaced-content git commit \
+		--template "$TEMPLATE" &&
+	commit_msg_is "commit message"
+'
+
 test_expect_success 'using alternate GIT_INDEX_FILE (1)' '
 
 	cp .git/index saved-index &&
diff --git a/t/t7500/add-whitespaced-content b/t/t7500/add-whitespaced-content
new file mode 100755
index 0000000..3d71c68
--- /dev/null
+++ b/t/t7500/add-whitespaced-content
@@ -0,0 +1,3 @@
+#!/bin/sh
+echo -e "\n \ncommit message  	 \n\n" >> "$1"
+exit 0
-- 
1.7.5.1.217.g4e3aa.dirty

Re: [PATCH/RFC v2] Do not strip empty lines / trailing spaces from a commit message template

[Sebastian Schuberth]

On 08.05.2011 12:31, Boris Faure wrote:

> 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.
>
> Added test with editor adding even more whitespace.
>
> Signed-off-by: Boris Faure<billiob@gmail.com>
> Based-on-patch-by:Sebastian Schuberth<sschuberth@gmail.com>

Thanks for rebasing the patch and adding the test! I've run the whole 
test suite on Linux, and all tests pass for me (including the added one).

-- 
Sebastian Schuberth

Re: [PATCH/RFC v2] Do not strip empty lines / trailing spaces from a commit message template

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On 08.05.2011 12:31, Boris Faure wrote:
>
>> 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.
>>
>> Added test with editor adding even more whitespace.
>>
>> Signed-off-by: Boris Faure<billiob@gmail.com>
>> Based-on-patch-by:Sebastian Schuberth<sschuberth@gmail.com>
>
> Thanks for rebasing the patch and adding the test! I've run the whole
> test suite on Linux, and all tests pass for me (including the added
> one).

Thanks, both.  The updated log message explains _why_ this change is a
good thing very well.

I'd avoid "echo -e" which is not portable, though.

[PATCH/RFC v3] Do not strip empty lines / trailing spaces from a commit message template

[Boris Faure]

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.

Added test with editor adding even more whitespace.

Signed-off-by: Boris Faure <billiob@gmail.com>
Based-on-patch-by:Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin/commit.c                |    4 +++-
 t/t7500-commit.sh               |   14 ++++++++++++++
 t/t7500/add-whitespaced-content |    8 ++++++++
 3 files changed, 25 insertions(+), 1 deletions(-)
 create mode 100755 t/t7500/add-whitespaced-content

diff --git a/builtin/commit.c b/builtin/commit.c
index 67757e9..411d5e4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -615,6 +615,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
 	int ident_shown = 0;
+	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
@@ -681,6 +682,7 @@ 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;
 	}
 
 	/*
@@ -708,7 +710,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (s->fp == NULL)
 		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
 
-	if (cleanup_mode != CLEANUP_NONE)
+	if (clean_message_contents)
 		stripspace(&sb, 0);
 
 	if (signoff) {
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 47096f9..dedbc0d 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -123,6 +123,20 @@ test_expect_success 'commit message from file should override template' '
 	commit_msg_is "standard input msg"
 '
 
+cat > "$TEMPLATE" << EOF
+
+
+### template
+
+EOF
+test_expect_success 'commit message from template with whitespace issue' '
+	echo "content galore" >> foo &&
+	git add foo &&
+	GIT_EDITOR="$TEST_DIRECTORY"/t7500/add-whitespaced-content git commit \
+		--template "$TEMPLATE" &&
+	commit_msg_is "commit message"
+'
+
 test_expect_success 'using alternate GIT_INDEX_FILE (1)' '
 
 	cp .git/index saved-index &&
diff --git a/t/t7500/add-whitespaced-content b/t/t7500/add-whitespaced-content
new file mode 100755
index 0000000..9cb5860
--- /dev/null
+++ b/t/t7500/add-whitespaced-content
@@ -0,0 +1,8 @@
+#!/bin/sh
+cat >> "$1" << EOF
+ 
+
+commit message       	 
+
+EOF
+exit 0
-- 
1.7.5.1.217.g4e3aa.dirty