git-commit: add a prepare-commit-msg hook

The prepare-commit-msg hook is run whenever a "fresh" commit message (i.e. not one taken from another commit with -c) is shown in the editor. It can modify the commit message in-place and/or abort the commit.

While the default hook just adds a Signed-Off-By line at the bottom of the commit messsage, the hook is more intended to build a template for the commit message following project standards.

---
 Documentation/git-commit.txt        |   13 ++++++++-----
 Documentation/hooks.txt             |   21 +++++++++++++++++++++
 builtin-commit.c                    |    3 +++
 templates/hooks--commit-msg         |    3 +++
 templates/hooks--prepare-commit-msg |   14 ++++++++++++++
 5 files changed, 49 insertions(+), 5 deletions(-)
 create mode 100644 templates/hooks--prepare-commit-msg

The real motivation for this is building the commit message from GNU ChangeLogs. But I figured that putting it in the real commit message rather than a cover letter would have implied rejection of the patch...

index c3725b2..c68191b 100644
@@ -62,18 +62,21 @@ OPTIONS
 	and the authorship information (including the timestamp)
 	when creating the commit.  With '-C', the editor is not
 	invoked; with '-c' the user can further edit the commit
-	message.
+	message.  In either case, the prepare-commit-msg hook is
+	bypassed.
 
 -F <file>::
 	Take the commit message from the given file.  Use '-' to
-	read the message from the standard input.
+	read the message from the standard input.  This option
+	bypasses the prepare-commit-msg hook.
 
 --author <author>::
 	Override the author name used in the commit.  Use
 	`A U Thor <[email protected]>` format.
 
 -m <msg>|--message=<msg>::
-	Use the given <msg> as the commit message.
+	Use the given <msg> as the commit message.  This option
+	bypasses the prepare-commit-msg hook.
 
 -t <file>|--template=<file>::
 	Use the contents of the given file as the initial version

@@ -280,8 +283,8 @@ order).
 
 HOOKS
 -----
-This command can run `commit-msg`, `pre-commit`, and
-`post-commit` hooks.  See link:hooks.html[hooks] for more
+This command can run `commit-msg`, `prepare-commit-msg`, `pre-commit`,
+and `post-commit` hooks.  See link:hooks.html[hooks] for more
 information.
 
 

index f110162..2ef5567 100644
@@ -61,6 +61,27 @@ The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
 such a line is found.
 
+prepare-commit-msg
+------------------
+
+This hook is invoked by `git-commit` right before starting the editor
+with an empty log message.  It is not executed if the commit message is
+specified otherwise, such as with the `\-m`, `\-F`, `\-c`, `\-C` options.
+It takes a single parameter, the name of the file that holds git's own
+template commit log message.  Exiting with non-zero status causes
+`git-commit` to abort.
+
+The hook is allowed to edit the message file in place, and
+can be used to augment the default commit message with some
+project standard information.  It can also be used for the same
+purpose as the pre-commit message, if the verification has
+to be skipped for automatic commits (e.g. during rebasing).
+
+The default 'prepare-commit-msg' hook adds a Signed-Off-By line
+(doing it with a hook is not necessarily a good idea, but doing
+it in 'commit-msg' is worse because you are not reminded in
+the editor).
+
 commit-msg
 ----------
 

index 0227936..1de0d02 100644
@@ -869,6 +869,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		char index[PATH_MAX];
 		const char *env[2] = { index, NULL };
 		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+		if (!edit_message) {
+			run_hook(index_file, "prepare-commit-msg", git_path(commit_editmsg));
+		}
 		launch_editor(git_path(commit_editmsg), NULL, env);
 	}
 	if (!no_verify &&

index c5cdb9d..4ef86eb 100644
@@ -9,6 +9,9 @@
 # To enable this hook, make this file executable.
 
 # Uncomment the below to add a Signed-off-by line to the message.
+# Doing this in a hook is a bad idea in general, but the prepare-commit-msg
+# hook is more suited to it.
+#
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
 # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
 

new file mode 100644
index 0000000..176283b
@@ -0,0 +1,14 @@
+#!/bin/sh
+#
+# An example hook script to prepare the commit log message.
+# Called by git-commit with one argument, the name of the file
+# that has the commit message.  The hook should exit with non-zero
+# status after issuing an appropriate message if it wants to stop the
+# commit.  The hook is allowed to edit the commit message file.
+#
+# To enable this hook, make this file executable.
+
+# This example adds a Signed-off-by line to the message, that can
+# still be edited.
+SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
+grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"

1.5.3.4.910.gc5122-dirty

Paolo Bonzini, Fri, Jan 18, 2008 15:51:25 +0100:

It is, but quite a powerful one. :-) template-commit-msg?

Paolo

Paolo Bonzini, Fri, Jan 18, 2008 20:46:49 +0100:

Except that "template" is already taken. Someone uses it and some may even got used to "template" having that meaning.

Not really. It will be run even if the template (the one Git have already) is not used.

It really looks a bit complicated. If at all, how about running that "pre-editor" hook with information about where the message comes from?

