Re: [PATCH 3/4] hook: add signature using "interpret-trailers"

Ramsay Jones [off-list ref] writes:

quoted

It works well in all cases except when the user invokes
"git commit" without any arguments. In that case manually
add a new line after the first line to ensure it's consistent
with the output of "-s" option.

Again, s/signature/sign-off/g, or similar (including subject line).

Thanks for being a careful reader.

quoted

Signed-off-by: Kaartic Sivaraam <redacted>
---
 templates/hooks--prepare-commit-msg.sample | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index 708f0e92c..a15d6d634 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample

@@ -32,4 +32,8 @@ SHA1=$3
 # esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$COMMIT_MSG_FILE" || echo "$SOB" >> "$COMMIT_MSG_FILE"
+# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
+# if test -z "$COMMIT_SOURCE"
+# then
+#   @PERL_PATH@ -i.bak -pe 'print "\n" if($first_line++ == 0)' "$COMMIT_MSG_FILE"
+# fi

I think we should do

	print "\n" if !$first_line++

for brevity (and also avoid "if(" that lacks SP) here.

Prepare the 'preare-commit-msg' sample script for
upcoming changes. Preparation includes removal of
an example that has outlived it's purpose. The example
is the one that comments the "Conflicts:" part of a
merge commit message. It isn't relevant anymore as
it's done by default since 261f315b ("merge & sequencer:
turn "Conflicts:" hint into a comment", 2014-08-28).

Further update the relevant comments from the sample script
and update the documentation.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 Documentation/githooks.txt                 |  3 ---
 templates/hooks--prepare-commit-msg.sample | 24 ++++++++++--------------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 706091a56..fdc01aa25 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt

@@ -121,9 +121,6 @@ it is not suppressed by the `--no-verify` option.  A non-zero exit
 means a failure of the hook and aborts the commit.  It should not
 be used as replacement for pre-commit hook.
 
-The sample `prepare-commit-msg` hook that comes with Git comments
-out the `Conflicts:` part of a merge's commit message.
-
 commit-msg
 ~~~~~~~~~~
 

diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index 86b8f227e..279ddc1a7 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample

@@ -9,28 +9,24 @@
 #
 # To enable this hook, rename this file to "prepare-commit-msg".
 
-# This hook includes three examples.  The first comments out the
-# "Conflicts:" part of a merge commit.
+# This hook includes two examples.
 #
-# The second includes the output of "git diff --name-status -r"
+# The first includes the output of "git diff --name-status -r"
 # into the message, just before the "git status" output.  It is
 # commented because it doesn't cope with --amend or with squashed
 # commits.
 #
-# The third example adds a Signed-off-by line to the message, that can
+# The second example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-case "$2,$3" in
-  merge,)
-    @PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' "$1" ;;
 
-# ,|template,)
-#   @PERL_PATH@ -i.bak -pe '
-#      print "\n" . `git diff --cached --name-status -r`
-#	 if /^#/ && $first++ == 0' "$1" ;;
-
-  *) ;;
-esac
+# case "$2,$3" in
+#  ,|template,)
+#    @PERL_PATH@ -i.bak -pe '
+#       print "\n" . `git diff --cached --name-status -r`
+# 	 if /^#/ && $first++ == 0' "$1" ;;
+#  *) ;;
+# esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
 # grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"

-- 
2.13.2.957.g457671ade

The sample hook to prepare the commit message before
a commit allows users to opt-in to add the sign-off
to the commit message. The sign-off is added at a place
that isn't consistent with the "-s" option of "git commit".
Further, it could go out of view in certain cases.

Add the sign-off in a way similar to "-s" option of
"git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes
"git commit" without any arguments. In that case manually
add a new line after the first line to ensure it's consistent
with the output of "-s" option.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 templates/hooks--prepare-commit-msg.sample | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index eb5912163..87d770592 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample

@@ -32,4 +32,8 @@ SHA1=$3
 # esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$COMMIT_MSG_FILE" || echo "$SOB" >> "$COMMIT_MSG_FILE"
+# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
+# if test -z "$COMMIT_SOURCE"
+# then
+#   @PERL_PATH@ -i.bak -pe 'print "\n" if !$first_line++' "$COMMIT_MSG_FILE"
+# fi

-- 
2.13.2.957.g457671ade

It's always nice to have named variables instead of
positional variables as they communicate their purpose
well.

Appropriately name the positional variables of the hook
to make it easier to see what's going on.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 templates/hooks--prepare-commit-msg.sample | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index 279ddc1a7..eb5912163 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample

@@ -19,14 +19,17 @@
 # The second example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
+COMMIT_MSG_FILE=$1
+COMMIT_SOURCE=$2
+SHA1=$3
 
-# case "$2,$3" in
+# case "$COMMIT_SOURCE,$SHA1" in
 #  ,|template,)
 #    @PERL_PATH@ -i.bak -pe '
 #       print "\n" . `git diff --cached --name-status -r`
-# 	 if /^#/ && $first++ == 0' "$1" ;;
+# 	 if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;;
 #  *) ;;
 # esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+# grep -qs "^$SOB" "$COMMIT_MSG_FILE" || echo "$SOB" >> "$COMMIT_MSG_FILE"

-- 
2.13.2.957.g457671ade

Add a simple example that replaces an outdated example
that was removed. This ensures that there's at the least
a simple example that illustrates what could be done
using the hook just by enabling it.

