[PATCH] git-commit: add --verbatim to allow unstripped commit messages

[PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Alex Riesen]

It also implies --allow-empty.

Sometimes the message just have to be the way user wants it.
For instance, a template can contain "#" characters, or the message
must be kept as close to its original source as possible for reimport
reasons. Or maybe the user just copied a shell script including its
comments into the commit message for future reference.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

I just happen to have a corporate template (for perforce messages, I
reuse it my git mirror repo) which contains "#" and at least one time
lost my bash comments in a commit.

 Documentation/git-commit.txt |    7 ++++++-
 builtin-commit.c             |   10 ++++++++--
 t/t7502-commit.sh            |   17 +++++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 4261384..862543f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git-commit' [-a | --interactive] [-s] [-v] [-u]
 	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
 	   [--allow-empty] [--no-verify] [-e] [--author <author>]
-	   [--] [[-i | -o ]<file>...]
+	   [--verbatim] [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -95,6 +95,11 @@ OPTIONS
 	from making such a commit.  This option bypasses the safety, and
 	is primarily for use by foreign scm interface scripts.
 
+--verbatim::
+	Inhibits stripping of leading and trailing spaces,
+	empty lines and #commentary from the commit message.
+	Implies --allow-empty.
+
 -e|--edit::
 	The message taken from file with `-F`, command line with
 	`-m`, and from file with `-C` are usually used as the
diff --git a/builtin-commit.c b/builtin-commit.c
index 0a91013..cc77ba9 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -47,6 +47,7 @@ static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify, allow_empty;
+static int verbatim_message;
 
 static int no_edit, initial_commit, in_merge;
 const char *only_include_assumed;
@@ -88,6 +89,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
 	OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
+	OPT_BOOLEAN(0, "verbatim", &verbatim_message, "do not strip spaces and #comments from message"),
 
 	OPT_END()
 };
@@ -346,7 +348,8 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	if (fp == NULL)
 		die("could not open %s", git_path(commit_editmsg));
 
