[PATCH] Teach git-commit about commit message templates.

[PATCH] Teach git-commit about commit message templates.

[Steven Grimm]

These are useful in organizations that enforce particular formats
for commit messages, e.g., to specify bug IDs or test plans.
Use of the template is not enforced; it is simply used as the
initial content when the editor is invoked.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---
	This came up at my company when someone was comparing git to
	svn; we use svn's commit template feature.

	I know we can abuse the pre-commit hook and stick a template in
	the SQUASH_MSG file (thanks to Dscho for that idea on IRC) but
	that has the annoying side effect of using the template as the
	commit message if you change your mind and quit the editor
	without entering anything, rather than aborting the commit.

	I also know that git-commit is being builtin-ified, but as far
	as I know that's still a WIP and this will work today.

	This isn't perfect -- what you'd ideally want is a way to avoid
	using the template for purely local "checkpoint of work in
	progress" commits while still using it for commits you intend to
	publish. But obviously there's no way for git to know which is
	which. For local commits you can easily just empty out the
	editor and type a message from scratch.

	I was surprised there wasn't already a test script for commit,
	but I guess it gets tested implicitly in every other script
	anyway.

 Documentation/git-commit.txt |    8 ++++
 git-commit.sh                |   54 +++++++++++++++++++++--
 t/t7500-commit.sh            |   96 ++++++++++++++++++++++++++++++++++++++++++
 t/t7500/add-comments         |    4 ++
 t/t7500/add-content          |    3 +
 t/t7500/add-signed-off       |    3 +
 6 files changed, 163 insertions(+), 5 deletions(-)
 create mode 100755 t/t7500-commit.sh
 create mode 100755 t/t7500/add-comments
 create mode 100755 t/t7500/add-content
 create mode 100755 t/t7500/add-signed-off

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 8e0e7e2..3f36c67 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -74,6 +74,14 @@ OPTIONS
 -m <msg>|--message=<msg>::
 	Use the given <msg> as the commit message.
 
+-t <file>|--template=<file>::
+	Use the contents of the given file as the initial version
+	of the commit message. The editor is invoked and you can
+	make subsequent changes. If a message is specified using
+	the `-m` or `-F` options, this option has no effect. The
+	template file may also be specified using the `commit.template`
+	configuration variable.
+
 -s|--signoff::
 	Add Signed-off-by line at the end of the commit message.
 
diff --git a/git-commit.sh b/git-commit.sh
index 92749df..4290ae2 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2005 Linus Torvalds
 # Copyright (c) 2006 Junio C Hamano
 
-USAGE='[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [[-i | -o] <path>...]'
+USAGE='[-a | --interactive] [-s] [-v] [--no-verify] [-m <message> | -F <logfile> | (-C|-c) <commit> | --amend] [-u] [-e] [--author <author>] [--template <file>] [[-i | -o] <path>...]'
 SUBDIRECTORY_OK=Yes
 . git-sh-setup
 require_work_tree
@@ -87,6 +87,7 @@ signoff=
 force_author=
 only_include_assumed=
 untracked_files=
+templatefile="`git config commit.template`"
 while case "$#" in 0) break;; esac
 do
 	case "$1" in
@@ -248,6 +249,13 @@ $1"
 		signoff=t
 		shift
 		;;
+	-t|--t|--te|--tem|--temp|--templ|--templa|--templat|--template)
+		case "$#" in 1) usage ;; esac
+		shift
+		templatefile="$1"
+		no_edit=
+		shift
+		;;
 	-q|--q|--qu|--qui|--quie|--quiet)
 		quiet=t
 		shift
@@ -321,6 +329,14 @@ t,,[1-9]*)
 	die "No paths with -i does not make sense." ;;
 esac
 
+if test ! -z "$templatefile" -a -z "$log_given"
+then
+	if test ! -f "$templatefile"
+	then
+		die "Commit template file does not exist."
+	fi
+fi
+
 ################################################################
 # Prepare index to have a tree to be committed
 