I.e. if the message comes from a template:

preedit-commit-msg .git/COMMIT_MSG template

or, for a message coming from commit:

    preedit-commit-msg .git/COMMIT_MSG commit 0123456789abdef

or, for a message coming from a file

preedit-commit-msg .git/COMMIT_MSG file user/path/to/file

and finally, for a new message:

preedit-commit-msg .git/COMMIT_MSG

?

I do not particularly like hooks that act before or after an operation is initiated locally, act solely on local data. This is maybe because I still consider git tools building blocks suitable for higher level scripting more than other people do.

There are five valid reasons you might want a hook to a git operation:

(1) A hook that countermands the normal decision made by the underlying command. Examples of this class are the update hook and the pre-commit hook.

(2) A hook that operates on data generated after the command starts to run. The ability to munge the commit log message by the commit-msg hook is an example.

(3) A hook that operates on the remote end of the connection that you may not otherwise have access to other than over the git protocol. An example is the post-update hook.

(4) A hook that runs under a lock that is acquired by the command for mutual exclusion. Currently there is no example, but if we allowed the update hook to modify the commit that was pushed through send-pack => receive-pack pair, which was discussed on the list a while ago, it would be a good example of this.

(5) A hook that is run differently depending on the outcome of the command. The post-merge hook conditionally run by git-pull is an example of this (it is not even run if no merge takes place). Another example is the post-checkout hook that gets information that is otherwise harder to get (namely, if it was a branch checkout or file checkout -- you can figure it out by examining the command line but that already is part of the processing git-checkout does anyway, so no need to force duplicating that code in the userland).

You cannot do an equivalent operation from outside the git command for the above classes of operations. You need hooks for them.

On the other hand, if you want to always cause an action before running a git opeation locally, you do not have to have a hook. You can just prepare such a message based on GNU ChangeLog and then run git-commit with -F, both inside your wrapper.

Of course there can be a very valid exception to the above policy. If it is common enough so that the policy means effectively everybody has to reinvent the same wrapper. But for this particular case I still do not see that is the case.

I see two other possibilities:

1) Would you prefer allowing to run a command by setting commit.template to something starting with an exclamation mark, i.e. something like "!git diff --cached --name-status -r"?

2) The pre-commit could receive a file name on the command line. If it creates that file, it would be used as a template for the commit message. The default implementation of pre-commit could include, commented out, a reimplementation of prepare_log_message. I like this the least.

3) Consider that the patch, as I implemented it, does not act solely on local data, because the index could be modified by the pre-commit hook. This possibility is contemplated explicitly in builtin-commit.c:

/*
* Re-read the index as pre-commit hook could have updated it,
* and write it out as a tree.
*/

Paolo

Hi,

Of course, there is a fourth of "two other" possibilities:

Make a script calling git-commit with "-F - -e" and pipe your generated template into it.

Use this script whenever you want to create a new commit.

Hth,
Dscho

That's what I've done for over a year (http://repo.or.cz/w/svn-wrapper.git -- it started out as a wrapper around SVN but also works fine with Git) and many people also made their own script to achieve something similar (e.g, vc-dwim http://lists.gnu.org/archive/html/bug-gnulib/2007-10/msg00135.html).

Having such a wrapper in Git would just make our life easier.

Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory

(The third was just the previously posted patch, so two "other than the posted one") :-)

You considered that this script should parse -a, -i, -o, whatever, right? ;-)

The point is that a hook can use the index as prepared by git-commit.

Paolo

I think that the approach has one huge advantage. Commands other than "git commit" itself ("git merge", "git rebase", "git am", etc.) do call "git commit" to record the changes they made. I suspect these command would not want this template behaviour, and not adding this custom commit message "feature" to "git commit" would avoid the risk of breaking them.

At the same time, this exact issue could be a drawback. Some of them _might_ want it. But in that case, the the custom template "hook" needs to be told _why_ it is being called, so that it can adjust its behaviour.

My patch does not do that. Of all these commands, only git-rebase uses porcelain git-commit rather than plumbing git-commit-tree, and it uses the -C or -F options (which disable the hook in my patch).

I would like to understand why both of you have not considered the point
that the script would need the updated index from git-commit. Because
short of reusing half of the old Bourne-shell git-commit, I don't see how this would be possible, but maybe there's something obvious. Or maybe we need new plumbing like git-build-index -- then I'd still need to complicate my script to build a template commit message, but I wouldn't need to muddle myself (not too much at least) in handling the command line.

Paolo

Hi,

I have. But I want to avoid a regression at any cost. And your patch just looks to me like it could do that.

But it _has_ been already suggested that you could provide arguments for the existing msg-hook, which would not break anything, since the hook does not get any argument yet, and therefore existing hooks would be unaffected.

Also, the change would be non-intrusive, easy-to-review, and it would not take so much time away from the bug-fixing that we want to do right now in the rc-cycle.

Okay?
Dscho

What kind of regression?

Sure it won't break anything, but it won't work either! The existing message hook runs after the editing session -- I want the hook to introduce text that is merely a suggestion that the user can delete, or a template that the user needs to customize further.