-	stripspace(&sb, 0);
+	if (!verbatim_message)
+		stripspace(&sb, 0);
 
 	if (signoff) {
 		struct strbuf sob;
@@ -512,6 +515,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		no_edit = 1;
 	if (edit_flag)
 		no_edit = 0;
+	if (verbatim_message)
+		allow_empty = 1;
 
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;
@@ -813,7 +818,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (p != NULL)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 
-	stripspace(&sb, 1);
+	if (!verbatim_message)
+		stripspace(&sb, 1);
 	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
 		rollback_index_files();
 		die("no commit message?  aborting commit.");
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 21ac785..42f98bc 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -89,4 +89,21 @@ test_expect_success 'verbose' '
 
 '
 
+test_expect_success 'verbatim commit messages' '
+
+	echo >>negative &&
+	git add negative &&
+	{ echo;echo "# text";echo; } >expect &&
+	git commit --verbatim -t expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
+	diff -u expect actual &&
+	git commit --verbatim -F expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual &&
+	git commit --verbatim -m "$(cat expect)" -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
 test_done
-- 
1.5.4.rc1.33.gbd32b

Re: [PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Linus Torvalds]

On Thu, 20 Dec 2007, Alex Riesen wrote:
> 
> I just happen to have a corporate template (for perforce messages, I
> reuse it my git mirror repo) which contains "#" and at least one time
> lost my bash comments in a commit.

I think that this is a real bug, but I don't think this is something that 
we should add a flag for.

Basically, I don't think we should really strip lines starting with '#' 
unless *we* added them. In particular, I don't think we should strip them 
at all unless we're running the editor.

So I think that instead of your thing, we should do somethign like the 
appended, which allows you to do things like

	git commit -m "# Message starting with a hash-mark"

which the current code makes impossible ("empty commit message").

That may be enough for your case, although it still does leave the "use 
editor on a template thing", so if that is your usage scenario, I guess we 
still do need a flag for it.

But even if we *do* add a flag (like "--verbatim") you should at the 
*least* also then remove the

	"# (Comment lines starting with '#' will not be included)\n"

printout! Which you didn't.

So I say NAK on this patch.

> It also implies --allow-empty.

I disagree with this one too.

We have had *way* too many problems with various tools generating bogus 
empty commits. I get them from stgit users (and I think this is a serious 
BUG in stgit, dammit!), but I have this memory of some other usage 
scenario that did it too. 

In other words, empty commits are almost always just bogus. And dammit, if 
they aren't bogus, you should *say* so. No "implied" permissions, please. 
If you really want your commits to be empty, what's the downside of just 
adding an explicit "--allow-empty"?

		Linus

---
 builtin-commit.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 0a91013..4685938 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -434,7 +434,7 @@ static int message_is_empty(struct strbuf *sb, int start)
 	/* See if the template is just a prefix of the message. */
 	strbuf_init(&tmpl, 0);
 	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
-		stripspace(&tmpl, 1);
+		stripspace(&tmpl, !no_edit);
 		if (start + tmpl.len <= sb->len &&
 		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
 			start += tmpl.len;
@@ -813,7 +813,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (p != NULL)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 
-	stripspace(&sb, 1);
+	stripspace(&sb, !no_edit);
 	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
 		rollback_index_files();
 		die("no commit message?  aborting commit.");

Re: [PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Alex Riesen]

Linus Torvalds, Thu, Dec 20, 2007 22:40:13 +0100:
> On Thu, 20 Dec 2007, Alex Riesen wrote:
> > 
> > I just happen to have a corporate template (for perforce messages, I
> > reuse it my git mirror repo) which contains "#" and at least one time
> > lost my bash comments in a commit.
> 
> I think that this is a real bug, but I don't think this is something that 
> we should add a flag for.
> 
> Basically, I don't think we should really strip lines starting with '#' 
> unless *we* added them. In particular, I don't think we should strip them 
> at all unless we're running the editor.

Right

> That may be enough for your case, although it still does leave the "use 
> editor on a template thing", so if that is your usage scenario, I guess we 
> still do need a flag for it.

Yes, I afraid I need both. I use "git commit -t" almost (submission in
perforce takes careful planning) every day. I also would like to keep
the empty leading and trailing lines (perforce default GUI P4Win does
not show them, but our scripts which check the descriptions will test
the description text according to template which does have trailing
empty lines).

> But even if we *do* add a flag (like "--verbatim") you should at the 
> *least* also then remove the
> 
> 	"# (Comment lines starting with '#' will not be included)\n"
> 
> printout! Which you didn't.

I did think about it. It wont be read, I believe (at least I ignore
the status listing git-commit generates today). But then, it can be
removed for verbatim message as no one (I think) will probably care.
Including the "Please..." so it does not look stupid in the commit
message later. Will do.

> > It also implies --allow-empty.
> 
> I disagree with this one too.
> 

I agree. Will remove (I am not even sure myself, why I did that. It is
not even tested)

[PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Alex Riesen]

Sometimes the message just have to be the way user wants it.
For instance, a template can contain "#" characters, or the message
must be kept as close to its original source as possible for reimport
reasons. Or maybe the user just copied a shell script including its
comments into the commit message for future reference.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Updated patch. It conflicts with yours a bit. Will update it

 Documentation/git-commit.txt |    7 ++++++-
 builtin-commit.c             |   20 ++++++++++++++------
 t/t7502-commit.sh            |   18 ++++++++++++++++++
 3 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 4261384..862543f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git-commit' [-a | --interactive] [-s] [-v] [-u]
 	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
 	   [--allow-empty] [--no-verify] [-e] [--author <author>]
-	   [--] [[-i | -o ]<file>...]
+	   [--verbatim] [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -95,6 +95,11 @@ OPTIONS
 	from making such a commit.  This option bypasses the safety, and
 	is primarily for use by foreign scm interface scripts.
 
+--verbatim::
+	Inhibits stripping of leading and trailing spaces,
+	empty lines and #commentary from the commit message.
+	Implies --allow-empty.
+
 -e|--edit::
 	The message taken from file with `-F`, command line with
 	`-m`, and from file with `-C` are usually used as the
diff --git a/builtin-commit.c b/builtin-commit.c
index 0a91013..3127247 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -47,6 +47,7 @@ static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify, allow_empty;
+static int verbatim_message;
 
 static int no_edit, initial_commit, in_merge;
 const char *only_include_assumed;
@@ -88,6 +89,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
 	OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
+	OPT_BOOLEAN(0, "verbatim", &verbatim_message, "do not strip spaces and #comments from message"),
 
 	OPT_END()
 };
@@ -346,7 +348,8 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	if (fp == NULL)
 		die("could not open %s", git_path(commit_editmsg));
 
-	stripspace(&sb, 0);
+	if (!verbatim_message)
+		stripspace(&sb, 0);
 
 	if (signoff) {
 		struct strbuf sob;
@@ -404,10 +407,11 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 			"#\n",
 			git_path("MERGE_HEAD"));
 
-	fprintf(fp,
-		"\n"
-		"# Please enter the commit message for your changes.\n"
-		"# (Comment lines starting with '#' will not be included)\n");
+	if (!verbatim_message)
+		fprintf(fp,
+			"\n"
+			"# Please enter the commit message for your changes.\n"
+			"# (Comment lines starting with '#' will not be included)\n");
 	if (only_include_assumed)
 		fprintf(fp, "# %s\n", only_include_assumed);
 
@@ -431,6 +435,9 @@ static int message_is_empty(struct strbuf *sb, int start)
 	const char *nl;
 	int eol, i;
 
+	if (verbatim_message && sb->len)
+		return 1;
+
 	/* See if the template is just a prefix of the message. */
 	strbuf_init(&tmpl, 0);
 	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
@@ -813,7 +820,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (p != NULL)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 
-	stripspace(&sb, 1);
+	if (!verbatim_message)
+		stripspace(&sb, 1);
 	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
 		rollback_index_files();
 		die("no commit message?  aborting commit.");
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 21ac785..d549728 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -89,4 +89,22 @@ test_expect_success 'verbose' '
 
 '
 
+test_expect_success 'verbatim commit messages' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo; } >expect &&
+	git commit --verbatim -t expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
+	diff -u expect actual &&
+	echo >>negative &&
+	git commit --verbatim -F expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual &&
+	echo >>negative &&
+	git commit --verbatim -m "$(cat expect)" -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
 test_done
-- 
1.5.4.rc1.33.gbd32b

[PATCH] Only filter "#" comments from commits if the editor is used

[Alex Riesen]

Originally-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-commit.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 3127247..1356d20 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -441,7 +441,7 @@ static int message_is_empty(struct strbuf *sb, int start)
 	/* See if the template is just a prefix of the message. */
 	strbuf_init(&tmpl, 0);
 	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
-		stripspace(&tmpl, 1);
+		stripspace(&tmpl, !no_edit);
 		if (start + tmpl.len <= sb->len &&
 		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
 			start += tmpl.len;
@@ -821,7 +821,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 
 	if (!verbatim_message)
-		stripspace(&sb, 1);
+		stripspace(&sb, !no_edit);
 	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
 		rollback_index_files();
 		die("no commit message?  aborting commit.");
-- 
1.5.4.rc1.33.gbd32b

Re: [PATCH] Only filter "#" comments from commits if the editor is used

[Junio C Hamano]

Thanks.

Re: [PATCH] Only filter "#" comments from commits if the editor is used

[Junio C Hamano]

I said thanks a bit too early.  That is on top of verbatim, which I have
doubts about putting in 1.5.4.  I on the other hand think Linus's is a
pure bugfix.

Will tweak and apply, thanks.

Re: [PATCH] Only filter "#" comments from commits if the editor is used

[Alex Riesen]

Junio C Hamano, Fri, Dec 21, 2007 00:39:37 +0100:
> Thanks.

Please hold on. The test fails.

[PATCH] Fix thinko in checking for commit message emptiness

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Sorry. It must be late. It it must have been late yesterday
and still is today.

 builtin-commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 1356d20..eae7661 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -436,7 +436,7 @@ static int message_is_empty(struct strbuf *sb, int start)
 	int eol, i;
 
 	if (verbatim_message && sb->len)
-		return 1;
+		return 0;
 
 	/* See if the template is just a prefix of the message. */
 	strbuf_init(&tmpl, 0);
-- 
1.5.4.rc1.33.gbd32b

Re: [PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Junio C Hamano]

I said in the log message when applying your version:

    We even stripped "#" lines from messages given this way:

        git commit -m "# Message starting with a hash-mark"

    which was argurably a bug.

But I wonder if people are using a workflow like this:

	$ cp $company_template my_message
        $ edit my_message
        $ git commit -F my_message

To them, this change is a regression, as $company_template may have had
insn like "# Here you describe your changes..." and the workflow relied
on the current behaviour of stripping them.

Re: [PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Linus Torvalds]

On Fri, 21 Dec 2007, Alex Riesen wrote:
> 
> Yes, I afraid I need both. I use "git commit -t" almost (submission in
> perforce takes careful planning) every day. I also would like to keep
> the empty leading and trailing lines (perforce default GUI P4Win does
> not show them, but our scripts which check the descriptions will test
> the description text according to template which does have trailing
> empty lines).

Hmm. I think your updated patch was pretty good, although I still think it 
could be improved a bit. In particular, thinking more about it, I think we 
have more than an "on/off" switch - we really have three cases:

 a) strip whitespace _and_ comments
 b) strip unnecessary whitespace only
 c) leave things _totally_ alone

and on top of that we also have the issue of an editor.

So my patch basically said that in the absense of an editor, we'll still 
clean up whitespace, but not comments (ie "no_edit" implies doing (b) 
rather than (b)), while your patch basically results in (c) regardless of 
whether we run an editor or not.

But that still leaves one case: do we ever want to do (b) even *if* we use 
an editor? There's another possible choice: our old behaviour of (a) in 
the presense of an editor is now gone.