@@ -454,6 +470,9 @@ then
 elif test -f "$GIT_DIR/SQUASH_MSG"
 then
 	cat "$GIT_DIR/SQUASH_MSG"
+elif test "$templatefile" != ""
+then
+	cat "$templatefile"
 fi | git stripspace >"$GIT_DIR"/COMMIT_EDITMSG
 
 case "$signoff" in
@@ -572,10 +591,35 @@ else
 fi |
 git stripspace >"$GIT_DIR"/COMMIT_MSG
 
-if cnt=`grep -v -i '^Signed-off-by' "$GIT_DIR"/COMMIT_MSG |
-	git stripspace |
-	wc -l` &&
-   test 0 -lt $cnt
+# Test whether the commit message has any content we didn't supply.
+have_commitmsg=
+grep -v -i '^Signed-off-by' "$GIT_DIR"/COMMIT_MSG |
+	git stripspace > "$GIT_DIR"/COMMIT_BAREMSG
+
+# Is the commit message totally empty?
+if test -s "$GIT_DIR"/COMMIT_BAREMSG
+then
+	if test "$templatefile" != ""
+	then
+		# Test whether this is just the unaltered template.
+		if cnt=`sed -e '/^#/d' < "$templatefile" |
+			git stripspace |
+			diff "$GIT_DIR"/COMMIT_BAREMSG - |
+			wc -l` &&
+		   test 0 -lt $cnt
+		then
+			have_commitmsg=t
+		fi
+	else
+		# No template, so the content in the commit message must
+		# have come from the user.
+		have_commitmsg=t
+	fi
+fi
+
+rm -f "$GIT_DIR"/COMMIT_BAREMSG
+
+if test "$have_commitmsg" = "t"
 then
 	if test -z "$TMP_INDEX"
 	then
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
new file mode 100755
index 0000000..f11ada8
--- /dev/null
+++ b/t/t7500-commit.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Steven Grimm
+#
+
+test_description='git-commit
+
+Tests for selected commit options.'
+
+. ./test-lib.sh
+
+commit_msg_is () {
+	test "`git log --pretty=format:%s%b -1`" = "$1"
+}
+
+# A sanity check to see if commit is working at all.
+test_expect_success 'a basic commit in an empty tree should succeed' '
+	echo content > foo &&
+	git add foo &&
+	git commit -m "initial commit"
+'
+
+test_expect_success 'nonexistent template file should return error' '
+	echo changes >> foo &&
+	git add foo &&
+	! git commit --template "$PWD"/notexist
+'
+
+test_expect_success 'nonexistent template file in config should return error' '
+	git config commit.template "$PWD"/notexist &&
+	! git commit &&
+	git config --unset commit.template
+'
+
+# From now on we'll use a template file that exists.
+TEMPLATE="$PWD"/template
+
+test_expect_success 'unedited template should not commit' '
+	echo "template line" > "$TEMPLATE" &&
+	! git commit --template "$TEMPLATE"
+'
+
+test_expect_success 'unedited template with comments should not commit' '
+	echo "# comment in template" >> "$TEMPLATE" &&
+	! git commit --template "$TEMPLATE"
+'
+
+test_expect_success 'a Signed-off-by line by itself should not commit' '
+	! GIT_EDITOR=../t7500/add-signed-off git commit --template "$TEMPLATE"
+'
+
+test_expect_success 'adding comments to a template should not commit' '
+	! GIT_EDITOR=../t7500/add-comments git commit --template "$TEMPLATE"
+'
+
+test_expect_success 'adding real content to a template should commit' '
+	GIT_EDITOR=../t7500/add-content git commit --template "$TEMPLATE" &&
+	commit_msg_is "template linecommit message"
+'
+
+test_expect_success '-t option should be short for --template' '
+	echo "short template" > "$TEMPLATE" &&
+	echo "new content" >> foo &&
+	git add foo &&
+	GIT_EDITOR=../t7500/add-content git commit -t "$TEMPLATE" &&
+	commit_msg_is "short templatecommit message"
+'
+
+test_expect_success 'config-specified template should commit' '
+	echo "new template" > "$TEMPLATE" &&
+	git config commit.template "$TEMPLATE" &&
+	echo "more content" >> foo &&
+	git add foo &&
+	GIT_EDITOR=../t7500/add-content git commit &&
+	git config --unset commit.template &&
+	commit_msg_is "new templatecommit message"
+'
+
+test_expect_success 'explicit commit message should override template' '
+	echo "still more content" >> foo &&
+	git add foo &&
+	GIT_EDITOR=../t7500/add-content git commit --template "$TEMPLATE" \
+		-m "command line msg" &&
+	commit_msg_is "command line msg<unknown>"
+'
+
+test_expect_success 'commit message from file should override template' '
+	echo "content galore" >> foo &&
+	git add foo &&
+	echo "standard input msg" |
+		GIT_EDITOR=../t7500/add-content git commit \
+			--template "$TEMPLATE" --file - &&
+	commit_msg_is "standard input msg<unknown>"
+'
+
+test_done
diff --git a/t/t7500/add-comments b/t/t7500/add-comments
new file mode 100755
index 0000000..a72e65c
--- /dev/null
+++ b/t/t7500/add-comments
@@ -0,0 +1,4 @@
+#!/bin/sh
+echo "# this is a new comment" >> "$1"
+echo "# and so is this" >> "$1"
+exit 0
diff --git a/t/t7500/add-content b/t/t7500/add-content
new file mode 100755
index 0000000..2fa3d86
--- /dev/null
+++ b/t/t7500/add-content
@@ -0,0 +1,3 @@
+#!/bin/sh
+echo "commit message" >> "$1"
+exit 0
diff --git a/t/t7500/add-signed-off b/t/t7500/add-signed-off
new file mode 100755
index 0000000..e1d856a
--- /dev/null
+++ b/t/t7500/add-signed-off
@@ -0,0 +1,3 @@
+#!/bin/sh
+echo "Signed-off-by: foo <bar@frotz>" >> "$1"
+exit 0
-- 
1.5.3.rc2.4.g726f9

