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

[Alex Riesen <raa.lkml@gmail.com>]

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