Now, that last choice (ie "case (a) without an editor") is not only 
unlikely to be anything people want to do anyway, it's also easy enough to 
do by just using "git stripspace -s" on whatever non-editor thing you feed 
to "git commit", so I don't think we need to worry about that one.

But the "maybe you want to run an editor, and you _do_ want case (b)" 
sounds like a case that is not at all unlikely. I could easily see the 
case where you want to have a template that uses '#', and *despite* that 
you want to (a) allow the user to edit things _and_ (b) clean up 
whitespace too.

So I'd almost suggest you make the "--verbatim" flag a three-way switch, 
to allow "totally verbatim" (leave everything in place) and a "don't touch 
comments" (just fix up whitespace) mode.

Hmm? Does that make sense to you?

			Linus

Re: [PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Linus Torvalds]

On Thu, 20 Dec 2007, Junio C Hamano wrote:
> 
> But I wonder if people are using a workflow like this:
> 
> 	$ cp $company_template my_message
>         $ edit my_message
>         $ git commit -F my_message

Yes. See the email I just sent to Alex.

I think that the above kind of workflow is trivial to support with the 
"new" semantics (ie the patch under discussion), by just rewriting that 
last step as

	$ git stripspace -s < my_message | git-commit -F -

so in that sense, we don't really "lose" anything, although I do agree 
that backwards compatibility (in case somebody actually does this!) 
suffers.

But for the case of actually using the built-in editing capability, we 
don't have that choice, so I'd argue that regardless, we should make the 
"--verbatim" switch be a three-way choice between (a) not touching things 
at all, (b) touching up just whitespace, or (c) doing the full enchilada.

And if we introduce such a flag, then I think we can make the *default* 
(in the absense of an explicit flag) be something like

	if (no_edit)
		default = touch_up_just_whitespace;
	else
		default = whole_enchilada

and obviously it could also be a configuration option.

That way, you could always get the existing behaviour with

	git commit --verbatim=full-enchilada -F my_message

(yeah, bad name - I'm not seriously suggesting it be called 
"full-enchilada", and I'm also not sure the flag should be called 
"--verbatim" any more if it has choices, it's more likely that we should 
call it something like "--cleanup={verbatim,whitespace,strip}" or 
something like that.

			Linus

Re: [PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Björn Steinbrink]

On 2007.12.20 15:55:18 -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 21 Dec 2007, Alex Riesen wrote:
> > 
> > Yes, I afraid I need both. I use "git commit -t" almost (submission in
> > perforce takes careful planning) every day. I also would like to keep
> > the empty leading and trailing lines (perforce default GUI P4Win does
> > not show them, but our scripts which check the descriptions will test
> > the description text according to template which does have trailing
> > empty lines).
> 
> Hmm. I think your updated patch was pretty good, although I still think it 
> could be improved a bit. In particular, thinking more about it, I think we 
> have more than an "on/off" switch - we really have three cases:
> 
>  a) strip whitespace _and_ comments
>  b) strip unnecessary whitespace only
>  c) leave things _totally_ alone
> 
> and on top of that we also have the issue of an editor.
> 
> So my patch basically said that in the absense of an editor, we'll still 
> clean up whitespace, but not comments (ie "no_edit" implies doing (b) 
> rather than (b)), while your patch basically results in (c) regardless of 
> whether we run an editor or not.
> 
> But that still leaves one case: do we ever want to do (b) even *if* we use 
> an editor? There's another possible choice: our old behaviour of (a) in 
> the presense of an editor is now gone.
> 
> Now, that last choice (ie "case (a) without an editor") is not only 
> unlikely to be anything people want to do anyway, it's also easy enough to 
> do by just using "git stripspace -s" on whatever non-editor thing you feed 
> to "git commit", so I don't think we need to worry about that one.
> 
> But the "maybe you want to run an editor, and you _do_ want case (b)" 
> sounds like a case that is not at all unlikely. I could easily see the 
> case where you want to have a template that uses '#', and *despite* that 
> you want to (a) allow the user to edit things _and_ (b) clean up 
> whitespace too.
> 
> So I'd almost suggest you make the "--verbatim" flag a three-way switch, 
> to allow "totally verbatim" (leave everything in place) and a "don't touch 
> comments" (just fix up whitespace) mode.
> 
> Hmm? Does that make sense to you?

Hm, this is a bit more intrusive, but should catch most cases.

At the top of the comments in the commit message template add:
#GIT CUT HERE
(And adjust the descriptive text)

That line hopefully being uncommon enough to not affect any existing
stuff.

If that line is present, comment lines above it are kept, otherwise
they're removed. Whitespace is always fixed(?).

Results:
 - git commit -m "# Foo and bar"
   * keeps the comment, looks like the expected thing

 - git commit with editor
   * Comments that are manually added are kept
   * For the (probably seldom) case, that you want manually added
     comments to be stripped, you can still remove the "#GIT CUR HERE"
     line

 - git commit with template
   * The existing templates probably won't have a "#GIT CUT HERE" line,
     so we're backwards compatible
   * Templates that want to keep the comments can simple get a "#GIT CUT
     HERE" line at the end and Just Work, regardless of whether or not
     you forget to pass --verbatim.


Hmm?

thanks,
Björn

Re: [PATCH] git-commit: add --verbatim to allow unstripped commit messages

[Linus Torvalds]

On Fri, 21 Dec 2007, Bj?rn Steinbrink wrote:
> 
> Hm, this is a bit more intrusive, but should catch most cases.
> 
> At the top of the comments in the commit message template add:
> #GIT CUT HERE
> (And adjust the descriptive text)

Ouch. I'd personally hate to see something like that at the top and then 
have to save it. I always add my text to the top of the message, and all 
the pre-made messages for me are at the top (ie the kinds you get with 
"git commit --amend", where the top of the thing is the old message).

That said, I might well agree with this approach if we made the marker 
line be the *last* line of the message, ie make it be something that ends 
with (ignoring empty whitespace at the end, of course, since those are 
invisible in most editors):

	# Remove this line to keep all comment lines

or something like that.

That still keeps the question about whitespace cleanups. But if it's just 
about whitespace or no whitespace, then a simple "--verbatim" flag would 
work.

			Linus

[PATCH] Allow selection of different cleanup modes for commit messages

[Alex Riesen]

The modes being default, verbatim, whitespace and strip. The default
mode depends on if the message is being edited and will either strip
whitespace and comments (if editor active) or just strip the
whitespace (for where the message is given explicitely).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

---
Linus Torvalds, Fri, Dec 21, 2007 01:03:26 +0100:
> But for the case of actually using the built-in editing capability, we 
> don't have that choice, so I'd argue that regardless, we should make the 
> "--verbatim" switch be a three-way choice between (a) not touching things 
> at all, (b) touching up just whitespace, or (c) doing the full enchilada.
> 
> And if we introduce such a flag, then I think we can make the *default* 
> (in the absense of an explicit flag) be something like
> 
> 	if (no_edit)
> 		default = touch_up_just_whitespace;
> 	else
> 		default = whole_enchilada
> 
> and obviously it could also be a configuration option.

which makes it four-way choice...