Re: [PATCH] Teach git-commit about commit message templates.

[Johannes Schindelin]

Hi,

On Sun, 22 Jul 2007, Steven Grimm wrote:

> @@ -572,10 +591,35 @@ else
>  fi |
>  git stripspace >"$GIT_DIR"/COMMIT_MSG
>  
> -if cnt=`grep -v -i '^Signed-off-by' "$GIT_DIR"/COMMIT_MSG |
> -	git stripspace |
> -	wc -l` &&
> -   test 0 -lt $cnt
> +# Test whether the commit message has any content we didn't supply.
> +have_commitmsg=
> +grep -v -i '^Signed-off-by' "$GIT_DIR"/COMMIT_MSG |
> +	git stripspace > "$GIT_DIR"/COMMIT_BAREMSG

Up until here, I was with you.  But this feels very wrong.

Why not compare COMMIT_MSG to the templatefile, if there is one?  I.e.

test ! -z "$templatefile" && cmp "$GIT_DIR"/COMMIT_MSG "$templatefile" &&
	die "Unchanged message; will not commit"

Hmm?

Ciao,
Dscho

Re: [PATCH] Teach git-commit about commit message templates.

[Steven Grimm]

Johannes Schindelin wrote:
> Up until here, I was with you.  But this feels very wrong.
>
> Why not compare COMMIT_MSG to the templatefile, if there is one?  I.e.
>
> test ! -z "$templatefile" && cmp "$GIT_DIR"/COMMIT_MSG "$templatefile" &&
> 	die "Unchanged message; will not commit"
>   

The template can itself have comments -- instructions or explanations of 
fields to fill in, for example -- and since comments have been stripped 
from COMMIT_MSG at this point, a comparison against such a template 
would always fail. And, consistent with the current behavior, simply 
adding a Signed-off-by: line shouldn't count as supplying a commit message.

