[PATCH] git-commit: add a prepare-commit-msg hook

[PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

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.

Signed-Off-By: Paolo Bonzini <bonzini@gnu.org>
---
 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...

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index c3725b2..c68191b 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -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 <author@example.com>` 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.


diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index f110162..2ef5567 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -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
 ----------

diff --git a/builtin-commit.c b/builtin-commit.c
index 0227936..1de0d02 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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 &&
diff --git a/templates/hooks--commit-msg b/templates/hooks--commit-msg
index c5cdb9d..4ef86eb 100644
--- a/templates/hooks--commit-msg
+++ b/templates/hooks--commit-msg
@@ -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"

diff --git a/templates/hooks--prepare-commit-msg b/templates/hooks--prepare-commit-msg
new file mode 100644
index 0000000..176283b
--- /dev/null
+++ b/templates/hooks--prepare-commit-msg
@@ -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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Johannes Schindelin]

Hi,

On Fri, 18 Jan 2008, Paolo Bonzini wrote:

> 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.

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

>> 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.
> 
> Would it not be much better for that hook to verify that the template has 
> not been added?

I fail to parse this.

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

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Johannes Schindelin]

Hi,

On Fri, 18 Jan 2008, Paolo Bonzini wrote:

> 
> > > 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.
> > 
> > Would it not be much better for that hook to verify that the template 
> > has not been added?
> 
> I fail to parse this.

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

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

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Johannes Schindelin]

Hi,

On Fri, 18 Jan 2008, Paolo Bonzini wrote:

> 
> > In the message hook, just grep if the template was already added.  If it
> > was, just return.  If it was not, add it.
> 
> Ah, so you want me to always type ":q!" to exit and unnecessarily 
> complicate the commit-msg hook.  Cunning, but no, thanks.

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

> 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.

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Benoit Sigoure]

On Jan 18, 2008, at 7:06 PM, Johannes Schindelin wrote:

> On Fri, 18 Jan 2008, Paolo Bonzini wrote:
>
>>
>>> In the message hook, just grep if the template was already  
>>> added.  If it
>>> was, just return.  If it was not, add it.
>>
>> Ah, so you want me to always type ":q!" to exit and unnecessarily
>> complicate the commit-msg hook.  Cunning, but no, thanks.
>
> 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.


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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Alex Riesen]

Paolo Bonzini, Fri, Jan 18, 2008 15:51:25 +0100:
> +		if (!edit_message) {
> +			run_hook(index_file, "prepare-commit-msg", git_path(commit_editmsg));
> +		}
>  		launch_editor(git_path(commit_editmsg), NULL, env);

"preedit-new-commit-msg", perhaps. A "prepare-" suggests it is called
every time, even if no editor is specified, which it is not.

And it really does look like a template...

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

Alex Riesen wrote:
> Paolo Bonzini, Fri, Jan 18, 2008 15:51:25 +0100:
>> +		if (!edit_message) {
>> +			run_hook(index_file, "prepare-commit-msg", git_path(commit_editmsg));
>> +		}
>>  		launch_editor(git_path(commit_editmsg), NULL, env);
> 
> "preedit-new-commit-msg", perhaps. A "prepare-" suggests it is called
> every time, even if no editor is specified, which it is not.
> 
> And it really does look like a template...

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

Paolo

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Alex Riesen]

Paolo Bonzini, Fri, Jan 18, 2008 20:46:49 +0100:
> Alex Riesen wrote:
>> Paolo Bonzini, Fri, Jan 18, 2008 15:51:25 +0100:
>>> +		if (!edit_message) {
>>> +			run_hook(index_file, "prepare-commit-msg", git_path(commit_editmsg));
>>> +		}
>>>  		launch_editor(git_path(commit_editmsg), NULL, env);
>> "preedit-new-commit-msg", perhaps. A "prepare-" suggests it is called
>> every time, even if no editor is specified, which it is not.
>> And it really does look like a template...
>
> It is, but quite a powerful one. :-)

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

> template-commit-msg?

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

?

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Junio C Hamano]

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.

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

> 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.

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Johannes Schindelin]

Hi,

On Sat, 19 Jan 2008, Paolo Bonzini wrote:

> > 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.
> 
> I see two other possibilities:
> 
> 1) [..]
>
> 2) [..]
>
> 3) [..]

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Benoit Sigoure]

On Jan 19, 2008, at 12:20 PM, Johannes Schindelin wrote:

> On Sat, 19 Jan 2008, Paolo Bonzini wrote:
>
>>> 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.
>>
>> I see two other possibilities:
>>
>> 1) [..]
>>
>> 2) [..]
>>
>> 3) [..]
>
> 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.


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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

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

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

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

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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.

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.

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

> 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.

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).

> 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.

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Paolo Bonzini wrote:

> 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.

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

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

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

What kind of regression?

> But it _has_ been already suggested that you could provide arguments for 
> the existing msg-hook, which would not break anything

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.

> since the hook does 
> not get any argument yet, and therefore existing hooks would be 
> unaffected.

How does adding a new hook affect existing hooks?

> Also, the change would be non-intrusive, easy-to-review

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.

> take so much time away from the bug-fixing that we want to do right now in 

That's the first sensible argument that I hear.

Paolo

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Paolo Bonzini wrote:

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

See below.

> > But it _has_ been already suggested that you could provide arguments 
> > for the existing msg-hook, which would not break anything
> 
> 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.

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".

> > since the hook does not get any argument yet, and therefore existing 
> > hooks would be unaffected.
> 
> How does adding a new hook affect existing hooks?

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.

> > Also, the change would be non-intrusive, easy-to-review
> 
> 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).

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.

> 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.

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.

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

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

> > take so much time away from the bug-fixing that we want to do right 
> > now in
> 
> That's the first sensible argument that I hear.

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

Ciao,
Dscho

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

>> 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.
> 
> 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".

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

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

> 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. 

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

>>> Also, the change would be non-intrusive, easy-to-review
>> 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).
> 
> Actually, after reading the commit message I was in 
> "this-is-not-necessary" mode, and therefore the diffstat looked too large 
> for me.

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

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

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

[PATCH 1/4] git-commit: support variable number of hook arguments

[Paolo Bonzini]

This is a preparatory patch to allow using run_hook for the
prepare-commit-msg hook.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 builtin-commit.c |   59 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 0227936..fc59660 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -339,6 +339,38 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	return s.commitable;
 }
 
+static int run_hook(const char *index_file, const char *name, ...)
+{
+	struct child_process hook;
+	const char *argv[10], *env[2];
+	char index[PATH_MAX];
+	va_list args;
+	int i;
+
+	va_start(args, name);
+	argv[0] = git_path("hooks/%s", name);
+	i = 0;
+	do {
+		argv[++i] = va_arg(args, const char *);
+	} while (argv[i]);
+	va_end(args);
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	env[0] = index;
+	env[1] = NULL;
+
+	if (access(argv[0], X_OK) < 0)
+		return 0;
+
+	memset(&hook, 0, sizeof(hook));
+	hook.argv = argv;
+	hook.no_stdin = 1;
+	hook.stdout_to_stderr = 1;
+	hook.env = env;
+
+	return run_command(&hook);
+}
+
 static const char sign_off_header[] = "Signed-off-by: ";
 
 static int prepare_log_message(const char *index_file, const char *prefix)
@@ -673,31 +705,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	return commitable ? 0 : 1;
 }
 
-static int run_hook(const char *index_file, const char *name, const char *arg)
-{
-	struct child_process hook;
-	const char *argv[3], *env[2];
-	char index[PATH_MAX];
-
-	argv[0] = git_path("hooks/%s", name);
-	argv[1] = arg;
-	argv[2] = NULL;
-	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-	env[0] = index;
-	env[1] = NULL;
-
-	if (access(argv[0], X_OK) < 0)
-		return 0;
-
-	memset(&hook, 0, sizeof(hook));
-	hook.argv = argv;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.env = env;
-
-	return run_command(&hook);
-}
-
 static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
@@ -872,7 +879,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		launch_editor(git_path(commit_editmsg), NULL, env);
 	}
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg))) {
+	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
 		rollback_index_files();
 		exit(1);
 	}

[PATCH 2/4] git-commit: set GIT_EDITOR=: if editor will not be launched

[Paolo Bonzini]

This is a preparatory patch that provides a simple way for the future
prepare-commit-msg hook to discover if the editor will be launched.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 Documentation/hooks.txt |    4 ++++
 builtin-commit.c        |    2 ++
 2 files changed, 6 insertions(+)

diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index f110162..e8d80cf 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -61,6 +61,10 @@ The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
 such a line is found.
 
+All the `git-commit` hooks are invoked with the environment
+variable `GIT_EDITOR=:` if the command will not bring up an editor
+to modify the commit message.
+
 commit-msg
 ----------
 
diff --git a/builtin-commit.c b/builtin-commit.c
index fc59660..955cc84 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -593,6 +593,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (edit_flag)
 		use_editor = 1;
+	if (!use_editor)
+		setenv("GIT_EDITOR", ":", 1);
 
 	if (get_sha1("HEAD", head_sha1))
 		initial_commit = 1;

[PATCH] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

This series of patches adds a prepare-commit-msg hook.
The prepare-commit-msg hook is run whenever a "fresh" commit message
is prepared, just before it is shown in the editor (if it is).
It can modify the commit message in-place and/or abort the commit.
I implemented Alex Riesen's suggestion to tell the hook where the
message came from, and now run the hook even if the editor is not
run.

Patches 1 and 2 are small changes.  Patch 1 changes run_hook to
accept a variable-length NULL-terminated list of arguments.  Patch 2
forces GIT_EDITOR to : if editor will not be launched; this is the
simplest way I found to tell the prepare-commit-msg hook whether
the editor will be launched.

Patch 3 is bigger; it refactors parts of git-commit to do all the
log message processing at the same time.  Currently the message
is prepared soon, but only edited after the first part of the commit
object is prepared.  This simplifies a little the code for part 4.

Part 4 actually adds the hook, including documentation and testcases.
The hook takes two parameters.  The first is the source of the commit
message (detailed more in the commit message and in the docs), which
is either an English word or a commit SHA1.  The second
parameter if the name of the file that the commit log message.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

[PATCH 4/4] git-commit: add a prepare-commit-msg hook

[Paolo Bonzini]

The prepare-commit-msg hook is run whenever a "fresh" commit message
is prepared, just before it is shown in the editor (if it is).
It can modify the commit message in-place and/or abort the commit.

It takes two parameters.  The first is the source of the commit
message, and can be: `message` (if a `\-m` or `\-F` option was
given); `template` (if a `\-t` option was given or the
configuration option `commit.template` is set); `merge` (if the
commit is a merge or a `.git/MERGE_MSG file exists); `squash`
(if a `.git/SQUASH_MSG file exists); or a commit id (if a
`\-c`, `\-C` or `\--amend` option was given).  The second
parameter if the name of the file that the commit log message.

The hook is not suppressed by the `\--no-verify` option.  However,
exiting with non-zero status only aborts the commit if said option
is not given to `git-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, that the user
can then edit or discard altogether.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 Documentation/git-commit.txt        |    4 
 Documentation/hooks.txt             |   33 +++
 builtin-commit.c                    |   21 ++
 t/t7505-prepare-commit-msg-hook.sh  |  303 ++++++++++++++++++++++++++++++++++++
 templates/hooks--commit-msg         |    3 
 templates/hooks--prepare-commit-msg |   14 +
 6 files changed, 375 insertions(+), 3 deletions(-)

	The whole patch series is against the `next' branch.

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index c3725b2..b4ae61f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -280,8 +280,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.
 
 
diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt
index e8d80cf..f25bcd5 100644
--- a/Documentation/hooks.txt
+++ b/Documentation/hooks.txt
@@ -55,7 +55,8 @@ This hook is invoked by `git-commit`, and can be bypassed
 with `\--no-verify` option.  It takes no parameter, and is
 invoked before obtaining the proposed commit log message and
 making a commit.  Exiting with non-zero status from this script
-causes the `git-commit` to abort.
+causes the `git-commit` to abort.  This hook can also modify
+the index.
 
 The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
@@ -65,6 +66,36 @@ All the `git-commit` hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
 to modify the commit message.
 
+prepare-commit-msg
+------------------
+
+This hook is invoked by `git-commit` right after preparing the
+default log message, and before the editor is started.
+
+It takes two parameters.  The first is the source of the commit
+message, and can be: `message` (if a `\-m` or `\-F` option was
+given); `template` (if a `\-t` option was given or the
+configuration option `commit.template` is set); `merge` (if the
+commit is a merge or a `.git/MERGE_MSG file exists); `squash`
+(if a `.git/SQUASH_MSG file exists); or a commit id (if a
+`\-c`, `\-C` or `\--amend` option was given).  The second
+parameter if the name of the file that the commit log message.
+
+The hook is not suppressed by the `\--no-verify` option.  However,
+exiting with non-zero status only aborts the commit if said option
+is not given to `git-commit`.
+
+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
 ----------
 
diff --git a/builtin-commit.c b/builtin-commit.c
index fed549e..1a083f7 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -388,36 +388,49 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 	struct strbuf sb;
 	char *buffer;
 	FILE *fp;
+	const char *hook_arg = NULL;
 
 	strbuf_init(&sb, 0);
 	if (message.len) {
 		strbuf_addbuf(&sb, &message);
+		hook_arg = "message";
 	} else if (logfile && !strcmp(logfile, "-")) {
 		if (isatty(0))
 			fprintf(stderr, "(reading log message from standard input)\n");
 		if (strbuf_read(&sb, 0, 0) < 0)
 			die("could not read log from standard input");
+		hook_arg = "message";
 	} else if (logfile) {
 		if (strbuf_read_file(&sb, logfile, 0) < 0)
 			die("could not read log file '%s': %s",
 			    logfile, strerror(errno));
+		hook_arg = "message";
 	} else if (use_message) {
 		buffer = strstr(use_message_buffer, "\n\n");
 		if (!buffer || buffer[2] == '\0')
 			die("commit has empty message");
 		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
+		hook_arg = use_message;
 	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
 			die("could not read MERGE_MSG: %s", strerror(errno));
+		hook_arg = "merge";
 	} else if (!stat(git_path("SQUASH_MSG"), &statbuf)) {
 		if (strbuf_read_file(&sb, git_path("SQUASH_MSG"), 0) < 0)
 			die("could not read SQUASH_MSG: %s", strerror(errno));
+		hook_arg = "squash";
 	} else if (template_file && !stat(template_file, &statbuf)) {
 		if (strbuf_read_file(&sb, template_file, 0) < 0)
 			die("could not read %s: %s",
 			    template_file, strerror(errno));
+		hook_arg = "template";
 	}
 
+	/* This final case does not modify the template message, it just sets
+	   the argument to the prepare-commit-msg hook.  */
+	else if (in_merge)
+		hook_arg = "merge";
+
 	fp = fopen(git_path(commit_editmsg), "w");
 	if (fp == NULL)
 		die("could not open %s", git_path(commit_editmsg));
@@ -511,6 +524,14 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		return 0;
 	}
 
+	/* Note that we always run the hook, even if no_verify! */
+	if (run_hook(index_file, "prepare-commit-msg",
+		     git_path(commit_editmsg), hook_arg, NULL) &&
+	    !no_verify) {
+		rollback_index_files();
+		return 0;
+	}
+
 	if (use_editor) {
 		char index[PATH_MAX];
 		const char *env[2] = { index, NULL };
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
new file mode 100755
index 0000000..e632bbe
--- /dev/null
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -0,0 +1,303 @@
+#!/bin/sh
+
+test_description='prepare-commit-msg hook'
+
+. ./test-lib.sh
+
+test_expect_success 'with no hook' '
+
+	echo "foo" > file &&
+	git add file &&
+	git commit -m "first"
+
+'
+
+# set up fake editor for interactive editing
+cat > fake-editor <<'EOF'
+#!/bin/sh
+exit 0
+EOF
+chmod +x fake-editor
+FAKE_EDITOR="$(pwd)/fake-editor"
+export FAKE_EDITOR
+
+# now install hook that always succeeds and adds a message
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+HOOK="$HOOKDIR/prepare-commit-msg"
+mkdir -p "$HOOKDIR"
+cat > "$HOOK" <<'EOF'
+#!/bin/sh
+if test "$2" = HEAD; then
+  set "$1" $(git-rev-parse HEAD)
+fi
+if test "$GIT_EDITOR" = :; then
+  sed -e "1s/.*/${2-default} (no editor)/" "$1" > msg.tmp
+else
+  sed -e "1s/.*/${2-default}/" "$1" > msg.tmp
+fi
+mv msg.tmp "$1"
+exit 0
+EOF
+chmod +x "$HOOK"
+
+echo dummy template > "$(git rev-parse --git-dir)/template"
+
+test_expect_success 'with succeeding hook (-m)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -m "more" &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with succeeding hook (-m editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -e -m "more more" &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with succeeding hook (-t)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -t "$(git rev-parse --git-dir)/template" &&
+	test "`git log -1 --pretty=format:%s`" = template
+
+'
+
+test_expect_success 'with succeeding hook (-F)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more | git commit -F -) &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with succeeding hook (-F editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more more | GIT_EDITOR="$FAKE_EDITOR" git commit -e -F -) &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with succeeding hook (-C)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	git commit -C $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head (no editor)"
+
+'
+
+test_expect_success 'with succeeding hook (editor)' '
+
+	echo "more more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit &&
+	test "`git log -1 --pretty=format:%s`" = default
+
+'
+
+test_expect_success 'with succeeding hook (--amend)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --amend &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+test_expect_success 'with succeeding hook (-c)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -c $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+# now a hook that fails
+cat > "$HOOK" << 'EOF'
+#!/bin/sh
+if test "$2" = HEAD; then
+  set "$1" $(git-rev-parse HEAD)
+fi
+if test "$GIT_EDITOR" = :; then
+  sed -e "1s/.*/${2-default} (no editor)/" "$1" > msg.tmp
+else
+  sed -e "1s/.*/${2-default}/" "$1" > msg.tmp
+fi
+mv msg.tmp "$1"
+exit 1
+EOF
+
+test_expect_success 'with failing hook and --no-verify (-m)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit --no-verify -m "more" &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-m editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify -e -m "more more" &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-t)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit --no-verify -t "$(git rev-parse --git-dir)/template" &&
+	test "`git log -1 --pretty=format:%s`" = template
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-F)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more | git commit --no-verify -F -) &&
+	test "`git log -1 --pretty=format:%s`" = "message (no editor)"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-F editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more more | GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify -e -F -) &&
+	test "`git log -1 --pretty=format:%s`" = message
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-C)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	git commit --no-verify -C $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head (no editor)"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (editor)' '
+
+	echo "more more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify &&
+	test "`git log -1 --pretty=format:%s`" = default
+
+'
+
+test_expect_success 'with failing hook and --no-verify (--amend)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify --amend &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+test_expect_success 'with failing hook and --no-verify (-c)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --no-verify -c $head &&
+	test "`git log -1 --pretty=format:%s`" = "$head"
+
+'
+
+test_expect_failure 'with failing hook (-m)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -m "more"
+
+'
+
+test_expect_failure 'with failing hook (-m editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -e -m "more more"
+
+'
+
+test_expect_failure 'with failing hook (-t)' '
+
+	echo "more" >> file &&
+	git add file &&
+	git commit -t "$(git rev-parse --git-dir)/template"
+
+'
+
+test_expect_failure 'with failing hook (-F)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more | git commit -F -)
+
+'
+
+test_expect_failure 'with failing hook (-F editor)' '
+
+	echo "more" >> file &&
+	git add file &&
+	(echo more more | GIT_EDITOR="$FAKE_EDITOR" git commit -e -F -)
+
+'
+
+test_expect_failure 'with failing hook (-C)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	git commit -C $head
+
+'
+
+test_expect_failure 'with failing hook (editor)' '
+
+	echo "more more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit
+
+'
+
+test_expect_failure 'with failing hook (--amend)' '
+
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit --amend
+
+'
+
+test_expect_failure 'with failing hook (-c)' '
+
+	head=`git rev-parse HEAD` &&
+	echo "more" >> file &&
+	git add file &&
+	GIT_EDITOR="$FAKE_EDITOR" git commit -c $head
+
+'
+
+
+test_done
diff --git a/templates/hooks--commit-msg b/templates/hooks--commit-msg
index c5cdb9d..4ef86eb 100644
--- a/templates/hooks--commit-msg
+++ b/templates/hooks--commit-msg
@@ -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"
 
diff --git a/templates/hooks--prepare-commit-msg b/templates/hooks--prepare-commit-msg
new file mode 100644
index 0000000..176283b
--- /dev/null
+++ b/templates/hooks--prepare-commit-msg
@@ -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"

[PATCH 3/4] git-commit: Refactor creation of log message.

[Paolo Bonzini]

This patch moves around some code from run_commit to prepare_log_message.
This simplifies a little the code for the next patch.  The biggest change
is that the editor and the commit-msg hook are invoked before building
the prolog of the commit object.

This means that: 1) the commit may be aborted after editing the message
if there is a problem writing out the tree object (slight disadvantage);
2) no tree will be written out if the commit-msg hook fails (slight
advantage).

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

---
 builtin-commit.c |  114 ++++++++++++++++++++++++++++---------------------------
 1 files changed, 60 insertions(+), 54 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 955cc84..fed549e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -371,6 +371,14 @@ static int run_hook(const char *index_file, const char *name, ...)
 	return run_command(&hook);
 }
 
+static int is_a_merge(const unsigned char *sha1)
+{
+	struct commit *commit = lookup_commit(sha1);
+	if (!commit || parse_commit(commit))
+		die("could not parse HEAD commit");
+	return !!(commit->parents && commit->parents->next);
+}
+
 static const char sign_off_header[] = "Signed-off-by: ";
 
 static int prepare_log_message(const char *index_file, const char *prefix)
@@ -441,13 +449,38 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 
 	strbuf_release(&sb);
 
-	if (!use_editor) {
+	if (use_editor) {
+		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"));
+
+		fprintf(fp,
+			"\n"
+			"# Please enter the commit message for your changes.\n"
+			"# (Comment lines starting with '#' will ");
+		if (cleanup_mode == CLEANUP_ALL)
+			fprintf(fp, "not be included)\n");
+		else /* CLEANUP_SPACE, that is. */
+			fprintf(fp, "be kept.\n"
+				"# You can remove them yourself if you want to)\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);
+		wt_status_use_color = saved_color_setting;
+	} else {
 		struct rev_info rev;
 		unsigned char sha1[20];
 		const char *parent = "HEAD";
 
-		fclose(fp);
-
 		if (!active_nr && read_cache() < 0)
 			die("Cannot read index");
 
@@ -464,39 +499,31 @@ static int prepare_log_message(const char *index_file, const char *prefix)
 		DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
 		run_diff_index(&rev, 1 /* cached */);
 
-		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
+		commitable = !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
 	}
 
-	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"));
-
-	fprintf(fp,
-		"\n"
-		"# Please enter the commit message for your changes.\n"
-		"# (Comment lines starting with '#' will ");
-	if (cleanup_mode == CLEANUP_ALL)
-		fprintf(fp, "not be included)\n");
-	else /* CLEANUP_SPACE, that is. */
-		fprintf(fp, "be kept.\n"
-			"# You can remove them yourself if you want to)\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);
-	wt_status_use_color = saved_color_setting;
-
 	fclose(fp);
 
-	return commitable;
+	if (!commitable && !in_merge && !allow_empty &&
+	    !(amend && is_a_merge(head_sha1))) {
+		run_status(stdout, index_file, prefix, 0);
+		unlink(commit_editmsg);
+		return 0;
+	}
+
+	if (use_editor) {
+		char index[PATH_MAX];
+		const char *env[2] = { index, NULL };
+		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+		launch_editor(git_path(commit_editmsg), NULL, env);
+	}
+
+	if (!no_verify &&
+	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+		return 0;
+	}
+
+	return 1;
 }
 
 /*
@@ -755,14 +782,6 @@ int git_commit_config(const char *k, const char *v)
 	return git_status_config(k, v);
 }
 
-static int is_a_merge(const unsigned char *sha1)
-{
-	struct commit *commit = lookup_commit(sha1);
-	if (!commit || parse_commit(commit))
-		die("could not parse HEAD commit");
-	return !!(commit->parents && commit->parents->next);
-}
-
 static const char commit_utf8_warn[] =
 "Warning: commit message does not conform to UTF-8.\n"
 "You may want to amend it after fixing the message, or set the config\n"
@@ -799,11 +818,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		return 1;
 	}
 
-	if (!prepare_log_message(index_file, prefix) && !in_merge &&
-	    !allow_empty && !(amend && is_a_merge(head_sha1))) {
-		run_status(stdout, index_file, prefix, 0);
+	/* Prepare, let the user edit, and validate the log message.  */
+	if (!prepare_log_message(index_file, prefix)) {
 		rollback_index_files();
-		unlink(commit_editmsg);
 		return 1;
 	}
 
@@ -872,19 +889,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&sb, "encoding %s\n", git_commit_encoding);
 	strbuf_addch(&sb, '\n');
 
-	/* Get the commit message and validate it */
+	/* Finally, get the commit message */
 	header_len = sb.len;
-	if (use_editor) {
-		char index[PATH_MAX];
-		const char *env[2] = { index, NULL };
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		launch_editor(git_path(commit_editmsg), NULL, env);
-	}
-	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
-		rollback_index_files();
-		exit(1);
-	}
 	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
 		rollback_index_files();
 		die("could not read commit message");

Re: [PATCH] git-commit: add a prepare-commit-msg hook

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

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

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.

Re: [PATCH 1/4] git-commit: support variable number of hook arguments

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Paolo Bonzini wrote:

> +static int run_hook(const char *index_file, const char *name, ...)
> +{
> +	struct child_process hook;
> +	const char *argv[10], *env[2];
> +	char index[PATH_MAX];
> +	va_list args;
> +	int i;
> +
> +	va_start(args, name);
> +	argv[0] = git_path("hooks/%s", name);
> +	i = 0;
> +	do {

Here, an

		if (++i >= ARRAY_SIZE(argv))
			die ("run_hook(): too many arguments");

is missing.  Even nicer, you could have

	int argc = 1;
	char **argv = malloc(sizeof(*argv) * 2);

and

		if (++i >= argc)
			argc = ALLOC_GROW(argv, i + 1, argc);

in the loop.  Of course, you would have to change the "++i" to "i" in this 
line:

> +		argv[++i] = va_arg(args, const char *);

Ciao,
Dscho

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Johannes Schindelin]

Hi,

On Mon, 21 Jan 2008, Paolo Bonzini wrote:

> This means that: 1) the commit may be aborted after editing the message
> if there is a problem writing out the tree object (slight disadvantage);

I consider this more than a slight disadvantage.  I regularly take ages 
coming up with a good commit message, because I think that the overall 
time balance is better with me spending more time on the message, but 
every reader spending less time to guess what I meant.

So I would be quite annoyed to edit a message, only to find out that for 
whatever reason the commit was not successful.

Are you _sure_ you need 3/4 for 4/4?

Ciao,
Dscho

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Paolo Bonzini]

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 21 Jan 2008, Paolo Bonzini wrote:
> 
>> This means that: 1) the commit may be aborted after editing the message
>> if there is a problem writing out the tree object (slight disadvantage);
> 
> I consider this more than a slight disadvantage.  I regularly take ages 
> coming up with a good commit message, because I think that the overall 
> time balance is better with me spending more time on the message, but 
> every reader spending less time to guess what I meant.
> 
> So I would be quite annoyed to edit a message, only to find out that for 
> whatever reason the commit was not successful.

Just to make it clearer, the piece of code that would have to fail, for 
the behavior to change, is this:

         discard_cache();
         read_cache_from(index_file);
         if (!active_cache_tree)
                 active_cache_tree = cache_tree();
         if (cache_tree_update(active_cache_tree,
                               active_cache, active_nr, 0, 0) < 0) {
                 rollback_index_files();
                 die("Error building trees");
         }

There are a couple of failure modes hidden in update_one (in cache-tree.c):

         sub = find_subtree(it, path + baselen, entlen, 0);
         if (!sub)
                 die("cache-tree.c: '%.*s' in '%s' not found",
                     entlen, path + baselen, path);

         ...

         if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))
                 return error("invalid object %s", sha1_to_hex(sha1));

but I don't really understand how they could happen.  If you do, I 
appreciate being taught.  :-)

Also note that some problems writing the tree object (I cannot think of 
anything but running out of diskspace -- permission errors could be 
caught creating the index and logmessage too) could also happen writing 
the commit object.  I don't think in practice the disadvantage I 
mentioned can happen.

> Are you _sure_ you need 3/4 for 4/4?

Probably no, but it makes it much easier to avoid duplicated code (all 
the cases are already enumerated in prepare_log_message).  I tried it 
first and did not finish because the code was really really ugly.

Thanks for the review.

Paolo

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> Johannes Schindelin wrote:
>>
>> On Mon, 21 Jan 2008, Paolo Bonzini wrote:
>>
>>> This means that: 1) the commit may be aborted after editing the message
>>> if there is a problem writing out the tree object (slight disadvantage);
>>
>> I consider this more than a slight disadvantage.  I regularly take
>> ages coming up with a good commit message, because I think that the
>> overall time balance is better with me spending more time on the
>> message, but every reader spending less time to guess what I meant.
>>
>> So I would be quite annoyed to edit a message, only to find out that
>> for whatever reason the commit was not successful.
>
> Just to make it clearer, the piece of code that would have to fail,
> for the behavior to change, is this:

I suspect Dscho was worried about the case where he says "git
commit", types message and then write-tree finds out that the
index is still unmerged and the tree cannot be written out.

And I'd be majorly annoyed if the "slight disadvange" was about
that.

>         discard_cache();
>         read_cache_from(index_file);
>         if (!active_cache_tree)
>                 active_cache_tree = cache_tree();
>         if (cache_tree_update(active_cache_tree,
>                               active_cache, active_nr, 0, 0) < 0) {
>                 rollback_index_files();
>                 die("Error building trees");
>         }

I think this _could_ error out if your index is unmerged.

However, if you have other code to error out early upon unmeregd
index before you collect the message from the editor, I think
you are Ok.

Re: [PATCH 4/4] git-commit: add a prepare-commit-msg hook

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> It takes two parameters.  The first is the source of the commit
> message, and can be: `message` (if a `\-m` or `\-F` option was
> given); `template` (if a `\-t` option was given or the
> configuration option `commit.template` is set); `merge` (if the
> commit is a merge or a `.git/MERGE_MSG file exists); `squash`
> (if a `.git/SQUASH_MSG file exists); or a commit id (if a
> `\-c`, `\-C` or `\--amend` option was given).  The second
> parameter if the name of the file that the commit log message.

Please do without funny mark-ups.  The commit log is not
AsciiDoc.

> The hook is not suppressed by the `\--no-verify` option.  However,
> exiting with non-zero status only aborts the commit if said option
> is not given to `git-commit`.

I do not understand why.  I do understand that you do not want
to bypass prepare-commit-msg with or without --no-verify,
because this hook is not about input validation but about input
preparation.  But I do not understand why a failure exit from it
should be treated any differently with or without --no-verify?

If you want to be strict and be safe catching a breakage in the
prepare-commit-msg hook, you should always abort.  You could
also choose to ignore.  In either case, shouldn't the validation
be left to pre-commit hook (for the tree) and commit-msg hook
(for the message, that is left after this hook)?

> While the default hook just adds a Signed-Off-By line at the bottom

It's "s/[OB]/ob/;", and ...

> of the commit messsage, the hook is more intended to build a template
> for the commit message following project standards, that the user
> can then edit or discard altogether.
>
> Signed-off-by: Paolo Bonzini <bonzini@gnu.org>

... you know it ;-)

Re: [PATCH 1/4] git-commit: support variable number of hook arguments

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Mon, 21 Jan 2008, Paolo Bonzini wrote:
>
>> +static int run_hook(const char *index_file, const char *name, ...)
>> +{
>> +	struct child_process hook;
>> +	const char *argv[10], *env[2];
>> +	char index[PATH_MAX];
>> +	va_list args;
>> +	int i;
>> +
>> +	va_start(args, name);
>> +	argv[0] = git_path("hooks/%s", name);
>> +	i = 0;
>> +	do {
>
> Here, an
>
> 		if (++i >= ARRAY_SIZE(argv))
> 			die ("run_hook(): too many arguments");
>
> is missing.  Even nicer, you could have
>
> 	int argc = 1;
> 	char **argv = malloc(sizeof(*argv) * 2);
>
> and
>
> 		if (++i >= argc)
> 			argc = ALLOC_GROW(argv, i + 1, argc);

The sanity check to make sure we do not feed too many is
definitely needed.  For this application I think ALLOC_GROW() is
overkill, as we do not pass unbound number of arguments to the
hook.

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Junio C Hamano]

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

> I suspect Dscho was worried about the case where he says "git
> commit", types message and then write-tree finds out that the
> index is still unmerged and the tree cannot be written out.
>
> And I'd be majorly annoyed if the "slight disadvange" was about
> that.
>
>>         discard_cache();
>>         read_cache_from(index_file);
>>         if (!active_cache_tree)
>>                 active_cache_tree = cache_tree();
>>         if (cache_tree_update(active_cache_tree,
>>                               active_cache, active_nr, 0, 0) < 0) {
>>                 rollback_index_files();
>>                 die("Error building trees");
>>         }
>
> I think this _could_ error out if your index is unmerged.

I just tried.  This would annoy me.

It is common to do something like this:

	$ git cherry-pick -n somethingelse
        $ git add this-and-that-path.c
        $ git commit

to get partial changes from somethingelse for paths you only
care about, and commit the result.  And you often get conflicts
to paths that do not matter (think of backporting trivial part
of fixes).  You need to reset the paths you do not care about
that conflicted, but you can forget that before running "git
commit".

With our "git commit", you first get "foo: unmerged" and you do
not see the editor.  With this change, you edit the message and
then see "foo: ummerged".

Re: [PATCH 3/4] git-commit: Refactor creation of log message.

[Paolo Bonzini]

>> I think this _could_ error out if your index is unmerged.
> 
> I just tried.  This would annoy me.

Right, you beat me to it.  It would annoy me too.

Paolo