> That way, you could always get the existing behaviour with
> 
> 	git commit --verbatim=full-enchilada -F my_message
> 
> (yeah, bad name - I'm not seriously suggesting it be called 
> "full-enchilada", and I'm also not sure the flag should be called 
> "--verbatim" any more if it has choices, it's more likely that we should 
> call it something like "--cleanup={verbatim,whitespace,strip}" or 
> something like that.

"verbatim", "whitespace", "strip", "default".

The patch is on top of my previos patches. Junio, if you wish, I can
squash and resend.

 Documentation/git-commit.txt |   15 ++++++---
 builtin-commit.c             |   69 +++++++++++++++++++++++++++++-------------
 t/t7502-commit.sh            |   28 +++++++++++++++--
 3 files changed, 83 insertions(+), 29 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 862543f..ac2bfd5 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git-commit' [-a | --interactive] [-s] [-v] [-u]
 	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
 	   [--allow-empty] [--no-verify] [-e] [--author <author>]
-	   [--verbatim] [--] [[-i | -o ]<file>...]
+	   [--cleanup=<mode>] [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -95,10 +95,15 @@ OPTIONS
 	from making such a commit.  This option bypasses the safety, and
 	is primarily for use by foreign scm interface scripts.
 
---verbatim::
-	Inhibits stripping of leading and trailing spaces,
-	empty lines and #commentary from the commit message.
-	Implies --allow-empty.
+--cleanup=<mode>::
+	This option sets how the commit message is cleaned up.
+	The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
+	and 'default'. The 'default' mode will strip leading and
+	trailing empty lines and #commentary from the commit message
+	only if the message is to be edited. Otherwise only whitespace
+	removed. The 'verbatim' mode wont change message at all,
+	'whitespace' removes just leading/trailing whitespace lines
+	and 'strip' removes both whitespace and commentary.
 
 -e|--edit::
 	The message taken from file with `-F`, command line with
diff --git a/builtin-commit.c b/builtin-commit.c
index eae7661..6f98537 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -47,7 +47,20 @@ static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify, allow_empty;
-static int verbatim_message;
+/*
+ * The default commit message cleanup mode will remove the lines
+ * beginning with # (shell comments) and leading and trailing
+ * whitespaces (empty lines or containing only whitespaces)
+ * if editor is used, and only the whitespaces if the message
+ * is specified explicitly.
+ */
+static enum {
+	CLEANUP_DEFAULT,
+	CLEANUP_NONE,
+	CLEANUP_SPACE,
+	CLEANUP_ALL,
+} cleanup_mode;
+static char *cleanup_arg;
 
 static int no_edit, initial_commit, in_merge;
 const char *only_include_assumed;
@@ -89,7 +102,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
 	OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
-	OPT_BOOLEAN(0, "verbatim", &verbatim_message, "do not strip spaces and #comments from message"),
+	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
 
 	OPT_END()
 };
@@ -348,7 +361,7 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	if (fp == NULL)
 		die("could not open %s", git_path(commit_editmsg));
 
-	if (!verbatim_message)
+	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, 0);
 
 	if (signoff) {
@@ -397,21 +410,24 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
 	}
 
-	if (in_merge && !no_edit)
-		fprintf(fp,
-			"#\n"
-			"# It looks like you may be committing a MERGE.\n"
-			"# If this is not correct, please remove the file\n"
-			"#	%s\n"
-			"# and try again.\n"
-			"#\n",
-			git_path("MERGE_HEAD"));
-
-	if (!verbatim_message)
-		fprintf(fp,
-			"\n"
-			"# Please enter the commit message for your changes.\n"
-			"# (Comment lines starting with '#' will not be included)\n");
+	if (!no_edit) {
+		if (in_merge)
+			fprintf(fp,
+				"#\n"
+				"# It looks like you may be committing a MERGE.\n"
+				"# If this is not correct, please remove the file\n"
+				"#	%s\n"
+				"# and try again.\n"
+				"#\n",
+				git_path("MERGE_HEAD"));
+		if (cleanup_mode != CLEANUP_NONE)
+			fprintf(fp,
+				"\n"
+				"# Please enter the commit message for your changes.\n");
+		if (cleanup_mode == CLEANUP_DEFAULT)
+			fprintf(fp,
+				"# (Comment lines starting with '#' will not be included)\n");
+	}
 	if (only_include_assumed)
 		fprintf(fp, "# %s\n", only_include_assumed);
 
@@ -435,7 +451,7 @@ static int message_is_empty(struct strbuf *sb, int start)
 	const char *nl;
 	int eol, i;
 
-	if (verbatim_message && sb->len)
+	if (cleanup_mode == CLEANUP_NONE && sb->len)
 		return 0;
 
 	/* See if the template is just a prefix of the message. */
@@ -594,6 +610,16 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		only_include_assumed = "Explicit paths specified without -i nor -o; assuming --only paths...";
 		also = 0;
 	}
+	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
+		cleanup_mode = CLEANUP_DEFAULT;
+	else if (!strcmp(cleanup_arg, "verbatim"))
+		cleanup_mode = CLEANUP_NONE;
+	else if (!strcmp(cleanup_arg, "whitespace"))
+		cleanup_mode = CLEANUP_SPACE;
+	else if (!strcmp(cleanup_arg, "strip"))
+		cleanup_mode = CLEANUP_ALL;
+	else
+		die("Invalid cleanup mode %s", cleanup_arg);
 
 	if (all && argc > 0)
 		die("Paths with -a does not make sense.");
@@ -820,8 +846,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (p != NULL)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 
-	if (!verbatim_message)
-		stripspace(&sb, !no_edit);
+	if (cleanup_mode != CLEANUP_NONE)
+		stripspace(&sb, cleanup_mode == CLEANUP_DEFAULT ?
+			   !no_edit: cleanup_mode == CLEANUP_ALL);
 	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
 		rollback_index_files();
 		die("no commit message?  aborting commit.");
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index d549728..6219378 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -93,15 +93,37 @@ test_expect_success 'verbatim commit messages' '
 
 	echo >>negative &&
 	{ echo;echo "# text";echo; } >expect &&
-	git commit --verbatim -t expect -a &&
+	git commit --cleanup=verbatim -t expect -a &&
 	git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
 	diff -u expect actual &&
 	echo >>negative &&
-	git commit --verbatim -F expect -a &&
+	git commit --cleanup=verbatim -F expect -a &&
 	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
 	diff -u expect actual &&
 	echo >>negative &&
-	git commit --verbatim -m "$(cat expect)" -a &&
+	git commit --cleanup=verbatim -m "$(cat expect)" -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup only whitespace from commit messages' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo; } >text &&
+	echo "# text" >expect &&
+	git commit --cleanup=whitespace -F text -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup commit messages' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo sample;echo; } >text &&
+	echo sample >expect &&
+	git commit --cleanup=strip -F text -a &&
 	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
 	diff -u expect actual
 
-- 
1.5.4.rc1.34.gf21f1

Re: [PATCH] Allow selection of different cleanup modes for commit messages

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> The patch is on top of my previos patches. Junio, if you wish, I can
> squash and resend.