I could do this test before stripping comments from COMMIT_MSG, but then 
I'd still fail the comparison if the user just deleted some comment 
lines manually, which also seems wrong to me -- the comments should be 
totally ignored when doing this comparison, IMO. Plus that wouldn't 
ignore Signed-off-by: lines.

If I'm coming at the design the wrong way, I'm of course happy to adjust 
it, but insensitivity to both comments and Signed-off-by: lines seemed 
like the right behavior from the user's POV to me, and I didn't see a 
cleaner way to do it.

Thanks for looking at the patch!

-Steve

Re: [PATCH] Teach git-commit about commit message templates.

[Johannes Schindelin]

Hi,

On Mon, 23 Jul 2007, Steven Grimm wrote:

> Johannes Schindelin wrote:
> > Up until here, I was with you.  But this feels very wrong.
> > 
> > Why not compare COMMIT_MSG to the templatefile, if there is one?  I.e.
> > 
> > test ! -z "$templatefile" && cmp "$GIT_DIR"/COMMIT_MSG "$templatefile" &&
> > 	die "Unchanged message; will not commit"
> >   
> 
> The template can itself have comments -- instructions or explanations of
> fields to fill in, for example -- and since comments have been stripped 
> from COMMIT_MSG at this point, a comparison against such a template 
> would always fail.

Ah, I missed that.  But IIRC your patch does not wrap that logic behind 
test ! -z "$templatefile", right?  So this is my modified suggestion:

test ! -z "$templatefile" && {
	grep -vie '^Signed-off-by:' < "$GIT_DIR"/COMMIT_MSG" > "$GIT_DIR"/tmp1
	grep -ve '^#' < "$templatefile" > "$GIT_DIR"/tmp1
	trap 'rm "$GIT_DIR"/tmp[12]' 0
	cmp "$GIT_DIR"/tmp[12] &&
	die "Unchanged message; will not commit"
}

Ciao,
Dscho

Re: [PATCH] Teach git-commit about commit message templates.

[Steven Grimm]

Johannes Schindelin wrote:
> Ah, I missed that.  But IIRC your patch does not wrap that logic behind 
> test ! -z "$templatefile", right?  So this is my modified suggestion:
>
> test ! -z "$templatefile" && {
> 	grep -vie '^Signed-off-by:' < "$GIT_DIR"/COMMIT_MSG" > "$GIT_DIR"/tmp1
> 	grep -ve '^#' < "$templatefile" > "$GIT_DIR"/tmp1
> 	trap 'rm "$GIT_DIR"/tmp[12]' 0
> 	cmp "$GIT_DIR"/tmp[12] &&
> 	die "Unchanged message; will not commit"
> }
>   

So you are suggesting I do this in addition to the existing git-commit 
stripping of Signed-off-by: lines? I can certainly do that, but I didn't 
want to make two passes over the commit message doing exactly the same 
stripping.

Hmm, maybe I should outline my understanding of the current (unpatched) 
behavior and what I want it to do. Currently:

* Strip off all comment lines (happens when COMMIT_MSG is created)
* Strip off all Signed-off-by: lines
* Trim whitespace
* If the result has no content (`wc -l` == 0), abort.

With the patch, my intent was:

* Strip off all comment lines
* Strip off all Signed-off-by: lines
* Trim whitespace
* If the result has no content (! -s file), abort.
* If a template file was specified:
   * Strip off all comment and Signed-off-by: lines from the template
   * Trim whitespace from the template
   * If the resulting trimmed template is the same as the trimmed commit 
message, abort.

So I guess before getting to the specifics of the code, I'll ask: does 
the above make sense as a design? I wanted to preserve the existing 
behavior in the absence of a template.

Since the existing code is already stripping Signed-off-by: lines to 
test for a zero-length commit message, I figured I should reuse that 
work. However, it's no big deal to do it twice if people feel that 
results in more readable code -- certainly no human will ever notice the 
time it takes to re-grep the commit message.

-Steve

Re: [PATCH] Teach git-commit about commit message templates.

[Johannes Schindelin]

Hi,

On Mon, 23 Jul 2007, Steven Grimm wrote:

