threads / patch / 27294

patch, 2 partscommit: do not cleanup template

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

## tl;dr

11 messages between May 7, 2011 and May 8, 2011. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Boris Faure· May 7, 2011, 15:53 UTC · lore

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(-)
Show changes to builtin/commit.c +1 −1
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
Boris Faure· May 7, 2011, 15:53 UTC · re: Boris Faure · lore

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

Signed-off-by: Boris Faure <billiob@gmail.com>
---
 builtin/commit.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to builtin/commit.c +2 −1
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
Junio C Hamano· May 7, 2011, 20:13 UTC · re: Boris Faure · lore

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

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.

Boris 'billiob' Faure· May 7, 2011, 21:17 UTC · re: Junio C Hamano · lore

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

On Sat, May 7, 2011 at 22:13, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> […]
> 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
Junio C Hamano· May 7, 2011, 22:31 UTC · re: Boris 'billiob' Faure · lore

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

"Boris 'billiob' Faure" <billiob@gmail.com> writes:
Show 12 quoted lines
> 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.

Sebastian Schuberth· May 8, 2011, 08:38 UTC · re: Junio C Hamano · lore

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

On 07.05.2011 22:13, Junio C Hamano wrote:
Show 8 quoted lines
> 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
Sebastian Schuberth· May 8, 2011, 08:47 UTC · re: Junio C Hamano · lore

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

On 07.05.2011 22:13, Junio C Hamano wrote:
Show 6 quoted lines
> 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
Boris Faure· May 8, 2011, 10:31 UTC · re: Sebastian Schuberth · lore

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

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
Show changes to 3 files +20 −1

builtin/commit.c, t/t7500-commit.sh, 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
Sebastian Schuberth· May 8, 2011, 12:28 UTC · re: Boris Faure · lore

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

On 08.05.2011 12:31, Boris Faure wrote:
Show 8 quoted lines
> 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
Junio C Hamano· May 8, 2011, 18:14 UTC · re: Sebastian Schuberth · lore

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

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 14 quoted lines
> 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.
Boris Faure· May 8, 2011, 18:55 UTC · re: Junio C Hamano · lore

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

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
Show changes to 3 files +25 −1

builtin/commit.c, t/t7500-commit.sh, 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

← back to recent threads