That would be a sane thing to do.

[PATCH] Allow selection of different cleanup modes for commit messages

[Alex Riesen]

Sometimes the message just have to be the way user wants it.
For instance, a template can contain "#" characters, or the message
must be kept as close to its original source as possible for reimport
reasons. Or maybe the user just copied a shell script including its
comments into the commit message for future reference.

The cleanup modes are default, verbatim, whitespace and strip. The
default mode depends on if the message is being edited and will either
strip whitespace and comments (if editor active) or just strip the
whitespace (for where the message is given explicitely).

Suggested by Linus.

Signed-off-by: Alex Riesen <raa@gmail.com>
---

Junio C Hamano, Fri, Dec 21, 2007 22:43:49 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > The patch is on top of my previos patches. Junio, if you wish, I can
> > squash and resend.
> 
> That would be a sane thing to do.

Done

 Documentation/git-commit.txt |   12 +++++++-
 builtin-commit.c             |   69 +++++++++++++++++++++++++++++++----------
 t/t7502-commit.sh            |   40 ++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 4261384..ac2bfd5 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git-commit' [-a | --interactive] [-s] [-v] [-u]
 	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
 	   [--allow-empty] [--no-verify] [-e] [--author <author>]
-	   [--] [[-i | -o ]<file>...]
+	   [--cleanup=<mode>] [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -95,6 +95,16 @@ OPTIONS
 	from making such a commit.  This option bypasses the safety, and
 	is primarily for use by foreign scm interface scripts.
 
+--cleanup=<mode>::
+	This option sets how the commit message is cleaned up.
+	The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
+	and 'default'. The 'default' mode will strip leading and
+	trailing empty lines and #commentary from the commit message
+	only if the message is to be edited. Otherwise only whitespace
+	removed. The 'verbatim' mode wont change message at all,
+	'whitespace' removes just leading/trailing whitespace lines
+	and 'strip' removes both whitespace and commentary.
+
 -e|--edit::
 	The message taken from file with `-F`, command line with
 	`-m`, and from file with `-C` are usually used as the
diff --git a/builtin-commit.c b/builtin-commit.c
index 0a91013..6f98537 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -47,6 +47,20 @@ static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify, allow_empty;
+/*
+ * The default commit message cleanup mode will remove the lines
+ * beginning with # (shell comments) and leading and trailing
+ * whitespaces (empty lines or containing only whitespaces)
+ * if editor is used, and only the whitespaces if the message
+ * is specified explicitly.
+ */
+static enum {
+	CLEANUP_DEFAULT,
+	CLEANUP_NONE,
+	CLEANUP_SPACE,
+	CLEANUP_ALL,
+} cleanup_mode;
+static char *cleanup_arg;
 
 static int no_edit, initial_commit, in_merge;
 const char *only_include_assumed;
@@ -88,6 +102,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
 	OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
+	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
 
 	OPT_END()
 };
@@ -346,7 +361,8 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	if (fp == NULL)
 		die("could not open %s", git_path(commit_editmsg));
 
-	stripspace(&sb, 0);
+	if (cleanup_mode != CLEANUP_NONE)
+		stripspace(&sb, 0);
 
 	if (signoff) {
 		struct strbuf sob;
@@ -394,20 +410,24 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
 	}
 
-	if (in_merge && !no_edit)
-		fprintf(fp,
-			"#\n"
-			"# It looks like you may be committing a MERGE.\n"
-			"# If this is not correct, please remove the file\n"
-			"#	%s\n"
-			"# and try again.\n"
-			"#\n",
-			git_path("MERGE_HEAD"));
-
-	fprintf(fp,
-		"\n"
-		"# Please enter the commit message for your changes.\n"
-		"# (Comment lines starting with '#' will not be included)\n");
+	if (!no_edit) {
+		if (in_merge)
+			fprintf(fp,
+				"#\n"
+				"# It looks like you may be committing a MERGE.\n"
+				"# If this is not correct, please remove the file\n"
+				"#	%s\n"
+				"# and try again.\n"
+				"#\n",
+				git_path("MERGE_HEAD"));
+		if (cleanup_mode != CLEANUP_NONE)
+			fprintf(fp,
+				"\n"
+				"# Please enter the commit message for your changes.\n");
+		if (cleanup_mode == CLEANUP_DEFAULT)
+			fprintf(fp,
+				"# (Comment lines starting with '#' will not be included)\n");
+	}
 	if (only_include_assumed)
 		fprintf(fp, "# %s\n", only_include_assumed);
 
@@ -431,10 +451,13 @@ static int message_is_empty(struct strbuf *sb, int start)
 	const char *nl;
 	int eol, i;
 
+	if (cleanup_mode == CLEANUP_NONE && sb->len)
+		return 0;
+
 	/* See if the template is just a prefix of the message. */
 	strbuf_init(&tmpl, 0);
 	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
-		stripspace(&tmpl, 1);
+		stripspace(&tmpl, !no_edit);
 		if (start + tmpl.len <= sb->len &&
 		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
 			start += tmpl.len;
@@ -587,6 +610,16 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		only_include_assumed = "Explicit paths specified without -i nor -o; assuming --only paths...";
 		also = 0;
 	}
+	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
+		cleanup_mode = CLEANUP_DEFAULT;
+	else if (!strcmp(cleanup_arg, "verbatim"))
+		cleanup_mode = CLEANUP_NONE;
+	else if (!strcmp(cleanup_arg, "whitespace"))
+		cleanup_mode = CLEANUP_SPACE;
+	else if (!strcmp(cleanup_arg, "strip"))
+		cleanup_mode = CLEANUP_ALL;
+	else
+		die("Invalid cleanup mode %s", cleanup_arg);
 
 	if (all && argc > 0)
 		die("Paths with -a does not make sense.");
@@ -813,7 +846,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (p != NULL)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 
-	stripspace(&sb, 1);
+	if (cleanup_mode != CLEANUP_NONE)
+		stripspace(&sb, cleanup_mode == CLEANUP_DEFAULT ?
+			   !no_edit: cleanup_mode == CLEANUP_ALL);
 	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
 		rollback_index_files();
 		die("no commit message?  aborting commit.");
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 21ac785..6219378 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -89,4 +89,44 @@ test_expect_success 'verbose' '
 
 '
 
+test_expect_success 'verbatim commit messages' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo; } >expect &&
+	git commit --cleanup=verbatim -t expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
+	diff -u expect actual &&
+	echo >>negative &&
+	git commit --cleanup=verbatim -F expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual &&
+	echo >>negative &&
+	git commit --cleanup=verbatim -m "$(cat expect)" -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup only whitespace from commit messages' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo; } >text &&
+	echo "# text" >expect &&
+	git commit --cleanup=whitespace -F text -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup commit messages' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo sample;echo; } >text &&
+	echo sample >expect &&
+	git commit --cleanup=strip -F text -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
 test_done
-- 
1.5.4.rc1.36.g24341