> Johannes Schindelin wrote:
> > Ah, I missed that.  But IIRC your patch does not wrap that logic behind test
> > ! -z "$templatefile", right?  So this is my modified suggestion:
> > 
> > test ! -z "$templatefile" && {
> > 	grep -vie '^Signed-off-by:' < "$GIT_DIR"/COMMIT_MSG" > "$GIT_DIR"/tmp1
> > 	grep -ve '^#' < "$templatefile" > "$GIT_DIR"/tmp1
> > 	trap 'rm "$GIT_DIR"/tmp[12]' 0
> > 	cmp "$GIT_DIR"/tmp[12] &&
> > 	die "Unchanged message; will not commit"
> > }
> >   
> 
> So you are suggesting I do this in addition to the existing git-commit
> stripping of Signed-off-by: lines? I can certainly do that, but I didn't want
> to make two passes over the commit message doing exactly the same stripping.
> 
> Hmm, maybe I should outline my understanding of the current (unpatched)
> behavior and what I want it to do. Currently:
> 
> * Strip off all comment lines (happens when COMMIT_MSG is created)
> * Strip off all Signed-off-by: lines
> * Trim whitespace
> * If the result has no content (`wc -l` == 0), abort.
> 
> With the patch, my intent was:
> 
> * Strip off all comment lines
> * Strip off all Signed-off-by: lines
> * Trim whitespace
> * If the result has no content (! -s file), abort.
> * If a template file was specified:
>   * Strip off all comment and Signed-off-by: lines from the template
>   * Trim whitespace from the template
>   * If the resulting trimmed template is the same as the trimmed commit
> message, abort.
> 
> So I guess before getting to the specifics of the code, I'll ask: does the
> above make sense as a design? I wanted to preserve the existing behavior in
> the absence of a template.

Yes, I think that makes sense.

> Since the existing code is already stripping Signed-off-by: lines to test for
> a zero-length commit message, I figured I should reuse that work. However,
> it's no big deal to do it twice if people feel that results in more readable
> code -- certainly no human will ever notice the time it takes to re-grep the
> commit message.

Okay, but I was really confused by the big if thing.

I'd make sure that COMMIT_BAREMSG is removed with a trap, and really try 
to just enhance the

	if cnt=`grep -v -i '^Signed-off-by' "$GIT_DIR"/COMMIT_MSG |
	        git stripspace |
	        wc -l` &&
	   test 0 -lt $cnt

so that it reads something like

	if test -s "$GIT_DIR"/COMMIT_BAREMSG &&
		test -z "$templatefile" ||
		(grep -ve '^#' < "$templatefile" |
		 git stripspace |
		 git diff --quiet - "$GIT_DIR"/COMMIT_BAREMSG)
	then
		...

(Totally untested, of course.) Hmm?

Ciao,
Dscho

Re: [PATCH] Teach git-commit about commit message templates.

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> With the patch, my intent was:
>
> * Strip off all comment lines
> * Strip off all Signed-off-by: lines
> * Trim whitespace
> * If the result has no content (! -s file), abort.
> * If a template file was specified:
>   * Strip off all comment and Signed-off-by: lines from the template
>   * Trim whitespace from the template
>   * If the resulting trimmed template is the same as the trimmed
> commit message, abort.
>
> So I guess before getting to the specifics of the code, I'll ask: does
> the above make sense as a design? I wanted to preserve the existing
> behavior in the absence of a template.

Offhand, an "interesting" side effect of the above I can see is
that you will cry "wolf" if the only thing the user did is to
add his own Signed-off-by: line ;-)  But that is sane in the
context of coming up with totally new commit log message.

I am more worried about how this should interact with cases
where you usually do not start the log message from scratch.
For example, are there cases / policies where being able to use
templates to leave comments on merge commits are needed?
Squash-commits?  Perhaps "apply this template but only when you
have hand resolved a conflicting merges"?

Or even the case of amending a commit made by somebody else,
without changing the tree contents, in order to make the commit
log message to conform to the company standard?

Re: [PATCH] Teach git-commit about commit message templates.

