Re: [PATCH-v3 4/4] git-commit: add a prepare-commit-msg hook
flat view
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:10
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH-v3 4/4] git-commit: add a prepare-commit-msg hook · Paolo Bonzini <hidden>
Paolo Bonzini [off-list ref] writes:
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 SHA1 (if a -c, -C or --amend option was given). The second parameter if the name of the file that the commit log message. If the exit status is non-zero, `git-commit` will abort. The hook is not suppressed by the --no-verify option, and this is the only hook that can abort a commit if --no-verify is given.
I doubt the last two lines should be said. What it says is correct, but that is because we happen to have only two hooks that are not at all about validation (after this patch is applied), and the other one is post-commit that is _defined_ to ignore the error. It should be the norm for hook failure to abort the execution of the main command.
Signed-off-by: Paolo Bonzini <redacted>
Good. The earlier three patches lacked S-o-b.
quoted hunk
diff --git a/Documentation/hooks.txt b/Documentation/hooks.txt index e8d80cf..6df5c42 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
This hunk is a good documentation fix but does not belong to the series.
quoted hunk
@@ -65,6 +66,37 @@ 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 SHA1 (if a +`\-c`, `\-C` or `\--amend` option was given). The second +parameter if the name of the file that the commit log message.
The second parameter "is"??? I suspect "or a commit SHA1" is a wrong interface. Wouldn't it be much easier for the hook to parse its arguments if you give a token "commit" and the commit object name as separate arguments?
quoted hunk
+If the exit status is non-zero, `git-commit` will abort. +The hook is not suppressed by the `\--no-verify` option, and +this is the only hook that can abort a commit if `\--no-verify` +is given.
I'd suggest removing ", and this is.....given".
quoted hunk
+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.
"with some project standard information." Is that something you need to say here? It is allowed to augment the default commit message in _any way_, isn't it?
quoted hunk
+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).
As we seem to have agreed to make its exit status matter, it cannot be used for that. A non-zero exit from the script is an error and not verification failure.
quoted hunk
+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).
I am moderately opposed to call something that blindly adds S-o-b "the default". S-o-b should be a conscious choice. I am Ok with an "example" for people whose work contract says "what I write is always Open Source", but that is far from the norm, so it can hardly be the default nor necessarily is a good example. Perhaps we can add diffstat with '#' prefix as an example, like this, instead? #!/bin/sh # Note that this needs to be tightened to deal with the root # commit sensibly. git diff --name-status HEAD -- >>"$1"
quoted hunk
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"
And this is a good change.