Re: [PATCH] Allow selection of different cleanup modes for commit messages

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Junio C Hamano, Fri, Dec 21, 2007 22:43:49 +0100:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>> 
>> > The patch is on top of my previos patches. Junio, if you wish, I can
>> > squash and resend.
>> 
>> That would be a sane thing to do.
>
> Done

Thanks.  I am afraid I have to ask you to either refute my
misunderstanding or a second round.

> +--cleanup=<mode>::
> +	This option sets how the commit message is cleaned up.
> +	The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
> +	and 'default'. The 'default' mode will strip leading and
> +	trailing empty lines and #commentary from the commit message
> +	only if the message is to be edited. Otherwise only whitespace
> +	removed. The 'verbatim' mode wont change message at all,
> +	'whitespace' removes just leading/trailing whitespace lines
> +	and 'strip' removes both whitespace and commentary.

(minor) s/wont/does not/

> @@ -394,20 +410,24 @@ static int prepare_log_message(const char *index_file, const char *prefix)
>  		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
>  	}
>  
> +	if (!no_edit) {
> +		if (in_merge)
> +			fprintf(fp,
> +				"#\n"
> +				"# It looks like you may be committing a MERGE.\n"
> +				"# If this is not correct, please remove the file\n"
> +				"#	%s\n"
> +				"# and try again.\n"
> +				"#\n",
> +				git_path("MERGE_HEAD"));
> +		if (cleanup_mode != CLEANUP_NONE)
> +			fprintf(fp,
> +				"\n"
> +				"# Please enter the commit message for your changes.\n");
> +		if (cleanup_mode == CLEANUP_DEFAULT)
> +			fprintf(fp,
> +				"# (Comment lines starting with '#' will not be included)\n");

Can't cleanup_mode be CLEANUP_ALL at this point, and if so
shouldn't this insn be given as well?

In addition, if:

 - we are talking with editor, and
 - the mode is verbatim, and
 - we added any "#" line ourselves (e.g. in_merge adds the insn
   shown above, or perhaps we added "git status" output),

then perhaps we would want to say that "#" lines need to be
stripped by the user; otherwise it will be left in the commit.

> @@ -431,10 +451,13 @@ static int message_is_empty(struct strbuf *sb, int start)
>  	const char *nl;
>  	int eol, i;
>  
> +	if (cleanup_mode == CLEANUP_NONE && sb->len)
> +		return 0;
> +
>  	/* See if the template is just a prefix of the message. */
>  	strbuf_init(&tmpl, 0);
>  	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
> -		stripspace(&tmpl, 1);
> +		stripspace(&tmpl, !no_edit);

We at this point know that some stripping would happen.  The
template needs to be cleaned the same way as the commit message
was stripped.  Is checking no_edit the right thing to do?  What
if cleanup_mode was CLEANUP_ALL and there is no editing going
on?

> @@ -813,7 +846,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	if (p != NULL)
>  		strbuf_setlen(&sb, p - sb.buf + 1);
>  
> -	stripspace(&sb, 1);
> +	if (cleanup_mode != CLEANUP_NONE)
> +		stripspace(&sb, cleanup_mode == CLEANUP_DEFAULT ?
> +			   !no_edit: cleanup_mode == CLEANUP_ALL);

When writing such a long ? : expression, laying it out like:

	condition
        ? if-true
        : if-false

would be easier to read.  You tilt your head sideways 90
degrees, just like you read a smiley ;-), and you will see the
parse tree.

But I suspect, assuming that the two issues around CLEANUP_DEFAULT vs
CLEANUP_ALL I mentioned above are indeed bugs, it may be cleaner
to:

  - parse the options; you get one of NONE/DEFAULT/SPACE/ALL

  - convert DEFAULT to SPACE or ALL if editor is in effect.

  - do not worry about no_editor or DEFAULT in the rest of the
    code when calling stripspace().  The only values you will
    see are NONE, SPACE or ALL.

> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 21ac785..6219378 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -89,4 +89,44 @@ test_expect_success 'verbose' '
>  
>  '
>  
> +test_expect_success 'verbatim commit messages' '
> +

These tests check too many things at once, and it would be very
hard to diagnose when breakage does happen.  Please split them
perhaps into three separate tests like this:

> +	echo >>negative &&
> +	{ echo;echo "# text";echo; } >expect &&
> +	git commit --cleanup=verbatim -t expect -a &&
> +	git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
> +	diff -u expect actual &&

This is one test --- making sure the commit body match the
unedited template verbatim, except the status part to be shown
in the editor.  But we are not using editor in this test and I
thought the point of this --cleanup=verbatim exercise was that
the "status" part is not even have to be added to the commit log
message when editor is not in effect...  Isn't the "head -n 3"
just sweeping a bug under the rug?

> +	echo >>negative &&
> +	git commit --cleanup=verbatim -F expect -a &&
> +	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> +	diff -u expect actual &&

And this is another test.

> +	echo >>negative &&
> +	git commit --cleanup=verbatim -m "$(cat expect)" -a &&
> +	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> +	diff -u expect actual

And this is yet another.

> +'

Also, the tests do not check "-F file -e", "-m msg -e" etc.  We
would want to make sure the right insn are shown to the user in
the editor, wouldn't we?

[PATCH] Allow selection of different cleanup modes for commit messages

[Alex Riesen]

Sometimes the message just have to be the way user wants it.
For instance, a template can contain "#" characters, or the message
must be kept as close to its original source as possible for reimport
reasons. Or maybe the user just copied a shell script including its
comments into the commit message for future reference.

The cleanup modes are default, verbatim, whitespace and strip. The
default mode depends on if the message is being edited and will either
strip whitespace and comments (if editor active) or just strip the
whitespace (for where the message is given explicitely).

Suggested by Linus.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Junio C Hamano, Sat, Dec 22, 2007 08:59:34 +0100:
> > +	removed. The 'verbatim' mode wont change message at all,
> > +	'whitespace' removes just leading/trailing whitespace lines
> > +	and 'strip' removes both whitespace and commentary.
> 
> (minor) s/wont/does not/
> 

Done.

> > +		if (cleanup_mode == CLEANUP_DEFAULT)
> > +			fprintf(fp,
> > +				"# (Comment lines starting with '#' will not be included)\n");
> 
> Can't cleanup_mode be CLEANUP_ALL at this point, and if so
> shouldn't this insn be given as well?

Rewritten as suggested (set cleanup_mode depending on no_edit in
parse_and_validate_options).

> In addition, if:
> 
>  - we are talking with editor, and
>  - the mode is verbatim, and
>  - we added any "#" line ourselves (e.g. in_merge adds the insn
>    shown above, or perhaps we added "git status" output),
> 
> then perhaps we would want to say that "#" lines need to be
> stripped by the user; otherwise it will be left in the commit.

Well, I don't know. I find these click-through (look-through)
instructions uninteresting, and I sometimes feel tempted to suggest
a config option like "core.autohelp_level" and make it 0 by default.
As soon as someone complains - he'll be told to increase it. The
commit message hints would go at 1. We have plenty of space for
other helpfulness levels in 31 bits.

Seriously though, I agree with Linus: we better just make sure that
whatever was generated by Git never ends up in the commit message. And
make very sure user knows it. Maybe just don't put anything in commit
message? Show the info elsewhere? Go with something like Björns
suggestion?

> >  	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
> > -		stripspace(&tmpl, 1);
> > +		stripspace(&tmpl, !no_edit);
> 
> We at this point know that some stripping would happen.  The
> template needs to be cleaned the same way as the commit message
> was stripped.  Is checking no_edit the right thing to do?  What
> if cleanup_mode was CLEANUP_ALL and there is no editing going
> on?

The cleanup_mode initialization rewrite took care of this as well

> > @@ -813,7 +846,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> >  	if (p != NULL)
> >  		strbuf_setlen(&sb, p - sb.buf + 1);
> >  
> > -	stripspace(&sb, 1);
> > +	if (cleanup_mode != CLEANUP_NONE)
> > +		stripspace(&sb, cleanup_mode == CLEANUP_DEFAULT ?
> > +			   !no_edit: cleanup_mode == CLEANUP_ALL);
> 
> When writing such a long ? : expression, laying it out like:
> 
> 	condition
>         ? if-true
>         : if-false
> 
> would be easier to read.  You tilt your head sideways 90
> degrees, just like you read a smiley ;-), and you will see the
> parse tree.

I just tried it. Still don't know if I like it :)
Sadly, the mentioned rewrite removed the need for it.

> But I suspect, assuming that the two issues around CLEANUP_DEFAULT vs
> CLEANUP_ALL I mentioned above are indeed bugs, it may be cleaner
> to:
> 
>   - parse the options; you get one of NONE/DEFAULT/SPACE/ALL
> 
>   - convert DEFAULT to SPACE or ALL if editor is in effect.
> 
>   - do not worry about no_editor or DEFAULT in the rest of the
>     code when calling stripspace().  The only values you will
>     see are NONE, SPACE or ALL.

Well, why didn't you said this at start?! :)

> > +test_expect_success 'verbatim commit messages' '
> > +
> 
> These tests check too many things at once, and it would be very
> hard to diagnose when breakage does happen.  Please split them
> perhaps into three separate tests like this:

Done

> > +'
> 
> Also, the tests do not check "-F file -e", "-m msg -e" etc.  We
> would want to make sure the right insn are shown to the user in
> the editor, wouldn't we?

As I am not convinced whether they are needed at all, I just added a
placeholder which expects what we have today.

 Documentation/git-commit.txt |   12 ++++++-
 builtin-commit.c             |   72 ++++++++++++++++++++++++++++++-----------
 t/t7502-commit.sh            |   65 +++++++++++++++++++++++++++++++++++++
 3 files changed, 128 insertions(+), 21 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 4261384..96383b6 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git-commit' [-a | --interactive] [-s] [-v] [-u]
 	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
 	   [--allow-empty] [--no-verify] [-e] [--author <author>]
-	   [--] [[-i | -o ]<file>...]
+	   [--cleanup=<mode>] [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -95,6 +95,16 @@ OPTIONS
 	from making such a commit.  This option bypasses the safety, and
 	is primarily for use by foreign scm interface scripts.
 
+--cleanup=<mode>::
+	This option sets how the commit message is cleaned up.
+	The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
+	and 'default'. The 'default' mode will strip leading and
+	trailing empty lines and #commentary from the commit message
+	only if the message is to be edited. Otherwise only whitespace
+	removed. The 'verbatim' mode does not change message at all,
+	'whitespace' removes just leading/trailing whitespace lines
+	and 'strip' removes both whitespace and commentary.
+
 -e|--edit::
 	The message taken from file with `-F`, command line with
 	`-m`, and from file with `-C` are usually used as the
diff --git a/builtin-commit.c b/builtin-commit.c
index 96410de..34c5fd2 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -47,6 +47,19 @@ static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;
 static int quiet, verbose, untracked_files, no_verify, allow_empty;
+/*
+ * The default commit message cleanup mode will remove the lines
+ * beginning with # (shell comments) and leading and trailing
+ * whitespaces (empty lines or containing only whitespaces)
+ * if editor is used, and only the whitespaces if the message
+ * is specified explicitly.
+ */
+static enum {
+	CLEANUP_SPACE,
+	CLEANUP_NONE,
+	CLEANUP_ALL,
+} cleanup_mode;
+static char *cleanup_arg;
 
 static int no_edit, initial_commit, in_merge;
 const char *only_include_assumed;
@@ -88,6 +101,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
 	OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
+	OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
 
 	OPT_END()
 };
@@ -346,7 +360,8 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	if (fp == NULL)
 		die("could not open %s", git_path(commit_editmsg));
 
-	stripspace(&sb, 0);
+	if (cleanup_mode != CLEANUP_NONE)
+		stripspace(&sb, 0);
 
 	if (signoff) {
 		struct strbuf sob;
@@ -398,23 +413,26 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
 	}
 
-	if (in_merge && !no_edit)
-		fprintf(fp,
-			"#\n"
-			"# It looks like you may be committing a MERGE.\n"
-			"# If this is not correct, please remove the file\n"
-			"#	%s\n"
-			"# and try again.\n"
-			"#\n",
-			git_path("MERGE_HEAD"));
-
-	fprintf(fp,
-		"\n"
-		"# Please enter the commit message for your changes.\n"
-		"# (Comment lines starting with '#' will not be included)\n");
-	if (only_include_assumed)
-		fprintf(fp, "# %s\n", only_include_assumed);
-
+	if (!no_edit) {
+		if (in_merge)
+			fprintf(fp,
+				"#\n"
+				"# It looks like you may be committing a MERGE.\n"
+				"# If this is not correct, please remove the file\n"
+				"#	%s\n"
+				"# and try again.\n"
+				"#\n",
+				git_path("MERGE_HEAD"));
+		if (cleanup_mode != CLEANUP_NONE)
+			fprintf(fp,
+				"\n"
+				"# Please enter the commit message for your changes.\n");
+		if (cleanup_mode == CLEANUP_ALL)
+			fprintf(fp,
+				"# (Comment lines starting with '#' will not be included)\n");
+		if (only_include_assumed)
+			fprintf(fp, "# %s\n", only_include_assumed);
+	}
 	saved_color_setting = wt_status_use_color;
 	wt_status_use_color = 0;
 	commitable = run_status(fp, index_file, prefix, 1);
@@ -435,10 +453,13 @@ static int message_is_empty(struct strbuf *sb, int start)
 	const char *nl;
 	int eol, i;
 
+	if (cleanup_mode == CLEANUP_NONE && sb->len)
+		return 0;
+
 	/* See if the template is just a prefix of the message. */
 	strbuf_init(&tmpl, 0);
 	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
-		stripspace(&tmpl, 1);
+		stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);
 		if (start + tmpl.len <= sb->len &&
 		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
 			start += tmpl.len;
@@ -591,6 +612,16 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		only_include_assumed = "Explicit paths specified without -i nor -o; assuming --only paths...";
 		also = 0;
 	}