[Steven Grimm]

Junio C Hamano wrote:
> Offhand, an "interesting" side effect of the above I can see is
> that you will cry "wolf" if the only thing the user did is to
> add his own Signed-off-by: line ;-)  But that is sane in the
> context of coming up with totally new commit log message.
>   

Yes, that is intentional -- the existing git-commit code wasn't treating 
Signed-off-by: as content when it checked for a commit message, so I 
wanted to preserve that behavior.

> I am more worried about how this should interact with cases
> where you usually do not start the log message from scratch.
> For example, are there cases / policies where being able to use
> templates to leave comments on merge commits are needed?
> Squash-commits?  Perhaps "apply this template but only when you
> have hand resolved a conflicting merges"?
>   

Those are all valid cases. They also happen to be ones that are not 
personally useful to me; none of the stuff in my company's commit 
template would really apply to merges. I'm happy to do it if you think 
those cases need to be handled before this can go in, but given there's 
at least one user out there who doesn't need those things, I wonder if 
this makes more sense to hold off on until someone actually asks for it.

> Or even the case of amending a commit made by somebody else,
> without changing the tree contents, in order to make the commit
> log message to conform to the company standard?
>   

It's not clear to me that I can do anything useful in that case, given 
that there's no way to tell in what way the commit message needs to be 
fixed up. If it's a case of, "This engineer committed without using the 
template at all," then I can see an argument for loading the template 
above / below the existing comment, but IMO a more common case will be 
something like, "The format is right, but the bug ID isn't filled in or 
is incorrect," in which case any template I supply would be useless and 
annoying.

Is your hunch about the common case different than mine? Again, I can go 
address this if it'll actually be useful. Maybe by always including the 
template if the command-line option is used, but skipping it in these 
cases if the config option is used? Or something like that? (On the 
other hand, if you have to specify the template on the command line, 
maybe it's easier to just load it into the editor as needed.)

-Steve

Re: [PATCH] Teach git-commit about commit message templates.

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> Junio C Hamano wrote:
> ...
>> I am more worried about how this should interact with cases
>> where you usually do not start the log message from scratch.
>> For example, are there cases / policies where being able to use
>> templates to leave comments on merge commits are needed?
>> Squash-commits?  Perhaps "apply this template but only when you
>> have hand resolved a conflicting merges"?
>>
>
> Those are all valid cases. They also happen to be ones that are not
> personally useful to me; none of the stuff in my company's commit
> template would really apply to merges. I'm happy to do it if you think
> those cases need to be handled before this can go in, but given
> there's at least one user out there who doesn't need those things, I
> wonder if this makes more sense to hold off on until someone actually
> asks for it.
> ...
> Is your hunch about the common case different than mine? Again, I can
> go address this if it'll actually be useful.

No, I am way more cunning and lazy than that.  I did not have
hunch about the common case, so I just had you (and anybody else
who would join the thread) do necessary thinking and guessing
for me ;-)

Re: [PATCH] Teach git-commit about commit message templates.

[Steven Grimm]

Junio C Hamano wrote:
> No, I am way more cunning and lazy than that.  I did not have
> hunch about the common case, so I just had you (and anybody else
> who would join the thread) do necessary thinking and guessing
> for me ;-)
>   

Ah, good. So it sounds like what I've got is at least an okay first cut 
from a functionality point of view. Dscho (or anyone else), any 
objections to the code in v2 of my patch? I believe I addressed all the 
feedback from v1.

-Steve

Re: [PATCH] Teach git-commit about commit message templates.

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> Junio C Hamano wrote:
>> No, I am way more cunning and lazy than that.  I did not have
>> hunch about the common case, so I just had you (and anybody else
>> who would join the thread) do necessary thinking and guessing
>> for me ;-)
>>
>
> Ah, good. So it sounds like what I've got is at least an okay first
> cut from a functionality point of view. Dscho (or anyone else), any
> objections to the code in v2 of my patch? I believe I addressed all
> the feedback from v1.

I have already queued it for 1.5.3-rc3.  Thanks.