Also, update the documentation.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 Documentation/githooks.txt                 | 3 +++
 templates/hooks--prepare-commit-msg.sample | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index fdc01aa25..59f38efba 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt

@@ -121,6 +121,9 @@ it is not suppressed by the `--no-verify` option.  A non-zero exit
 means a failure of the hook and aborts the commit.  It should not
 be used as replacement for pre-commit hook.
 
+The sample `prepare-commit-msg` hook that comes with Git removes the
+help message found in the commented portion of the commit template.
+
 commit-msg
 ~~~~~~~~~~
 

diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index 87d770592..f0228c1cb 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample

@@ -9,7 +9,8 @@
 #
 # To enable this hook, rename this file to "prepare-commit-msg".
 
-# This hook includes two examples.
+# This hook includes three examples. The first one removes the
+# "# Please enter the commit message..." help message.
 #
 # The first includes the output of "git diff --name-status -r"
 # into the message, just before the "git status" output.  It is

@@ -23,6 +24,8 @@ COMMIT_MSG_FILE=$1
 COMMIT_SOURCE=$2
 SHA1=$3
 
+@PERL_PATH@ -i.bak -ne 'print unless(m/^. Please enter the commit message/..m/^#$/)' "$COMMIT_MSG_FILE"
+
 # case "$COMMIT_SOURCE,$SHA1" in
 #  ,|template,)
 #    @PERL_PATH@ -i.bak -pe '

-- 
2.13.2.957.g457671ade

Add a simple example that replaces an outdated example
that was removed. This ensures that there's at the least
a simple example that illustrates what could be done
using the hook just by enabling it.

Also, update the documentation.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 Documentation/githooks.txt                 | 3 +++
 templates/hooks--prepare-commit-msg.sample | 9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index fdc01aa25..59f38efba 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt

@@ -121,6 +121,9 @@ it is not suppressed by the `--no-verify` option.  A non-zero exit
 means a failure of the hook and aborts the commit.  It should not
 be used as replacement for pre-commit hook.
 
+The sample `prepare-commit-msg` hook that comes with Git removes the
+help message found in the commented portion of the commit template.
+
 commit-msg
 ~~~~~~~~~~
 

diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index 87d770592..dc707e46e 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample

@@ -9,20 +9,23 @@
 #
 # To enable this hook, rename this file to "prepare-commit-msg".
 
-# This hook includes two examples.
+# This hook includes three examples. The first one removes the
+# "# Please enter the commit message..." help message.
 #
-# The first includes the output of "git diff --name-status -r"
+# The second includes the output of "git diff --name-status -r"
 # into the message, just before the "git status" output.  It is
 # commented because it doesn't cope with --amend or with squashed
 # commits.
 #
-# The second example adds a Signed-off-by line to the message, that can
+# The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
 COMMIT_MSG_FILE=$1
 COMMIT_SOURCE=$2
 SHA1=$3
 
+@PERL_PATH@ -i.bak -ne 'print unless(m/^. Please enter the commit message/..m/^#$/)' "$COMMIT_MSG_FILE"
+
 # case "$COMMIT_SOURCE,$SHA1" in
 #  ,|template,)
 #    @PERL_PATH@ -i.bak -pe '

-- 
2.13.2.957.g457671ade

Sign-off added should be that of the "committer" not that of the
"commit's author".

Use the correct logical variable that identifies the committer.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 This fixes a small issue when trying to do the following with the script enabled,

    $ git commit --amend -s

 If the commit being amended was signed off by the commit's author then the above command
 would *append* the sign-off of the committer followed by that of the commit's author.
 That' because the script is invoked only after the sign-off is added by the '-s' option AND
 the default of 'trailer.ifexists' for interpret-trailers currently defaults to the 'addIfDifferentNeighbor'
 thus interpret-trailer fails to identify the existing sign-off of the commit's author and adds it.

 Anyways, it doesn't make sense for a script to add the sign-off of the commit's author. So,
 fixing it seemed correct to me.

 templates/hooks--prepare-commit-msg.sample | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/templates/hooks--prepare-commit-msg.sample b/templates/hooks--prepare-commit-msg.sample
index a84c3e5a8..12dd8fd88 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample

@@ -34,7 +34,7 @@ SHA1=$3
 #  *) ;;
 # esac
 
-# SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
+# SOB=$(git var GIT_COMMITTER_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
 # git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
 # if test -z "$COMMIT_SOURCE"
 # then

-- 
2.14.1.534.g641031ecb

On Mon, Aug 14, 2017 at 1:46 AM, Kaartic Sivaraam
[off-list ref] wrote:

Sign-off added should be that of the "committer" not that of the
"commit's author".

Use the correct logical variable that identifies the committer.

Signed-off-by: Kaartic Sivaraam <redacted>
---
 This fixes a small issue when trying to do the following with the script enabled,

    $ git commit --amend -s

 If the commit being amended was signed off by the commit's author then the above command
 would *append* the sign-off of the committer followed by that of the commit's author.
 That' because the script is invoked only after the sign-off is added by the '-s' option AND
 the default of 'trailer.ifexists' for interpret-trailers currently defaults to the 'addIfDifferentNeighbor'
 thus interpret-trailer fails to identify the existing sign-off of the commit's author and adds it.

The background knowledge provided up to here seems like
a valuable information that we'd want to preserve in the commit
history, i.e. make it part of the commit message?

Code looks good.

Thanks,
Stefan