+	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
+		cleanup_mode = no_edit ? CLEANUP_SPACE: CLEANUP_ALL;
+	else if (!strcmp(cleanup_arg, "verbatim"))
+		cleanup_mode = CLEANUP_NONE;
+	else if (!strcmp(cleanup_arg, "whitespace"))
+		cleanup_mode = CLEANUP_SPACE;
+	else if (!strcmp(cleanup_arg, "strip"))
+		cleanup_mode = CLEANUP_ALL;
+	else
+		die("Invalid cleanup mode %s", cleanup_arg);
 
 	if (all && argc > 0)
 		die("Paths with -a does not make sense.");
@@ -817,7 +848,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (p != NULL)
 		strbuf_setlen(&sb, p - sb.buf + 1);
 
-	stripspace(&sb, 1);
+	if (cleanup_mode != CLEANUP_NONE)
+		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
 	if (sb.len < header_len || message_is_empty(&sb, header_len)) {
 		rollback_index_files();
 		die("no commit message?  aborting commit.");
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 21ac785..aaf497e 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -89,4 +89,69 @@ test_expect_success 'verbose' '
 
 '
 
+test_expect_success 'cleanup commit messages (verbatim,-t)' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo; } >expect &&
+	git commit --cleanup=verbatim -t expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup commit messages (verbatim,-F)' '
+
+	echo >>negative &&
+	git commit --cleanup=verbatim -F expect -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup commit messages (verbatim,-m)' '
+
+	echo >>negative &&
+	git commit --cleanup=verbatim -m "$(cat expect)" -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup commit messages (whitespace,-F)' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo; } >text &&
+	echo "# text" >expect &&
+	git commit --cleanup=whitespace -F text -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cleanup commit messages (strip,-F)' '
+
+	echo >>negative &&
+	{ echo;echo "# text";echo sample;echo; } >text &&
+	echo sample >expect &&
+	git commit --cleanup=strip -F text -a &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	diff -u expect actual
+
+'
+
+echo "sample
+
+# Please enter the commit message for your changes.
+# (Comment lines starting with '#' will not be included)" >expect
+
+test_expect_success 'cleanup commit messages (strip,-F,-e)' '
+
+	echo >>negative &&
+	{ echo;echo sample;echo; } >text &&
+	git commit -e -F text -a &&
+	head -n 4 .git/COMMIT_EDITMSG >actual &&
+	diff -u expect actual
+
+'
+
 test_done
-- 
1.5.4.rc1.48.gb232

Re: [PATCH] Allow selection of different cleanup modes for commit messages

[Linus Torvalds]

On Sat, 22 Dec 2007, Alex Riesen wrote:
>
> +	if (!no_edit) {

This is unrelated to the rest of the patch, but I do think double 
negations are horrible, so I thoink we should probably make the "no_edit" 
flag change meaning, and call it "run_editor" or something.

That said, the thing that I'm actually reacting to is:

> +		if (in_merge)
> +			fprintf(fp,
> +				"#\n"
> +				"# It looks like you may be committing a MERGE.\n"
> +				"# If this is not correct, please remove the file\n"
> +				"#	%s\n"
> +				"# and try again.\n"
> +				"#\n",
> +				git_path("MERGE_HEAD"));
> +		if (cleanup_mode != CLEANUP_NONE)
> +			fprintf(fp,
> +				"\n"
> +				"# Please enter the commit message for your changes.\n");
> +		if (cleanup_mode == CLEANUP_ALL)
> +			fprintf(fp,
> +				"# (Comment lines starting with '#' will not be included)\n");
> +		if (only_include_assumed)
> +			fprintf(fp, "# %s\n", only_include_assumed);

The thing that still worries me about this set is that if you have 
"cleanup_mode == CLEANUP_NONE" or "cleanup_mode == CLEANUP_SPACE", we're 
now adding all these messages, and we depend on the user knowing that *he* 
has to remove them.

So I wonder if we should perhaps:

 (a) Only add these messages at all when we do *not* do CLEANUP_ALL

     In other words, change the

		if (!no_edit) {

     line to a

		if (cleanup_mode == CLEANUP_ALL && !no_edit) {

     instead, or

 (b) Add a a new line to replace he "will not be included" message, ie

		if (cleanup_mode != CLEANUP_ALL)
			fprintf(p,
				"# Please remove these comment lines manually,  "
				"they will not be automatically removed\n");

     or something similar.

I personally would prefer (a) - since anybody who then explicitly uses
--cleanup={space|none} would presumably already know what he is doing.

But this is not a huge deal. Regardless, the patch looks ok, so you can 
add a "Acked-by:" from me.

		Linus

Re: [PATCH] Allow selection of different cleanup modes for commit messages

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sat, 22 Dec 2007, Alex Riesen wrote:
>>
>> +	if (!no_edit) {
>
> This is unrelated to the rest of the patch, but I do think double 
> negations are horrible, so I thoink we should probably make the "no_edit" 
> flag change meaning, and call it "run_editor" or something.
> ...
> So I wonder if we should perhaps:
>
>  (a) Only add these messages at all when we do *not* do CLEANUP_ALL
> ...
>  (b) Add a a new line to replace he "will not be included" message, ie
> ...
> I personally would prefer (a) - since anybody who then explicitly uses
> --cleanup={space|none} would presumably already know what he is doing.
>
> But this is not a huge deal. Regardless, the patch looks ok, so you can 
> add a "Acked-by:" from me.

I was composing essentially the same message, except my
preference was (as you can guess by the fact that I hinted the
additional instruction) (b), but I agree that (a) is better at
least for now because the user has to ask for verbatim every
time (i.e. there is no config).

By the way, the "if (!no_edit)" conditional itself you quoted
above as the first thing in your message is not needed at all.
If no_edit is set, the function already has returned and we
would not reach that point.

So a third round?

Re: [PATCH] Allow selection of different cleanup modes for commit messages

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So I wonder if we should perhaps:
>
>  (a) Only add these messages at all when we do *not* do CLEANUP_ALL
>
> I personally would prefer (a) - since anybody who then explicitly uses
> --cleanup={space|none} would presumably already know what he is doing.

I thought about this a bit more, but then realized that people
would still want to see the status output in their editors.
This applies even to "verbatim" case.

Which means that they need to remove the '#' lines when they do
not use CLEANUP_ALL.

So I am inclined to suggest we need to take (b).

Re: [PATCH] Allow selection of different cleanup modes for commit messages

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> So I wonder if we should perhaps:
>>
>>  (a) Only add these messages at all when we do *not* do CLEANUP_ALL
>>
>> I personally would prefer (a) - since anybody who then explicitly uses
>> --cleanup={space|none} would presumably already know what he is doing.
>
> I thought about this a bit more, but then realized that people
> would still want to see the status output in their editors.
> This applies even to "verbatim" case.
>
> Which means that they need to remove the '#' lines when they do
> not use CLEANUP_ALL.
>
> So I am inclined to suggest we need to take (b).

... and I ended up doing the clean-up myself as I found a
handful buglets in the code before Alex's patch.

Will be posting them shortly.