How does adding a new hook affect existing hooks?

Please. That's ludicrous.

My patch is 3 lines of inserted code and 0 modified lines, checking one variable that is set once in builtin-commit.c (edit_message). The documentation says that it runs whenever the editor runs except for -c, and launch_editor runs after the 3 lines of code I inserted.

Seeing how biased you are, I don't really know why I bothered answering you.

That's the first sensible argument that I hear.

Paolo

Hi,

See below.

OMG you're right. But why didn't you say so in the commit message? Something like "This hook complements the commit-msg hook, in that it runs _before_ the editor is launched".

My impression -- even after reading your commit message -- was that it does almost the same as the commit-msg hook, only that it runs _in addition_ to it when doing a non-amend commit.

FWIW this is the first reply by you that uncovers this error of mine.

Actually, after reading the commit message I was in "this-is-not-necessary" mode, and therefore the diffstat looked too large for me.

That is why I thought that a regression was looming somewhere.

And in reality, your patch should be 2 lines of code.

Actually, reading your patch again I think it also triggers for "-c", as well as for "[-C|-F|-m] ... -e".

And it does not necessarily start with "an empty log message"; think of "--signoff".

Besides, I think that the name should be more to the point, something like "pre-edit-commit-msg".

Also, I should think that the hook should get some information about the circumstances (possibly as command line arguments), given that it is called in more cases than you said.

So I think the patch is not ready yet, although I finally got the _point_ of having yet-another hook.

So I am biased. And you have to convince me. It's not that hard to convince me.

Heh. Then I'm happy that I put it into my mail, too!

Ciao,
Dscho

I can't believe this. ;-) At least I'm laughing for something nice now!

You're right, I should have said instead of this:

... just before a "fresh" commit message is shown in the editor.

         ^^^^^^

Yes, but still the diffstat was all .txt files... ;-)

Not for "-c", that's the point of the "edit_message" check. You're right about "-e" though.

Points taken, and patch will be resubmitted after 1.5.4.

Paolo

Thanks. I think the approach is sane, and it naturally falls into the second category of "why we might want to a hook for" list I sent earlier.

I do not think adding "-X makes that hook ignored" for description of every option is warranted, though. Users should not have to be reminded that there is a hook he may never use and it does not trigger if he specifies his own message using -X or -Y or -Z option.

The usage of the hook is optional, and the primary description of what it does and why a user might want to use it should be in its own description (perhaps in hooks.txt and a section that lists hooks in git-commit manpage). As long as that description makes it clear that the hook is a way to specify a dynamic template in a situation that requries a fresh message, the users who are interested in using it would perfectly well understand that options that make git-commit not to take a fresh message would not invoke the hook.

Hi,

Would it not be much better for that hook to verify that the template has not been added?

Or would not be yet even better to use the commit.template config variable, which was intended for that purpose?

Ciao,
Dscho

I fail to parse this.

The template might not necessarily be fixed, for example it could be preloaded with the list of modified files. See the cover letter for an example.

Paolo

Hi,

What I meant is this:

In the message hook, just grep if the template was already added. If it was, just return. If it was not, add it.

No need for yet another hook.

Ciao,
Dscho

Ah, so you want me to always type ":q!" to exit and unnecessarily complicate the commit-msg hook. Cunning, but no, thanks.

I'll make an example. This is my prepare-commit-msg hook:

diff_collect_changelog ()
{
git diff "$@" -- \

   `git diff "$@" --name-status -r | \
        awk '/ChangeLog/ { print substr ($0, 3) }'` | sed -n \
    -e '/^@@/,/^+/ {' \
    -e '  s/^ //p' \
    -e '  t' \
    -e '}' \
    -e '/^diff/,/^@@/ {' \
    -e '  s/^diff --git a\/\(.*\)\/ChangeLog[^ ]* b\/.*/\1:/p' \
    -e '  tdummy' \
    -e '  :dummy' \
    -e '  d' \
    -e '}' \
    -e 's/^+//p' \
    -e 't'
}

diff_collect_changelog --cached > /tmp/foo$$
cat "$GIT_COMMIT_MSG" >> /tmp/foo$$ && \
mv /tmp/foo$$ "$GIT_COMMIT_MSG"
rm -f /tmp/foo$$

or something like that. The alternative I see would be to start the vi editing session with "!!make_changelog --cached". So I thought about having an hook that runs the command for me. Do you have better ideas?

Paolo

Hi,

No, my intention was to avoid complications. Like introducing yet another commit hook. I like clean, elegant semantics. I don't like overbloated semantics. That's why I speak up whenever I fear it is entering git.

Hth,
Dscho

Do you think my three line patch is too complicated? Possibly I was too zealous in documenting the semantics for the new hook and that looked like unelegant semantics.

Do you have another possibility which allows the same workflow (git commit shows the output of that script) within the existing framework?

Paolo

FWIW I'd love such a hook and I've been writing wrappers around Git commit for over a year now to simulate this. I know many people with whom I work that would also love such a feature.

Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory